* [dpdk-dev] [PATCH] __rte_hash_add_key_with_hash not releasing multiwriter_lock in failure paths @ 2017-05-26 12:30 mstolarchuk 2017-07-05 14:50 ` De Lara Guarch, Pablo 2017-07-07 5:54 ` [dpdk-dev] [PATCH v2] hash: fix lock release on add Pablo de Lara 0 siblings, 2 replies; 5+ messages in thread From: mstolarchuk @ 2017-05-26 12:30 UTC (permalink / raw) To: bruce.richardson; +Cc: dev Signed-off-by: mstolarchuk <mike.stolarchuk@bigswitch.com> --- lib/librte_hash/rte_cuckoo_hash.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/lib/librte_hash/rte_cuckoo_hash.c b/lib/librte_hash/rte_cuckoo_hash.c index 645c0cf..37a8110 100644 --- a/lib/librte_hash/rte_cuckoo_hash.c +++ b/lib/librte_hash/rte_cuckoo_hash.c @@ -538,8 +538,10 @@ struct rte_hash * n_slots = rte_ring_mc_dequeue_burst(h->free_slots, cached_free_slots->objs, LCORE_CACHE_SIZE, NULL); - if (n_slots == 0) - return -ENOSPC; + if (n_slots == 0) { + ret = -ENOSPC; + goto failure; + } cached_free_slots->len += n_slots; } @@ -548,8 +550,10 @@ struct rte_hash * cached_free_slots->len--; slot_id = cached_free_slots->objs[cached_free_slots->len]; } else { - if (rte_ring_sc_dequeue(h->free_slots, &slot_id) != 0) - return -ENOSPC; + if (rte_ring_sc_dequeue(h->free_slots, &slot_id) != 0) { + ret = -ENOSPC; + goto failure; + } } new_k = RTE_PTR_ADD(keys, (uintptr_t)slot_id * h->key_entry_size); @@ -659,6 +663,7 @@ struct rte_hash * /* Error in addition, store new slot back in the ring and return error */ enqueue_slot_back(h, cached_free_slots, (void *)((uintptr_t) new_idx)); +failure: if (h->add_key == ADD_KEY_MULTIWRITER) rte_spinlock_unlock(h->multiwriter_lock); return ret; -- 1.9.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [dpdk-dev] [PATCH] __rte_hash_add_key_with_hash not releasing multiwriter_lock in failure paths 2017-05-26 12:30 [dpdk-dev] [PATCH] __rte_hash_add_key_with_hash not releasing multiwriter_lock in failure paths mstolarchuk @ 2017-07-05 14:50 ` De Lara Guarch, Pablo 2017-07-07 13:55 ` De Lara Guarch, Pablo 2017-07-07 5:54 ` [dpdk-dev] [PATCH v2] hash: fix lock release on add Pablo de Lara 1 sibling, 1 reply; 5+ messages in thread From: De Lara Guarch, Pablo @ 2017-07-05 14:50 UTC (permalink / raw) To: mstolarchuk, Richardson, Bruce; +Cc: dev > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of mstolarchuk > Sent: Friday, May 26, 2017 1:31 PM > To: bruce.richardson@intel.co > Cc: dev@dpdk.org > Subject: [dpdk-dev] [PATCH] __rte_hash_add_key_with_hash not releasing > multiwriter_lock in failure paths > > Signed-off-by: mstolarchuk <mike.stolarchuk@bigswitch.com> > --- > lib/librte_hash/rte_cuckoo_hash.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/lib/librte_hash/rte_cuckoo_hash.c > b/lib/librte_hash/rte_cuckoo_hash.c > index 645c0cf..37a8110 100644 > --- a/lib/librte_hash/rte_cuckoo_hash.c > +++ b/lib/librte_hash/rte_cuckoo_hash.c > @@ -538,8 +538,10 @@ struct rte_hash * > n_slots = rte_ring_mc_dequeue_burst(h- > >free_slots, > cached_free_slots->objs, > LCORE_CACHE_SIZE, NULL); > - if (n_slots == 0) > - return -ENOSPC; > + if (n_slots == 0) { > + ret = -ENOSPC; > + goto failure; > + } > > cached_free_slots->len += n_slots; > } > @@ -548,8 +550,10 @@ struct rte_hash * > cached_free_slots->len--; > slot_id = cached_free_slots->objs[cached_free_slots->len]; > } else { > - if (rte_ring_sc_dequeue(h->free_slots, &slot_id) != 0) > - return -ENOSPC; > + if (rte_ring_sc_dequeue(h->free_slots, &slot_id) != 0) { > + ret = -ENOSPC; > + goto failure; > + } > } > > new_k = RTE_PTR_ADD(keys, (uintptr_t)slot_id * h- > >key_entry_size); @@ -659,6 +663,7 @@ struct rte_hash * > /* Error in addition, store new slot back in the ring and return error > */ > enqueue_slot_back(h, cached_free_slots, (void *)((uintptr_t) > new_idx)); > > +failure: > if (h->add_key == ADD_KEY_MULTIWRITER) > rte_spinlock_unlock(h->multiwriter_lock); > return ret; > -- > 1.9.1 Hi Mike, Thanks for the patch, it looks correct to me. The code is fine, but the commit message needs some rework. Title should start with: "hash: ...", since this is aiming the hash library, So the title could be: "hash: fix lock release on add" Then, some explanation on what the fix is doing would be good. Lastly, since this is a fix, it requires a fixes line and CC to the stable branch: Fixes: be856325cba3 ("hash: add scalable multi-writer insertion with Intel TSX") CC: stable@dpdk.org Thanks, Pablo ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [dpdk-dev] [PATCH] __rte_hash_add_key_with_hash not releasing multiwriter_lock in failure paths 2017-07-05 14:50 ` De Lara Guarch, Pablo @ 2017-07-07 13:55 ` De Lara Guarch, Pablo 0 siblings, 0 replies; 5+ messages in thread From: De Lara Guarch, Pablo @ 2017-07-07 13:55 UTC (permalink / raw) To: De Lara Guarch, Pablo, mstolarchuk, Richardson, Bruce; +Cc: dev > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of De Lara Guarch, > Pablo > Sent: Wednesday, July 5, 2017 3:51 PM > To: mstolarchuk <mike.stolarchuk@bigswitch.com>; Richardson, Bruce > <bruce.richardson@intel.com> > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH] __rte_hash_add_key_with_hash not > releasing multiwriter_lock in failure paths > > > > > -----Original Message----- > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of mstolarchuk > > Sent: Friday, May 26, 2017 1:31 PM > > To: bruce.richardson@intel.co > > Cc: dev@dpdk.org > > Subject: [dpdk-dev] [PATCH] __rte_hash_add_key_with_hash not > releasing > > multiwriter_lock in failure paths > > > > Signed-off-by: mstolarchuk <mike.stolarchuk@bigswitch.com> > > --- > > lib/librte_hash/rte_cuckoo_hash.c | 13 +++++++++---- > > 1 file changed, 9 insertions(+), 4 deletions(-) > > > > diff --git a/lib/librte_hash/rte_cuckoo_hash.c > > b/lib/librte_hash/rte_cuckoo_hash.c > > index 645c0cf..37a8110 100644 > > --- a/lib/librte_hash/rte_cuckoo_hash.c > > +++ b/lib/librte_hash/rte_cuckoo_hash.c > > @@ -538,8 +538,10 @@ struct rte_hash * > > n_slots = rte_ring_mc_dequeue_burst(h- > > >free_slots, > > cached_free_slots->objs, > > LCORE_CACHE_SIZE, NULL); > > - if (n_slots == 0) > > - return -ENOSPC; > > + if (n_slots == 0) { > > + ret = -ENOSPC; > > + goto failure; > > + } > > > > cached_free_slots->len += n_slots; > > } > > @@ -548,8 +550,10 @@ struct rte_hash * > > cached_free_slots->len--; > > slot_id = cached_free_slots->objs[cached_free_slots->len]; > > } else { > > - if (rte_ring_sc_dequeue(h->free_slots, &slot_id) != 0) > > - return -ENOSPC; > > + if (rte_ring_sc_dequeue(h->free_slots, &slot_id) != 0) { > > + ret = -ENOSPC; > > + goto failure; > > + } > > } > > > > new_k = RTE_PTR_ADD(keys, (uintptr_t)slot_id * h- > > >key_entry_size); @@ -659,6 +663,7 @@ struct rte_hash * > > /* Error in addition, store new slot back in the ring and return > > error */ > > enqueue_slot_back(h, cached_free_slots, (void *)((uintptr_t) > > new_idx)); > > > > +failure: > > if (h->add_key == ADD_KEY_MULTIWRITER) > > rte_spinlock_unlock(h->multiwriter_lock); > > return ret; > > -- > > 1.9.1 > > Hi Mike, > > Thanks for the patch, it looks correct to me. > The code is fine, but the commit message needs some rework. > Title should start with: "hash: ...", since this is aiming the hash library, So > the title could be: "hash: fix lock release on add" > > Then, some explanation on what the fix is doing would be good. > > Lastly, since this is a fix, it requires a fixes line and CC to the stable branch: > > Fixes: be856325cba3 ("hash: add scalable multi-writer insertion with Intel > TSX") > CC: stable@dpdk.org FYI, I just submitted a v2 with the commit fixed for you, as you might not be available these days and the RC1 will be created soon. Pablo > > Thanks, > Pablo ^ permalink raw reply [flat|nested] 5+ messages in thread
* [dpdk-dev] [PATCH v2] hash: fix lock release on add 2017-05-26 12:30 [dpdk-dev] [PATCH] __rte_hash_add_key_with_hash not releasing multiwriter_lock in failure paths mstolarchuk 2017-07-05 14:50 ` De Lara Guarch, Pablo @ 2017-07-07 5:54 ` Pablo de Lara 2017-07-08 17:00 ` Thomas Monjalon 1 sibling, 1 reply; 5+ messages in thread From: Pablo de Lara @ 2017-07-07 5:54 UTC (permalink / raw) To: dev; +Cc: mstolarchuk, stable From: mstolarchuk <mike.stolarchuk@bigswitch.com> When adding items to a hash table with multiple threads, there is an spinlock used to prevent data corruption (unless Transactional Memory is supported). If there is a failure, the spinlock should be released, but there were cases where that was not happening. Fixes: be856325cba3 ("hash: add scalable multi-writer insertion with Intel TSX") CC: stable@dpdk.org Signed-off-by: mstolarchuk <mike.stolarchuk@bigswitch.com> Acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com> --- Changes in v2: - Fixed commit message lib/librte_hash/rte_cuckoo_hash.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/lib/librte_hash/rte_cuckoo_hash.c b/lib/librte_hash/rte_cuckoo_hash.c index 80391cf..5b2b8dd 100644 --- a/lib/librte_hash/rte_cuckoo_hash.c +++ b/lib/librte_hash/rte_cuckoo_hash.c @@ -539,8 +539,10 @@ __rte_hash_add_key_with_hash(const struct rte_hash *h, const void *key, n_slots = rte_ring_mc_dequeue_burst(h->free_slots, cached_free_slots->objs, LCORE_CACHE_SIZE, NULL); - if (n_slots == 0) - return -ENOSPC; + if (n_slots == 0) { + ret = -ENOSPC; + goto failure; + } cached_free_slots->len += n_slots; } @@ -549,8 +551,10 @@ __rte_hash_add_key_with_hash(const struct rte_hash *h, const void *key, cached_free_slots->len--; slot_id = cached_free_slots->objs[cached_free_slots->len]; } else { - if (rte_ring_sc_dequeue(h->free_slots, &slot_id) != 0) - return -ENOSPC; + if (rte_ring_sc_dequeue(h->free_slots, &slot_id) != 0) { + ret = -ENOSPC; + goto failure; + } } new_k = RTE_PTR_ADD(keys, (uintptr_t)slot_id * h->key_entry_size); @@ -660,6 +664,7 @@ __rte_hash_add_key_with_hash(const struct rte_hash *h, const void *key, /* Error in addition, store new slot back in the ring and return error */ enqueue_slot_back(h, cached_free_slots, (void *)((uintptr_t) new_idx)); +failure: if (h->add_key == ADD_KEY_MULTIWRITER) rte_spinlock_unlock(h->multiwriter_lock); return ret; -- 2.9.4 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [dpdk-dev] [PATCH v2] hash: fix lock release on add 2017-07-07 5:54 ` [dpdk-dev] [PATCH v2] hash: fix lock release on add Pablo de Lara @ 2017-07-08 17:00 ` Thomas Monjalon 0 siblings, 0 replies; 5+ messages in thread From: Thomas Monjalon @ 2017-07-08 17:00 UTC (permalink / raw) To: Pablo de Lara, mstolarchuk; +Cc: dev, stable 07/07/2017 07:54, Pablo de Lara: > From: mstolarchuk <mike.stolarchuk@bigswitch.com> > > When adding items to a hash table with multiple threads, > there is an spinlock used to prevent data corruption > (unless Transactional Memory is supported). > > If there is a failure, the spinlock should be released, > but there were cases where that was not happening. > > Fixes: be856325cba3 ("hash: add scalable multi-writer insertion with Intel TSX") > CC: stable@dpdk.org > > Signed-off-by: mstolarchuk <mike.stolarchuk@bigswitch.com> > Acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com> Applied, thanks ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-07-08 17:00 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-05-26 12:30 [dpdk-dev] [PATCH] __rte_hash_add_key_with_hash not releasing multiwriter_lock in failure paths mstolarchuk 2017-07-05 14:50 ` De Lara Guarch, Pablo 2017-07-07 13:55 ` De Lara Guarch, Pablo 2017-07-07 5:54 ` [dpdk-dev] [PATCH v2] hash: fix lock release on add Pablo de Lara 2017-07-08 17:00 ` Thomas Monjalon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).