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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 22/06/17 08:25, Jan Beulich wrote:
>>>> 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 I said, one of my reasons for restricting variable scope is to make
sure that stale values don't get reused.  Having multiple instances of
identically named variables used for different purposes is explicitly a
case where the risk of stale values being reused is high.

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

If either `if` clause is executed, then the value of `flags` and `pfn`
will be stale after that clause, since p2m_entry will have changed.

As it happens, in the current code this doesn't matter because if the
first `if` statement is executed, then the second one won't be (since
`type` doesn't change); and since `flags` and `pfn` are only used in the
two `if` clauses, then they won't be reused.

But one could imagine a situation where someone adds some code below the
second `if` clause which uses those values, and is is written with the
assumption that no superpage is broken up; but if a superpage *is*
broken up, would use the stale values.

Keeping the scope local would prevent that.

> (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.

[snip]

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

Well I mostly wrote it to make sure the suggestion would actually work,
and to make it clear what I was suggesting.  I didn't intend to take
over this clean-up work.

 -George


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