On 08.08.2022 12:21, Juergen Gross wrote:
> On 03.08.22 11:53, Jan Beulich wrote:
>> On 02.08.2022 15:36, Juergen Gross wrote:
>>> 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 -
>
> Okay.
>
>> wouldn't this better be part of ...
>>
>>> + schedule_cpu_rm_free(mem, cpu);
>>
>> ... this anyway?
>
> This would add a layering violation IMHO.
>
>>
>>> @@ -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.
>
> I could do that, but I still think this would pull cpupool specific needs
> into sched/core.c.
But the struct isn't cpupool specific, and hence controlling the setting up
of the field via a function parameter doesn't really look like a layering
violation to me. While imo the end result would be more clean (as in - all
allocations / freeing in one place), I'm not going to insist (not the least
because I'm not maintainer of that code anyway).
Jan
