> On 22 Aug 2022, at 18:51, Alexander Bluhm <[email protected]> wrote:
>
> 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?
May be I misunderstood something, but what is the reason to compiler to
drop this read? The value is not yet loaded, and we have no other
unlocked access to this variable before rwlock.
void
mld6_fasttimeo(void)
{
struct ifnet *ifp;
/*
* Quick check to see if any work needs to be done, in order
* to minimize the overhead of fasttimo processing.
* Variable mld_timers_are_running is read atomically. In case we
* miss a fast timer due to MP races, just run it next time.
*/
if (!mld_timers_are_running)
return;
>
> 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:
>
> - Make the comment more verbose to make clear this oportunistic
> read is MP safe.
This one.
