On 02.08.2022 15:36, Juergen Gross wrote:
> --- a/xen/common/sched/cpupool.c
> +++ b/xen/common/sched/cpupool.c
> @@ -419,6 +419,8 @@ static int cpupool_alloc_affin_masks(struct
> affinity_masks *masks)
> return 0;
>
> free_cpumask_var(masks->hard);
> + memset(masks, 0, sizeof(*masks));
FREE_CPUMASK_VAR()?
> @@ -1031,10 +1041,23 @@ static int cf_check cpu_callback(
> {
> unsigned int cpu = (unsigned long)hcpu;
> int rc = 0;
> + static struct cpu_rm_data *mem;
When you mentioned your plan, I was actually envisioning a slightly
different model: Instead of doing the allocation at CPU_DOWN_PREPARE,
allocate a single instance during boot, which would never be freed.
Did you consider such, and it turned out worse? I guess the main
obstacle would be figuring an upper bound for sr->granularity, but
of course schedule_cpu_rm_alloc(), besides the allocations, also
does quite a bit of filling in values, which can't be done up front.
> switch ( action )
> {
> case CPU_DOWN_FAILED:
> + if ( system_state <= SYS_STATE_active )
> + {
> + if ( mem )
> + {
> + if ( memchr_inv(&mem->affinity, 0, sizeof(mem->affinity)) )
> + cpupool_free_affin_masks(&mem->affinity);
I don't think the conditional is really needed - it merely avoids two
xfree(NULL) invocations at the expense of readability here. Plus -
wouldn't this better be part of ...
> + schedule_cpu_rm_free(mem, cpu);
... this anyway?
> @@ -1042,12 +1065,32 @@ static int cf_check cpu_callback(
> case CPU_DOWN_PREPARE:
> /* Suspend/Resume don't change assignments of cpus to cpupools. */
> if ( system_state <= SYS_STATE_active )
> + {
> rc = cpupool_cpu_remove_prologue(cpu);
> + if ( !rc )
> + {
> + ASSERT(!mem);
> + mem = schedule_cpu_rm_alloc(cpu);
> + rc = mem ? cpupool_alloc_affin_masks(&mem->affinity) :
> -ENOMEM;
Ah - here you actually want a non-boolean return value. No need to
change that then in the earlier patch (albeit of course a change
there could be easily accommodated here).
Along the lines of the earlier comment this 2nd allocation may also
want to move into schedule_cpu_rm_alloc(). If other users of the
function don't need the extra allocations, perhaps by adding a bool
parameter.
Jan
