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?

> 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?

bluhm