[RFC PATCH 1/2] xen: page_alloc: introduce alloc_domheap_pages_nodemask()

[RFC PATCH 1/2] xen: page_alloc: introduce alloc_domheap_pages_nodemask()

[Bob Liu]

Introduce alloc_domheap_pages_nodemask() to allow specification of which node(s)
to allocate memory from even when 'd == NULL' is true.

Signed-off-by: Bob Liu <bob.liu@oracle.com>
---
 xen/common/page_alloc.c |   25 +++++++++++++++++--------
 xen/include/xen/mm.h    |    4 ++++
 2 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 601319c..85e8188 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -561,16 +561,18 @@ static void check_low_mem_virq(void)
 static struct page_info *alloc_heap_pages(
     unsigned int zone_lo, unsigned int zone_hi,
     unsigned int order, unsigned int memflags,
-    struct domain *d)
+    struct domain *d, nodemask_t nodemask)
 {
     unsigned int first_node, i, j, zone = 0, nodemask_retry = 0;
     unsigned int node = (uint8_t)((memflags >> _MEMF_node) - 1);
     unsigned long request = 1UL << order;
     struct page_info *pg;
-    nodemask_t nodemask = (d != NULL ) ? d->node_affinity : node_online_map;
     bool_t need_tlbflush = 0;
     uint32_t tlbflush_timestamp = 0;
 
+    if (d != NULL)
+        nodemask = d->node_affinity;
+
     if ( node == NUMA_NO_NODE )
     {
         memflags &= ~MEMF_exact_node;
@@ -1338,7 +1340,7 @@ void *alloc_xenheap_pages(unsigned int order, unsigned int memflags)
     ASSERT(!in_irq());
 
     pg = alloc_heap_pages(MEMZONE_XEN, MEMZONE_XEN,
-                          order, memflags, NULL);
+                          order, memflags, NULL, node_online_map);
     if ( unlikely(pg == NULL) )
         return NULL;
 
@@ -1490,9 +1492,9 @@ int assign_pages(
     return -1;
 }
 
-
-struct page_info *alloc_domheap_pages(
-    struct domain *d, unsigned int order, unsigned int memflags)
+struct page_info *alloc_domheap_pages_nodemask(
+    struct domain *d, unsigned int order, unsigned int memflags,
+    nodemask_t nodemask)
 {
     struct page_info *pg = NULL;
     unsigned int bits = memflags >> _MEMF_bits, zone_hi = NR_ZONES - 1;
@@ -1505,12 +1507,13 @@ struct page_info *alloc_domheap_pages(
         return NULL;
 
     if ( dma_bitsize && ((dma_zone = bits_to_zone(dma_bitsize)) < zone_hi) )
-        pg = alloc_heap_pages(dma_zone + 1, zone_hi, order, memflags, d);
+        pg = alloc_heap_pages(dma_zone + 1, zone_hi, order, memflags, d,
+                              nodemask);
 
     if ( (pg == NULL) &&
          ((memflags & MEMF_no_dma) ||
           ((pg = alloc_heap_pages(MEMZONE_XEN + 1, zone_hi, order,
-                                  memflags, d)) == NULL)) )
+                                  memflags, d, nodemask)) == NULL)) )
          return NULL;
 
     if ( (d != NULL) && assign_pages(d, pg, order, memflags) )
@@ -1522,6 +1525,12 @@ struct page_info *alloc_domheap_pages(
     return pg;
 }
 
