Re: [PATCH v1 1/1] wq: Avoid using isolated cpus' timers on unbounded queue_delayed_work

Tejun Heo
2024-01-26T21:49:05Z

Hello,

On Thu, Jan 25, 2024 at 10:03:20PM -0300, Leonardo Bras wrote:
...
> AS an optimization, if the current cpu is not isolated, use it's timer
^ ^
As its

> instead of looking for another candidate.

The sentence reads weird tho. It's always the same timer. We're deciding
which CPU to queue the timer on.

> @@ -1958,10 +1958,24 @@ static void __queue_delayed_work(int cpu, struct workqueue_struct *wq,
> dwork->cpu = cpu;
> timer->expires = jiffies + delay;
>
> - if (unlikely(cpu != WORK_CPU_UNBOUND))
> - add_timer_on(timer, cpu);
> - else
> - add_timer(timer);
> + if (likely(cpu == WORK_CPU_UNBOUND)) {
> + if (!housekeeping_enabled(HK_TYPE_TIMER)) {
> + /* Reuse the same timer */

This comment is confusing because it's always the same timer.

> + add_timer(timer);
> + return;
> + }
> +
> + /*
> + * If the work is cpu-unbound, and cpu isolation is in place,
> + * only use timers from housekeeping cpus.
> + * If the current cpu is a housekeeping cpu, use it instead.
> + */
> + cpu = smp_processor_id();
> + if (!housekeeping_test_cpu(cpu, HK_TYPE_TIMER))
> + cpu = housekeeping_any_cpu(HK_TYPE_TIMER);
> + }
> +
> + add_timer_on(timer, cpu);
> }

I find the control flow a bit difficult to follow. It's not the end of the
world to have two add_timer_on() calls. Would something like the following
be easier to read?

if (housekeeping_enabled(HK_TYPE_TIMER)) {
cpu = smp_processor_id();
if (!housekeeping_test_cpu(cpu, HK_TYPE_TIMER))
cpu = housekeeping_any_cpu(HK_TYPE_TIMER);
add_timer_on(timer, cpu);
} else {
if (likely(cpu == WORK_CPU_UNBOUND))
add_timer(timer, cpu);
else
add_timer_on(timer, cpu);
}

Thanks.

--
tejun