Re: [Xen-devel] [PATCH tip/x86/mm] x86_32: calculate additional memory needed by the fixmap

Re: [Xen-devel] [PATCH tip/x86/mm] x86_32: calculate additional memory needed by the fixmap

[Ian Jackson]

stefano.stabellini@eu.citrix.com writes ("[Xen-devel] [PATCH tip/x86/mm] x86_32: calculate additional memory needed by the fixmap"):
> Unfortunately this code is rather complex and depends on the behaviour
> of other functions but I hope to have covered all the corner cases.

I'm sorry to say that think this is really the wrong approach.
I agree that *some* change needs to be made, as this is currently a
serious regression in tip/x86/mm.

But the correct change is in fact to undo the reversion of
  "x86,xen: introduce x86_init.mapping.pagetable_reserve"
That was a hook with a reasonable and defined interface.

What we are having instead, now, is a fragile piece of code that tries
to second-guess the complex config- and hardware-dependent
memory-allocation behaviour of the rest of the startup code.  This is
a recipe for making things break.

Indeed, since the reversion of "mapping.pagetable_reserve" and its
replacement with the new "exact calculation" code, tip/x86/mm has
already had to have two separated config-dependent fixes - and the
thing hasn't even been pushed to Linus's tip yet !

This is a hopelessly fragile approach.  We should go back to the new
pvop.  If the interface documentation in 279b706bf800b596 is not
satisfactory I would be happy to help improve it.  To be honest I
think much of the contents of the commit message should be in comments
in the code.

Indeed, I agree that the current lack of coherent semantic
specification for the pvops is a problem.  But the solution is not to
abolish pvops in favour of fragile duplication of logic.  The solution
is to fix the specification comments.

But, if the x86 maintainers absolutely won't have the new pvop then
this new second-guessing code, fragile as it is, has to go in to fix
the regression.

Thanks,
Ian.

Re: [Xen-devel] [PATCH tip/x86/mm] x86_32: calculate additional memory needed by the fixmap

[Ian Jackson]

stefano.stabellini@eu.citrix.com writes ("[Xen-devel] [PATCH tip/x86/mm] x86_32: calculate additional memory needed by the fixmap"):
> Unfortunately this code is rather complex and depends on the behaviour
> of other functions but I hope to have covered all the corner cases.

I'm sorry to say that think this is really the wrong approach.
I agree that *some* change needs to be made, as this is currently a
serious regression in tip/x86/mm.

But the correct change is in fact to undo the reversion of
  "x86,xen: introduce x86_init.mapping.pagetable_reserve"
That was a hook with a reasonable and defined interface.

What we are having instead, now, is a fragile piece of code that tries
to second-guess the complex config- and hardware-dependent
memory-allocation behaviour of the rest of the startup code.  This is
a recipe for making things break.

Indeed, since the reversion of "mapping.pagetable_reserve" and its
replacement with the new "exact calculation" code, tip/x86/mm has
already had to have two separated config-dependent fixes - and the
thing hasn't even been pushed to Linus's tip yet !

This is a hopelessly fragile approach.  We should go back to the new
pvop.  If the interface documentation in 279b706bf800b596 is not
satisfactory I would be happy to help improve it.  To be honest I
think much of the contents of the commit message should be in comments
in the code.

Indeed, I agree that the current lack of coherent semantic
specification for the pvops is a problem.  But the solution is not to
abolish pvops in favour of fragile duplication of logic.  The solution
is to fix the specification comments.

But, if the x86 maintainers absolutely won't have the new pvop then
this new second-guessing code, fragile as it is, has to go in to fix
the regression.

Thanks,
Ian.

Re: [Xen-devel] [PATCH tip/x86/mm] x86_32: calculate additional memory needed by the fixmap

[Konrad Rzeszutek Wilk]

On Mon, Jul 18, 2011 at 05:29:21PM +0100, Ian Jackson wrote:
> stefano.stabellini@eu.citrix.com writes ("[Xen-devel] [PATCH tip/x86/mm] x86_32: calculate additional memory needed by the fixmap"):
> > Unfortunately this code is rather complex and depends on the behaviour
> > of other functions but I hope to have covered all the corner cases.
> 
> I'm sorry to say that think this is really the wrong approach.
> I agree that *some* change needs to be made, as this is currently a
> serious regression in tip/x86/mm.

tip/x86/mm should boot with this patch, right?

> 
> But the correct change is in fact to undo the reversion of
>   "x86,xen: introduce x86_init.mapping.pagetable_reserve"
> That was a hook with a reasonable and defined interface.
> 
> What we are having instead, now, is a fragile piece of code that tries
> to second-guess the complex config- and hardware-dependent
> memory-allocation behaviour of the rest of the startup code.  This is
> a recipe for making things break.

Stefano did an excellent job at doing an archaeological excavation
and finding all the little pieces and bringing them up to surface
via this function. We have now the full accounting of the early
bootup code and its page table creation. And based on that we can
tighten the dance between pgt_end/pgt_begin/pgt_top to WARN much
sooner - and I hope make the code more simpler.

Sure there are some bugs that we won't find until more folks start
testing it - I've only found this one b/c of playing with a bigger
set of .config variants. But that is part of the 3.1-rcX cycle - to
find them and fix them. Or if we cannot fix them, then revert this
whole patchset and think then some more - maybe at the Linux Plumbers
Conference or the Linux Kernel mini-summits.