Re: [PATCH 1/3] llist: add ability to remove a particular entry from the list

[Jeff Layton <jlayton@kernel.org>]

On Thu, 2025-01-16 at 10:33 -0500, Chuck Lever wrote:
> On 1/16/25 9:54 AM, Olga Kornievskaia wrote:
> > On Thu, Jan 16, 2025 at 9:27 AM Chuck Lever <chuck.lever@oracle.com> wrote:
> > > 
> > > On 1/15/25 6:24 PM, Olga Kornievskaia wrote:
> > > > nfsd stores its network transports in a lwq (which is a lockless list)
> > > > llist has no ability to remove a particular entry which nfsd needs
> > > > to remove a listener thread.
> > > 
> > > Note that scripts/get_maintainer.pl says that the maintainer of this
> > > file is:
> > > 
> > >     linux-kernel@vger.kernel.org
> > > 
> > > so that address should appear on the cc: list of this series. So
> > > should the list of reviewers for NFSD that appear in MAINTAINERS.
> > 
> > I will resend and include the mentioned list.
> > 
> > > ISTR Neil found the same gap in the llist API. I don't think it's
> > > possible to safely remove an item from the middle of an llist. Much
> > > of the complexity of the current svc thread scheduler is due to this
> > > complication.
> > > 
> > > I think you will need to mark the listener as dead and find some
> > > way of safely dealing with it later.
> > 
> > A listener can only be removed if there are no active threads. This
> > code in nfsd_nl_listener_set_doit()  won't allow it which happens
> > before we are manipulating the listener:
> >          /* For now, no removing old sockets while server is running */
> >          if (serv->sv_nrthreads && !list_empty(&permsocks)) {
> >                  list_splice_init(&permsocks, &serv->sv_permsocks);
> >                  spin_unlock_bh(&serv->sv_lock);
> >                  err = -EBUSY;
> >                  goto out_unlock_mtx;
> >          }
> 
> Got it.
> 
> I'm splitting hairs, but this seems like a special case that might be
> true only for NFSD and could be abused by other API consumers.
> 
> At the least, the opening block comment in llist.h should be updated
> to include del_entry in the locking table.
> 
> I would be more comfortable with a solution that works in alignment with
> the current llist API, but if others are fine with this solution, then I
> won't object strenuously.
> 
> (And to be clear, I agree that there is a bug in set_doit() that needs
> to be addressed quickly -- it's the specific fix that I'm queasy about).
> 

Yeah, it would be good to address it quickly since you can crash the
box with this right now.

I'm not thrilled with adding the llist_del_entry() footgun either, but
it should work.

Another approach we could consider instead would be to delay queueing
all of these sockets to the lwq until after the sv_permsocks list is
settled. You could even just queue up the whole sv_permsocks list at
the end of this. If there's no work there, then there's no real harm.
That is a bit more surgery, however, since you'd have to lift
svc_xprt_received() handling out of svc_xprt_create_from_sa().

> 
> > > > Suggested-by: Jeff Layton <jlayton@kernel.org>
> > > > Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
> > > > ---
> > > >    include/linux/llist.h | 36 ++++++++++++++++++++++++++++++++++++
> > > >    1 file changed, 36 insertions(+)
> > > > 
> > > > diff --git a/include/linux/llist.h b/include/linux/llist.h
> > > > index 2c982ff7475a..fe6be21897d9 100644
> > > > --- a/include/linux/llist.h
> > > > +++ b/include/linux/llist.h
> > > > @@ -253,6 +253,42 @@ static inline bool __llist_add(struct llist_node *new, struct llist_head *head)
> > > >        return __llist_add_batch(new, new, head);
> > > >    }
> > > > 
> > > > +/**
> > > > + * llist_del_entry - remove a particular entry from a lock-less list
> > > > + * @head: head of the list to remove the entry from
> > > > + * @entry: entry to be removed from the list
> > > > + *
> > > > + * Walk the list, find the given entry and remove it from the list.
> 
> The above sentence repeats the comments in the code and the code itself.
> It visually obscures the next, much more important, sentence.
> 
> 
> > > > + * The caller must ensure that nothing can race in and change the
> > > > + * list while this is running.
> > > > + *
> > > > + * Returns true if the entry was found and removed.
> > > > + */
> > > > +static inline bool llist_del_entry(struct llist_head *head, struct llist_node *entry)
> > > > +{
> > > > +     struct llist_node *pos;
> > > > +
> > > > +     if (!head->first)
> > > > +             return false;
> 
> if (llist_empty()) ?
> 
> 
> > > > +
> > > > +     /* Is it the first entry? */
> > > > +     if (head->first == entry) {
> > > > +             head->first = entry->next;
> > > > +             entry->next = entry;
> > > > +             return true;
> 
> llist_del_first() or even llist_del_first_this()
> 
> Basically I would avoid open-coding this logic and use the existing
> helpers where possible. The new code doesn't provide memory release
> semantics that would ensure the next access of the llist sees these
> updates.
> 
> 
> > > > +     }
> > > > +
> > > > +     /* Find it in the list */
> > > > +     llist_for_each(head->first, pos) {
> > > > +             if (pos->next == entry) {
> > > > +                     pos->next = entry->next;
> > > > +                     entry->next = entry;
> > > > +                     return true;
> > > > +             }
> > > > +     }
> > > > +     return false;
> > > > +}
> > > > +
> > > >    /**
> > > >     * llist_del_all - delete all entries from lock-less list
> > > >     * @head:   the head of lock-less list to delete all entries
> > > 
> > > 
> > > --
> > > Chuck Lever
> > > 
> > 
> 
> 

-- 
Jeff Layton <jlayton@kernel.org>