+struct page_info *alloc_domheap_pages(
+    struct domain *d, unsigned int order, unsigned int memflags)
+{
+    return alloc_domheap_pages_nodemask(d, order, memflags, node_online_map);
+}
+
 void free_domheap_pages(struct page_info *pg, unsigned int order)
 {
     struct domain *d = page_get_owner(pg);
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index b183189..9df1137 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -30,6 +30,7 @@
 
 #include <xen/types.h>
 #include <xen/list.h>
+#include <xen/nodemask.h>
 #include <xen/spinlock.h>
 
 struct domain;
@@ -65,6 +66,9 @@ void get_outstanding_claims(uint64_t *free_pages, uint64_t *outstanding_pages);
 void init_domheap_pages(paddr_t ps, paddr_t pe);
 struct page_info *alloc_domheap_pages(
     struct domain *d, unsigned int order, unsigned int memflags);
+struct page_info *alloc_domheap_pages_nodemask(
+    struct domain *d, unsigned int order, unsigned int memflags,
+    nodemask_t nodemask);
 void free_domheap_pages(struct page_info *pg, unsigned int order);
 unsigned long avail_domheap_pages_region(
     unsigned int node, unsigned int min_width, unsigned int max_width);
-- 
1.7.10.4

[RFC PATCH 2/2] xen: tmem: make tmem aware of NUMA affinity

[Bob Liu]

Persistant pages in tmem are allocated by alloc_domheap_pages(domain) which
is consistent with that domain's node affinity. But that's not true for
ephemeral pages, because domain=NULL have to be passed to
alloc_domheap_pages(NULL).

This patch makes use of function alloc_domheap_pages_nodemask() introduced by
previous patch, so that we can still only allocate memory from nodes in
d->node_affinity even when domain=NULL.

Signed-off-by: Bob Liu <bob.liu@oracle.com>
---
 xen/common/tmem.c          |    4 ++--
 xen/include/xen/tmem_xen.h |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/common/tmem.c b/xen/common/tmem.c
index 02e7e2e..0ca50b2 100644
--- a/xen/common/tmem.c
+++ b/xen/common/tmem.c
@@ -303,7 +303,7 @@ static struct page_info *tmem_alloc_page(struct tmem_pool *pool)
     if ( pool != NULL && is_persistent(pool) )
         pfp = __tmem_alloc_page_thispool(pool->client->domain);
     else
-        pfp = __tmem_alloc_page();
+        pfp = __tmem_alloc_page(pool->client->domain);
     if ( pfp == NULL )
         alloc_page_failed++;
     else
@@ -326,7 +326,7 @@ static noinline void *tmem_mempool_page_get(unsigned long size)
     struct page_info *pi;
 
     ASSERT(size == PAGE_SIZE);
-    if ( (pi = __tmem_alloc_page()) == NULL )
+    if ( (pi = __tmem_alloc_page(current->domain)) == NULL )
         return NULL;
     return page_to_virt(pi);
 }
diff --git a/xen/include/xen/tmem_xen.h b/xen/include/xen/tmem_xen.h
index 885ee21..be31aed 100644
--- a/xen/include/xen/tmem_xen.h
+++ b/xen/include/xen/tmem_xen.h
@@ -144,12 +144,12 @@ static inline void __tmem_free_page_thispool(struct page_info *pi)
 /*
  * Memory allocation for ephemeral (non-persistent) data
  */
