> -----Original Message----- > From: John Ferlan [mailto:jferlan@xxxxxxxxxx] > Sent: Saturday, September 8, 2018 1:41 AM > To: Wang, Huaqiang <huaqiang.wang@xxxxxxxxx>; libvir-list@xxxxxxxxxx > Cc: Feng, Shaohe <shaohe.feng@xxxxxxxxx>; Niu, Bing <bing.niu@xxxxxxxxx>; > Ding, Jian-feng <jian-feng.ding@xxxxxxxxx>; Zang, Rui <rui.zang@xxxxxxxxx> > Subject: Re: [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. > Let's use the name virResctrlCreateGroupPath. > >>> +{ > >>> + 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. > Thanks for clarification. I'll check code and make sure virResctrlInfoIsEmpty is not necessarily called in next version patches. Thanks for review. Huaqinag > John > > [...] -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
