linux-kernel - Re: [PATCH V12] powerpc/vphn: Update CPU topology when VPHN enabled

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <87h8wm686b.fsf@concordia.ellerman.id.au>
Date:   Fri, 01 Sep 2017 20:10:36 +1000
From:   Michael Ellerman <mpe@...erman.id.au>
To:     Michael Bringmann <mwb@...ux.vnet.ibm.com>,
        linuxppc-dev@...ts.ozlabs.org, linux-kernel@...r.kernel.org
Cc:     Michael Bringmann <mwb@...ux.vnet.ibm.com>,
        Nathan Fontenot <nfont@...ux.vnet.ibm.com>,
        John Allen <jallen@...ux.vnet.ibm.com>
Subject: Re: [PATCH V12] powerpc/vphn: Update CPU topology when VPHN enabled

Hi Michael,

Thanks for trying to reduce this down to the minimum required.

But ...

Michael Bringmann <mwb@...ux.vnet.ibm.com> writes:
> powerpc/numa: On Power systems with shared configurations of CPUs
> and memory, there are some issues with the association of additional
> CPUs and memory to nodes when hot-adding resources.  This patch
> addresses some of those problems.
>
> First, it corrects the currently broken capability to set the
> topology for shared CPUs in LPARs.  At boot time for shared CPU
> lpars, the topology for each CPU was being set to node zero.  Now
> when numa_update_cpu_topology() is called appropriately, the Virtual
> Processor Home Node (VPHN) capabilities information provided by the
> pHyp allows the appropriate node in the shared configuration to be
> selected for the CPU.

That should be one patch.

> Next, it updates the initialization checks to independently recognize
> PRRN or VPHN support.

And another.

> Next, during hotplug CPU operations, it resets the timer on topology
> update work function to a small value to better ensure that the CPU
> topology is detected and configured sooner.

Another.

> Also, fix an end-of-updates processing problem observed occasionally
> in numa_update_cpu_topology().

And another.


I would have let you get away with lumping it all in together, except
that it doesn't compile when CONFIG_PPC_SPLPAR=n:

  In file included from ../include/linux/topology.h:35:0,
                   from ../include/linux/gfp.h:8,
                   from ../include/linux/irq.h:17,
                   from ../arch/powerpc/include/asm/hardirq.h:5,
                   from ../include/linux/hardirq.h:8,
                   from ../include/linux/interrupt.h:12,
                   from ../arch/powerpc/platforms/pseries/hotplug-cpu.c:24:
  ../arch/powerpc/platforms/pseries/hotplug-cpu.c: In function ‘dlpar_online_cpu’:
  ../arch/powerpc/include/asm/topology.h:103:38: error: statement with no effect [-Werror=unused-value]
   #define timed_topology_update(nsecs) 0
                                        ^
  ../arch/powerpc/platforms/pseries/hotplug-cpu.c:366:4: note: in expansion of macro ‘timed_topology_update’
      timed_topology_update(1);
      ^~~~~~~~~~~~~~~~~~~~~
  ../arch/powerpc/platforms/pseries/hotplug-cpu.c: In function ‘dlpar_offline_cpu’:
  ../arch/powerpc/include/asm/topology.h:103:38: error: statement with no effect [-Werror=unused-value]
   #define timed_topology_update(nsecs) 0
                                        ^
  ../arch/powerpc/platforms/pseries/hotplug-cpu.c:533:5: note: in expansion of macro ‘timed_topology_update’
       timed_topology_update(1);
       ^~~~~~~~~~~~~~~~~~~~~
  cc1: all warnings being treated as errors


So please fix that up, split it, and resend.

I know it would have been fairly easy for me to fix that compiler error,
but it tells me you haven't tested that configuration at all, which is
the bigger problem. Please at least give it a smoke test.

cheers