Re: [xen-devel][vNUMA v2][PATCH 1/8] Config options

[Andre Przywara <andre.przywara@xxxxxxx>]

Dulloor wrote:
> On Tue, Aug 3, 2010 at 5:40 AM, Andre Przywara <andre.przywara@xxxxxxx> wrote:
> > Dulloor wrote:
> > > Implement the following config options :
> > >
> > > strategy = "str", where str is confine/stripe/split/auto
> > > vnodes = <num-nodes>
> > > stripesz = <size-in-pages>
> > As Ian already said, I'd also prefer NUMA related names instead of
> > the generic "strategy". Also "vnodes" may be a bit misleading, what about
> > "guestnodes"?
> That's right. How about "numa_strategy", "numa_vnodes", and "numa_stripesz" ?
The numa_vnodes looks quite cumbersome and isn't very intuitive in 
understanding. I'd prefer numa_guestnodes (although this is a bit long) 
cause it is clear in what it means. But I can also live with numa_vnodes 
if this is put in the example config file together with an appropriate 
comment.
> > > +typedef struct xc_domain_numa_config
> > > +{
> > > +    uint32_t strategy;      /* By default, DONTCARE (for now) */
> > > +    uint32_t nr_nodes;      /* For SPLIT/STRIPE */
> > > +    uint32_t stripe_size;   /* For STRIPE only */
> > Are 32 bit here sufficient? Although for the stripe size probably 4GB
> > are more than needed, I'd prefer to use 64bit (or long) for each
> > memory-related variable.
> Here, stripe_size is in 4K pages, so effectively we have 44 bits,
> which should suffice.
> But, for consistency, I will change that to 64-bit.
Thanks. I think we all should have learned from the "640 KB is enough" 
story ;-)
> > > +} xc_domain_numa_config_t;
> > > +
> > > +++ b/tools/libxl/xl_cmdimpl.c
> > > +static uint32_t numa_str_to_val(const char *str)
> > > +{
> > > +    if (!strcasecmp(str, "AUTO"))
> > > +        return XC_DOM_NUMA_AUTO;
> > > +    if (!strcasecmp(str, "CONFINE"))
> > > +        return XC_DOM_NUMA_CONFINE;
> > > +    if (!strcasecmp(str, "SPLIT"))
> > > +        return XC_DOM_NUMA_SPLIT;
> > > +    if (!strcasecmp(str, "STRIPE"))
> > > +        return XC_DOM_NUMA_STRIPE;
> > > +
> > > +    return XC_DOM_NUMA_NONE;
> > Shouldn't the function return something like "unknown" here?
> > This would allow to detect typos in the config file.
> I  was thinking that if someone misconfigures (or for typos), we fall back
> to the current case. But, it makes sense not to do that. Will change this.
I was already seeing the mails on xen-devel asking why 
numa_strategy=strip doesn't make a difference ;-) If we have only a few 
possible strategies, we should fail on illegal ones. That would also 
make later extension easier to use, as they fail on older setups and the 
user gets a hint on this.
Regards,
Andre.


--
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 448-3567-12


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel