Re: [PATCH 5/5][RFC] rcu: start new grace period from rcu_pending()

[Paul E. McKenney]

On Mon, Jan 09, 2006 at 05:32:55PM +0300, Oleg Nesterov wrote:
> Oleg Nesterov wrote:
> >
> > I think it is better to set ->qs_pending = 1 directly in __rcu_pending():
> 
> This patch has a bug. I am sending a trivial fix, but now I am not
> sure myself that 1 timer tick saved worth the code uglification.

This is indeed an accident waiting to happen -- someone is bound to
replace the "|" with an "||", a change that is too easy for someone
to miss.  Once Vatsa is satisfied with the CPU-hotplug aspects of
this set of patches, if __rcu_pending() still has side-effects, I would
suggest something like the following:

	int rcu_pending(int cpu)
	{
		int retval = 0;

		if (__rcu_pending(&rcu_ctrlblk, &per_cpu(rcu_data, cpu)))
			retval = 1;
 		if (__rcu_pending(&rcu_bh_ctrlblk, &per_cpu(rcu_bh_data, cpu)))
			retval = 1;
		return retval;
	}

A few more lines, but the intent is much more clear.  And I bet that
gcc generates reasonable code in either case.

Or maybe this is just me...

							Thanx, Paul

> [PATCH 6/5] rcu: start new grace period from rcu_pending() fix
> 
> We should not miss __rcu_pending(&rcu_bh_ctrlblk) in rcu_pending().
> 
> Signed-off-by: Oleg Nesterov <oleg@xxxxxxxxxx>
> 
> --- 2.6.15/kernel/rcupdate.c~6_FIX	2006-01-09 00:26:44.000000000 +0300
> +++ 2.6.15/kernel/rcupdate.c	2006-01-09 19:19:27.000000000 +0300
> @@ -464,7 +464,7 @@ static int __rcu_pending(struct rcu_ctrl
>  
>  int rcu_pending(int cpu)
>  {
> -	return __rcu_pending(&rcu_ctrlblk, &per_cpu(rcu_data, cpu)) ||
> +	return __rcu_pending(&rcu_ctrlblk, &per_cpu(rcu_data, cpu)) |
>  		__rcu_pending(&rcu_bh_ctrlblk, &per_cpu(rcu_bh_data, cpu));
>  }
> 
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/