On Mon, Aug 22, 2022 at 05:59:09PM +0300, Vitaliy Makkoveev wrote: > On Mon, Aug 22, 2022 at 05:51:00PM +0300, Vitaliy Makkoveev wrote: > > On Mon, Aug 22, 2022 at 03:59:39PM +0200, Alexander Bluhm wrote: > > > On Sun, Aug 21, 2022 at 10:10:53PM +0300, Vitaliy Makkoveev wrote: > > > > On Sun, Aug 21, 2022 at 07:19:23PM +0200, Alexander Bluhm wrote: > > > > > When testing parallel UDP input, I see contention on the netlock > > > > > from the protocol timer. There is a check to avoid loop traversal > > > > > if igmp and mld6 are not active. Move the shortcut to avoid grabbing > > > > > netlock. This should be save as variables igmp_timers_are_running > > > > > and mld_timers_are_running are read atomically. In case we miss > > > > > an fast timer due to MP races, we just run it next time. > > > > > > > > Could we have a commentary like this before unlocked check? The rest of > > > > diff looks ok by me. > > > > > > > > > This should be save as variables igmp_timers_are_running > > > > > and mld_timers_are_running are read atomically. In case we miss > > > > > an fast timer due to MP races, we just run it next time. > > > > > > I commited it with comment. I wonder if we should use read once. > > > It is an unprotected atomic acces. Read once makes this explicit. > > > > > > > READ_ONCE() is useless here. It will be loaded in any case. But since > > this check and `*_timers_are_running' modifications are not serialized, > > we could miss this timer run. > > > > > * Variable igmp_timers_are_running is read atomically. In case we > > > * miss a fast timer due to MP races, just run it next time. > > > */ > > > - if (!igmp_timers_are_running) > > > + if (!READ_ONCE(igmp_timers_are_running)) > > > return; > > > > May be it was not clean, I asked to put this comment for mark, that this > lockless check was made intentionally and this is not the error.
Now I am confused what you wanted to say. Of course the READ_ONCE() does not change behavior here. The function starts before the read and after read NET_LOCK() is a barrier. Unless the compiler is stupid, it has no other possibility than to read the value once. But in general, when we do a lockless read access to a variable that is written from multiple CPUs, we should use atomic_load_int() or READ_ONCE(). Otherwise the compiler could add some optimizations that results in multiple inconsistent reads. Is my impression from standard C correct? That is the reason why I wanted to add READ_ONCE(). It is an hint that we have a not serialized access and have thought about it. So what do want to say: - Do not put the READ_ONCE() here as it is confusing. - Make the comment more verbose to make clear this oportunistic read is MP safe. - Something else? bluhm
