Re: [libvirt] [PATCH v2 1/2] add nodeset='all' for interleave mode

On 9/29/18 6:58 AM, Peng Hao wrote: Not so sure you followed Michal's v1 review comments with respect to needing an RNG update and test coverage (xml2xml and xml2argv). For the RNG, look at docs/schemas/*.rng and search on "nodeset" and "cpuset". Eventually you will find where nodeset gets defined for the schema and perhaps be able to figure out a way to adjust the pattern that's allowable for the schema. The XML file you create that would have "all" would need to pass a virt-xml-validate type command. For the xml2xml and xml2argv you will need to make sure your grammar (RNG) change works for XML processing and it would create the command line you expect. Next, do you think the very narrow band of numatune / memory / nodeset would be the only "set" to benefit from having being able to specify "in some manner" all the for a set? As noted above nodeset and cpuset are reusable throughout libvirt in various forms. Dig deeper - think of how that string is presented and parsed (hint: see virBitmap*). Knowing how many "nodes" there are is part of the <numa> child of <cpu>, right? While knowing that "all" means one thing "today" it could mean something different on some subsequent change and if the numa topology is changed there's lots of code currently around that would seemingly need to handle "all". Anyway, back to the cpuset/nodeset presentation. If today it's "A,C,E" or "A-E,^B,^D", then why not devise a way to use "0,~0" meaning 0 and the one's complement of 0 (e.g. everything else). That way if I wanted to, I could "0,~0,^4" meaning everything except 4. Of course the downside is that I need to know what the guest maximum is. The only way to know that is in post parse processing. So doing this may involve moving the translation of @nodeset and @cpuset... Whether any of this is of use to anyone - who knows. I just think using "all" is not the best idea. If you've defined your guest to have 4 vCPUs and/or use 4 NUMA nodes, then it should be very simple to say 0-3 for the cpuset or nodeset. Allowing "all" is in a way the lazy way and may not be true at some point in the future. Maybe someone adds a new node or vCPU to the guest and wants it to perform some other specific task. If adding that "misses" someone setting "all" for some nodeset, then that usage model would be unexpected. As an aside, are there hugepage implications to this? Or other nodeset configuration options related to NUMA that I'm not as aware of... Personally, probably should stick to only having src/util/virnuma.c do this. What does it matter? Seems to me there'd already be some check related to NUMA availability in the event some nodeset range was defined. Doing this in grammar checking doesn't feel right. I took a cursory look, but you can dig deeper yourself to understand what happens if nodeset is defined and NUMA isn't available otherwise. ^ The alignment here is wrong. So "all" would mean we wait until some time in the future to determine what that means as opposed to setting those up now. What happens to various checks that may expect that numa->memory.nodeset contains something? What other checks are out there that will be missed? There would be no need for the == true comparison, it's a boolean operation. Method name doesn't adhere to standards in https://libvirt.org/hacking.html Consider, if (!(bitmap = xxx()))) I see a virNumaGetMaxNode... So are we apply "all" to the guest or host configuration? I guess that part just isn't clear enough to me. Configuring NUMA isn't in my normal list of things to look at, but since the patch has been sitting for a bit I've looked. So what is the value of @maxnode in relation to the value of VIR_DOMAIN_CPUMASK_LEN (1024)? Less I hope... Furthermore if counting starts at 0, then it should be < not <=, right? ^^^^ This wouldn't be static in a src/util/vir*.c module... And @numa would be ATTRIBUTE_UNUSED; otherwise, there'd be compiler issues. This would be wrong because there'd be a failure that some parsing failing for some unknown reason. You'd need to add a real error message. So if the next hunk would theoretically fill in numa->memory.nodeset for you, then why modify this check now? Why wouldn't the next hunk go above this hunk? Seems to me XML post parse processing would have needed to check that if you had this allnode set, then you better also have static and interleave (and of course that Numa is available). That way, all you'd have to do is determine that allnode was set and call your function to do that. Those are "my" thoughts on the matter. Still not clear as to the need. I'm 50/50 on it. I see benefit, but there are concerns especially as how things relate to migration and domain save/restore. John