Re: [PATCH v2 02/22] sched: Add interfaces for IPC classes

[Ionela Voinescu <ionela.voinescu@arm.com>]

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