From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andre Przywara Subject: Re: [vNUMA v2][PATCH 1/8] Config options Date: Tue, 3 Aug 2010 23:28:31 +0200 Message-ID: <4C5889FF.1040500@amd.com> References: <1BEA8649F0C00540AB2811D7922ECB6C9338B4CB@orsmsx507.amr.corp.intel.com> <4C580E29.1090302@amd.com> Mime-Version: 1.0 Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Dulloor Cc: "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org Dulloor wrote: > On Tue, Aug 3, 2010 at 5:40 AM, Andre Przywara wrote: >> Dulloor wrote: >>> Implement the following config options : >>> >>> strategy = "str", where str is confine/stripe/split/auto >>> vnodes = >>> stripesz = >> 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