Re: [PATCH v2 5/9] mm: Initialize struct vm_unmapped_area_info

From: "Edgecombe, Rick P" <rick.p.edgecombe@intel.com>
To: "keescook@chromium.org" <keescook@chromium.org>,
	"christophe.leroy@csgroup.eu" <christophe.leroy@csgroup.eu>
Cc: "linux-s390@vger.kernel.org" <linux-s390@vger.kernel.org>,
	"linux-sh@vger.kernel.org" <linux-sh@vger.kernel.org>,
	"luto@kernel.org" <luto@kernel.org>,
	"dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>,
	"debug@rivosinc.com" <debug@rivosinc.com>,
	"akpm@linux-foundation.org" <akpm@linux-foundation.org>,
	"Liam.Howlett@oracle.com" <Liam.Howlett@oracle.com>,
	"mingo@redhat.com" <mingo@redhat.com>,
	"linux-csky@vger.kernel.org" <linux-csky@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	"linux-mm@kvack.org" <linux-mm@kvack.org>,
	"kirill.shutemov@linux.intel.com"
	<kirill.shutemov@linux.intel.com>,
	"linux-snps-arc@lists.infradead.org"
	<linux-snps-arc@lists.infradead.org>,
	"linux-parisc@vger.kernel.org" <linux-parisc@vger.kernel.org>,
	"loongarch@lists.linux.dev" <loongarch@lists.linux.dev>,
	"hpa@zytor.com" <hpa@zytor.com>,
	"peterz@infradead.org" <peterz@infradead.org>,
	"sparclinux@vger.kernel.org" <sparclinux@vger.kernel.org>,
	"linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>,
	"bp@alien8.de" <bp@alien8.de>,
	"linux-alpha@vger.kernel.org" <linux-alpha@vger.kernel.org>,
	"linux-mips@vger.kernel.org" <linux-mips@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	"broonie@kernel.org" <broonie@kernel.org>,
	"x86@kernel.org" <x86@kernel.org>
Subject: Re: [PATCH v2 5/9] mm: Initialize struct vm_unmapped_area_info
Date: Tue, 27 Feb 2024 20:25:38 +0000	[thread overview]
Message-ID: <91384b505cb78b9d9b71ad58e037c1ed8dfb10d1.camel@intel.com> (raw)
In-Reply-To: <8265f804-4540-4858-adc3-a09c11a677eb@csgroup.eu>

On Tue, 2024-02-27 at 18:16 +0000, Christophe Leroy wrote:
> > > Why doing a full init of the struct when all fields are re-
> > > written a few
> > > lines after ?
> > 
> > It's a nice change for robustness and makes future changes easier.
> > It's
> > not actually wasteful since the compiler will throw away all
> > redundant
> > stores.
> 
> Well, I tend to dislike default init at declaration because it often 
> hides missed real init. When a field is not initialized GCC should
> emit 
> a Warning, at least when built with W=2 which sets 
> -Wmissing-field-initializers ?

Sorry, I'm not following where you are going with this. There aren't
any struct vm_unmapped_area_info users that use initializers today, so
that warning won't apply in this case. Meanwhile, designated style
struct initialization (which would zero new members) is very common, as
well as not get anything checked by that warning. Anything with this
many members is probably going to use the designated style.

If we are optimizing to avoid bugs, the way this struct is used today
is not great. It is essentially being used as an argument passer.
Normally when a function signature changes, but a caller is missed, of
course the compiler will notice loudly. But not here. So I think
probably zero initializing it is safer than being setup to pass
garbage.

I'm trying to figure out what to do here. If I changed it so that just
powerpc set the new field manually, then the convention across the
kernel would be for everything to be default zero, and future other new
parameters could have a greater chance of turning into garbage on
powerpc. Since it could be easy to miss that powerpc was special. Would
you prefer it?

Or maybe I could try a new vm_unmapped_area() that takes the extra
argument separately? The old callers could call the old function and
not need any arch updates. It all seems strange though, because
automatic zero initializing struct members is so common in the kernel.
But it also wouldn't add the cleanup Kees was pointing out. Hmm.

Any preference? Or maybe am I missing your point and talking nonsense?