Re: [Xen-devel] [PATCH v1][RFC] drivers/xen, balloon driver numa support in kernel

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

Hi Yechen.

Thanks for doing and sharing this part of the thing too.

See what I already told you on xen-devel about Cc-ing the relevant
people. For Linux as well, you can check at the MAINTAINERS file. This
exact thing (i.e., who the actual maintainers are) is changing a bit
right in these days... See this commit:

http://git.kernel.org/cgit/linux/kernel/git/xen/tip.git/commit/?h=devel/for-jens-3.12&id=3eeef8f72a9365fe20f2c9d84b0afe60a20788cd

On lun, 2013-08-12 at 22:13 +0800, Yechen Li wrote:
>   This small patch adds numa support for balloon driver. Kernel version: 
> 3.11-rc5
>
Mmm... Is that the case? I tried to apply it there and I have one failed
hunk (#12). Anyway, could you please rebase it on the tip of some
relevant git tree?

Linus' tree would be fine, I guess:

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git

However, since these kind of patches should probably go through Konrad's
(or other Linux Xen maintainers) tree anyway, you could well base on top
of:

http://git.kernel.org/cgit/linux/kernel/git/konrad/xen.git/

Or perhaps on some branch of this one here:

http://git.kernel.org/cgit/linux/kernel/git/xen/tip.git/

Konrad, David, would you find 5 minutes to explain Yechen which tree and
which branch he should use, or point him to the appropriate
documentation for understanding that? Thanks...

>   It's just a RFC version, since I'm waiting for the interface of numa 
> topology.
>   The balloon driver will read arguments from xenstore: 
> /local/domain/(id)/memory
> /target_nid, and settle the memory increase/decrease operation on specified
> p-nodeID. 
>   To achieve this, I expand the page-list: ballooned_pages to an array:
> ballooned_pages[MAX_BALLOONNODES], so that balloon can distinguish pages from 
> different node.
>   For the guest without numa, this MAX_BALLOONNODES = 1 so that the balloon 
> falls
> back to a no-numa version.
>
This one you have here is already a better explanation about what the
big picture is, as compared to what you did put in the Xen patch. You
still could explain a bit better, I think, especially the interactions
between virtual and physical NUMA.

It is true that is someone else that is doing the core of that job, but
at the same time, if you want people to give feedback to your work, it
is your call to put them in a position where they can understand as much
as possible of it.

>   The small functions mark //todo: is the interface to numa topology. Now they
> looks stupid because I'm still testing this code. The balloon works well (at
> least it seems to) with this small debug interface. Please ignore the more 
> stupid
> commemts, I'll remove them in some version later...
>   the patch of libxl is here: 
> http://lists.xenproject.org/archives/html/xen-devel/2013-08/msg01157.html
>
I would have done the other way around, i.e., send the Linux patch
(this) and then send the Xen patch and reference this one there.

>   It's my first time submitting a patch, please point out the problems so that
> I could work better in future, thanks very much!
>
That is the right attitude, man. Much appreciated! :-D

> ---
>  drivers/xen/balloon.c     | 358 
> ++++++++++++++++++++++++++++++++++++++++------
>  drivers/xen/xen-balloon.c |  21 ++-
>  include/xen/balloon.h     |  17 +++
>  3 files changed, 345 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> index 2a2ef97..09ca1eb 100644
> --- a/drivers/xen/balloon.c
> +++ b/drivers/xen/balloon.c
> @@ -36,8 +36,6 @@
>   * IN THE SOFTWARE.
>   */
>  
> -#define pr_fmt(fmt) "xen:" KBUILD_MODNAME ": " fmt
> -
>
So you are killing this... Why?

>  #include <linux/kernel.h>
>  #include <linux/sched.h>
>  #include <linux/errno.h>
> @@ -53,6 +51,9 @@
>  #include <linux/memory.h>
>  #include <linux/memory_hotplug.h>
>  
> +//lcc:
> +#include <linux/numa.h>
> +
>
Get rid of these "lcc tags". They're done with '//' commenting, which is
bad. Also, git does a good enough job already in tracking what are your
own changes! :-P

>  #include <asm/page.h>
>  #include <asm/pgalloc.h>
>  #include <asm/pgtable.h>
> @@ -81,18 +82,43 @@ enum bp_state {
>       BP_EAGAIN,
>       BP_ECANCELED
>  };
> +struct bp_rt{
> +    unsigned long donepages;
> +    enum bp_state state;
> +};
> +#define DECLARE_BP_RT(bp_rt)     \
> +    struct bp_rt bp_rt = {       \
> +        .donepages = 0,          \
> +        .state = BP_DONE        \
> +    }
>  
Coding style! Do you remember when I said to you that Linux and Xen
coding styles are different? :-)

In this specific case, the thing is that you need to use 'tab' (and set
the width for that to 8) instead than hard coding 4 spaces. In general,
have a look below for a complete description of Linux's style, and make
sure your patch complies to that:

https://www.kernel.org/doc/Documentation/CodingStyle

Also, do you remember when I mention the SubmittingPatches document:

https://www.kernel.org/doc/Documentation/SubmittingPatches

Have another look there, and see the section called "Style check your
changes". It mentions running your patch through scripts/checkpatch.pl,
which is something really really helpful. :-)

BTW, what exactly is this BP_RT thing? How about adding a few words on
comment about it?

> 
>  static DEFINE_MUTEX(balloon_mutex);
>  
> +//lcc todo: should this balloon_stats change to 
> balloon_stats[MAX_BALLOONNODES]?
>
Comments like this are fine in an RFC patch. While at it, do that
properly, i.e., something like:

/*
 * TODO: will this nee to be converted in
 *  balloon_stats[MAX_BALLOONNODES] ?
 */


>  struct balloon_stats balloon_stats;
>  EXPORT_SYMBOL_GPL(balloon_stats);
>  
>  /* We increase/decrease in batches which fit in a page */
>  static xen_pfn_t frame_list[PAGE_SIZE / sizeof(unsigned long)];
>  
> +#ifdef CONFIG_HIGHMEM
> +#define inc_totalhigh_pages() (totalhigh_pages++)
> +#define dec_totalhigh_pages() (totalhigh_pages--)
> +#else
> +#define inc_totalhigh_pages() do {} while (0)
> +#define dec_totalhigh_pages() do {} while (0)
> +#endif
> +
>
Oh, I see... This is the stuff Jan was talking about in Xen devel, that
you probably are reintroducing after it was removed. So, I'm sorry, but
it is indeed quite hard to review something without knowing what hunks
are really new/needed.

For instance, see what happened in this changeset:

$ git show 3dcc0571cd64816309765b7c7e4691a4cadf2ee7

commit 3dcc0571cd64816309765b7c7e4691a4cadf2ee7
Author: Jiang Liu <liuj97@xxxxxxxxx>
Date:   Wed Jul 3 15:03:21 2013 -0700

    mm: correctly update zone->managed_pages
    
    ...
    
    Remove inc_totalhigh_pages() and dec_totalhigh_pages() from xen/balloon
    driver bacause adjust_managed_page_count() has already adjusted
    totalhigh_pages.

    ...

So, could you perhaps remove these? It is probably just a matter of
rebasing your changes (and only them) on a more recent git tree (e.g.,
from above)?

Once you did that, please, resend the patch, if possible with all the
additional information about the design and intended usage that both Jan
and David are asking, and we'll perform a proper review of it (or at
least, I will :-P).

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

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