Re: [PATCH] i386: fix vmalloc_sync_all() for Xen

[Jeremy Fitzhardinge]

Jan Beulich wrote:
> > I think it would be better to separately test whether the vmalloc 
> > mapping is present in the init_mm and skip the syncing loop in that 
> > case, rather than this somewhat convoluted logic to overload the test in 
> > vmalloc_sync_one.
> >     
>
> That's what the x86-64 code does. When I wrote this originally, I tried
> to keep the pre-existing logic as much as possible, so I split out
> vmalloc_sync_one() by mostly moving existing code. I certainly agree
> that this has room for cleaning up (and then possibly including unification
> with x86-64).
>   

Yep.  I think the first step should be to simplify the code to remove 
all the insync/start optimisations and just implement it in a very 
straightforward way.  I can't see any way this code could be even 
slightly performance critical.

> > with respect to any other pgd you pass in.  So I don't think the BUG_ON 
> > will ever fire, and it's unclear what actual logical property it's 
> > testing for.
> >     
>
> My point of adding the BUG_ON() is that in vmalloc_sync_all() it is not
> clear that vmalloc_sync_one() can fail only due to init_mm's page table
> not being appropriately populated. So yes, this BUG_ON() is not
> expected to ever fire - but isn't that a property of all BUG_ON()'s?

Uh, I guess that's one way of putting it.  The other is that it tests 
for an obscure condition that's indirectly related to something that 
might indicate a bug if the code were written differently.  A good 
BUG_ON/assert acts as documentation because it tells the reader about 
the expected logical state at that point, but it needs to be expressed 
in terms which are directly relevant to the algorithm in question.

   J

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