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

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

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: signature.asc
Description: This is a digitally signed message part

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel