Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1675538 > unrolled thread

Re: [PATCH 1/2] x86/CPU/AMD: Present package as die instead of socket

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/2] x86/CPU/AMD: Present package as die instead of socket Borislav Petkov <bp@alien8.de> - 2017-06-27 12:50 +0200
    Re: [PATCH 1/2] x86/CPU/AMD: Present package as die instead of socket Borislav Petkov <bp@alien8.de> - 2017-06-27 15:50 +0200
      Re: [PATCH 1/2] x86/CPU/AMD: Present package as die instead of socket Borislav Petkov <bp@alien8.de> - 2017-06-27 19:50 +0200
        RE: [PATCH 1/2] x86/CPU/AMD: Present package as die instead of socket "Ghannam, Yazen" <Yazen.Ghannam@amd.com> - 2017-06-27 20:40 +0200
          Re: [PATCH 1/2] x86/CPU/AMD: Present package as die instead of socket Borislav Petkov <bp@alien8.de> - 2017-06-27 20:50 +0200
        Re: [PATCH 1/2] x86/CPU/AMD: Present package as die instead of socket Borislav Petkov <bp@alien8.de> - 2017-06-28 11:20 +0200
    Re: [PATCH 1/2] x86/CPU/AMD: Present package as die instead of  socket Thomas Gleixner <tglx@linutronix.de> - 2017-06-27 16:30 +0200
      Re: [PATCH 1/2] x86/CPU/AMD: Present package as die instead of socket Brice Goglin <brice.goglin@gmail.com> - 2017-06-27 17:00 +0200
      RE: [PATCH 1/2] x86/CPU/AMD: Present package as die instead of socket "Duran, Leo" <leo.duran@amd.com> - 2017-06-27 17:50 +0200
        RE: [PATCH 1/2] x86/CPU/AMD: Present package as die instead of  socket Thomas Gleixner <tglx@linutronix.de> - 2017-06-27 18:20 +0200
          RE: [PATCH 1/2] x86/CPU/AMD: Present package as die instead of socket "Duran, Leo" <leo.duran@amd.com> - 2017-06-27 18:40 +0200
        Re: [PATCH 1/2] x86/CPU/AMD: Present package as die instead of socket Borislav Petkov <bp@alien8.de> - 2017-06-27 18:20 +0200
          RE: [PATCH 1/2] x86/CPU/AMD: Present package as die instead of socket "Duran, Leo" <leo.duran@amd.com> - 2017-06-27 18:30 +0200
            Re: [PATCH 1/2] x86/CPU/AMD: Present package as die instead of socket Borislav Petkov <bp@alien8.de> - 2017-06-27 18:40 +0200
              Re: [PATCH 1/2] x86/CPU/AMD: Present package as die instead of socket Borislav Petkov <bp@alien8.de> - 2017-06-27 18:50 +0200
                RE: [PATCH 1/2] x86/CPU/AMD: Present package as die instead of socket "Duran, Leo" <leo.duran@amd.com> - 2017-06-27 19:10 +0200
              RE: [PATCH 1/2] x86/CPU/AMD: Present package as die instead of socket "Duran, Leo" <leo.duran@amd.com> - 2017-06-27 18:50 +0200
      Re: [PATCH 1/2] x86/CPU/AMD: Present package as die instead of socket Borislav Petkov <bp@alien8.de> - 2017-06-27 18:20 +0200

#1675538 — Re: [PATCH 1/2] x86/CPU/AMD: Present package as die instead of socket

Remove stable from CC - this is not how you do a stable submission.

See Documentation/process/stable-kernel-rules.rst

On Tue, Jun 27, 2017 at 01:40:52AM -0500, Suravee Suthikulpanit wrote:
> According to the Documentation/x86/topology.txt, AMD nomenclature for
> package is NUMA node (or die).

You guys keep talking about die so you should add the definition of
"die" to that document so that we're all on the same page. In a separate
patch.

Which reminds me, this patch does a gazillion things. It needs to be
split.

> However, this is not the case on AMD family17h multi-die processor
> platforms, which can have up to 4 dies per socket as shown in the
> following system topology.

So what exactly does that mean?

A die is a package on ZN and you can have up to 4 packages on a physical
socket?

Please be specific and verbose.

> Die (Dx) View :
>              ----------------------------
>          C0  | T0 T1 |    ||    | T0 T1 | C4
>              --------|    ||    |--------
>          C1  | T0 T1 | L3 || L3 | T0 T1 | C5
>              --------|    ||    |--------
>          C2  | T0 T1 | #0 || #1 | T0 T1 | C6
>              --------|    ||    |--------
>          C3  | T0 T1 |    ||    | T0 T1 | C7
>              ----------------------------
> 
> System View (with 2 socket) :
>            --------------------
>            |     -------------|------
>            |     |            |     |
>          ------------       ------------
>          | D1 -- D0 |       | D7 -- D6 |
>          | |  \/ |  |       | |  \/ |  |
>  SOCKET0 | |  /\ |  |       | |  /\ |  | SOCKET1
>          | D2 -- D3 |       | D4 -- D5 |
>          ------------       ------------
>            |     |            |     |
>            ------|------------|     |
>                  --------------------
> 
> Current logic interpretes package as socket (i.e. phys_proc_id is
> socket id), which results in setting x86_has_numa_in_package, and omits
> the DIE schedule domain.

Well, this is what we do on MCM Magny-Cours. Btw, here's where you
explain *why* you need the DIE domain.

> However, NUMA schedule domains are derived from
> SRAT/SLIT, which assumes NUMA node is a die,

Ok, so I don't understand this: SRAT/SLIT should basically say that you
have 4 *logical* NUMA nodes on that *physical* socket. Is that what you
want?

Or is not what you want?

Because it makes sense to me that BIOS should describe how the logical
grouping of the cores in a node is. And x86_numa_in_package_topology
roughly means that we rely on SRAT/SLIT to tell us the topology.

