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?
Hmm, don't you think adding further overloading of "this_cpu" is a bad
idea?

> > 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 guess there is a reason why csched_load_balance() takes cpu and not
a vcpu pointer as parameter today. Changing that might be possible, but
I don't think it should be part of this patch series.


Juergen

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