Re: [Xen-devel] [PATCH v2 21/48] xen/sched: use sched_resource cpu instead smp_processor_id in schedulers

[Juergen Gross <jgross@xxxxxxxx>]

On 12.09.19 12:04, Jan Beulich wrote:
> On 12.09.2019 11:34, Juergen Gross wrote:
> > On 09.09.19 16:17, Jan Beulich wrote:
> > > On 09.08.2019 16:58, Juergen Gross wrote:
> > > > @@ -1825,8 +1825,9 @@ static struct task_slice
> > > >    csched_schedule(
> > > >        const struct scheduler *ops, s_time_t now, bool_t tasklet_work_scheduled)
> > > >    {
> > > > -    const int cpu = smp_processor_id();
> > > > -    struct list_head * const runq = RUNQ(cpu);
> > > > +    const unsigned int cpu = smp_processor_id();
> > > > +    const unsigned int sched_cpu = sched_get_resource_cpu(cpu);
> > > > +    struct list_head * const runq = RUNQ(sched_cpu);
> > > By retaining a local variable named "cpu" you make it close to
> > > impossible to notice, during a re-base, an addition to the
> > > function still referencing a variable of this name. Similarly
> > > review is being made harder because one needs to go hunt all
> > > the remaining uses of "cpu". For example there a trace entry
> > > being generated, and it's not obvious to me whether this wouldn't
> > > better also used sched_cpu.
> > Okayy, I'll rename "cpu" to "my_cpu".
> We've got a number of instances of "this_cpu" in such cases already,
> but no single "my_cpu". May I suggest to stick to this naming here
> as well?
>
> > I used cpu in the trace entry on purpose, as it might be interesting on
> > which cpu the entry has been produced.
> Right, that's how I understood it; it simply seemed like there
> might be a similarly valid view to the contrary.
>
> > > > @@ -1967,7 +1968,7 @@ csched_schedule(
> > > >        if ( snext->pri > CSCHED_PRI_TS_OVER )
> > > >            __runq_remove(snext);
> > > >        else
> > > > -        snext = csched_load_balance(prv, cpu, snext, &ret.migrated);
> > > > +        snext = csched_load_balance(prv, sched_cpu, snext, &ret.migrated);
> > > And in a case like this one I wonder whether passing a "sort of
> > > CPU" isn't sufficiently confusing, compared to e.g. simply
> > > passing the corresponding unit.
> > I guess you mean sched_resource.
> Not sure - with scheduling acting on units, it would seem to me that
> passing around the unit pointers would be the most appropriate thing.
>
> > I don't think changing the parameter type is a good idea. We need both
> > (resource and cpu number) on caller and callee side, but the main
> > object csched_load_balance() is working on is the cpu number.
> I see. Part of my thinking here also was towards the added type
> safety if passing pointers instead of numeric values.
>
> > > > @@ -1975,12 +1976,12 @@ csched_schedule(
> > > >         */
> > > >        if ( !tasklet_work_scheduled && snext->pri == CSCHED_PRI_IDLE )
> > > >        {
> > > > -        if ( !cpumask_test_cpu(cpu, prv->idlers) )
> > > > -            cpumask_set_cpu(cpu, prv->idlers);
> > > > +        if ( !cpumask_test_cpu(sched_cpu, prv->idlers) )
> > > > +            cpumask_set_cpu(sched_cpu, prv->idlers);
> > > >        }
> > > > -    else if ( cpumask_test_cpu(cpu, prv->idlers) )
> > > > +    else if ( cpumask_test_cpu(sched_cpu, prv->idlers) )
> > > >        {
> > > > -        cpumask_clear_cpu(cpu, prv->idlers);
> > > > +        cpumask_clear_cpu(sched_cpu, prv->idlers);
> > > >        }
> > > And this looks to be a pretty gross abuse of CPU masks then.
> > > (Nevertheless I can see that using a CPU as a vehicle here is
> > > helpful to limit the scope of the already long series, but I
> > > think it needs to be made much more apparent what is meant.)
> > I don't think it is an abuse. Think of it as a cpumask where only
> > the bits related to the resource's master_cpus can be set.
> Well, I understand that this was your thinking behind retaining
> the use of CPU masks here. With the "master_cpu" naming it may
> indeed end up looking less abuse-like, but I still wonder (as
> also suggested elsewhere) whether an ID concept similar to that
> of APIC ID vs (derived) core/socket/node ID wouldn't be better
> in cases where an ID is to represent multiple CPUs.
Thinking of a future support of cpupools with different scheduling
granularity I don't think this is a good idea. The only way to not
letting that become rather awful would be to let the ID have the
same numerical value as the cpu today and duplicate the cpumask
stuff into a resourcemask framework compoletely the same but with
different names.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel