[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.