Re: [PATCH v2 0/3] Clean the policy manipulation path in domain creation

[Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>]

On 20/05/2024 13:45, Roger Pau Monné wrote:
> On Fri, May 17, 2024 at 05:08:33PM +0100, Alejandro Vallejo wrote:
>> v2:
>>   * Removed xc_cpu_policy from xenguest.h
>>   * Added accessors for xc_cpu_policy so the serialised form can be 
>> extracted.
>>   * Modified xen-cpuid to use accessors.
>>
>> ==== Original cover letter ====
>>
>> In the context of creating a domain, we currently issue a lot of hypercalls
>> redundantly while populating its CPU policy; likely a side effect of
>> organic growth more than anything else.
>>
>> However, the worst part is not the overhead (this is a glacially cold
>> path), but the insane amounts of boilerplate that make it really hard to
>> pick apart what's going on. One major contributor to this situation is the
>> fact that what's effectively "setup" and "teardown" phases in policy
>> manipulation are not factored out from the functions that perform said
>> manipulations, leading to the same getters and setter being invoked many
>> times, when once each would do.
>>
>> Another big contributor is the code being unaware of when a policy is
>> serialised and when it's not.
>>
>> This patch attempts to alleviate this situation, yielding over 200 LoC
>> reduction.
>>
>> Patch 1: Mechanical change. Makes xc_cpu_policy_t public so it's usable
>>          from clients of libxc/libxg.
>> Patch 2: Changes the (de)serialization wrappers in xenguest so they always
>>          serialise to/from the internal buffers of xc_cpu_policy_t. The
>>          struct is suitably expanded to hold extra information required.
>> Patch 3: Performs the refactor of the policy manipulation code so that it
>>          follows a strict: PULL_POLICIES, MUTATE_POLICY (n times), 
>> PUSH_POLICY.
> 
> I think patch 3 is no longer part of the set?  I don't see anything
> in the review of v1 that suggests patch 3 was not going to be part of
> the next submission?
> 
> Thanks, Roger.

It's there, there was just a shift. The implication of the first line of
the changelog is that v1/patch1 was dropped. Sorry, should've been clearer.

    v1/patch1 => dropped
    v1/patch2 => v2/patch1
    v1/patch3 => v2/patch2

Cheers,
Alejandro