Re: [PATCH] spapr: Add a new level of NUMA for GPUs

[Reza Arbab]

Hi David,

Thanks for your quick response!

On Mon, May 11, 2020 at 04:17:45PM +1000, David Gibson wrote:
> 1)
>
> This would all be much simpler, if PAPR's representation of NUMA
> distances weren't so awful.  Somehow it manages to be both so complex
> that it's very hard to understand, and yet very limited in that it
> has no way to represent distances in any absolute units, or even
> specific ratios between distances.
>
> Both qemu and the guest kernel can have an arbitrary set of nodes,
> with an arbitrary matrix of distances between each pair, which we then
> have to lossily encode into this PAPR nonsense.

Completely agree. I've revisited that section many times now and still 
find the descriptions of these properties almost incomprehensible.

The only way I've been able to make sense of it comes from reading the 
implementation, or experimentally tweaking the code/device tree to see 
if distances behave the way I expect.

> 2)
>
> Alas, I don't think we can simply change this information.  We'll have
> to do it conditionally on a new machine type.  This is guest visible
> information, which shouldn't change under a running guest across
> migration between different qemu versions.  At least for Linux guests
> we'd probably get away with it, since I think it only reads this info
> at boot, and across a migration we'd at worst get non-optimal
> behaviour, not actual breakage.

Sure, that makes sense. I'll try making the change conditional on a flag 
that can be set on new machine types.

> 3)
>
> I'm not sure that this version is totally correct w.r.t. PAPR.  But
> then, I'm also not really sure of that for the existing version.
> Specifically it's not at all clear from PAPR if the IDs used at each
> level of the ibm,associativity need to be:
>   a) globally unique
>   b) unique only within the associativity level they appear at
> or c) unique only within the "node" at the next higher level they
>      belong to

Again, I'm no authority but it seems to be (b):

  "To determine the associativity between any two resources, the OS
scans down the two resources associativity lists in all pair wise 
combinations counting how many domains are the same until the first 
domain where the two list do not agree."

FWIW, using the same number for id at multiple levels has been working 
in practice.

> 4)
>
> I'm not totally clear on the rationale for using the individual gpu's
> numa ID at all levels, rather than just one.  I'm guessing this is so
> that the gpu memory blocks are distant from each other as well as
> distant from main memory.  Is that right?

It's not necessary to use it at all levels--that was me trying to 
completely replicate that old firmware change. Strictly speaking, since 
only reference-points 4 (and now 2) are significant, that part could 
just be:

@@ -362,7 +362,7 @@ void spapr_phb_nvgpu_ram_populate_dt(SpaprPhbState *sphb, 
void *fdt)
         uint32_t associativity[] = {
             cpu_to_be32(0x4),
             SPAPR_GPU_NUMA_ID,
-            SPAPR_GPU_NUMA_ID,
+            cpu_to_be32(nvslot->numa_id),
             SPAPR_GPU_NUMA_ID,
             cpu_to_be32(nvslot->numa_id)
         };

I think the rationale is that if those other levels got added to 
reference-points in the future, you'd likely want the GPU to be distinct 
there too.

--
Reza Arbab