Message ID | 20190909135142.3510-1-ruifeng.wang@arm.com (mailing list archive) |
---|---|
State | Accepted, archived |
Delegated to: | David Marchand |
Headers | show |
Series | lib/rcu: fix possible spurious thread unregister | expand |
Context | Check | Description |
---|---|---|
ci/checkpatch | success | coding style OK |
ci/iol-dpdk_compile_ovs | success | Compile Testing PASS |
ci/iol-dpdk_compile | success | Compile Testing PASS |
ci/iol-dpdk_compile_spdk | success | Compile Testing PASS |
ci/intel-Performance | success | Performance Testing PASS |
ci/mellanox-Performance | success | Performance Testing PASS |
ci/Intel-compilation | success | Compilation OK |
Thanks Ruifeng, looks good. > -----Original Message----- > From: Ruifeng Wang <ruifeng.wang@arm.com> > Sent: Monday, September 9, 2019 8:52 AM > To: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com> > Cc: dev@dpdk.org; Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>; > nd <nd@arm.com>; Ruifeng Wang (Arm Technology China) > <Ruifeng.Wang@arm.com>; stable@dpdk.org > Subject: [PATCH] lib/rcu: fix possible spurious thread unregister > > Thread unregister returns success while unregister not been performed. > This is due to incorrect thread registration status check. > Fix this issue by correcting bitmap check. > > Fixes: 64994b56cfd7 ("rcu: add RCU library supporting QSBR mechanism") > Cc: stable@dpdk.org > > Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com> > Reviewed-by: Gavin Hu <gavin.hu@arm.com> > --- > lib/librte_rcu/rte_rcu_qsbr.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/lib/librte_rcu/rte_rcu_qsbr.c b/lib/librte_rcu/rte_rcu_qsbr.c index > ce7f93dd3..8c37c88cd 100644 > --- a/lib/librte_rcu/rte_rcu_qsbr.c > +++ b/lib/librte_rcu/rte_rcu_qsbr.c > @@ -158,7 +158,7 @@ rte_rcu_qsbr_thread_unregister(struct rte_rcu_qsbr > *v, unsigned int thread_id) > /* Check if the thread is already unregistered */ > old_bmap = __atomic_load_n(__RTE_QSBR_THRID_ARRAY_ELM(v, i), > __ATOMIC_RELAXED); > - if (old_bmap & ~(1UL << id)) > + if (!(old_bmap & (1UL << id))) > return 0; > > do { > @@ -175,7 +175,7 @@ rte_rcu_qsbr_thread_unregister(struct rte_rcu_qsbr > *v, unsigned int thread_id) > if (success) > __atomic_fetch_sub(&v->num_threads, > 1, __ATOMIC_RELAXED); > - else if (old_bmap & ~(1UL << id)) > + else if (!(old_bmap & (1UL << id))) > /* Someone else unregistered this thread. > * Counter should not be incremented. > */ > -- Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com> > 2.17.1
On Mon, Sep 9, 2019 at 3:52 PM Ruifeng Wang <ruifeng.wang@arm.com> wrote: > > Thread unregister returns success while unregister not been performed. > This is due to incorrect thread registration status check. > Fix this issue by correcting bitmap check. > > Fixes: 64994b56cfd7 ("rcu: add RCU library supporting QSBR mechanism") > Cc: stable@dpdk.org > > Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com> > Reviewed-by: Gavin Hu <gavin.hu@arm.com> > --- > lib/librte_rcu/rte_rcu_qsbr.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/lib/librte_rcu/rte_rcu_qsbr.c b/lib/librte_rcu/rte_rcu_qsbr.c > index ce7f93dd3..8c37c88cd 100644 > --- a/lib/librte_rcu/rte_rcu_qsbr.c > +++ b/lib/librte_rcu/rte_rcu_qsbr.c > @@ -158,7 +158,7 @@ rte_rcu_qsbr_thread_unregister(struct rte_rcu_qsbr *v, unsigned int thread_id) > /* Check if the thread is already unregistered */ > old_bmap = __atomic_load_n(__RTE_QSBR_THRID_ARRAY_ELM(v, i), > __ATOMIC_RELAXED); > - if (old_bmap & ~(1UL << id)) > + if (!(old_bmap & (1UL << id))) > return 0; > > do { > @@ -175,7 +175,7 @@ rte_rcu_qsbr_thread_unregister(struct rte_rcu_qsbr *v, unsigned int thread_id) > if (success) > __atomic_fetch_sub(&v->num_threads, > 1, __ATOMIC_RELAXED); > - else if (old_bmap & ~(1UL << id)) > + else if (!(old_bmap & (1UL << id))) > /* Someone else unregistered this thread. > * Counter should not be incremented. > */ Reviewed-by: David Marchand <david.marchand@redhat.com> Honnappa, While looking at the rcu doxygen, I noted it does not describe what return values to expect from register/unregister. We also have typos on s/rte_rcu_thread_offline/rte_rcu_qsbr_thread_offline/g. Could you send a patch for this? Thanks. -- David Marchand
On Wed, Sep 25, 2019 at 10:01 AM David Marchand <david.marchand@redhat.com> wrote: > > On Mon, Sep 9, 2019 at 3:52 PM Ruifeng Wang <ruifeng.wang@arm.com> wrote: > > > > Thread unregister returns success while unregister not been performed. > > This is due to incorrect thread registration status check. > > Fix this issue by correcting bitmap check. > > > > Fixes: 64994b56cfd7 ("rcu: add RCU library supporting QSBR mechanism") > > Cc: stable@dpdk.org > > > > Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com> > > Reviewed-by: Gavin Hu <gavin.hu@arm.com> > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com> > Reviewed-by: David Marchand <david.marchand@redhat.com> Applied, thanks. -- David Marchand
diff --git a/lib/librte_rcu/rte_rcu_qsbr.c b/lib/librte_rcu/rte_rcu_qsbr.c index ce7f93dd3..8c37c88cd 100644 --- a/lib/librte_rcu/rte_rcu_qsbr.c +++ b/lib/librte_rcu/rte_rcu_qsbr.c @@ -158,7 +158,7 @@ rte_rcu_qsbr_thread_unregister(struct rte_rcu_qsbr *v, unsigned int thread_id) /* Check if the thread is already unregistered */ old_bmap = __atomic_load_n(__RTE_QSBR_THRID_ARRAY_ELM(v, i), __ATOMIC_RELAXED); - if (old_bmap & ~(1UL << id)) + if (!(old_bmap & (1UL << id))) return 0; do { @@ -175,7 +175,7 @@ rte_rcu_qsbr_thread_unregister(struct rte_rcu_qsbr *v, unsigned int thread_id) if (success) __atomic_fetch_sub(&v->num_threads, 1, __ATOMIC_RELAXED); - else if (old_bmap & ~(1UL << id)) + else if (!(old_bmap & (1UL << id))) /* Someone else unregistered this thread. * Counter should not be incremented. */