Re: [RFC 0/2] Add RISC-V cpu topology

From: Nick Kossifidis
Date: Thu Nov 08 2018 - 22:58:28 EST

ÎÏÎÏ 2018-11-08 17:54, Mark Rutland ÎÎÏÎÏÎ:
On Thu, Nov 08, 2018 at 03:45:36PM +0200, Nick Kossifidis wrote:
ÎÏÎÏ 2018-11-07 14:06, Mark Rutland ÎÎÏÎÏÎ:
> On Wed, Nov 07, 2018 at 04:31:34AM +0200, Nick Kossifidis wrote:
> > Mark and Sundeep thanks a lot for your feedback, I guess you convinced
> > me that having a device tree binding for the scheduler is not a
> > correct approach. It's not a device after all and I agree that the
> > device tree shouldn't become an OS configuration file.
>
> Good to hear.
>
> > Regarding multiple levels of shared resources my point is that since
> > cpu-map doesn't contain any information of what is shared among the
> > cluster/core members it's not easy to do any further translation. Last
> > time I checked the arm code that uses cpu-map, it only defines one
> > domain for SMT, one for MC and then everything else is ignored. No
> > matter how many clusters have been defined, anything above the core
> > level is the same (and then I guess you started talking about adding
> > "packages" on the representation side).
>
> While cpu-map doesn't contain that information today, we can *add* that
> information to the cpu-map binding if necessary.
>
> > The reason I proposed to have a binding for the scheduler directly is
> > not only because it's simpler and closer to what really happens in the
> > code, it also makes more sense to me than the combination of cpu-map
> > with all the related mappings e.g. for numa or caches or power
> > domains etc.
> >
> > However you are right we could definitely augment cpu-map to include
> > support for what I'm saying and clean things up, and since you are
> > open about improving it here is a proposal that I hope you find
> > interesting:
> >
> > At first let's get rid of the <thread> nodes, they don't make sense:
> >
> > thread0 {
> > cpu = <&CPU0>;
> > };
> >
> > A thread node can't have more than one cpu entry and any properties
> > should be on the cpu node itself, so it doesn't / can't add any
> > more information. We could just have an array of cpu nodes on the
> > <core> node, it's much cleaner this way.
> >
> > core0 {
> > members = <&CPU0>, <&CPU1>;
> > };
>
> Hold on. Rather than reinventing things from first principles, can we
> please discuss what you want to *achieve*, i.e. what information you
> need?
>
> Having a node is not a significant cost, and there are reasons we may
> want thread nodes. For example, it means that we can always refer to any
> level of topology with a phandle, and we might want to describe
> thread-affine devices in future.

You can use the phandle of the cpu node, the thread node doesn't add
anything more than complexity to the representation.

That adds complexity elsewhere, because the phandle of a CPU node is not
a child of the cpu-map node, and you can't simply walk up the tree
hierarchy to find parent nodes.

I see no reason to change this part of the binding. Given it's already
out there, with existing parsing code, changing it only serves to add
complexity to any common code which has to parse this.

Can we please drop the idea of dropping the thread node?

> There are a tonne of existing bindings that are ugly, but re-inventing
> them for taste reasons alone is more costly to the ecosystem than simply
> using the existing bindings. We avoid re-inventing bindings unless there
> is a functional problem e.g. cases which they cannot possibly describe.

We are talking about using something for RISC-V and possibly common across
different archs and, I don't see why we should keep the ugliness of a
binding spec plus in this case the <trhead> node can't possibly describe
anything else than a cpu=<node> alias, it's redundant.

While it may be ugly, removing it only serves to add complexity for
common parsing code, and removes flexibility that we may want in future.
Its presence does not cause any technical problem.

For better or worse we're all sharing the same codebase here. The common
case matters, as does the complexity of the codebase as a whole.

I realise it can be frustrating to have to use something you feel is
sub-optimal, but putting up with a few nodes which you personally feel
are redundant is of much lower cost to the ecosystem than having
incompatible bindings where we could have one. If you put up with that,
you can focus your efforts on more worthwhile technical exercises.


The only reason I mentioned the issue with the <thread> node is because
you said that you are open for improving cpu-map. I don't think it's
such a big deal and even if we decide against it, it's not a big deal
to be backwards compatible with what's already there. I fully understand
what you say about the impact on the ecosystem and agree with you.

