Xen devel - Jul 2008 - [PATCH 1/4] hvm: NUMA guest: extend memops hypercall

This patch extends the memops hypercall in a compatible way to transport
a desired NUMA node number. The address_bits field will be limited to 8
bits and is now embedded in the mem_flags member, which additionally
contains the node number (limited to 8 bit). Passing a node number of
''0'' (currently the default) will revert to automatic node
selection
(based on currently scheduled node).

Signed-off-by: Andre Przywara <andre.przywara@amd.com>

-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 277-84917
----to satisfy European Law for business letters:
AMD Saxony Limited Liability Company & Co. KG,
Wilschdorfer Landstr. 101, 01109 Dresden, Germany
Register Court Dresden: HRA 4896, General Partner authorized
to represent: AMD Saxony LLC (Wilmington, Delaware, US)
General Manager of AMD Saxony LLC: Dr. Hans-R. Deppe, Thomas McCoy


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

On 4/7/08 08:57, "Andre Przywara" <andre.przywara@amd.com>
wrote:
> This patch extends the memops hypercall in a compatible way to transport
> a desired NUMA node number. The address_bits field will be limited to 8
> bits and is now embedded in the mem_flags member, which additionally
> contains the node number (limited to 8 bit). Passing a node number of
> ''0'' (currently the default) will revert to automatic node
selection
> (based on currently scheduled node).
> 
> Signed-off-by: Andre Przywara <andre.przywara@amd.com>
Should unprivileged domUs be allowed to specify the NUMA node they allocate
from, regardless of whether they even run there? Seems like a breakage of
guest isolation to me.

 -- Keir



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

Keir Fraser wrote:
> On 4/7/08 08:57, "Andre Przywara" <andre.przywara@amd.com>
wrote:
> 
>> This patch extends the memops hypercall in a compatible way to
transport
>> a desired NUMA node number. The address_bits field will be limited to 8
>> bits and is now embedded in the mem_flags member, which additionally
>> contains the node number (limited to 8 bit). Passing a node number of
>> ''0'' (currently the default) will revert to automatic
node selection
>> (based on currently scheduled node).
> 
> Should unprivileged domUs be allowed to specify the NUMA node they allocate
> from, regardless of whether they even run there? Seems like a breakage of
> guest isolation to me.
Good catch (I code in HVM land most of the time), I hope this small 
(attached) patch fixes this.

Signed-off-by: Andre Przywara <andre.przywara@amd.com>

Regards,
Andre.

-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 277-84917
----to satisfy European Law for business letters:
AMD Saxony Limited Liability Company & Co. KG,
Wilschdorfer Landstr. 101, 01109 Dresden, Germany
Register Court Dresden: HRA 4896, General Partner authorized
to represent: AMD Saxony LLC (Wilmington, Delaware, US)
General Manager of AMD Saxony LLC: Dr. Hans-R. Deppe, Thomas McCoy


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

On 4/7/08 12:14, "Andre Przywara" <andre.przywara@amd.com>
wrote:
>> Should unprivileged domUs be allowed to specify the NUMA node they
allocate
>> from, regardless of whether they even run there? Seems like a breakage
of
>> guest isolation to me.
> Good catch (I code in HVM land most of the time), I hope this small
> (attached) patch fixes this.
Looking some more, I still don''t see that this patch can work.
Don''t the
subfunctions in memory.c go and OR in MEMF_node() values on top of what the
caller may have specified??

 -- Keir



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

Keir Fraser wrote:
> On 4/7/08 12:14, "Andre Przywara" <andre.przywara@amd.com>
wrote:
> 
>>> Should unprivileged domUs be allowed to specify the NUMA node they
allocate
>>> from, regardless of whether they even run there? Seems like a
breakage of
>>> guest isolation to me.
>> Good catch (I code in HVM land most of the time), I hope this small
>> (attached) patch fixes this.
> 
> Looking some more, I still don''t see that this patch can work.
Don''t the
> subfunctions in memory.c go and OR in MEMF_node() values on top of what the
> caller may have specified??
Maybe I don''t get your question right, but the only part where the 
caller specified node number is used is the line I handled in the last 
patch. Later they only use the member memflags of struct memop_args, not 
struct xen_memory_reservation. If the node number is not specified (or 
blocked), it will be later determined by looking at the current 
scheduled pCPU (and thus node), but this is the current behavior anyway.

Regards,
Andre.

-- 
Andre Przywara
AMD-OSRC (Dresden)
Tel: x84917


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

On 4/7/08 13:48, "Andre Przywara" <andre.przywara@amd.com>
wrote:
>> Looking some more, I still don''t see that this patch can work.
Don''t the
>> subfunctions in memory.c go and OR in MEMF_node() values on top of what
the
>> caller may have specified??
> Maybe I don''t get your question right, but the only part where the
> caller specified node number is used is the line I handled in the last
> patch. Later they only use the member memflags of struct memop_args, not
> struct xen_memory_reservation. If the node number is not specified (or
> blocked), it will be later determined by looking at the current
> scheduled pCPU (and thus node), but this is the current behavior anyway.
Take common/memory.c:populate_physmap() as a specific example. It
unconditionally specifies MEMF_node() in its invocation of
alloc_domheap_pages(), regardless of whether its caller has already
specified a node in the memop_args structure that is passed into it.

 -- Keir



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

