Date:   Sun, 6 Mar 2022 13:19:09 +0100
From:   Jakob Koschel <jakobkoschel@...il.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Xiaomeng Tong <xiam0nd.tong@...il.com>,
        Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        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 6. Mar 2022, at 01:35, Linus Torvalds <torvalds@...ux-foundation.org> wrote:
> 
> 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?

I guess we could apply this to list_for_each_entry() as well
once all the uses after the loop are fixed?

I feel like this simply introduces a new set of macros
(we would also need list_traverse_reverse(), list_traverse_continue_reverse()
etc) and end up with a second set of macros that do pretty much
the same as the first one.

I like the way of using list_traversal_head() to only remember the type.
The interface of list_traverse() is the same as list_for_each_entry() so
we could just do this with a simple coccinelle script once 'pos' is no
longer used after the loop:

-struct some_struct *pos;
+list_traversal_head(struct some_struct, pos, target_member);


although there are *some* cases where 'pos' is also used separately
in the function which would need to change, e.g.:

struct some_struct *pos = some_variable;

if (pos)
	// do one thing
else
	list_for_each_entry(pos, ..., ...)


(I've fixed ~440/450 cases now and I'm chunking it into patch sets right now.
The once left over are non-obvious code I would need some input on)

Personally I guess I also prefer the name list_for_each_entry() over list_traverse()
and not having two types of iterators for the same thing at the same time.


> 
>                   Linus
> <patch.diff>

Jakob

Powered by blists - more mailing lists