Messages in this thread | | | Date | Mon, 10 Oct 2022 11:55:38 +0530 | From | Pavan Kondeti <> | Subject | Re: PSI idle-shutoff |
| |
On Wed, Oct 05, 2022 at 09:32:44AM -0700, Suren Baghdasaryan wrote: > On Sun, Oct 2, 2022 at 11:11 PM Suren Baghdasaryan <surenb@google.com> wrote: > > > > On Fri, Sep 16, 2022 at 10:45 PM Suren Baghdasaryan <surenb@google.com> wrote: > > > > > > On Wed, Sep 14, 2022 at 11:20 PM Pavan Kondeti > > > <quic_pkondeti@quicinc.com> wrote: > > > > > > > > On Tue, Sep 13, 2022 at 07:38:17PM +0530, Pavan Kondeti wrote: > > > > > Hi > > > > > > > > > > The fact that psi_avgs_work()->collect_percpu_times()->get_recent_times() > > > > > run from a kworker thread, PSI_NONIDLE condition would be observed as > > > > > there is a RUNNING task. So we would always end up re-arming the work. > > > > > > > > > > If the work is re-armed from the psi_avgs_work() it self, the backing off > > > > > logic in psi_task_change() (will be moved to psi_task_switch soon) can't > > > > > help. The work is already scheduled. so we don't do anything there. > > > > > > Hi Pavan, > > > Thanks for reporting the issue. IIRC [1] was meant to fix exactly this > > > issue. At the time it was written I tested it and it seemed to work. > > > Maybe I missed something or some other change introduced afterwards > > > affected the shutoff logic. I'll take a closer look next week when I'm > > > back at my computer and will consult with Johannes. > > > > Sorry for the delay. I had some time to look into this and test psi > > shutoff on my device and I think you are right. The patch I mentioned > > prevents new psi_avgs_work from being scheduled when the only non-idle > > task is psi_avgs_work itself, however the regular 2sec averaging work > > will still go on. I think we could record the fact that the only > > active task is psi_avgs_work in record_times() using a new > > psi_group_cpu.state_mask flag and then prevent psi_avgs_work() from > > rescheduling itself if that flag is set for all non-idle cpus. I'll > > test this approach and will post a patch for review if that works. >
<snip>
> > This should detect the activity caused by psi_avgs_work() itself and > ignore it when deciding to reschedule the averaging work. In the > formula you posted: > > non_idle_time = (work_start_now - wakeup_now) + (sleep_prev - work_end_prev) > > the first term is calculated only if the PSI state is still active > (https://elixir.bootlin.com/linux/latest/source/kernel/sched/psi.c#L271). > psi_group_change() will reset that state if psi_avgs_work() was the > only task on that CPU, so it won't affect non_idle_time. The code > above is to take care of the second term. Could you please check if > this approach helps? As I mentioned I'm having trouble getting all the > tasks silent on Android for a clear test. > > The issue with deferrable timers that you mentioned, how often does > that happen? If it happens only occasionally and prevents PSI shutoff > for a couple of update cycles then I don't think that's a huge > problem. Once PSI shutoff happens it should stay shut. Is that the > case?
In our system, there are periodic but deferrable timers/works (DCVS, QOS related) whose period is lower than PSI period. So once PSI average work runs, I see other unrelated work getting scheduled due to which PSI runs again. I actually added a hack not to re-arm the PSI work if the system has only one work (nr_running() == 1) and still see the problem. The moment, I made PSI work as deferrable, all the problems gone. I know that is wrong as it could simply not run PSI work when other CPUs are busy.
Thanks, Pavan
| |