Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Xiaomeng Tong <xiam0nd.tong@gmail.com>
To:     torvalds@linux-foundation.org
Cc:     arnd@arndb.de, gregkh@linuxfoundation.org, jakobkoschel@gmail.com,
        jannh@google.com, keescook@chromium.org,
        linux-kbuild@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org, netdev@vger.kernel.org, xiam0nd.tong@gmail.com
Subject: Re: [PATCH 2/6] list: add new MACROs to make iterator invisiable outside the loop
Date:   Sun,  6 Mar 2022 22:06:55 +0800
Message-Id: <20220306140655.19177-1-xiam0nd.tong@gmail.com>
In-Reply-To: <CAHk-=wiacQM76xec=Hr7cLchVZ8Mo9VDHmXRJzJ_EX4sOsApEA@mail.gmail.com>
References: <CAHk-=wiacQM76xec=Hr7cLchVZ8Mo9VDHmXRJzJ_EX4sOsApEA@mail.gmail.com>
Precedence: bulk

On Sat, 5 Mar 2022 16:35:36 -0800 Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Sat, Mar 5, 2022 at 1:09 PM Linus Torvalds
> <torvalds@linux-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