| Message ID | 20221128132100.30253-3-ricardo.neri-calderon@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | sched: Introduce IPC classes for load balance | expand |
Hi, On Monday 28 Nov 2022 at 05:20:40 (-0800), Ricardo Neri wrote: [..] > +#ifndef arch_has_ipc_classes > +/** > + * arch_has_ipc_classes() - Check whether hardware supports IPC classes of tasks > + * > + * Returns: true of IPC classes of tasks are supported. > + */ > +static __always_inline > +bool arch_has_ipc_classes(void) > +{ > + return false; > +} > +#endif > + > +#ifndef arch_update_ipcc > +/** > + * arch_update_ipcc() - Update the IPC class of the current task > + * @curr: The current task > + * > + * Request that the IPC classification of @curr is updated. > + * > + * Returns: none > + */ > +static __always_inline > +void arch_update_ipcc(struct task_struct *curr) > +{ > +} > +#endif > + > +#ifndef arch_get_ipcc_score > +/** > + * arch_get_ipcc_score() - Get the IPC score of a class of task > + * @ipcc: The IPC class > + * @cpu: A CPU number > + * > + * Returns the performance score of an IPC class when running on @cpu. > + * Error when either @class or @cpu are invalid. > + */ > +static __always_inline > +int arch_get_ipcc_score(unsigned short ipcc, int cpu) > +{ > + return 1; > +} > +#endif The interface looks mostly alright but this arch_get_ipcc_score() leaves unclear what are the characteristics of the returned value. Does it have a meaning as an absolute value or is it a value on an abstract scale? If it should be interpreted as instructions per cycle, if I wanted to have a proper comparison between the ability of two CPUs to handle this class of tasks then I would need to take into consideration the maximum frequency of each CPU. If it's a performance value on an abstract scale (more likely), similar cu capacity, then it might be good to better define this abstract scale. That would help with the default implementation where possibly the best choice for a return value would be the maximum value on the scale, suggesting equal/maximum performance for different CPUs handling the class of tasks. I suppose you avoided returning 0 for the default implementation as you intend that to mean the inability of the CPU to handle that class of tasks? It would be good to document this. > +#else /* CONFIG_IPC_CLASSES */ > + > +#define arch_get_ipcc_score(ipcc, cpu) (-EINVAL) > +#define arch_update_ipcc(curr) > + > +static inline bool sched_ipcc_enabled(void) { return false; } > + > +#endif /* CONFIG_IPC_CLASSES */ > + > #ifndef arch_scale_freq_capacity > /** > * arch_scale_freq_capacity - get the frequency scale factor of a given CPU. > diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c > index 8154ef590b9f..eb1654b64df7 100644 > --- a/kernel/sched/topology.c > +++ b/kernel/sched/topology.c > @@ -669,6 +669,9 @@ DEFINE_PER_CPU(struct sched_domain __rcu *, sd_numa); > DEFINE_PER_CPU(struct sched_domain __rcu *, sd_asym_packing); > DEFINE_PER_CPU(struct sched_domain __rcu *, sd_asym_cpucapacity); > DEFINE_STATIC_KEY_FALSE(sched_asym_cpucapacity); > +#ifdef CONFIG_IPC_CLASSES > +DEFINE_STATIC_KEY_FALSE(sched_ipcc); > +#endif > > static void update_top_cache_domain(int cpu) > { > @@ -2388,6 +2391,11 @@ build_sched_domains(const struct cpumask *cpu_map, struct sched_domain_attr *att > if (has_asym) > static_branch_inc_cpuslocked(&sched_asym_cpucapacity); > > +#ifdef CONFIG_IPC_CLASSES > + if (arch_has_ipc_classes()) > + static_branch_enable_cpuslocked(&sched_ipcc); > +#endif Wouldn't this be better placed directly in sched_init_smp()? It's not gated by and it does not need any sched domains information. Hope it helps, Ionela. > + > if (rq && sched_debug_verbose) { > pr_info("root domain span: %*pbl (max cpu_capacity = %lu)\n", > cpumask_pr_args(cpu_map), rq->rd->max_cpu_capacity); > -- > 2.25.1 > >
On Thu, Dec 08, 2022 at 08:48:46AM +0000, Ionela Voinescu wrote: > Hi, > > On Monday 28 Nov 2022 at 05:20:40 (-0800), Ricardo Neri wrote: > [..] > > +#ifndef arch_has_ipc_classes > > +/** > > + * arch_has_ipc_classes() - Check whether hardware supports IPC classes of tasks > > + * > > + * Returns: true of IPC classes of tasks are supported. > > + */ > > +static __always_inline > > +bool arch_has_ipc_classes(void) > > +{ > > + return false; > > +} > > +#endif > > + > > +#ifndef arch_update_ipcc > > +/** > > + * arch_update_ipcc() - Update the IPC class of the current task > > + * @curr: The current task > > + * > > + * Request that the IPC classification of @curr is updated. > > + * > > + * Returns: none > > + */ > > +static __always_inline > > +void arch_update_ipcc(struct task_struct *curr) > > +{ > > +} > > +#endif > > + > > +#ifndef arch_get_ipcc_score > > +/** > > + * arch_get_ipcc_score() - Get the IPC score of a class of task > > + * @ipcc: The IPC class > > + * @cpu: A CPU number > > + * > > + * Returns the performance score of an IPC class when running on @cpu. > > + * Error when either @class or @cpu are invalid. > > + */ > > +static __always_inline > > +int arch_get_ipcc_score(unsigned short ipcc, int cpu) > > +{ > > + return 1; > > +} > > +#endif Thank you very much for your feedback Ionela! > > The interface looks mostly alright but this arch_get_ipcc_score() leaves > unclear what are the characteristics of the returned value. Fair point. I mean for the return value to be defined by architectures; but yes, architectures need to know how to implement this function. > > Does it have a meaning as an absolute value or is it a value on an > abstract scale? If it should be interpreted as instructions per cycle, > if I wanted to have a proper comparison between the ability of two CPUs > to handle this class of tasks then I would need to take into consideration > the maximum frequency of each CPU. Do you mean when calling arch_get_ipcc_score()? If yes, then I agree, IPC class may not be the only factor, but the criteria to use the return value is up to the caller. In asym_packing it is assumed that higher-priority CPUs are preferred. When balancing load, IPC class scores are used to select between otherwise identical runqueues. This should also be the case for migrate_misfit: we know already that the tasks being considered do not fit on their current CPU. We would need to think what to do with other type of balancing, if at all. That said, arch_get_ipcc_score() should only return a metric of the instructions-per-*cycle*, independent of frequency, no? > If it's a performance value on an > abstract scale (more likely), similar cu capacity, then it might be good > to better define this abstract scale. That would help with the default > implementation where possibly the best choice for a return value would > be the maximum value on the scale, suggesting equal/maximum performance > for different CPUs handling the class of tasks. I guess something like: #define SCHED_IPCC_DEFAULT_SCALE 1024 ? I think I am fine with this value being the default. I also think that it is up to architectures to whether scale all IPC class scores from the best-performing class on the best-performing CPU. Doing so would introduce overhead, especially if hardware updates the IPC class scores multiple times during runtime. > > I suppose you avoided returning 0 for the default implementation as you > intend that to mean the inability of the CPU to handle that class of > tasks? It would be good to document this. I meant this to be minimum possible IPC class score for any CPU: any CPU should be able handle any IPC class. If not implemented, all CPUs handle all IPC classes equally. > > > +#else /* CONFIG_IPC_CLASSES */ > > + > > +#define arch_get_ipcc_score(ipcc, cpu) (-EINVAL) > > +#define arch_update_ipcc(curr) > > + > > +static inline bool sched_ipcc_enabled(void) { return false; } > > + > > +#endif /* CONFIG_IPC_CLASSES */ > > + > > #ifndef arch_scale_freq_capacity > > /** > > * arch_scale_freq_capacity - get the frequency scale factor of a given CPU. > > diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c > > index 8154ef590b9f..eb1654b64df7 100644 > > --- a/kernel/sched/topology.c > > +++ b/kernel/sched/topology.c > > @@ -669,6 +669,9 @@ DEFINE_PER_CPU(struct sched_domain __rcu *, sd_numa); > > DEFINE_PER_CPU(struct sched_domain __rcu *, sd_asym_packing); > > DEFINE_PER_CPU(struct sched_domain __rcu *, sd_asym_cpucapacity); > > DEFINE_STATIC_KEY_FALSE(sched_asym_cpucapacity); > > +#ifdef CONFIG_IPC_CLASSES > > +DEFINE_STATIC_KEY_FALSE(sched_ipcc); > > +#endif > > > > static void update_top_cache_domain(int cpu) > > { > > @@ -2388,6 +2391,11 @@ build_sched_domains(const struct cpumask *cpu_map, struct sched_domain_attr *att > > if (has_asym) > > static_branch_inc_cpuslocked(&sched_asym_cpucapacity); > > > > +#ifdef CONFIG_IPC_CLASSES > > + if (arch_has_ipc_classes()) > > + static_branch_enable_cpuslocked(&sched_ipcc); > > +#endif > > Wouldn't this be better placed directly in sched_init_smp()? > It's not gated by and it does not need any sched domains information. Very true. I will take your suggestion. > > Hope it helps, It does help significantly. Thanks again for your feedback. Thanks and BR, Ricardo
Hi Richardo, I have some generic comment for the design of those interfaces. On 11/28/22 13:20, Ricardo Neri wrote: > Add the interfaces that architectures shall implement to convey the data > to support IPC classes. > > arch_update_ipcc() updates the IPC classification of the current task as > given by hardware. > > arch_get_ipcc_score() provides a performance score for a given IPC class > when placed on a specific CPU. Higher scores indicate higher performance. > > The number of classes and the score of each class of task are determined > by hardware. > > Cc: Ben Segall <bsegall@google.com> > Cc: Daniel Bristot de Oliveira <bristot@redhat.com> > Cc: Dietmar Eggemann <dietmar.eggemann@arm.com> > Cc: Joel Fernandes (Google) <joel@joelfernandes.org> > Cc: Len Brown <len.brown@intel.com> > Cc: Mel Gorman <mgorman@suse.de> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: Tim C. Chen <tim.c.chen@intel.com> > Cc: Valentin Schneider <vschneid@redhat.com> > Cc: x86@kernel.org > Cc: linux-pm@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com> > --- > Changes since v1: > * Shortened the names of the IPCC interfaces (PeterZ): > sched_task_classes_enabled >> sched_ipcc_enabled > arch_has_task_classes >> arch_has_ipc_classes > arch_update_task_class >> arch_update_ipcc > arch_get_task_class_score >> arch_get_ipcc_score > * Removed smt_siblings_idle argument from arch_update_ipcc(). (PeterZ) > --- > kernel/sched/sched.h | 60 +++++++++++++++++++++++++++++++++++++++++ > kernel/sched/topology.c | 8 ++++++ > 2 files changed, 68 insertions(+) > > diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h > index b1d338a740e5..75e22baa2622 100644 > --- a/kernel/sched/sched.h > +++ b/kernel/sched/sched.h > @@ -2531,6 +2531,66 @@ void arch_scale_freq_tick(void) > } > #endif > > +#ifdef CONFIG_IPC_CLASSES > +DECLARE_STATIC_KEY_FALSE(sched_ipcc); > + > +static inline bool sched_ipcc_enabled(void) > +{ > + return static_branch_unlikely(&sched_ipcc); > +} > + > +#ifndef arch_has_ipc_classes > +/** > + * arch_has_ipc_classes() - Check whether hardware supports IPC classes of tasks > + * > + * Returns: true of IPC classes of tasks are supported. > + */ > +static __always_inline > +bool arch_has_ipc_classes(void) > +{ > + return false; > +} > +#endif > + > +#ifndef arch_update_ipcc > +/** > + * arch_update_ipcc() - Update the IPC class of the current task > + * @curr: The current task > + * > + * Request that the IPC classification of @curr is updated. > + * > + * Returns: none > + */ > +static __always_inline > +void arch_update_ipcc(struct task_struct *curr) > +{ > +} > +#endif > + > +#ifndef arch_get_ipcc_score > +/** > + * arch_get_ipcc_score() - Get the IPC score of a class of task > + * @ipcc: The IPC class > + * @cpu: A CPU number > + * > + * Returns the performance score of an IPC class when running on @cpu. > + * Error when either @class or @cpu are invalid. > + */ > +static __always_inline > +int arch_get_ipcc_score(unsigned short ipcc, int cpu) > +{ > + return 1; > +} > +#endif Those interfaces are quite simple, probably work really OK with your HW/FW. If any other architecture is going to re-use them in future, we might face some issue. Let me explain why. These kernel functions are start to be used very early in boot. Your HW/FW is probably instantly ready to work from the very beginning during boot. What is some other HW needs some preparation code, like setup communication channel to FW or enable needed clocks/events/etc. What I would like to see is a similar mechanism to the one in schedutil. Schedutil governor has to wait till cpufreq initialize the cpu freq driver and policy objects (which sometimes takes ~2-3 sec). After that cpufreq fwk starts the governor which populates this hook [1]. It's based on RCU mechanism with function pointer that can be then called from the task scheduler when everything is ready to work. If we (Arm) is going to use your proposed interfaces, we might need different mechanisms because the platform likely would be ready after our SCMI FW channels and cpufreq are setup. Would it be possible to address such need now or I would have to change that interface code later? Regards, Lukasz [1] https://elixir.bootlin.com/linux/latest/source/kernel/sched/cpufreq.c#L29
Hi, On Tuesday 13 Dec 2022 at 16:31:28 (-0800), Ricardo Neri wrote: > On Thu, Dec 08, 2022 at 08:48:46AM +0000, Ionela Voinescu wrote: > > Hi, > > > > On Monday 28 Nov 2022 at 05:20:40 (-0800), Ricardo Neri wrote: > > [..] > > > +#ifndef arch_has_ipc_classes > > > +/** > > > + * arch_has_ipc_classes() - Check whether hardware supports IPC classes of tasks > > > + * > > > + * Returns: true of IPC classes of tasks are supported. > > > + */ > > > +static __always_inline > > > +bool arch_has_ipc_classes(void) > > > +{ > > > + return false; > > > +} > > > +#endif > > > + > > > +#ifndef arch_update_ipcc > > > +/** > > > + * arch_update_ipcc() - Update the IPC class of the current task > > > + * @curr: The current task > > > + * > > > + * Request that the IPC classification of @curr is updated. > > > + * > > > + * Returns: none > > > + */ > > > +static __always_inline > > > +void arch_update_ipcc(struct task_struct *curr) > > > +{ > > > +} > > > +#endif > > > + > > > +#ifndef arch_get_ipcc_score > > > +/** > > > + * arch_get_ipcc_score() - Get the IPC score of a class of task > > > + * @ipcc: The IPC class > > > + * @cpu: A CPU number > > > + * > > > + * Returns the performance score of an IPC class when running on @cpu. > > > + * Error when either @class or @cpu are invalid. > > > + */ > > > +static __always_inline > > > +int arch_get_ipcc_score(unsigned short ipcc, int cpu) > > > +{ > > > + return 1; > > > +} > > > +#endif > > Thank you very much for your feedback Ionela! > > > > > The interface looks mostly alright but this arch_get_ipcc_score() leaves > > unclear what are the characteristics of the returned value. > > Fair point. I mean for the return value to be defined by architectures; > but yes, architectures need to know how to implement this function. > > > > > Does it have a meaning as an absolute value or is it a value on an > > abstract scale? If it should be interpreted as instructions per cycle, > > if I wanted to have a proper comparison between the ability of two CPUs > > to handle this class of tasks then I would need to take into consideration > > the maximum frequency of each CPU. > > Do you mean when calling arch_get_ipcc_score()? If yes, then I agree, IPC > class may not be the only factor, but the criteria to use the return value > is up to the caller. > Yes, but if different architectures give different meanings to this score (scale, relative difference between two values, etc) while the policies are common (uses of arch_get_ipcc_score() in common scheduler paths) then the outcome can be vastly different. If the "criteria to use the returned value is up to the caller", then the caller of arch_get_ipcc_score() should always be architecture specific code, which currently is not (see 09/22). > In asym_packing it is assumed that higher-priority CPUs are preferred. > When balancing load, IPC class scores are used to select between otherwise > identical runqueues. This should also be the case for migrate_misfit: we > know already that the tasks being considered do not fit on their current > CPU. > > We would need to think what to do with other type of balancing, if at all. > > That said, arch_get_ipcc_score() should only return a metric of the > instructions-per-*cycle*, independent of frequency, no? > Yes, performance on an abstract scale is preferred here. We would not want to have to scale the score by frequency :). It was just an example showing that the description of arch_get_ipcc_score() should be clarified. Another possible clarification: is it expected that the scores scale linearly with performance (does double the score mean double the performance?). > > If it's a performance value on an > > abstract scale (more likely), similar cu capacity, then it might be good > > to better define this abstract scale. That would help with the default > > implementation where possibly the best choice for a return value would > > be the maximum value on the scale, suggesting equal/maximum performance > > for different CPUs handling the class of tasks. > > I guess something like: > > #define SCHED_IPCC_DEFAULT_SCALE 1024 > > ? > > I think I am fine with this value being the default. I also think that it > is up to architectures to whether scale all IPC class scores from the > best-performing class on the best-performing CPU. Doing so would introduce > overhead, especially if hardware updates the IPC class scores multiple > times during runtime. > Yes, it's a very good point. Initially I thought that one would need to rescale the values anyway for them to make sense relative to each other, but I now realise that would not be needed. Therefore, you are right, to avoid this extra work it's best to leave the range of possible score values up to the implementer and not force something like [0 - 1024]. But again, this raises the point that if one architecture decides to return its scores on a scale [0 - 1024] and possibly use these scores to scale utilization/alter capacity for example, this cannot be generic policy as not all architectures are guaranteed to use this scale for its scores. So leaving the score unrestricted makes it more difficult to have generic policies across architectures that use them. > > > > I suppose you avoided returning 0 for the default implementation as you > > intend that to mean the inability of the CPU to handle that class of > > tasks? It would be good to document this. > > I meant this to be minimum possible IPC class score for any CPU: any > CPU should be able handle any IPC class. If not implemented, all CPUs > handle all IPC classes equally. > Ah, I see. In this case you might as well return 0 in the default implementation of arch_get_ipcc_score(). I know it does not matter much what gets returned there, but returning a meaningless "1" is strange to me :). Thanks, Ionela.
On Wed, Dec 14, 2022 at 07:36:44AM +0000, Lukasz Luba wrote: > Hi Richardo, > > I have some generic comment for the design of those interfaces. > > On 11/28/22 13:20, Ricardo Neri wrote: > > Add the interfaces that architectures shall implement to convey the data > > to support IPC classes. > > > > arch_update_ipcc() updates the IPC classification of the current task as > > given by hardware. > > > > arch_get_ipcc_score() provides a performance score for a given IPC class > > when placed on a specific CPU. Higher scores indicate higher performance. > > > > The number of classes and the score of each class of task are determined > > by hardware. > > > > Cc: Ben Segall <bsegall@google.com> > > Cc: Daniel Bristot de Oliveira <bristot@redhat.com> > > Cc: Dietmar Eggemann <dietmar.eggemann@arm.com> > > Cc: Joel Fernandes (Google) <joel@joelfernandes.org> > > Cc: Len Brown <len.brown@intel.com> > > Cc: Mel Gorman <mgorman@suse.de> > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > > Cc: Steven Rostedt <rostedt@goodmis.org> > > Cc: Tim C. Chen <tim.c.chen@intel.com> > > Cc: Valentin Schneider <vschneid@redhat.com> > > Cc: x86@kernel.org > > Cc: linux-pm@vger.kernel.org > > Cc: linux-kernel@vger.kernel.org > > Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com> > > --- > > Changes since v1: > > * Shortened the names of the IPCC interfaces (PeterZ): > > sched_task_classes_enabled >> sched_ipcc_enabled > > arch_has_task_classes >> arch_has_ipc_classes > > arch_update_task_class >> arch_update_ipcc > > arch_get_task_class_score >> arch_get_ipcc_score > > * Removed smt_siblings_idle argument from arch_update_ipcc(). (PeterZ) > > --- > > kernel/sched/sched.h | 60 +++++++++++++++++++++++++++++++++++++++++ > > kernel/sched/topology.c | 8 ++++++ > > 2 files changed, 68 insertions(+) > > > > diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h > > index b1d338a740e5..75e22baa2622 100644 > > --- a/kernel/sched/sched.h > > +++ b/kernel/sched/sched.h > > @@ -2531,6 +2531,66 @@ void arch_scale_freq_tick(void) > > } > > #endif > > +#ifdef CONFIG_IPC_CLASSES > > +DECLARE_STATIC_KEY_FALSE(sched_ipcc); > > + > > +static inline bool sched_ipcc_enabled(void) > > +{ > > + return static_branch_unlikely(&sched_ipcc); > > +} > > + > > +#ifndef arch_has_ipc_classes > > +/** > > + * arch_has_ipc_classes() - Check whether hardware supports IPC classes of tasks > > + * > > + * Returns: true of IPC classes of tasks are supported. > > + */ > > +static __always_inline > > +bool arch_has_ipc_classes(void) > > +{ > > + return false; > > +} > > +#endif > > + > > +#ifndef arch_update_ipcc > > +/** > > + * arch_update_ipcc() - Update the IPC class of the current task > > + * @curr: The current task > > + * > > + * Request that the IPC classification of @curr is updated. > > + * > > + * Returns: none > > + */ > > +static __always_inline > > +void arch_update_ipcc(struct task_struct *curr) > > +{ > > +} > > +#endif > > + > > +#ifndef arch_get_ipcc_score > > +/** > > + * arch_get_ipcc_score() - Get the IPC score of a class of task > > + * @ipcc: The IPC class > > + * @cpu: A CPU number > > + * > > + * Returns the performance score of an IPC class when running on @cpu. > > + * Error when either @class or @cpu are invalid. > > + */ > > +static __always_inline > > +int arch_get_ipcc_score(unsigned short ipcc, int cpu) > > +{ > > + return 1; > > +} > > +#endif > > Those interfaces are quite simple, probably work really OK with > your HW/FW. If any other architecture is going to re-use them > in future, we might face some issue. Let me explain why. > > These kernel functions are start to be used very early in boot. > Your HW/FW is probably instantly ready to work from the very > beginning during boot. What is some other HW needs some > preparation code, like setup communication channel to FW or enable > needed clocks/events/etc. > > What I would like to see is a similar mechanism to the one in schedutil. > Schedutil governor has to wait till cpufreq initialize the cpu freq > driver and policy objects (which sometimes takes ~2-3 sec). After that > cpufreq fwk starts the governor which populates this hook [1]. > It's based on RCU mechanism with function pointer that can be then > called from the task scheduler when everything is ready to work. > > If we (Arm) is going to use your proposed interfaces, we might need > different mechanisms because the platform likely would be ready after > our SCMI FW channels and cpufreq are setup. > > Would it be possible to address such need now or I would have to > change that interface code later? Thank you very much for your feeback, Lukas! I took a look a cpufreq implementation you refer. I can certainly try to accommodate your requirements. Before jumping into it, I have a few questions. I see that cpufreq_update_util() only does something when the per-CPU pointers cpufreq_update_util_data become non-NULL. I use static key for the same purpose. Is this not usable for you? Indeed, arch_has_ipc_classes() implies that has to return true very early after boot if called, as per Ionela's suggestion from sched_init_smp(). I can convert this interface to an arch_enable_ipc_classes() that drivers or preparation code can call when ready. Would this be acceptable? Do think that a hook per CPU would be needed? If unsure, perhaps this can be left for future work. Thanks and BR, Ricardo > > [1] > https://elixir.bootlin.com/linux/latest/source/kernel/sched/cpufreq.c#L29 >
On Wed, Dec 14, 2022 at 11:15:56PM +0000, Ionela Voinescu wrote: > Hi, > > On Tuesday 13 Dec 2022 at 16:31:28 (-0800), Ricardo Neri wrote: > > On Thu, Dec 08, 2022 at 08:48:46AM +0000, Ionela Voinescu wrote: > > > Hi, > > > > > > On Monday 28 Nov 2022 at 05:20:40 (-0800), Ricardo Neri wrote: > > > [..] > > > > +#ifndef arch_has_ipc_classes > > > > +/** > > > > + * arch_has_ipc_classes() - Check whether hardware supports IPC classes of tasks > > > > + * > > > > + * Returns: true of IPC classes of tasks are supported. > > > > + */ > > > > +static __always_inline > > > > +bool arch_has_ipc_classes(void) > > > > +{ > > > > + return false; > > > > +} > > > > +#endif > > > > + > > > > +#ifndef arch_update_ipcc > > > > +/** > > > > + * arch_update_ipcc() - Update the IPC class of the current task > > > > + * @curr: The current task > > > > + * > > > > + * Request that the IPC classification of @curr is updated. > > > > + * > > > > + * Returns: none > > > > + */ > > > > +static __always_inline > > > > +void arch_update_ipcc(struct task_struct *curr) > > > > +{ > > > > +} > > > > +#endif > > > > + > > > > +#ifndef arch_get_ipcc_score > > > > +/** > > > > + * arch_get_ipcc_score() - Get the IPC score of a class of task > > > > + * @ipcc: The IPC class > > > > + * @cpu: A CPU number > > > > + * > > > > + * Returns the performance score of an IPC class when running on @cpu. > > > > + * Error when either @class or @cpu are invalid. > > > > + */ > > > > +static __always_inline > > > > +int arch_get_ipcc_score(unsigned short ipcc, int cpu) > > > > +{ > > > > + return 1; > > > > +} > > > > +#endif > > > > Thank you very much for your feedback Ionela! > > > > > > > > The interface looks mostly alright but this arch_get_ipcc_score() leaves > > > unclear what are the characteristics of the returned value. > > > > Fair point. I mean for the return value to be defined by architectures; > > but yes, architectures need to know how to implement this function. > > > > > > > > Does it have a meaning as an absolute value or is it a value on an > > > abstract scale? If it should be interpreted as instructions per cycle, > > > if I wanted to have a proper comparison between the ability of two CPUs > > > to handle this class of tasks then I would need to take into consideration > > > the maximum frequency of each CPU. > > > > Do you mean when calling arch_get_ipcc_score()? If yes, then I agree, IPC > > class may not be the only factor, but the criteria to use the return value > > is up to the caller. > > > > Yes, but if different architectures give different meanings to this score > (scale, relative difference between two values, etc) while the policies > are common (uses of arch_get_ipcc_score() in common scheduler paths) > then the outcome can be vastly different. One more reason to leave to the caller the handling of the returned value. > > If the "criteria to use the returned value is up to the caller", then > the caller of arch_get_ipcc_score() should always be architecture > specific code, which currently is not (see 09/22). Agreed. I now get your point. I'll change my patch accordingly. > > > In asym_packing it is assumed that higher-priority CPUs are preferred. > > When balancing load, IPC class scores are used to select between otherwise > > identical runqueues. This should also be the case for migrate_misfit: we > > know already that the tasks being considered do not fit on their current > > CPU. > > > > We would need to think what to do with other type of balancing, if at all. > > > > That said, arch_get_ipcc_score() should only return a metric of the > > instructions-per-*cycle*, independent of frequency, no? > > > > Yes, performance on an abstract scale is preferred here. We would not > want to have to scale the score by frequency :). It was just an example > showing that the description of arch_get_ipcc_score() should be clarified. > Another possible clarification: is it expected that the scores scale > linearly with performance (does double the score mean double the > performance?). Indeed this seems sensible. > > > > If it's a performance value on an > > > abstract scale (more likely), similar cu capacity, then it might be good > > > to better define this abstract scale. That would help with the default > > > implementation where possibly the best choice for a return value would > > > be the maximum value on the scale, suggesting equal/maximum performance > > > for different CPUs handling the class of tasks. > > > > I guess something like: > > > > #define SCHED_IPCC_DEFAULT_SCALE 1024 > > > > ? > > > > I think I am fine with this value being the default. I also think that it > > is up to architectures to whether scale all IPC class scores from the > > best-performing class on the best-performing CPU. Doing so would introduce > > overhead, especially if hardware updates the IPC class scores multiple > > times during runtime. > > > > Yes, it's a very good point. Initially I thought that one would need to > rescale the values anyway for them to make sense relative to each other, > but I now realise that would not be needed. > > Therefore, you are right, to avoid this extra work it's best to leave > the range of possible score values up to the implementer and not force > something like [0 - 1024]. > > But again, this raises the point that if one architecture decides to > return its scores on a scale [0 - 1024] and possibly use these scores to > scale utilization/alter capacity for example, this cannot be generic > policy as not all architectures are guaranteed to use this scale for its > scores. Very good point. > > So leaving the score unrestricted makes it more difficult to have > generic policies across architectures that use them. > In asym_packing we select the CPU of higher priority, regardless of how big the priority delta is. IPC classes extend the same mechanism. (We do have a throughput calculation, but it does not require IPC class to be scaled). So yes, IPC classes need to be scaled when combined with another metric. Another addition to the documentation of the interface? :) > > > > > > > I suppose you avoided returning 0 for the default implementation as you > > > intend that to mean the inability of the CPU to handle that class of > > > tasks? It would be good to document this. > > > > I meant this to be minimum possible IPC class score for any CPU: any > > CPU should be able handle any IPC class. If not implemented, all CPUs > > handle all IPC classes equally. > > > > Ah, I see. In this case you might as well return 0 in the default > implementation of arch_get_ipcc_score(). I know it does not matter much > what gets returned there, but returning a meaningless "1" is strange to > me :). Yes, the value does not really matter to my use case, as long as it the same for all all CPUs. I can use 1024 as other scheduler metrics. Thanks and BR, Ricardo
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h index b1d338a740e5..75e22baa2622 100644 --- a/kernel/sched/sched.h +++ b/kernel/sched/sched.h @@ -2531,6 +2531,66 @@ void arch_scale_freq_tick(void) } #endif +#ifdef CONFIG_IPC_CLASSES +DECLARE_STATIC_KEY_FALSE(sched_ipcc); + +static inline bool sched_ipcc_enabled(void) +{ + return static_branch_unlikely(&sched_ipcc); +} + +#ifndef arch_has_ipc_classes +/** + * arch_has_ipc_classes() - Check whether hardware supports IPC classes of tasks + * + * Returns: true of IPC classes of tasks are supported. + */ +static __always_inline +bool arch_has_ipc_classes(void) +{ + return false; +} +#endif + +#ifndef arch_update_ipcc +/** + * arch_update_ipcc() - Update the IPC class of the current task + * @curr: The current task + * + * Request that the IPC classification of @curr is updated. + * + * Returns: none + */ +static __always_inline +void arch_update_ipcc(struct task_struct *curr) +{ +} +#endif + +#ifndef arch_get_ipcc_score +/** + * arch_get_ipcc_score() - Get the IPC score of a class of task + * @ipcc: The IPC class + * @cpu: A CPU number + * + * Returns the performance score of an IPC class when running on @cpu. + * Error when either @class or @cpu are invalid. + */ +static __always_inline +int arch_get_ipcc_score(unsigned short ipcc, int cpu) +{ + return 1; +} +#endif +#else /* CONFIG_IPC_CLASSES */ + +#define arch_get_ipcc_score(ipcc, cpu) (-EINVAL) +#define arch_update_ipcc(curr) + +static inline bool sched_ipcc_enabled(void) { return false; } + +#endif /* CONFIG_IPC_CLASSES */ + #ifndef arch_scale_freq_capacity /** * arch_scale_freq_capacity - get the frequency scale factor of a given CPU. diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c index 8154ef590b9f..eb1654b64df7 100644 --- a/kernel/sched/topology.c +++ b/kernel/sched/topology.c @@ -669,6 +669,9 @@ DEFINE_PER_CPU(struct sched_domain __rcu *, sd_numa); DEFINE_PER_CPU(struct sched_domain __rcu *, sd_asym_packing); DEFINE_PER_CPU(struct sched_domain __rcu *, sd_asym_cpucapacity); DEFINE_STATIC_KEY_FALSE(sched_asym_cpucapacity); +#ifdef CONFIG_IPC_CLASSES +DEFINE_STATIC_KEY_FALSE(sched_ipcc); +#endif static void update_top_cache_domain(int cpu) { @@ -2388,6 +2391,11 @@ build_sched_domains(const struct cpumask *cpu_map, struct sched_domain_attr *att if (has_asym) static_branch_inc_cpuslocked(&sched_asym_cpucapacity); +#ifdef CONFIG_IPC_CLASSES + if (arch_has_ipc_classes()) + static_branch_enable_cpuslocked(&sched_ipcc); +#endif + if (rq && sched_debug_verbose) { pr_info("root domain span: %*pbl (max cpu_capacity = %lu)\n", cpumask_pr_args(cpu_map), rq->rd->max_cpu_capacity);
Add the interfaces that architectures shall implement to convey the data to support IPC classes. arch_update_ipcc() updates the IPC classification of the current task as given by hardware. arch_get_ipcc_score() provides a performance score for a given IPC class when placed on a specific CPU. Higher scores indicate higher performance. The number of classes and the score of each class of task are determined by hardware. Cc: Ben Segall <bsegall@google.com> Cc: Daniel Bristot de Oliveira <bristot@redhat.com> Cc: Dietmar Eggemann <dietmar.eggemann@arm.com> Cc: Joel Fernandes (Google) <joel@joelfernandes.org> Cc: Len Brown <len.brown@intel.com> Cc: Mel Gorman <mgorman@suse.de> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> Cc: Steven Rostedt <rostedt@goodmis.org> Cc: Tim C. Chen <tim.c.chen@intel.com> Cc: Valentin Schneider <vschneid@redhat.com> Cc: x86@kernel.org Cc: linux-pm@vger.kernel.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com> --- Changes since v1: * Shortened the names of the IPCC interfaces (PeterZ): sched_task_classes_enabled >> sched_ipcc_enabled arch_has_task_classes >> arch_has_ipc_classes arch_update_task_class >> arch_update_ipcc arch_get_task_class_score >> arch_get_ipcc_score * Removed smt_siblings_idle argument from arch_update_ipcc(). (PeterZ) --- kernel/sched/sched.h | 60 +++++++++++++++++++++++++++++++++++++++++ kernel/sched/topology.c | 8 ++++++ 2 files changed, 68 insertions(+)