[PATCH v1][RFC] xen balloon driver numa support, libxl interface

[PATCH v1][RFC] xen balloon driver numa support, libxl interface

[Yechen Li]

---

  This small patch implements a numa support of memory operation for libxl
  The command is: xl mem-set-numa [-e] vmid memorysize nodeid
  To pass the parameters to balloon driver in kernel, I add a file of xen-store
as /local/domain/(id)/memory/target_nid, hoping this is ok....

  It's my first time submitting a patch, please point out the problems so that
I could work better in future, thanks very much!

 tools/libxl/libxl.c       | 14 ++++++++++++--
 tools/libxl/libxl.h       |  1 +
 tools/libxl/xl.h          |  1 +
 tools/libxl/xl_cmdimpl.c  | 45 +++++++++++++++++++++++++++++++++++++++++++++
 tools/libxl/xl_cmdtable.c |  7 +++++++
 5 files changed, 66 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 81785df..f027d59 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -3642,10 +3642,17 @@ retry:
     }
     return 0;
 }
-
 int libxl_set_memory_target(libxl_ctx *ctx, uint32_t domid,
         int32_t target_memkb, int relative, int enforce)
 {
+    return libxl_set_memory_target_numa(ctx, domid, target_memkb, relative,
+                enforce, -1, 0);
+}
+
+int libxl_set_memory_target_numa(libxl_ctx *ctx, uint32_t domid,
+        int32_t target_memkb, int relative, int enforce,
+        int node_specify, bool nodeexact)
+{
     GC_INIT(ctx);
     int rc = 1, abort_transaction = 0;
     uint32_t memorykb = 0, videoram = 0;
@@ -3754,7 +3761,10 @@ retry_transaction:
         abort_transaction = 1;
         goto out;
     }
-
+    //lcc:
+    LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "target_nid = %d focus= %d", node_specify, (int) nodeexact);
+    libxl__xs_write(gc, t, libxl__sprintf(gc, "%s/memory/target_nid",
+                dompath), "%"PRId32" %"PRId32, node_specify, (int)nodeexact);
     libxl__xs_write(gc, t, libxl__sprintf(gc, "%s/memory/target",
                 dompath), "%"PRIu32, new_target_memkb);
     rc = xc_domain_getinfolist(ctx->xch, domid, 1, &info);
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index be19bf5..e21d8c3 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -628,6 +628,7 @@ int libxl_domain_core_dump(libxl_ctx *ctx, uint32_t domid,
 
 int libxl_domain_setmaxmem(libxl_ctx *ctx, uint32_t domid, uint32_t target_memkb);
 int libxl_set_memory_target(libxl_ctx *ctx, uint32_t domid, int32_t target_memkb, int relative, int enforce);
+int libxl_set_memory_target_numa(libxl_ctx *ctx, uint32_t domid, int32_t target_memkb, int relative, int enforce, int node_specify, bool nodeexact);
 int libxl_get_memory_target(libxl_ctx *ctx, uint32_t domid, uint32_t *out_target);
 
 
diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
index e72a7d2..6e5873d 100644
--- a/tools/libxl/xl.h
+++ b/tools/libxl/xl.h
@@ -62,6 +62,7 @@ int main_vcpupin(int argc, char **argv);
 int main_vcpuset(int argc, char **argv);
 int main_memmax(int argc, char **argv);
 int main_memset(int argc, char **argv);
+int main_memset_numa(int argc, char **argv);
 int main_sched_credit(int argc, char **argv);
 int main_sched_credit2(int argc, char **argv);
 int main_sched_sedf(int argc, char **argv);
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 884f050..ddfb0d5 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -2523,6 +2523,51 @@ int main_memset(int argc, char **argv)
     return 0;
 }
 
