[Qemu-devel] [PATCH] spapr_pci: advertise explicit numa IDs even when there's 1 node

[Qemu-devel] [PATCH] spapr_pci: advertise explicit numa IDs even when there's 1 node

[Michael Roth]

With the addition of "numa_node" properties for PHBs we began
advertising NUMA affinity in cases where nb_numa_nodes > 1.

Since the default on the guest side is to make no assumptions about
PHB NUMA affinity (defaulting to -1), there is still a valid use-case
for explicitly defining a PHB's NUMA affinity even when there's just
one node. In particular, some workloads make faulty assumptions about
/sys/bus/pci/<devid>/numa_node being >= 0, warranting the use of
this property as a workaround even if there's just 1 PHB or NUMA
node.

Enable this use-case by always advertising the PHB's NUMA affinity
if "numa_node" has been explicitly set.

We could achieve this by relaxing the check to simply be
nb_numa_nodes > 0, but even safer would be to check
numa_info[nodeid].present explicitly, and to fail at start time
for cases where it does not exist.

This has an additional affect of no longer advertising PHB NUMA
affinity unconditionally if nb_numa_nodes > 1 and "numa_node"
property is unset/-1, but since the default value on the guest
side for each PHB is also -1, the behavior should be the same for
that situation. We could still retain the old behavior if desired,
but the decision seems arbitrary, so we take the simpler route.

Cc: Alexey Kardashevskiy <aik@ozlabs.ru>
Cc: Shivaprasad G. Bhat <shivapbh@in.ibm.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 hw/ppc/spapr_pci.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 2a1ccf5..7cde30e 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1392,6 +1392,12 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    if (sphb->numa_node != -1 &&
+        (sphb->numa_node >= MAX_NODES || !numa_info[sphb->numa_node].present)) {
+        error_setg(errp, "Invalid NUMA node ID for PCI host bridge");
+        return;
+    }
+
     sphb->dtbusname = g_strdup_printf("pci@%" PRIx64, sphb->buid);
 
     namebuf = alloca(strlen(sphb->dtbusname) + 32);
@@ -1880,7 +1886,7 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
     }
 
     /* Advertise NUMA via ibm,associativity */
-    if (nb_numa_nodes > 1) {
+    if (phb->numa_node != -1) {
         _FDT(fdt_setprop(fdt, bus_off, "ibm,associativity", associativity,
                          sizeof(associativity)));
     }
-- 
1.9.1

Re: [Qemu-devel] [PATCH] spapr_pci: advertise explicit numa IDs even when there's 1 node

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2845 bytes --]

On Tue, Oct 18, 2016 at 03:50:23PM -0500, Michael Roth wrote:
> With the addition of "numa_node" properties for PHBs we began
> advertising NUMA affinity in cases where nb_numa_nodes > 1.
> 
> Since the default on the guest side is to make no assumptions about
> PHB NUMA affinity (defaulting to -1), there is still a valid use-case
> for explicitly defining a PHB's NUMA affinity even when there's just
> one node. In particular, some workloads make faulty assumptions about
> /sys/bus/pci/<devid>/numa_node being >= 0, warranting the use of
> this property as a workaround even if there's just 1 PHB or NUMA
> node.
> 
> Enable this use-case by always advertising the PHB's NUMA affinity
> if "numa_node" has been explicitly set.
> 
> We could achieve this by relaxing the check to simply be
> nb_numa_nodes > 0, but even safer would be to check
> numa_info[nodeid].present explicitly, and to fail at start time
> for cases where it does not exist.
> 
> This has an additional affect of no longer advertising PHB NUMA
> affinity unconditionally if nb_numa_nodes > 1 and "numa_node"
> property is unset/-1, but since the default value on the guest
> side for each PHB is also -1, the behavior should be the same for
> that situation. We could still retain the old behavior if desired,
> but the decision seems arbitrary, so we take the simpler route.
> 
> Cc: Alexey Kardashevskiy <aik@ozlabs.ru>
> Cc: Shivaprasad G. Bhat <shivapbh@in.ibm.com>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>

Applied to ppc-for-2.8, thanks.

> ---
>  hw/ppc/spapr_pci.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 2a1ccf5..7cde30e 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1392,6 +1392,12 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> +    if (sphb->numa_node != -1 &&
> +        (sphb->numa_node >= MAX_NODES || !numa_info[sphb->numa_node].present)) {
> +        error_setg(errp, "Invalid NUMA node ID for PCI host bridge");
> +        return;
> +    }
> +
>      sphb->dtbusname = g_strdup_printf("pci@%" PRIx64, sphb->buid);
>  
>      namebuf = alloca(strlen(sphb->dtbusname) + 32);
> @@ -1880,7 +1886,7 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
>      }
>  
>      /* Advertise NUMA via ibm,associativity */
> -    if (nb_numa_nodes > 1) {
> +    if (phb->numa_node != -1) {
>          _FDT(fdt_setprop(fdt, bus_off, "ibm,associativity", associativity,
>                           sizeof(associativity)));
>      }

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]