Nick Child <[email protected]> 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 <[email protected]> >> >> 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 <[email protected]> >> --- >> 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; >> } >> >>
