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

[Leonardo Bras Soares Passos]

Hi,

On Fri, Oct 4, 2024 at 9:37 AM Frederic Weisbecker <frederic@xxxxxxxxxx> wrote:
>
> Hi,
>
> Le Mon, Jan 29, 2024 at 10:00:46PM -0300, Leonardo Bras a écrit :
> > When __queue_delayed_work() is called, it chooses a cpu for handling the
> > timer interrupt. As of today, it will pick either the cpu passed as
> > parameter or the last cpu used for this.
> >
> > This is not good if a system does use CPU isolation, because it can take
> > away some valuable cpu time to:
> > 1 - deal with the timer interrupt,
> > 2 - schedule-out the desired task,
> > 3 - queue work on a random workqueue, and
> > 4 - schedule the desired task back to the cpu.
> >
> > So to fix this, during __queue_delayed_work(), if cpu isolation is in
> > place, pick a random non-isolated cpu to handle the timer interrupt.
> >
> > As an optimization, if the current cpu is not isolated, use it instead
> > of looking for another candidate.
> >
> > Signed-off-by: Leonardo Bras <leobras@xxxxxxxxxx>
> > ---
> > Changes since v1:
> > - Make sure the CPU is isolated for any value of "cpu"
> >
> > Changes since RFC:
> > - Do not use the same cpu from the timer for queueing the work.
> > - If the current cpu is not isolated, use it's timer instead of
> >   looking for another candidate.
> >
> >  kernel/workqueue.c | 14 +++++++++++---
> >  1 file changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> > index 76e60faed8923..8dd7c01b326a4 100644
> > --- a/kernel/workqueue.c
> > +++ b/kernel/workqueue.c
> > @@ -1958,10 +1958,18 @@ static void __queue_delayed_work(int cpu, struct workqueue_struct *wq,
> >       dwork->cpu = cpu;
> >       timer->expires = jiffies + delay;
> >
> > -     if (unlikely(cpu != WORK_CPU_UNBOUND))
> > +     if (housekeeping_enabled(HK_TYPE_TIMER)) {
> > +             /* If the current cpu is a housekeeping cpu, use it. */
> > +             cpu = smp_processor_id();
> > +             if (!housekeeping_test_cpu(cpu, HK_TYPE_TIMER))
> > +                     cpu = housekeeping_any_cpu(HK_TYPE_TIMER);
>
> add_timer() already picks up a housekeeping CPU. So why is it needed?
>

Well, back when it was merged, I could see add_timer() using an
isolated CPU, and that's why I sent this change.
Did this change recently?

Also, previously add_timer() was only called if (cpu ==
WORK_CPU_UNBOUND), and now we make sure that for any value of 'cpu'
the timer to be put in a housekeeping cpu.

Thanks!
Leo

> Thanks.
>
> >               add_timer_on(timer, cpu);
> > -     else
> > -             add_timer(timer);
> > +     } else {
> > +             if (likely(cpu == WORK_CPU_UNBOUND))
> > +                     add_timer(timer);
> > +             else
> > +                     add_timer_on(timer, cpu);
> > +     }
>
> >  }
> >
> >  /**
> > --
> > 2.43.0
> >
> >
>