[PATCH RFC] domain: Compile with lock_profile=y enabled.

[PATCH RFC] domain: Compile with lock_profile=y enabled.

[Konrad Rzeszutek Wilk]

Our 'struct domain' has when lock profiling is enabled is bigger than
one page.

We can't use vmap nor vzalloc as both of those stash the
physical address in struct page which makes the assumptions
in 'arch_init_memory' trip over ASSERTs.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/arch/x86/domain.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 4420cfc..a85b994 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -237,6 +237,7 @@ struct domain *alloc_domain_struct(void)
 #ifdef CONFIG_BIGMEM
     const unsigned int bits = 0;
 #else
+    int order = get_order_from_bytes(sizeof(*d));
     /*
      * We pack the PDX of the domain structure into a 32-bit field within
      * the page_info structure. Hence the MEMF_bits() restriction.
@@ -247,10 +248,12 @@ struct domain *alloc_domain_struct(void)
          bits = _domain_struct_bits();
 #endif
 
-    BUILD_BUG_ON(sizeof(*d) > PAGE_SIZE);
-    d = alloc_xenheap_pages(0, MEMF_bits(bits));
+    d = alloc_xenheap_pages(order, MEMF_bits(bits));
     if ( d != NULL )
-        clear_page(d);
+    {
+        for ( ; order >= 0; order-- )
+            clear_page((void *)d + PAGE_SIZE*order);
+    }
     return d;
 }
 
-- 
2.1.0

Re: [PATCH RFC] domain: Compile with lock_profile=y enabled.

[Jan Beulich]

>>> On 02.11.15 at 18:12, <konrad.wilk@oracle.com> wrote:
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -237,6 +237,7 @@ struct domain *alloc_domain_struct(void)
>  #ifdef CONFIG_BIGMEM
>      const unsigned int bits = 0;
>  #else
> +    int order = get_order_from_bytes(sizeof(*d));

unsigned int

> @@ -247,10 +248,12 @@ struct domain *alloc_domain_struct(void)
>           bits = _domain_struct_bits();
>  #endif
>  
> -    BUILD_BUG_ON(sizeof(*d) > PAGE_SIZE);

Not unconditionally (i.e. at least non-debug builds should continue
to have this).

> -    d = alloc_xenheap_pages(0, MEMF_bits(bits));
> +    d = alloc_xenheap_pages(order, MEMF_bits(bits));
>      if ( d != NULL )
> -        clear_page(d);
> +    {
> +        for ( ; order >= 0; order-- )
> +            clear_page((void *)d + PAGE_SIZE*order);

This loop works for orders 0 and 1, but not anything else (not
clearing all of the pages).

Jan

Re: [PATCH RFC] domain: Compile with lock_profile=y enabled.

[Konrad Rzeszutek Wilk]

On Thu, Nov 05, 2015 at 10:12:26AM -0700, Jan Beulich wrote:
> >>> On 02.11.15 at 18:12, <konrad.wilk@oracle.com> wrote:
> > --- a/xen/arch/x86/domain.c
> > +++ b/xen/arch/x86/domain.c
> > @@ -237,6 +237,7 @@ struct domain *alloc_domain_struct(void)
> >  #ifdef CONFIG_BIGMEM
> >      const unsigned int bits = 0;
> >  #else
> > +    int order = get_order_from_bytes(sizeof(*d));
> 
> unsigned int
> 
> > @@ -247,10 +248,12 @@ struct domain *alloc_domain_struct(void)
> >           bits = _domain_struct_bits();
> >  #endif
> >  
> > -    BUILD_BUG_ON(sizeof(*d) > PAGE_SIZE);
> 
> Not unconditionally (i.e. at least non-debug builds should continue
> to have this).

The profile=y does not mandate debug=y?

Meaning if we leave this for non-debug builds with lock_profile=y
we will have an compile issue.
> 
> > -    d = alloc_xenheap_pages(0, MEMF_bits(bits));
> > +    d = alloc_xenheap_pages(order, MEMF_bits(bits));
> >      if ( d != NULL )
> > -        clear_page(d);
> > +    {
> > +        for ( ; order >= 0; order-- )
> > +            clear_page((void *)d + PAGE_SIZE*order);
> 
> This loop works for orders 0 and 1, but not anything else (not
> clearing all of the pages).

Right. This below should do it:

>From 2a611285161fa94ce6a8c2708ac1c141ed6f0830 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Fri, 6 Nov 2015 16:05:20 -0500
Subject: [PATCH] domain: Compile with lock_profile=y enabled.

Our 'struct domain' has when lock profiling is enabled is bigger than
one page.

We can't use vmap nor vzalloc as both of those stash the
physical address in struct page which makes the assumptions
in 'arch_init_memory' trip over ASSERTs.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/arch/x86/domain.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index fe3be30..96a79e4 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -233,6 +233,7 @@ struct domain *alloc_domain_struct(void)
 #ifdef CONFIG_BIGMEM
     const unsigned int bits = 0;
 #else
+    unsigned int order = get_order_from_bytes(sizeof(*d));
     /*
      * We pack the PDX of the domain structure into a 32-bit field within
      * the page_info structure. Hence the MEMF_bits() restriction.
@@ -243,10 +244,18 @@ struct domain *alloc_domain_struct(void)
          bits = _domain_struct_bits();
 #endif
 
+
+#ifndef LOCK_PROFILE
     BUILD_BUG_ON(sizeof(*d) > PAGE_SIZE);
-    d = alloc_xenheap_pages(0, MEMF_bits(bits));
+#endif
+    d = alloc_xenheap_pages(order, MEMF_bits(bits));
     if ( d != NULL )
-        clear_page(d);
+    {
+        unsigned int sz;
+
+        for ( sz = 0; sz < (1 << order) * PAGE_SIZE; sz += PAGE_SIZE )
+            clear_page((void *)d + sz);
+    }
     return d;
 }
 
-- 
2.1.0

> 
> Jan
> 

Re: [PATCH RFC] domain: Compile with lock_profile=y enabled.

[Jan Beulich]

>>> On 06.11.15 at 20:39, <konrad.wilk@oracle.com> wrote:
> On Thu, Nov 05, 2015 at 10:12:26AM -0700, Jan Beulich wrote:
>> >>> On 02.11.15 at 18:12, <konrad.wilk@oracle.com> wrote:
>> > @@ -247,10 +248,12 @@ struct domain *alloc_domain_struct(void)
>> >           bits = _domain_struct_bits();
>> >  #endif
>> >  
>> > -    BUILD_BUG_ON(sizeof(*d) > PAGE_SIZE);
>> 
>> Not unconditionally (i.e. at least non-debug builds should continue
>> to have this).
> 
> The profile=y does not mandate debug=y?
> 
> Meaning if we leave this for non-debug builds with lock_profile=y
> we will have an compile issue.

Good point, but still calling for this not to be done unconditionally
(even if not a true dependency, I don't expect production builds
to enable lock profiling).

>> > -    d = alloc_xenheap_pages(0, MEMF_bits(bits));
>> > +    d = alloc_xenheap_pages(order, MEMF_bits(bits));
>> >      if ( d != NULL )
>> > -        clear_page(d);
>> > +    {
>> > +        for ( ; order >= 0; order-- )
>> > +            clear_page((void *)d + PAGE_SIZE*order);
>> 
>> This loop works for orders 0 and 1, but not anything else (not
>> clearing all of the pages).
> 
> Right. This below should do it:

Yes, except that ...

> @@ -243,10 +244,18 @@ struct domain *alloc_domain_struct(void)
>           bits = _domain_struct_bits();
>  #endif
>  
> +
> +#ifndef LOCK_PROFILE
>      BUILD_BUG_ON(sizeof(*d) > PAGE_SIZE);
> -    d = alloc_xenheap_pages(0, MEMF_bits(bits));
> +#endif
> +    d = alloc_xenheap_pages(order, MEMF_bits(bits));
>      if ( d != NULL )
> -        clear_page(d);
> +    {
> +        unsigned int sz;
> +
> +        for ( sz = 0; sz < (1 << order) * PAGE_SIZE; sz += PAGE_SIZE )

... this wants to be simplified to PAGE_SIZE << order.

Jan