+static void set_memory_target_numa(uint32_t domid, const char *mem, int mnid, bool nodeexact)
+{
+    long long int memorykb;
+
+    memorykb = parse_mem_size_kb(mem);
+    if (memorykb == -1)  {
+        fprintf(stderr, "invalid memory size: %s\n", mem);
+        exit(3);
+    }
+
+    libxl_set_memory_target_numa(ctx, domid, memorykb, 0, /* enforce */ 1, mnid, nodeexact);
+}
+
+int main_memset_numa(int argc, char **argv)
+{
+    uint32_t domid;
+    int opt = 0;
+    int mnid = -1;
+    const char *mem;
+    bool nodeexact = false;
+    static const struct option opts[] = {
+        {"exact", 0, 0, 'e'},
+        COMMON_LONG_OPTS,
+        {0, 0, 0, 0}
+    };
+
+    SWITCH_FOREACH_OPT(opt, "e", opts, "mem-set-numa", 1) {
+    case 'e':
+        nodeexact = true;
+        break;
+    }
+    if (argc < optind + 3){
+        help("mem-set-numa");
+        return 2;
+    }
+    domid = find_domain(argv[optind]);
+    mem = argv[optind + 1];
+    if (sscanf(argv[optind + 2], "%d", &mnid) != 1){
+        fprintf(stderr, "invalid node id");
+    }
+    fprintf(stderr, "nodeexact = %d domid = %d mem = %s mnid = %d\n", nodeexact, domid, mem, mnid);
+    set_memory_target_numa(domid, mem, mnid, nodeexact);
+    return 0;
+}
+
 static int cd_insert(uint32_t domid, const char *virtdev, char *phys)
 {
     libxl_device_disk disk; /* we don't free disk's contents */
diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
index 326a660..ab918c0 100644
--- a/tools/libxl/xl_cmdtable.c
+++ b/tools/libxl/xl_cmdtable.c
@@ -199,6 +199,13 @@ struct cmd_spec cmd_table[] = {
       "Set the current memory usage for a domain",
       "<Domain> <MemMB['b'[bytes]|'k'[KB]|'m'[MB]|'g'[GB]|'t'[TB]]>",
     },
+    { "mem-set-numa",
+        &main_memset_numa, 0, 1,
+        "Set the current memory usage for a domain, with numa node specified",
+        "[-e] <Domain> <MemMB['b'[bytes]|'k'[KB]|'m'[MB]|'g'[GB]|'t'[TB]]> <nid>",
+        "-e, --exact: operatrion will force on this node exactly"
+        "nid: the machine(physical) node id\n"
+    },
     { "button-press",
       &main_button_press, 0, 1,
       "Indicate an ACPI button press to the domain",
-- 
1.8.1.4

Re: [PATCH v1][RFC] xen balloon driver numa support, libxl interface

[Jan Beulich]

>>> On 12.08.13 at 15:18, Yechen Li <lccycc123@gmail.com> wrote:
>   This small patch implements a numa support of memory operation for libxl
>   The command is: xl mem-set-numa [-e] vmid memorysize nodeid
>   To pass the parameters to balloon driver in kernel, I add a file of 
> xen-store
> as /local/domain/(id)/memory/target_nid, hoping this is ok....
> 
>   It's my first time submitting a patch, please point out the problems so 
> that I could work better in future, thanks very much!

The main problem here is the lack of a consumer of this ...

> @@ -3754,7 +3761,10 @@ retry_transaction:
>          abort_transaction = 1;
>          goto out;
>      }
> -
> +    //lcc:
> +    LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "target_nid = %d focus= %d", node_specify, (int) nodeexact);
> +    libxl__xs_write(gc, t, libxl__sprintf(gc, "%s/memory/target_nid",
> +                dompath), "%"PRId32" %"PRId32, node_specify, (int)nodeexact);

... new Xenbus node. Without (so far) a true view into the
host topology, even if you added a respective consumer to the
balloon driver, how would that driver honor such a request?
And even if it knew about host topology, unless the virtual
topology in the subject domain sufficiently closely resembled the
host's, it would - afaict - still have no way of obtaining suitable
memory from the page allocator.

Jan

Re: [PATCH v1][RFC] xen balloon driver numa support, libxl interface

[Li Yechen]

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

HI Jan,

> ... new Xenbus node. Without (so far) a true view into the
> host topology, even if you added a respective consumer to the
> balloon driver, how would that driver honor such a request?
> And even if it knew about host topology, unless the virtual
> topology in the subject domain sufficiently closely resembled the
> host's, it would - afaict - still have no way of obtaining suitable
> memory from the page allocator.
>
Do you mean the kernel part?
The modification of kernel is here:
http://lists.xenproject.org/archives/html/xen-devel/2013-08/msg01179.html
I still haven't find out how to generate two patches in one time....So I
have to send two email, sorry


On Mon, Aug 12, 2013 at 10:00 PM, Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 12.08.13 at 15:18, Yechen Li <lccycc123@gmail.com> wrote:
> >   This small patch implements a numa support of memory operation for
> libxl
> >   The command is: xl mem-set-numa [-e] vmid memorysize nodeid
> >   To pass the parameters to balloon driver in kernel, I add a file of
> > xen-store
> > as /local/domain/(id)/memory/target_nid, hoping this is ok....
> >
> >   It's my first time submitting a patch, please point out the problems so
> > that I could work better in future, thanks very much!
>
> The main problem here is the lack of a consumer of this ...
>
> > @@ -3754,7 +3761,10 @@ retry_transaction:
> >          abort_transaction = 1;
> >          goto out;
> >      }
> > -
> > +    //lcc:
> > +    LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "target_nid = %d focus= %d",
> node_specify, (int) nodeexact);
> > +    libxl__xs_write(gc, t, libxl__sprintf(gc, "%s/memory/target_nid",
> > +                dompath), "%"PRId32" %"PRId32, node_specify,
> (int)nodeexact);
>
> ... new Xenbus node. Without (so far) a true view into the
> host topology, even if you added a respective consumer to the
> balloon driver, how would that driver honor such a request?
> And even if it knew about host topology, unless the virtual
> topology in the subject domain sufficiently closely resembled the
> host's, it would - afaict - still have no way of obtaining suitable
> memory from the page allocator.
>
> Jan
>
>


-- 
Yechen Li

Team of System Virtualization and Cloud Computing
School of Electronic Engineering  and Computer Science,
Peking University, China

Nothing is impossible because impossible itself  says: " I'm possible "
lccycc From PKU

