Re: [PATCH] x86: Fix hwpoison code related build failure on 32-bitNUMAQ

[Ingo Molnar]

* Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:

> On Sat, 26 Sep 2009, Ingo Molnar wrote:
> > >  
> > > +config X86_SUPPORTS_MEMORY_FAILURE
> > > +	bool
> > > +	depends on !X86_NUMAQ
> > > +	select ARCH_SUPPORTS_MEMORY_FAILURE
> > > +	default y
> > 
> > Thanks Linus, this patch fixed the NUMAQ build problem.
> 
> I think it's slightly buggy still.
> 
> I think the X86_SUPPORTS_MEMORY_FAILURE thing should also have a
> 
> 	depends on X86_MCE
> 
> line, because we still depend on MCE.

Yeah.

( Note, it should not necessarily depend on it: while the only hw 
  mechanism that calls memory_failure() is indeed MCE, the act of having 
  a memory-failures subsystem does not depend on the presence of an x86 
  MCE subsystem. There's for example the injection debug-code which 
  allows the injection of memory_failure() calls. That should work fine 
  without having MCE build in as well.

  But that is a separate change. )

> And as you found out, there's also the sparsemem thing.
> 
> Don't make it one huge ugly thing, just split out the requirements like
> 
> 	depends on X86_MCE
> 	depends on !X86_NUMAQ
> 	depends on X86_64 || !SPARSEMEM
> 
> because I think the requirements are fairly independent, and it makes 
> it easier to read (you could even comment each line on why _that_ 
> particular issue needs to disable X86_SUPPORTS_MEMORY_FAILURE)

Good idea, have done that too.

> But yeah, with that, and some testing, please add my sign-off (or 
> acked-by, if you end up changing the patch so much that it has little 
> to do with my original one)

It's still mostly your patch so i've added your SOB, thanks. Below is 
the updated patch.

btw., i think mm/memory-failure.c needs similar cleanups to 
ARCH_SUPPORTS_MEMORY_FAILURE.

Right now it is full of x86 details, not sure that is right. 'MCE' for 
example is an x86 expression and goes way beyond just memory errors - it 
stands for 'Machine Check Exception' and covers IO/bus errors, etc.

We even put 'MCE' into new ABI details in include/asm-generic/siginfo.h:

/* hardware memory error consumed on a machine check: action required */
#define BUS_MCEERR_AR   (__SI_FAULT|4)
/* hardware memory error detected in process but not consumed: action optional*/
#define BUS_MCEERR_AO   (__SI_FAULT|5)
#define NSIGBUS         5

That should be fixed to be something like:

	 BUS_MEMERR_MANDATORY
	 BUS_MEMERR_OPTIONAL

before such a kernel is released, IMHO.

In mm/memory-failure.c i find the 'ao' / 'ar' abbreviations rather 
unreadable as well - they are totally meaningless (to me at least),
even if i know the code.

Same goes for the me_*() prefixes.

Also, 'struct to_kill' ....

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/