-static inline struct page_info *__tmem_alloc_page(void)
+static inline struct page_info *__tmem_alloc_page(struct domain *d)
 {
     struct page_info *pi = tmem_page_list_get();
 
     if ( pi == NULL)
-        pi = alloc_domheap_pages(0,0,MEMF_tmem);
+        pi = alloc_domheap_pages_nodemask(0, 0, MEMF_tmem, d->node_affinity);
 
     if ( pi )
         atomic_inc(&freeable_page_count);
-- 
1.7.10.4

Re: [RFC PATCH 2/2] xen: tmem: make tmem aware of NUMA affinity

[Jan Beulich]

>>> On 18.03.14 at 13:25, Bob Liu <lliubbo@gmail.com> wrote:
> Persistant pages in tmem are allocated by alloc_domheap_pages(domain) which
> is consistent with that domain's node affinity. But that's not true for
> ephemeral pages, because domain=NULL have to be passed to
> alloc_domheap_pages(NULL).
> 
> This patch makes use of function alloc_domheap_pages_nodemask() introduced 
> by
> previous patch, so that we can still only allocate memory from nodes in
> d->node_affinity even when domain=NULL.
> 
> Signed-off-by: Bob Liu <bob.liu@oracle.com>

While this may be considered reasonable, I don't think we want to
see feature enhancements to tmem prior to the security audit
having happened that has been outstanding for about 1.5 years
(XSA-15).

Jan

Re: [RFC PATCH 2/2] xen: tmem: make tmem aware of NUMA affinity

[Konrad Rzeszutek Wilk]

On Tue, Mar 18, 2014 at 02:29:34PM +0000, Jan Beulich wrote:
> >>> On 18.03.14 at 13:25, Bob Liu <lliubbo@gmail.com> wrote:
> > Persistant pages in tmem are allocated by alloc_domheap_pages(domain) which
> > is consistent with that domain's node affinity. But that's not true for
> > ephemeral pages, because domain=NULL have to be passed to
> > alloc_domheap_pages(NULL).
> > 
> > This patch makes use of function alloc_domheap_pages_nodemask() introduced 
> > by
> > previous patch, so that we can still only allocate memory from nodes in
> > d->node_affinity even when domain=NULL.
> > 
> > Signed-off-by: Bob Liu <bob.liu@oracle.com>
> 
> While this may be considered reasonable, I don't think we want to
> see feature enhancements to tmem prior to the security audit
> having happened that has been outstanding for about 1.5 years
> (XSA-15).

Hey Jan,

Bob has been going through the code to make it easier to understand
and also easier to analyze. As part of that it has already removed
some of the issues that at least Coverity has identified.

While I concur with your that adding this patch right now
might distract from the security aspects, I hope to have the
Coverity not report any issues for tmem. The posting of cleanup
patches that Bob had done in February addressed a lot of them -
and I finally got through them and are testing them.

Now Coverity is not the end answer to static analysis so we are
looking at using other tools to augment this.

The paper and pen methodology - which is what both me and Bob
had been using has been instrumental in the creation of the
cleanup patches to simplify some of the code paths.

It is not complete, but I hope by Xen 4.5 it will be done.

Would you perhaps reconsider reviewing the code that Bob
posted and ignore the committing part of it until a later time
(when tmem has gotten throught the security audit).

Thanks!
> 
> Jan
> 

Re: [RFC PATCH 1/2] xen: page_alloc: introduce alloc_domheap_pages_nodemask()

[Dario Faggioli]

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

On mar, 2014-03-18 at 20:25 +0800, Bob Liu wrote:
> Introduce alloc_domheap_pages_nodemask() to allow specification of which node(s)
> to allocate memory from even when 'd == NULL' is true.
> 
To me, this sentence would sound better, and more accurate wrt the
implementation below, without the 'even'.

Code wise...

> Signed-off-by: Bob Liu <bob.liu@oracle.com>
> ---
>  xen/common/page_alloc.c |   25 +++++++++++++++++--------
>  xen/include/xen/mm.h    |    4 ++++
>  2 files changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 601319c..85e8188 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -561,16 +561,18 @@ static void check_low_mem_virq(void)
>  static struct page_info *alloc_heap_pages(
>      unsigned int zone_lo, unsigned int zone_hi,
>      unsigned int order, unsigned int memflags,
> -    struct domain *d)
> +    struct domain *d, nodemask_t nodemask)
>  {
>      unsigned int first_node, i, j, zone = 0, nodemask_retry = 0;
>      unsigned int node = (uint8_t)((memflags >> _MEMF_node) - 1);
>      unsigned long request = 1UL << order;
>      struct page_info *pg;
> -    nodemask_t nodemask = (d != NULL ) ? d->node_affinity : node_online_map;
>      bool_t need_tlbflush = 0;
>      uint32_t tlbflush_timestamp = 0;
>  
> +    if (d != NULL)
> +        nodemask = d->node_affinity;
> +
>      if ( node == NUMA_NO_NODE )
>      {
>          memflags &= ~MEMF_exact_node;
>
... If it were me, I probably would have added
alloc_heap_pages_nodemask(...,nodemask_t nodemask) (similarly to what we
do up here, plus the actual renaming to alloc_heap_pages_nodemask() )
and have the 'new' alloc_heap_pages() be a wrapper of that, with

    (d != NULL ) ? d->node_affinity : node_online_map

as the nodemask parameter. The reason is it looks cleaner and easier to
use to me. In particular, what I dislike is this...

> @@ -1338,7 +1340,7 @@ void *alloc_xenheap_pages(unsigned int order, unsigned int memflags)
>      ASSERT(!in_irq());
>  
>      pg = alloc_heap_pages(MEMZONE_XEN, MEMZONE_XEN,
> -                          order, memflags, NULL);
> +                          order, memflags, NULL, node_online_map);
>
... the alloc_heap_pages(...,NULL,node_online_map) call, but that's
mostly a matter of taste, I guess, so if maintainers are fine with the
current approach, I certainly am too.

The only thing I'm a bit concerned about is the fact that, either way,
if one specifies a non NULL domain, the domain's node_affinity is used,
and the nodemask ignored. I'm fine with it, but shouldn't this be a bit
more evident (a comment, some if()/ASSERT, the changelog, etc.)?

BTW, apart from these minor observation, and FWIW, this change looks
fine to me.

Regards,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 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: [RFC PATCH 2/2] xen: tmem: make tmem aware of NUMA affinity

[Jan Beulich]

>>> On 19.03.14 at 18:12, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> Would you perhaps reconsider reviewing the code that Bob
> posted and ignore the committing part of it until a later time
> (when tmem has gotten throught the security audit).

The code looked reasonable, albeit I'm not certain it's to the
point: Aren't certain types of pages sharable between domains?
For those, setting the affinity based on the domain for which
the allocation gets done doesn't seem ideal. Plus the domain
heap use of tmem was found to be broken anyway, and hence
extending the respective allocation interface seems odd from a
second viewing angle too.

On the upside of things I would expect to find further uses for
the extended domain page allocation interface. I'm wondering
though whether we couldn't get away without adding yet
another wrapper function: Considering that the passed in
node mask is ignored if d != NULL, making the first argument
a transparent union and adding _MEMF_nodemask to indicate
that the passed in pointer is to a nodemask_t rather than a
struct domain, we could stay with the current single interface.
Inline type-safe accessors to both variants would then seem
to be the way to go.

Jan

Re: [RFC PATCH 2/2] xen: tmem: make tmem aware of NUMA affinity

[Bob Liu]

On 03/20/2014 05:26 PM, Jan Beulich wrote:
>>>> On 19.03.14 at 18:12, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
>> Would you perhaps reconsider reviewing the code that Bob
>> posted and ignore the committing part of it until a later time
>> (when tmem has gotten throught the security audit).
> 
> The code looked reasonable, albeit I'm not certain it's to the
> point: Aren't certain types of pages sharable between domains?

Yes.

> For those, setting the affinity based on the domain for which
> the allocation gets done doesn't seem ideal. Plus the domain

The ideal situation for tmem is try to allocate pages from nodes in
domain's node_affinity, if there isn't enough memory on those nodes then
fall back to other online nodes.

Do you have any suggestion how to implement this?

> heap use of tmem was found to be broken anyway, and hence

Yes, but I think it's still better if tmem prefers nodes in
domain->node_affinity rather than all online nodes.

> extending the respective allocation interface seems odd from a
> second viewing angle too.
> 
> On the upside of things I would expect to find further uses for
> the extended domain page allocation interface. I'm wondering
> though whether we couldn't get away without adding yet
> another wrapper function: Considering that the passed in
> node mask is ignored if d != NULL, making the first argument
> a transparent union and adding _MEMF_nodemask to indicate
> that the passed in pointer is to a nodemask_t rather than a
> struct domain, we could stay with the current single interface.
> Inline type-safe accessors to both variants would then seem
> to be the way to go.
> 

Thank you for your review!

-- 
Regards,
-Bob

Re: [RFC PATCH 2/2] xen: tmem: make tmem aware of NUMA affinity

[Jan Beulich]

>>> On 20.03.14 at 12:59, Bob Liu <bob.liu@oracle.com> wrote:
> On 03/20/2014 05:26 PM, Jan Beulich wrote:
>>>>> On 19.03.14 at 18:12, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
>>> Would you perhaps reconsider reviewing the code that Bob
>>> posted and ignore the committing part of it until a later time
>>> (when tmem has gotten throught the security audit).
>> 
>> The code looked reasonable, albeit I'm not certain it's to the
>> point: Aren't certain types of pages sharable between domains?
> 
> Yes.
> 
>> For those, setting the affinity based on the domain for which
>> the allocation gets done doesn't seem ideal. Plus the domain
> 
> The ideal situation for tmem is try to allocate pages from nodes in
> domain's node_affinity, if there isn't enough memory on those nodes then
> fall back to other online nodes.
> 
> Do you have any suggestion how to implement this?

Nothing beyond what your wording already says.

>> heap use of tmem was found to be broken anyway, and hence
> 
> Yes, but I think it's still better if tmem prefers nodes in
> domain->node_affinity rather than all online nodes.

But that comment of yours relates to the earlier aspect, not
the last comment I made. If you don't recall what I'm referring
to, just go check xen/arch/x86/setup.c for the condition of
tmem getting forcibly disabled.

Jan