Re: [PATCH] cpufreq: queue policy->update work to rt thread to reduce its schedule latency

[Rafael J. Wysocki]

On Mon, Jul 22, 2024 at 4:30 AM Gaowei Pu <pugaowei@xxxxxxxx> wrote:
>
> Hi Tim,
>
> On 2024/7/19 6:03, Tim Chen wrote:
> > On Wed, 2024-07-17 at 14:33 +0800, Gaowei Pu wrote:
> >> Currently we encountered a problem that the cpufreq boost latency
> >> is about 10 milliseconds or worse when we boost through cpufreq QOS request
> >> under high workload scenarios, while the boost latency mainly consumed by
> >> schedule latency of policy->update work.
> >
> > What is the tail latency now after your change?
> >
> >>
> >> We should ensure the low schedule latency of cpu frequency limits work
> >> to meet performance and power demands. so queue the policy->update work
> >> to rt thread to reduce its schedule latency.
> >
> > If my understanding is correct, kthread has a default nice
> > value of 0 and is not a rt thread.
> >
> > I think the gain you see is
> > your patch created a dedicated kthread work queue on CPU 0.
> > The work from policy change no longer have to compete time with other
> > requests coming from schedule_work().
>
> It's not just other requests coming from schedule_work(), also some normal
> cfs tasks running on the same cpu.

Do you have any data to support this statement?

> In order to not competing time with the above threads, i change the thread
> policy to rt and prio set to 98 to reduce the schedule latency.

By how much?

> >
> > If the policy change really needs to get ahead
> > of other tasks, I think you need a dedicated
> > workqueue with alloc_workqueue() using WQ_HIGHPRI flag.
>
> I think the cpufreq boost or limit action should be trigger in time to meet
> performance and power demands. An dedicated workqueue with highpri will be
> better but maybe not good enough because cfs pick or preempt policy is not
> purely based on thread nice value. So i think the final solution is rt thread
> and the policy change work deserves it :)

The "I think" and "maybe" in the above paragraph are not particularly
convincing.

Switching it over to use a dedicated workqueue would be a no-brainer
as using dedicated workqueues is recommended anyway and if it
measurably improves performance, that's for the better.

However, making it use a worker thread the way this patch does
requires quite a clear demonstration that the above is not sufficient.

Thanks!