> > Then let's allow the cluster and core nodes to accept attributes
> > that are
> > common for the cpus they contain. Right now this is considered
> > invalid.
> >
> > For power domains we have a generic binding described on
> > Documentation/devicetree/bindings/power/power_domain.txt
> > which basically says that we need to put power-domains = <power domain
> > specifiers>
> > attribute on each of the cpu nodes.
>
> FWIW, given this is arguably topological, I'm not personally averse to
> describing this in the cpu-map, if that actually gains us more than the
> complexity require to support it.
>
> Given we don't do this for device power domains, I suspect that it's
> simpler to stick with the existing binding.
>
> > The same happens with the capacity binding specified for arm on
> > Documentation/devicetree/bindings/arm/cpu-capacity.txt
> > which says we should add the capacity-dmips-mhz on each of the cpu
> > nodes.
>
> The cpu-map was intended to expose topological dtails, and this isn't
> really a topological property. For example, Arm DynamIQ systems can have
> heterogeneous CPUs within clusters.
>
> I do not think it's worth moving this, tbh.
>
> > The same also happens with the generic numa binding on
> > Documentation/devicetree/bindings/numa.txt
> > which says we should add the nuna-node-id on each of the cpu nodes.
>
> Is there a strong gain from moving this?
>
> [...]

Right now with the device tree spec and the above bindings we can
use the information from cpu nodes to infer the cache topology,
the memory topology, the power domain topology etc.

We have of_find_next_cache_node and of_find_last_cache_level for example
that use "next-level-cache" and are used on powerpc (where this spec comes
from), that we can further build on top of them (since this is now part
of the device tree spec anyway), to e.g. populate the levels of cache,
the levels of shared cache and finally create cpu masks for the different
cache sharing levels.

The cpu-map binding does not describe cache topology. I never suggested
that it did.


Sorry for the misunderstanding Mark !

On ARM the CPU topology is used for configuring the scheduling domain topology
(arch/arm/kernel/topology.c) and blindly sets the SD_SHARE_PKG_RESOURCES
and SD_SHARE_POWERDOMAIN flags for a core without taking into account the
cache hierarchy (hence validating that SD_SHARE_PKG_RESOURCES is correct)
or the power domains (but ok, I doubt it's possible to have two threads on
the same core that are powered independently). It also supports only two
scheduling domains, one for SMT and another one for MC (hence my comments
on multiple levels of shared resources not being supported).

Since according to the docs cpu-map is a binding common between ARM and ARM64
for describing the CPU topology, I assumed it was a part of that process.

Turns out it's only used on ARM64, where set_sched_topology() is not used for
configuring the scheduling domain topology (there is a commend there that
implies that you pass on the clusters to the scheduler that also led me to
believe the issue is also present there). So my assumption was wrong and
cpu-map has nothing to do with configuring the scheduler and the issues
I mentioned above.

This is standards-compliant, arch-independent (they are on of/base.c), and
it provides a concrete hint on CPU topology rather grouping harts to classes
like "core", "package", "socket", "cluster" etc that don't mean anything
specific and we assume to map to specific levels of cache sharing.

Please note that I have never suggested "package", and I'm not sure what
that's in response to.


https://lkml.org/lkml/2018/11/7/918

[...]

In a sense it goes against your rule of "re-inventing them for taste
reasons alone is more costly to the ecosystem than simply using the
existing bindings", I fail to see anything else than "taste reasons"
when it comes to cpu-map, since there are existing bindings for
inferring the CPU topology already, they are just not that convenient.

If you believe that's the case, then you can perfectly use the existing
cache and NUMA bindings, and not describe the cpu topology at all, no
need to change cpu-map or come up with a new binding.


The problem is that right now nobody is using the generic bindings
for configuring the scheduler domain topology. Only the numa bindings
are used as expected on ARM64. Since cpu-map is only there for representing
the cpu topology/layout, allowing them to exist there is obviously not the
right approach.

I'm proposing to add the option (not enforce) of adding those
attributes that are meant to be used on cpu nodes, on the various
groups/classes of the cpu-map, this way cpu-map will provide something
more meaningful and at least improve the representation side of
things. On the implementation side it might save us the burden of
infering the topology from parsing all cpu nodes each time. It's also
backwards compatible with what you already have, the only thing that's
not backwards compatible is the removal of the <thread> node, which I
don't think is such a big deal to fix.

Sorry, but as I have stated repeatedly, this complicates the common code
for what you admit is a matter of taste. I would rather maintain the
simplicity of this binding and existing parsing code, and not
potentially break other parsers, if the only cost is a few nodes which
you personally consider to be redundant.


[...]

So we agree, the "core" doesn't say anything useful regarding the
topology, why keep using this distinction on a binding for RISC-V and
possibly other archs (since we are also talking on an arch-independent
approach) ?

We keep it because the binding has already been finalised and is use in
practice. The existing binding and parsing code is *sufficient* for your
needs. Personal taste and minor savings in the number of nodes in a DT
are not compelling reasons to change this when the cost is complexity
that we have to maintain *forever*.


Totally fine with that, as I mentioned above I pointed these out since I
consider it an improvement. Forgive me if I gave you the impression that
was a deal-breaker or something.

Regards,
Nick