Re: [PATCH v4 18/24] sched/task_struct: Add helpers for IPC classification

[Ionela Voinescu]

Hi,

On Monday 12 Jun 2023 at 21:24:16 (-0700), Ricardo Neri wrote:
> The raw classification that hardware provides for a task may not
> be directly usable by the scheduler: the classification may change too
> frequently or architecture-specific implementations may need to consider
> additional factors. For instance, some processors with Intel Thread
> Director need to consider the state of the SMT siblings of a core.
> 
> Provide per-task helper variables that architectures can use to
> postprocess the classification that hardware provides.
> 
> Cc: Ben Segall <bsegall@xxxxxxxxxx>
> Cc: Daniel Bristot de Oliveira <bristot@xxxxxxxxxx>
> Cc: Dietmar Eggemann <dietmar.eggemann@xxxxxxx>
> Cc: Ionela Voinescu <ionela.voinescu@xxxxxxx>
> Cc: Joel Fernandes (Google) <joel@xxxxxxxxxxxxxxxxx>
> Cc: Len Brown <len.brown@xxxxxxxxx>
> Cc: Lukasz Luba <lukasz.luba@xxxxxxx>
> Cc: Mel Gorman <mgorman@xxxxxxx>
> Cc: Perry Yuan <Perry.Yuan@xxxxxxx>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> Cc: Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>
> Cc: Steven Rostedt <rostedt@xxxxxxxxxxx>
> Cc: Tim C. Chen <tim.c.chen@xxxxxxxxx>
> Cc: Valentin Schneider <vschneid@xxxxxxxxxx>
> Cc: Zhao Liu <zhao1.liu@xxxxxxxxxxxxxxx>
> Cc: x86@xxxxxxxxxx
> Cc: linux-pm@xxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> Signed-off-by: Ricardo Neri <ricardo.neri-calderon@xxxxxxxxxxxxxxx>
> ---
> Changes since v3:
>  * None
> 
> Changes since v2:
>  * None
> 
> Changes since v1:
>  * Used bit-fields to fit all the IPC class data in 4 bytes. (PeterZ)
>  * Shortened names of the helpers.
>  * Renamed helpers with the ipcc_ prefix.
>  * Reworded commit message for clarity
> ---
>  include/linux/sched.h | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 0e1c38ad09c2..719147460ca8 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1541,7 +1541,17 @@ struct task_struct {
>  	 * A hardware-defined classification of task that reflects but is
>  	 * not identical to the number of instructions per cycle.
>  	 */
> -	unsigned short			ipcc;
> +	unsigned int			ipcc : 9;
> +	/*
> +	 * A candidate classification that arch-specific implementations
> +	 * qualify for correctness.
> +	 */
> +	unsigned int			ipcc_tmp : 9;
> +	/*
> +	 * Counter to filter out transient candidate classifications
> +	 * of a task.
> +	 */
> +	unsigned int			ipcc_cntr : 14;
>  #endif
>  

Why does this need to be split in task_struct? Isn't this architecture
specific? IMO the scheduler should never make use of class information
itself. It only receives the value though the call of an arch function
and passes it as an argument to an arch function to get a performance
score. So the way one interprets the class value (splits it in relevant
and helper bits) should be defined and considered in the architecture
specific code.

Thanks,
Ionela.

>  	/*
> -- 
> 2.25.1
>