From: John Allen <hidden> Date: 2017-08-23 17:18:52
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 <redacted>
---
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 <redacted>
---
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2017-08-24 10:33:04
John Allen [off-list ref] 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 <redacted>
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
From: John Allen <hidden> Date: 2017-08-24 14:46:21
On 08/24/2017 05:33 AM, Michael Ellerman wrote:
John Allen [off-list ref] writes:
quoted
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 <redacted>
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.
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2017-08-30 02:35:08
John Allen [off-list ref] writes:
quoted hunk
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 <redacted>
---
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
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 <redacted>
---
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
From: John Allen <hidden> Date: 2017-08-30 14:43:12
On 08/30/2017 09:35 AM, Nathan Fontenot wrote:
On 08/29/2017 09:35 PM, Michael Ellerman wrote:
quoted
John Allen [off-list ref] writes:
quoted
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 <redacted>
---
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
quoted
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.
quoted
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.
quoted
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
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2017-08-31 10:24:30
Nathan Fontenot [off-list ref] 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.
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2017-08-31 10:33:00
John Allen [off-list ref] writes:
On 08/30/2017 09:35 AM, Nathan Fontenot wrote:
quoted
On 08/29/2017 09:35 PM, Michael Ellerman wrote:
quoted
John Allen [off-list ref] writes:
quoted
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 <redacted>
---
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
From: Michael Ellerman <hidden> Date: 2017-09-01 13:29:56
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 <redacted>
Reviewed-by: Nathan Fontenot <redacted>