Messages in this thread | | | Date | Thu, 23 Aug 2012 18:13:39 +0100 | From | Attilio Rao <> | Subject | Re: [PATCH v4 1/2] XEN/X86: Improve semantic support for x86_init.mapping.pagetable_reserve |
| |
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@eu.citrix.com> 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
| |