Re: [Xen-devel] [PATCH 1/2] x86/p2m: simplify p2m_next_level()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.06.17 at 17:20, <george.dunlap@xxxxxxxxxx> wrote:
> On 21/06/17 11:25, Jan Beulich wrote:
>> Calculate entry PFN and flags just once, making the respective
>> variables (and also pg) function wide. Take the opportunity and also
>> make the induction variable unsigned.
> 
> Hmm -- I'm a fan of keeping the scope of variables limited to where
> they're actually used, to make sure stale values don't end up being
> used.  pg in particular I think should be kept local to the if()
> statements; there's absolutely no benefit to making it function-wide.  I
> don't really see much benefit in making 'flags' and 'pfn' function-wide
> either; it's not easier to read, IMHO, it just might save a tiny bit of
> extra code, at the expense of removing some safety.

I share your desire to restrict variable scope, with the difference
that I think in the common case there shouldn't be multiple
identically named variables in the same function.

As to flags and pfn - they _are_ actually the same value in both
if()-s, so other than for pg there's no risk of using stale values.
With that it's hard to see for me why they should be fetched
more than once in the function. To me this _is_ making the code
easier to read. (In fact you widen the scope of both mentioned
variables already anyway, and for pfn further widening would
indeed no longer be necessary. For flags, though, I think it
should still be pulled out to avoid the recurring l1e_get_flags().)

Otoh it's not clear to me why, with your argumentation, other
function wide variables (i, p2m_entry, new_entry, and perhaps
more) would remain to be such.

> If you want to avoid code duplication, it seems like merging the two
> if() statements (of which at most one can be true) would be a better
> approach.
> 
> Something like the attached (build-tested only)?

With the above in mind (as additional adjustments to make), that's
certainly also an approach to take. Two cosmetic remarks though:
You appear to both leave in place and introduce anew trailing white
space, and neither flags nor level have a need to be unsigned long.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel