Messages in this thread | | | Date | Fri, 20 Jun 2008 09:10:07 -0700 | From | Jeremy Fitzhardinge <> | Subject | Re: [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
| |