Re: [PATCH v6 08/10] libxl: build numa nodes memory blocks

[Wei Liu <wei.liu2@citrix.com>]

On Sun, Jul 20, 2014 at 08:58:14AM -0400, Elena Ufimtseva wrote:
> On Fri, Jul 18, 2014 at 7:01 AM, Wei Liu <wei.liu2@citrix.com> wrote:
> >
> > On Fri, Jul 18, 2014 at 01:50:07AM -0400, Elena Ufimtseva wrote:
> > [...]
> > >
> > > +bool libxl__vnodemap_is_usable(libxl__gc *gc, libxl_domain_build_info *info);
> > > +
> > > +int e820_sanitize(libxl_ctx *ctx, struct e820entry src[],
> > > +                                  uint32_t *nr_entries,
> > > +                                  unsigned long map_limitkb,
> > > +                                  unsigned long balloon_kb);
> > > +
> >
> Hi Wei
> 
> Thanks for comments.
> 
> >
> > e820_sanitize should not take a ctx? It's internal function anyway.
> > But this is not your fault so don't worry about it.
> >
> > And this function seems to be arch-specific so I wonder if there's
> > better place for it.
> 
> 
> I do feel the same way. Would be libxl_arch.h is a better place?

I think this one is for Ian and Ian.

> >
> >
> > > +int libxl__vnuma_align_mem(libxl__gc *gc,
> > > +                                     uint32_t domid,
> > > +                                     struct libxl_domain_build_info *b_info,
> > > +                                     vmemrange_t *memblks);
> > > +
> >
> > Indentation.
> >
> > > +/*
> > > + * For each node, build memory block start and end addresses.
> > > + * Substract any memory hole from the range found in e820 map.
> > > + * vnode memory size are passed here in megabytes, the result is
> > > + * in memory block addresses.
> > > + * Linux kernel will adjust numa memory block sizes on its own.
> > > + * But we want to provide to the kernel numa block addresses that
> > > + * will be the same in kernel and hypervisor.
> > > + */
> > > +#define max(a,b) ((a > b) ? a : b)
> >
> > You won't need to redefine max, I think we have this somewhere already.
> 
> I think there is a patch in libxc (from Andrew Cooper) "tools/libxc:
> Shuffle definitions and uses of min()/max() macros"
> This one you are talking about?

I think so.

Wei.