Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
From:   Joel Fernandes <joel@joelfernandes.org>
Mime-Version: 1.0 (1.0)
Subject: Re: Question about the barrier() in hlist_nulls_for_each_entry_rcu()
Date:   Fri, 21 Jul 2023 12:00:17 -0400
Message-Id: <8EE165A3-B6C9-4FE7-8C50-F6D75E250B8B@joelfernandes.org>
References: <7D269BDF-102C-46B8-B31C-A559D2E410E0@gmail.com>
Cc:     Eric Dumazet <edumazet@google.com>, linux-kernel@vger.kernel.org,
        netdev@vger.kernel.org, rcu@vger.kernel.org,
        "Paul E. McKenney" <paulmck@kernel.org>, roman.gushchin@linux.dev
In-Reply-To: <7D269BDF-102C-46B8-B31C-A559D2E410E0@gmail.com>
To:     Alan Huang <mmpgouride@gmail.com>
Precedence: bulk


> On Jul 21, 2023, at 11:55 AM, Alan Huang <mmpgouride@gmail.com> wrote:
>=20
> =EF=BB=BF
>> 2023=E5=B9=B47=E6=9C=8821=E6=97=A5 23:21=EF=BC=8CJoel Fernandes <joel@joe=
lfernandes.org> =E5=86=99=E9=81=93=EF=BC=9A
>>=20
>> On 7/21/23 10:27, Alan Huang wrote:
>>>> 2023=E5=B9=B47=E6=9C=8821=E6=97=A5 20:54=EF=BC=8CJoel Fernandes <joel@j=
oelfernandes.org> =E5=86=99=E9=81=93=EF=BC=9A
>>>>=20
>>>>=20
>>>>=20
>>>>> On Jul 20, 2023, at 4:00 PM, Alan Huang <mmpgouride@gmail.com> wrote:
>>>>>=20
>>>>> =EF=BB=BF
>>>>>> 2023=E5=B9=B47=E6=9C=8821=E6=97=A5 03:22=EF=BC=8CEric Dumazet <edumaz=
et@google.com> =E5=86=99=E9=81=93=EF=BC=9A
>>>>>>=20
>>>>>>>> On Thu, Jul 20, 2023 at 8:54=E2=80=AFPM Alan Huang <mmpgouride@gmai=
l.com> wrote:
>>>>>>>>=20
>>>>>>>> Hi,
>>>>>>>>=20
>>>>>>>> I noticed a commit c87a124a5d5e(=E2=80=9Cnet: force a reload of fir=
st item in hlist_nulls_for_each_entry_rcu=E2=80=9D)
>>>>>>>> and a related discussion [1].
>>>>>>>>=20
>>>>>>>> After reading the whole discussion, it seems like that ptr->field w=
as cached by gcc even with the deprecated
>>>>>>>> ACCESS_ONCE(), so my question is:
>>>>>>>>=20
>>>>>>>>     Is that a compiler bug? If so, has this bug been fixed today, t=
en years later?
>>>>>>>>=20
>>>>>>>>     What about READ_ONCE(ptr->field)?
>>>>>>>=20
>>>>>>> Make sure sparse is happy.
>>>>>>=20
>>>>>> It caused a problem without barrier(), and the deprecated ACCESS_ONCE=
() didn=E2=80=99t help:
>>>>>>=20
>>>>>>  https://lore.kernel.org/all/519D19DA.50400@yandex-team.ru/
>>>>>>=20
>>>>>> So, my real question is: With READ_ONCE(ptr->field), are there still s=
ome unusual cases where gcc
>>>>>> decides not to reload ptr->field?
>>>>>=20
>>>>> I am a bit doubtful there will be strong (any?) interest in replacing t=
he barrier() with READ_ONCE() without any tangible reason, regardless of whe=
ther a gcc issue was fixed.
>>>>>=20
>>>>> But hey, if you want to float the idea=E2=80=A6
>>> We already had the READ_ONCE() in rcu_deference_raw().
>>> The barrier() here makes me think we need write code like below:
>>>=20
>>> READ_ONCE(head->first);
>>> barrier();
>>> READ_ONCE(head->first);
>>> With READ_ONCE (or the deprecated ACCESS_ONCE),
>>> I don=E2=80=99t think a compiler should cache the value of head->first.
>>=20
>>=20
>> Right, it shouldn't need to cache. To Eric's point it might be risky to r=
emove the barrier() and someone needs to explain that issue first (or IMO th=
ere needs to be another tangible reason like performance etc). Anyway, FWIW I=
 wrote a simple program and I am not seeing the head->first cached with the p=
attern you shared above:
>>=20
>> #include <stdlib.h>
>>=20
>> #define READ_ONCE(x) (*(volatile typeof(x) *)&(x))
>> #define barrier() __asm__ __volatile__("": : :"memory")
>>=20
>> typedef struct list_head {
>>   int first;
>>   struct list_head *next;
>> } list_head;
>>=20
>> int main() {
>>   list_head *head =3D (list_head *)malloc(sizeof(list_head));
>>   head->first =3D 1;
>>   head->next =3D 0;
>>=20
>>   READ_ONCE(head->first);
>>   barrier();
>=20
> Thanks for your time!
>=20
> However, what I'm trying to say here is that without this barrier(), GCC w=
ouldn't cache this value either.
>=20

Yes that is what I tried too, removing the barrier.

Apologies for confusing, the code I shared did have it but I had also tried r=
emoving it.

Thanks,

 - Joel


> So, I removed the barrier() and tested, GCC didn=E2=80=99t cache the value=
 of head->first.
> (Only tested on x86-64 (all the possible versions of gcc that Compiler Exp=
lorer has) where the original issue occurred [1].)
>=20
> Therefore, the commit message and the related discussion ten years ago is m=
isleading.
>=20
> Thanks again!
>=20
> [1] https://lkml.org/lkml/2013/4/16/371
>=20
>=20
>>   READ_ONCE(head->first);
>>=20
>>   free(head);
>>   return 0;
>> }
>>=20
>> On ARM 32-bit, 64-bit and x86_64, with -Os and then another experiment wi=
th -O2 on new gcc versions.
>=20
>=20