lkml.org 
[lkml]   [2008]   [Jun]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] i386: fix vmalloc_sync_all() for Xen
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



\
 
 \ /
  Last update: 2008-06-20 18:13    [W:0.220 / U:0.076 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site