linux-kernel - Re: [PATCH] sched/topology: remove unneeded do while loop in cpu_attach

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230621185749.GA24063@ranerica-svr.sc.intel.com>
Date:   Wed, 21 Jun 2023 11:57:49 -0700
From:   Ricardo Neri <ricardo.neri-calderon@...ux.intel.com>
To:     Miaohe Lin <linmiaohe@...wei.com>
Cc:     Peter Zijlstra <peterz@...radead.org>, mingo@...hat.com,
        juri.lelli@...hat.com, vincent.guittot@...aro.org,
        dietmar.eggemann@....com, rostedt@...dmis.org, bsegall@...gle.com,
        mgorman@...e.de, bristot@...hat.com, vschneid@...hat.com,
        linux-kernel@...r.kernel.org, yu.c.chen@...el.com,
        tim.c.chen@...el.com
Subject: Re: [PATCH] sched/topology: remove unneeded do while loop in
 cpu_attach_domain()

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