Date:   Sat, 5 Mar 2022 16:35:36 -0800
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Xiaomeng Tong <xiam0nd.tong@...il.com>
Cc:     Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jakob Koschel <jakobkoschel@...il.com>,
        Jann Horn <jannh@...gle.com>,
        Kees Cook <keescook@...omium.org>,
        Linux Kbuild mailing list <linux-kbuild@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux-MM <linux-mm@...ck.org>, Netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH 2/6] list: add new MACROs to make iterator invisiable
 outside the loop

On Sat, Mar 5, 2022 at 1:09 PM Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
>
> Now, I'd love for the list head entry itself to "declare the type",
> and solve it that way. That would in many ways be the optimal
> situation, in that when a structure has that
>
>         struct list_head xyz;
>
> entry, it would be lovely to declare *there* what the list entry type
> is - and have 'list_for_each_entry()' just pick it up that way.
>
> It would be doable in theory - with some preprocessor trickery [...]

Ok, I decided to look at how that theory looks in real life.

The attached patch does actually work for me. I'm not saying this is
*beautiful*, but I made the changes to kernel/exit.c to show how this
can be used, and while the preprocessor tricks and the odd "unnamed
union with a special member to give the target type" is all kinds of
hacky, the actual use case code looks quite nice.

In particular, look at the "good case" list_for_each_entry() transformation:

   static int do_wait_thread(struct wait_opts *wo, struct task_struct *tsk)
   {
  -     struct task_struct *p;
  -
  -     list_for_each_entry(p, &tsk->children, sibling) {
  +     list_traverse(p, &tsk->children, sibling) {

IOW, it avoided the need to declare 'p' entirely, and it avoids the
need for a type, because the macro now *knows* the type of that
'tsk->children' list and picks it out automatically.

So 'list_traverse()' is basically a simplified version of
'list_for_each_entry()'.

That patch also has - as another example - the "use outside the loop"
case in mm_update_next_owner(). That is more of a "rewrite the loop
cleanly using list_traverse() thing, but it's also quite simple and
natural.

One nice part of this approach is that it allows for incremental changes.

In fact, the patch very much is meant to demonstrate exactly that:
yes, it converts the uses in kernel/exit.c, but it does *not* convert
the code in kernel/fork.c, which still does that old-style traversal:

                list_for_each_entry(child, &parent->children, sibling) {

and the kernel/fork.c code continues to work as well as it ever did.

So that new 'list_traverse()' function allows for people to say "ok, I
will now declare that list head with that list_traversal_head() macro,
and then I can convert 'list_for_each_entry()' users one by one to
this simpler syntax that also doesn't allow the list iterator to be
used outside the list.

What do people think? Is this clever and useful, or just too subtle
and odd to exist?

NOTE! I decided to add that "name of the target head in the target
type" to the list_traversal_head() macro, but it's not actually used
as is. It's more of a wishful "maybe we could add some sanity checking
of the target list entries later".

Comments?

                   Linus

View attachment "patch.diff" of type "text/x-patch" (4856 bytes)

Powered by blists - more mailing lists