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:   Wed, 21 Jun 2023 11:57:49 -0700
From:   Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
To:     Miaohe Lin <linmiaohe@huawei.com>
Cc:     Peter Zijlstra <peterz@infradead.org>, mingo@redhat.com,
        juri.lelli@redhat.com, vincent.guittot@linaro.org,
        dietmar.eggemann@arm.com, rostedt@goodmis.org, bsegall@google.com,
        mgorman@suse.de, bristot@redhat.com, vschneid@redhat.com,
        linux-kernel@vger.kernel.org, yu.c.chen@intel.com,
        tim.c.chen@intel.com
Subject: Re: [PATCH] sched/topology: remove unneeded do while loop in
 cpu_attach_domain()
Message-ID: <20230621185749.GA24063@ranerica-svr.sc.intel.com>
References: <20230617081926.2035113-1-linmiaohe@huawei.com>
 <20230620141120.GA2016469@hirez.programming.kicks-ass.net>
 <53a9ec35-8332-755c-40ed-54315ffb0d78@huawei.com>
 <20230621131159.GA23663@ranerica-svr.sc.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230621131159.GA23663@ranerica-svr.sc.intel.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

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