* [PATCH] powerpc/pseries: Don't attempt to acquire drc during memory hot add for assigned lmbs @ 2017-08-23 17:18 John Allen 2017-08-23 18:54 ` Nathan Fontenot ` (3 more replies) 0 siblings, 4 replies; 10+ messages in thread From: John Allen @ 2017-08-23 17:18 UTC (permalink / raw) To: linuxppc-dev, Nathan Fontenot 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; ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] powerpc/pseries: Don't attempt to acquire drc during memory hot add for assigned lmbs 2017-08-23 17:18 [PATCH] powerpc/pseries: Don't attempt to acquire drc during memory hot add for assigned lmbs John Allen @ 2017-08-23 18:54 ` Nathan Fontenot 2017-08-24 10:33 ` Michael Ellerman ` (2 subsequent siblings) 3 siblings, 0 replies; 10+ messages in thread From: Nathan Fontenot @ 2017-08-23 18:54 UTC (permalink / raw) To: John Allen, linuxppc-dev 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; > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] powerpc/pseries: Don't attempt to acquire drc during memory hot add for assigned lmbs 2017-08-23 17:18 [PATCH] powerpc/pseries: Don't attempt to acquire drc during memory hot add for assigned lmbs John Allen 2017-08-23 18:54 ` Nathan Fontenot @ 2017-08-24 10:33 ` Michael Ellerman 2017-08-24 14:46 ` John Allen 2017-08-30 2:35 ` Michael Ellerman 2017-09-01 13:29 ` Michael Ellerman 3 siblings, 1 reply; 10+ messages in thread From: Michael Ellerman @ 2017-08-24 10:33 UTC (permalink / raw) To: John Allen, linuxppc-dev, Nathan Fontenot 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] powerpc/pseries: Don't attempt to acquire drc during memory hot add for assigned lmbs 2017-08-24 10:33 ` Michael Ellerman @ 2017-08-24 14:46 ` John Allen 0 siblings, 0 replies; 10+ messages in thread From: John Allen @ 2017-08-24 14:46 UTC (permalink / raw) To: Michael Ellerman, linuxppc-dev, Nathan Fontenot 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 > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] powerpc/pseries: Don't attempt to acquire drc during memory hot add for assigned lmbs 2017-08-23 17:18 [PATCH] powerpc/pseries: Don't attempt to acquire drc during memory hot add for assigned lmbs John Allen 2017-08-23 18:54 ` Nathan Fontenot 2017-08-24 10:33 ` Michael Ellerman @ 2017-08-30 2:35 ` Michael Ellerman 2017-08-30 14:35 ` Nathan Fontenot 2017-09-01 13:29 ` Michael Ellerman 3 siblings, 1 reply; 10+ messages in thread From: Michael Ellerman @ 2017-08-30 2:35 UTC (permalink / raw) To: John Allen, linuxppc-dev, Nathan Fontenot 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] powerpc/pseries: Don't attempt to acquire drc during memory hot add for assigned lmbs 2017-08-30 2:35 ` Michael Ellerman @ 2017-08-30 14:35 ` Nathan Fontenot 2017-08-30 14:43 ` John Allen 2017-08-31 10:24 ` Michael Ellerman 0 siblings, 2 replies; 10+ messages in thread From: Nathan Fontenot @ 2017-08-30 14:35 UTC (permalink / raw) To: Michael Ellerman, John Allen, linuxppc-dev 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 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] powerpc/pseries: Don't attempt to acquire drc during memory hot add for assigned lmbs 2017-08-30 14:35 ` Nathan Fontenot @ 2017-08-30 14:43 ` John Allen 2017-08-31 10:32 ` Michael Ellerman 2017-08-31 10:24 ` Michael Ellerman 1 sibling, 1 reply; 10+ messages in thread From: John Allen @ 2017-08-30 14:43 UTC (permalink / raw) To: Nathan Fontenot, Michael Ellerman, linuxppc-dev 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 >> > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] powerpc/pseries: Don't attempt to acquire drc during memory hot add for assigned lmbs 2017-08-30 14:43 ` John Allen @ 2017-08-31 10:32 ` Michael Ellerman 0 siblings, 0 replies; 10+ messages in thread From: Michael Ellerman @ 2017-08-31 10:32 UTC (permalink / raw) To: John Allen, Nathan Fontenot, linuxppc-dev 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] powerpc/pseries: Don't attempt to acquire drc during memory hot add for assigned lmbs 2017-08-30 14:35 ` Nathan Fontenot 2017-08-30 14:43 ` John Allen @ 2017-08-31 10:24 ` Michael Ellerman 1 sibling, 0 replies; 10+ messages in thread From: Michael Ellerman @ 2017-08-31 10:24 UTC (permalink / raw) To: Nathan Fontenot, John Allen, linuxppc-dev 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: powerpc/pseries: Don't attempt to acquire drc during memory hot add for assigned lmbs 2017-08-23 17:18 [PATCH] powerpc/pseries: Don't attempt to acquire drc during memory hot add for assigned lmbs John Allen ` (2 preceding siblings ...) 2017-08-30 2:35 ` Michael Ellerman @ 2017-09-01 13:29 ` Michael Ellerman 3 siblings, 0 replies; 10+ messages in thread From: Michael Ellerman @ 2017-09-01 13:29 UTC (permalink / raw) To: John Allen, linuxppc-dev, Nathan Fontenot 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-09-01 13:29 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-08-23 17:18 [PATCH] powerpc/pseries: Don't attempt to acquire drc during memory hot add for assigned lmbs John Allen 2017-08-23 18:54 ` Nathan Fontenot 2017-08-24 10:33 ` Michael Ellerman 2017-08-24 14:46 ` John Allen 2017-08-30 2:35 ` Michael Ellerman 2017-08-30 14:35 ` Nathan Fontenot 2017-08-30 14:43 ` John Allen 2017-08-31 10:32 ` Michael Ellerman 2017-08-31 10:24 ` Michael Ellerman 2017-09-01 13:29 ` Michael Ellerman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).