Re: [PATCH] mce: fix warning messages about static struct mce_device

[Ingo Molnar <mingo@elte.hu>]

* Greg KH <gregkh@suse.de> wrote:

> On Tue, Jan 17, 2012 at 09:38:43AM +0100, Ingo Molnar wrote:
> > 
> > * Greg KH <gregkh@suse.de> wrote:
> > 
> > > diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
> > > index f35ce43..6aefb14 100644
> > > --- a/arch/x86/include/asm/mce.h
> > > +++ b/arch/x86/include/asm/mce.h
> > > @@ -151,7 +151,7 @@ static inline void enable_p5_mce(void) {}
> > >  
> > >  void mce_setup(struct mce *m);
> > >  void mce_log(struct mce *m);
> > > -DECLARE_PER_CPU(struct device, mce_device);
> > > +extern struct device *mce_device[CONFIG_NR_CPUS];
> > 
> > Minor nit, i don't think we have any other such [CONFIG_NR_CPUS] 
> > pattern in the kernel.
> > 
> > This should be something like:
> > 
> >   DECLARE_PER_CPU(struct device *, mce_device);
> 
> That is what we used to have, but with just a static struct 
> device. [...]

Which was fine in itself for a per CPU data structure - wouldnt 
the warning be fixed by memset()-ing before registering the 
device or such, if device registry absolutely needs a pre-zeroed 
buffer?

I still think there must be some bug/assumption lurking in the 
device layer - do you require all device allocations to be one 
via zalloc()? Seems like a weird and unrobust requirement.

I don't object to the quick fix that gets rid of the warnings, 
but that quick fix came at the price of leaving the real bug 
unfixed and at the price of introducing a new ugliness ;-)

> [...] We really don't need this to be in the per-cpu area, a 
> flat array should be just fine, why can't we use the 
> CONFIG_NR_CPUS value?  Should we use something else?

By that argument we don't really need PER_CPU() areas to begin 
with, a flat [CONFIG_NR_CPUS] array is just fine, right?

Amongst other things we use PER_CPU to have an array of just 2 
elements on a dual core system, even if it boots a 
CONFIG_NR_CPUS=512 distro kernel. That saves RAM, and with 
higher CONFIG_NR_CPUS values it adds up quickly.

> > Or the pointer should be attached to the CPU info structure.
> 
> Ok, I have no objection to that, do you want me to make a 
> patch doing that, now that this is already in Linus's tree?

Would be nice if you could do that or some other equivalent 
solution, i'd really not like to see the [CONFIG_NR_CPUS] 
pattern to spread in the kernel, we spent a lot of time getting 
rid of such uses ;-)

Thanks,

	Ingo