Hi Tglx,
On 2020-05-06 06:28, Thomas Gleixner wrote:
Prasad Sodagudi <psoda...@codeaurora.org> writes:
To make all cpu unbound deferrable timers are scalable, introduce a
common
timer base which is only for cpu unbound deferrable timers to make
those
are indeed cpu unbound so that can be scheduled by any of non idle
cpus.
This common timer fixes scalability issue of delayed work and all
other cpu
unbound deferrable timer using implementations.
Scalability? That's really the wrong term here. A global timer base is
the opposite and you really want to explain why this is not creating a
scalability problem on large systems.
#ifdef CONFIG_SMP
+struct timer_base timer_base_deferrable;
unsigned int sysctl_timer_migration = 1;
DEFINE_STATIC_KEY_FALSE(timers_migration_enabled);
@@ -841,8 +842,14 @@ static inline struct timer_base
*get_timer_cpu_base(u32 tflags, u32 cpu)
* If the timer is deferrable and NO_HZ_COMMON is set then we need
* to use the deferrable base.
*/
- if (IS_ENABLED(CONFIG_NO_HZ_COMMON) && (tflags & TIMER_DEFERRABLE))
- base = per_cpu_ptr(&timer_bases[BASE_DEF], cpu);
+ if (IS_ENABLED(CONFIG_NO_HZ_COMMON) && (tflags & TIMER_DEFERRABLE))
{
+#ifdef CONFIG_SMP
+ base = &timer_base_deferrable;
+#endif
There are definitely smarter ways of solving this than sprinkling
#ifdef's around the code.
I am able to understand all other comments and I will address all those
comments in the next patch set.
It is not clear to me how to avoid #ifdef's in this case. Could you
please share an example here?
+ if (tflags & TIMER_PINNED)
+ base = per_cpu_ptr(&timer_bases[BASE_DEF], cpu);
+ }
+
return base;
}
@@ -1785,8 +1798,14 @@ static __latent_entropy void
run_timer_softirq(struct softirq_action *h)
struct timer_base *base = this_cpu_ptr(&timer_bases[BASE_STD]);
__run_timers(base);
- if (IS_ENABLED(CONFIG_NO_HZ_COMMON))
+ if (IS_ENABLED(CONFIG_NO_HZ_COMMON)) {
__run_timers(this_cpu_ptr(&timer_bases[BASE_DEF]));
+#ifdef CONFIG_SMP
+ if (tick_do_timer_cpu == TICK_DO_TIMER_NONE ||
+ tick_do_timer_cpu == smp_processor_id())
+ __run_timers(&timer_base_deferrable);
+#endif
Again, this can be solved in readable ways. Just slapping #ifdefs all
over the place is sloppy and lazy.
Sorry. I will try to address this. It is not clear to me how to avoid
#ifdefs in this case too.
Please provide me more information.
Aside of that accessing the tick internals here open coded is just a
layering violation.
I will fix this and avoid using referring to tick internals here.
+ }
}
/*
@@ -2025,6 +2044,16 @@ static void __init init_timer_cpu(int cpu)
}
}
+#if defined(CONFIG_NO_HZ_COMMON) && defined(CONFIG_SMP)
+static void __init init_timer_deferrable_global(void)
+{
+ timer_base_deferrable.cpu = nr_cpu_ids;
This was obviously never tested with CONFIG_DEBUG_PER_CPU_MAPS=y as
this
will simply result in out of bounds accesses.
Sure. I will test this CONFIG_DEBUG_PER_CPU_MAPS=y enabled before
pushing the next patch set.
static void __init init_timer_cpus(void)
{
int cpu;
@@ -2036,6 +2065,9 @@ static void __init init_timer_cpus(void)
void __init init_timers(void)
{
init_timer_cpus();
+#if defined(CONFIG_NO_HZ_COMMON) && defined(CONFIG_SMP)
+ init_timer_deferrable_global();
+#endif
Stub functions exist to avoid this unreadable #ifdef garbage.
Got it. I will fix this in the next patch set.
Thanks,
tglx
