| Message ID | 20231114-pseries-memhp-fixes-v1-2-fb8f2bb7c557@linux.ibm.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
| Series | powerpc/pseries/memhp: Fix minor bugs and improve error logging | expand |
Hi Nathan, Patches 1 and 3 LGTM Regarding this patch, dlpar_memory_remove_by_count() calls dlpar_add_lmb() and does not free drc on add error. dlpar_add_lmb() is called here in error recovery so probably not a big deal. This is all new code to me but it looks like if the requested number of lmbs cannot be removed then it attempts to add back the ones that were successfully removed. So if you cannot add an lmb that WAS successfully removed, it seems sane to also release the drc. On 11/14/23 11:01, Nathan Lynch via B4 Relay wrote: > From: Nathan Lynch <nathanl@linux.ibm.com> > > Callers of dlpar_add_lmb() are responsible for first acquiring the DRC > and releasing it if dlpar_add_lmb() fails. > > However, dlpar_add_lmb() performs a dlpar_release_drc() in one error > branch. There is no corresponding dlpar_acquire_drc() in the > function, nor is there any stated justification. None of the other > error paths in dlpar_add_lmb() release the DRC. > > This is a potential source of redundant attempts to release DRCs, > which is likely benign, but is confusing and inconsistent. Remove it. > > Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com> > --- > arch/powerpc/platforms/pseries/hotplug-memory.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c > index 6f2eebae7bee..ba883c1b9f6d 100644 > --- a/arch/powerpc/platforms/pseries/hotplug-memory.c > +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c > @@ -575,7 +575,6 @@ static int dlpar_add_lmb(struct drmem_lmb *lmb) > > rc = update_lmb_associativity_index(lmb); > if (rc) { > - dlpar_release_drc(lmb->drc_index); > return rc; > } > >
Nick Child <nnac123@linux.ibm.com> writes: > Hi Nathan, > Patches 1 and 3 LGTM thanks. > Regarding this patch, dlpar_memory_remove_by_count() calls > dlpar_add_lmb() and does not free drc on add error. > dlpar_add_lmb() is called here in error recovery so probably > not a big deal. > > This is all new code to me but it looks like if the requested > number of lmbs cannot be removed then it attempts to add back > the ones that were successfully removed. So if you cannot add > an lmb that WAS successfully removed, it seems sane to also > release the drc. Maybe I'll drop this one for now and turn my attention to removing all the high-level rollback logic in this code. There's no reliable way to accomplish what it's trying to do (i.e. restore the original quantity of LMBs) and it just complicates things. > On 11/14/23 11:01, Nathan Lynch via B4 Relay wrote: >> From: Nathan Lynch <nathanl@linux.ibm.com> >> >> Callers of dlpar_add_lmb() are responsible for first acquiring the DRC >> and releasing it if dlpar_add_lmb() fails. >> >> However, dlpar_add_lmb() performs a dlpar_release_drc() in one error >> branch. There is no corresponding dlpar_acquire_drc() in the >> function, nor is there any stated justification. None of the other >> error paths in dlpar_add_lmb() release the DRC. >> >> This is a potential source of redundant attempts to release DRCs, >> which is likely benign, but is confusing and inconsistent. Remove it. >> >> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com> >> --- >> arch/powerpc/platforms/pseries/hotplug-memory.c | 1 - >> 1 file changed, 1 deletion(-) >> >> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c >> index 6f2eebae7bee..ba883c1b9f6d 100644 >> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c >> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c >> @@ -575,7 +575,6 @@ static int dlpar_add_lmb(struct drmem_lmb *lmb) >> >> rc = update_lmb_associativity_index(lmb); >> if (rc) { >> - dlpar_release_drc(lmb->drc_index); >> return rc; >> } >> >>
> On Nov 28, 2023, at 9:21 AM, Nathan Lynch <nathanl@linux.ibm.com> wrote: > > Nick Child <nnac123@linux.ibm.com> writes: >> Hi Nathan, >> Patches 1 and 3 LGTM > > thanks. > >> Regarding this patch, dlpar_memory_remove_by_count() calls >> dlpar_add_lmb() and does not free drc on add error. >> dlpar_add_lmb() is called here in error recovery so probably >> not a big deal. >> >> This is all new code to me but it looks like if the requested >> number of lmbs cannot be removed then it attempts to add back >> the ones that were successfully removed. So if you cannot add >> an lmb that WAS successfully removed, it seems sane to also >> release the drc. > > Maybe I'll drop this one for now and turn my attention to removing all > the high-level rollback logic in this code. There's no reliable way to > accomplish what it's trying to do (i.e. restore the original quantity of > LMBs) and it just complicates things. Removing all of the rollback code is a wonderful idea.
diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c index 6f2eebae7bee..ba883c1b9f6d 100644 --- a/arch/powerpc/platforms/pseries/hotplug-memory.c +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c @@ -575,7 +575,6 @@ static int dlpar_add_lmb(struct drmem_lmb *lmb) rc = update_lmb_associativity_index(lmb); if (rc) { - dlpar_release_drc(lmb->drc_index); return rc; }