Message-ID: <CANn89iLQMVms1GF_oY1WSCtmxLZaBJrTKaeHnwRo5p9uzFwnVw@mail.gmail.com>
Date: Mon, 13 Oct 2025 02:49:14 -0700
From: Eric Dumazet <edumazet@...gle.com>
To: luoxuanqiang <xuanqiang.luo@...ux.dev>
Cc: kuniyu@...gle.com, "Paul E. McKenney" <paulmck@...nel.org>, kerneljasonxing@...il.com, 
	davem@...emloft.net, kuba@...nel.org, netdev@...r.kernel.org, 
	Xuanqiang Luo <luoxuanqiang@...inos.cn>, Frederic Weisbecker <frederic@...nel.org>, 
	Neeraj Upadhyay <neeraj.upadhyay@...nel.org>
Subject: Re: [PATCH net-next v7 1/3] rculist: Add hlist_nulls_replace_rcu()
 and hlist_nulls_replace_init_rcu()

On Mon, Oct 13, 2025 at 1:26 AM luoxuanqiang <xuanqiang.luo@...ux.dev> wrote:
>
>
> 在 2025/10/13 15:31, Eric Dumazet 写道:
> > On Fri, Sep 26, 2025 at 12:41 AM <xuanqiang.luo@...ux.dev> wrote:
> >> From: Xuanqiang Luo <luoxuanqiang@...inos.cn>
> >>
> >> Add two functions to atomically replace RCU-protected hlist_nulls entries.
> >>
> >> Keep using WRITE_ONCE() to assign values to ->next and ->pprev, as
> >> mentioned in the patch below:
> >> commit efd04f8a8b45 ("rcu: Use WRITE_ONCE() for assignments to ->next for
> >> rculist_nulls")
> >> commit 860c8802ace1 ("rcu: Use WRITE_ONCE() for assignments to ->pprev for
> >> hlist_nulls")
> >>
> >> Signed-off-by: Xuanqiang Luo <luoxuanqiang@...inos.cn>
> >> ---
> >>   include/linux/rculist_nulls.h | 59 +++++++++++++++++++++++++++++++++++
> >>   1 file changed, 59 insertions(+)
> >>
> >> diff --git a/include/linux/rculist_nulls.h b/include/linux/rculist_nulls.h
> >> index 89186c499dd4..c26cb83ca071 100644
> >> --- a/include/linux/rculist_nulls.h
> >> +++ b/include/linux/rculist_nulls.h
> >> @@ -52,6 +52,13 @@ static inline void hlist_nulls_del_init_rcu(struct hlist_nulls_node *n)
> >>   #define hlist_nulls_next_rcu(node) \
> >>          (*((struct hlist_nulls_node __rcu __force **)&(node)->next))
> >>
> >> +/**
> >> + * hlist_nulls_pprev_rcu - returns the dereferenced pprev of @node.
> >> + * @node: element of the list.
> >> + */
> >> +#define hlist_nulls_pprev_rcu(node) \
> >> +       (*((struct hlist_nulls_node __rcu __force **)(node)->pprev))
> >> +
> >>   /**
> >>    * hlist_nulls_del_rcu - deletes entry from hash list without re-initialization
> >>    * @n: the element to delete from the hash list.
> >> @@ -152,6 +159,58 @@ static inline void hlist_nulls_add_fake(struct hlist_nulls_node *n)
> >>          n->next = (struct hlist_nulls_node *)NULLS_MARKER(NULL);
> >>   }
> >>
> >> +/**
> >> + * hlist_nulls_replace_rcu - replace an old entry by a new one
> >> + * @old: the element to be replaced
> >> + * @new: the new element to insert
> >> + *
> >> + * Description:
> >> + * Replace the old entry with the new one in a RCU-protected hlist_nulls, while
> >> + * permitting racing traversals.
> >> + *
> >> + * The caller must take whatever precautions are necessary (such as holding
> >> + * appropriate locks) to avoid racing with another list-mutation primitive, such
> >> + * as hlist_nulls_add_head_rcu() or hlist_nulls_del_rcu(), running on this same
> >> + * list.  However, it is perfectly legal to run concurrently with the _rcu
> >> + * list-traversal primitives, such as hlist_nulls_for_each_entry_rcu().
> >> + */
> >> +static inline void hlist_nulls_replace_rcu(struct hlist_nulls_node *old,
> >> +                                          struct hlist_nulls_node *new)
> >> +{
> >> +       struct hlist_nulls_node *next = old->next;
> >> +
> >> +       WRITE_ONCE(new->next, next);
> >> +       WRITE_ONCE(new->pprev, old->pprev);
> > I do not think these two WRITE_ONCE() are needed.
> >
> > At this point new is not yet visible.
> >
> > The following  rcu_assign_pointer() is enough to make sure prior
> > writes are committed to memory.
>
> Dear Eric,
>
> I’m quoting your more detailed explanation from the other patch [0], thank
> you for that!
>
> However, regarding new->next, if the new object is allocated with
> SLAB_TYPESAFE_BY_RCU, would we still encounter the same issue as in commit
> efd04f8a8b45 (“rcu: Use WRITE_ONCE() for assignments to ->next for
> rculist_nulls”)?
>
> Also, for the WRITE_ONCE() assignments to ->pprev introduced in commit
> 860c8802ace1 (“rcu: Use WRITE_ONCE() for assignments to ->pprev for
> hlist_nulls”) within hlist_nulls_add_head_rcu(), is that also unnecessary?

I forgot sk_unhashed()/sk_hashed() could be called from lockless contexts.

It is a bit weird to annotate the writes, but not the lockless reads,
even if apparently KCSAN
is okay with that.


>
> [0]: https://lore.kernel.org/all/CANn89iKQM=4wjCLxpg-m3jYoUm=rsSk68xVLN2902di2+FkSFg@mail.gmail.com/
>
> Thanks!
>
> >> +       rcu_assign_pointer(hlist_nulls_pprev_rcu(new), new);
> >> +       if (!is_a_nulls(next))
> >> +               WRITE_ONCE(next->pprev, &new->next);
> >> +}
> >> +
> >> +/**
> >> + * hlist_nulls_replace_init_rcu - replace an old entry by a new one and
> >> + * initialize the old
> >> + * @old: the element to be replaced
> >> + * @new: the new element to insert
> >> + *
> >> + * Description:
> >> + * Replace the old entry with the new one in a RCU-protected hlist_nulls, while
> >> + * permitting racing traversals, and reinitialize the old entry.
> >> + *
> >> + * Note: @old must be hashed.
> >> + *
> >> + * The caller must take whatever precautions are necessary (such as holding
> >> + * appropriate locks) to avoid racing with another list-mutation primitive, such
> >> + * as hlist_nulls_add_head_rcu() or hlist_nulls_del_rcu(), running on this same
> >> + * list. However, it is perfectly legal to run concurrently with the _rcu
> >> + * list-traversal primitives, such as hlist_nulls_for_each_entry_rcu().
> >> + */
> >> +static inline void hlist_nulls_replace_init_rcu(struct hlist_nulls_node *old,
> >> +                                               struct hlist_nulls_node *new)
> >> +{
> >> +       hlist_nulls_replace_rcu(old, new);
> >> +       WRITE_ONCE(old->pprev, NULL);
> >> +}
> >> +
> >>   /**
> >>    * hlist_nulls_for_each_entry_rcu - iterate over rcu list of given type
> >>    * @tpos:      the type * to use as a loop cursor.
> >> --
> >> 2.25.1
> >>

Powered by blists - more mailing lists