Re: [PATCH v4 1/2] XEN/X86: Improve semantic support for x86_init.mapping.pagetable_reserve

[Attilio Rao]

On 23/08/12 18:14, Thomas Gleixner wrote:
> On Thu, 23 Aug 2012, Attilio Rao wrote:
>    
> > On 23/08/12 16:46, Thomas Gleixner wrote:
> >      
> > > On Wed, 22 Aug 2012, Attilio Rao wrote:
> > >
> > >
> > >        
> > > > - Allow xen_mapping_pagetable_reserve() to handle a start different from
> > > >     pgt_buf_start, but still bigger than it.
> > > >
> > > >          
> > > What's the purpose of this and how is this going to be used and how is
> > > it useful at all?
> > >
> > >        
> > (Just replying here as all the other your comments are derivative)
> > Looks like you are missing the whole point of the patch.
> > What the patch is supposed to do is just to "enforce a correct semantic for
> > the setup function" and not fix an actual problem found in the code.
> > This means that after the patch you know exactly what expect in terms of
> > semantic by the function and the callers will work following it.
> >
> > Otherwise, what could happen is that if one day for a reason or another begin
> > start being different from pgt_buf_start this function will just break
> > silently, reintroducing the original problem in a different form.
> >      
> Which original problem?
>
>    

This one:

http://marc.info/?l=linux-kernel&m=129901609503574
http://marc.info/?l=linux-kernel&m=130133909408229

and more specifically the one fixed in:
commit 279b706bf800b5967037f492dbe4fc5081ad5d0f
Author: Stefano Stabellini<stefano.stabellini@xxxxxxxxxxxxx>
Date:   Thu Apr 14 15:49:41 2011 +0100

    x86,xen: introduce x86_init.mapping.pagetable_reserve


> > I think this was clarified by the 0/2 but evidently you completely missed it.
> >      
> No, I did not miss it. And it's still not telling what the whole thing
> is about.
>
> There is no reason why start should ever be different. So your whole
> argument is that someone might change the call site of
> x86_init.mapping.pagetable_reserve().
>    

My actual reason is that I want a clear semantic for this function and 
enforce it.

> And then you tell in 1/2 changelog:
>
>   - Allow xen_mapping_pagetable_reserve() to handle a start different from
>     pgt_buf_start, but still bigger than it.
>
> without giving a rationale why this is necessary and why this can ever
> happen. It's wrong to begin with to feed that function something else
> than pgt_buf_start, period.
>    
> Don't misunderstand me. I'm not against documenting and not against
> making code safe and less error prone, but we do not add checks just
> because some moron might change the callee arguments to random
> nonsense or because some tinkerer might use the same function for
> something else.
>
> Following your argumentation we'd need to plaster the kernel code with
> sanity checks. This is not a random Java API used by a gazillion of
> code monkeys; it's low level architecture code and not a driver
> API. People who touch that code should better know what they are
> doing.
>    

You seriously think that adding a single-check, that will be certainly 
skipped (now), in a boot-time function is going to add any performance 
burden?

> What you are doing is actively wrong. You suggest that it's fine to
> call that function with something different than pgt_buf_start as the
> start argument. That's complete nonsense. The early pages are
> allocated bottom up beginning at pgt_buf_start. So what the heck would
> make it sane to change that argument ever?
>    

If you really don't like this approach, at this point I think the best 
thing to do is to assume that the start address will be pgt_buf_start 
and loose the starting argument at all.
If you agree I can make a patch for that.

Attilio
--
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/