diff mbox series

[v2,21/48] xen/sched: use sched_resource cpu instead smp_processor_id in schedulers

Message ID 20190809145833.1020-22-jgross@suse.com (mailing list archive)
State Superseded
Headers show
Series xen: add core scheduling support | expand

Commit Message

Jürgen Groß Aug. 9, 2019, 2:58 p.m. UTC
Especially in the do_schedule() functions of the different schedulers
using smp_processor_id() for the local cpu number is correct only if
the sched_unit is a single vcpu. As soon as larger sched_units are
used most uses should be replaced by the cpu number of the local
sched_resource instead.

Add a helper to get that sched_resource cpu and modify the schedulers
to use it in a correct way.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/sched_arinc653.c |  2 +-
 xen/common/sched_credit.c   | 21 +++++++++---------
 xen/common/sched_credit2.c  | 53 +++++++++++++++++++++++----------------------
 xen/common/sched_null.c     | 17 ++++++++-------
 xen/common/sched_rt.c       | 17 ++++++++-------
 xen/common/schedule.c       |  2 +-
 xen/include/xen/sched-if.h  |  5 +++++
 7 files changed, 63 insertions(+), 54 deletions(-)

Comments

Jan Beulich Sept. 9, 2019, 2:17 p.m. UTC | #1
On 09.08.2019 16:58, Juergen Gross wrote:
> Especially in the do_schedule() functions of the different schedulers
> using smp_processor_id() for the local cpu number is correct only if
> the sched_unit is a single vcpu. As soon as larger sched_units are
> used most uses should be replaced by the cpu number of the local
> sched_resource instead.

I have to admit that I don't follow this argument, not the least because
(as I think I had indicated before) it is unclear to me what _the_ (i.e.
single) CPU for a sched unit is. I've gone back to patches 4 and 7
without finding what the conceptual model behind this is intended to be.
Besides an explanation I think one or both of those two patches also
want to be revisited wrt the use of the name "processor" for the
respective field.

> --- a/xen/common/sched_credit.c
> +++ b/xen/common/sched_credit.c
> @@ -1684,7 +1684,7 @@ csched_load_balance(struct csched_private *prv, int cpu,
>      int peer_cpu, first_cpu, peer_node, bstep;
>      int node = cpu_to_node(cpu);
>  
> -    BUG_ON( cpu != sched_unit_cpu(snext->unit) );
> +    BUG_ON( sched_get_resource_cpu(cpu) != sched_unit_cpu(snext->unit) );

In cases like this one, would you mind dropping the stray blanks
immediately inside the parentheses?

> @@ -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.

> @@ -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.

> @@ -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.)

> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -112,7 +112,7 @@ static struct task_slice sched_idle_schedule(
>      const unsigned int cpu = smp_processor_id();
>      struct task_slice ret = { .time = -1 };
>  
> -    ret.task = sched_idle_unit(cpu);
> +    ret.task = sched_idle_unit(sched_get_resource_cpu(cpu));

Shouldn't sched_idle_unit(cpu) == sched_idle_unit(sched_get_resource_cpu(cpu))
here?

Jan
Jürgen Groß Sept. 12, 2019, 9:34 a.m. UTC | #2
On 09.09.19 16:17, Jan Beulich wrote:
> On 09.08.2019 16:58, Juergen Gross wrote:
>> Especially in the do_schedule() functions of the different schedulers
>> using smp_processor_id() for the local cpu number is correct only if
>> the sched_unit is a single vcpu. As soon as larger sched_units are
>> used most uses should be replaced by the cpu number of the local
>> sched_resource instead.
> 
> I have to admit that I don't follow this argument, not the least because
> (as I think I had indicated before) it is unclear to me what _the_ (i.e.
> single) CPU for a sched unit is. I've gone back to patches 4 and 7
> without finding what the conceptual model behind this is intended to be.
> Besides an explanation I think one or both of those two patches also
> want to be revisited wrt the use of the name "processor" for the
> respective field.

Fair point. Naming it "master_cpu" in struct sched_resource and when
referencing it seems to be a good idea.

> 
>> --- a/xen/common/sched_credit.c
>> +++ b/xen/common/sched_credit.c
>> @@ -1684,7 +1684,7 @@ csched_load_balance(struct csched_private *prv, int cpu,
>>       int peer_cpu, first_cpu, peer_node, bstep;
>>       int node = cpu_to_node(cpu);
>>   
>> -    BUG_ON( cpu != sched_unit_cpu(snext->unit) );
>> +    BUG_ON( sched_get_resource_cpu(cpu) != sched_unit_cpu(snext->unit) );
> 
> In cases like this one, would you mind dropping the stray blanks
> immediately inside the parentheses?

Will do.

> 
>> @@ -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".

I used cpu in the trace entry on purpose, as it might be interesting on
which cpu the entry has been produced.

> 
>> @@ -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.

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.

> 
>> @@ -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.

> 
>> --- a/xen/common/schedule.c
>> +++ b/xen/common/schedule.c
>> @@ -112,7 +112,7 @@ static struct task_slice sched_idle_schedule(
>>       const unsigned int cpu = smp_processor_id();
>>       struct task_slice ret = { .time = -1 };
>>   
>> -    ret.task = sched_idle_unit(cpu);
>> +    ret.task = sched_idle_unit(sched_get_resource_cpu(cpu));
> 
> Shouldn't sched_idle_unit(cpu) == sched_idle_unit(sched_get_resource_cpu(cpu))
> here?

Yes.


Juergen
Jan Beulich Sept. 12, 2019, 10:04 a.m. UTC | #3
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.

Jan
Jürgen Groß Sept. 12, 2019, 11:03 a.m. UTC | #4
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
Jürgen Groß Sept. 12, 2019, 11:17 a.m. UTC | #5
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
Jan Beulich Sept. 12, 2019, 11:46 a.m. UTC | #6
On 12.09.2019 13:17, Juergen Gross wrote:
> 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?

Not at all, no. A function-like macro and a variable of the same
name will happily coexist.

Jan
Jürgen Groß Sept. 12, 2019, 11:53 a.m. UTC | #7
On 12.09.19 13:46, Jan Beulich wrote:
> On 12.09.2019 13:17, Juergen Gross wrote:
>> 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?
> 
> Not at all, no. A function-like macro and a variable of the same
> name will happily coexist.

I am aware that this is working correctly.

I just think such overloading isn't helping for readability and ease
of modification.

In the end I'm not feeling strong here, so in case there are no
objections I'll go with this_cpu.


Juergen
Jan Beulich Sept. 12, 2019, 12:08 p.m. UTC | #8
On 12.09.2019 13:53, Juergen Gross wrote:
> On 12.09.19 13:46, Jan Beulich wrote:
>> On 12.09.2019 13:17, Juergen Gross wrote:
>>> On 12.09.19 12:04, Jan Beulich wrote:
>>>> On 12.09.2019 11:34, Juergen Gross wrote:
>>>>> 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?
>>
>> Not at all, no. A function-like macro and a variable of the same
>> name will happily coexist.
> 
> I am aware that this is working correctly.
> 
> I just think such overloading isn't helping for readability and ease
> of modification.
> 
> In the end I'm not feeling strong here, so in case there are no
> objections I'll go with this_cpu.

Okay, so let's consider another alternative: cur_cpu? What I
sincerely dislike are identifiers of the my_* form, for being
apparently common in absolute beginner examples.

Jan
Jürgen Groß Sept. 12, 2019, 12:13 p.m. UTC | #9
On 12.09.19 14:08, Jan Beulich wrote:
> On 12.09.2019 13:53, Juergen Gross wrote:
>> On 12.09.19 13:46, Jan Beulich wrote:
>>> On 12.09.2019 13:17, Juergen Gross wrote:
>>>> On 12.09.19 12:04, Jan Beulich wrote:
>>>>> On 12.09.2019 11:34, Juergen Gross wrote:
>>>>>> 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?
>>>
>>> Not at all, no. A function-like macro and a variable of the same
>>> name will happily coexist.
>>
>> I am aware that this is working correctly.
>>
>> I just think such overloading isn't helping for readability and ease
>> of modification.
>>
>> In the end I'm not feeling strong here, so in case there are no
>> objections I'll go with this_cpu.
> 
> Okay, so let's consider another alternative: cur_cpu? What I

Yes, I like that one better. :-)

> sincerely dislike are identifiers of the my_* form, for being
> apparently common in absolute beginner examples.

We should try to avoid that, yes. :-D

Amazing - there is no my_* identifier in the hypervisor yet.


Juergen
diff mbox series

Patch

diff --git a/xen/common/sched_arinc653.c b/xen/common/sched_arinc653.c
index 213bc960ef..e48f2b2eb9 100644
--- a/xen/common/sched_arinc653.c
+++ b/xen/common/sched_arinc653.c
@@ -513,7 +513,7 @@  a653sched_do_schedule(
     static unsigned int sched_index = 0;
     static s_time_t next_switch_time;
     a653sched_priv_t *sched_priv = SCHED_PRIV(ops);
-    const unsigned int cpu = smp_processor_id();
+    const unsigned int cpu = sched_get_resource_cpu(smp_processor_id());
     unsigned long flags;
 
     spin_lock_irqsave(&sched_priv->lock, flags);
diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
index 4ce0f7668a..87cb62c632 100644
--- a/xen/common/sched_credit.c
+++ b/xen/common/sched_credit.c
@@ -1684,7 +1684,7 @@  csched_load_balance(struct csched_private *prv, int cpu,
     int peer_cpu, first_cpu, peer_node, bstep;
     int node = cpu_to_node(cpu);
 
-    BUG_ON( cpu != sched_unit_cpu(snext->unit) );
+    BUG_ON( sched_get_resource_cpu(cpu) != sched_unit_cpu(snext->unit) );
     online = cpupool_online_cpumask(c);
 
     /*
@@ -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);
     struct sched_unit *unit = current->sched_unit;
     struct csched_unit * const scurr = CSCHED_UNIT(unit);
     struct csched_private *prv = CSCHED_PRIV(ops);
@@ -1937,7 +1938,7 @@  csched_schedule(
     {
         BUG_ON( is_idle_unit(unit) || list_empty(runq) );
         /* Current has blocked. Update the runnable counter for this cpu. */
-        dec_nr_runnable(cpu);
+        dec_nr_runnable(sched_cpu);
     }
 
     snext = __runq_elem(runq->next);
@@ -1947,7 +1948,7 @@  csched_schedule(
     if ( tasklet_work_scheduled )
     {
         TRACE_0D(TRC_CSCHED_SCHED_TASKLET);
-        snext = CSCHED_UNIT(sched_idle_unit(cpu));
+        snext = CSCHED_UNIT(sched_idle_unit(sched_cpu));
         snext->pri = CSCHED_PRI_TS_BOOST;
     }
 
@@ -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);
 
     /*
      * Update idlers mask if necessary. When we're idling, other CPUs
@@ -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);
     }
 
     if ( !is_idle_unit(snext->unit) )
diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index e3ac9c5460..548b87af8b 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -3448,7 +3448,8 @@  static struct task_slice
 csched2_schedule(
     const struct scheduler *ops, s_time_t now, bool tasklet_work_scheduled)
 {
-    const int cpu = smp_processor_id();
+    const unsigned int cpu = smp_processor_id();
+    const unsigned int sched_cpu = sched_get_resource_cpu(cpu);
     struct csched2_runqueue_data *rqd;
     struct sched_unit *currunit = current->sched_unit;
     struct csched2_unit * const scurr = csched2_unit(currunit);
@@ -3460,22 +3461,22 @@  csched2_schedule(
     SCHED_STAT_CRANK(schedule);
     CSCHED2_UNIT_CHECK(currunit);
 
-    BUG_ON(!cpumask_test_cpu(cpu, &csched2_priv(ops)->initialized));
+    BUG_ON(!cpumask_test_cpu(sched_cpu, &csched2_priv(ops)->initialized));
 
-    rqd = c2rqd(ops, cpu);
-    BUG_ON(!cpumask_test_cpu(cpu, &rqd->active));
+    rqd = c2rqd(ops, sched_cpu);
+    BUG_ON(!cpumask_test_cpu(sched_cpu, &rqd->active));
 
-    ASSERT(spin_is_locked(get_sched_res(cpu)->schedule_lock));
+    ASSERT(spin_is_locked(get_sched_res(sched_cpu)->schedule_lock));
 
     BUG_ON(!is_idle_unit(currunit) && scurr->rqd != rqd);
 
     /* Clear "tickled" bit now that we've been scheduled */
-    tickled = cpumask_test_cpu(cpu, &rqd->tickled);
+    tickled = cpumask_test_cpu(sched_cpu, &rqd->tickled);
     if ( tickled )
     {
-        __cpumask_clear_cpu(cpu, &rqd->tickled);
+        __cpumask_clear_cpu(sched_cpu, &rqd->tickled);
         cpumask_andnot(cpumask_scratch, &rqd->idle, &rqd->tickled);
-        smt_idle_mask_set(cpu, cpumask_scratch, &rqd->smt_idle);
+        smt_idle_mask_set(sched_cpu, cpumask_scratch, &rqd->smt_idle);
     }
 
     if ( unlikely(tb_init_done) )
@@ -3485,10 +3486,10 @@  csched2_schedule(
             unsigned tasklet:8, idle:8, smt_idle:8, tickled:8;
         } d;
         d.cpu = cpu;
-        d.rq_id = c2r(cpu);
+        d.rq_id = c2r(sched_cpu);
         d.tasklet = tasklet_work_scheduled;
         d.idle = is_idle_unit(currunit);
-        d.smt_idle = cpumask_test_cpu(cpu, &rqd->smt_idle);
+        d.smt_idle = cpumask_test_cpu(sched_cpu, &rqd->smt_idle);
         d.tickled = tickled;
         __trace_var(TRC_CSCHED2_SCHEDULE, 1,
                     sizeof(d),
@@ -3528,10 +3529,10 @@  csched2_schedule(
     {
         __clear_bit(__CSFLAG_unit_yield, &scurr->flags);
         trace_var(TRC_CSCHED2_SCHED_TASKLET, 1, 0, NULL);
-        snext = csched2_unit(sched_idle_unit(cpu));
+        snext = csched2_unit(sched_idle_unit(sched_cpu));
     }
     else
-        snext = runq_candidate(rqd, scurr, cpu, now, &skipped_units);
+        snext = runq_candidate(rqd, scurr, sched_cpu, now, &skipped_units);
 
     /* If switching from a non-idle runnable unit, put it
      * back on the runqueue. */
@@ -3556,10 +3557,10 @@  csched2_schedule(
         }
 
         /* Clear the idle mask if necessary */
-        if ( cpumask_test_cpu(cpu, &rqd->idle) )
+        if ( cpumask_test_cpu(sched_cpu, &rqd->idle) )
         {
-            __cpumask_clear_cpu(cpu, &rqd->idle);
-            smt_idle_mask_clear(cpu, &rqd->smt_idle);
+            __cpumask_clear_cpu(sched_cpu, &rqd->idle);
+            smt_idle_mask_clear(sched_cpu, &rqd->smt_idle);
         }
 
         /*
@@ -3578,18 +3579,18 @@  csched2_schedule(
          */
         if ( skipped_units == 0 && snext->credit <= CSCHED2_CREDIT_RESET )
         {
-            reset_credit(ops, cpu, now, snext);
-            balance_load(ops, cpu, now);
+            reset_credit(ops, sched_cpu, now, snext);
+            balance_load(ops, sched_cpu, now);
         }
 
         snext->start_time = now;
         snext->tickled_cpu = -1;
 
         /* Safe because lock for old processor is held */
-        if ( sched_unit_cpu(snext->unit) != cpu )
+        if ( sched_unit_cpu(snext->unit) != sched_cpu )
         {
             snext->credit += CSCHED2_MIGRATE_COMPENSATION;
-            sched_set_res(snext->unit, get_sched_res(cpu));
+            sched_set_res(snext->unit, get_sched_res(sched_cpu));
             SCHED_STAT_CRANK(migrated);
             ret.migrated = 1;
         }
@@ -3602,17 +3603,17 @@  csched2_schedule(
          */
         if ( tasklet_work_scheduled )
         {
-            if ( cpumask_test_cpu(cpu, &rqd->idle) )
+            if ( cpumask_test_cpu(sched_cpu, &rqd->idle) )
             {
-                __cpumask_clear_cpu(cpu, &rqd->idle);
-                smt_idle_mask_clear(cpu, &rqd->smt_idle);
+                __cpumask_clear_cpu(sched_cpu, &rqd->idle);
+                smt_idle_mask_clear(sched_cpu, &rqd->smt_idle);
             }
         }
-        else if ( !cpumask_test_cpu(cpu, &rqd->idle) )
+        else if ( !cpumask_test_cpu(sched_cpu, &rqd->idle) )
         {
-            __cpumask_set_cpu(cpu, &rqd->idle);
+            __cpumask_set_cpu(sched_cpu, &rqd->idle);
             cpumask_andnot(cpumask_scratch, &rqd->idle, &rqd->tickled);
-            smt_idle_mask_set(cpu, cpumask_scratch, &rqd->smt_idle);
+            smt_idle_mask_set(sched_cpu, cpumask_scratch, &rqd->smt_idle);
         }
         /* Make sure avgload gets updated periodically even
          * if there's no activity */
@@ -3622,7 +3623,7 @@  csched2_schedule(
     /*
      * Return task to run next...
      */
-    ret.time = csched2_runtime(ops, cpu, snext, now);
+    ret.time = csched2_runtime(ops, sched_cpu, snext, now);
     ret.task = snext->unit;
 
     CSCHED2_UNIT_CHECK(ret.task);
diff --git a/xen/common/sched_null.c b/xen/common/sched_null.c
index a630951110..56ef078c5a 100644
--- a/xen/common/sched_null.c
+++ b/xen/common/sched_null.c
@@ -785,6 +785,7 @@  static struct task_slice null_schedule(const struct scheduler *ops,
 {
     unsigned int bs;
     const unsigned int cpu = smp_processor_id();
+    const unsigned int sched_cpu = sched_get_resource_cpu(cpu);
     struct null_private *prv = null_priv(ops);
     struct null_unit *wvc;
     struct task_slice ret;
@@ -800,14 +801,14 @@  static struct task_slice null_schedule(const struct scheduler *ops,
         } d;
         d.cpu = cpu;
         d.tasklet = tasklet_work_scheduled;
-        if ( per_cpu(npc, cpu).unit == NULL )
+        if ( per_cpu(npc, sched_cpu).unit == NULL )
         {
             d.unit = d.dom = -1;
         }
         else
         {
-            d.unit = per_cpu(npc, cpu).unit->unit_id;
-            d.dom = per_cpu(npc, cpu).unit->domain->domain_id;
+            d.unit = per_cpu(npc, sched_cpu).unit->unit_id;
+            d.dom = per_cpu(npc, sched_cpu).unit->domain->domain_id;
         }
         __trace_var(TRC_SNULL_SCHEDULE, 1, sizeof(d), &d);
     }
@@ -815,10 +816,10 @@  static struct task_slice null_schedule(const struct scheduler *ops,
     if ( tasklet_work_scheduled )
     {
         trace_var(TRC_SNULL_TASKLET, 1, 0, NULL);
-        ret.task = sched_idle_unit(cpu);
+        ret.task = sched_idle_unit(sched_cpu);
     }
     else
-        ret.task = per_cpu(npc, cpu).unit;
+        ret.task = per_cpu(npc, sched_cpu).unit;
     ret.migrated = 0;
     ret.time = -1;
 
@@ -849,9 +850,9 @@  static struct task_slice null_schedule(const struct scheduler *ops,
                      !has_soft_affinity(wvc->unit) )
                     continue;
 
-                if ( unit_check_affinity(wvc->unit, cpu, bs) )
+                if ( unit_check_affinity(wvc->unit, sched_cpu, bs) )
                 {
-                    unit_assign(prv, wvc->unit, cpu);
+                    unit_assign(prv, wvc->unit, sched_cpu);
                     list_del_init(&wvc->waitq_elem);
                     ret.task = wvc->unit;
                     goto unlock;
@@ -866,7 +867,7 @@  static struct task_slice null_schedule(const struct scheduler *ops,
     }
 
     if ( unlikely(ret.task == NULL || !unit_runnable(ret.task)) )
-        ret.task = sched_idle_unit(cpu);
+        ret.task = sched_idle_unit(sched_cpu);
 
     NULL_UNIT_CHECK(ret.task);
     return ret;
diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
index 95262aff95..7b9d25f138 100644
--- a/xen/common/sched_rt.c
+++ b/xen/common/sched_rt.c
@@ -1057,7 +1057,8 @@  runq_pick(const struct scheduler *ops, const cpumask_t *mask)
 static struct task_slice
 rt_schedule(const struct scheduler *ops, s_time_t now, bool_t tasklet_work_scheduled)
 {
-    const int cpu = smp_processor_id();
+    const unsigned int cpu = smp_processor_id();
+    const unsigned int sched_cpu = sched_get_resource_cpu(cpu);
     struct rt_private *prv = rt_priv(ops);
     struct rt_unit *const scurr = rt_unit(current->sched_unit);
     struct rt_unit *snext = NULL;
@@ -1071,7 +1072,7 @@  rt_schedule(const struct scheduler *ops, s_time_t now, bool_t tasklet_work_sched
         } d;
         d.cpu = cpu;
         d.tasklet = tasklet_work_scheduled;
-        d.tickled = cpumask_test_cpu(cpu, &prv->tickled);
+        d.tickled = cpumask_test_cpu(sched_cpu, &prv->tickled);
         d.idle = is_idle_unit(currunit);
         trace_var(TRC_RTDS_SCHEDULE, 1,
                   sizeof(d),
@@ -1079,7 +1080,7 @@  rt_schedule(const struct scheduler *ops, s_time_t now, bool_t tasklet_work_sched
     }
 
     /* clear ticked bit now that we've been scheduled */
-    cpumask_clear_cpu(cpu, &prv->tickled);
+    cpumask_clear_cpu(sched_cpu, &prv->tickled);
 
     /* burn_budget would return for IDLE UNIT */
     burn_budget(ops, scurr, now);
@@ -1087,13 +1088,13 @@  rt_schedule(const struct scheduler *ops, s_time_t now, bool_t tasklet_work_sched
     if ( tasklet_work_scheduled )
     {
         trace_var(TRC_RTDS_SCHED_TASKLET, 1, 0,  NULL);
-        snext = rt_unit(sched_idle_unit(cpu));
+        snext = rt_unit(sched_idle_unit(sched_cpu));
     }
     else
     {
-        snext = runq_pick(ops, cpumask_of(cpu));
+        snext = runq_pick(ops, cpumask_of(sched_cpu));
         if ( snext == NULL )
-            snext = rt_unit(sched_idle_unit(cpu));
+            snext = rt_unit(sched_idle_unit(sched_cpu));
 
         /* if scurr has higher priority and budget, still pick scurr */
         if ( !is_idle_unit(currunit) &&
@@ -1118,9 +1119,9 @@  rt_schedule(const struct scheduler *ops, s_time_t now, bool_t tasklet_work_sched
             q_remove(snext);
             __set_bit(__RTDS_scheduled, &snext->flags);
         }
-        if ( sched_unit_cpu(snext->unit) != cpu )
+        if ( sched_unit_cpu(snext->unit) != sched_cpu )
         {
-            sched_set_res(snext->unit, get_sched_res(cpu));
+            sched_set_res(snext->unit, get_sched_res(sched_cpu));
             ret.migrated = 1;
         }
         ret.time = snext->cur_budget; /* invoke the scheduler next time */
diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index 6281e884cf..d8402878d4 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -112,7 +112,7 @@  static struct task_slice sched_idle_schedule(
     const unsigned int cpu = smp_processor_id();
     struct task_slice ret = { .time = -1 };
 
-    ret.task = sched_idle_unit(cpu);
+    ret.task = sched_idle_unit(sched_get_resource_cpu(cpu));
     return ret;
 }
 
diff --git a/xen/include/xen/sched-if.h b/xen/include/xen/sched-if.h
index 1440055250..1a3981e78a 100644
--- a/xen/include/xen/sched-if.h
+++ b/xen/include/xen/sched-if.h
@@ -115,6 +115,11 @@  static inline struct sched_unit *sched_idle_unit(unsigned int cpu)
     return idle_vcpu[cpu]->sched_unit;
 }
 
+static inline unsigned int sched_get_resource_cpu(unsigned int cpu)
+{
+    return get_sched_res(cpu)->processor;
+}
+
 /*
  * Scratch space, for avoiding having too many cpumask_t on the stack.
  * Within each scheduler, when using the scratch mask of one pCPU: