Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Fri, 11 Mar 2022 14:27:54 +0000
From:   Daniel Thompson <daniel.thompson@linaro.org>
To:     Linus Torvalds <torvalds@linux-foundation.org>
Cc:     Xiaomeng Tong <xiam0nd.tong@gmail.com>,
        Arnd Bergmann <arnd@arndb.de>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jakob Koschel <jakobkoschel@gmail.com>,
        Jann Horn <jannh@google.com>,
        Kees Cook <keescook@chromium.org>,
        Linux Kbuild mailing list <linux-kbuild@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux-MM <linux-mm@kvack.org>, Netdev <netdev@vger.kernel.org>
Subject: Re: [PATCH 2/6] list: add new MACROs to make iterator invisiable
 outside the loop
Message-ID: <20220311142754.a3jnnjqxpok75qgp@maple.lan>
References: <CAHk-=whJX52b1jNsmzXeVr6Z898R=9rBcSYx2oLt69XKDbqhOg@mail.gmail.com>
 <20220304025109.15501-1-xiam0nd.tong@gmail.com>
 <CAHk-=wjesxw9U6JvTw34FREFAsayEE196Fi=VHtJXL8_9wgi=A@mail.gmail.com>
 <CAHk-=wiacQM76xec=Hr7cLchVZ8Mo9VDHmXRJzJ_EX4sOsApEA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAHk-=wiacQM76xec=Hr7cLchVZ8Mo9VDHmXRJzJ_EX4sOsApEA@mail.gmail.com>
Precedence: bulk

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

It is possible simply to use spelling to help uncover errors in
list_traverse()?

Something like:

#define list_traversal_head(type, name, target_member) \
	union { \
		struct list_head name; \
		type *name##_traversal_mismatch_##target_member; \
	}

And:

#define list_traverse(pos, head, member) \
	for (typeof(*head##_traversal_mismatch_##member) pos = list_first_entry(head, typeof(*pos), member); \
		!list_entry_is_head(pos, head, member);	\
		pos = list_next_entry(pos, member))

If I deliberately insert an error into your modified exit.c then the
resulting errors even make helpful suggestions about what you did
wrong:

kernel/exit.c:412:32: error: ‘struct task_struct’ has no member named
‘children_traversal_mismatch_children’; did you mean
‘children_traversal_mismatch_sibling’?

The suggestions are not always as good as the above
(children_traversal_mismatch_ptrace_entry suggests
ptraced_traversal_mismatch_ptrace_entry) but, nevertheless, it does
 appears to be robust in detecting incorrect traversal.


> diff --git a/include/linux/list.h b/include/linux/list.h
> index dd6c2041d09c..1e8b3e495b51 100644
> --- a/include/linux/list.h
> +++ b/include/linux/list.h
> @@ -25,6 +25,9 @@
>  #define LIST_HEAD(name) \
>  	struct list_head name = LIST_HEAD_INIT(name)

Seeing this in the diff did set me thinking about static/global
list heads.

For architectures without HAVE_LD_DEAD_CODE_DATA_ELIMINATION then the
"obvious" extension of list_traversal_head() ends up occupying bss
space. Even replacing the pointer with a zero length array is still
provoking gcc-11 (arm64) to allocate a byte from bss (often with a lot
of padding added).

Perhaps in the grand scheme of things this doesn't matter. Across the
whole tree and all architecture I see only ~1200 instances so even in
the worst case and with padding everywhere the wasted RAM is only a few
kb.

Nevertheless I was curious if there is any cunning tricks to avoid
this? Naturally LIST_HEAD() could just declare a union but that would
require all sites of use to be updated simultaneously and I rather
like the way list_traverse_head() is entirely incremental.


Daniel.