[PATCH] Fix domheap structure allocation when NUMA=on

[PATCH] Fix domheap structure allocation when NUMA=on

[Yang, Xiaowei]

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

DIRECTMAP_VIRT_END can't be passed to virt_to_mfn(), as it's just beyond 
direct map boundary and triggers ASSERT very early at boot time.

Signed-off-by: Xiaowei Yang <xiaowei.yang@intel.com>

[-- Attachment #2: numa.patch --]
[-- Type: text/x-patch, Size: 1130 bytes --]

diff -r 78ed606cb1ab xen/common/page_alloc.c
--- a/xen/common/page_alloc.c	Thu Mar 19 10:57:23 2009 +0800
+++ b/xen/common/page_alloc.c	Thu Mar 19 23:01:32 2009 +0800
@@ -293,7 +297,7 @@ static unsigned long init_node_heap(int 
     }
 #ifdef DIRECTMAP_VIRT_END
     else if ( nr >= needed &&
-              mfn + needed <= virt_to_mfn(DIRECTMAP_VIRT_END) )
+              mfn + needed <= virt_to_mfn(DIRECTMAP_VIRT_END - PAGE_SIZE) )
     {
         _heap[node] = mfn_to_virt(mfn);
         avail[node] = mfn_to_virt(mfn + needed) - sizeof(**avail) * NR_ZONES;
diff -r 78ed606cb1ab xen/include/asm-x86/x86_64/page.h
--- a/xen/include/asm-x86/x86_64/page.h	Thu Mar 19 10:57:23 2009 +0800
+++ b/xen/include/asm-x86/x86_64/page.h	Thu Mar 19 23:00:26 2009 +0800
@@ -40,7 +40,7 @@ static inline unsigned long __virt_to_ma
     ASSERT(va >= XEN_VIRT_START);
     ASSERT(va < DIRECTMAP_VIRT_END);
     ASSERT((va < XEN_VIRT_END) || (va >= DIRECTMAP_VIRT_START));
-    if ( va > DIRECTMAP_VIRT_START )
+    if ( va >= DIRECTMAP_VIRT_START )
         return va - DIRECTMAP_VIRT_START;
     return va - XEN_VIRT_START + xen_phys_start;
 }

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

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

Re: [PATCH] Fix domheap structure allocation when NUMA=on

[Jan Beulich]

>>> "Yang, Xiaowei" <xiaowei.yang@intel.com> 20.03.09 06:05 >>>
>DIRECTMAP_VIRT_END can't be passed to virt_to_mfn(), as it's just beyond 
>direct map boundary and triggers ASSERT very early at boot time.

While I agree to the analysis, I would think that this

+              mfn + needed <= virt_to_mfn(DIRECTMAP_VIRT_END - PAGE_SIZE) )

should rather be

+              mfn + needed <= virt_to_mfn(DIRECTMAP_VIRT_END - 1) + 1 )

Jan

Re: [PATCH] Fix domheap structure allocation when NUMA=on

[Yang, Xiaowei]

Jan Beulich wrote:
>>>> "Yang, Xiaowei" <xiaowei.yang@intel.com> 20.03.09 06:05 >>>
>> DIRECTMAP_VIRT_END can't be passed to virt_to_mfn(), as it's just beyond 
>> direct map boundary and triggers ASSERT very early at boot time.
> 
> While I agree to the analysis, I would think that this
> 
> +              mfn + needed <= virt_to_mfn(DIRECTMAP_VIRT_END - PAGE_SIZE) )
> 
> should rather be
> 
> +              mfn + needed <= virt_to_mfn(DIRECTMAP_VIRT_END - 1) + 1 )
> 
virt_to_mfn(DIRECTMAP_VIRT_END - 1) is equal to 
virt_to_mfn(DIRECTMAP_VIRT_END - PAGE_SIZE). Why +1? We use '<=' here.

Thanks,
xiaowei

Re: [PATCH] Fix domheap structure allocation when NUMA=on

[Jan Beulich]

>>> "Yang, Xiaowei" <xiaowei.yang@intel.com> 20.03.09 09:34 >>>
>Jan Beulich wrote:
>>>>> "Yang, Xiaowei" <xiaowei.yang@intel.com> 20.03.09 06:05 >>>
>>> DIRECTMAP_VIRT_END can't be passed to virt_to_mfn(), as it's just beyond 
>>> direct map boundary and triggers ASSERT very early at boot time.
>> 
>> While I agree to the analysis, I would think that this
>> 
>> +              mfn + needed <= virt_to_mfn(DIRECTMAP_VIRT_END - PAGE_SIZE) )
>> 
>> should rather be
>> 
>> +              mfn + needed <= virt_to_mfn(DIRECTMAP_VIRT_END - 1) + 1 )
>> 
>virt_to_mfn(DIRECTMAP_VIRT_END - 1) is equal to 

