Re: [libvirt] [PATCH 05/10] util: resctrl: refactoring some functions

-----Original Message----- From: John Ferlan [mailto:jferlan@redhat.com] Sent: Saturday, September 8, 2018 1:41 AM To: Wang, Huaqiang <huaqiang.wang(a)intel.com>; libvir-list(a)redhat.com Cc: Feng, Shaohe <shaohe.feng(a)intel.com>; Niu, Bing <bing.niu(a)intel.com>; Ding, Jian-feng <jian-feng.ding(a)intel.com>; Zang, Rui <rui.zang(a)intel.com&gt; Subject: Re: [libvirt] [PATCH 05/10] util: resctrl: refactoring some functions On 09/07/2018 06:52 AM, Wang, Huaqiang wrote: > > [...] >>> +static int >>> +virResctrlCreateGroup(virResctrlInfoPtr resctrl, >>> + char *path) >> >> s/char/const char/ >> > > Will be fixed. > >> should be: >> >> virResctrlCreateGroupPath >> > > I prefer the original name ' virResctrlCreateGroup' than 'virResctrlCreateGroupPath'. > The main role of this function is to make a directory, and the > directory is called 'resource group' in kernel's document. See > document https://www.kernel.org/doc/Documentation/x86/intel_rdt_ui.txt > If you still think 'virResctrlCreateGroupPath' is better, OK, let's change it. > I don't really care... I also don't follow the kernel naming rules.

>>> +{ >>> + int ret = -1; >>> + int lockfd = -1; >>> + >>> + if (!path) >>> + return -1; >> >> This would cause some sort of unknown error, but it's a caller bug >> isn't it? That is if @path is empty before calling in here, then >> we've missed some other condition, so in this instance it doesn't quite make sense. >> > > OK. I need to pay more attention on these places that could cause 'unknown error'. > >>> + >>> + if (virResctrlInfoIsEmpty(resctrl)) { >>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", >>> + _("Resource control is not supported on this host")); >>> + return -1; >>> + } >> >> Not quite sure what this has to do with creating the GroupPath. > > Does 'this' mean the invoking of ' virResctrlInfoIsEmpty'? > virResctrlInfoIsEmpty return true ensures that the 'resctrl fs' is supported here. > Yes and I know why it was called, but the rest of the text explained why I thought with overkill thrown in for good measure. >> Feels like some >> that should be in the caller, but I guess that depends on future >> usage.... I see this helper is called in the next patch by >> virResctrlAllocCreateMonitor which isn't used until patch9 and only called once/if virResctrlAllocCreate is successful. >> > > Awesome, your feeling is right. > My design is, virResctrlAllocCreate creates an resource 'allocation', > and virResctrlAllocCreateMonitor creates a resource 'monitor'. The > 'monitor' belongs to some specific 'allocation'. If you want to create > a 'monitor', an 'allocation' must be created already, and link the 'monitor' to the 'allocation'. > So when virResctrlAllocCreateMonitor is called, the > virResctrlAllocCreate must be called successfully. > And the ' virResctrlInfoIsEmpty' is checked more than one times. > Will move the call of virResctrlInfoIsEmpty into virResctrlAllocCreate. > >> So it doesn't seem that calling it once for each time >> virResctrlAllocCreateMonitor is called is really necessary since >> @resctrl doesn't change. >> >> In fact, going back to qemuProcessResctrlCreate it would seem that >> calling virResctrlAllocCreate once for each vm->def->nresctrls would >> also be somewhat inefficient since caps->host.resctrl (a/k/a >> @resctrl) doesn't change. But moving it back there may mean needing >> to check if >> vm->def->resctrls[i]->alloc is NULL... >> >> I think perhaps some more thought needs to be placed on "efficient" >> code paths before adding the monitor code paths. > > Confused. Do you still talking about the mult-call over function virResctrlInfoIsEmpty? > In the long run it's perhaps a "cheap call", but using a "static int" means you can control who calls it and whether they have checked virResctrlInfoIsEmpty prior to calling thus this can assume it has. Having multiple functions calling IsEmpty is overkill.

John [...]