Re: [PATCH RFC 0/2] sched: Introduce idle SMT priority for asymmetric capacity systems

[Mete Durlu]

[..snip..]

> > Considerations and Open Questions
> > ===========================================================================
> > The main goal for this series is adding a simple mechanism for
> > architectures to switch between idle core and idle SMT priority while
> > keeping the introduced footprint as small as possible. But there are
> > some ideas and questions to consider;
> >
> > 1. Should arch_needs_idle_smt_prio() hook be removed?
>
> Hi Mete,
>
> One assumption in the series should be made explicit, and I think
> it answers your question 1.
>
> The idea only works if two CPUs that the guest sees as SMT siblings
> really run on the same physical core. "An idle sibling of a busy
> high-capacity core" is cheaper than a fully idle low-capacity core
> only because it uses a physical core the partition already has. That
> holds on an LPAR, where hypervisor dispatches whole cores. 

True, the whole idea is based on this assumption and it is not
mentioned. I'll add this.

> It does not
> hold in general for a guest whose hypervisor schedules vCPUs one at a
> time, such as a typical KVM guest with "threads=2". There the guest's
> sibling masks are just a description, and packing onto "siblings"
> gives no benefit.
Right, if the whole topology is virtual, the vCPUs are just
separate independent entities from each other. I am not sure
if there is a way to configure a KVM guest to reflect this
and make the KVM hypervisor to schedule whole guest cores.
Regardless, I agree that this has to be explicitly stated
somewhere, probably in Kconfig's help text.

> Nothing in the series checks this, and it is not documented:
>
> - In patch 1, the default arch_needs_idle_smt_prio() returns true,
>    and the Kconfig help does not mention that siblings must share a
>    physical core. Any arch that selects
>    ARCH_SUPPORTS_SCHED_IDLE_SMT_PRIO takes this on without being told.
>
> - In patch 2, the hook checks only the topology mode:
>
> > +bool arch_needs_idle_smt_prio(void)
> > +{
> > +     if (topology_mode != TOPOLOGY_MODE_HW)
> > +             return false;
> > +     return true;
> > +}
>
> Today s390 seems to be safe because sibling masks are
> only built in HW mode.  It is the only place where this
> precondition can be enforced.
>
> So I would keep the hook. It is the only place where this
> precondition can be enforced. Removing it and tying the feature to
> "Kconfig + asymmetric capacities" would drop the check entirely.
> Concretely:
>
> 1) Document the contract in the Kconfig help and above the hook,
>     for example: "Only select this if CPUs reported as SMT siblings
>     are always dispatched on the same physical core."

Will do!

> 2) Make the s390 hook test the conditions from the cover letter,
>     something like:
>
>          return machine_is_lpar() &&
>                 topology_mode == TOPOLOGY_MODE_HW &&
>                 smp_cpu_mtid;

Makes sense, as this won't help on systems with SMT disabled.

Thanks for the comments!
-Mete