Depending on whether DIRECTMAP_VIRT_END is the last byte or the first
following byte. Using "- 1" avoids such a dependency.

>virt_to_mfn(DIRECTMAP_VIRT_END - PAGE_SIZE). Why +1? We use '<=' here.

Because on the left side of the comparison we also calculate the first
following mfn, not the last included one.

Jan

Re: [PATCH] Fix domheap structure allocation when NUMA=on

[Keir Fraser]

On 20/03/2009 08:47, "Jan Beulich" <jbeulich@novell.com> wrote:

> virt_to_mfn(DIRECTMAP_VIRT_END - 1) is equal to
> 
> Depending on whether DIRECTMAP_VIRT_END is the last byte or the first
> following byte. Using "- 1" avoids such a dependency.
> 
>> virt_to_mfn(DIRECTMAP_VIRT_END - PAGE_SIZE). Why +1? We use '<=' here.
> 
> Because on the left side of the comparison we also calculate the first
> following mfn, not the last included one.

Since DIRECTMAP_VIRT_END is always the following byte not the last byte, it
sounds like you think the page_alloc.c chunk is not needed at all. I'll just
check in the header fix for now.

 -- Keir

Re: [PATCH] Fix domheap structure allocation when NUMA=on

[Jan Beulich]

>>> Keir Fraser <keir.fraser@eu.citrix.com> 20.03.09 09:56 >>>
>On 20/03/2009 08:47, "Jan Beulich" <jbeulich@novell.com> wrote:
>
>> virt_to_mfn(DIRECTMAP_VIRT_END - 1) is equal to
>> 
>> Depending on whether DIRECTMAP_VIRT_END is the last byte or the first
>> following byte. Using "- 1" avoids such a dependency.
>> 
>>> virt_to_mfn(DIRECTMAP_VIRT_END - PAGE_SIZE). Why +1? We use '<=' here.
>> 
>> Because on the left side of the comparison we also calculate the first
>> following mfn, not the last included one.
>
>Since DIRECTMAP_VIRT_END is always the following byte not the last byte, it
>sounds like you think the page_alloc.c chunk is not needed at all. I'll just
>check in the header fix for now.

No, that chunk is necessary afaics - as I said I agree to Xiaowei's diagnosis
that virt_to_mfn() should not be used on DIRECTMAP_VIRT_END alone.
Subtracting 1 from it, and then adding back 1 to the result is indeed what
I think is the right thing to do here.

Jan

Re: [PATCH] Fix domheap structure allocation when NUMA=on

[Keir Fraser]

On 20/03/2009 09:27, "Jan Beulich" <jbeulich@novell.com> wrote:

>> Since DIRECTMAP_VIRT_END is always the following byte not the last byte, it
>> sounds like you think the page_alloc.c chunk is not needed at all. I'll just
>> check in the header fix for now.
> 
> No, that chunk is necessary afaics - as I said I agree to Xiaowei's diagnosis
> that virt_to_mfn() should not be used on DIRECTMAP_VIRT_END alone.
> Subtracting 1 from it, and then adding back 1 to the result is indeed what
> I think is the right thing to do here.

Ah... virt_to_mfn() has a discontinuity at DIRECTMAP_VIRT_END. I get you.
I'll apply that chunk as a separate changeset then.

 -- Keir

Re: [PATCH] Fix domheap structure allocation when NUMA=on

[Yang, Xiaowei]

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

Jan Beulich wrote:
>>>> "Yang, Xiaowei" <xiaowei.yang@intel.com> 20.03.09 09:34 >>>
>> Jan Beulich wrote:
>>>>>> "Yang, Xiaowei" <xiaowei.yang@intel.com> 20.03.09 06:05 >>>
>>>> DIRECTMAP_VIRT_END can't be passed to virt_to_mfn(), as it's just beyond 
>>>> direct map boundary and triggers ASSERT very early at boot time.
>>> While I agree to the analysis, I would think that this
>>>
>>> +              mfn + needed <= virt_to_mfn(DIRECTMAP_VIRT_END - PAGE_SIZE) )
>>>
>>> should rather be
>>>
>>> +              mfn + needed <= virt_to_mfn(DIRECTMAP_VIRT_END - 1) + 1 )
>>>
>> virt_to_mfn(DIRECTMAP_VIRT_END - 1) is equal to 
> 
> Depending on whether DIRECTMAP_VIRT_END is the last byte or the first
> following byte. Using "- 1" avoids such a dependency.
> 
>> virt_to_mfn(DIRECTMAP_VIRT_END - PAGE_SIZE). Why +1? We use '<=' here.
> 
> Because on the left side of the comparison we also calculate the first
> following mfn, not the last included one.
> 
Jan,
Thanks for the clarification!

