Re: [PATCH] fix misplaced mb() in rcu_enter/exit_nohz()

[Heiko Carstens <heiko.carstens@de.ibm.com>]

On Mon, Mar 17, 2008 at 01:43:57PM -0700, Paul E. McKenney wrote:
> On Mon, Mar 17, 2008 at 11:17:41PM +0300, Oleg Nesterov wrote:
> > (to clarify: my question is completely offtopic to this patch)
> > On 03/17, Paul E. McKenney wrote:
> > > On Mon, Mar 17, 2008 at 09:30:47PM +0300, Oleg Nesterov wrote:
> > > > I'm not sure the code below is up to date, but what I have in
> > > > arch/s390/kernel/time.c is:
> > > > 
> > > > 	stop_hz_timer:
> > > > 
> > > > 		cpu_set(cpu, nohz_cpu_mask);
> > > > 		
> > > > 		if (rcu_needs_cpu(cpu) || local_softirq_pending()) {
> > > > 			cpu_clear(cpu, nohz_cpu_mask);
> > > > 			return;
> > > > 		}
> > > > 
> > > > Don't we need smp_mb() after cpu_set() ?
> > > 
> > > S390's memory model is quite strong, so it might not be needed.
> > 
> > OK, in that case we shouldn't worry.
> 
> I don't know if I would go -that- far.  ;-)

Not sure if I can follow you here, but for the smp case cpu_set() is
nothing but a set_bit() which implies both: a memory barrier and a compiler
barrier on s390. The compare and swap instruction used here ensures that
all previous memory accesses will be seen by other cpus.

Btw. the code sequence above will go away soon anyway, since I'm converting
our code to the generic NO_HZ infrastructure.

> > > In any
> > > case, if needed, it goes -before- the cpu_set(), because the problems
> > > would arise if prior RCU read-side critical sections were to be reordered
> > > to follow this cpu_set(), right?
> > 
> > No, but it is very possible I missed something.
> > 
> > What if rcu_needs_cpu(cpu) is executed before cpu_set(cpu, nohz_cpu_mask)?
> > It can miss rcu_start_batch() -> rcp->cur++ and return false, but at the
> > same time rcu_start_batch() may see nohz_cpu_mask without this CPU.
> 
> If you mean that the rcu_needs_cpu() executes before the cpu_set() in
> the code fragment above, while the rcu_start_batch() executes on some
> other CPU?

That should never happen because of the compiler and memory barrier semantics
of cpu_set(). Or... I'm completely misunderstanding you, which wouldn't me
surprise me too much :)