Message-Id: <20220306140655.19177-1-xiam0nd.tong@gmail.com>
Date:   Sun,  6 Mar 2022 22:06:55 +0800
From:   Xiaomeng Tong <xiam0nd.tong@...il.com>
To:     torvalds@...ux-foundation.org
Cc:     arnd@...db.de, gregkh@...uxfoundation.org, jakobkoschel@...il.com,
        jannh@...gle.com, keescook@...omium.org,
        linux-kbuild@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org, netdev@...r.kernel.org, xiam0nd.tong@...il.com
Subject: Re: [PATCH 2/6] list: add new MACROs to make iterator invisiable outside the loop

On Sat, 5 Mar 2022 16:35:36 -0800 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()'.
>

Yes, brilliant! It is tricky and hacky. In your example: &tsk->children
will be expanded to &tsk->children_traversal_type.
And it also reduces column of the calling line with simplified version
of list_for_each_entry.

But, maybe there are some more cases the union-based way need to handle.
Such as, in your example, if the &HEAD passing to list_for_each_entry is
*not* "&tsk->children", but just a *naked head* with no any extra
information provoided:
void foo(...) {
    bar(&tsk->children);
}
noinline void bar(struct list_head *naked_head) {
    struct task_struct *p;
    list_for_each_entry(p, naked_head, sibling) {
    ...
    }
}
you should change all declares like "struct list_head" here with the union
one, but not only in the structure of task_struct itself.

I'm going to dig into this union-base way and re-send a patch if necessary.

> 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.
> 

Yes, the "c" is as the found entry for outside use -- it is natural.
And the "pos" as a inside-defined variable -- it is our goal.
And the "continue" trick to reduce 1 line -- it is nice.

> 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.
> 

Yes, i am very glad that you accepted and agreed my suggestion for the
*incremental changes* part, just like my "_inside" way used.
It means a lot for me to have your approval.

> 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

--
Xiaomeng Tong

Powered by blists - more mailing lists