[PATCH] Correct cpu affinity for dlpar added cpus

[PATCH] Correct cpu affinity for dlpar added cpus

[Nathan Fontenot]

The incorrect ordering of operations during cpu dlpar causes the affinity
of cpus being added to be invalid. Phyp does not assign affinity information
for a cpu until the rtas set-indicator calls are made to set the isolation
and allocation state. In the current code we call rtas configure-connector
before making the set-indicator calls which results in invalid data in the
ibm,associativity property for the cpu we're adding.

This patch corrects the order of operations to make the set-indicator
calls (done in acquire_drc) before calling configure-connector.

Signed-off-by: Nathan Fontenot <nfont@linux.vnet.ibm.com>
---
 arch/powerpc/platforms/pseries/dlpar.c |   10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c
index b4b1109..019d34a 100644
--- a/arch/powerpc/platforms/pseries/dlpar.c
+++ b/arch/powerpc/platforms/pseries/dlpar.c
@@ -412,6 +412,10 @@ static ssize_t dlpar_cpu_probe(const char *buf, size_t count)
 	if (rc)
 		return -EINVAL;
 
+	rc = dlpar_acquire_drc(drc_index);
+	if (rc)
+		return -EINVAL;
+
 	parent = of_find_node_by_path("/cpus");
 	if (!parent)
 		return -ENODEV;
@@ -422,12 +426,6 @@ static ssize_t dlpar_cpu_probe(const char *buf, size_t count)
 
 	of_node_put(parent);
 
-	rc = dlpar_acquire_drc(drc_index);
-	if (rc) {
-		dlpar_free_cc_nodes(dn);
-		return -EINVAL;
-	}
-
 	rc = dlpar_attach_node(dn);
 	if (rc) {
 		dlpar_release_drc(drc_index);

Re: [PATCH] Correct cpu affinity for dlpar added cpus

[Michael Ellerman]

Subject should be "powerpc/pseries: ..." please.

On Tue, 2015-04-28 at 10:37 -0500, Nathan Fontenot wrote:
> The incorrect ordering of operations during cpu dlpar causes the affinity
> of cpus being added to be invalid. Phyp does not assign affinity information
> for a cpu until the rtas set-indicator calls are made to set the isolation
> and allocation state. In the current code we call rtas configure-connector
> before making the set-indicator calls which results in invalid data in the
> ibm,associativity property for the cpu we're adding.

Invalid and benign? Or invalid and causes an oops or ..?

> This patch corrects the order of operations to make the set-indicator
> calls (done in acquire_drc) before calling configure-connector.

Which commit added the code and/or caused it to be wrong?

  https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches#n187


While looking at the code I notice it looks like we leak a reference if
dlpar_configure_connector() fails:

	parent = of_find_node_by_path("/cpus");
	if (!parent)
		return -ENODEV;

	dn = dlpar_configure_connector(cpu_to_be32(drc_index), parent);
	if (!dn)
		return -EINVAL;

	of_node_put(parent);


Please send a separate patch to fix that.

cheers

Re: [PATCH] Correct cpu affinity for dlpar added cpus

[Nathan Fontenot]

On 04/28/2015 11:53 PM, Michael Ellerman wrote:
> Subject should be "powerpc/pseries: ..." please.

ok

> 
> On Tue, 2015-04-28 at 10:37 -0500, Nathan Fontenot wrote:
>> The incorrect ordering of operations during cpu dlpar causes the affinity
>> of cpus being added to be invalid. Phyp does not assign affinity information
>> for a cpu until the rtas set-indicator calls are made to set the isolation
>> and allocation state. In the current code we call rtas configure-connector
>> before making the set-indicator calls which results in invalid data in the
>> ibm,associativity property for the cpu we're adding.
> 
> Invalid and benign? Or invalid and causes an oops or ..?
>

Invalid and benign. I'll add this to the description.
 
>> This patch corrects the order of operations to make the set-indicator
>> calls (done in acquire_drc) before calling configure-connector.
> 
> Which commit added the code and/or caused it to be wrong?
> 
>   https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches#n187
> 

And the commit that this fixes..

> 
> While looking at the code I notice it looks like we leak a reference if
> dlpar_configure_connector() fails:
> 
> 	parent = of_find_node_by_path("/cpus");
> 	if (!parent)
> 		return -ENODEV;
> 
> 	dn = dlpar_configure_connector(cpu_to_be32(drc_index), parent);
> 	if (!dn)
> 		return -EINVAL;
> 
> 	of_node_put(parent);
> 
> 
> Please send a separate patch to fix that.

Yep, saw that too.

New patches on their way.

Thanks for the feedback,
-Nathan