Messages in this thread | | | From | "Rafael J. Wysocki" <> | Date | Wed, 9 May 2018 11:06:24 +0200 | Subject | Re: [RFC PATCH] sched/cpufreq/schedutil: handling urgent frequency requests |
| |
On Wed, May 9, 2018 at 10:51 AM, Joel Fernandes <joel@joelfernandes.org> wrote: > On Wed, May 09, 2018 at 10:30:37AM +0200, Rafael J. Wysocki wrote: >> On Wed, May 9, 2018 at 10:06 AM, Joel Fernandes <joel@joelfernandes.org> wrote: >> > On Wed, May 09, 2018 at 08:45:30AM +0200, Juri Lelli wrote: >> >> On 08/05/18 21:54, Joel Fernandes wrote: >> >> >> >> [...] >> >> >> >> > Just for discussion sake, is there any need for work_in_progress? If we can >> >> > queue multiple work say kthread_queue_work can handle it, then just queuing >> >> > works whenever they are available should be Ok and the kthread loop can >> >> > handle them. __cpufreq_driver_target is also protected by the work lock if >> >> > there is any concern that can have races... only thing is rate-limiting of >> >> > the requests, but we are doing a rate limiting, just not for the "DL >> >> > increased utilization" type requests (which I don't think we are doing at the >> >> > moment for urgent DL requests anyway). >> >> > >> >> > Following is an untested diff to show the idea. What do you think? >> >> > >> >> > thanks, >> >> > >> >> > - Joel >> >> > >> >> > ----8<--- >> >> > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c >> >> > index d2c6083304b4..862634ff4bf3 100644 >> >> > --- a/kernel/sched/cpufreq_schedutil.c >> >> > +++ b/kernel/sched/cpufreq_schedutil.c >> >> > @@ -38,7 +38,6 @@ struct sugov_policy { >> >> > struct mutex work_lock; >> >> > struct kthread_worker worker; >> >> > struct task_struct *thread; >> >> > - bool work_in_progress; >> >> > >> >> > bool need_freq_update; >> >> > }; >> >> > @@ -92,16 +91,8 @@ static bool sugov_should_update_freq(struct sugov_policy *sg_policy, u64 time) >> >> > !cpufreq_can_do_remote_dvfs(sg_policy->policy)) >> >> > return false; >> >> > >> >> > - if (sg_policy->work_in_progress) >> >> > - return false; >> >> > - >> >> > if (unlikely(sg_policy->need_freq_update)) { >> >> > sg_policy->need_freq_update = false; >> >> > - /* >> >> > - * This happens when limits change, so forget the previous >> >> > - * next_freq value and force an update. >> >> > - */ >> >> > - sg_policy->next_freq = UINT_MAX; >> >> > return true; >> >> > } >> >> > >> >> > @@ -129,7 +120,6 @@ static void sugov_update_commit(struct sugov_policy *sg_policy, u64 time, >> >> > policy->cur = next_freq; >> >> > trace_cpu_frequency(next_freq, smp_processor_id()); >> >> > } else { >> >> > - sg_policy->work_in_progress = true; >> >> > irq_work_queue(&sg_policy->irq_work); >> >> >> >> Isn't this potentially introducing unneeded irq pressure (and doing the >> >> whole wakeup the kthread thing), while the already active kthread could >> >> simply handle multiple back-to-back requests before going to sleep? >> > >> > How about this? Will use the latest request, and also doesn't do unnecessary >> > irq_work_queue: >> > >> > (untested) >> > -----8<-------- >> > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c >> > index d2c6083304b4..6a3e42b01f52 100644 >> > --- a/kernel/sched/cpufreq_schedutil.c >> > +++ b/kernel/sched/cpufreq_schedutil.c >> > @@ -38,7 +38,7 @@ struct sugov_policy { >> > struct mutex work_lock; >> > struct kthread_worker worker; >> > struct task_struct *thread; >> > - bool work_in_progress; >> > + bool work_in_progress; /* Has kthread been kicked */ >> > >> > bool need_freq_update; >> > }; >> > @@ -92,9 +92,6 @@ static bool sugov_should_update_freq(struct sugov_policy *sg_policy, u64 time) >> > !cpufreq_can_do_remote_dvfs(sg_policy->policy)) >> > return false; >> > >> > - if (sg_policy->work_in_progress) >> > - return false; >> > - >> >> Why this change? >> >> Doing the below is rather pointless if work_in_progress is set, isn't it? > > The issue being discussed is that if a work was already in progress, then new > frequency updates will be dropped. So say even if DL increased in > utilization, nothing will happen because if work_in_progress = true and > need_freq_update = true, we would skip an update. In this diff, I am > allowing the frequency request to be possible while work_in_progress is true. > In the end the latest update will be picked.
I'm not sure if taking new requests with the irq_work in flight is a good idea.
>> >> You'll drop the results of it on the floor going forward anyway then AFAICS. > > Why?
Because you cannot queue up a new irq_work before the previous one is complete?
| |