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 B6C59C433EF for ; Fri, 18 Mar 2022 16:29:42 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S239221AbiCRQa6 (ORCPT ); Fri, 18 Mar 2022 12:30:58 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50404 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S239155AbiCRQap (ORCPT ); Fri, 18 Mar 2022 12:30:45 -0400 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id D3442195D8D for ; Fri, 18 Mar 2022 09:28:47 -0700 (PDT) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 8F2C71515; Fri, 18 Mar 2022 09:28:46 -0700 (PDT) Received: from [192.168.178.6] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id DA7D03F7B4; Fri, 18 Mar 2022 09:28:44 -0700 (PDT) Message-ID: <7c75e5d4-75a2-8ea5-64ad-13794a6036b6@arm.com> Date: Fri, 18 Mar 2022 17:28:43 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.5.0 Subject: Re: [PATCH] sched/fair: Refactor cpu_util_without() Content-Language: en-US To: Vincent Guittot Cc: Ingo Molnar , Peter Zijlstra , Juri Lelli , Steven Rostedt , Mel Gorman , Ben Segall , Patrick Bellasi , Vincent Donnefort , linux-kernel@vger.kernel.org References: <20220301171727.812157-1-dietmar.eggemann@arm.com> From: Dietmar Eggemann In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org - Valentin Schneider On 02/03/2022 10:09, Vincent Guittot wrote: > On Tue, 1 Mar 2022 at 18:17, Dietmar Eggemann wrote: [...] > I have only minor comment Thanks for the review! [...] >> +static unsigned long cpu_util_next(int cpu, struct task_struct *p, int dst_cpu) >> +{ [...] >> + if (sched_feat(UTIL_EST)) { >> + util_est = READ_ONCE(cfs_rq->avg.util_est.enqueued); >> + >> + /* >> + * During wake-up, the task isn't enqueued yet and doesn't >> + * appear in the cfs_rq->avg.util_est.enqueued of any rq, >> + * so just add it (if needed) to "simulate" what will be >> + * cpu_util after the task has been enqueued. >> + */ >> + if (dst_cpu == cpu) >> + util_est += _task_util_est(p); >> + > > Could you add a comment that explains why the addition above will not > be removed below by the lsub_positive below so it isn't worth trying > to optimize such a case? Yes. I rewored the comments in cpu_util_next() so they also apply when called by cpu_util_without(). And I use a `if{}/else if{}` here too in v2. >> + /* >> + * Despite the following checks we still have a small window >> + * for a possible race, when an execl's select_task_rq_fair() >> + * races with LB's detach_task(): >> + * >> + * detach_task() >> + * p->on_rq = TASK_ON_RQ_MIGRATING; >> + * ---------------------------------- A >> + * deactivate_task() \ >> + * dequeue_task() + RaceTime >> + * util_est_dequeue() / >> + * ---------------------------------- B >> + * >> + * The additional check on "current == p" it's required to >> + * properly fix the execl regression and it helps in further >> + * reducing the chances for the above race. >> + */ >> + if (unlikely(task_on_rq_queued(p) || current == p)) >> + lsub_positive(&util_est, _task_util_est(p)); I did a lot of testing on mainline & v4.20 and there wasn't one occurrence of `p->on_rq == TASK_ON_RQ_MIGRATING` here. Not for WF_EXEC tasks (p->on_rq = TASK_ON_RQ_QUEUED) and in case of v4.20 not for WF_EXEC and WF_TTWU tasks (p->on_rq = 0). So I assume it's not needed. I left it in v2 though and mentioned it in the additional comment section of the patch. [...] >> static unsigned long cpu_util_without(int cpu, struct task_struct *p) >> { [...] >> /* >> * Covered cases: >> * >> @@ -6560,82 +6609,8 @@ static unsigned long cpu_util_without(int cpu, struct task_struct *p) >> * estimation of the spare capacity on that CPU, by just >> * considering the expected utilization of tasks already >> * runnable on that CPU. > > The comment about the covered cases above should be moved in > cpu_util_next() which is where the cases are covered now Yes. I Incorporated it into the comments in cpu_util_next() in v2. [...]