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:   Fri,  4 Mar 2022 10:51:09 +0800
Message-Id: <20220304025109.15501-1-xiam0nd.tong@gmail.com>
In-Reply-To: <CAHk-=whJX52b1jNsmzXeVr6Z898R=9rBcSYx2oLt69XKDbqhOg@mail.gmail.com>
References: <CAHk-=whJX52b1jNsmzXeVr6Z898R=9rBcSYx2oLt69XKDbqhOg@mail.gmail.com>
Precedence: bulk

First of all, thank you very much for your patient reply and valuable
comments. This is a great inspiration to me.

> On Mon, Feb 28, 2022 at 11:59 PM Xiaomeng Tong <xiam0nd.tong@gmail.com> wrote:
> >
> > +#define list_for_each_entry_inside(pos, type, head, member)            \
> 
> So as mentioned in another thread, I actually tried exactly this.
> 
> And it was horrendous.
> 
> It's _technically_ probably a very nice solution, but

Yes, I always think it is a perfect solution _technically_, since you
first proposed in the thread of Jakob's first subject.

> 
>  - it means that the already *good* cases are the ones that are
> penalized by having to change

Yes, but it also kills potential risks that one day somebody mistakely
uses iterator after the loop in this already *good* cases, as it removed
the original declare of pos and any use-after-loop will be catched by
compiler.

> 
>  - the syntax of the thing becomes absolutely nasty
> 
> which means that _practially_ it's exactly the wrong thing to do.
> 
> Just as an example, this is a random current "good user" in kernel/exit.c:
> 
> -       list_for_each_entry_safe(p, n, dead, ptrace_entry) {
> +       list_for_each_entry_safe_inside(p, n, struct task_struct,
> dead, ptrace_entry) {
> 
> and while some of the effects are nice (no need to declare p/n ahead
> of time), just look at how nasty that line is.
> 
> Basically every single use will result in an over-long line. The above
> example has minimal indentation, almost minimal variable names (to the
> point of not being very descriptive at all), and one of the most basic
> kernel structure types. And it still ended up 87 columns wide.
> 
>  And no, the answer to that is not "do it on multiple lines then".
> That is just even worse.

Two avoid multiple lines,  there are some mitigations:
1. use a shorter macro name: (add 2 chars)
list_for_each_entry_i instead of list_for_each_entry_inside

2. using a shorter type passing to the macro: (add 3 chars)
+ #define t struct sram_bank_info
- list_for_each_entry(pos, head, member) {
+ list_for_each_entry_i(pos, t, head, member) {

3. restore all name back to list_for_each_entry after everything is done:
   (minus 2 chars)
Although we need replace all the use of list_for_each_entry* (15000+)
with list_for_each_entry*_i, the work can be done gradually rather
than all at once. We can incrementally replace these callers until
all these in the kernel are completely updated with *_i* one. At
that time, we can just remove the implements of origin macros and rename
the *_i* macro back to the origin name just in one single patch.

4. As you mentioned, the "safe" version of list_for_each_entry do not
   need "n" argument anymore with the help of -std=gnu11. (minus 3 chars)

Thus, after all mitigations applied, the "safe" version adds *no* chars to
columns wide, and other version adds 3 chars totally, which is acceptable
to me.

> 
> So I really think this is a major step in the wrong direction.

Maybe yes or maybe no.
Before the list_for_each_entry_inside way, I have tried something like
"typeof(pos) pos" way as and before you proposed in the thread of Jakob's
second subject, to avoid any changes to callers of the macros. But it also
has potential problems. see my previous reply to you here:
https://lore.kernel.org/lkml/20220302093106.8402-1-xiam0nd.tong@gmail.com/

> 
> We should strive for the *bad* cases to have to do extra work, and
> even there we should really strive for legibility.

Indeed, there are many "multiple lines" problems in the current kernel
code, for example (drivers/dma/iop-adma.c):
				list_for_each_entry_from(grp_iter,
					&iop_chan->chain, chain_node) {

> 
> Now, I think that "safe" version in particular can be simplified:
> there's no reason to give the "n" variable a name. Now that we can
> (with -stc=gnu11) just declare our own variables in the for-loop, the
> need for that externally visible 'next' declaration just goes away.
> 
> So three of those 87 columns are pointless and should be removed. The
> macro can just internally decare 'n' like it always wanted (but
> couldn't do due to legacy C language syntax restrictions).

Great, this does reduce three chars. and i will look into other versions.

> 
> But even with that fixed, it's still a very cumbersome line.

With other mitigations mentioned above, the addition to line will be
acceptable.

> 
> Note how the old syntax was "only" 60 characters - long but still
> quite legible (and would have space for two more levels of indentation
> without even hitting 80 characters). And that was _despute_ having to
> have that 'n' declaration.
> 
> And yes, the old syntax does require that
> 
>         struct task_struct *p, *n;
> 
> line to declare the types, but that really is not a huge burden, and
> is not complicated. It's just another "variables of the right type"
> line (and as mentioned, the 'n' part has always been a C syntax
> annoyance).

Yes, that really is not a huge burden, so is the mitigation 2 mentioned
above which defining a shorter type passing to the macro, to shorten the 
new line.

> 
>               Linus

--
Xiaomeng Tong