[-- Attachment #1.2: Type: text/html, Size: 3934 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 v1][RFC] xen balloon driver numa support, libxl interface

[Jan Beulich]

>>> On 12.08.13 at 16:18, Li Yechen <lccycc123@gmail.com> wrote:
> HI Jan,
> 
>> ... new Xenbus node. Without (so far) a true view into the
>> host topology, even if you added a respective consumer to the
>> balloon driver, how would that driver honor such a request?
>> And even if it knew about host topology, unless the virtual
>> topology in the subject domain sufficiently closely resembled the
>> host's, it would - afaict - still have no way of obtaining suitable
>> memory from the page allocator.
>>
> Do you mean the kernel part?
> The modification of kernel is here:
> http://lists.xenproject.org/archives/html/xen-devel/2013-08/msg01179.html 
> I still haven't find out how to generate two patches in one time....So I
> have to send two email, sorry

Sending two mails is fine - you just should have referred to
the other one in the description.

But that kernel change is very bogus; first of all you need
to clean it up to undo all the restoration of code that 3.11-rc
changed. And then you'll need to explain what the
correlation between virtual and physical node IDs is.

Jan

Re: [PATCH v1][RFC] xen balloon driver numa support, libxl interface

[Li Yechen]

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

>
> But that kernel change is very bogus; first of all you need
> to clean it up to undo all the restoration of code that 3.11-rc
> changed. And then you'll need to explain what the
> correlation between virtual and physical node IDs is.

Oh no! Acturally I start this work from 3.9 and do not notice that the code
changes a little.....
Thank you very much I'll merge it right now.....
The relation between virtual and physical node IDs is belong to another
guy's work, I think we could see her email soon.

[-- Attachment #1.2: Type: text/html, Size: 720 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 v1][RFC] xen balloon driver numa support, libxl interface

[Li Yechen]

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

OK...The different from 3.9 to 3.11 is only a litte log print....I have
merge the code, it does not affect this patch...


On Mon, Aug 12, 2013 at 10:57 PM, Li Yechen <lccycc123@gmail.com> wrote:

> But that kernel change is very bogus; first of all you need
>> to clean it up to undo all the restoration of code that 3.11-rc
>> changed. And then you'll need to explain what the
>> correlation between virtual and physical node IDs is.
>
> Oh no! Acturally I start this work from 3.9 and do not notice that the
> code changes a little.....
> Thank you very much I'll merge it right now.....
> The relation between virtual and physical node IDs is belong to another
> guy's work, I think we could see her email soon.
>



-- 
Yechen Li

Team of System Virtualization and Cloud Computing
School of Electronic Engineering  and Computer Science,
Peking University, China

Nothing is impossible because impossible itself  says: " I'm possible "
lccycc From PKU

[-- Attachment #1.2: Type: text/html, Size: 2069 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 v1][RFC] xen balloon driver numa support, libxl interface

[Jan Beulich]

>>> On 12.08.13 at 17:15, Li Yechen <lccycc123@gmail.com> wrote:
> OK...The different from 3.9 to 3.11 is only a litte log print....I have
> merge the code, it does not affect this patch...

No - the patch you sent re-added things like explicit updating
of totalram_pages and totalhigh_pages. This needs to be dropped;
only then will it make sense to actually look at the patch.

Jan

Re: [PATCH v1][RFC] xen balloon driver numa support, libxl interface

[Dario Faggioli]

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

Hey, Yechen... Could you please switch to text-only e-mails? That would
be much more respectful of the actual list etiquette, but most important
it will all look a lot better looking when replying...

On lun, 2013-08-12 at 22:57 +0800, Li Yechen wrote:
>         But that kernel change is very bogus; first of all you need
>         to clean it up to undo all the restoration of code that
>         3.11-rc
>         changed. And then you'll need to explain what the
>         correlation between virtual and physical node IDs is.
> 
> The relation between virtual and physical node IDs is belong to
> another guy's work, I think we could see her email soon. 
> 
Well, although that's mostly true, I think it would not have harmed to
have some sort of high level description of the overall design, trying
to explain what the final goal is, who all the involved actors are, what
role they play, etc.

Of course, I know all these kind of stuff, since you sent me in a
private e-mail already, but can I ask you to come up with a summary of
that, and attach it somehow to the public submission (a 0/xx message,
even if this just a single patch, would have been great).

Thanks and 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: [PATCH v1][RFC] xen balloon driver numa support, libxl interface

[Dario Faggioli]

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

On lun, 2013-08-12 at 15:00 +0100, Jan Beulich wrote:
> >>> On 12.08.13 at 15:18, Yechen Li <lccycc123@gmail.com> wrote:
> > @@ -3754,7 +3761,10 @@ retry_transaction:
> >          abort_transaction = 1;
> >          goto out;
> >      }
> > -
> > +    //lcc:
> > +    LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "target_nid = %d focus= %d", node_specify, (int) nodeexact);
> > +    libxl__xs_write(gc, t, libxl__sprintf(gc, "%s/memory/target_nid",
> > +                dompath), "%"PRId32" %"PRId32, node_specify, (int)nodeexact);
> 
> ... new Xenbus node. Without (so far) a true view into the
> host topology, even if you added a respective consumer to the
> balloon driver, how would that driver honor such a request?
>
Right. So, as I asked in another e-mail, Yechen, could you provide
(either here or when submitting an new/updated version of the patch)
some more details about the overall design, so that people wanting to
review the patch could have an insight of the big picture? :-)

Really quickly, Jan, this is meant to be effective once:
 1. we will have a way to specify and build a virtual NUMA topology for
    the guests;
 2. we will have a way for a virtual NUMA enabled guest to figure out
    what physical host NUMA node X has the pages of its virtual NUMA
    node Y.

As Yechen said, Elena is working on the PV side of 1., while Matt has
something for the HVM side of it. Once we will have both in place, we
can figure out 2., and then have Yechen's work integrate with that all.

