Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Valentin Schneider <vschneid@redhat.com>
To:     Ricardo Neri <ricardo.neri-calderon@linux.intel.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
In-Reply-To: <20221229190249.GA14462@ranerica-svr.sc.intel.com>
References: <20221122203532.15013-1-ricardo.neri-calderon@linux.intel.com>
 <20221122203532.15013-6-ricardo.neri-calderon@linux.intel.com>
 <Y5IKuJTjE6Pjrw9I@arm.com>
 <20221214165900.GA972@ranerica-svr.sc.intel.com>
 <xhsmhwn6s62b5.mognet@vschneid.remote.csb>
 <20221220004238.GB23844@ranerica-svr.sc.intel.com>
 <xhsmhr0wre5rg.mognet@vschneid.remote.csb>
 <20221229190249.GA14462@ranerica-svr.sc.intel.com>
Date:   Tue, 10 Jan 2023 19:17:51 +0000
Message-ID: <xhsmh1qo2dwrk.mognet@vschneid.remote.csb>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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.

> Thanks and BR,
> Ricardo