linux-kernel - Re: [RFC PATCH 1/2] add list_for_each_entry

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAHk-=wgy8p4is8ApEQCT5NS7XFb+NXeo-TKz7jRRZVksLLBSrQ@mail.gmail.com>
Date:   Thu, 26 Oct 2023 08:42:35 -1000
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Miklos Szeredi <mszeredi@...hat.com>
Cc:     David Laight <David.Laight@...lab.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Takashi Iwai <tiwai@...e.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Al Viro <viro@...iv.linux.org.uk>,
        Christian Brauner <brauner@...nel.org>,
        James Bottomley <James.Bottomley@...senpartnership.com>
Subject: Re: [RFC PATCH 1/2] add list_for_each_entry_del()

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