| Message ID | f38687fe-7d46-830b-e4bb-ef78a7f99a65@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | powerpc/migration: Affinity fix for memory | expand |
Michael Bringmann <mwb@linux.vnet.ibm.com> writes: > diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c > index 2b796da..9c76345 100644 > --- a/arch/powerpc/platforms/pseries/hotplug-memory.c > +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c > @@ -541,6 +549,23 @@ static int dlpar_memory_readd_by_index(u32 drc_index) > return rc; > } > > +static int dlpar_memory_readd_multiple(void) > +{ > + struct drmem_lmb *lmb; > + int rc; > + > + pr_info("Attempting to update multiple LMBs\n"); > + > + for_each_drmem_lmb(lmb) { > + if (drmem_lmb_update(lmb)) { > + rc = dlpar_memory_readd_helper(lmb); > + drmem_remove_lmb_update(lmb); > + } > + } > + > + return rc; > +} This leaves rc potentially uninitialised. What should the result be in that case, -EINVAL ? cheers
On 10/15/2018 07:39 PM, Michael Ellerman wrote: > Michael Bringmann <mwb@linux.vnet.ibm.com> writes: >> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c >> index 2b796da..9c76345 100644 >> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c >> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c >> @@ -541,6 +549,23 @@ static int dlpar_memory_readd_by_index(u32 drc_index) >> return rc; >> } >> >> +static int dlpar_memory_readd_multiple(void) >> +{ >> + struct drmem_lmb *lmb; >> + int rc; >> + >> + pr_info("Attempting to update multiple LMBs\n"); >> + >> + for_each_drmem_lmb(lmb) { >> + if (drmem_lmb_update(lmb)) { >> + rc = dlpar_memory_readd_helper(lmb); >> + drmem_remove_lmb_update(lmb); >> + } >> + } >> + >> + return rc; >> +} > > This leaves rc potentially uninitialised. > > What should the result be in that case, -EINVAL ? I will force it to be zero (0). Failure to find anything to update is not an error. > > cheers Thanks.
On 10/15/2018 05:39 PM, Michael Ellerman wrote: > Michael Bringmann <mwb@linux.vnet.ibm.com> writes: >> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c >> index 2b796da..9c76345 100644 >> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c >> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c >> @@ -541,6 +549,23 @@ static int dlpar_memory_readd_by_index(u32 drc_index) >> return rc; >> } >> >> +static int dlpar_memory_readd_multiple(void) >> +{ >> + struct drmem_lmb *lmb; >> + int rc; >> + >> + pr_info("Attempting to update multiple LMBs\n"); >> + >> + for_each_drmem_lmb(lmb) { >> + if (drmem_lmb_update(lmb)) { >> + rc = dlpar_memory_readd_helper(lmb); >> + drmem_remove_lmb_update(lmb); >> + } >> + } >> + >> + return rc; >> +} > > This leaves rc potentially uninitialised. > > What should the result be in that case, -EINVAL ? On another note if there are multiple LMBs to update the value of rc only reflects the final dlpar_memory_readd_helper() call. -Tyrel > > cheers >
On 10/16/2018 02:57 PM, Tyrel Datwyler wrote: > On 10/15/2018 05:39 PM, Michael Ellerman wrote: >> Michael Bringmann <mwb@linux.vnet.ibm.com> writes: >>> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c >>> index 2b796da..9c76345 100644 >>> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c >>> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c >>> @@ -541,6 +549,23 @@ static int dlpar_memory_readd_by_index(u32 drc_index) >>> return rc; >>> } >>> >>> +static int dlpar_memory_readd_multiple(void) >>> +{ >>> + struct drmem_lmb *lmb; >>> + int rc; >>> + >>> + pr_info("Attempting to update multiple LMBs\n"); >>> + >>> + for_each_drmem_lmb(lmb) { >>> + if (drmem_lmb_update(lmb)) { >>> + rc = dlpar_memory_readd_helper(lmb); >>> + drmem_remove_lmb_update(lmb); >>> + } >>> + } >>> + >>> + return rc; >>> +} >> >> This leaves rc potentially uninitialised. >> >> What should the result be in that case, -EINVAL ? > > On another note if there are multiple LMBs to update the value of rc only reflects the final dlpar_memory_readd_helper() call. Correct. But that is what happens when we compress common code between two disparate uses i.e. updating memory association after a migration event with no reporting mechanism other than the console log, vs re-adding a single LMB by index for the purposes of DLPAR / drmgr. I could discard the return value from dlpar_memory_readd_helper entirely in this function and just return 0, but in my experience, once errors start to occur in memory dlpar ops, they tend to keep on occurring, so I was returning the last one. We could also make the code smart enough to capture and return the first/last non-zero return code. I didn't believe that the frequency of errors for this operation warranted the overhead. > > -Tyrel Michael
Tyrel Datwyler <tyreld@linux.vnet.ibm.com> writes: > On 10/15/2018 05:39 PM, Michael Ellerman wrote: >> Michael Bringmann <mwb@linux.vnet.ibm.com> writes: >>> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c >>> index 2b796da..9c76345 100644 >>> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c >>> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c >>> @@ -541,6 +549,23 @@ static int dlpar_memory_readd_by_index(u32 drc_index) >>> return rc; >>> } >>> >>> +static int dlpar_memory_readd_multiple(void) >>> +{ >>> + struct drmem_lmb *lmb; >>> + int rc; >>> + >>> + pr_info("Attempting to update multiple LMBs\n"); >>> + >>> + for_each_drmem_lmb(lmb) { >>> + if (drmem_lmb_update(lmb)) { >>> + rc = dlpar_memory_readd_helper(lmb); >>> + drmem_remove_lmb_update(lmb); >>> + } >>> + } >>> + >>> + return rc; >>> +} >> >> This leaves rc potentially uninitialised. >> >> What should the result be in that case, -EINVAL ? > > On another note if there are multiple LMBs to update the value of rc > only reflects the final dlpar_memory_readd_helper() call. Good point. cheers
Michael Bringmann <mwb@linux.vnet.ibm.com> writes: > On 10/16/2018 02:57 PM, Tyrel Datwyler wrote: >> On 10/15/2018 05:39 PM, Michael Ellerman wrote: >>> Michael Bringmann <mwb@linux.vnet.ibm.com> writes: >>>> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c >>>> index 2b796da..9c76345 100644 >>>> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c >>>> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c >>>> @@ -541,6 +549,23 @@ static int dlpar_memory_readd_by_index(u32 drc_index) >>>> return rc; >>>> } >>>> >>>> +static int dlpar_memory_readd_multiple(void) >>>> +{ >>>> + struct drmem_lmb *lmb; >>>> + int rc; >>>> + >>>> + pr_info("Attempting to update multiple LMBs\n"); >>>> + >>>> + for_each_drmem_lmb(lmb) { >>>> + if (drmem_lmb_update(lmb)) { >>>> + rc = dlpar_memory_readd_helper(lmb); >>>> + drmem_remove_lmb_update(lmb); >>>> + } >>>> + } >>>> + >>>> + return rc; >>>> +} >>> >>> This leaves rc potentially uninitialised. >>> >>> What should the result be in that case, -EINVAL ? >> >> On another note if there are multiple LMBs to update the value of rc only reflects the final dlpar_memory_readd_helper() call. > > Correct. But that is what happens when we compress common code > between two disparate uses i.e. updating memory association after > a migration event with no reporting mechanism other than the console > log, vs re-adding a single LMB by index for the purposes of DLPAR / drmgr. > > I could discard the return value from dlpar_memory_readd_helper entirely > in this function and just return 0, but in my experience, once errors start > to occur in memory dlpar ops, they tend to keep on occurring, so I was > returning the last one. We could also make the code smart enough to > capture and return the first/last non-zero return code. I didn't believe > that the frequency of errors for this operation warranted the overhead. The actual error value is probably not very relevant. But dropping errors entirely is almost always a bad idea. So I think you should at least return an error if any error occurred, that way at least an error will be returned up to the caller(s). Something like: int rc; rc = 0; for_each_drmem_lmb(lmb) { if (drmem_lmb_update(lmb)) { rc |= dlpar_memory_readd_helper(lmb); drmem_remove_lmb_update(lmb); } } if (rc) return -EIO; cheers
On 10/16/2018 07:48 PM, Michael Ellerman wrote: > Michael Bringmann <mwb@linux.vnet.ibm.com> writes: >> On 10/16/2018 02:57 PM, Tyrel Datwyler wrote: >>> On 10/15/2018 05:39 PM, Michael Ellerman wrote: >>>> Michael Bringmann <mwb@linux.vnet.ibm.com> writes: >>>>> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c >>>>> index 2b796da..9c76345 100644 >>>>> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c >>>>> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c >>>>> @@ -541,6 +549,23 @@ static int dlpar_memory_readd_by_index(u32 drc_index) >>>>> return rc; >>>>> } >>>>> >>>>> +static int dlpar_memory_readd_multiple(void) >>>>> +{ >>>>> + struct drmem_lmb *lmb; >>>>> + int rc; >>>>> + >>>>> + pr_info("Attempting to update multiple LMBs\n"); >>>>> + >>>>> + for_each_drmem_lmb(lmb) { >>>>> + if (drmem_lmb_update(lmb)) { >>>>> + rc = dlpar_memory_readd_helper(lmb); >>>>> + drmem_remove_lmb_update(lmb); >>>>> + } >>>>> + } >>>>> + >>>>> + return rc; >>>>> +} >>>> >>>> This leaves rc potentially uninitialised. >>>> >>>> What should the result be in that case, -EINVAL ? >>> >>> On another note if there are multiple LMBs to update the value of rc only reflects the final dlpar_memory_readd_helper() call. >> >> Correct. But that is what happens when we compress common code >> between two disparate uses i.e. updating memory association after >> a migration event with no reporting mechanism other than the console >> log, vs re-adding a single LMB by index for the purposes of DLPAR / drmgr. >> >> I could discard the return value from dlpar_memory_readd_helper entirely >> in this function and just return 0, but in my experience, once errors start >> to occur in memory dlpar ops, they tend to keep on occurring, so I was >> returning the last one. We could also make the code smart enough to >> capture and return the first/last non-zero return code. I didn't believe >> that the frequency of errors for this operation warranted the overhead. > > The actual error value is probably not very relevant. > > But dropping errors entirely is almost always a bad idea. > > So I think you should at least return an error if any error occurred, > that way at least an error will be returned up to the caller(s). > > Something like: > > int rc; > > rc = 0; > for_each_drmem_lmb(lmb) { > if (drmem_lmb_update(lmb)) { > rc |= dlpar_memory_readd_helper(lmb); > drmem_remove_lmb_update(lmb); > } > } > > if (rc) > return -EIO; Okay. > > cheers > Thanks.
diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h index 0183e95..cc00451 100644 --- a/arch/powerpc/include/asm/rtas.h +++ b/arch/powerpc/include/asm/rtas.h @@ -333,6 +333,7 @@ struct pseries_hp_errorlog { #define PSERIES_HP_ELOG_ACTION_ADD 1 #define PSERIES_HP_ELOG_ACTION_REMOVE 2 #define PSERIES_HP_ELOG_ACTION_READD 3 +#define PSERIES_HP_ELOG_ACTION_READD_MULTIPLE 4 #define PSERIES_HP_ELOG_ID_DRC_NAME 1 #define PSERIES_HP_ELOG_ID_DRC_INDEX 2 diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c index 2b796da..9c76345 100644 --- a/arch/powerpc/platforms/pseries/hotplug-memory.c +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c @@ -507,6 +507,19 @@ static int dlpar_memory_remove_by_index(u32 drc_index) return rc; } +static int dlpar_memory_readd_helper(struct drmem_lmb *lmb) +{ + int rc; + + rc = dlpar_remove_lmb(lmb); + if (!rc) { + rc = dlpar_add_lmb(lmb); + if (rc) + dlpar_release_drc(lmb->drc_index); + } + return rc; +} + static int dlpar_memory_readd_by_index(u32 drc_index) { struct drmem_lmb *lmb; @@ -519,12 +532,7 @@ static int dlpar_memory_readd_by_index(u32 drc_index) for_each_drmem_lmb(lmb) { if (lmb->drc_index == drc_index) { lmb_found = 1; - rc = dlpar_remove_lmb(lmb); - if (!rc) { - rc = dlpar_add_lmb(lmb); - if (rc) - dlpar_release_drc(lmb->drc_index); - } + rc = dlpar_memory_readd_helper(lmb); break; } } @@ -541,6 +549,23 @@ static int dlpar_memory_readd_by_index(u32 drc_index) return rc; } +static int dlpar_memory_readd_multiple(void) +{ + struct drmem_lmb *lmb; + int rc; + + pr_info("Attempting to update multiple LMBs\n"); + + for_each_drmem_lmb(lmb) { + if (drmem_lmb_update(lmb)) { + rc = dlpar_memory_readd_helper(lmb); + drmem_remove_lmb_update(lmb); + } + } + + return rc; +} + static int dlpar_memory_remove_by_ic(u32 lmbs_to_remove, u32 drc_index) { struct drmem_lmb *lmb, *start_lmb, *end_lmb; @@ -641,6 +666,10 @@ static int dlpar_memory_readd_by_index(u32 drc_index) { return -EOPNOTSUPP; } +static int dlpar_memory_readd_multiple(void) +{ + return -EOPNOTSUPP; +} static int dlpar_memory_remove_by_ic(u32 lmbs_to_remove, u32 drc_index) { @@ -918,6 +947,9 @@ int dlpar_memory(struct pseries_hp_errorlog *hp_elog) drc_index = hp_elog->_drc_u.drc_index; rc = dlpar_memory_readd_by_index(drc_index); break; + case PSERIES_HP_ELOG_ACTION_READD_MULTIPLE: + rc = dlpar_memory_readd_multiple(); + break; default: pr_err("Invalid action (%d) specified\n", hp_elog->action); rc = -EINVAL;
migration/memory: This patch adds a new pseries hotplug action for CPU and memory operations, PSERIES_HP_ELOG_ACTION_READD_MULTIPLE. This is a variant of the READD operation which performs the action upon multiple instances of the resource at one time. The operation is to be triggered by device-tree analysis of updates by RTAS events analyzed by 'migation_store' during post-migration processing. It will be used for memory updates, initially. Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com> --- Changes in v05: -- Provide dlpar_memory_readd_helper routine to compress some common code Changes in v04: -- Move init of 'lmb->internal_flags' in init_drmem_v2_lmbs to previous patch. -- Pull in implementation of dlpar_memory_readd_multiple() to go with operation flag. --- arch/powerpc/include/asm/rtas.h | 1 + arch/powerpc/platforms/pseries/hotplug-memory.c | 44 ++++++++++++++++++++--- 2 files changed, 39 insertions(+), 6 deletions(-)