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:   Fri, 4 Nov 2022 12:09:12 +0800
From:   Chen Yu <yu.c.chen@intel.com>
To:     Peter Zijlstra <peterz@infradead.org>
CC:     Vincent Guittot <vincent.guittot@linaro.org>,
        Tim Chen <tim.c.chen@intel.com>,
        Mel Gorman <mgorman@techsingularity.net>,
        Juri Lelli <juri.lelli@redhat.com>,
        Rik van Riel <riel@surriel.com>,
        Aaron Lu <aaron.lu@intel.com>,
        Abel Wu <wuyun.abel@bytedance.com>,
        K Prateek Nayak <kprateek.nayak@amd.com>,
        Yicong Yang <yangyicong@hisilicon.com>,
        "Gautham R . Shenoy" <gautham.shenoy@amd.com>,
        Ingo Molnar <mingo@redhat.com>,
        "Dietmar Eggemann" <dietmar.eggemann@arm.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Ben Segall <bsegall@google.com>,
        Daniel Bristot de Oliveira <bristot@redhat.com>,
        Valentin Schneider <vschneid@redhat.com>,
        Hillf Danton <hdanton@sina.com>,
        Honglei Wang <wanghonglei@didichuxing.com>,
        Len Brown <len.brown@intel.com>,
        Chen Yu <yu.chen.surf@gmail.com>,
        <linux-kernel@vger.kernel.org>
Subject: Re: [RFC PATCH v2 1/2] sched/fair: Introduce short duration task
 check
