Re: [Xen-devel] [PATCH] x86: fix determination of bit count for struct domain allocations

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.03.14 at 09:56, <keir.xen@xxxxxxxxx> wrote:

>> Jan Beulich <mailto:JBeulich@xxxxxxxx>
>> 14 March 2014 15:23
>> We can't just add in the hole shift value, as the hole may be at or
>> above the 44-bit boundary. Instead we need to determine the total bit
>> count until reaching 32 significant (not squashed out) bits in PFN
>> representations.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -180,6 +180,19 @@ void dump_pageframe_info(struct domain *
>> spin_unlock(&d->page_alloc_lock);
>> }
>>
>> +static unsigned int __init noinline _domain_struct_bits(void)
>> +{
>> + unsigned int bits = 32 + PAGE_SHIFT;
>> + unsigned int sig = hweight32(~pfn_hole_mask);
>> + unsigned int mask = pfn_hole_mask >> 32;
>> +
>> + for ( ; bits < BITS_PER_LONG && sig < 32; ++bits, mask >>= 1 )
>> + if ( !(mask & 1) )
>> + ++sig;
> 
> This logic took a few looks to be sure what it was doing, let alone 
> correct. Without the patch comment I'd be even a bit more lost. I think 
> the changeset comment, or similar, should be included as a code comment 
> for this new function.

In which case I'll probably also add something along the lines of what
Andrew had asked for.

> I'm sure given there is only one hole there is a simpler form of this 
> based on checking whether bottom_shift > 44. But this is more general I 
> suppose, so probably better despite being harder to understand.

Actually I noticed this being wrong in the context of generalizing
the PFN compaction logic (pending people having access to suitable
hardware to verify those changes), which among other things will
allow for multiple holes.

Jan


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