Received: by 2002:a25:8b91:0:0:0:0:0 with SMTP id j17csp21330067ybl; Mon, 6 Jan 2020 02:29:28 -0800 (PST) X-Google-Smtp-Source: APXvYqylf7VWbVU/+lxXz+4MCXG+K4Sx8uMZ0W2jj4d3BiidKKg/VQCdkboyFT+3QnswJavsrzt+ X-Received: by 2002:a9d:68d1:: with SMTP id i17mr96743204oto.367.1578306568675; Mon, 06 Jan 2020 02:29:28 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1578306568; cv=none; d=google.com; s=arc-20160816; b=O9eoMjJT9lrpoKP2nMOIVS1LLjTroygLtqhHKEriP9tQ7w+daDqfJfd9kSLHv2xG9C rcKvwBcZZ96MIFxnPl9poFLfUPxtAmCHS587/pIsOljlp48EVxCiLA165MA6sSJ9Kcmo Jjb44pj7yAdn5VVPvUKA5nAAa/zd0tRtQW2tJCMDS1TzqThL5QcCZWXFC8QZ50suY10j AyZd121ehn0gN/O9q5elXdkC0STDSwGR8oOncFAzMWc74z4YgOzt+XqN1JDiGViNLx97 jhW8j30qFlv8I0WSTOky9WNpoaMqqU10+VMk5RZZtSAxPfG7wbK9OItJsGMkxNoe6UAu kH3g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-disposition:mime-version:references:message-id:subject:cc :to:from:date:dkim-signature; bh=Lz46G6qRS0iXyjoqa8gTgIUca8qdvcSqxaUA8OlqGiA=; b=U+Qo12s3N3ErvYlvOVbQm65BUV2i8dK3LzT1EG01+mYG3EgFFdfwLUgxhQTjz/Y6LS URxZCo9xVtodoXJ5ROXGLvu+urVQlvIcmiX1V/kFCIlfFTAK9Ft8/mCWBM79barAKzXX MoAPHKfiv5LcCI3UJMDIiWYSKpWJeV3k1LCRT78wYTcPSc2mFbbWmgm6Q12Hh68teZGy P3bhib6+l5Is7i/DJRp9sH/TMpHu1xP8nheZM9PPF8apCouZmN6WVQBTzQe7Yu5AJn8E BNfbCHxdTOP+ABGowLdX+srRmfp4X0p7QUmCYWJm9Z2R5Gkf6Jt+yUstkKnD+agMDteI C/yg== ARC-Authentication-Results: i=1; mx.google.com; dkim=fail header.i=@infradead.org header.s=merlin.20170209 header.b=RYFVFEjG; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id i15si34030429otk.120.2020.01.06.02.29.15; Mon, 06 Jan 2020 02:29:28 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=fail header.i=@infradead.org header.s=merlin.20170209 header.b=RYFVFEjG; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726290AbgAFK2f (ORCPT + 99 others); Mon, 6 Jan 2020 05:28:35 -0500 Received: from merlin.infradead.org ([205.233.59.134]:40422 "EHLO merlin.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725446AbgAFK2e (ORCPT ); Mon, 6 Jan 2020 05:28:34 -0500 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=merlin.20170209; h=In-Reply-To:Content-Type:MIME-Version: References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=Lz46G6qRS0iXyjoqa8gTgIUca8qdvcSqxaUA8OlqGiA=; b=RYFVFEjG2yG4Q+Y7aFP46IKAz ndmqMBU4hVaUjfQZ0ot9M7T5I6lXeU2ZXGFx3UkRkssrcOF0/hSvq1H8Nge9H7xH1A0LMEphTq3Ec xRGZ2h82ux/ALJpOnxEMhXQLxevkaPIg2pqELwDJGNpwVAzd4RaLvGG+zqZ5R4u4MQW8JRLVi94aO D1v4XPBlq1C0uE/mSrdEy4PsnO8NrqG/ApcQurqx22kjAPan+nL3TdzrPAb0FusoNqqQjBDd7e1kP DyF9hEAmMTzOQV9fr6ysxgMSJsZ5f0PaQ0WR/BXg+qmvBxztf5SLKDmhuCf55oMkvB4XpjuO+lU6L vyEZY4T4A==; Received: from j217100.upc-j.chello.nl ([24.132.217.100] helo=noisy.programming.kicks-ass.net) by merlin.infradead.org with esmtpsa (Exim 4.92.3 #3 (Red Hat Linux)) id 1ioPcQ-0000wS-9o; Mon, 06 Jan 2020 10:28:18 +0000 Received: from hirez.programming.kicks-ass.net (hirez.programming.kicks-ass.net [192.168.1.225]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by noisy.programming.kicks-ass.net (Postfix) with ESMTPS id B3420306368; Mon, 6 Jan 2020 11:26:44 +0100 (CET) Received: by hirez.programming.kicks-ass.net (Postfix, from userid 1000) id B72F62B27B121; Mon, 6 Jan 2020 11:28:16 +0100 (CET) Date: Mon, 6 Jan 2020 11:28:16 +0100 From: Peter Zijlstra To: Dietmar Eggemann Cc: Peng Liu , linux-kernel@vger.kernel.org, mingo@redhat.com, juri.lelli@redhat.com, vincent.guittot@linaro.org, rostedt@goodmis.org, bsegall@google.com, mgorman@suse.de, qais.yousef@arm.com, morten.rasmussen@arm.com, valentin.schneider@arm.com Subject: Re: [PATCH v2] sched/fair: fix sgc->{min,max}_capacity miscalculate Message-ID: <20200106102816.GN2810@hirez.programming.kicks-ass.net> References: <20200104130828.GA7718@iZj6chx1xj0e0buvshuecpZ> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Jan 06, 2020 at 10:25:49AM +0100, Dietmar Eggemann wrote: > On 04/01/2020 14:08, Peng Liu wrote: > > Could you add a hint that this is about the SD_OVERLAP path? Something > like 'Fix sgc->{min,max}_capacity calculation for SD_OVERLAP' > > > commit bf475ce0a3dd ("sched/fair: Add per-CPU min capacity to > > sched_group_capacity") introduced per-cpu min_capacity. > > > > commit e3d6d0cb66f2 ("sched/fair: Add sched_group per-CPU max capacity") > > introduced per-cpu max_capacity. > > > > Here, capacity is the accumulated sum of (maybe) many CPUs' capacity. > > Compare with capacity to get {min,max}_capacity makes no sense. Instead, > > we should compare one by one in each iteration to get > > sgc->{min,max}_capacity of the group. > > > > Also, the only CPU in rq->sd->groups should be rq's CPU. Thus, > > capacity_of(cpu_of(rq)) should be equal to rq->sd->groups->sgc->capacity. > > Code can be simplified by removing the if/else. > > Could we improve the description of the issue and the change a little > bit? Something like: > > In the SD_OVERLAP case, the local variable 'capacity' represents the sum > of CPU capacity of all CPUs in the first sched group (sg) of the sched > domain (sd). > > It is erroneously used to calculate sg's min and max CPU capacity. > To fix this use capacity_of(cpu) instead of 'capacity'. > > The code which achieves this via cpu_rq(cpu)->sd->groups->sgc->capacity > (for rq->sd != NULL) can be removed since it delivers the same value as > capacity_of(cpu) which is currently only used for the (!rq->sd) case > (see update_cpu_capacity()). > A sg of the lowest sd (rq->sd or sd->child == NULL) represents a single > CPU (and hence sg->sgc->capacity == capacity_of(cpu)). > I've made it like so. --- Subject: sched/fair: Fix sgc->{min,max}_capacity calculation for SD_OVERLAP From: Peng Liu Date: Sat, 4 Jan 2020 21:08:28 +0800 commit bf475ce0a3dd ("sched/fair: Add per-CPU min capacity to sched_group_capacity") introduced per-cpu min_capacity. commit e3d6d0cb66f2 ("sched/fair: Add sched_group per-CPU max capacity") introduced per-cpu max_capacity. In the SD_OVERLAP case, the local variable 'capacity' represents the sum of CPU capacity of all CPUs in the first sched group (sg) of the sched domain (sd). It is erroneously used to calculate sg's min and max CPU capacity. To fix this use capacity_of(cpu) instead of 'capacity'. The code which achieves this via cpu_rq(cpu)->sd->groups->sgc->capacity (for rq->sd != NULL) can be removed since it delivers the same value as capacity_of(cpu) which is currently only used for the (!rq->sd) case (see update_cpu_capacity()). An sg of the lowest sd (rq->sd or sd->child == NULL) represents a single CPU (and hence sg->sgc->capacity == capacity_of(cpu)). Signed-off-by: Peng Liu Signed-off-by: Peter Zijlstra (Intel) Reviewed-by: Valentin Schneider Link: https://lkml.kernel.org/r/20200104130828.GA7718@iZj6chx1xj0e0buvshuecpZ --- kernel/sched/fair.c | 26 ++++---------------------- 1 file changed, 4 insertions(+), 22 deletions(-) --- a/kernel/sched/fair.c +++ b/kernel/sched/fair.c @@ -7855,29 +7855,11 @@ void update_group_capacity(struct sched_ */ for_each_cpu(cpu, sched_group_span(sdg)) { - struct sched_group_capacity *sgc; - struct rq *rq = cpu_rq(cpu); + unsigned long cpu_cap = capacity_of(cpu); - /* - * build_sched_domains() -> init_sched_groups_capacity() - * gets here before we've attached the domains to the - * runqueues. - * - * Use capacity_of(), which is set irrespective of domains - * in update_cpu_capacity(). - * - * This avoids capacity from being 0 and - * causing divide-by-zero issues on boot. - */ - if (unlikely(!rq->sd)) { - capacity += capacity_of(cpu); - } else { - sgc = rq->sd->groups->sgc; - capacity += sgc->capacity; - } - - min_capacity = min(capacity, min_capacity); - max_capacity = max(capacity, max_capacity); + capacity += cpu_cap; + min_capacity = min(cpu_cap, min_capacity); + max_capacity = max(cpu_cap, max_capacity); } } else { /*