[libvirt] [PATCH 7/9] conf: Refactor virDomainCachetuneDefParse

[bing.niu]

On 2018年07月27日 01:48, John Ferlan wrote:
> 
> 
> On 07/18/2018 03:57 AM, bing.niu at intel.com wrote:
>> From: Bing Niu <bing.niu at intel.com>
>>
>> Refactoring virDomainCachetuneDefParse, extracting vcpus extracting,
>> overlapping detecting and new resctrl allocation creating logic.
>> Those two logic is common among different resource allocation
>> technologies.
>>
>> Signed-off-by: Bing Niu <bing.niu at intel.com>
>> ---
>>   src/conf/domain_conf.c | 195 +++++++++++++++++++++++++++++++++----------------
>>   1 file changed, 131 insertions(+), 64 deletions(-)
>>
> 
> You could make 3 patches out of this - one for each reduction of
> virDomainCachetuneDefParse...

Will split that.
> 
> The virDomainFindResctrlAlloc should have used Restune instead of
> ResctrlAlloc, right? Of course that changes based on how the naming
> patch shakes out. Actually I think perhaps virDomainRestuneVcpuMatch
> would be even better (where Restune ends up being whatever shakes out of
> the previous patch naming decision).

Let's use virDomainResctrlVcpuMatch. :)
> 
> In this area, I'm wondering what purpose does @new_alloc serve?  If
> @alloc was already defined, then you added an error message. So the
> reality is - @new_alloc is always true.

As you already note in patch 8. This is used by memory bandwidth to find 
a resctrl group defined by CAT already.
> 
> Secondary to that you've added a new error related to identical vcpus in
> the parsing logic. Unfortunately that could make a domain disappear on a
> libvirtd restart if for some reason it was already defined in that
> manner. If there's a parsing error because two entries had identical
> cpus, then there will be an error. Errors would cause a previously
> OK/found domain to not be defined. So that type of check (now that the

Sorry, I am not sure if I understand correctly. Are you talking about 
two domains(or VMs) with the same vcpus setting?
If so, above vpu match is not for different domain but for the memory 
bandwidth allocation and the cache line allocation of one domain.

And above vcpu match function doesn't change any logic of existing 
virDomainCachetuneDefParseCache. It just extract from 
virDomainCachetuneDefParseCache and packing into one function. So that 
memory bandwidth parsing logic in patch 8 can reuse this vcpu matching 
part. :)
> code has been around for more than a release) would need to go in some
> sort of Validation API. This is one of those libvirt define and libvirtd
> restart quirks.

If my understanding is incorrect. Could you please clarify here or give 
me some example? :)
> 
> Finally, virDomainUpdateRestune should be virDomainRestuneUpdate since
> you started with virDomainRestuneParseVcpus and it should go after
> virDomainCachetuneDefParseCache
> 

Let's use virDomainResctrlUpdate
> Hope this all makes sense!
> 
> John
> 
[....]