sched/topology: remove unneeded do while loop in cpu_attach_domain()

  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(-)
  

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.
>