Re: PSI idle-shutoff

From: Pavan Kondeti <quic_pkondeti@quicinc.com>
To: Suren Baghdasaryan <surenb@google.com>
Cc: Pavan Kondeti <quic_pkondeti@quicinc.com>,
	Johannes Weiner <hannes@cmpxchg.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Charan Teja Kalla <quic_charante@quicinc.com>
Subject: Re: PSI idle-shutoff
Date: Mon, 10 Oct 2022 11:55:38 +0530	[thread overview]
Message-ID: <20221010062538.GC1474@hu-pkondeti-hyd.qualcomm.com> (raw)
In-Reply-To: <CAJuCfpFr3JfwkWbDqkU=NUJbCYuCWGySwNusMCdmS3z95WD2AQ@mail.gmail.com>

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