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(-)

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