Re: [Xen-devel] [PATCH 4/9] xen: sched: add .init_pdata hook to the scheduler interface

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.10.15 at 11:26, <dario.faggioli@xxxxxxxxxx> wrote:
> On Thu, 2015-10-01 at 02:17 -0600, Jan Beulich wrote:
>> > > > On 29.09.15 at 18:55, <dario.faggioli@xxxxxxxxxx> wrote:
>> > This happens because, right now, the scheduler of the
>> > target pool remaps the runqueue lock during (rt_)alloc_pdata,
>> > which is called at the very beginning of schedule_cpu_switch().
>> > Credit2's csched2_free_pdata(), however, wants to find the spin
>> > lock the same way it was put by csched2_alloc_pdata(), and
>> > explodes, it not being the case.
>> > 
>> > This patch also fixes this as, now, spin lock remapping
>> > happens in the init_pdata hook of the target scheduler for
>> > the pCPU, which we can easily make sure it is ivoked *after*
>> > the free_pdata hook of the old scheduler (which is exactly
>> > what is done in this patch), without needing to restructure
>> > schedule_cpu_switch().
>> 
>> Hmm, while I can see this to be okay for the
>> cpupool_unassign_cpu_helper() case (due to the call to
>> cpu_disable_scheduler() up front) as well as for the
>> cpupool_cpu_add() one (I take it that the CPU can't be
>> scheduled on in that case), I don't see how this would be
>> safe for the XEN_SYSCTL_CPUPOOL_OP_ADDCPU case: How is
>> scheduling activity being prevented to happen after the new
>> ppriv got installed, but before it gets initialized via the
>> init_pdata
>> hook? Or how would scheduling activity be safe with an allocated
>> but not yet initialized ppriv?
>> 
> Not sure I'm getting, but let me try.
> 
> XEN_SYSCTL_CPUPOOL_OP_ADDCPU is what adds a CPU to a pool. If we are
> adding a CPU to a pool, it means the CPU is currently free, i.e.,
> outside of any pool. This, in turn, means no pool has the CPU's bit set
> in its cpupool->cpu_valid mask, which is what we check, within the
> scheduler, to see where we can schedule the vcpus subject to a
> particular scheduler.
> 
> If no pool has that bit set in its mask, nothing is being scheduled on
> that CPU. (Actually, making sure that that is really the case, has been
> the subject of some patches I sent back in July.)
> 
> Ok, now XEN_SYSCTL_CPUPOOL_OP_ADDCPU calls cpupool_assign_cpu_locked(),
> which calls schedule_cpu_switch(cpu --> c) and, *only* *after* *that*,
> does the following:
> 
>     cpumask_clear_cpu(cpu, &cpupool_free_cpus);
>     ...
>     cpumask_set_cpu(cpu, c->cpu_valid);
> 
> So, still assuming that I got your question right, the answer is:
> scheduling activity was stopped already, and is restarted only after
> both allocation and initialization have happened.

Okay. The thing is that looking at schedule_cpu_switch() alone
(and also considering its name) it is not clear that all callers either
move the CPU into unusable state (from scheduling pov) or out
of it, but never between CPUs from usable to usable. No
assertion, no comment, nothing. IOW even if not an active bug,
at least a latent one with your changes.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel