From 7f2f228d00161f2862a713739a58186895b67adb Mon Sep 17 00:00:00 2001 From: nagorny Date: Wed, 12 Aug 2020 14:26:20 +0000 Subject: [PATCH 1/2] explicit try_init instead of lazy initialization --- lib/nvmf/io_pacer.c | 129 ++++++++++++++++++++++++++++++++------------ lib/nvmf/io_pacer.h | 52 +++--------------- lib/nvmf/rdma.c | 1 + 3 files changed, 102 insertions(+), 80 deletions(-) diff --git a/lib/nvmf/io_pacer.c b/lib/nvmf/io_pacer.c index 7969b73fe68..b065b699973 100644 --- a/lib/nvmf/io_pacer.c +++ b/lib/nvmf/io_pacer.c @@ -241,40 +241,6 @@ spdk_io_pacer_destroy(struct spdk_io_pacer *pacer) SPDK_NOTICELOG("Destroyed IO pacer %p\n", pacer); } -void spdk_io_pacer_drive_stats_setup(struct spdk_io_pacer_drives_stats *stats, int32_t entries) -{ - struct rte_hash_parameters hash_params = { - .name = "DRIVE_STATS", - .entries = entries, - .key_len = sizeof(uint64_t), - .socket_id = rte_socket_id(), - .hash_func = rte_jhash, - .hash_func_init_val = 0, - }; - struct rte_hash *h = NULL; - - if (stats->h != NULL) { - return; - } - - rte_spinlock_lock(&drives_stats_create_lock); - if (stats->h != NULL) { - goto exit; - } - - h = rte_hash_create(&hash_params); - if (h == NULL) { - SPDK_ERRLOG("IO pacer can't create drive statistics dict\n"); - } - - stats->h = h; - rte_spinlock_init(&stats->lock); - SPDK_NOTICELOG("Drives stats setup done\n"); - - exit: - rte_spinlock_unlock(&drives_stats_create_lock); -} - int spdk_io_pacer_create_queue(struct spdk_io_pacer *pacer, uint64_t key) { @@ -300,7 +266,6 @@ spdk_io_pacer_create_queue(struct spdk_io_pacer *pacer, uint64_t key) pacer->queues[pacer->num_queues].key = key; STAILQ_INIT(&pacer->queues[pacer->num_queues].queue); - spdk_io_pacer_drive_stats_setup(&drives_stats, MAX_DRIVES_STATS); pacer->queues[pacer->num_queues].stats = spdk_io_pacer_drive_stats_get(&drives_stats, key); pacer->num_queues++; SPDK_NOTICELOG("Created IO pacer queue: pacer %p, key %016lx\n", @@ -574,3 +539,97 @@ spdk_io_pacer_tuner2_sub(struct spdk_io_pacer_tuner2 *tuner, uint32_t value) assert(tuner != NULL); tuner->value -= value; } + +struct drive_stats* spdk_io_pacer_drive_stats_create(struct spdk_io_pacer_drives_stats *stats, + uint64_t key, + struct spdk_bdev *bdev) +{ + int32_t ret = 0; + struct drive_stats *data = NULL; + struct rte_hash *h = stats->h; + + ret = rte_hash_lookup(h, &key); + if (ret != -ENOENT) + return 0; + + drive_stats_lock(stats); + data = calloc(1, sizeof(struct drive_stats)); + rte_atomic32_init(&data->ops_in_flight); + /* FIXME just workaround to test */ + data->bdev = bdev; + ret = rte_hash_add_key_data(h, (void *) &key, data); + if (ret < 0) { + SPDK_ERRLOG("Can't add key to drive statistics dict: %" PRIx64 "\n", + key); + goto err; + } + goto exit; +err: + free(data); + data = NULL; +exit: + drive_stats_unlock(stats); + return data; +} + +struct drive_stats * spdk_io_pacer_drive_stats_get(struct spdk_io_pacer_drives_stats *stats, + uint64_t key) +{ + struct drive_stats *data = NULL; + int ret = 0; + ret = rte_hash_lookup_data(stats->h, (void*) &key, (void**) &data); + if (unlikely(ret < 0)) { + SPDK_ERRLOG("Drive statistics seems broken\n"); + } + return data; +} + +static inline void spdk_io_pacer_drive_stats_setup(struct spdk_io_pacer_drives_stats *stats, int32_t entries) +{ + struct rte_hash_parameters hash_params = { + .name = "DRIVE_STATS", + .entries = entries, + .key_len = sizeof(uint64_t), + .socket_id = rte_socket_id(), + .hash_func = rte_jhash, + .hash_func_init_val = 0, + }; + struct rte_hash *h = NULL; + + if (stats->h != NULL) { + return; + } + + rte_spinlock_lock(&drives_stats_create_lock); + if (stats->h != NULL) { + goto exit; + } + + h = rte_hash_create(&hash_params); + if (h == NULL) { + SPDK_ERRLOG("IO pacer can't create drive statistics dict\n"); + } + + stats->h = h; + rte_spinlock_init(&stats->lock); + SPDK_NOTICELOG("Drives stats setup done\n"); + + exit: + rte_spinlock_unlock(&drives_stats_create_lock); +} + +void spdk_io_pacer_drive_stats_try_init(uint64_t key, struct spdk_bdev *bdev) +{ + struct drive_stats *data; + int ret = 0; + + spdk_io_pacer_drive_stats_setup(&drives_stats, MAX_DRIVES_STATS); + ret = rte_hash_lookup_data(drives_stats.h, (void*) &key, (void**) &data); + if (unlikely(ret == -EINVAL)) { + SPDK_ERRLOG("Drive statistics seems broken\n"); + } else if (unlikely(ret == -ENOENT)) { + SPDK_NOTICELOG("Creating drive stats for key: %" PRIx64 "\n", key); + data = spdk_io_pacer_drive_stats_create(&drives_stats, key, bdev); + } +} + diff --git a/lib/nvmf/io_pacer.h b/lib/nvmf/io_pacer.h index d3ad21e1ebb..ec1de9eeb19 100644 --- a/lib/nvmf/io_pacer.h +++ b/lib/nvmf/io_pacer.h @@ -61,6 +61,7 @@ extern struct spdk_io_pacer_drives_stats drives_stats; struct drive_stats { rte_atomic32_t ops_in_flight; + struct spdk_bdev *bdev; }; typedef void (*spdk_io_pacer_pop_cb)(void *io); @@ -89,6 +90,12 @@ void spdk_io_pacer_tuner2_destroy(struct spdk_io_pacer_tuner2 *tuner); void spdk_io_pacer_tuner2_add(struct spdk_io_pacer_tuner2 *tuner, uint32_t value); void spdk_io_pacer_tuner2_sub(struct spdk_io_pacer_tuner2 *tuner, uint32_t value); +struct drive_stats* spdk_io_pacer_drive_stats_create(struct spdk_io_pacer_drives_stats *stats, + uint64_t key, struct spdk_bdev *bdev); +struct drive_stats * spdk_io_pacer_drive_stats_get(struct spdk_io_pacer_drives_stats *stats, + uint64_t key); +void spdk_io_pacer_drive_stats_try_init(uint64_t key, struct spdk_bdev *bdev); + static inline void drive_stats_lock(struct spdk_io_pacer_drives_stats *stats) { rte_spinlock_lock(&stats->lock); } @@ -97,49 +104,6 @@ static inline void drive_stats_unlock(struct spdk_io_pacer_drives_stats *stats) rte_spinlock_unlock(&stats->lock); } -static inline struct drive_stats* spdk_io_pacer_drive_stats_create(struct spdk_io_pacer_drives_stats *stats, - uint64_t key) -{ - int32_t ret = 0; - struct drive_stats *data = NULL; - struct rte_hash *h = stats->h; - - ret = rte_hash_lookup(h, &key); - if (ret != -ENOENT) - return 0; - - drive_stats_lock(stats); - data = calloc(1, sizeof(struct drive_stats)); - rte_atomic32_init(&data->ops_in_flight); - ret = rte_hash_add_key_data(h, (void *) &key, data); - if (ret < 0) { - SPDK_ERRLOG("Can't add key to drive statistics dict: %" PRIx64 "\n", key); - goto err; - } - goto exit; -err: - free(data); - data = NULL; -exit: - drive_stats_unlock(stats); - return data; -} - -static inline struct drive_stats * spdk_io_pacer_drive_stats_get(struct spdk_io_pacer_drives_stats *stats, - uint64_t key) -{ - struct drive_stats *data = NULL; - int ret = 0; - ret = rte_hash_lookup_data(stats->h, (void*) &key, (void**) &data); - if (ret == -EINVAL) { - SPDK_ERRLOG("Drive statistics seems broken\n"); - } else if (unlikely(ret == -ENOENT)) { - SPDK_NOTICELOG("Creating drive stats for key: %" PRIx64 "\n", key); - data = spdk_io_pacer_drive_stats_create(stats, key); - } - return data; -} - static inline void spdk_io_pacer_drive_stats_add(struct spdk_io_pacer_drives_stats *stats, uint64_t key, uint32_t val) @@ -156,6 +120,4 @@ static inline void spdk_io_pacer_drive_stats_sub(struct spdk_io_pacer_drives_sta rte_atomic32_sub(&drive_stats->ops_in_flight, val); } -void spdk_io_pacer_drive_stats_setup(struct spdk_io_pacer_drives_stats *stats, int32_t entries); - #endif /* IO_PACER_H */ diff --git a/lib/nvmf/rdma.c b/lib/nvmf/rdma.c index 0b8e03a6178..27ae8109b37 100644 --- a/lib/nvmf/rdma.c +++ b/lib/nvmf/rdma.c @@ -2194,6 +2194,7 @@ spdk_nvmf_rdma_request_process(struct spdk_nvmf_rdma_transport *rtransport, rdma_req->pacer_key = ((uint64_t)rqpair->qpair.ctrlr->subsys->id << 32) + rdma_req->req.cmd->nvme_cmd.nsid; + spdk_io_pacer_drive_stats_try_init(rdma_req->pacer_key, ns->bdev); spdk_io_pacer_push(rgroup->pacer, rdma_req->pacer_key, &rdma_req->pacer_entry); From f81064df686c77b9e06fa2a8e87b23dd3e1aa011 Mon Sep 17 00:00:00 2001 From: nagorny Date: Thu, 13 Aug 2020 08:19:49 +0000 Subject: [PATCH 2/2] race condition fix with double check --- lib/nvmf/io_pacer.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/nvmf/io_pacer.c b/lib/nvmf/io_pacer.c index b065b699973..36bc04456ea 100644 --- a/lib/nvmf/io_pacer.c +++ b/lib/nvmf/io_pacer.c @@ -553,6 +553,11 @@ struct drive_stats* spdk_io_pacer_drive_stats_create(struct spdk_io_pacer_drives return 0; drive_stats_lock(stats); + + ret = rte_hash_lookup(h, &key); + if (ret != -ENOENT) + return 0; + data = calloc(1, sizeof(struct drive_stats)); rte_atomic32_init(&data->ops_in_flight); /* FIXME just workaround to test */