So why can't SRAT/SLIT do that just like they did on Magy-Cours MCM
packages?

> and build NUMA schedule
> domains on top of NUMA nodes. This results in incomplete schedule domains
> as following:
>     domain 0: SMT
>     domain 1: MC       /* core complex w/ shared L3*/
>     ---- Missing DIE level domain ----
>     domain 2: NUMA     /* socket */
>     domain 3: NUMA     /* platform */
> 
> Presenting package-as-die does not set x86_has_numa_in_package.
> 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Signed-off-by: Leo Duran <leo.duran@amd.com>
> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>

This SOB chain is wrong, of course. Lemme guess, you want to say that
you all three worked on this patch, right?

Well, this is not how we express that. Please read this:

Documentation/process/submitting-patches.rst, section 11

And while you're at it, read section 3 too pls.

> Cc: <stable@vger.kernel.org> # v4.10+
> ---
>  arch/x86/kernel/cpu/amd.c | 189 +++++++++++++++++++++++++++-------------------
>  1 file changed, 112 insertions(+), 77 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
> index bb5abe8..2f5869c 100644
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -1,3 +1,5 @@
> +#define pr_fmt(fmt) "x86/AMD: " fmt
> +

This needs to go in a separate patch along with auditing all pr_* calls
in that file and removing any other prefixes they have.

>  #include <linux/export.h>
>  #include <linux/bitops.h>
>  #include <linux/elf.h>
> @@ -32,6 +34,12 @@ static bool cpu_has_amd_erratum(struct cpuinfo_x86 *cpu, const int *erratum);
>   */
>  static u32 nodes_per_socket = 1;
>  
> +/*
> + * l3_num_threads_sharing: Stores the number of threads sharing L3 cache.
> + * Refer to CPUID_Fn8000001D_EAX_x03 [Cache Properties (L3)] NumSharingCache.
> + */
> +static u32 l3_num_threads_sharing;

u32 is clearly too much. I know, nodes_per_socket is u32 too but that
should be turned into an u8 too. In a separate patch.

> +
>  static inline int rdmsrl_amd_safe(unsigned msr, unsigned long long *p)
>  {
>  	u32 gprs[8] = { 0 };
> @@ -296,96 +304,122 @@ static int nearby_node(int apicid)
>  }
>  #endif
>  
> +#ifdef CONFIG_SMP
> +

This is an unnecessary move.

