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