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;
MIME-Version: 1.0
References: <04C1E631-725C-47AD-9914-25D5CE04DFF4@gmail.com>
 <CANn89iKJWw7zUP-E_d=Yhaz=Qw0R3Ae7ULaGgrtsi1yf2pfpGg@mail.gmail.com>
 <E9CF24C7-3080-4720-B540-BAF03068336B@gmail.com> <CANn89iLqU=huOuCt2kXmrXf68TUU-N90aQnMykkYcZ+Arx9-aA@mail.gmail.com>
 <EDE1C0AE-B479-49F9-995D-DA9CC1A6EA57@gmail.com>
In-Reply-To: <EDE1C0AE-B479-49F9-995D-DA9CC1A6EA57@gmail.com>
From:   Eric Dumazet <edumazet@google.com>
Date:   Fri, 21 Jul 2023 16:47:54 +0200
Message-ID: <CANn89iL47bu8bWu2EziK2gCJDBDb4Z5LAZJt1xhb_jQMaieG-g@mail.gmail.com>
Subject: Re: Question about the barrier() in hlist_nulls_for_each_entry_rcu()
To:     Alan Huang <mmpgouride@gmail.com>
Cc:     linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
        rcu@vger.kernel.org, "Paul E. McKenney" <paulmck@kernel.org>,
        roman.gushchin@linux.dev
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Jul 21, 2023 at 4:31=E2=80=AFPM Alan Huang <mmpgouride@gmail.com> w=
rote:
>
>
> > 2023=E5=B9=B47=E6=9C=8821=E6=97=A5 05:11=EF=BC=8CEric Dumazet <edumazet=
@google.com> =E5=86=99=E9=81=93=EF=BC=9A
> >
> > On Thu, Jul 20, 2023 at 10:00=E2=80=AFPM Alan Huang <mmpgouride@gmail.c=
om> wrote:
> >>
> >>
> >>> 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
> >>>
> >>> On Thu, Jul 20, 2023 at 8:54=E2=80=AFPM Alan Huang <mmpgouride@gmail.=
com> wrote:
> >>>>
> >>>> Hi,
> >>>>
> >>>> I noticed a commit c87a124a5d5e(=E2=80=9Cnet: force a reload of firs=
t item in hlist_nulls_for_each_entry_rcu=E2=80=9D)
> >>>> and a related discussion [1].
> >>>>
> >>>> After reading the whole discussion, it seems like that ptr->field wa=
s cached by gcc even with the deprecated
> >>>> ACCESS_ONCE(), so my question is:
> >>>>
> >>>>       Is that a compiler bug? If so, has this bug been fixed today, =
ten years later?
> >>>>
> >>>>       What about READ_ONCE(ptr->field)?
> >>>
> >>> Make sure sparse is happy.
> >>
> >> It caused a problem without barrier(), and the deprecated ACCESS_ONCE(=
) didn=E2=80=99t help:
> >>
> >>        https://lore.kernel.org/all/519D19DA.50400@yandex-team.ru/
> >>
> >> 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?
> >
> > I can not really answer without seeing an actual patch...
>
> The content of the potential patch:
>
> diff --git a/include/linux/rculist_nulls.h b/include/linux/rculist_nulls.=
h
> index 89186c499dd4..bcd39670f359 100644
> --- a/include/linux/rculist_nulls.h
> +++ b/include/linux/rculist_nulls.h
> @@ -158,15 +158,9 @@ static inline void hlist_nulls_add_fake(struct hlist=
_nulls_node *n)
>   * @pos:       the &struct hlist_nulls_node to use as a loop cursor.
>   * @head:      the head of the list.
>   * @member:    the name of the hlist_nulls_node within the struct.
> - *
> - * The barrier() is needed to make sure compiler doesn't cache first ele=
ment [1],
> - * as this loop can be restarted [2]
> - * [1] Documentation/memory-barriers.txt around line 1533
> - * [2] Documentation/RCU/rculist_nulls.rst around line 146
>   */
>  #define hlist_nulls_for_each_entry_rcu(tpos, pos, head, member)         =
               \
> -       for (({barrier();}),                                             =
       \
> -            pos =3D rcu_dereference_raw(hlist_nulls_first_rcu(head));   =
         \
> +       for (pos =3D rcu_dereference_raw(hlist_nulls_first_rcu(head));   =
         \
>                 (!is_a_nulls(pos)) &&                                    =
       \
>                 ({ tpos =3D hlist_nulls_entry(pos, typeof(*tpos), member)=
; 1; }); \
>                 pos =3D rcu_dereference_raw(hlist_nulls_next_rcu(pos)))
> @@ -180,8 +174,7 @@ static inline void hlist_nulls_add_fake(struct hlist_=
nulls_node *n)
>   * @member:    the name of the hlist_nulls_node within the struct.
>   */
>  #define hlist_nulls_for_each_entry_safe(tpos, pos, head, member)        =
       \
> -       for (({barrier();}),                                             =
       \
> -            pos =3D rcu_dereference_raw(hlist_nulls_first_rcu(head));   =
         \
> +       for (pos =3D rcu_dereference_raw(hlist_nulls_first_rcu(head));   =
         \
>                 (!is_a_nulls(pos)) &&                                    =
       \
>                 ({ tpos =3D hlist_nulls_entry(pos, typeof(*tpos), member)=
;        \
>                    pos =3D rcu_dereference_raw(hlist_nulls_next_rcu(pos))=
; 1; });)
>
>
> >
> > Why are you asking ? Are you tracking compiler bug fixes ?
>
> The barrier() here makes me confused.
>
> If we really need that, do we need:
>
>         READ_ONCE(head->first);
>         barrier();
>         READ_ONCE(head->first);
>

Nope, the patch you want to revert (while it did fix (by pure luck
???) a real bug back in the days) was replacing

ACCESS_ONCE()

by

barrier();
ACCESS_ONCE();

(There is one ACCESS_ONCE(), not two of them)

BTW,
  barrier();
  followed by an arbitrary number of barrier(); back to back,
translates to one barrier()

Frankly, I would not change the code, unless someone can explain what
was the issue.
(Perhaps there was a missing barrier elsewhere)