> On 6. Mar 2022, at 01:35, Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> On Sat, Mar 5, 2022 at 1:09 PM Linus Torvalds
> <torvalds@xxxxxxxxxxxxxxxxxxxx> 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