Yes, I know, sending this patch as the first item _is_ a bit confusing,
but we thought that, since this involves new interfaces (e.g., the new
xenstore node), it could be useful to start gathering some early
feedback. :-)

Hope Yechen can explain the problem better, and make things even more
clear.

> And even if it knew about host topology, unless the virtual
> topology in the subject domain sufficiently closely resembled the
> host's, it would - afaict - still have no way of obtaining suitable
> memory from the page allocator.
> 
Not sure I get entirely what you mean... Are you saying that, if the
pages for a guest virtual NUMA node X come from multiple host nodes
(instead than from just one single host NUMA node Y), this won't work
properly?

If yes, well, I agree. In that case, we'll more or less fall back on
what we have right now (and if that is not the case in the code, well,
we should make it so), so no better, no worse, right?
However, what about the case in which we do manage in putting a guest's
virtual node onto a host's virtual node? That should work a lot better
(than now), right?

In summary, this is meant at being a performance and resource
optimization, for all the cases and setups where a specific set of
conditions can be satisfied. I think this is the case for most
performance and resource optimization, and I think this would be a nice
one to have.

Thanks for your feedback and 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: [PATCH v1][RFC] xen balloon driver numa support, libxl interface

[Dario Faggioli]

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

Hello Yechen, nice to see you and your code here!! :-)

That's already a nice job as a first submission. See some comments about
both the code and "the process" below and inline.

First of all, both xen-devel and LKML are very busy lists. One could
think that people just read all the messages that come across, but, as a
matter of fact, that is hardly the case. That's why you usually do not
just send the e-mail(s) with the patch (series) to the list, but you
also put the relevant people you want and need feedback from on Cc.

These "relevant people" are at least the maintainers of the subsystem
you're modifying with your patches. In this case, your changes affects
bits of the Xen toolsack, i.e., xl and libxl, so you should at least
have the toolstack maintainers in Cc.

To figure out who they are, most projects have a MAINTAINERS file (both
Xen and Linux does). If you look there you will fin out this entry:

TOOLSTACK
M:      Ian Jackson <ian_DOT_jackson_AT_eu.citrix.com>
M:      Stefano Stabellini <stefano_DOT_stabellini_AT_eu.citrix.com>
M:      Ian Campbell <ian_DOT_campbell_AT_citrix.com>
S:      Supported
F:      tools/

Meaning that what is under the 'tools/' directory, is under the
responsibility of those three people, which are the ones you should have
in the Cc list.

You may well put other people there too (provided you do not
exaggerate! :-P). For instance, I'm taking care of NUMA in Xen, so you
should have me there. Also, this patch couples with the other one for
Linux, so you can have the Linux maintainers (that you will/would Cc to
that one, and that would be Konrad) in Cc as well.

For this time, I did this for you (by replying to your e-mail and adding
those people). Next time, don't forget to put them there yourself.

On lun, 2013-08-12 at 21:18 +0800, Yechen Li wrote:
> ---
>
That's weird... This really should not be here. Usually, what you have
is:
 - the changelog
 - the "---"
 - the diffstat.

Check out other submissions to xen-devel to see what I mean (e.g.,
http://comments.gmane.org/gmane.comp.emulators.xen.devel/165396 )

Perhaps something is not completely right with your
git-format-patch/git-send-email settings or parameters?

Regarding the changelog here below, formatting is important, and you
usually do not want any indentation (tools like git-log will put it
there themselves), and you want lines there to be even shorter that 80
characters (for the exact same reason! :-D).

About the content:

>   This small patch implements a numa support of memory operation for libxl
>
Perhaps something like "This patch adds NUMA support to setting the
memory target from libxl and xl"

>   The command is: xl mem-set-numa [-e] vmid memorysize nodeid
>   To pass the parameters to balloon driver in kernel, I add a file of xen-store
> as /local/domain/(id)/memory/target_nid, hoping this is ok....
> 
Although stuff like "hoping is ok" is fair for RFC patches or series,
just make sure that, in future versions, that will no longer be RFCs,
you do not put those kind of stuff in the actual changelogs, ok? :-P

>   It's my first time submitting a patch, please point out the problems so that
> I could work better in future, thanks very much!
> 
And the same applies to this hunk here. It is not at all required, but
personally, when it comes to RFC, I always include a 0/XX message, right
for hosting this kind of comments.

Not to mention that, as I said already in my other e-mail (while
replying to Jan), you could have used it to specify some more details
about the design, the final goal, etc., too.

Anyway: "please point out the problems so that I could work better in
future" that's the right attitude, I really appreciate this!! :-P :-P

> tools/libxl/libxl.c       | 14 ++++++++++++--
> tools/libxl/libxl.h       |  1 +
> tools/libxl/xl.h          |  1 +
> tools/libxl/xl_cmdimpl.c  | 45 +++++++++++++++++++++++++++++++++++++++++++++
> tools/libxl/xl_cmdtable.c |  7 +++++++
> 5 files changed, 66 insertions(+), 2 deletions(-)
>
About the splitting of this patch in an easier to review and to
understand patch series, what about having two patches, one (the first
in the series) modifying libxl, and another one modifying xl?

> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 81785df..f027d59 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -3642,10 +3642,17 @@ retry:
>      }
>      return 0;
>  }
> -
>
Removing a white space could be a good thing, if having them there
violates the coding style.

