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

Leonardo Bras
2024-01-26T22:06:13Z

On Fri, Jan 26, 2024 at 11:49:02AM -1000, Tejun Heo wrote:
> 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.
>

Hello,

Thanks for pointing that out, I will improve it in the v2.

> > @@ -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.

Thanks, I will point out this being the last cpu used to handle the 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.

I am not really against it, but for me it's kind of weird to have that many
calls to add_timer_on() if we can avoid it.

I would rather go with:

###
if (unlikely(cpu != WORK_CPU_UNBOUND)) {
add_timer_on(timer, cpu);
return;
}

if (!housekeeping_enabled(HK_TYPE_TIMER)) {
add_timer(timer);
return;
}

cpu = smp_processor_id();
if (!housekeeping_test_cpu(cpu, HK_TYPE_TIMER))
cpu = housekeeping_any_cpu(HK_TYPE_TIMER);

add_timer_on(timer, cpu);
###

What do you think?

Thanks,
Leo

>
> --
> tejun
>