Now I found another potential issue. Since mfn+needed could be equal to 
virt_to_mfn(DIRECTMAP_VIRT_END-1)+1, it can't be passed to mfn_to_virt() 
directly - the valid range is [0,DIRECT_MAP_BYTES>>PAGE_SHIFT).

Here is the patch to fix it. More assert and 32 bit counterpart are added.

Signed-off-by: Xiaowei Yang <xiaowei.yang@intel.com>

Thanks,
xiaowei


[-- Attachment #2: mfn_to_virt.patch --]
[-- Type: text/x-patch, Size: 2559 bytes --]

diff -r 0477f9061c8a xen/common/page_alloc.c
--- a/xen/common/page_alloc.c	Fri Mar 20 17:42:46 2009 +0000
+++ b/xen/common/page_alloc.c	Mon Mar 23 03:04:18 2009 +0800
@@ -302,7 +302,8 @@ static unsigned long init_node_heap(int 
               (mfn + needed) <= (virt_to_mfn(DIRECTMAP_VIRT_END - 1) + 1) )
     {
         _heap[node] = mfn_to_virt(mfn);
-        avail[node] = mfn_to_virt(mfn + needed) - sizeof(**avail) * NR_ZONES;
+        avail[node] = mfn_to_virt(mfn + needed - 1) +
+                      PAGE_SIZE - sizeof(**avail) * NR_ZONES;
     }
 #endif
     else if ( get_order_from_bytes(sizeof(**_heap)) ==
diff -r 0477f9061c8a xen/include/asm-x86/x86_32/page.h
--- a/xen/include/asm-x86/x86_32/page.h	Fri Mar 20 17:42:46 2009 +0000
+++ b/xen/include/asm-x86/x86_32/page.h	Mon Mar 23 03:04:18 2009 +0800
@@ -27,9 +27,6 @@
 #define __PAGE_OFFSET           (0xFF000000)
 #define __XEN_VIRT_START        __PAGE_OFFSET
 
-#define virt_to_maddr(va) ((unsigned long)(va)-DIRECTMAP_VIRT_START)
-#define maddr_to_virt(ma) ((void *)((unsigned long)(ma)+DIRECTMAP_VIRT_START))
-
 #define VADDR_BITS              32
 #define VADDR_MASK              (~0UL)
 
@@ -43,6 +40,22 @@
 
 #include <xen/config.h>
 #include <asm/types.h>
+
+static inline unsigned long __virt_to_maddr(unsigned long va)
+{
+    ASSERT(va >= DIRECTMAP_VIRT_START && va < DIRECTMAP_VIRT_END);
+    return va - DIRECTMAP_VIRT_START;
+}
+#define virt_to_maddr(va)       \
+    (__virt_to_maddr((unsigned long)(va)))
+
+static inline void *__maddr_to_virt(unsigned long ma)
+{
+    ASSERT(ma < DIRECTMAP_VIRT_END - DIRECTMAP_VIRT_START);
+    return (void *)(ma + DIRECTMAP_VIRT_START);
+}
+#define maddr_to_virt(ma)       \
+    (__maddr_to_virt((unsigned long)(ma)))
 
 /* read access (should only be used for debug printk's) */
 typedef u64 intpte_t;
diff -r 0477f9061c8a xen/include/asm-x86/x86_64/page.h
--- a/xen/include/asm-x86/x86_64/page.h	Fri Mar 20 17:42:46 2009 +0000
+++ b/xen/include/asm-x86/x86_64/page.h	Mon Mar 23 03:04:42 2009 +0800
@@ -46,8 +46,14 @@ static inline unsigned long __virt_to_ma
 }
 #define virt_to_maddr(va)       \
     (__virt_to_maddr((unsigned long)(va)))
+
+static inline void *__maddr_to_virt(unsigned long ma)
+{
+    ASSERT(ma < DIRECTMAP_VIRT_END - DIRECTMAP_VIRT_START);
+    return (void *)(ma + DIRECTMAP_VIRT_START);
+}
 #define maddr_to_virt(ma)       \
-    ((void *)((unsigned long)(ma)+DIRECTMAP_VIRT_START))
+    (__maddr_to_virt((unsigned long)(ma)))
 
 /* read access (should only be used for debug printk's) */
 typedef u64 intpte_t;

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

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

Re: [PATCH] Fix domheap structure allocation when NUMA=on

[Jan Beulich]

>>> "Yang, Xiaowei" <xiaowei.yang@intel.com> 24.03.09 11:48 >>>
>Now I found another potential issue. Since mfn+needed could be equal to 
>virt_to_mfn(DIRECTMAP_VIRT_END-1)+1, it can't be passed to mfn_to_virt() 
>directly - the valid range is [0,DIRECT_MAP_BYTES>>PAGE_SHIFT).

Oh, indeed. Thanks for noticing and fixing.

Jan