Re: [PATCH v2 5/7] x86/sched: Remove SD_ASYM_PACKING from the "SMT" domain

[Ricardo Neri]

On Thu, Dec 15, 2022 at 04:48:14PM +0000, Valentin Schneider wrote:
> On 14/12/22 08:59, Ricardo Neri wrote:
> > On Thu, Dec 08, 2022 at 04:03:04PM +0000, Ionela Voinescu wrote:
> >> Based on:
> >>
> >> kernel/sched/topology.c:
> >> sd = highest_flag_domain(cpu, SD_ASYM_PACKING);
> >> rcu_assign_pointer(per_cpu(sd_asym_packing, cpu), sd);
> >>
> >> and described at:
> >>
> >> include/linux/sched/sd_flags.h:
> >> /*
> >>  * Place busy tasks earlier in the domain
> >>  *
> >>  * SHARED_CHILD: Usually set on the SMT level. Technically could be set further
> >>  *               up, but currently assumed to be set from the base domain
> >>  *               upwards (see update_top_cache_domain()).
> >>  * NEEDS_GROUPS: Load balancing flag.
> >>  */
> >> SD_FLAG(SD_ASYM_PACKING, SDF_SHARED_CHILD | SDF_NEEDS_GROUPS)
> >>
> >> doesn't your change result in sd_asym_packing being NULL?
> >
> > Yes. This is a good catch. Thanks!
> >
> 
> Nice to see those being useful :-) FYI if you run your kernel with
> CONFIG_SCHED_DEBUG=y and sched_debug on the cmdline, you should get a
> warning at boot time from the topology debug code checking assertions
> against those flags.

Thanks! I missed this warning. Indeed, it is there.
> 
> >>
> >> The SD_ASYM_PACKING flag requires all children of a domain to have it set
> >> as well. So having SMT not setting the flag, while CLUSTER and MC having
> >> set the flag would result in a broken topology, right?
> >
> > I'd say that highest_flag_domain(..., flag) requires all children to have
> > `flag`, but clearly the comment you quote allows for SD_ASYM_PACKING to
> > be located in upper domains.
> >
> > Perhaps this can be fixed with a variant of highest_flag_domain() that do
> > not require all children to have the flag?
> >
> 
> So I gave that flag SDF_SHARED_CHILD because its cached SD pointer was set
> up using highest_flag_domain(). Looking for the highest level where it is
> set matches how it is used in nohz_balancer_kick(), so you might want a new
> helper.

Right. I was thinking on a highest_flag_domain_weak() or a changing
highest_flag_domain(..., bool shared_child).

> 
> With that said, so far all but one flag (SD_PREFER_SIBLING, and that's
> because of big.LITTLE woes) follow the SDF_SHARED_{CHILD, PARENT} pattern,
> if SD_ASYM_PACKING no longer does then we need to think whether we're
> trying to make it do funky things.

My thesis is that x86 does not need the SD_ASYM_PACKING flag at the SMT
level because all SMT siblings are identical. There are cores of higher
priority at the "MC" level (maybe in the future at the "CLS" level).

Power7 is fine because it only uses SD_ASYM_PACKING at the SMT level.

> I need to look at the rest of your
> series to get an idea, that unfortunately won't be today but it's now in my
> todolist.

Thank you!

BR,
Ricardo