RE: [PATCH] cpu-topology: warn if NUMA configurations conflicts with lower layer

[Zengtao (B)]

Hi Sudeep:

Thanks for your reply.

> -----Original Message-----
> From: Sudeep Holla [mailto:sudeep.holla@xxxxxxx]
> Sent: Wednesday, January 01, 2020 12:41 AM
> To: Zengtao (B)
> Cc: Linuxarm; Greg Kroah-Hartman; Rafael J. Wysocki;
> linux-kernel@xxxxxxxxxxxxxxx; Sudeep Holla; Morten Rasmussen
> Subject: Re: [PATCH] cpu-topology: warn if NUMA configurations conflicts
> with lower layer
> 
> On Mon, Dec 23, 2019 at 04:16:19PM +0800, z00214469 wrote:
> > As we know, from sched domain's perspective, the DIE layer should be
> > larger than or at least equal to the MC layer, and in some cases, MC
> > is defined by the arch specified hardware, MPIDR for example, but
> NUMA
> > can be defined by users,
> 
> Who are the users you are referring above ?
For example, when I use QEMU to start a guest linux, I can define the
 NUMA topology of the guest linux whatever i want.
> > with the following system configrations:
> 
> Do you mean ACPI tables or DT or some firmware tables ?
> 
> > *************************************
> > NUMA:      	 0-2,  3-7
> 
> Is the above simply wrong with respect to hardware and it actually match
> core_siblings ?
> 
Actually, we can't simply say this is wrong, i just want to show an example.
 And this example also can be:
 NUMA:  0~23,  24~47
 core_siblings:   0-15,  16-31, 32~47

> > core_siblings:   0-3,  4-7
> > *************************************
> > Per the current code, for core 3, its MC cpu map fallbacks to 3~7(its
> > core_sibings is 0~3 while its numa node map is 3~7).
> >
> > For the sched MC, when we are build sched groups:
> > step1. core3 's sched groups chain is built like this: 3->4->5->6->7->3
> > step2. core4's sched groups chain is built like this: 4->5->6->7->4
> > so after step2, core3's sched groups for MC level is overlapped, more
> > importantly, it will fall to dead loop if while(sg != sg->groups)
> >
> > Obviously, the NUMA node with cpu 3-7 conflict with the MC level cpu
> > map, but unfortunately, there is no way even detect such cases.
> >
> 
> Again, is cpu 3-7 actually in a NUMA node or is it 4-7 ?
> 
> > In this patch, prompt a warning message to help with the above cases.
> >
> > Signed-off-by: Zeng Tao <prime.zeng@xxxxxxxxxxxxx>
> > ---
> >  drivers/base/arch_topology.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> > index 1eb81f11..5fe44b3 100644
> > --- a/drivers/base/arch_topology.c
> > +++ b/drivers/base/arch_topology.c
> > @@ -439,10 +439,18 @@ const struct cpumask
> *cpu_coregroup_mask(int cpu)
> >  	if (cpumask_subset(&cpu_topology[cpu].core_sibling, core_mask)) {
> >  		/* not numa in package, lets use the package siblings */
> >  		core_mask = &cpu_topology[cpu].core_sibling;
> > -	}
> > +	} else
> > +		pr_warn_once("Warning: suspicous broken topology: cpu:[%d]'s
> core_sibling:[%*pbl] not a subset of numa node:[%*pbl]\n",
> > +			cpu, cpumask_pr_args(&cpu_topology[cpu].core_sibling),
> > +			cpumask_pr_args(core_mask));
> > +
> 
> Won't this print warning on all systems that don't have numa within a
> package ? What are you trying to achieve here ?

Since in my case, when this corner case happens, the linux kernel just fall into
dead loop with no prompt, here this is a helping message will help a lot.

> 
> >  	if (cpu_topology[cpu].llc_id != -1) {
> >  		if (cpumask_subset(&cpu_topology[cpu].llc_sibling, core_mask))
> >  			core_mask = &cpu_topology[cpu].llc_sibling;
> > +		else
> > +			pr_warn_once("Warning: suspicous broken topology:
> cpu:[%d]'s llc_sibling:[%*pbl] not a subset of numa node:[%*pbl]\n",
> > +				cpu,
> cpumask_pr_args(&cpu_topology[cpu].llc_sibling),
> > +				cpumask_pr_args(core_mask));
> >  	}
> >
> 
> This will trigger warning on all systems that lack cacheinfo topology.
> I don't understand the intent of this patch at all. Can you explain
> all the steps you follow and the issue you face ?

Can you show me an example, what I really want to warn is the case that 
NUMA topology conflicts with lower level. 

> 
> --
> Regards,
> Sudeep