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

>> +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.

>> +{ >> + 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.

> 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?