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

From: Juergen Gross <jgross@suse.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: George Dunlap <george.dunlap@citrix.com>,
	Dario Faggioli <dfaggioli@suse.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Julien Grall <julien@xen.org>,
	Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH v2 1/3] xen/sched: introduce cpupool_update_node_affinity()
Date: Mon, 15 Aug 2022 13:58:34 +0200	[thread overview]
Message-ID: <1e86164f-b08c-a9bd-3437-af569342cc1e@suse.com> (raw)
In-Reply-To: <9abc061b-2d98-ace8-af4b-0881a5737dd6@suse.com>

[-- Attachment #1.1.1: Type: text/plain, Size: 2278 bytes --]

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@suse.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 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?

Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]