Message-ID: <Y2SQaBDcq7AVPStq@chenyu5-mobl1>
References: <cover.1666531576.git.yu.c.chen@intel.com>
 <6b81eea9a8cafb7634f36586f1744b8d4ac49da5.1666531576.git.yu.c.chen@intel.com>
 <Y2O3nDrvcy+KuGg1@hirez.programming.kicks-ass.net>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <Y2O3nDrvcy+KuGg1@hirez.programming.kicks-ass.net>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?P0hzCAZJ5DgwHtp7R3gOco4d65o0dvtQIyPIEAoE6+c3XvcGGKY8paBEBbW9?=
 =?us-ascii?Q?m3zbrYsTmWDqOGDv8dANUb7ljzTfa6B9gkRK+Y13nz89f/leWgMT4Mhc2yKY?=
 =?us-ascii?Q?jzza1HpVIVIj1755qKKfdKnFrY1/cRbjrFZt/rq8bOJFP5a2jXnyeB8xPAnT?=
 =?us-ascii?Q?cpAntsdsh4Dd/5RTQ9tVj7iM2OHkcTe0QfyAbP/KDuHn3uVuCr1oWVRuLerR?=
 =?us-ascii?Q?yRyZbM2bzU6+WvbR6hOTbNf9jrtbR4VpEIQAo7NQK4xK4PFo6JzfgoWHflaG?=
 =?us-ascii?Q?ZdYey73WxK306Nnoj/qRLiE0DEwCmeIJs+tEvLM6soIHr0S2d/RA3EfgpVKw?=
 =?us-ascii?Q?ZDEyfV8aIHFzthRd5qFvpLXNfpE3RJuMAMvUl6Fks2kpB99b5LHcmOq53Cex?=
 =?us-ascii?Q?6zEUDVQvmkxWnKOghkQXKsoIAqPjsWNw/Kbd8yvB/mmfXzmj7bbJonF13qVb?=
 =?us-ascii?Q?POXwLDSkRre3OJdXvGjNFosBqyqvaikvYc6YDDeKQY5CzL9lOw5m8th1WRoS?=
 =?us-ascii?Q?u9zTUkWqZ0bcdnZw1ChBmI0qKS242PMSNcMgSr2HSZHzmKQUZ8G2EeigpjPZ?=
 =?us-ascii?Q?F8xc8Ujfnjr45WLeUIgtivuogBGUpA9y2yfRzyrQJ7kfvnL3TECVDdT1kinh?=
 =?us-ascii?Q?9CGGKPQGArwIUBXmQwMDaAkPB2N+mPwN/10m0A0566LqMkXGcg3nhnl3qUXv?=
 =?us-ascii?Q?nX3hV8s1mJYQZgbeb56HgKHNpj47YzaJihJ3XDloKclaLe/tMPjDCAaQo/KO?=
 =?us-ascii?Q?VCCqbKJy5hGSIUkI6PmMHJB5c/Ys57HyT/jntmvkqoxQ+DwtVhos9yGth2OL?=
 =?us-ascii?Q?WKpsBnG/iBTKfRfbw45PRWzidYfAoDJcRhC9KXFR+YebzAOsv1zC7+G6Oa6R?=
 =?us-ascii?Q?mrK2wiE02JRSN7eMukTpR9KQIxzbSxGuXlb6P84lMsCRMXZtQlxoQYhAx8Ov?=
 =?us-ascii?Q?ll3rgNHYEhijMzdWz86B8sSqJkkX9KxQ5moqJQ5mNZugpVMZZ1w/C4Lnu2tx?=
 =?us-ascii?Q?3rRDVVFh/TjFhqUPoOb8V5ZsAS9Kuvh7ntJ3RB0SjEnJrJj3pchisViv0/o0?=
 =?us-ascii?Q?dgkFY9Qip+Y7hfLk69t4K7aGDC8AirYCJstSBfcYWIET4h5xOxVcXMwboLJK?=
 =?us-ascii?Q?Y6wW11Mpkxl+LfM+1606e57yeFr0QMglw5Ea/Qo9hkpgPoI7WFv8mvwHVGME?=
 =?us-ascii?Q?JoPZlLEIKA8UoAInwr4KBkKUhHeEOGrxrEDvSL+Rglt6OdCU7PKnXLWNq4Fg?=
 =?us-ascii?Q?+xSsGwRM3LYPjpK5NPs21Xw7AjyUKfKTke2C9+BG0YdsXNxkHlqyb/uFbF2y?=
 =?us-ascii?Q?/kNTgV/ijJX8+Ziyx9RJ5bWSD2FVZ39Xw2M6zAYrz7n1PIqz7lcZqikVK3ra?=
 =?us-ascii?Q?mqs3oXMdNkVOt60eNqxAp6j4b7DAP6XgkCsI6zA3shg7xTQCvK0aULx4JxOu?=
 =?us-ascii?Q?m2+6cLAYZgcrmLaHCd1Pyntsg7UF9kzgZGmtRkZEoyhzmm+wT8JDNo7Q18Nd?=
 =?us-ascii?Q?qo30otPsL58gxrf/mKm0fZo53EVSAdoto4yH6cbLT250I/wOhQRwV22s9j5P?=
 =?us-ascii?Q?oJR740qgxrHzBvaAD/e7yi37htDi4/t7lOuQHC0S?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 29949f6a-4ccc-40d6-7bbd-08dabe1a5e74
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6206.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Nov 2022 04:09:29.1184
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: BOF3q3EBw4ztyyjfS9bG+TPvVIC3bHilNfBhUqHZnodlfY3m822oz1ersWDPLw0LS4Ob3jlBpekxNfITqvoocA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB6450
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-8.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_HI,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Peter,
thanks for your review,
On 2022-11-03 at 13:44:12 +0100, Peter Zijlstra wrote:
> On Sun, Oct 23, 2022 at 11:32:31PM +0800, Chen Yu wrote:
> 
> > +	if (sched_feat(SIS_SHORT) && !prev->on_rq) {
> > +		/*
> > +		 * sum_exec_runtime has been updated in update_curr()
> > +		 * because we reach here via dequeue.
> > +		 */
> > +		this_dur_avg = se->sum_exec_runtime - se->prev_sum_runtime_vol;
> > +		/*
> > +		 * Record the accumulated runtime when task voluntarily
> > +		 * switches out. End of old duration period, a new period
> > +		 * starts.
> > +		 */
> > +		se->prev_sum_runtime_vol = se->sum_exec_runtime;
> > +
> > +		last_dur_avg = se->dur_avg;
> > +		delta = this_dur_avg - last_dur_avg;
> > +		/* consider large change to avoid frequent update */
> > +		if (abs(delta) >= sysctl_sched_min_granularity) {
> > +			/*
> > +			 * If it is the first time the task starts to
> > +			 * record dur_avg, discard the initial value 0.
> > +			 * Otherwise, calculate the EWMA.
> > +			 */
> > +			if (unlikely(!this_dur_avg))
> > +				se->dur_avg = this_dur_avg;
> > +			else
> > +				update_avg(&se->dur_avg, this_dur_avg);
> > +		}
> > +	}
> 
> This seems excessively convoluted; what's wrong with something like:
> 
> 	if (sched_feat(SIS_SHORT) && !prev->on_rq) {
> 		u64 this_dur = se->sum_exec_runtime - se->prev_sum_exec_runtime_vol;
> 		se->prev_sum_exec_runtime_vol = se->sum_exec_runtime;
> 
> 		update_avg(&se->dur_avg, this_dur);
> 	}
> 
> All that other stuff just makes it unreadable and probably slower.
OK, I'll try your suggestion in next version.

thanks,
Chenyu