[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZoPn65jVGSRn2mTI@localhost.localdomain>
Date: Tue, 2 Jul 2024 13:43:39 +0200
From: Frederic Weisbecker <frederic@...nel.org>
To: Anna-Maria Behnsen <anna-maria@...utronix.de>
Cc: Thomas Gleixner <tglx@...utronix.de>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 4/8] timers/migration: Use a single struct for
hierarchy walk data
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@...utronix.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@...nel.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.
Powered by blists - more mailing lists