[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZbQsr1pNSoiMbDrO@LeoBras>
Date: Fri, 26 Jan 2024 19:05:35 -0300
From: Leonardo Bras <leobras@...hat.com>
To: Tejun Heo <tj@...nel.org>
Cc: Leonardo Bras <leobras@...hat.com>,
Lai Jiangshan <jiangshanlai@...il.com>,
Marcelo Tosatti <mtosatti@...hat.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 1/1] wq: Avoid using isolated cpus' timers on unbounded queue_delayed_work
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
>
Powered by blists - more mailing lists