Re: [PATCH v2 3/3] timers: Exclude isolated cpus from timer migation

From: Gabriele Monaco
Date: Tue Apr 15 2025 - 11:49:50 EST

Next message: Lee Jones: "Re: [PATCH 3/8] mfd: max14577: Fix wakeup source leaks on device unbind"
Previous message: K Prateek Nayak: "Re: [RFC PATCH v2 0/7] Defer throttle when task exits to user"
In reply to: Waiman Long: "Re: [PATCH v2 3/3] timers: Exclude isolated cpus from timer migation"
Next in thread: Waiman Long: "Re: [PATCH v2 3/3] timers: Exclude isolated cpus from timer migation"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tue, 2025-04-15 at 11:30 -0400, Waiman Long wrote:
>
> On 4/15/25 6:25 AM, Gabriele Monaco wrote:
> > The timer migration mechanism allows active CPUs to pull timers
> > from
> > idle ones to improve the overall idle time. This is however
> > undesired
> > when CPU intensive workloads run on isolated cores, as the
> > algorithm
> > would move the timers from housekeeping to isolated cores,
> > negatively
> > affecting the isolation.
> >
> > This effect was noticed on a 128 cores machine running oslat on the
> > isolated cores (1-31,33-63,65-95,97-127). The tool monopolises
> > CPUs,
> > and the CPU with lowest count in a timer migration hierarchy (here
> > 1
> > and 65) appears as always active and continuously pulls global
> > timers,
> > from the housekeeping CPUs. This ends up moving driver work (e.g.
> > delayed work) to isolated CPUs and causes latency spikes:
> >
> > before the change:
> >
> > # oslat -c 1-31,33-63,65-95,97-127 -D 62s
> > ...
> >    Maximum:     1203 10 3 4 ... 5 (us)
> >
> > after the change:
> >
> > # oslat -c 1-31,33-63,65-95,97-127 -D 62s
> > ...
> >    Maximum:      10 4 3 4 3 ... 5 (us)
> >
> > Exclude isolated cores from the timer migration algorithm, extend
> > the
> > concept of unavailable cores, currently used for offline ones, to
> > isolated ones:
> > * A core is unavailable if isolated or offline;
> > * A core is available if isolated and offline;
> I think you mean "A core is available if NOT isolated and NOT
> offline".
> Right?

Yes, of course.. My bad. Thanks for spotting.

