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

[John Ferlan]

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

[...]