Re: [PATCH v3 4/8] timers/migration: Use a single struct for hierarchy walk data

[Frederic Weisbecker <frederic@kernel.org>]

Le Mon, Jul 01, 2024 at 12:18:40PM +0200, Anna-Maria Behnsen a écrit :
> Two different structs are defined for propagating data from one to another
> level when walking the hierarchy. Several struct members exist in both
> structs which makes generalization harder.
> 
> Merge those two structs into a single one and use it directly in
> walk_groups() and the corresponding function pointers instead of
> introducing pointer casting all over the place.
> 
> Signed-off-by: Anna-Maria Behnsen <anna-maria@linutronix.de>
> ---
>  kernel/time/timer_migration.c | 126 ++++++++++++++++++------------------------
>  1 file changed, 55 insertions(+), 71 deletions(-)
> 
> diff --git a/kernel/time/timer_migration.c b/kernel/time/timer_migration.c
> index 0ae7f2084d27..b4391abfb4a9 100644
> --- a/kernel/time/timer_migration.c
> +++ b/kernel/time/timer_migration.c
> @@ -475,69 +475,31 @@ static bool tmigr_check_lonely(struct tmigr_group *group)
>  	return bitmap_weight(&active, BIT_CNT) <= 1;
>  }
>  
> -typedef bool (*up_f)(struct tmigr_group *, struct tmigr_group *, void *);
> -
> -static void __walk_groups(up_f up, void *data,
> -			  struct tmigr_cpu *tmc)
> -{
> -	struct tmigr_group *child = NULL, *group = tmc->tmgroup;
> -
> -	do {
> -		WARN_ON_ONCE(group->level >= tmigr_hierarchy_levels);
> -
> -		if (up(group, child, data))
> -			break;
> -
> -		child = group;
> -		group = group->parent;
> -	} while (group);
> -}
> -
> -static void walk_groups(up_f up, void *data, struct tmigr_cpu *tmc)
> -{
> -	lockdep_assert_held(&tmc->lock);
> -
> -	__walk_groups(up, data, tmc);
> -}
> -
>  /**
>   * struct tmigr_walk - data required for walking the hierarchy
>   * @nextexp:		Next CPU event expiry information which is handed into
>   *			the timer migration code by the timer code
>   *			(get_next_timer_interrupt())
> - * @firstexp:		Contains the first event expiry information when last
> - *			active CPU of hierarchy is on the way to idle to make
> - *			sure CPU will be back in time. It is updated in top
> - *			level group only. Be aware, there could occur a new top
> - *			level of the hierarchy between the 'top level call' in
> - *			tmigr_update_events() and the check for the parent group
> - *			in walk_groups(). Then @firstexp might contain a value
> - *			!= KTIME_MAX even if it was not the final top
> - *			level. This is not a problem, as the worst outcome is a
> - *			CPU which might wake up a little early.
> + * @firstexp:		Contains the first event expiry information when
> + *			hierarchy is completely idle.  When CPU itself was the
> + *			last going idle, information makes sure, that CPU will
> + *			be back in time. When using this value in the remote
> + *			expiry case, firstexp is stored in the per CPU tmigr_cpu
> + *			struct of CPU which expires remote timers. It is updated
> + *			in top level group only. Be aware, there could occur a
> + *			new top level of the hierarchy between the 'top level
> + *			call' in tmigr_update_events() and the check for the
> + *			parent group in walk_groups(). Then @firstexp might
> + *			contain a value != KTIME_MAX even if it was not the
> + *			final top level. This is not a problem, as the worst
> + *			outcome is a CPU which might wake up a little early.
>   * @evt:		Pointer to tmigr_event which needs to be queued (of idle
>   *			child group)
>   * @childmask:		childmask of child group
>   * @remote:		Is set, when the new timer path is executed in
>   *			tmigr_handle_remote_cpu()
> - */
> -struct tmigr_walk {
> -	u64			nextexp;
> -	u64			firstexp;
> -	struct tmigr_event	*evt;
> -	u8			childmask;
> -	bool			remote;
> -};
> -
> -/**
> - * struct tmigr_remote_data - data required for remote expiry hierarchy walk
>   * @basej:		timer base in jiffies
>   * @now:		timer base monotonic
> - * @firstexp:		returns expiry of the first timer in the idle timer
> - *			migration hierarchy to make sure the timer is handled in
> - *			time; it is stored in the per CPU tmigr_cpu struct of
> - *			CPU which expires remote timers
> - * @childmask:		childmask of child group
>   * @check:		is set if there is the need to handle remote timers;
>   *			required in tmigr_requires_handle_remote() only
>   * @tmc_active:		this flag indicates, whether the CPU which triggers
> @@ -546,15 +508,43 @@ struct tmigr_walk {
>   *			idle, only the first event of the top level has to be
>   *			considered.
>   */
> -struct tmigr_remote_data {
> -	unsigned long	basej;
> -	u64		now;
> -	u64		firstexp;
> -	u8		childmask;
> -	bool		check;
> -	bool		tmc_active;
> +struct tmigr_walk {
> +	u64			nextexp;
> +	u64			firstexp;
> +	struct tmigr_event	*evt;
> +	u8			childmask;
> +	bool			remote;
> +	unsigned long		basej;
> +	u64			now;
> +	bool			check;
> +	bool			tmc_active;


Reviewed-by: Frederic Weisbecker <frederic@kernel.org>

Ideas for a subsequent patch:

Is tmc_active actually useful? It must always be true in
tmigr_requires_handle_remote_up() since that function is only called
on active CPUs.

In fact the following condition is dead code:

	/*
	 * When there is a parent group and the CPU which triggered the
	 * hierarchy walk is not active, proceed the walk to reach the top level
	 * group before reading the next_expiry value.
	 */
	if (group->parent && !data->tmc_active)
		goto out;

Thanks.