Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
MIME-Version: 1.0
References: <20231020102901.3354273-1-mszeredi@redhat.com> <20231020102901.3354273-2-mszeredi@redhat.com>
 <3ab474953c734d0bbf0177bf5d208799@AcuMS.aculab.com> <CAOssrKd2tO9a18qEUPnxnoWzD0WSvT2G7_kcJr=uunCd1xKXYA@mail.gmail.com>
In-Reply-To: <CAOssrKd2tO9a18qEUPnxnoWzD0WSvT2G7_kcJr=uunCd1xKXYA@mail.gmail.com>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Thu, 26 Oct 2023 08:42:35 -1000
Message-ID: <CAHk-=wgy8p4is8ApEQCT5NS7XFb+NXeo-TKz7jRRZVksLLBSrQ@mail.gmail.com>
Subject: Re: [RFC PATCH 1/2] add list_for_each_entry_del()
To:     Miklos Szeredi <mszeredi@redhat.com>
Cc:     David Laight <David.Laight@aculab.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Takashi Iwai <tiwai@suse.com>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Al Viro <viro@zeniv.linux.org.uk>,
        Christian Brauner <brauner@kernel.org>,
        James Bottomley <James.Bottomley@hansenpartnership.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, 25 Oct 2023 at 23:52, Miklos Szeredi <mszeredi@redhat.com> wrote:
>
> Something like this?
>
> - list_for_each_entry_del(entry, head, member))
> + while (list_del_first(entry, head, member))
>
> This allows the compact loop condition, and I always hated having to
> pass the type to list_*entry()...  Disadvantage being that the
> assignment is now implicit (just as with all the list iterators).

So I have two issues with this.

One is purely syntactical: I would be personally happier if any new
list iterators kept the declaration of the iterator internally to
itself.

IOW, instead of

        struct request *rq;

        list_for_each_entry_del(rq, list, queuelist) {
                ...

we'd move on to modern C which allows declaring variables in for loops
(so they aren't visible outside the loop), and turn it into

        list_for_each_entry_del(struct request, rq, list, queuelist) {
                ...

which would then behind the scenes turn into

        for (struct request *rq; rq = list_del_first(list, queuelist); ) {
                ...

or something. None of these "we have stale 'rq' variables outside the
list" things.

(Of course, I realize that people then sometimes intentionally use
those stale variables outside the list by breaking out of the loop in
the middle, but it's ugly)

That said, the *second* issue I have with this whole
list_for_each_entry_del() is that it seems a bit mis-designed. You
have two cases:

 - you want to unconditionally delete everything

 - you might want to have a condition on it and break out early

and the first one is often better dealt with by first moving the list
to a private list head (presumably using list_splice_init()), and then
iterating the private list head instead. That can avoid having to hold
a lock over the whole loop (or dropping it and re-taking it), for
example.

And that *second* case would presumably want to use
list_for_each_entry_safe(), and then do the "list_del()" at the _end_
of the loop, not at the beginning?

Hmm?

I guess the "delete the whole list" is the simple case you want to
deal with, and people who can use a private list to avoid locking
already do the list_splice_init() thing separately (or create the
private list as a separate phase, to also deal with the "I'm not going
to delete everything" issue).

                 Linus