Re: [PATCH] sched/topology: remove unneeded do while loop in cpu_attach_domain()

[Ricardo Neri]

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