Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 10 Oct 2022 11:27:14 +0530
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
Message-ID: <20221010055714.GA1474@hu-pkondeti-hyd.qualcomm.com>
References: <20220913140817.GA9091@hu-pkondeti-hyd.qualcomm.com>
 <20220915062027.GA14713@hu-pkondeti-hyd.qualcomm.com>
 <CAJuCfpE_nM2uqixnds0d6wbsz4=OQ3KPoJ5HOqDhQXaxFGxwXQ@mail.gmail.com>
 <CAJuCfpEeNzDQ-CvMN3fP5LejOzpnfgUgvkzpPj1CLF-8NqNoww@mail.gmail.com>
 <CAJuCfpFr3JfwkWbDqkU=NUJbCYuCWGySwNusMCdmS3z95WD2AQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <CAJuCfpFr3JfwkWbDqkU=NUJbCYuCWGySwNusMCdmS3z95WD2AQ@mail.gmail.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk

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.
> 
> Hi Pavan,
> Testing PSI shutoff on Android proved more difficult than I expected.
> Lots of tasks to silence and I keep encountering new ones.
> The approach I was thinking about is something like this:
> 
> ---
>  include/linux/psi_types.h |  3 +++
>  kernel/sched/psi.c        | 12 +++++++++---
>  2 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/psi_types.h b/include/linux/psi_types.h
> index c7fe7c089718..8d936f22cb5b 100644
> --- a/include/linux/psi_types.h
> +++ b/include/linux/psi_types.h
> @@ -68,6 +68,9 @@ enum psi_states {
>          NR_PSI_STATES = 7,
>  };
> 
> +/* state_mask flag to keep re-arming averaging work */
> +#define PSI_STATE_WAKE_CLOCK        (1 << NR_PSI_STATES)
> +
>  enum psi_aggregators {
>          PSI_AVGS = 0,
>          PSI_POLL,
> diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
> index ecb4b4ff4ce0..dd62ad28bacd 100644
> --- a/kernel/sched/psi.c
> +++ b/kernel/sched/psi.c
> @@ -278,6 +278,7 @@ static void get_recent_times(struct psi_group
> *group, int cpu,
>                  if (delta)
>                          *pchanged_states |= (1 << s);
>          }
> +        *pchanged_states |= (state_mask & PSI_STATE_WAKE_CLOCK);
>  }
> 
>  static void calc_avgs(unsigned long avg[3], int missed_periods,
> @@ -413,7 +414,7 @@ static void psi_avgs_work(struct work_struct *work)
>          struct delayed_work *dwork;
>          struct psi_group *group;
>          u32 changed_states;
> -        bool nonidle;
> +        bool wake_clock;
>          u64 now;
> 
>          dwork = to_delayed_work(work);
> @@ -424,7 +425,7 @@ static void psi_avgs_work(struct work_struct *work)
>          now = sched_clock();
> 
>          collect_percpu_times(group, PSI_AVGS, &changed_states);
> -        nonidle = changed_states & (1 << PSI_NONIDLE);
> +        wake_clock = changed_states & PSI_STATE_WAKE_CLOCK;
>          /*
>           * If there is task activity, periodically fold the per-cpu
>           * times and feed samples into the running averages. If things
> @@ -435,7 +436,7 @@ static void psi_avgs_work(struct work_struct *work)
>          if (now >= group->avg_next_update)
>                  group->avg_next_update = update_averages(group, now);
> 
> -        if (nonidle) {
> +        if (wake_clock) {
>                  schedule_delayed_work(dwork, nsecs_to_jiffies(
>                                  group->avg_next_update - now) + 1);
>          }
> @@ -742,6 +743,11 @@ static void psi_group_change(struct psi_group
> *group, int cpu,
>          if (unlikely(groupc->tasks[NR_ONCPU] && cpu_curr(cpu)->in_memstall))
>                  state_mask |= (1 << PSI_MEM_FULL);
> 
> +        if (wake_clock || test_state(groupc->tasks, PSI_NONIDLE)) {
> +                /* psi_avgs_work was not the only task on the CPU */
> +                state_mask |= PSI_STATE_WAKE_CLOCK;
> +        }
> +

Thanks Suren for taking a look. I was not at work last week, so could not
reply earlier. 

As chengming pointed out already, when kworker is woken up (some tasks ran in
the last window, so we scheudled a work. now kworker woke up when system is
idle), PSI_NONIDLE condition will be true here,  we end up setting the
PSI_STATE_WAKE_CLOCK flag. Correct? Any ways, I am going to test this patch and
report my observations.

Thanks,
Pavan