[PATCH] x86: fix determination of bit count for struct domain allocations

[PATCH] x86: fix determination of bit count for struct domain allocations

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1383 bytes --]

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@suse.com>

--- 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;
+
+    return bits;
+}
+
 struct domain *alloc_domain_struct(void)
 {
     struct domain *d;
@@ -187,7 +200,10 @@ struct domain *alloc_domain_struct(void)
      * We pack the PDX of the domain structure into a 32-bit field within
      * the page_info structure. Hence the MEMF_bits() restriction.
      */
-    unsigned int bits = 32 + PAGE_SHIFT + pfn_pdx_hole_shift;
+    static unsigned int __read_mostly bits;
+
+    if ( unlikely(!bits) )
+         bits = _domain_struct_bits();
 
     BUILD_BUG_ON(sizeof(*d) > PAGE_SIZE);
     d = alloc_xenheap_pages(0, MEMF_bits(bits));




[-- Attachment #2: x86-domain-struct-bits.patch --]
[-- Type: text/plain, Size: 1446 bytes --]

x86: fix determination of bit count for struct domain allocations

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@suse.com>

--- 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;
+
+    return bits;
+}
+
 struct domain *alloc_domain_struct(void)
 {
     struct domain *d;
@@ -187,7 +200,10 @@ struct domain *alloc_domain_struct(void)
      * We pack the PDX of the domain structure into a 32-bit field within
      * the page_info structure. Hence the MEMF_bits() restriction.
      */
-    unsigned int bits = 32 + PAGE_SHIFT + pfn_pdx_hole_shift;
+    static unsigned int __read_mostly bits;
+
+    if ( unlikely(!bits) )
+         bits = _domain_struct_bits();
 
     BUILD_BUG_ON(sizeof(*d) > PAGE_SIZE);
     d = alloc_xenheap_pages(0, MEMF_bits(bits));

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

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

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 1643 bytes --]

On 14/03/14 15:23, Jan Beulich wrote:
> 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@suse.com>
>
> --- 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)

noinline for debugging purposes?

~Andrew

> +{
> +    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;
> +
> +    return bits;
> +}
> +
>  struct domain *alloc_domain_struct(void)
>  {
>      struct domain *d;
> @@ -187,7 +200,10 @@ struct domain *alloc_domain_struct(void)
>       * We pack the PDX of the domain structure into a 32-bit field within
>       * the page_info structure. Hence the MEMF_bits() restriction.
>       */
> -    unsigned int bits = 32 + PAGE_SHIFT + pfn_pdx_hole_shift;
> +    static unsigned int __read_mostly bits;
> +
> +    if ( unlikely(!bits) )
> +         bits = _domain_struct_bits();
>  
>      BUILD_BUG_ON(sizeof(*d) > PAGE_SIZE);
>      d = alloc_xenheap_pages(0, MEMF_bits(bits));
>
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 2458 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[Jan Beulich]

>>> On 14.03.14 at 16:30, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> On 14/03/14 15:23, Jan Beulich wrote:
>> 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@suse.com>
>>
>> --- 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)
> 
> noinline for debugging purposes?

No, for it to really end up in .init.text (as the caller is in .text).

Jan

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

[Andrew Cooper]

On 14/03/14 15:46, Jan Beulich wrote:
>>>> On 14.03.14 at 16:30, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> On 14/03/14 15:23, Jan Beulich wrote:
>>> 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@suse.com>
>>>
>>> --- 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)
>> noinline for debugging purposes?
> No, for it to really end up in .init.text (as the caller is in .text).
>
> Jan
>

In which case it should assert/bug if the function returns 0, to be sure
we will never fall into the unlikely case again and try to call it from
a non-init function.  Also a comment to explain this.

~Andrew

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

[Jan Beulich]

>>> On 14.03.14 at 16:51, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> On 14/03/14 15:46, Jan Beulich wrote:
>>>>> On 14.03.14 at 16:30, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>> On 14/03/14 15:23, Jan Beulich wrote:
>>>> 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@suse.com>
>>>>
>>>> --- 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)
>>> noinline for debugging purposes?
>> No, for it to really end up in .init.text (as the caller is in .text).
> 
> In which case it should assert/bug if the function returns 0, to be sure
> we will never fall into the unlikely case again and try to call it from
> a non-init function.  Also a comment to explain this.

I really dislike asserting the absolutely obvious: The function can
be very easily proven to only return values in the range [44,64].

Jan

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

[Andrew Cooper]

On 14/03/14 16:12, Jan Beulich wrote:
>>>> On 14.03.14 at 16:51, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> On 14/03/14 15:46, Jan Beulich wrote:
>>>>>> On 14.03.14 at 16:30, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>>> On 14/03/14 15:23, Jan Beulich wrote:
>>>>> 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@suse.com>
>>>>>
>>>>> --- 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)
>>>> noinline for debugging purposes?
>>> No, for it to really end up in .init.text (as the caller is in .text).
>> In which case it should assert/bug if the function returns 0, to be sure
>> we will never fall into the unlikely case again and try to call it from
>> a non-init function.  Also a comment to explain this.
> I really dislike asserting the absolutely obvious: The function can
> be very easily proven to only return values in the range [44,64].
>
> Jan
>

Hmm ok, but at the very least this deserves a comment to that effect.

~Andrew

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

[Keir Fraser]

[-- Attachment #1.1: Type: text/plain, Size: 1873 bytes --]


> Jan Beulich <mailto:JBeulich@suse.com>
> 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@suse.com>
>
> --- 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.

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.

Acked-by: Keir Fraser <keir@xen.org>
> + return bits;
> +}
> +
> struct domain *alloc_domain_struct(void)
> {
> struct domain *d;
> @@ -187,7 +200,10 @@ struct domain *alloc_domain_struct(void)
> * We pack the PDX of the domain structure into a 32-bit field within
> * the page_info structure. Hence the MEMF_bits() restriction.
> */
> - unsigned int bits = 32 + PAGE_SHIFT + pfn_pdx_hole_shift;
> + static unsigned int __read_mostly bits;
> +
> + if ( unlikely(!bits) )
> + bits = _domain_struct_bits();
>
> BUILD_BUG_ON(sizeof(*d) > PAGE_SIZE);
> d = alloc_xenheap_pages(0, MEMF_bits(bits));
>
>
>

[-- Attachment #1.2.1: Type: text/html, Size: 3725 bytes --]

[-- Attachment #1.2.2: compose-unknown-contact.jpg --]
[-- Type: image/jpeg, Size: 770 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[Jan Beulich]

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

>> Jan Beulich <mailto:JBeulich@suse.com>
>> 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@suse.com>
>>
>> --- 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