>  /*
> - * Fixup core topology information for
> - * (1) AMD multi-node processors
> + * Per Documentation/x86/topology.c,

$ ls Documentation/x86/topology.c
ls: cannot access 'Documentation/x86/topology.c': No such file or directory

> + * the kernel works with
> + *  {packages, cores, threads}, and we will map:
> + *
> + *  thread  = core in compute-unit (CMT), or thread in core (SMT)
> + *  core    = compute-unit (CMT), or core (SMT)
> + *  package = node (die)

Put all those definitions into Documentation/x86/topology.txt and point
only to that file in the comment here.

> + *
> + * Discover topology based on available information from CPUID first,
> + * and only derive them as needed.
> + *
> + * (1) phys_proc_id is die ID in AMD multi-die processors.
>   *     Assumption: Number of cores in each internal node is the same.

So that's wrong:

  - cpuinfo_x86.phys_proc_id:

    The physical ID of the package. This information is retrieved via CPUID
    and deduced from the APIC IDs of the cores in the package.

> - * (2) AMD processors supporting compute units
> + * (2) cpu_core_id is derived from either CPUID topology extension
> + *     or initial APIC_ID.
> + * (3) cpu_llc_id is either L3 or per-node

Move to Documentation/x86/topology.txt

>   */
> -#ifdef CONFIG_SMP
>  static void amd_get_topology(struct cpuinfo_x86 *c)
>  {
> -	u8 node_id;
>  	int cpu = smp_processor_id();
>  
> -	/* get information required for multi-node processors */
>  	if (boot_cpu_has(X86_FEATURE_TOPOEXT)) {
>  		u32 eax, ebx, ecx, edx;
>  
>  		cpuid(0x8000001e, &eax, &ebx, &ecx, &edx);
>  
> -		node_id  = ecx & 0xff;
> +		c->phys_proc_id = ecx & 0xff;
>  		smp_num_siblings = ((ebx >> 8) & 0xff) + 1;
>  
> -		if (c->x86 == 0x15)
> -			c->cu_id = ebx & 0xff;

You can't remove this, of course, see

79a8b9aa388b ("x86/CPU/AMD: Bring back Compute Unit ID")

And so on and so on.

So I'm going to stop reviewing here because:

* this patch needs to be split
* I still don't understand why we need all that dance and not describe the
topology properly with SRAT/SLIT

Thanks.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [next] | [standalone]

#1675712

On Tue, Jun 27, 2017 at 08:07:10PM +0700, Suravee Suthikulpanit wrote:
> What we are trying point out here is that (NUMA) "node" and "die" are the
> same thing in most of AMD processors, not necessary trying to introduce
> another term here.

So don't use it then. The whole topology topic is confusing as it is to
people so that every time I, for example, have to explain myself with an
example when talking about it. Adding a "die" into the mix makes it more
confusing, not less.

So pick the terms, please, and document them properly so that we all are
on the same page when talking about topology.

> Yes. 4 packages (or 4 dies, or 4 NUMA nodes) in a socket.

See above.

I'd like to have the topology terminology all explained and written
down, pls.

> As I have described in the cover letter, this patch series changes how

I know what you've described in the cover letter - I've read it. I
meant, put that same explanation in the commit message to state *why*
this patch is needed.

> SLIT table is showing
> 
> node   0   1   2   3   4   5   6   7
>   0:  10  16  16  16  32  32  32  32
>   1:  16  10  16  16  32  32  32  32
>   2:  16  16  10  16  32  32  32  32
>   3:  16  16  16  10  32  32  32  32
>   4:  32  32  32  32  10  16  16  16
>   5:  32  32  32  32  16  10  16  16
>   6:  32  32  32  32  16  16  10  16
>   7:  32  32  32  32  16  16  16  10
>

> However, SRAT/SLIT does not describe the DIE. So, using
> x86_numa_in_packge_topology on multi-die Zen processor will result in
> missing the DIE sched-domain for cpus within a die.

What does "does not describe the DIE" mean exactly? How exactly you need
to describe a die. And forget the die sched domain - first answer the
question: how is the NUMA info in SRAT/SLIT insufficient for scheduling?

Are you saying, you want to have all threads on a die belong to a
separate scheduling entity?

> Again, Magny-Cours MCM DIE sched-domain is the same as MC sched-domain.
> So, we can omit the DIE sched-domain. Here is an example of /proc/schedstat

Ok, we can forget the DIE thing.

> Magney-Cours cpu0
> domain0 00000000,00000003 (SMT)
> domain1 00000000,000000ff (MC which is the same as DIE)
> domain2 00ff00ff,00ffffff (NUMA 1 hop)
> domain3 ffffffff,ffffffff (NUMA platform)
> 
> Zen cpu0 (package-as-socket)
> domain0 00000000,00000001,00000000,00000001 (SMT)
> domain1 00000000,0000000f,00000000,0000000f (MC ccx)
> domain2 00000000,ffffffff,00000000,ffffffff (NUMA socket)
> domain3 ffffffff,ffffffff,ffffffff,ffffffff (NUMA platform)
> 
> Zen cpu0 (package-as-die)
> domain0 00000000,00000001,00000000,00000001 (SMT)
> domain1 00000000,0000000f,00000000,0000000f (MC ccx)
> domain2 00000000,000000ff,00000000,000000ff (DIE)

So this is 8 threads IINM.

You want to have those 8 threads as a separate scheduling entity?

But looking at this picture:

Die (Dx) View :
             ----------------------------
         C0  | T0 T1 |    ||    | T0 T1 | C4
             --------|    ||    |--------
         C1  | T0 T1 | L3 || L3 | T0 T1 | C5
             --------|    ||    |--------
         C2  | T0 T1 | #0 || #1 | T0 T1 | C6
             --------|    ||    |--------
         C3  | T0 T1 |    ||    | T0 T1 | C7
             ----------------------------

That's 16 threads on a die.

So are you trying to tell me that you want to have all threads sharing
an L3 into a single scheduling domain? Is that it?

Or do you want to have all threads on a die in a single scheduling
domain?

Also, bear in mind that if we do this picking of
threads/cores/nodes/dies/... apart like this now, it will definitely
need touching in the future when the hw guys change topology again.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1676039

On Tue, Jun 27, 2017 at 11:54:12PM +0700, Suravee Suthikulpanit wrote:
> The 8 threads sharing each L3 are already in the same sched-domain1 (MC
> CCX). So, cpu0 is in the same sched-domain1 as cpu1,2,3,64,65,66,67. Here,
> we need the DIE sched-domain because it represents all cpus that are in the
> same NUMA node (since we have one memory controller per DIE).

So this is still confusing. Please drop the "DIE sched-domain" as that is
something you're trying to define and I'm trying to parse what you're
trying to define and why.

> IIUC, for Zen, w/o the DIE sched-domain, the scheduler could try to
> re-balance the tasks from one CCX (schedule group) to another CCX
> across NUMA node, and

CCX, schedule group, NUMA node, ... now my head is spinning. Do you
see what I mean with agreeing on the nomenclature and proper term
definitions first?

> potentially causing unnecessary performance due to remote memory access.
> 
> Please note also that SRAT/SLIT information are used to derive the NUMA
> sched-domains, while the DIE sched-domain is non-NUMA sched-domain (derived
> from CPUID topology extension which is available on newer families).

So let's try to discuss this without using DIE sched-domain, CCX, etc,
and let's start simple.

So in that die graphic:

              ----------------------------
          C0  | T0 T1 |    ||    | T0 T1 | C4
              --------|    ||    |--------
          C1  | T0 T1 | L3 || L3 | T0 T1 | C5
              --------|    ||    |--------
          C2  | T0 T1 | #0 || #1 | T0 T1 | C6
              --------|    ||    |--------
          C3  | T0 T1 |    ||    | T0 T1 | C7
              ----------------------------

you want all those threads to belong to a single scheduling group.
Correct?

Now that thing has a memory controller attached to it, correct?

If so, why is this thing not a logical NUMA node, as described in
SRAT/SLIT?

If not, what does a NUMA node entail on Zen as described by SRAT/SLIT?
I.e., what is the difference between the two things? I.e., how many dies
as above are in a NUMA node?

Now, SRAT should contain the assignment which core belongs to which
node. Why is that not sufficient?

Ok, that should be enough questions for now. Let's start with them.

Thanks.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1676074

> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
> owner@vger.kernel.org] On Behalf Of Borislav Petkov
> Sent: Tuesday, June 27, 2017 1:44 PM
> To: Suthikulpanit, Suravee <Suravee.Suthikulpanit@amd.com>
> Cc: x86@kernel.org; linux-kernel@vger.kernel.org; Duran, Leo
> <leo.duran@amd.com>; Ghannam, Yazen <Yazen.Ghannam@amd.com>;
> Peter Zijlstra <peterz@infradead.org>
> Subject: Re: [PATCH 1/2] x86/CPU/AMD: Present package as die instead of
> socket
> 
> On Tue, Jun 27, 2017 at 11:54:12PM +0700, Suravee Suthikulpanit wrote:
> > The 8 threads sharing each L3 are already in the same sched-domain1
> > (MC CCX). So, cpu0 is in the same sched-domain1 as
> > cpu1,2,3,64,65,66,67. Here, we need the DIE sched-domain because it
> > represents all cpus that are in the same NUMA node (since we have one
> memory controller per DIE).
> 
> So this is still confusing. Please drop the "DIE sched-domain" as that is
> something you're trying to define and I'm trying to parse what you're trying to
> define and why.
> 
> > IIUC, for Zen, w/o the DIE sched-domain, the scheduler could try to
> > re-balance the tasks from one CCX (schedule group) to another CCX
> > across NUMA node, and
> 
> CCX, schedule group, NUMA node, ... now my head is spinning. Do you see
> what I mean with agreeing on the nomenclature and proper term definitions
> first?
> 
> > potentially causing unnecessary performance due to remote memory
> access.
> >
> > Please note also that SRAT/SLIT information are used to derive the
> > NUMA sched-domains, while the DIE sched-domain is non-NUMA
> > sched-domain (derived from CPUID topology extension which is available on
> newer families).
> 
> So let's try to discuss this without using DIE sched-domain, CCX, etc, and let's
> start simple.
> 
> So in that die graphic:
> 
>               ----------------------------
>           C0  | T0 T1 |    ||    | T0 T1 | C4
>               --------|    ||    |--------
>           C1  | T0 T1 | L3 || L3 | T0 T1 | C5
>               --------|    ||    |--------
>           C2  | T0 T1 | #0 || #1 | T0 T1 | C6
>               --------|    ||    |--------
>           C3  | T0 T1 |    ||    | T0 T1 | C7
>               ----------------------------
> 
> you want all those threads to belong to a single scheduling group.
> Correct?
> 
> Now that thing has a memory controller attached to it, correct?
> 
> If so, why is this thing not a logical NUMA node, as described in SRAT/SLIT?
> 
> If not, what does a NUMA node entail on Zen as described by SRAT/SLIT?
> I.e., what is the difference between the two things? I.e., how many dies as
> above are in a NUMA node?
> 
> Now, SRAT should contain the assignment which core belongs to which node.
> Why is that not sufficient?
> 
> Ok, that should be enough questions for now. Let's start with them.
> 

This group is a NUMA node. It is the "identity" NUMA node. Linux skips the
identity NUMA node when finding the NUMA levels. This is fine as long as the
MC domain is equivalent to the identity NUMA node. However, this is not the
case on Zen systems.

We could patch the sched/topology.c to not skip the identity NUMA node.
Though this will affect all systems not just AMD.

Something like this:

diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
index 1b0b4fb..98d856c 100644
--- a/kernel/sched/topology.c
+++ b/kernel/sched/topology.c
@@ -1103,6 +1103,8 @@ void sched_init_numa(void)
 	 * node_distance(i,j) in order to avoid cubic time.
 	 */
 	next_distance = curr_distance;
+	sched_domains_numa_distance[level++] = next_distance;
+	sched_domains_numa_levels = level;
 	for (i = 0; i < nr_node_ids; i++) {
 		for (j = 0; j < nr_node_ids; j++) {
 			for (k = 0; k < nr_node_ids; k++) {

Thanks,
Yazen 

[toc] | [prev] | [next] | [standalone]

#1676078

On Tue, Jun 27, 2017 at 06:32:34PM +0000, Ghannam, Yazen wrote:
> > you want all those threads to belong to a single scheduling group.
> > Correct?
> > 
> > Now that thing has a memory controller attached to it, correct?
> > 
> > If so, why is this thing not a logical NUMA node, as described in SRAT/SLIT?
> > 
> > If not, what does a NUMA node entail on Zen as described by SRAT/SLIT?
> > I.e., what is the difference between the two things? I.e., how many dies as
> > above are in a NUMA node?
> > 
> > Now, SRAT should contain the assignment which core belongs to which node.
> > Why is that not sufficient?
> > 
> > Ok, that should be enough questions for now. Let's start with them.
> > 
> 
> This group is a NUMA node. It is the "identity" NUMA node. Linux skips the

Please be more specific. Which group exactly? Which question above are
you answering?

> identity NUMA node when finding the NUMA levels. This is fine as long as the
> MC domain is equivalent to the identity NUMA node. However, this is not the
> case on Zen systems.
> 
> We could patch the sched/topology.c to not skip the identity NUMA node.
> Though this will affect all systems not just AMD.

We can always add a X86_FEATURE flag but we need to agree on what you
guys are actually trying to change and why?

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1676468

+ Matt and Mel.

On Wed, Jun 28, 2017 at 03:26:10AM +0700, Suravee Suthikulpanit wrote:
> So, from the definition above, we would like all those 16 threads to be in
> the same sched-domain, where threads from C0,1,2,3 are in the same
> sched-group, and threads in C4,5,6,7 are in another sched-group.

Figures, you want to have a sched group per L3.

> > Now that thing has a memory controller attached to it, correct?
> 
> Yes
> 
> > If so, why is this thing not a logical NUMA node, as described in
> > SRAT/SLIT?
> 
> Yes, this thing is a logical NUMA node and represented correctly in the SRAT/SLIT.
> 
> > Now, SRAT should contain the assignment which core belongs to which
> > node. Why is that not sufficient?
> 
> Yes, SRAT provides cpu-to-node mapping, which is sufficient to tell
> scheduler what are the cpus within a NUMA node.
> 
> However, looking at the current sched-domain below. Notice that there is no
> sched-domain with 16 threads to represent a NUMA node:
> 
> cpu0
> domain0 00000000,00000001,00000000,00000001 (SMT)
> domain1 00000000,0000000f,00000000,0000000f (MC)
> domain2 00000000,ffffffff,00000000,ffffffff (NUMA)
> domain3 ffffffff,ffffffff,ffffffff,ffffffff (NUMA)
> 
> sched-domain2 (which represents a sched-domain containing all cpus within a
> socket) would have 8 sched-groups (based on the cpumasks from domain1).
> According to the documentation snippet above regarding balancing within a
> sched-domain, scheduler will try to do (NUMA) load-balance between 8 groups
> (spanning 4 NUMA node). Here, IINM, it would be more beneficial if the
> scheduler would try to load balance between the two groups within the same
> NUMA node first before, going across NUMA node in order to minimize memory
> latency. This would require another sched-domain between domain 1 and 2,
> which represent all 16 threads within a NUMA node (i.e. die sched-domain),
> this would allow scheduler to load balance within the NUMA node first,
> before going across NUMA node.
> 
> However, since the current code decides that x86_has_numa_in_package is
> true, it omits the die sched-domain. In order to avoid this, we are
> proposing to represent cpuinfo_x86.phys_proc_id using NUMA node ID (i.e. die
> ID). And this is the main point of the patch series.

Ok, so here's what I'm reading from all this: a 16-thread package *is*
represented as a logical NUMA node properly by SRAT/SLIT, after all.
However, sched_init_numa() doesn't parse it properly or something else
along that path keeps the scheduler from seeing the 4 NUMA nodes on the
physical socket.

Because we do use the SLIT table to build the NUMA domains. So
considering that 16T package really is represented properly as a logical
NUMA node, why doesn't it get detected as such?

This is what needs asnwering first and not some forceful fitting the
topology into a DIE domain.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1675798 — Re: [PATCH 1/2] x86/CPU/AMD: Present package as die instead of socket

On Tue, 27 Jun 2017, Suravee Suthikulpanit wrote:
> On 6/27/17 17:48, Borislav Petkov wrote:
> > On Tue, Jun 27, 2017 at 01:40:52AM -0500, Suravee Suthikulpanit wrote:
> > > However, this is not the case on AMD family17h multi-die processor
> > > platforms, which can have up to 4 dies per socket as shown in the
> > > following system topology.
> > 
> > So what exactly does that mean? A die is a package on ZN and you can have up
> > to 4 packages on a physical socket?
> 
> Yes. 4 packages (or 4 dies, or 4 NUMA nodes) in a socket.

And why is this relevant at all?

The kernel does not care about sockets. Sockets are electromechanical
components and completely irrelevant.

The kernel cares about :

    Threads	 - Single scheduling unit

    Cores	 - Contains one or more threads

    Packages	 - Contains one or more cores. The cores share L3.
    
    NUMA Node	 - Contains one or more Packages which share a memory
    	 	   controller.

		   I'm not aware of x86 systems which have several Packages
		   sharing a memory controller, so Package == NUMA Node
		   (but I might be wrong here).

    Platform	 - Contains one or more Numa Nodes

All the kernel is interested in is the above and the NUMA Node distance so
it knows about memory access latencies. No sockets, no MCMs, that's all
completely useless for the scheduler.

So if the current CPUID stuff gives you the same phycial package ID for all
packages in a MCM, then this needs to be fixed at the CPUID/ACPI/BIOS level
and not hacked around in the kernel.

The only reason why a MCM might need it's own ID is, when it contains
infrastructure which is shared between the packages, but again that's
irrelevant for the scheduler. That'd be only relevant to implement a driver
for that shared infrastructure.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1675861

Le 27/06/2017 16:21, Thomas Gleixner a écrit :
> On Tue, 27 Jun 2017, Suravee Suthikulpanit wrote:
>> On 6/27/17 17:48, Borislav Petkov wrote:
>>> On Tue, Jun 27, 2017 at 01:40:52AM -0500, Suravee Suthikulpanit wrote:
>>>> However, this is not the case on AMD family17h multi-die processor
>>>> platforms, which can have up to 4 dies per socket as shown in the
>>>> following system topology.
>>> So what exactly does that mean? A die is a package on ZN and you can have up
>>> to 4 packages on a physical socket?
>> Yes. 4 packages (or 4 dies, or 4 NUMA nodes) in a socket.
> And why is this relevant at all?
>
> The kernel does not care about sockets. Sockets are electromechanical
> components and completely irrelevant.
>
> The kernel cares about :
>
>     Threads	 - Single scheduling unit
>
>     Cores	 - Contains one or more threads
>
>     Packages	 - Contains one or more cores. The cores share L3.
>     
>     NUMA Node	 - Contains one or more Packages which share a memory
>     	 	   controller.
>
> 		   I'm not aware of x86 systems which have several Packages
> 		   sharing a memory controller, so Package == NUMA Node
> 		   (but I might be wrong here).

You often have multiple NUMA nodes inside a single package. That's what
we see in sysfs on Intel (since haswell, when Cluster-on-Die is enabled)
and on AMD (since magny-cours).

Kernel "packages" contains cores whose "physical_package_id" are the
same. Documentation says:
     physical_package_id: physical package id of cpu#. Typically
     corresponds to a physical socket number, but the actual value
     is architecture and platform dependent.

I don't know if it's possible on x86 to have different
physical_package_ids for different cores on a single socket.

Brice

[toc] | [prev] | [next] | [standalone]

#1675924

Hi Thomas, et al,

Just a quick comment below.
Leo.


> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx@linutronix.de]
> Sent: Tuesday, June 27, 2017 9:21 AM
> To: Suthikulpanit, Suravee <Suravee.Suthikulpanit@amd.com>
> Cc: Borislav Petkov <bp@alien8.de>; x86@kernel.org; linux-
> kernel@vger.kernel.org; Duran, Leo <leo.duran@amd.com>; Ghannam,
> Yazen <Yazen.Ghannam@amd.com>; Peter Zijlstra <peterz@infradead.org>
> Subject: Re: [PATCH 1/2] x86/CPU/AMD: Present package as die instead of
> socket
> 
> On Tue, 27 Jun 2017, Suravee Suthikulpanit wrote:
> > On 6/27/17 17:48, Borislav Petkov wrote:
> > > On Tue, Jun 27, 2017 at 01:40:52AM -0500, Suravee Suthikulpanit wrote:
> > > > However, this is not the case on AMD family17h multi-die processor
> > > > platforms, which can have up to 4 dies per socket as shown in the
> > > > following system topology.
> > >
> > > So what exactly does that mean? A die is a package on ZN and you can
> > > have up to 4 packages on a physical socket?
> >
> > Yes. 4 packages (or 4 dies, or 4 NUMA nodes) in a socket.
> 
> And why is this relevant at all?
> 
> The kernel does not care about sockets. Sockets are electromechanical
> components and completely irrelevant.
> 
> The kernel cares about :
> 
>     Threads	 - Single scheduling unit
> 
>     Cores	 - Contains one or more threads
> 
>     Packages	 - Contains one or more cores. The cores share L3.
> 
>     NUMA Node	 - Contains one or more Packages which share a memory
>     	 	   controller.
> 
> 		   I'm not aware of x86 systems which have several Packages
> 		   sharing a memory controller, so Package == NUMA Node
> 		   (but I might be wrong here).
> 
>     Platform	 - Contains one or more Numa Nodes
[Duran, Leo] 
That is my understanding of intent as well... However, regarding the L3:

The sentence 'The cores share L3.' under 'Packages' may give the impression that all cores in a package share an L3.
In our case, we define a Package a group of cores sharing a memory controller, a 'Die' in hardware terms.
Also, it turns out that within a Package we may have separate groups of cores each having their own L3 (in hardware terms we refer to those as a 'Core Complex').

Basically, in our case a Package may contain more than one L3 (i.e., in hardware terms, there may more than one 'Core complex' in a 'Die').
The important point is that all logical processors (threads) that share an L3 have a common "cpu_llc_id".

> 
> All the kernel is interested in is the above and the NUMA Node distance so it
> knows about memory access latencies. No sockets, no MCMs, that's all
> completely useless for the scheduler.
> 
> So if the current CPUID stuff gives you the same phycial package ID for all
> packages in a MCM, then this needs to be fixed at the CPUID/ACPI/BIOS
> level and not hacked around in the kernel.
> 
> The only reason why a MCM might need it's own ID is, when it contains
> infrastructure which is shared between the packages, but again that's
> irrelevant for the scheduler. That'd be only relevant to implement a driver for
> that shared infrastructure.
> 
> Thanks,
> 
> 	tglx

[toc] | [prev] | [next] | [standalone]

#1675956 — RE: [PATCH 1/2] x86/CPU/AMD: Present package as die instead of socket

On Tue, 27 Jun 2017, Duran, Leo wrote:
> > From: Thomas Gleixner [mailto:tglx@linutronix.de]
> > On Tue, 27 Jun 2017, Suravee Suthikulpanit wrote:
> > > On 6/27/17 17:48, Borislav Petkov wrote:
> > > > On Tue, Jun 27, 2017 at 01:40:52AM -0500, Suravee Suthikulpanit wrote:
> > > > > However, this is not the case on AMD family17h multi-die processor
> > > > > platforms, which can have up to 4 dies per socket as shown in the
> > > > > following system topology.
> > > >
> > > > So what exactly does that mean? A die is a package on ZN and you can
> > > > have up to 4 packages on a physical socket?
> > >
> > > Yes. 4 packages (or 4 dies, or 4 NUMA nodes) in a socket.
> > 
> > And why is this relevant at all?
> > 
> > The kernel does not care about sockets. Sockets are electromechanical
> > components and completely irrelevant.
> > 
> > The kernel cares about :
> > 
> >     Threads	 - Single scheduling unit
> > 
> >     Cores	 - Contains one or more threads
> > 
> >     Packages	 - Contains one or more cores. The cores share L3.
> > 
> >     NUMA Node	 - Contains one or more Packages which share a memory
> >     	 	   controller.
> > 
> > 		   I'm not aware of x86 systems which have several Packages
> > 		   sharing a memory controller, so Package == NUMA Node
> > 		   (but I might be wrong here).
> > 
> >     Platform	 - Contains one or more Numa Nodes
> [Duran, Leo] 

> That is my understanding of intent as well... However, regarding the L3:

Obviously is that not your understanding.

> The sentence 'The cores share L3.' under 'Packages' may give the
> impression that all cores in a package share an L3.
> In our case, we define a Package a group of cores sharing a memory
> controller, a 'Die' in hardware terms.
>
> Also, it turns out that within a Package we may have separate groups of
> cores each having their own L3 (in hardware terms we refer to those as a
> 'Core Complex').
>
> Basically, in our case a Package may contain more than one L3 (i.e., in
> hardware terms, there may more than one 'Core complex' in a 'Die').
>
> The important point is that all logical processors (threads) that share
> an L3 have a common "cpu_llc_id".

I don't care at all what you call a package. Really. The only relevant view
is what the kernel thinks a package is. And I made that entirely clear.

If you think that's confusing, then feel free to submit patches which
change the names we are using throughout x86 including the documentation.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1675992

Thomas,
Please see my comments & question below.
Leo.

> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx@linutronix.de]
> Sent: Tuesday, June 27, 2017 11:19 AM
> To: Duran, Leo <leo.duran@amd.com>
> Cc: Suthikulpanit, Suravee <Suravee.Suthikulpanit@amd.com>; Borislav
> Petkov <bp@alien8.de>; x86@kernel.org; linux-kernel@vger.kernel.org;
> Ghannam, Yazen <Yazen.Ghannam@amd.com>; Peter Zijlstra
> <peterz@infradead.org>
> Subject: RE: [PATCH 1/2] x86/CPU/AMD: Present package as die instead of
> socket
> 
> On Tue, 27 Jun 2017, Duran, Leo wrote:
> > > From: Thomas Gleixner [mailto:tglx@linutronix.de] On Tue, 27 Jun
> > > 2017, Suravee Suthikulpanit wrote:
> > > > On 6/27/17 17:48, Borislav Petkov wrote:
> > > > > On Tue, Jun 27, 2017 at 01:40:52AM -0500, Suravee Suthikulpanit
> wrote:
> > > > > > However, this is not the case on AMD family17h multi-die
> > > > > > processor platforms, which can have up to 4 dies per socket as
> > > > > > shown in the following system topology.
> > > > >
> > > > > So what exactly does that mean? A die is a package on ZN and you
> > > > > can have up to 4 packages on a physical socket?
> > > >
> > > > Yes. 4 packages (or 4 dies, or 4 NUMA nodes) in a socket.
> > >
> > > And why is this relevant at all?
> > >
> > > The kernel does not care about sockets. Sockets are
> > > electromechanical components and completely irrelevant.
> > >
> > > The kernel cares about :
> > >
> > >     Threads	 - Single scheduling unit
> > >
> > >     Cores	 - Contains one or more threads
> > >
> > >     Packages	 - Contains one or more cores. The cores share L3.
> > >
> > >     NUMA Node	 - Contains one or more Packages which share a
> memory
> > >     	 	   controller.
> > >
> > > 		   I'm not aware of x86 systems which have several Packages
> > > 		   sharing a memory controller, so Package == NUMA Node
> > > 		   (but I might be wrong here).
> > >
> > >     Platform	 - Contains one or more Numa Nodes
> > [Duran, Leo]
> 
> > That is my understanding of intent as well... However, regarding the L3:
> 
> Obviously is that not your understanding.
> 
> > The sentence 'The cores share L3.' under 'Packages' may give the
> > impression that all cores in a package share an L3.
> > In our case, we define a Package a group of cores sharing a memory
> > controller, a 'Die' in hardware terms.
> >
> > Also, it turns out that within a Package we may have separate groups
> > of cores each having their own L3 (in hardware terms we refer to those
> > as a 'Core Complex').
> >
> > Basically, in our case a Package may contain more than one L3 (i.e.,
> > in hardware terms, there may more than one 'Core complex' in a 'Die').
> >
> > The important point is that all logical processors (threads) that
> > share an L3 have a common "cpu_llc_id".
> 
> I don't care at all what you call a package. Really. The only relevant view is
> what the kernel thinks a package is. And I made that entirely clear.
[Duran, Leo] 
Well, our intent is to return what the kernel expects.
So, if we're not doing that properly, we need to fix it.

To be clear,
Are you saying that there can only be a single L3 per 'Package'?


> 
> If you think that's confusing, then feel free to submit patches which change
> the names we are using throughout x86 including the documentation.
> 
> Thanks,
> 
> 	tglx

[toc] | [prev] | [next] | [standalone]

#1675967

On Tue, Jun 27, 2017 at 03:48:55PM +0000, Duran, Leo wrote:
> Basically, in our case a Package may contain more than one L3 (i.e.,
> in hardware terms, there may more than one 'Core complex' in a 'Die').
> The important point is that all logical processors (threads) that
> share an L3 have a common "cpu_llc_id".

All that means nothing if it doesn't have any effect on scheduling.

If moving the working set between L3s within the package, as you call
it, is not measureable, i.e., task migration, then we don't care. The
whole exercise is not about modelling the hardware topology accurately
but for presenting sufficient detailed topology to the scheduler so that
it works optimally on Zen.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1675972

Boris,
Please see my comments below.
Leo.

> -----Original Message-----
> From: Borislav Petkov [mailto:bp@alien8.de]
> Sent: Tuesday, June 27, 2017 11:12 AM
> To: Duran, Leo <leo.duran@amd.com>
> Cc: 'Thomas Gleixner' <tglx@linutronix.de>; Suthikulpanit, Suravee
> <Suravee.Suthikulpanit@amd.com>; x86@kernel.org; linux-
> kernel@vger.kernel.org; Ghannam, Yazen <Yazen.Ghannam@amd.com>;
> Peter Zijlstra <peterz@infradead.org>
> Subject: Re: [PATCH 1/2] x86/CPU/AMD: Present package as die instead of
> socket
> 
> On Tue, Jun 27, 2017 at 03:48:55PM +0000, Duran, Leo wrote:
> > Basically, in our case a Package may contain more than one L3 (i.e.,
> > in hardware terms, there may more than one 'Core complex' in a 'Die').
> > The important point is that all logical processors (threads) that
> > share an L3 have a common "cpu_llc_id".
> 
> All that means nothing if it doesn't have any effect on scheduling.
> 
> If moving the working set between L3s within the package, as you call it, is
> not measureable, i.e., task migration, then we don't care. The whole exercise
> is not about modelling the hardware topology accurately but for presenting
> sufficient detailed topology to the scheduler so that it works optimally on
> Zen.
[Duran, Leo] 
My understanding is the intent of the vendor-specific CPU topology code (in our case, amd.c) is to accurately present topology to the kernel, based on the 'abstractions' the kernel has defined.
(abstractions being terms like "Package", "cpu_llc_id", etc)

It is then up-to code that consumes the kernel-defined abstraction to ensure optimal use of the returned topology information.

That is,
My understating is that it is *not* our job at the "amd.c" level to return information that influences the scheduler in any prescribed way.
If that were the case, it would seem to me that the code consuming the returned topology information is 'broken', or that the defined abstractions are 'broken'.


> 
> --
> Regards/Gruss,
>     Boris.
> 
> Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1675988

On Tue, Jun 27, 2017 at 04:23:16PM +0000, Duran, Leo wrote:
> My understating is that it is *not* our job at the "amd.c" level to
> return information that influences the scheduler in any prescribed
> way.

Your understanding is wrong.

The abstractions we present to the rest of the kernel is so that other
facilities can operate in generic way without having to know the
processor they're running on.

In your particular case you're trying to address two, AFAIU, scheduling
problems.

So no, we want to tell the scheduler *exactly* what it needs to know.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1675999

On Tue, Jun 27, 2017 at 04:42:32PM +0000, Duran, Leo wrote:

First of all, please do not top-post.

> Are you saying that "amd.c' should be scheduler-aware?.. Really?

Please read again what I said.

> If so, are you saying that information returned by kernel-defined
> terms like 'Package', 'Core',

"information returned by kernel-defined terms"... hmmm, I don't know
what that means.

> etc, should done in the context of understanding the scheduler, rather
> than in the context what is being documented for those terms to
> actually mean or represent.

-ENOPARSE.

> I'd hope that "amd.c" should be doing the latter... and that perhaps
> we're simply not returning information as specified by the intended
> definition of those terms (in which case we need to fix our code)

-ENOPARSE.

I can't really understand what you're trying to tell me here.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1676010

> -----Original Message-----
> From: Borislav Petkov [mailto:bp@alien8.de]
> Sent: Tuesday, June 27, 2017 11:46 AM
> To: Duran, Leo <leo.duran@amd.com>
> Cc: 'Thomas Gleixner' <tglx@linutronix.de>; Suthikulpanit, Suravee
> <Suravee.Suthikulpanit@amd.com>; x86@kernel.org; linux-
> kernel@vger.kernel.org; Ghannam, Yazen <Yazen.Ghannam@amd.com>;
> Peter Zijlstra <peterz@infradead.org>
> Subject: Re: [PATCH 1/2] x86/CPU/AMD: Present package as die instead of
> socket
> 
> On Tue, Jun 27, 2017 at 04:42:32PM +0000, Duran, Leo wrote:
> 
> First of all, please do not top-post.
> 
> > Are you saying that "amd.c' should be scheduler-aware?.. Really?
> 
> Please read again what I said.
> 
> > If so, are you saying that information returned by kernel-defined
> > terms like 'Package', 'Core',
> 
> "information returned by kernel-defined terms"... hmmm, I don't know what
> that means.
[Duran, Leo] 
I'm referring to: Documentation\x86\ topology.txt
Started by Thomas Gleixner <tglx@linutronix.de> and Borislav Petkov <bp@alien8.de>.

> 
> > etc, should done in the context of understanding the scheduler, rather
> > than in the context what is being documented for those terms to
> > actually mean or represent.
> 
> -ENOPARSE.
> 
> > I'd hope that "amd.c" should be doing the latter... and that perhaps
> > we're simply not returning information as specified by the intended
> > definition of those terms (in which case we need to fix our code)
> 
> -ENOPARSE.
> 
> I can't really understand what you're trying to tell me here.
> 
[Duran, Leo] 
OK, let me try again:
I'd hope that "amd.c" returns 'Package', 'Core', et al, in compliance with the document I referred to above.
Allowing code that consumes the returned information to make (hopefully optimal) decisions in a vendor-agnostic way.

> --
> Regards/Gruss,
>     Boris.
> 
> Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1676000

Boris,

Are you saying that "amd.c' should be scheduler-aware?.. Really?

If so, are you saying that information returned by kernel-defined terms like 'Package', 'Core', etc, should done in the context of understanding the scheduler,
rather than in the context what is being documented for those terms to actually mean or represent.

I'd hope that "amd.c" should be doing the latter... and that perhaps we're simply not returning information as specified by the intended definition of those terms (in which case we need to fix our code)

Leo.

> -----Original Message-----
> From: Borislav Petkov [mailto:bp@alien8.de]
> Sent: Tuesday, June 27, 2017 11:32 AM
> To: Duran, Leo <leo.duran@amd.com>
> Cc: 'Thomas Gleixner' <tglx@linutronix.de>; Suthikulpanit, Suravee
> <Suravee.Suthikulpanit@amd.com>; x86@kernel.org; linux-
> kernel@vger.kernel.org; Ghannam, Yazen <Yazen.Ghannam@amd.com>;
> Peter Zijlstra <peterz@infradead.org>
> Subject: Re: [PATCH 1/2] x86/CPU/AMD: Present package as die instead of
> socket
> 
> On Tue, Jun 27, 2017 at 04:23:16PM +0000, Duran, Leo wrote:
> > My understating is that it is *not* our job at the "amd.c" level to
> > return information that influences the scheduler in any prescribed
> > way.
> 
> Your understanding is wrong.
> 
> The abstractions we present to the rest of the kernel is so that other facilities
> can operate in generic way without having to know the processor they're
> running on.
> 
> In your particular case you're trying to address two, AFAIU, scheduling
> problems.
> 
> So no, we want to tell the scheduler *exactly* what it needs to know.
> 
> --
> Regards/Gruss,
>     Boris.
> 
> Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1675970

On Tue, Jun 27, 2017 at 10:55:52PM +0700, Suravee Suthikulpanit wrote:
> The reason we are trying to present "package == NUMA node (die)" here is
> because the topology.txt defines package to contain a number of cores plus
> shared resources (e.g. DRAM controller, shared caches, etc). Since the
> cpuinfo_x86.phys_proc_id is also defined as the physical ID of the package,

Ok, it seems we will continue talking past each other here. So let's
look at the issues separately:

 * irqbalance fails to allocating IRQs to individual CPU within the die.

Why does it fail? What is the root cause for this?

 * The scheduler fails to load-balance across 8 threads within a die
   (e.g. running 8-thread application w/ taskset -c 0-7 ) with
   the DIE schedule domain omitted due to x86_has_numa_in_package.

Why do you need to load-balance within the die? Why not load-balance
within the 0-15 threads?

What are the disadvantages of the situation now?

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web