On Fri, May 17, 2024 at 10:01:37PM +0200, Alexander Bluhm wrote:
> On Fri, May 17, 2024 at 09:14:31PM +0200, Claudio Jeker wrote:
> > > > -	if (!igmp_timers_are_running)
> > > > +	if (!READ_ONCE(igmp_timers_are_running))
> > > >  		return;
> > 
> > This can't be correct. I know what you want but that is not the right way
> > of doing this check. Which daemon are you removing by using READ_ONCE()
> > here?
> 
> As long as the compiler knows that foo() and bar() don't touch
> variable, the compiler can optimize this code:
> 
> 	if (variable)
> 		foo();
> 	else
> 		bar();
> 
> to this construct:
> 
> 	if (variable)
> 		foo();
> 	if (!variable)
> 		bar();
> 
> If variable is global and accessed by another CPU the following can
> happen:
> 
> CPU 0 sets variable = 1;
> 
> CPU 1 calls foo()
> 
> CPU 0 sets variable = 0;
> 
> CPU 1 calls bar()
> 
> This was never intended in the if/else code and READ_ONCE() or
> volatile prevents it.
> 
> Of course in the code we are talking about it is very unlikely that
> something like this can ever happen.  But I have a very bad feeling
> about accessing a variable from multiple CPU without any protection.
> I would like to have a consistent pattern to prevent MP surprise.
> 
> But I looks that I am the only one who cares about this problem.
> Am I too paranoid for MP?

Stupid compilers are broken. Some of these transformations they do are
just not side-effect free. So no you are not too paranoid. You are not
paranoid enough since the code would probably need some memory barriers as
well (at least on the producer side). Right now I think it is fine but if
someone changes the NET_LOCK in the timeout even a shared NET_LOCK then we
are in trouble.
 
> > These constant timeouts are just horrible. We now have a timeout API that
> > can handle timeouts at scale and we don't need to tick every 200ms for
> > nothing.
> 
> Should we keep kernel lock because nobody has implemented individual
> timers in the previous 20 years?

No. We should implement individual timers instead of hacking around this
turd. There is no reason to wait another 20 years to fix this.
Using idividual timers makes all of the above go away and we can kill a
200ms constant wakeup in the kernel.

-- 
:wq Claudio