> >
> > A core is considered unavailable as idle if:
> > * is in the isolcpus list
> > * is in the nohz_full list
> > * is in an isolated cpuset
> >
> > Due to how the timer migration algorithm works, any CPU part of the
> > hierarchy can have their global timers pulled by remote CPUs and
> > have to
> > pull remote timers, only skipping pulling remote timers would break
> > the
> > logic.
> > For this reason, we prevents isolated CPUs from pulling remote
> > global
> > timers, but also the other way around: any global timer started on
> > an
> > isolated CPU will run there. This does not break the concept of
> > isolation (global timers don't come from outside the CPU) and, if
> > considered inappropriate, can usually be mitigated with other
> > isolation
> > techniques (e.g. IRQ pinning).
> >
> > Signed-off-by: Gabriele Monaco <gmonaco@xxxxxxxxxx>
> > ---
> > include/linux/timer.h         | 6 ++++++
> > kernel/cgroup/cpuset.c        | 14 ++++++++------
> > kernel/time/tick-internal.h   | 1 +
> > kernel/time/timer.c           | 10 ++++++++++
> > kernel/time/timer_migration.c | 24 +++++++++++++++++++++---
> > 5 files changed, 46 insertions(+), 9 deletions(-)
> >
> > diff --git a/include/linux/timer.h b/include/linux/timer.h
> > index 10596d7c3a346..4722e075d9843 100644
> > --- a/include/linux/timer.h
> > +++ b/include/linux/timer.h
> > @@ -190,4 +190,10 @@ int timers_dead_cpu(unsigned int cpu);
> > #define timers_dead_cpu NULL
> > #endif
> >
> > +#if defined(CONFIG_SMP) && defined(CONFIG_NO_HZ_COMMON)
> > +extern void tmigr_isolated_exclude_cpumask(cpumask_var_t
> > exclude_cpumask);
> > +#else
> > +static inline void tmigr_isolated_exclude_cpumask(cpumask_var_t
> > exclude_cpumask) { }
> > +#endif
> > +
> > #endif
> > diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
> > index 306b604300914..866b4b8188118 100644
> > --- a/kernel/cgroup/cpuset.c
> > +++ b/kernel/cgroup/cpuset.c
> > @@ -1323,7 +1323,7 @@ static bool partition_xcpus_del(int old_prs,
> > struct cpuset *parent,
> >    return isolcpus_updated;
> > }
> >
> > -static void update_unbound_workqueue_cpumask(bool
> > isolcpus_updated)
> > +static void update_exclusion_cpumasks(bool isolcpus_updated)
> > {
> >    int ret;
> >
> > @@ -1334,6 +1334,8 @@ static void
> > update_unbound_workqueue_cpumask(bool isolcpus_updated)
> >
> >    ret = workqueue_unbound_exclude_cpumask(isolated_cpus);
> >    WARN_ON_ONCE(ret < 0);
> > +
> > + tmigr_isolated_exclude_cpumask(isolated_cpus);
> > }
> >
> > /**
> > @@ -1454,7 +1456,7 @@ static int remote_partition_enable(struct
> > cpuset *cs, int new_prs,
> >    list_add(&cs->remote_sibling, &remote_children);
> >    cpumask_copy(cs->effective_xcpus, tmp->new_cpus);
> >    spin_unlock_irq(&callback_lock);
> > - update_unbound_workqueue_cpumask(isolcpus_updated);
> > + update_exclusion_cpumasks(isolcpus_updated);
> >    cpuset_force_rebuild();
> >    cs->prs_err = 0;
> >
> > @@ -1495,7 +1497,7 @@ static void remote_partition_disable(struct
> > cpuset *cs, struct tmpmasks *tmp)
> >    compute_effective_exclusive_cpumask(cs, NULL, NULL);
> >    reset_partition_data(cs);
> >    spin_unlock_irq(&callback_lock);
> > - update_unbound_workqueue_cpumask(isolcpus_updated);
> > + update_exclusion_cpumasks(isolcpus_updated);
> >    cpuset_force_rebuild();
> >
> >    /*
> > @@ -1563,7 +1565,7 @@ static void remote_cpus_update(struct cpuset
> > *cs, struct cpumask *xcpus,
> >    if (xcpus)
> >    cpumask_copy(cs->exclusive_cpus, xcpus);
> >    spin_unlock_irq(&callback_lock);
> > - update_unbound_workqueue_cpumask(isolcpus_updated);
> > + update_exclusion_cpumasks(isolcpus_updated);
> >    if (adding || deleting)
> >    cpuset_force_rebuild();
> >
> > @@ -1906,7 +1908,7 @@ static int
> > update_parent_effective_cpumask(struct cpuset *cs, int cmd,
> >    WARN_ON_ONCE(parent->nr_subparts < 0);
> >    }
> >    spin_unlock_irq(&callback_lock);
> > - update_unbound_workqueue_cpumask(isolcpus_updated);
> > + update_exclusion_cpumasks(isolcpus_updated);
> >
> >    if ((old_prs != new_prs) && (cmd == partcmd_update))
> >    update_partition_exclusive_flag(cs, new_prs);
> > @@ -2931,7 +2933,7 @@ static int update_prstate(struct cpuset *cs,
> > int new_prs)
> >    else if (isolcpus_updated)
> >    isolated_cpus_update(old_prs, new_prs, cs-
> > >effective_xcpus);
> >    spin_unlock_irq(&callback_lock);
> > - update_unbound_workqueue_cpumask(isolcpus_updated);
> > + update_exclusion_cpumasks(isolcpus_updated);
> >
> >    /* Force update if switching back to member & update
> > effective_xcpus */
> >    update_cpumasks_hier(cs, &tmpmask, !new_prs);
> > diff --git a/kernel/time/tick-internal.h b/kernel/time/tick-
> > internal.h
> > index faac36de35b9e..75580f7c69c64 100644
> > --- a/kernel/time/tick-internal.h
> > +++ b/kernel/time/tick-internal.h
> > @@ -167,6 +167,7 @@ extern void
> > fetch_next_timer_interrupt_remote(unsigned long basej, u64 basem,
> > extern void timer_lock_remote_bases(unsigned int cpu);
> > extern void timer_unlock_remote_bases(unsigned int cpu);
> > extern bool timer_base_is_idle(void);
> > +extern bool timer_base_remote_is_idle(unsigned int cpu);
> > extern void timer_expire_remote(unsigned int cpu);
> > # endif
> > #else /* CONFIG_NO_HZ_COMMON */
> > diff --git a/kernel/time/timer.c b/kernel/time/timer.c
> > index 4d915c0a263c3..f04960091eba9 100644
> > --- a/kernel/time/timer.c
> > +++ b/kernel/time/timer.c
> > @@ -2162,6 +2162,16 @@ bool timer_base_is_idle(void)
> >    return __this_cpu_read(timer_bases[BASE_LOCAL].is_idle);
> > }
> >
> > +/**
> > + * timer_base_remote_is_idle() - Return whether timer base is set
> > idle for cpu
> > + *
> > + * Returns value of local timer base is_idle value for remote cpu.
> > + */
> > +bool timer_base_remote_is_idle(unsigned int cpu)
> > +{
> > + return per_cpu(timer_bases[BASE_LOCAL].is_idle, cpu);
> > +}
> > +
> > static void __run_timer_base(struct timer_base *base);
> >
> > /**
> > diff --git a/kernel/time/timer_migration.c
> > b/kernel/time/timer_migration.c
> > index 1fae38fbac8c2..6fe6ca798e98d 100644
> > --- a/kernel/time/timer_migration.c
> > +++ b/kernel/time/timer_migration.c
> > @@ -10,6 +10,7 @@
> > #include <linux/spinlock.h>
> > #include <linux/timerqueue.h>
> > #include <trace/events/ipi.h>
> > +#include <linux/sched/isolation.h>
> >
> > #include "timer_migration.h"
> > #include "tick-internal.h"
> > @@ -1445,7 +1446,7 @@ static long tmigr_trigger_active(void
> > *unused)
> >
> > static int tmigr_cpu_unavailable(unsigned int cpu)
> > {
> > - struct tmigr_cpu *tmc = this_cpu_ptr(&tmigr_cpu);
> > + struct tmigr_cpu *tmc = per_cpu_ptr(&tmigr_cpu, cpu);
> >    int migrator;
> >    u64 firstexp;
> >
> > @@ -1472,15 +1473,18 @@ static int tmigr_cpu_unavailable(unsigned
> > int cpu)
> >
> > static int tmigr_cpu_available(unsigned int cpu)
> > {
> > - struct tmigr_cpu *tmc = this_cpu_ptr(&tmigr_cpu);
> > + struct tmigr_cpu *tmc = per_cpu_ptr(&tmigr_cpu, cpu);
> >
> >    /* Check whether CPU data was successfully initialized */
> >    if (WARN_ON_ONCE(!tmc->tmgroup))
> >    return -EINVAL;
> >
> > + /* Isolated CPUs don't participate in timer migration */
> > + if (cpu_is_isolated(cpu))
> > + return 0;
>
> There are two main sets of isolated CPUs used by cpu_is_isolated() -
> boot-time isolated CPUs via "isolcpus" and "nohz_full" boot command
> time
> options and runtime isolated CPUs via cpuset isolated partitions. The
> check for runtime isolated CPUs is redundant here as those CPUs won't
> be
> passed to tmigr_cpu_available().

Since tmigr_cpu_available is shared between isolated and offline CPUs,
I added this check also to make sure bringing an isolated CPU back
online won't make it available for tmigr.

> So this call is effectively removing
> the boot time isolated CPUs away from the available cpumask
> especially
> during the boot up process. Maybe you can add some comment about this
> behavioral change.
>

Do you mean I should make clear that the check in tmigr_cpu_available
is especially meaningful at boot time (i.e. when CPUs are first brought
online)?

Yeah, I probably should, good point. I had that kind of comment in v1
while allocating the mask and removed it while changing a few things.

I'm going to make that comment more verbose to clarify when exactly
it's needed.

>
> >    raw_spin_lock_irq(&tmc->lock);
> >    trace_tmigr_cpu_available(tmc);
> > - tmc->idle = timer_base_is_idle();
> > + tmc->idle = timer_base_remote_is_idle(cpu);
> >    if (!tmc->idle)
> >    __tmigr_cpu_activate(tmc);
> >    tmc->available = true;
> > @@ -1489,6 +1493,20 @@ static int tmigr_cpu_available(unsigned int
> > cpu)
> >    return 0;
> > }
> >
> > +void tmigr_isolated_exclude_cpumask(cpumask_var_t exclude_cpumask)
> > +{
> > + int cpu;
> > +
> > + lockdep_assert_cpus_held();
> > +
> > + for_each_cpu_and(cpu, exclude_cpumask,
> > tmigr_available_cpumask)
> > + tmigr_cpu_unavailable(cpu);
> > +
> > + for_each_cpu_andnot(cpu, cpu_online_mask, exclude_cpumask)
> > + if (!cpumask_test_cpu(cpu,
> > tmigr_available_cpumask))
> > + tmigr_cpu_available(cpu);
> > +}
> > +
> > static void tmigr_init_group(struct tmigr_group *group, unsigned
> > int lvl,
> >         int node)
> > {
>
> So far, I haven't seen any major issue with this patch series.
>

Thanks for the review!

Cheers,
Gabriele

Next message: Lee Jones: "Re: [PATCH 3/8] mfd: max14577: Fix wakeup source leaks on device unbind"
Previous message: K Prateek Nayak: "Re: [RFC PATCH v2 0/7] Defer throttle when task exits to user"
In reply to: Waiman Long: "Re: [PATCH v2 3/3] timers: Exclude isolated cpus from timer migation"
Next in thread: Waiman Long: "Re: [PATCH v2 3/3] timers: Exclude isolated cpus from timer migation"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]