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

From: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
To: Valentin Schneider <vschneid@redhat.com>
Cc: Ionela Voinescu <ionela.voinescu@arm.com>,
	"Peter Zijlstra (Intel)" <peterz@infradead.org>,
	Juri Lelli <juri.lelli@redhat.com>,
	Vincent Guittot <vincent.guittot@linaro.org>,
	Ricardo Neri <ricardo.neri@intel.com>,
	"Ravi V. Shankar" <ravi.v.shankar@intel.com>,
	Ben Segall <bsegall@google.com>,
	Daniel Bristot de Oliveira <bristot@redhat.com>,
	Dietmar Eggemann <dietmar.eggemann@arm.com>,
	Len Brown <len.brown@intel.com>, Mel Gorman <mgorman@suse.de>,
	"Rafael J. Wysocki" <rafael.j.wysocki@intel.com>,
	Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>,
	Steven Rostedt <rostedt@goodmis.org>,
	Tim Chen <tim.c.chen@linux.intel.com>,
	x86@kernel.org, linux-kernel@vger.kernel.org,
	"Tim C . Chen" <tim.c.chen@intel.com>
Subject: Re: [PATCH v2 5/7] x86/sched: Remove SD_ASYM_PACKING from the "SMT" domain
Date: Thu, 12 Jan 2023 17:31:29 -0800	[thread overview]
Message-ID: <20230113013129.GA9379@ranerica-svr.sc.intel.com> (raw)
In-Reply-To: <xhsmh1qo2dwrk.mognet@vschneid.remote.csb>

On Tue, Jan 10, 2023 at 07:17:51PM +0000, Valentin Schneider wrote:
> On 29/12/22 11:02, Ricardo Neri wrote:
> > On Thu, Dec 22, 2022 at 04:56:51PM +0000, Valentin Schneider wrote:
> >> diff --git a/include/linux/sched/sd_flags.h b/include/linux/sched/sd_flags.h
> >> index 57bde66d95f7a..8dc16942135b4 100644
> >> --- a/include/linux/sched/sd_flags.h
> >> +++ b/include/linux/sched/sd_flags.h
> >> @@ -132,12 +132,12 @@ SD_FLAG(SD_SERIALIZE, SDF_SHARED_PARENT | SDF_NEEDS_GROUPS)
> >>  /*
> >>   * 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()).
> >> + * SHARED_PARENT: Usually set on the SMT level. Can be set further up if all
> >> + *                siblings of an SMT core are identical, but SMT cores themselves
> >> + *                have different priorites.
> >>   * NEEDS_GROUPS: Load balancing flag.
> >>   */
> >> -SD_FLAG(SD_ASYM_PACKING, SDF_SHARED_CHILD | SDF_NEEDS_GROUPS)
> >> +SD_FLAG(SD_ASYM_PACKING, SDF_SHARED_PARENT | SDF_NEEDS_GROUPS)
> >
> > But this would not work for Power7. It only has SD_ASYM_PACKING in the SMT
> > sched domain. Must it have either of these flags?
> >
> 
> It's not mandatory, but making sure SD flags conform to either of them
> means the topology debugging infra can help spot misshapen topologies...
> 
> > In Power7 SMT siblings have the different priority but, IIUC, physical
> > cores are identical.
> >
> 
> 
> ...But you're right, this doesn't work with Power7 as it would need
> SD_ASYM_PACKING all the way up the topology to conform with
> SDF_SHARED_PARENT, which clearly doesn't work with how Power7 uses
> asym_packing.
> 
> > It seems to me that asym_packing is specific to a domain.
> >
> 
> For Power7 it is, since the asymmetry is only between siblings of a given
> core. For other systems where the asymmetry is between cores, that could
> theoretically affect several levels. Consider:
> 
>   DIE [                      ]
>   MC  [          ][          ]
>   SMT [    ][    ][    ][    ]
>   CPU  0  1  2  3  4  5  6  7
>   prio 3  3  2  2  1  1  0  0
> 
> As done in your patch, here asym_packing doesn't make sense for SMT, but it
> does for MC and DIE.
> 
> Anywho, I think what this means if we should drop the SDF_SHARED_* metaflag
> for SD_ASYM_PACKING, unless we can think of a nice way to programmatically
> describe how SD_ASYM_PACKING should be set.

Perhaps it can be done by adding an extra check for sg::asym_prefer_cpu. In
the example you give, DIE does not need SD_ASYM_PACKING if asym_prefer_cpu 
all the MC sched groups of have the same priority. This would satisfy both
Power7 and x86.

This assumes that priorities are available when checking the sanity of the
topology.

Thanks and BR,
Ricardo