Message ID | 20230617081926.2035113-1-linmiaohe@huawei.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:994d:0:b0:3d9:f83d:47d9 with SMTP id k13csp1854854vqr; Sat, 17 Jun 2023 01:36:33 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ4tZZPQ/8k4zUiVquYELbRciiRqKYra5zEPEEojUmcYpGaj2gJVJIJ2UQavIpJhJfRd3OFT X-Received: by 2002:a05:6a00:15cc:b0:655:89f1:2db8 with SMTP id o12-20020a056a0015cc00b0065589f12db8mr6558922pfu.16.1686990993201; Sat, 17 Jun 2023 01:36:33 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1686990993; cv=none; d=google.com; s=arc-20160816; b=oV65tgXZK3+hE1RRwsLpcONV14gzPVpPg62cmcIIYbgFvYBJyd+FHX4j5DxAP/x6Hb az5OAywzNge1Z5KXUqEcJT5RSpp+wtH5wZnnv2rYbF5Dor7jipI0Fcuicl0pPHsDEmQA MTY7ZaOuEt/t6a9P1Iijp+gtDPVZ5ZOfeXFe6kBljnipKBEuCeHFcpk+5l3OB0cmGUDv fJ5b1JjIibeJqkmLXmXv3Ato1j3Qh6LEahet/LHt9lcjs9YfsBrZy7r6BOOrS6dcvVd7 Nw71x8jVDj4g/E/nZ1DoXVRbeAn1DdSkNWeAv1MYWp84YANldQJovCcJMpoZJXWO8zJJ Mryw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from; bh=M87+fjaUz8Wn0QWb5HW65ws4H0ZMsGoR6DXDXcqj09o=; b=ZbmOzQkdyS7Z+qQsUBBiB+GSuU+nbwt6pIFO551v5QJDcSov+1xeCvSmEf7OEIHv6h kh3gES9CsVx/aojqHU+fveN0oMfu5eI5DLyntSqP+s4HCve/6SPK53HdYoo4WYdQL+EK PUA5L8BWZpKzzeeqoURCy8Zkss3mRy5d7wSTl0BliLXfcnPZbcaojBlVi032QimPXiMm bgMO2xvGlNUY9KAS3UrDTGt8fyI+0zQ/TEDLaseqbD7EeSoO5LeMHGNd44kTBW/sYfts JB6wC0mwt8w1grxkKT2pH9GrfZNvkzUHTM5EB1qHBUf59ZOlVlEoH/zwo+QJUrpEny4c iaSQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=huawei.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id y12-20020aa793cc000000b0066854219b5esi678139pff.186.2023.06.17.01.36.20; Sat, 17 Jun 2023 01:36:33 -0700 (PDT) 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; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=huawei.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1346126AbjFQIUA (ORCPT <rfc822;maxin.john@gmail.com> + 99 others); Sat, 17 Jun 2023 04:20:00 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:59670 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230313AbjFQIT6 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Sat, 17 Jun 2023 04:19:58 -0400 Received: from szxga08-in.huawei.com (szxga08-in.huawei.com [45.249.212.255]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 5D976173A for <linux-kernel@vger.kernel.org>; Sat, 17 Jun 2023 01:19:57 -0700 (PDT) Received: from canpemm500002.china.huawei.com (unknown [172.30.72.53]) by szxga08-in.huawei.com (SkyGuard) with ESMTP id 4QjpqF1rqjz18MBy; Sat, 17 Jun 2023 16:19:53 +0800 (CST) Received: from huawei.com (10.175.104.170) by canpemm500002.china.huawei.com (7.192.104.244) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.23; Sat, 17 Jun 2023 16:19:54 +0800 From: Miaohe Lin <linmiaohe@huawei.com> To: <mingo@redhat.com>, <peterz@infradead.org>, <juri.lelli@redhat.com>, <vincent.guittot@linaro.org> CC: <dietmar.eggemann@arm.com>, <rostedt@goodmis.org>, <bsegall@google.com>, <mgorman@suse.de>, <bristot@redhat.com>, <vschneid@redhat.com>, <linux-kernel@vger.kernel.org>, <linmiaohe@huawei.com> Subject: [PATCH] sched/topology: remove unneeded do while loop in cpu_attach_domain() Date: Sat, 17 Jun 2023 16:19:26 +0800 Message-ID: <20230617081926.2035113-1-linmiaohe@huawei.com> X-Mailer: git-send-email 2.23.0 MIME-Version: 1.0 Content-Transfer-Encoding: 7BIT Content-Type: text/plain; charset=US-ASCII X-Originating-IP: [10.175.104.170] X-ClientProxiedBy: dggems705-chm.china.huawei.com (10.3.19.182) To canpemm500002.china.huawei.com (7.192.104.244) X-CFilter-Loop: Reflected X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_MED, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE 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 X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1768938267391991812?= X-GMAIL-MSGID: =?utf-8?q?1768938267391991812?= |
Series |
sched/topology: remove unneeded do while loop in cpu_attach_domain()
|
|
Commit Message
Miaohe Lin
June 17, 2023, 8:19 a.m. UTC
When sg != sd->groups, the do while loop would cause deadloop here. But
that won't occur because sg is always equal to sd->groups now. Remove
this unneeded do while loop.
Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
kernel/sched/topology.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
Comments
On 2023-06-17 at 16:19:26 +0800, Miaohe Lin wrote: > When sg != sd->groups, the do while loop would cause deadloop here. But > that won't occur because sg is always equal to sd->groups now. Remove > this unneeded do while loop. > > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > --- > kernel/sched/topology.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c > index ca4472281c28..9010c93c3fdb 100644 > --- a/kernel/sched/topology.c > +++ b/kernel/sched/topology.c > @@ -750,10 +750,7 @@ cpu_attach_domain(struct sched_domain *sd, struct root_domain *rd, int cpu) > * domain for convenience. Clear such flags since > * the child is being destroyed. > */ > - do { > - sg->flags = 0; I guess we don't need to clear the flag of remote groups for now. > - } while (sg != sd->groups); > - > + sg->flags = 0; > sd->child = NULL; > } > } > -- > 2.27.0 > Reviewed-by: Chen Yu <yu.c.chen@intel.com> thanks, Chenyu
On Sat, Jun 17, 2023 at 04:19:26PM +0800, Miaohe Lin wrote: > When sg != sd->groups, the do while loop would cause deadloop here. But > that won't occur because sg is always equal to sd->groups now. Remove > this unneeded do while loop. This Changelog makes no sense to me.. Yes, as is the do {} while loop is dead code, but it *should* have read like: do { sg->flags = 0; sg = sg->next; } while (sg != sd->groups); as I noted here: https://lore.kernel.org/all/20230523105935.GN83892@hirez.programming.kicks-ass.net/T/#u So what this changelog should argue is how there cannot be multiple groups here -- or if there can be, add the missing iteration. > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > --- > kernel/sched/topology.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c > index ca4472281c28..9010c93c3fdb 100644 > --- a/kernel/sched/topology.c > +++ b/kernel/sched/topology.c > @@ -750,10 +750,7 @@ cpu_attach_domain(struct sched_domain *sd, struct root_domain *rd, int cpu) > * domain for convenience. Clear such flags since > * the child is being destroyed. > */ > - do { > - sg->flags = 0; > - } while (sg != sd->groups); > - > + sg->flags = 0; > sd->child = NULL; > } > } > -- > 2.27.0 >
On 2023/6/20 22:11, Peter Zijlstra wrote: > On Sat, Jun 17, 2023 at 04:19:26PM +0800, Miaohe Lin wrote: >> When sg != sd->groups, the do while loop would cause deadloop here. But >> that won't occur because sg is always equal to sd->groups now. Remove >> this unneeded do while loop. > > This Changelog makes no sense to me.. Yes, as is the do {} while loop is > dead code, but it *should* have read like: > > do { > sg->flags = 0; > sg = sg->next; > } while (sg != sd->groups); > > as I noted here: > > https://lore.kernel.org/all/20230523105935.GN83892@hirez.programming.kicks-ass.net/T/#u [1] > > So what this changelog should argue is how there cannot be multiple > groups here -- or if there can be, add the missing iteration. [1] said: " Yes, I missed that. That being said, the only reason for sd to be degenerate is that there is only 1 group. Otherwise we will keep it and degenerate parents instead " IOW, "sg = sg->next;" is missed intentionally in the do while{} loop to show that there's only 1 sched group. This looks weird to me but no persist on change the code. Thanks.
On Wed, Jun 21, 2023 at 10:53:57AM +0800, Miaohe Lin wrote: > On 2023/6/20 22:11, Peter Zijlstra wrote: > > On Sat, Jun 17, 2023 at 04:19:26PM +0800, Miaohe Lin wrote: > >> When sg != sd->groups, the do while loop would cause deadloop here. But > >> that won't occur because sg is always equal to sd->groups now. Remove > >> this unneeded do while loop. > > > > This Changelog makes no sense to me.. Yes, as is the do {} while loop is > > dead code, but it *should* have read like: > > > > do { > > sg->flags = 0; > > sg = sg->next; > > } while (sg != sd->groups); Yes, I agree that this is the correct solution. > > > > as I noted here: > > > > https://lore.kernel.org/all/20230523105935.GN83892@hirez.programming.kicks-ass.net/T/#u I apologize. I missed this e-mail. > > [1] > > > > > So what this changelog should argue is how there cannot be multiple > > groups here -- or if there can be, add the missing iteration. > > [1] said: > " > Yes, I missed that. > > That being said, the only reason for sd to be degenerate is that there > is only 1 group. Otherwise we will keep it and degenerate parents > instead > " In the section of the code in question ,`sd` now points to the parent of the sched group being degenerated. Thus, it may have more than one group, and we should iterate over them to clear the flags. > > IOW, "sg = sg->next;" is missed intentionally in the do while{} loop to show that > there's only 1 sched group. This looks weird to me but no persist on change the code. Not having `sg = sg->next;` is a bug, IMO. Thanks and BR, Ricardo
On Wed, Jun 21, 2023 at 06:11:59AM -0700, Ricardo Neri wrote: > On Wed, Jun 21, 2023 at 10:53:57AM +0800, Miaohe Lin wrote: > > On 2023/6/20 22:11, Peter Zijlstra wrote: > > > On Sat, Jun 17, 2023 at 04:19:26PM +0800, Miaohe Lin wrote: > > >> When sg != sd->groups, the do while loop would cause deadloop here. But > > >> that won't occur because sg is always equal to sd->groups now. Remove > > >> this unneeded do while loop. > > > > > > This Changelog makes no sense to me.. Yes, as is the do {} while loop is > > > dead code, but it *should* have read like: > > > > > > do { > > > sg->flags = 0; > > > sg = sg->next; > > > } while (sg != sd->groups); > > Yes, I agree that this is the correct solution. I take this back. I think we should do this: @@ -758,19 +758,14 @@ cpu_attach_domain(struct sched_domain *sd, struct root_domain *rd, int cpu) sd = sd->parent; destroy_sched_domain(tmp); if (sd) { - struct sched_group *sg = sd->groups; - /* * sched groups hold the flags of the child sched * domain for convenience. Clear such flags since * the child is being destroyed. */ - do { - sg->flags = 0; - } while (sg != sd->groups); + sd->groups->flags = 0; sd->child = NULL; - } } sched_domain_debug(sd, cpu); A comment from Chenyu made got me thinking that we should only clear the flags of the local group as viewed from the parent domain. This is because the domain being degenerated defines the flags of such group only. The current code does the right thing, but in a fortuitous and ugly manner. Thanks and BR, Ricardo
On 2023/6/22 2:57, Ricardo Neri wrote: > On Wed, Jun 21, 2023 at 06:11:59AM -0700, Ricardo Neri wrote: >> On Wed, Jun 21, 2023 at 10:53:57AM +0800, Miaohe Lin wrote: >>> On 2023/6/20 22:11, Peter Zijlstra wrote: >>>> On Sat, Jun 17, 2023 at 04:19:26PM +0800, Miaohe Lin wrote: >>>>> When sg != sd->groups, the do while loop would cause deadloop here. But >>>>> that won't occur because sg is always equal to sd->groups now. Remove >>>>> this unneeded do while loop. >>>> >>>> This Changelog makes no sense to me.. Yes, as is the do {} while loop is >>>> dead code, but it *should* have read like: >>>> >>>> do { >>>> sg->flags = 0; >>>> sg = sg->next; >>>> } while (sg != sd->groups); >> >> Yes, I agree that this is the correct solution. > > I take this back. I think we should do this: > > @@ -758,19 +758,14 @@ cpu_attach_domain(struct sched_domain *sd, struct root_domain *rd, int cpu) > sd = sd->parent; > destroy_sched_domain(tmp); > if (sd) { > - struct sched_group *sg = sd->groups; > - > /* > * sched groups hold the flags of the child sched > * domain for convenience. Clear such flags since > * the child is being destroyed. > */ > - do { > - sg->flags = 0; > - } while (sg != sd->groups); > + sd->groups->flags = 0; > > sd->child = NULL; > - } > } > > sched_domain_debug(sd, cpu); > > A comment from Chenyu made got me thinking that we should only clear the > flags of the local group as viewed from the parent domain. This is because > the domain being degenerated defines the flags of such group only. This looks better to my patch. Thanks.
On Sun, Jun 25, 2023 at 09:59:36AM +0800, Miaohe Lin wrote: > On 2023/6/22 2:57, Ricardo Neri wrote: > > On Wed, Jun 21, 2023 at 06:11:59AM -0700, Ricardo Neri wrote: > >> On Wed, Jun 21, 2023 at 10:53:57AM +0800, Miaohe Lin wrote: > >>> On 2023/6/20 22:11, Peter Zijlstra wrote: > >>>> On Sat, Jun 17, 2023 at 04:19:26PM +0800, Miaohe Lin wrote: > >>>>> When sg != sd->groups, the do while loop would cause deadloop here. But > >>>>> that won't occur because sg is always equal to sd->groups now. Remove > >>>>> this unneeded do while loop. > >>>> > >>>> This Changelog makes no sense to me.. Yes, as is the do {} while loop is > >>>> dead code, but it *should* have read like: > >>>> > >>>> do { > >>>> sg->flags = 0; > >>>> sg = sg->next; > >>>> } while (sg != sd->groups); > >> > >> Yes, I agree that this is the correct solution. > > > > I take this back. I think we should do this: > > > > @@ -758,19 +758,14 @@ cpu_attach_domain(struct sched_domain *sd, struct root_domain *rd, int cpu) > > sd = sd->parent; > > destroy_sched_domain(tmp); > > if (sd) { > > - struct sched_group *sg = sd->groups; > > - > > /* > > * sched groups hold the flags of the child sched > > * domain for convenience. Clear such flags since > > * the child is being destroyed. > > */ > > - do { > > - sg->flags = 0; > > - } while (sg != sd->groups); > > + sd->groups->flags = 0; > > > > sd->child = NULL; > > - } > > } > > > > sched_domain_debug(sd, cpu); > > > > A comment from Chenyu made got me thinking that we should only clear the > > flags of the local group as viewed from the parent domain. This is because > > the domain being degenerated defines the flags of such group only. > > This looks better to my patch. Thanks. Are you planning on posting a v2? Maybe I missed it. >
diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c index ca4472281c28..9010c93c3fdb 100644 --- a/kernel/sched/topology.c +++ b/kernel/sched/topology.c @@ -750,10 +750,7 @@ cpu_attach_domain(struct sched_domain *sd, struct root_domain *rd, int cpu) * domain for convenience. Clear such flags since * the child is being destroyed. */ - do { - sg->flags = 0; - } while (sg != sd->groups); - + sg->flags = 0; sd->child = NULL; } }