Re: [RFC PATCH 1/2] add list_for_each_entry_del()

[Linus Torvalds]

On Wed, 25 Oct 2023 at 23:52, Miklos Szeredi <mszeredi@xxxxxxxxxx> 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