From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id E2390C433FE for ; Mon, 10 Oct 2022 05:57:28 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231470AbiJJF51 (ORCPT ); Mon, 10 Oct 2022 01:57:27 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40092 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230435AbiJJF5Y (ORCPT ); Mon, 10 Oct 2022 01:57:24 -0400 Received: from mx0a-0031df01.pphosted.com (mx0a-0031df01.pphosted.com [205.220.168.131]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 840FD51A04 for ; Sun, 9 Oct 2022 22:57:23 -0700 (PDT) Received: from pps.filterd (m0279866.ppops.net [127.0.0.1]) by mx0a-0031df01.pphosted.com (8.17.1.5/8.17.1.5) with ESMTP id 29A4UHUI025543; Mon, 10 Oct 2022 05:57:21 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=quicinc.com; h=date : from : to : cc : subject : message-id : references : mime-version : content-type : in-reply-to; s=qcppdkim1; bh=nI0KDHpRqxcEgXThQ0ftfTnqusKOnSOB3o1TNaJEVjA=; b=EGdWSbDXgRnM9HQFMBJOFUboUk8AhT+wQbQaUyJW6EDrP24mdxbhpVw0Lh4X8CrPqAeu OCdAJUpoWm2pyJohJcHukxW4adqNH7AwpBCwB3Wk52CSmMdHn9/VP4cnAqatpDFlMw6q J+n2fE0XH5cBTMooxvsd5S1eijzHPIreALSmD3NBiauvh3ViAUjU3XDg5kqsGC4NgieL N9ZeoB/GMZV8B9Ir4GNGo/szWUPfNwu9pz0+Z+7PFp3gW6jiP5osITsdfIT15M3Fq4fq QLcYCVe/gTEKKP5JdethnjC+Gc1XDHuyxVDU6AqbAjWRwr3gH8NGaKq+5X/VhFe6jMIq cQ== Received: from nalasppmta04.qualcomm.com (Global_NAT1.qualcomm.com [129.46.96.20]) by mx0a-0031df01.pphosted.com (PPS) with ESMTPS id 3k32gekb3u-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 10 Oct 2022 05:57:21 +0000 Received: from nalasex01a.na.qualcomm.com (nalasex01a.na.qualcomm.com [10.47.209.196]) by NALASPPMTA04.qualcomm.com (8.17.1.5/8.17.1.5) with ESMTPS id 29A5vKGs000818 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 10 Oct 2022 05:57:20 GMT Received: from hu-pkondeti-hyd.qualcomm.com (10.80.80.8) by nalasex01a.na.qualcomm.com (10.47.209.196) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.986.29; Sun, 9 Oct 2022 22:57:18 -0700 Date: Mon, 10 Oct 2022 11:27:14 +0530 From: Pavan Kondeti To: Suren Baghdasaryan CC: Pavan Kondeti , Johannes Weiner , LKML , Charan Teja Kalla 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> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) X-Originating-IP: [10.80.80.8] X-ClientProxiedBy: nasanex01a.na.qualcomm.com (10.52.223.231) To nalasex01a.na.qualcomm.com (10.47.209.196) X-QCInternal: smtphost X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=5800 signatures=585085 X-Proofpoint-GUID: fYVEwInpr9-FJM1Zi3hYoauXKftvKyUU X-Proofpoint-ORIG-GUID: fYVEwInpr9-FJM1Zi3hYoauXKftvKyUU X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.528,FMLib:17.11.122.1 definitions=2022-10-07_04,2022-10-07_01,2022-06-22_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 impostorscore=0 spamscore=0 malwarescore=0 mlxlogscore=999 clxscore=1015 adultscore=0 lowpriorityscore=0 bulkscore=0 phishscore=0 priorityscore=1501 suspectscore=0 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2209130000 definitions=main-2210100035 Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Oct 05, 2022 at 09:32:44AM -0700, Suren Baghdasaryan wrote: > On Sun, Oct 2, 2022 at 11:11 PM Suren Baghdasaryan wrote: > > > > On Fri, Sep 16, 2022 at 10:45 PM Suren Baghdasaryan wrote: > > > > > > On Wed, Sep 14, 2022 at 11:20 PM Pavan Kondeti > > > 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