[PATCH] powerpc/pseries: Don't attempt to acquire drc during memory hot add for assigned lmbs

[PATCH] powerpc/pseries: Don't attempt to acquire drc during memory hot add for assigned lmbs

[John Allen]

Check if an LMB is assigned before attempting to call dlpar_acquire_drc in
order to avoid any unnecessary rtas calls. This substantially reduces the
running time of memory hot add on lpars with large amounts of memory.

Signed-off-by: John Allen <jallen@linux.vnet.ibm.com>
---
diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
index ca9b2f4..95cf2ff 100644
--- a/arch/powerpc/platforms/pseries/hotplug-memory.c
+++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
@@ -817,6 +817,9 @@ static int dlpar_memory_add_by_count(u32 lmbs_to_add, struct property *prop)
 		return -EINVAL;

 	for (i = 0; i < num_lmbs && lmbs_to_add != lmbs_added; i++) {
+		if (lmbs[i].flags & DRCONF_MEM_ASSIGNED)
+			continue;
+
 		rc = dlpar_acquire_drc(lmbs[i].drc_index);
 		if (rc)
 			continue;

Re: [PATCH] powerpc/pseries: Don't attempt to acquire drc during memory hot add for assigned lmbs

[Nathan Fontenot]

On 08/23/2017 12:18 PM, John Allen wrote:
> Check if an LMB is assigned before attempting to call dlpar_acquire_drc in
> order to avoid any unnecessary rtas calls. This substantially reduces the
> running time of memory hot add on lpars with large amounts of memory.
> 
> Signed-off-by: John Allen <jallen@linux.vnet.ibm.com>
> ---
> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
> index ca9b2f4..95cf2ff 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
> @@ -817,6 +817,9 @@ static int dlpar_memory_add_by_count(u32 lmbs_to_add, struct property *prop)
>  		return -EINVAL;
> 
>  	for (i = 0; i < num_lmbs && lmbs_to_add != lmbs_added; i++) {
> +		if (lmbs[i].flags & DRCONF_MEM_ASSIGNED)
> +			continue;
> +

Reviewed-by: Nathan Fontenot <nfont@linux.vnet.ibm.com>

>  		rc = dlpar_acquire_drc(lmbs[i].drc_index);
>  		if (rc)
>  			continue;
> 

Re: [PATCH] powerpc/pseries: Don't attempt to acquire drc during memory hot add for assigned lmbs

[Michael Ellerman]

John Allen <jallen@linux.vnet.ibm.com> writes:

> Check if an LMB is assigned before attempting to call dlpar_acquire_drc in
> order to avoid any unnecessary rtas calls. This substantially reduces the
> running time of memory hot add on lpars with large amounts of memory.
>
> Signed-off-by: John Allen <jallen@linux.vnet.ibm.com>

I'll add:

  Fixes: c21f515c7436 ("powerpc/pseries: Make the acquire/release of the drc for memory a seperate step")

?

How bad is the slow down, do we need to backport to stable/distros?

cheers

Re: [PATCH] powerpc/pseries: Don't attempt to acquire drc during memory hot add for assigned lmbs

[John Allen]

On 08/24/2017 05:33 AM, Michael Ellerman wrote:
> John Allen <jallen@linux.vnet.ibm.com> writes:
> 
>> Check if an LMB is assigned before attempting to call dlpar_acquire_drc in
>> order to avoid any unnecessary rtas calls. This substantially reduces the
>> running time of memory hot add on lpars with large amounts of memory.
>>
>> Signed-off-by: John Allen <jallen@linux.vnet.ibm.com>
> 
> I'll add:
> 
>   Fixes: c21f515c7436 ("powerpc/pseries: Make the acquire/release of the drc for memory a seperate step")
> 
> ?

Yes, thanks.

> 
> How bad is the slow down, do we need to backport to stable/distros?

On an lpar with 16 TB of memory assigned, I observed that adding 1 GB of
memory took several minutes without this fix and improved to several
seconds with this fix. Yep, this will need to be backported. Memory hotplug
performance is a hot issue for our team right now and we'll want to have
solid performance improvement to give to customers relatively soon.

> 
> cheers
> 
> 

Re: [PATCH] powerpc/pseries: Don't attempt to acquire drc during memory hot add for assigned lmbs

[Michael Ellerman]

John Allen <jallen@linux.vnet.ibm.com> writes:

> Check if an LMB is assigned before attempting to call dlpar_acquire_drc in
> order to avoid any unnecessary rtas calls. This substantially reduces the
> running time of memory hot add on lpars with large amounts of memory.
>
> Signed-off-by: John Allen <jallen@linux.vnet.ibm.com>
> ---
> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
> index ca9b2f4..95cf2ff 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
> @@ -817,6 +817,9 @@ static int dlpar_memory_add_by_count(u32 lmbs_to_add, struct property *prop)
>  		return -EINVAL;
>
>  	for (i = 0; i < num_lmbs && lmbs_to_add != lmbs_added; i++) {
> +		if (lmbs[i].flags & DRCONF_MEM_ASSIGNED)
> +			continue;
> +
>  		rc = dlpar_acquire_drc(lmbs[i].drc_index);
>  		if (rc)
>  			continue;

This doesn't build for me, see below. What compiler are you using to
test this?

  arch/powerpc/platforms/pseries/hotplug-memory.c: In function 'dlpar_memory':
  arch/powerpc/platforms/pseries/hotplug-memory.c:1081:2: error: 'rc' may be used uninitialized in this function [-Werror=maybe-uninitialized]
    return rc;
    ^

It's a bit confusing because you didn't modify that function, but the
function you did modify has been inlined into there.

Possibly the compiler is wrong and we do always initialise rc, but it's
not clear at all.

And it raises a bigger question, how is this supposed to actually work?

If we go around the loop and find that something is already assigned:

	for (i = 0; i < num_lmbs && lmbs_to_add != lmbs_added; i++) {
		if (lmbs[i].flags & DRCONF_MEM_ASSIGNED)
			continue;
                ...
		lmbs_added++;
                ...
        }

We don't increment lmbs_added, so at the end of the loop we will see
that lmbs_added is not equal to lmbs_to_add, and so we remove
everything:

	if (lmbs_added != lmbs_to_add) {
		pr_err("Memory hot-add failed, removing any added LMBs\n");

		for (i = 0; i < num_lmbs; i++) {
			if (!lmbs[i].reserved)
				continue;

			rc = dlpar_remove_lmb(&lmbs[i]);


So it seems like if we ever hit that continue you added, the whole
operation will fail anyway. So I'm confused.

cheers

Re: [PATCH] powerpc/pseries: Don't attempt to acquire drc during memory hot add for assigned lmbs

[Nathan Fontenot]

On 08/29/2017 09:35 PM, Michael Ellerman wrote:
> John Allen <jallen@linux.vnet.ibm.com> writes:
> 
>> Check if an LMB is assigned before attempting to call dlpar_acquire_drc in
>> order to avoid any unnecessary rtas calls. This substantially reduces the
>> running time of memory hot add on lpars with large amounts of memory.
>>
>> Signed-off-by: John Allen <jallen@linux.vnet.ibm.com>
>> ---
>> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
>> index ca9b2f4..95cf2ff 100644
>> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
>> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
>> @@ -817,6 +817,9 @@ static int dlpar_memory_add_by_count(u32 lmbs_to_add, struct property *prop)
>>  		return -EINVAL;
>>
>>  	for (i = 0; i < num_lmbs && lmbs_to_add != lmbs_added; i++) {
>> +		if (lmbs[i].flags & DRCONF_MEM_ASSIGNED)
>> +			continue;
>> +
>>  		rc = dlpar_acquire_drc(lmbs[i].drc_index);
>>  		if (rc)
>>  			continue;
> 
> This doesn't build for me, see below. What compiler are you using to
> test this?> 
>   arch/powerpc/platforms/pseries/hotplug-memory.c: In function 'dlpar_memory':
>   arch/powerpc/platforms/pseries/hotplug-memory.c:1081:2: error: 'rc' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>     return rc;
>     ^
>

I don't see this build failure either, looks like its time to update my
compiler too.
 
> It's a bit confusing because you didn't modify that function, but the
> function you did modify has been inlined into there.
> 
> Possibly the compiler is wrong and we do always initialise rc, but it's
> not clear at all.
> 
> And it raises a bigger question, how is this supposed to actually work?
> 
> If we go around the loop and find that something is already assigned:
> 
> 	for (i = 0; i < num_lmbs && lmbs_to_add != lmbs_added; i++) {
> 		if (lmbs[i].flags & DRCONF_MEM_ASSIGNED)
> 			continue;
>                 ...
> 		lmbs_added++;
>                 ...
>         }
> 
> We don't increment lmbs_added, so at the end of the loop we will see
> that lmbs_added is not equal to lmbs_to_add, and so we remove
> everything:

There is a loop just before this code verifies there are enough
LMBs available to satisfy the add request. This counts the number
of LMBs that do not have the assigned flag set. This is where we
update lmbs_available and if there are not enough lmbs available to
satisfy the request, we bail.

When the loop you referenced above exits we should have attempted to
enough LMBs to satisfy the request.

> 
> 	if (lmbs_added != lmbs_to_add) {
> 		pr_err("Memory hot-add failed, removing any added LMBs\n");
> 
> 		for (i = 0; i < num_lmbs; i++) {
> 			if (!lmbs[i].reserved)
> 				continue;
> 
> 			rc = dlpar_remove_lmb(&lmbs[i]);
> 
>

if we get into this recovery path, we only try to remove any LMBs
that were added in the loop above. The code marks any LMB that is
added as 'reserved' so that we only try to remove LMBs marked
as 'reserved' in the cleanup path.
 
> So it seems like if we ever hit that continue you added, the whole
> operation will fail anyway. So I'm confused.

The added code was to skip over any LMBs that are already assigned to
the partition. As the code is written now, we walk through the entire list
of LMBs possible. For large systems this can be tens, or hundreds of thousands
of LMBs.

Without this check the code will try to acquire the drc for (this
is done via rtas-set-indicator calls) which will fail for LMBs that are
already assigned to the partition.

As an example on a large system that has 100,000 LMBs where the first
50,000 are assigned to the partition. When we get an add request to add
X more LMBs we do see that there are 50,000 available to add. We then try
to add X LMBs but start at the beginning of the LMB list when trying to
add them. This results in 50,000 attempts to acquire the drc for LMbs
that we already own. Instead we should just skip trying to acquire them.

Hopefully that helps clarify things.

-Nathan

> 
> cheers
> 

Re: [PATCH] powerpc/pseries: Don't attempt to acquire drc during memory hot add for assigned lmbs

[John Allen]

On 08/30/2017 09:35 AM, Nathan Fontenot wrote:
> On 08/29/2017 09:35 PM, Michael Ellerman wrote:
>> John Allen <jallen@linux.vnet.ibm.com> writes:
>>
>>> Check if an LMB is assigned before attempting to call dlpar_acquire_drc in
>>> order to avoid any unnecessary rtas calls. This substantially reduces the
>>> running time of memory hot add on lpars with large amounts of memory.
>>>
>>> Signed-off-by: John Allen <jallen@linux.vnet.ibm.com>
>>> ---
>>> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
>>> index ca9b2f4..95cf2ff 100644
>>> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
>>> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
>>> @@ -817,6 +817,9 @@ static int dlpar_memory_add_by_count(u32 lmbs_to_add, struct property *prop)
>>>  		return -EINVAL;
>>>
>>>  	for (i = 0; i < num_lmbs && lmbs_to_add != lmbs_added; i++) {
>>> +		if (lmbs[i].flags & DRCONF_MEM_ASSIGNED)
>>> +			continue;
>>> +
>>>  		rc = dlpar_acquire_drc(lmbs[i].drc_index);
>>>  		if (rc)
>>>  			continue;
>>
>> This doesn't build for me, see below. What compiler are you using to
>> test this?> 
>>   arch/powerpc/platforms/pseries/hotplug-memory.c: In function 'dlpar_memory':
>>   arch/powerpc/platforms/pseries/hotplug-memory.c:1081:2: error: 'rc' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>     return rc;
>>     ^
>>
> 
> I don't see this build failure either, looks like its time to update my
> compiler too.

This also builds for me. I'm using gcc version 4.8.5

> 
>> It's a bit confusing because you didn't modify that function, but the
>> function you did modify has been inlined into there.
>>
>> Possibly the compiler is wrong and we do always initialise rc, but it's
>> not clear at all.
>>
>> And it raises a bigger question, how is this supposed to actually work?
>>
>> If we go around the loop and find that something is already assigned:
>>
>> 	for (i = 0; i < num_lmbs && lmbs_to_add != lmbs_added; i++) {
>> 		if (lmbs[i].flags & DRCONF_MEM_ASSIGNED)
>> 			continue;
>>                 ...
>> 		lmbs_added++;
>>                 ...
>>         }
>>
>> We don't increment lmbs_added, so at the end of the loop we will see
>> that lmbs_added is not equal to lmbs_to_add, and so we remove
>> everything:
> 
> There is a loop just before this code verifies there are enough
> LMBs available to satisfy the add request. This counts the number
> of LMBs that do not have the assigned flag set. This is where we
> update lmbs_available and if there are not enough lmbs available to
> satisfy the request, we bail.
> 
> When the loop you referenced above exits we should have attempted to
> enough LMBs to satisfy the request.
> 
>>
>> 	if (lmbs_added != lmbs_to_add) {
>> 		pr_err("Memory hot-add failed, removing any added LMBs\n");
>>
>> 		for (i = 0; i < num_lmbs; i++) {
>> 			if (!lmbs[i].reserved)
>> 				continue;
>>
>> 			rc = dlpar_remove_lmb(&lmbs[i]);
>>
>>
> 
> if we get into this recovery path, we only try to remove any LMBs
> that were added in the loop above. The code marks any LMB that is
> added as 'reserved' so that we only try to remove LMBs marked
> as 'reserved' in the cleanup path.
> 
>> So it seems like if we ever hit that continue you added, the whole
>> operation will fail anyway. So I'm confused.
> 
> The added code was to skip over any LMBs that are already assigned to
> the partition. As the code is written now, we walk through the entire list
> of LMBs possible. For large systems this can be tens, or hundreds of thousands
> of LMBs.
> 
> Without this check the code will try to acquire the drc for (this
> is done via rtas-set-indicator calls) which will fail for LMBs that are
> already assigned to the partition.
> 
> As an example on a large system that has 100,000 LMBs where the first
> 50,000 are assigned to the partition. When we get an add request to add
> X more LMBs we do see that there are 50,000 available to add. We then try
> to add X LMBs but start at the beginning of the LMB list when trying to
> add them. This results in 50,000 attempts to acquire the drc for LMbs
> that we already own. Instead we should just skip trying to acquire them.
> 
> Hopefully that helps clarify things.
> 
> -Nathan
> 
>>
>> cheers
>>
> 

Re: [PATCH] powerpc/pseries: Don't attempt to acquire drc during memory hot add for assigned lmbs

[Michael Ellerman]

John Allen <jallen@linux.vnet.ibm.com> writes:

> On 08/30/2017 09:35 AM, Nathan Fontenot wrote:
>> On 08/29/2017 09:35 PM, Michael Ellerman wrote:
>>> John Allen <jallen@linux.vnet.ibm.com> writes:
>>>
>>>> Check if an LMB is assigned before attempting to call dlpar_acquire_drc in
>>>> order to avoid any unnecessary rtas calls. This substantially reduces the
>>>> running time of memory hot add on lpars with large amounts of memory.
>>>>
>>>> Signed-off-by: John Allen <jallen@linux.vnet.ibm.com>
>>>> ---
>>>> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
>>>> index ca9b2f4..95cf2ff 100644
>>>> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
>>>> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
>>>> @@ -817,6 +817,9 @@ static int dlpar_memory_add_by_count(u32 lmbs_to_add, struct property *prop)
>>>>  		return -EINVAL;
>>>>
>>>>  	for (i = 0; i < num_lmbs && lmbs_to_add != lmbs_added; i++) {
>>>> +		if (lmbs[i].flags & DRCONF_MEM_ASSIGNED)
>>>> +			continue;
>>>> +
>>>>  		rc = dlpar_acquire_drc(lmbs[i].drc_index);
>>>>  		if (rc)
>>>>  			continue;
>>>
>>> This doesn't build for me, see below. What compiler are you using to
>>> test this?> 
>>>   arch/powerpc/platforms/pseries/hotplug-memory.c: In function 'dlpar_memory':
>>>   arch/powerpc/platforms/pseries/hotplug-memory.c:1081:2: error: 'rc' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>>     return rc;
>>>     ^
>>>
>> 
>> I don't see this build failure either, looks like its time to update my
>> compiler too.
>
> This also builds for me. I'm using gcc version 4.8.5

OK, that is fairly old.

I generally use 6.x or 7.x, but I see the warning on 5.4.1 also.

cheers

Re: [PATCH] powerpc/pseries: Don't attempt to acquire drc during memory hot add for assigned lmbs

[Michael Ellerman]

Nathan Fontenot <nfont@linux.vnet.ibm.com> writes:
...
>
> Hopefully that helps clarify things.

Yep thanks. In short, read the code better Michael :)

I folded in this hunk, which will keep the compiler happy and is also
correct I believe. Basically in the success case (lmbs_added == lmbs_to_add)
we explicitly set rc to 0.

@@ -870,6 +873,7 @@ static int dlpar_memory_add_by_count(u32 lmbs_to_add, struct property *prop)
                                lmbs[i].base_addr, lmbs[i].drc_index);
                        lmbs[i].reserved = 0;
                }
+               rc = 0;
        }
 
        return rc;

cheers

Re: powerpc/pseries: Don't attempt to acquire drc during memory hot add for assigned lmbs

[Michael Ellerman]

On Wed, 2017-08-23 at 17:18:43 UTC, John Allen wrote:
> Check if an LMB is assigned before attempting to call dlpar_acquire_drc in
> order to avoid any unnecessary rtas calls. This substantially reduces the
> running time of memory hot add on lpars with large amounts of memory.
> 
> Signed-off-by: John Allen <jallen@linux.vnet.ibm.com>
> Reviewed-by: Nathan Fontenot <nfont@linux.vnet.ibm.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/afb5519fdb346201728040cab4e08c

cheers