Re: [PATCH v2 1/3] xen/sched: introduce cpupool_update_node_affinity()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.08.2022 13:58, Juergen Gross wrote:
> On 15.08.22 13:41, Jan Beulich wrote:
>> On 15.08.2022 13:04, Juergen Gross wrote:
>>> For updating the node affinities of all domains in a cpupool add a new
>>> function cpupool_update_node_affinity().
>>>
>>> In order to avoid multiple allocations of cpumasks carve out memory
>>> allocation and freeing from domain_update_node_affinity() into new
>>> helpers, which can be used by cpupool_update_node_affinity().
>>>
>>> Modify domain_update_node_affinity() to take an additional parameter
>>> for passing the allocated memory in and to allocate and free the memory
>>> via the new helpers in case NULL was passed.
>>>
>>> This will help later to pre-allocate the cpumasks in order to avoid
>>> allocations in stop-machine context.
>>>
>>> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
>>
>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>> with the observation that ...
>>
>>> --- a/xen/common/sched/core.c
>>> +++ b/xen/common/sched/core.c
>>> @@ -1824,9 +1824,28 @@ int vcpu_affinity_domctl(struct domain *d, uint32_t 
>>> cmd,
>>>       return ret;
>>>   }
>>>   
>>> -void domain_update_node_affinity(struct domain *d)
>>> +bool update_node_aff_alloc(struct affinity_masks *affinity)
>>>   {
>>> -    cpumask_var_t dom_cpumask, dom_cpumask_soft;
>>> +    if ( !alloc_cpumask_var(&affinity->hard) )
>>> +        return false;
>>> +    if ( !alloc_cpumask_var(&affinity->soft) )
>>> +    {
>>> +        free_cpumask_var(affinity->hard);
>>> +        return false;
>>> +    }
>>> +
>>> +    return true;
>>> +}
>>> +
>>> +void update_node_aff_free(struct affinity_masks *affinity)
>>> +{
>>> +    free_cpumask_var(affinity->soft);
>>> +    free_cpumask_var(affinity->hard);
>>> +}
>>> +
>>> +void domain_update_node_aff(struct domain *d, struct affinity_masks 
>>> *affinity)
>>> +{
>>> +    struct affinity_masks masks = { };
>>
>> ... the initializer doesn't really look to be needed here, just like
>> you don't have one in cpupool_update_node_affinity(). The one thing
>> I'm not sure about is whether old gcc might mis-report a potentially
>> uninitialized variable with the initializer dropped ...
> 
> Hmm, yes, I think the initializer was needed only in V1.
> 
> I guess you could remove it while committing in case no respin of the
> series is needed otherwise?

Sure, I'll take a note.

Jan