| Message ID | 20140225165411.GA3883@red-moon (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Feb 25, 2014 at 04:54:11PM +0000, Lorenzo Pieralisi wrote: > I have been clobbering the code with some random dts configs, running on > top of the patch attached (to make code compliant with default if DT > info is missing for some CPUs). Can you give it a go please and run your > tests against it (I have just put it together) ? It should simplify things, > thoughts ? > AFAIK it should be correct, at least the info required by the scheduler > is reset to default (include/linux/topology.h) if DT nodes are > misconfigured. It applies on top of your series. It seems to work fine for me, I'll resend with it squashed in though one thing below which concerns me a bit. Can I suggest that given the sort of thing we're looking at here it might be easier to apply the code as-is and then do any further work incrementally? > If the device tree does not contain topology information for a CPU, > kernel code should set-up topology information for the CPU according > to the default values defined in Documentation/cputopology.txt. > + pr_warn("CPU%u: No topology information configured\n", cpuid); > + cpuid_topo->core_id = 0; I know this is what the document says and the scheduler copes but it does make me a bit nervous if we end up with only some CPUs in this undefined state. I can't point to any practical problems and it's going to be the scheduler's fault if it does but I wouldn't be surprised if there were issues in cases where there is partial topology information (as opposed to either full topology information or none). Having said that like I keep saying it seems most sensible to fall back to MPIDR if we don't have any better idea; it's guaranteed to be available, may well be correct and helps systems that did a good job with their hardware.
On Wed, Feb 26, 2014 at 12:50:52AM +0000, Mark Brown wrote: > On Tue, Feb 25, 2014 at 04:54:11PM +0000, Lorenzo Pieralisi wrote: > Can I suggest that given the sort of thing we're looking at here it > might be easier to apply the code as-is and then do any further work > incrementally? I think we have enough information to make code complete now, at least for the topology code, I prefer to merge what's considered to be final. > > If the device tree does not contain topology information for a CPU, > > kernel code should set-up topology information for the CPU according > > to the default values defined in Documentation/cputopology.txt. > > > + pr_warn("CPU%u: No topology information configured\n", cpuid); > > + cpuid_topo->core_id = 0; > > I know this is what the document says and the scheduler copes but it > does make me a bit nervous if we end up with only some CPUs in this > undefined state. I can't point to any practical problems and it's > going to be the scheduler's fault if it does but I wouldn't be surprised > if there were issues in cases where there is partial topology > information (as opposed to either full topology information or none). > > Having said that like I keep saying it seems most sensible to fall back > to MPIDR if we don't have any better idea; it's guaranteed to be > available, may well be correct and helps systems that did a good job > with their hardware. You have a point. Unless someone feels strongly against this, I would suggest falling back to MPIDR_EL1 if there is missing or wrong information in DT. To do that, you need to scrap the structures initialized from DT altogether if there is missing or wrong DT information, it is either DT for all CPUs or MPIDR_EL1 for all CPUs, we can't mix the approaches (and while at it I'd rather parse the HW MPIDR_EL1 so that you get get the MT bit from it, which is not present in cpu_logical_map). Let's sum it up: - if there is a cpu-map, it takes precedence. If there are missing bits of info in the cpu-map we WARN_ON (since this is a firmware bug, cpu-map overrides MPIDR_EL1 registers info, if the information in cpu-map is wrong or missing we should flag it up and that deserves a WARN_ON) - if cpu-map is missing, we fall back to MPIDR_EL1 and print a message just describing that. I noticed you will have to add some macros to read MPIDR_EL1 bits in the process. Thanks, Lorenzo
On Wed, Feb 26, 2014 at 12:32:08PM +0000, Lorenzo Pieralisi wrote: > On Wed, Feb 26, 2014 at 12:50:52AM +0000, Mark Brown wrote: > > Can I suggest that given the sort of thing we're looking at here it > > might be easier to apply the code as-is and then do any further work > > incrementally? > I think we have enough information to make code complete now, at least > for the topology code, I prefer to merge what's considered to be final. Hrm, in that case perhaps the first patch is OK to go? The changes still being discussed are to the DT binding handling and the addition of a new parsing mechanism (for MPIDR, which I'd do as a separate patch). This would be quite helpful, tweaks to the first patch typically cause manual rebases against the DT patches which increases the stop energy. > > Having said that like I keep saying it seems most sensible to fall back > > to MPIDR if we don't have any better idea; it's guaranteed to be > > available, may well be correct and helps systems that did a good job > > with their hardware. > You have a point. Unless someone feels strongly against this, I would > suggest falling back to MPIDR_EL1 if there is missing or wrong information in > DT. Catalin seemed very concerned about any use at all of MPIDR, that's why the code was removed originally. Catalin?
On Wed, Feb 26, 2014 at 02:46:10PM +0000, Mark Brown wrote: > On Wed, Feb 26, 2014 at 12:32:08PM +0000, Lorenzo Pieralisi wrote: > > On Wed, Feb 26, 2014 at 12:50:52AM +0000, Mark Brown wrote: > > > Having said that like I keep saying it seems most sensible to fall back > > > to MPIDR if we don't have any better idea; it's guaranteed to be > > > available, may well be correct and helps systems that did a good job > > > with their hardware. > > > You have a point. Unless someone feels strongly against this, I would > > suggest falling back to MPIDR_EL1 if there is missing or wrong information in > > DT. > > Catalin seemed very concerned about any use at all of MPIDR, that's why > the code was removed originally. Catalin? My concern is that the MPIDR is just considered a unique ID. The ARMv8 relaxes the requirement so that it no longer needs to start at 0 and increase monotonically. I checked with the architecture guys here and they still expect the affinity hierarchy to be described by MPIDR but we can have holes in the range for certain levels (i.e. an affinity level may not start at 0 and may not even increase monotonically for subsequent CPUs). So we can either add a tolerant MPIDR parsing or we simply assume that the topology is _flat_ when DT doesn't provide the information.
On Wed, Feb 26, 2014 at 03:48:58PM +0000, Catalin Marinas wrote: > On Wed, Feb 26, 2014 at 02:46:10PM +0000, Mark Brown wrote: > > On Wed, Feb 26, 2014 at 12:32:08PM +0000, Lorenzo Pieralisi wrote: > > > On Wed, Feb 26, 2014 at 12:50:52AM +0000, Mark Brown wrote: > > > > Having said that like I keep saying it seems most sensible to fall back > > > > to MPIDR if we don't have any better idea; it's guaranteed to be > > > > available, may well be correct and helps systems that did a good job > > > > with their hardware. > > > > > You have a point. Unless someone feels strongly against this, I would > > > suggest falling back to MPIDR_EL1 if there is missing or wrong information in > > > DT. > > > > Catalin seemed very concerned about any use at all of MPIDR, that's why > > the code was removed originally. Catalin? > > My concern is that the MPIDR is just considered a unique ID. The ARMv8 > relaxes the requirement so that it no longer needs to start at 0 and > increase monotonically. I checked with the architecture guys here and > they still expect the affinity hierarchy to be described by MPIDR but we > can have holes in the range for certain levels (i.e. an affinity level > may not start at 0 and may not even increase monotonically for > subsequent CPUs). > > So we can either add a tolerant MPIDR parsing or we simply assume that > the topology is _flat_ when DT doesn't provide the information. I think that as far as the topology structures are concerned we can fall back to MPIDR_EL1 in case cpu-map is missing or incomplete, since even if the affinity levels contain holes or are not sequential that should be fine (but see below). After all we have cpu-map to correct things for this same reason and that will take precedence over the MPIDR_EL1 anyway. Macros below are required for the scheduler to work. The ids are arch dependent (so generic code cannot rely on them being sequential or follow any rule) and if we fall back to MPIDR_EL1 they will get initialized. #define topology_physical_package_id(cpu) (cpu_topology[cpu].socket_id) #define topology_core_id(cpu) (cpu_topology[cpu].core_id) #define topology_core_cpumask(cpu) (&cpu_topology[cpu].core_sibling) #define topology_thread_cpumask(cpu) (&cpu_topology[cpu].thread_sibling) #define mc_capable() (cpu_topology[0].socket_id != -1) #define smt_capable() (cpu_topology[0].thread_id != -1) What would be wrong, is leaving the core_sibling and thread_sibling masks empty and this CAN happen even on current arm32 code if the cluster id field (ie MPIDR[15:8] for the sake of this discussion) is different for different CPUs, and that's a bug that fortunately was never hit (it is a totally valid MPIDR layout, a bit weird though). On an arm32 4 cpus system with MPIDR layout {0x100, 0x200, 0x300, 0x400} (which is allowed) the kernel would panic (since the thread_sibling and core_sibling masks would end up empty). So I agree with Catalin, either we allow a fall-back to MPIDR_EL1, with relaxed parsing rules (but let's not forget what I wrote above), or we keep the current set but with following changes: - if s topology node for a cpu (or some cpus) is missing or wrong, the whole topology structure should be reverted to default (as in include/linux/topology.h Documentation/cputopology.txt). This solves Mark's concern about having some cpus with missing topology information. NB: My patch does not do that, it resets topology only for the misconfigured cpu. That's the best we can do. I think that an MPIDR_EL1 fall back mechanism should be put in place, as a separate patch or squashed to patch 1. I would prefer the latter, but if you want to get the infrastructure in first I think that's acceptable. Apologies for all requested changes. Thanks, Lorenzo
On Wed, Feb 26, 2014 at 03:48:58PM +0000, Catalin Marinas wrote: > My concern is that the MPIDR is just considered a unique ID. The ARMv8 > relaxes the requirement so that it no longer needs to start at 0 and > increase monotonically. I checked with the architecture guys here and > they still expect the affinity hierarchy to be described by MPIDR but we > can have holes in the range for certain levels (i.e. an affinity level > may not start at 0 and may not even increase monotonically for > subsequent CPUs). I don't think anything has a problem with holes in the number range, the only thing I can think of which imposes that requirement is the DT binding but obviously that's not relevant if we are using MPIDR. The core topology documentation explicitly says that these are physical IDs and entirely up to the architecture/platform. It'd seem odd given that we have hotplug support. What problems do you anticipate? > So we can either add a tolerant MPIDR parsing or we simply assume that > the topology is _flat_ when DT doesn't provide the information. Right, that's what the current code does but I'm not sure it's the best option.
On Wed, Feb 26, 2014 at 05:50:04PM +0000, Lorenzo Pieralisi wrote: > What would be wrong, is leaving the core_sibling and thread_sibling > masks empty and this CAN happen even on current arm32 code if the > cluster id field (ie MPIDR[15:8] for the sake of this discussion) is > different for different CPUs, and that's a bug that fortunately was > never hit (it is a totally valid MPIDR layout, a bit weird though). This is a good point, I will make sure any MPIDR code handles it (we should fix the 32 bit code as well of course, I'll try to look at that too). > - if s topology node for a cpu (or some cpus) is missing or wrong, the whole > topology structure should be reverted to default (as in > include/linux/topology.h Documentation/cputopology.txt). This solves Mark's > concern about having some cpus with missing topology information. > NB: My patch does not do that, it resets topology only for the misconfigured > cpu. I would certainly prefer this, I'll do an update (though not today, trains again). > That's the best we can do. I think that an MPIDR_EL1 fall back mechanism > should be put in place, as a separate patch or squashed to patch 1. > I would prefer the latter, but if you want to get the infrastructure in > first I think that's acceptable. I'd certainly prefer to get the code merged and work incrementally, it'd make life easier to avoid having to manage the rebases.
diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c index e85560a..22ce390 100644 --- a/arch/arm64/kernel/topology.c +++ b/arch/arm64/kernel/topology.c @@ -317,6 +317,18 @@ static void update_siblings_masks(unsigned int cpuid) struct cpu_topology *cpu_topo, *cpuid_topo = &cpu_topology[cpuid]; int cpu; + if (cpuid_topo->cluster_id == -1) { + /* + * DT does not contain topology information for this cpu + * reset it to default behaviour + */ + pr_warn("CPU%u: No topology information configured\n", cpuid); + cpuid_topo->core_id = 0; + cpumask_set_cpu(cpuid, &cpuid_topo->core_sibling); + cpumask_set_cpu(cpuid, &cpuid_topo->thread_sibling); + return; + } + /* update core and thread sibling masks */ for_each_possible_cpu(cpu) { cpu_topo = &cpu_topology[cpu]; @@ -339,14 +351,7 @@ static void update_siblings_masks(unsigned int cpuid) void store_cpu_topology(unsigned int cpuid) { - struct cpu_topology *cpuid_topo = &cpu_topology[cpuid]; - - /* Something should have picked a topology by the time we get here */ - if (cpuid_topo->core_id == -1) - pr_warn("CPU%u: No topology information configured\n", cpuid); - else - update_siblings_masks(cpuid); - + update_siblings_masks(cpuid); update_cpu_power(cpuid); } @@ -372,17 +377,4 @@ void __init init_cpu_topology(void) } parse_dt_topology(); - - /* - * Assign all remaining CPUs to a cluster so the scheduler - * doesn't get confused. - */ - for_each_possible_cpu(cpu) { - struct cpu_topology *cpu_topo = &cpu_topology[cpu]; - - if (cpu_topo->cluster_id == -1) { - cpu_topo->cluster_id = INT_MAX; - cpu_topo->core_id = cpu; - } - } }