Re: [PATCH 05/11] x86 topology: export die_siblings

From: Len Brown <lenb@kernel.org>
To: Brice Goglin <Brice.Goglin@inria.fr>
Cc: X86 ML <x86@kernel.org>,
	linux-kernel@vger.kernel.org, Len Brown <len.brown@intel.com>,
	linux-doc@vger.kernel.org
Subject: Re: [PATCH 05/11] x86 topology: export die_siblings
Date: Thu, 21 Feb 2019 02:41:30 -0500	[thread overview]
Message-ID: <CAJvTdKm3gqY9G6nOVq_a3r0QX4ECcew7Z3cdr1OxDBGRVgUpkg@mail.gmail.com> (raw)
In-Reply-To: <ceb7549e-6ccb-c6c7-241c-36c6e5c30770@inria.fr>

Hi Brice,
Thank you for your suggestions!

> Patches #4 and #5 are changing the meaning the core_siblings (in the
> past, it always returned all threads in the entire package). All
> existing user-space tools will see each die as a separate package until
> they are updated to read die_siblings too. It only matters for multi-die
> CPUs when running a recent kernel with an old userspace tool, but it may
> still be consider as a sysfs ABI change.

I agree.

Exhibit 1 is the "lscpu" program.

> Worse, things will break again if you ever add tile_siblings for
> CPUID.1f "Tiles". User-space will suddenly see 2 dies of 2 cores instead
> 1 die of 2 tiles of 2 cores.

Agreed, the existing naming scheme is not resilient to future additions.

> I understand that this isn't easy to fix. But I want to make sure people
> are aware of the meaning of this change.

Here is my list of applications that care about the new CPUID leaf
and the concepts of packages and die:

cpuid
lscpu
x86_energy_perf_policy
turbostat

> The proper way to avoid this is to stop having file foo_siblings refer
> to "the container of foo" instead of "foo itself" (because that
> container changes when you add intermediate levels). Rename sysfs files
> like below, and you don't get any breakage anymore when adding
> intermediate levels:
>
> thread_siblings -> core_threads (can we do sysfs alias or symlink to
> keep the old name?)
>
> core_siblings -> die_threads
>
> die_siblings -> package_threads (needs an alias too)
>
> The documentation would also be much easier to read since "die_threads"
> is obviously "human-readable list of cpuX's hardware threads within the
> same die_id". And no need to modify the doc anymore when adding levels :)

I like your idea!

Hm, I think i'd skip creating "die_siblings", as it adds to the
fragile legacy naming scheme
that we want to deprecate.

And although it is ill-defined and has a mis-leading name, I now think
it would be
better to leave "core_siblings" as defined -- a legacy synonym for
"package_threads".  Deprecate it, but keep its original definition
until it is removed.

Updated applications would use:

core_threads
die_threads
package_threads

and they'll be future proof if/when we add any new levels.

the legacy thread_siblings and core_siblings will stick around as aliases:

core_threads (thread_siblings)
die_threads
package_threads (core_siblings)

thanks!
Len Brown, Intel Open Source Technology Center