Keir Fraser wrote:
> On 4/7/08 13:48, "Andre Przywara" <andre.przywara@amd.com>
wrote:
> 
>>> Looking some more, I still don''t see that this patch can
work. Don''t the
>>> subfunctions in memory.c go and OR in MEMF_node() values on top of
what the
>>> caller may have specified??
>> Maybe I don''t get your question right, but the only part where
the
>> caller specified node number is used is the line I handled in the last
>> patch. Later they only use the member memflags of struct memop_args,
not
>> struct xen_memory_reservation. If the node number is not specified (or
>> blocked), it will be later determined by looking at the current
>> scheduled pCPU (and thus node), but this is the current behavior
anyway.
> 
> Take common/memory.c:populate_physmap() as a specific example. It
> unconditionally specifies MEMF_node() in its invocation of
> alloc_domheap_pages(), regardless of whether its caller has already
> specified a node in the memop_args structure that is passed into it.
Have you applied the patches correctly? From 02_numa_guest.patch:
@@ -115,7 +113,7 @@
              goto out;

          page = alloc_domheap_pages(
-            d, a->extent_order, a->memflags | MEMF_node(node));
+            d, a->extent_order, a->memflags);
          if ( unlikely(page == NULL) )
          {
              gdprintk(XENLOG_INFO, "Could not allocate order=%d
extent:"

The other use of MEMF_node is in exchange_memory, which is not given any 
NUMA node info from the caller, so this is correct.

Regards,
Andre.

-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 277-84917
----to satisfy European Law for business letters:
AMD Saxony Limited Liability Company & Co. KG,
Wilschdorfer Landstr. 101, 01109 Dresden, Germany
Register Court Dresden: HRA 4896, General Partner authorized
to represent: AMD Saxony LLC (Wilmington, Delaware, US)
General Manager of AMD Saxony LLC: Dr. Hans-R. Deppe, Thomas McCoy


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

On 4/7/08 16:28, "Andre Przywara" <andre.przywara@amd.com>
wrote:
> Have you applied the patches correctly? From 02_numa_guest.patch:
> @@ -115,7 +113,7 @@
>               goto out;
> 
>           page = alloc_domheap_pages(
> -            d, a->extent_order, a->memflags | MEMF_node(node));
> +            d, a->extent_order, a->memflags);
>           if ( unlikely(page == NULL) )
>           {
>               gdprintk(XENLOG_INFO, "Could not allocate order=%d
extent:"
> 
> The other use of MEMF_node is in exchange_memory, which is not given any
> NUMA node info from the caller, so this is correct.
When sent a patch sequence I expect the patches to apply and work
independently (when applied one-by-one in order).

Anyway, your second patch changes the default NUMA allocation policy from
allocate on home node for the domain to allocate on node I''m currently
executing on. That would seem a net loss for PV guests (whose builder will
not be explicitly specifying the numa node for allocations).

 -- Keir



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

Keir Fraser wrote:
> On 4/7/08 16:28, "Andre Przywara" <andre.przywara@amd.com>
wrote:
> 
>> Have you applied the patches correctly? From 02_numa_guest.patch:
>> @@ -115,7 +113,7 @@
>>               goto out;
>>
>>           page = alloc_domheap_pages(
>> -            d, a->extent_order, a->memflags | MEMF_node(node));
>> +            d, a->extent_order, a->memflags);
>>           if ( unlikely(page == NULL) )
>>           {
>>               gdprintk(XENLOG_INFO, "Could not allocate order=%d
extent:"
>>
>> The other use of MEMF_node is in exchange_memory, which is not given
any
>> NUMA node info from the caller, so this is correct.
> 
> When sent a patch sequence I expect the patches to apply and work
> independently (when applied one-by-one in order).
IMHO that is what they do (beside the below issue patch 1 and 2 are more 
or less refactoring without functional changes), but anyway...
> Anyway, your second patch changes the default NUMA allocation policy from
> allocate on home node for the domain to allocate on node I''m
currently
> executing on. That would seem a net loss for PV guests (whose builder will
> not be explicitly specifying the numa node for allocations).
Right you are, I have missed the subtle difference between both (the 
code isn''t as clear as your sentence). The below patch should fix this 
(by catching NUMA_NO_NODE while still knowing struct domain*).
If there are no further issues, I will resend the patches.

Regards,
Andre.

-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 277-84917
----to satisfy European Law for business letters:
AMD Saxony Limited Liability Company & Co. KG,
Wilschdorfer Landstr. 101, 01109 Dresden, Germany
Register Court Dresden: HRA 4896, General Partner authorized
to represent: AMD Saxony LLC (Wilmington, Delaware, US)
General Manager of AMD Saxony LLC: Dr. Hans-R. Deppe, Thomas McCoy


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