However, I don't think this is the case (i.e., this particular white
space is fine where it is).

Also, even if it was the case, you do not want to mix coding style fixes
with actual bug fixing or new features implementation.

>  int libxl_set_memory_target(libxl_ctx *ctx, uint32_t domid,
>          int32_t target_memkb, int relative, int enforce)
>  {
> +    return libxl_set_memory_target_numa(ctx, domid, target_memkb, relative,
> +                enforce, -1, 0);
> +}
> +
> +int libxl_set_memory_target_numa(libxl_ctx *ctx, uint32_t domid,
> +        int32_t target_memkb, int relative, int enforce,
> +        int node_specify, bool nodeexact)
> +{
>
'node_specify' can probably be just 'node'. Actually, I'd probably
change the name of the function itself to 'libxl_set_memory_target_node'
instead of '_numa'.

Regarding 'nodeexact', see below.

>      GC_INIT(ctx);
>      int rc = 1, abort_transaction = 0;
>      uint32_t memorykb = 0, videoram = 0;
> @@ -3754,7 +3761,10 @@ retry_transaction:
>          abort_transaction = 1;
>          goto out;
>      }
> -
> +    //lcc:
>
Again, do not remove white spaces. Also, comments done via "//" are not
welcome. I know this is an RFC, so no need to worry too much, this is
just me trying to help you as much as I can to get next version in a
better shape, ok? :-)

> +    LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "target_nid = %d focus= %d", node_specify, (int) nodeexact);
>
Casts are bad. They hide errors at either design and/or implementation
level. There probably are cases where you can't avoid them (and you
should document things being like that somehow, e.g., in code comments),
but in general, you should think twice before introducing one, and see
if you really can't accomplish the same without it.

Regarding this nodeexact thing, I wonder whether we could turn it into
some kind of flag that you can || to 'node_specify' (or 'node'),
similarly to what actually happens in Xen with MEMF_exact_node.

Notice that this is not because Xen's and libxl's interface needs to
have these kinds of correlations and dependencies... It's just I think
I'd like it better that way. :-)

> +    libxl__xs_write(gc, t, libxl__sprintf(gc, "%s/memory/target_nid",
> +                dompath), "%"PRId32" %"PRId32, node_specify, (int)nodeexact);
>
Casts here too.

> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -628,6 +628,7 @@ int libxl_domain_core_dump(libxl_ctx *ctx, uint32_t domid,
>  
>  int libxl_domain_setmaxmem(libxl_ctx *ctx, uint32_t domid, uint32_t target_memkb);
>  int libxl_set_memory_target(libxl_ctx *ctx, uint32_t domid, int32_t target_memkb, int relative, int enforce);
> +int libxl_set_memory_target_numa(libxl_ctx *ctx, uint32_t domid, int32_t target_memkb, int relative, int enforce, int node_specify, bool nodeexact);
>
About names, see above. Also, this is a very long line. In general we
want lines to break at (well, before!) 80 characters. I appreciate that
this files have long lines already, but that does not mean that new code
should make the situation worse! :-P

>  int libxl_get_memory_target(libxl_ctx *ctx, uint32_t domid, uint32_t *out_target);
>  
> 
> diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
> index e72a7d2..6e5873d 100644
> --- a/tools/libxl/xl.h
> +++ b/tools/libxl/xl.h
> @@ -62,6 +62,7 @@ int main_vcpupin(int argc, char **argv);
>  int main_vcpuset(int argc, char **argv);
>  int main_memmax(int argc, char **argv);
>  int main_memset(int argc, char **argv);
> +int main_memset_numa(int argc, char **argv);
>
So, you are introducing a new command... Why not just add a "NUMA
option"(something like '-n'/'--numa') to mem-set?

Either way, look at the docs/man directory in the source tree. You'll
find a file called xl.pod.1, which hosts the manual page for `xl', in
markdown format. When adding or changing something (and that applies to
both the cases where you add a new command or a new option), you should
update that file too, to account for the new feature (or reflect the new
behavior).

> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -2523,6 +2523,51 @@ int main_memset(int argc, char **argv)
>      return 0;
>  }
>  
> +static void set_memory_target_numa(uint32_t domid, const char *mem, int mnid, bool nodeexact)
> +{
> +    long long int memorykb;
> +
> +    memorykb = parse_mem_size_kb(mem);
> +    if (memorykb == -1)  {
> +        fprintf(stderr, "invalid memory size: %s\n", mem);
> +        exit(3);
> +    }
> +
> +    libxl_set_memory_target_numa(ctx, domid, memorykb, 0, /* enforce */ 1, mnid, nodeexact);
> +}
> +
>
This is really similar to set_memory_target. Merging the two will avoid
code duplication.

> +int main_memset_numa(int argc, char **argv)
> +{
> +    uint32_t domid;
> +    int opt = 0;
> +    int mnid = -1;
> +    const char *mem;
> +    bool nodeexact = false;
> +    static const struct option opts[] = {
> +        {"exact", 0, 0, 'e'},
> +        COMMON_LONG_OPTS,
> +        {0, 0, 0, 0}
> +    };
> +
> +    SWITCH_FOREACH_OPT(opt, "e", opts, "mem-set-numa", 1) {
> +    case 'e':
> +        nodeexact = true;
> +        break;
> +    }
> +    if (argc < optind + 3){
> +        help("mem-set-numa");
> +        return 2;
> +    }
> +    domid = find_domain(argv[optind]);
> +    mem = argv[optind + 1];
> +    if (sscanf(argv[optind + 2], "%d", &mnid) != 1){
> +        fprintf(stderr, "invalid node id");
> +    }
>
If you make this all a mode of operation of the existing mem-set, this
will get easier and prettier, as you may rely on the option handling
machinery to retrieve the node-ID argument, instead of having to
manually fidle with optind, argv, sscanf, etc.

> +    fprintf(stderr, "nodeexact = %d domid = %d mem = %s mnid = %d\n", nodeexact, domid, mem, mnid);
>
This 'fprintf' is debug output, right? Make sure that future non-RFC
versions does not have this, or that you gate it properly.

So, you tell me now... Was the feedback useful? :-D

Anyway, thanks a lot for doing and sharing this. I know it's a bit hard
the first times, but if you keep going, you'll get used to it soon
enough!

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: [PATCH v1][RFC] xen balloon driver numa support, libxl interface

[Dario Faggioli]

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

[Adding potentially interested people in Cc: the Ian-s are the toolstack
maintainers, Elena is there because her work on PV-NUMA is somewhat
related, Konrad is there because of the Linux implications of this]

On lun, 2013-08-12 at 21:18 +0800, Yechen Li wrote:
> ---
> 
>   This small patch implements a numa support of memory operation for libxl
>   The command is: xl mem-set-numa [-e] vmid memorysize nodeid
>   To pass the parameters to balloon driver in kernel, I add a file of xen-store
> as /local/domain/(id)/memory/target_nid, hoping this is ok....
> 
>   It's my first time submitting a patch, please point out the problems so that
> I could work better in future, thanks very much!
> 
>  tools/libxl/libxl.c       | 14 ++++++++++++--
>  tools/libxl/libxl.h       |  1 +
>  tools/libxl/xl.h          |  1 +
>  tools/libxl/xl_cmdimpl.c  | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  tools/libxl/xl_cmdtable.c |  7 +++++++
>  5 files changed, 66 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 81785df..f027d59 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -3642,10 +3642,17 @@ retry:
>      }
>      return 0;
>  }
> -
>  int libxl_set_memory_target(libxl_ctx *ctx, uint32_t domid,
>          int32_t target_memkb, int relative, int enforce)
>  {
> +    return libxl_set_memory_target_numa(ctx, domid, target_memkb, relative,
> +                enforce, -1, 0);
> +}
> +
> +int libxl_set_memory_target_numa(libxl_ctx *ctx, uint32_t domid,
> +        int32_t target_memkb, int relative, int enforce,
> +        int node_specify, bool nodeexact)
> +{
>      GC_INIT(ctx);
>      int rc = 1, abort_transaction = 0;
>      uint32_t memorykb = 0, videoram = 0;
> @@ -3754,7 +3761,10 @@ retry_transaction:
>          abort_transaction = 1;
>          goto out;
>      }
> -
> +    //lcc:
> +    LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "target_nid = %d focus= %d", node_specify, (int) nodeexact);
> +    libxl__xs_write(gc, t, libxl__sprintf(gc, "%s/memory/target_nid",
> +                dompath), "%"PRId32" %"PRId32, node_specify, (int)nodeexact);
>      libxl__xs_write(gc, t, libxl__sprintf(gc, "%s/memory/target",
>                  dompath), "%"PRIu32, new_target_memkb);
>      rc = xc_domain_getinfolist(ctx->xch, domid, 1, &info);
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index be19bf5..e21d8c3 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -628,6 +628,7 @@ int libxl_domain_core_dump(libxl_ctx *ctx, uint32_t domid,
>  
>  int libxl_domain_setmaxmem(libxl_ctx *ctx, uint32_t domid, uint32_t target_memkb);
>  int libxl_set_memory_target(libxl_ctx *ctx, uint32_t domid, int32_t target_memkb, int relative, int enforce);
> +int libxl_set_memory_target_numa(libxl_ctx *ctx, uint32_t domid, int32_t target_memkb, int relative, int enforce, int node_specify, bool nodeexact);
>  int libxl_get_memory_target(libxl_ctx *ctx, uint32_t domid, uint32_t *out_target);
>  
> 
> diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
> index e72a7d2..6e5873d 100644
> --- a/tools/libxl/xl.h
> +++ b/tools/libxl/xl.h
> @@ -62,6 +62,7 @@ int main_vcpupin(int argc, char **argv);
>  int main_vcpuset(int argc, char **argv);
>  int main_memmax(int argc, char **argv);
>  int main_memset(int argc, char **argv);
> +int main_memset_numa(int argc, char **argv);
>  int main_sched_credit(int argc, char **argv);
>  int main_sched_credit2(int argc, char **argv);
>  int main_sched_sedf(int argc, char **argv);
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 884f050..ddfb0d5 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -2523,6 +2523,51 @@ int main_memset(int argc, char **argv)
>      return 0;
>  }
>  
> +static void set_memory_target_numa(uint32_t domid, const char *mem, int mnid, bool nodeexact)
> +{
> +    long long int memorykb;
> +
> +    memorykb = parse_mem_size_kb(mem);
> +    if (memorykb == -1)  {
> +        fprintf(stderr, "invalid memory size: %s\n", mem);
> +        exit(3);
> +    }
> +
> +    libxl_set_memory_target_numa(ctx, domid, memorykb, 0, /* enforce */ 1, mnid, nodeexact);
> +}
> +
> +int main_memset_numa(int argc, char **argv)
> +{
> +    uint32_t domid;
> +    int opt = 0;
> +    int mnid = -1;
> +    const char *mem;
> +    bool nodeexact = false;
> +    static const struct option opts[] = {
> +        {"exact", 0, 0, 'e'},
> +        COMMON_LONG_OPTS,
> +        {0, 0, 0, 0}
> +    };
> +
> +    SWITCH_FOREACH_OPT(opt, "e", opts, "mem-set-numa", 1) {
> +    case 'e':
> +        nodeexact = true;
> +        break;
> +    }
> +    if (argc < optind + 3){
> +        help("mem-set-numa");
> +        return 2;
> +    }
> +    domid = find_domain(argv[optind]);
> +    mem = argv[optind + 1];
> +    if (sscanf(argv[optind + 2], "%d", &mnid) != 1){
> +        fprintf(stderr, "invalid node id");
> +    }
> +    fprintf(stderr, "nodeexact = %d domid = %d mem = %s mnid = %d\n", nodeexact, domid, mem, mnid);
> +    set_memory_target_numa(domid, mem, mnid, nodeexact);
> +    return 0;
> +}
> +
>  static int cd_insert(uint32_t domid, const char *virtdev, char *phys)
>  {
>      libxl_device_disk disk; /* we don't free disk's contents */
> diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
> index 326a660..ab918c0 100644
> --- a/tools/libxl/xl_cmdtable.c
> +++ b/tools/libxl/xl_cmdtable.c
> @@ -199,6 +199,13 @@ struct cmd_spec cmd_table[] = {
>        "Set the current memory usage for a domain",
>        "<Domain> <MemMB['b'[bytes]|'k'[KB]|'m'[MB]|'g'[GB]|'t'[TB]]>",
>      },
> +    { "mem-set-numa",
> +        &main_memset_numa, 0, 1,
> +        "Set the current memory usage for a domain, with numa node specified",
> +        "[-e] <Domain> <MemMB['b'[bytes]|'k'[KB]|'m'[MB]|'g'[GB]|'t'[TB]]> <nid>",
> +        "-e, --exact: operatrion will force on this node exactly"
> +        "nid: the machine(physical) node id\n"
> +    },
>      { "button-press",
>        &main_button_press, 0, 1,
>        "Indicate an ACPI button press to the domain",

-- 
<<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: [PATCH v1][RFC] xen balloon driver numa support, libxl interface

[Ian Campbell]

On Mon, 2013-08-12 at 21:18 +0800, Yechen Li wrote:
> ---
> 
>   This small patch implements a numa support of memory operation for libxl
>   The command is: xl mem-set-numa [-e] vmid memorysize nodeid
>   To pass the parameters to balloon driver in kernel, I add a file of xen-store
> as /local/domain/(id)/memory/target_nid, hoping this is ok....

It might be OK if you document it in docs/misc/xenstore-paths.markdown.

>   It's my first time submitting a patch, please point out the problems so that
> I could work better in future, thanks very much!

Please see http://wiki.xen.org/wiki/Submitting_Xen_Patches, in
particular the bit about Signed-off-by.

> 
>  tools/libxl/libxl.c       | 14 ++++++++++++--
>  tools/libxl/libxl.h       |  1 +
>  tools/libxl/xl.h          |  1 +
>  tools/libxl/xl_cmdimpl.c  | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  tools/libxl/xl_cmdtable.c |  7 +++++++
>  5 files changed, 66 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 81785df..f027d59 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -3642,10 +3642,17 @@ retry:
>      }
>      return 0;
>  }
> -
>  int libxl_set_memory_target(libxl_ctx *ctx, uint32_t domid,
>          int32_t target_memkb, int relative, int enforce)
>  {
> +    return libxl_set_memory_target_numa(ctx, domid, target_memkb, relative,
> +                enforce, -1, 0);
> +}
> +
> +int libxl_set_memory_target_numa(libxl_ctx *ctx, uint32_t domid,
> +        int32_t target_memkb, int relative, int enforce,
> +        int node_specify, bool nodeexact)
> +{
>      GC_INIT(ctx);
>      int rc = 1, abort_transaction = 0;
>      uint32_t memorykb = 0, videoram = 0;
> @@ -3754,7 +3761,10 @@ retry_transaction:
>          abort_transaction = 1;
>          goto out;
>      }
> -
> +    //lcc:

Please don't leave debug dropping in place.

> +    LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "target_nid = %d focus= %d", node_specify, (int) nodeexact);
> +    libxl__xs_write(gc, t, libxl__sprintf(gc, "%s/memory/target_nid",
> +                dompath), "%"PRId32" %"PRId32, node_specify, (int)nodeexact);
>      libxl__xs_write(gc, t, libxl__sprintf(gc, "%s/memory/target",
>                  dompath), "%"PRIu32, new_target_memkb);
>      rc = xc_domain_getinfolist(ctx->xch, domid, 1, &info);
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index be19bf5..e21d8c3 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -628,6 +628,7 @@ int libxl_domain_core_dump(libxl_ctx *ctx, uint32_t domid,
>  
>  int libxl_domain_setmaxmem(libxl_ctx *ctx, uint32_t domid, uint32_t target_memkb);
>  int libxl_set_memory_target(libxl_ctx *ctx, uint32_t domid, int32_t target_memkb, int relative, int enforce);
> +int libxl_set_memory_target_numa(libxl_ctx *ctx, uint32_t domid, int32_t target_memkb, int relative, int enforce, int node_specify, bool nodeexact);

This needs a LIBXL_HAVE style declaration. I'm unsure about adding
another function as opposed to extending the current ABI using the
LIBXL_API_VERSION compatibility provisions.
> diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
> index 326a660..ab918c0 100644
> --- a/tools/libxl/xl_cmdtable.c
> +++ b/tools/libxl/xl_cmdtable.c
> @@ -199,6 +199,13 @@ struct cmd_spec cmd_table[] = {
>        "Set the current memory usage for a domain",
>        "<Domain> <MemMB['b'[bytes]|'k'[KB]|'m'[MB]|'g'[GB]|'t'[TB]]>",
>      },
> +    { "mem-set-numa",

Perhaps instead of adding a new function the existing mem-set should
take a -n <node> parameter?

> +        &main_memset_numa, 0, 1,
> +        "Set the current memory usage for a domain, with numa node specified",
> +        "[-e] <Domain> <MemMB['b'[bytes]|'k'[KB]|'m'[MB]|'g'[GB]|'t'[TB]]> <nid>",
> +        "-e, --exact: operatrion will force on this node exactly"

"operation"

> +        "nid: the machine(physical) node id\n"
> +    },
>      { "button-press",
>        &main_button_press, 0, 1,
>        "Indicate an ACPI button press to the domain",

Re: [PATCH v1][RFC] xen balloon driver numa support, libxl interface

[Ian Campbell]

On Mon, 2013-08-12 at 17:51 +0200, Dario Faggioli wrote:
> Hey, Yechen... Could you please switch to text-only e-mails? That would
> be much more respectful of the actual list etiquette, but most important
> it will all look a lot better looking when replying...
> 
> On lun, 2013-08-12 at 22:57 +0800, Li Yechen wrote:
> >         But that kernel change is very bogus; first of all you need
> >         to clean it up to undo all the restoration of code that
> >         3.11-rc
> >         changed. And then you'll need to explain what the
> >         correlation between virtual and physical node IDs is.
> > 
> > The relation between virtual and physical node IDs is belong to
> > another guy's work, I think we could see her email soon. 
> > 
> Well, although that's mostly true, I think it would not have harmed to
> have some sort of high level description of the overall design, trying
> to explain what the final goal is, who all the involved actors are, what
> role they play, etc.

Yechen,

If there are, as it sounds, several different patches from different
people within the group needed to implement this feature then please
could one of you take responsibility for combining them into a single
(or at most two, one xen.git and one linux.git) coherent series which
contains everything such that reviewers can get the whole picture.

Ian.

Re: [PATCH v1][RFC] xen balloon driver numa support, libxl interface

[Dario Faggioli]

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

On mar, 2013-08-13 at 21:56 +0100, Ian Campbell wrote:
> On Mon, 2013-08-12 at 17:51 +0200, Dario Faggioli wrote:
> > On lun, 2013-08-12 at 22:57 +0800, Li Yechen wrote:
> > > The relation between virtual and physical node IDs is belong to
> > > another guy's work, I think we could see her email soon. 
> > > 
> > Well, although that's mostly true, I think it would not have harmed to
> > have some sort of high level description of the overall design, trying
> > to explain what the final goal is, who all the involved actors are, what
> > role they play, etc.
> 
> Yechen,
> 
> If there are, as it sounds, several different patches from different
> people within the group needed to implement this feature then please
> could one of you take responsibility for combining them into a single
> (or at most two, one xen.git and one linux.git) coherent series which
> contains everything such that reviewers can get the whole picture.
> 
Yes, Ian, that is pretty much what is going on. Different people working
on different features that are mostly independent but have some
inter-dependencies.

We will definitely put things in such a way that the complete picture
could be available and evident as soon as possible, however, that is not
possible right now.

The reason why Yechen submitted this series, even if a fundamental
building block of it (virtual NUMA topology for guests) is still
missing, is that we thought that there was enough bits of _his_own_work_
--i.e., NUMA-aware ballooning-- in place already, for starting chasing a
bit of feedback, at least on the design of NUMA-aware ballooning itself.

For instance, he has designed it in such a way that it is the higher
toolstack layers (or the user, via xl) that are responsible for deciding
on what physical NUMA node we want some free memory, is that a good
approach? He is doing that by adding a new xenstore node, is that the
right interface? And so on and so forth...

So, basically, we figured that it was worth to try getting an early
enough answer for this kind of questions, especially considering that he
also had some RFC level code that could well exemplify the design
itself. Of course, as other are rightfully pointed out already, when
submitting the RFCs, he failed at describing the complete design, the
motivations and the intended usage of the code he was posting, and we
are sorry and are already addressing this. :-)

To be really honest, I think this is the  biggest issue with this
patches, much more than the fact that some enabling feature/code for it
is still missing. IOW, even if all the bit and pieces were there, it
would be very hard to review the  code without such an high level
explanation of how it is intended to be used anyway, wouldn't it? And as
I said, we're down to fix that.

I hope I clarified the situation at least a bit... In any case, thanks
for having a look at it, and for your suggestion, that I personally
commit to make happen, as soon as all the code will be there (and as
soon as I come back from my summer vacations which are starting in two
days :-P).

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