Re: [PATCH V9 1/2] powerpc/numa: Update CPU topology when VPHN enabled

Nathan Fontenot [off-list ref] writes:

On 08/23/2017 06:41 AM, Michael Ellerman wrote:

quoted

Michael Bringmann [off-list ref] writes:

quoted

powerpc/numa: Correct the currently broken capability to set the
topology for shared CPUs in LPARs.  At boot time for shared CPU
lpars, the topology for each shared CPU is set to node zero, however,
this is now updated correctly using the Virtual Processor Home Node
(VPHN) capabilities information provided by the pHyp.

Also, update initialization checks for device-tree attributes to
independently recognize PRRN or VPHN usage.

Did you ever do anything to address Nathan's comments on v4 ?

  http://patchwork.ozlabs.org/patch/767587/

Looking at this patch I do not see that VPHN is always enabled.

You mean *this* patch? Or v4?

I think you mean this patch, in which case I agree.

quoted

Also your change log doesn't describe anything about what the patch does
and why it is the correct fix for the problem.

When a DLPAR happens you modify the VPHN timer to run in 1 nsec, but you
don't wait for it. Why would we not just run the logic synchronously?

It also seems to make VPHN and PRRN no longer exclusive, which looking
at PAPR seems like it might be correct, but is also a major change so
please justify it in detail.

This is correct, they are not exclusive. When we first added PRRN support
we mistakenly thought they were exclusive which is why the code currently
only starts PRRN, or VPHN if PRRN is not present.

OK.

So we need a patch that does that and only that, and clearly explains
why we're doing that and why it's the correct thing to do.

Then a 2nd patch can fiddle with the timer, if we must.

...

quoted

quoted

+static int topology_timer_secs = TOPOLOGY_DEF_TIMER_SECS;
+static int topology_inited;
+static int topology_update_needed;

None of this code should be in numa.c. Which is not your fault but I'm
inclined to move it before we make it worse.

Agreed. Perhaps this should all be in mm/vphn.c

Actually I was thinking platforms/pseries/vphn.c

cheers