* [PATCH v06 3/5] migration/memory: Add hotplug READD_MULTIPLE
[not found] <20181015194738.5705.88953.stgit@ltcalpine2-lp9.aus.stglabs.ibm.com>
@ 2018-10-15 19:53 ` Michael Bringmann
2018-10-16 0:39 ` Michael Ellerman
0 siblings, 1 reply; 8+ messages in thread
From: Michael Bringmann @ 2018-10-15 19:53 UTC (permalink / raw)
To: linuxppc-dev
Cc: Thomas Falcon, Michael Bringmann, Juliet Kim, Tyrel Datwyler,
Nathan Fontenot
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(-)
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;
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v06 3/5] migration/memory: Add hotplug READD_MULTIPLE
2018-10-15 19:53 ` [PATCH v06 3/5] migration/memory: Add hotplug READD_MULTIPLE Michael Bringmann
@ 2018-10-16 0:39 ` Michael Ellerman
2018-10-16 14:07 ` Michael Bringmann
2018-10-16 19:57 ` Tyrel Datwyler
0 siblings, 2 replies; 8+ messages in thread
From: Michael Ellerman @ 2018-10-16 0:39 UTC (permalink / raw)
To: Michael Bringmann, linuxppc-dev
Cc: Nathan Fontenot, Michael Bringmann, Juliet Kim, Thomas Falcon,
Tyrel Datwyler
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v06 3/5] migration/memory: Add hotplug READD_MULTIPLE
2018-10-16 0:39 ` Michael Ellerman
@ 2018-10-16 14:07 ` Michael Bringmann
2018-10-16 19:57 ` Tyrel Datwyler
1 sibling, 0 replies; 8+ messages in thread
From: Michael Bringmann @ 2018-10-16 14:07 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev
Cc: Nathan Fontenot, Juliet Kim, Thomas Falcon, Tyrel Datwyler
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.
--
Michael W. Bringmann
Linux Technology Center
IBM Corporation
Tie-Line 363-5196
External: (512) 286-5196
Cell: (512) 466-0650
mwb@linux.vnet.ibm.com
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v06 3/5] migration/memory: Add hotplug READD_MULTIPLE
2018-10-16 0:39 ` Michael Ellerman
2018-10-16 14:07 ` Michael Bringmann
@ 2018-10-16 19:57 ` Tyrel Datwyler
2018-10-16 21:31 ` Michael Bringmann
2018-10-17 0:43 ` Michael Ellerman
1 sibling, 2 replies; 8+ messages in thread
From: Tyrel Datwyler @ 2018-10-16 19:57 UTC (permalink / raw)
To: Michael Ellerman, Michael Bringmann, linuxppc-dev
Cc: Nathan Fontenot, Juliet Kim, Thomas Falcon
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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v06 3/5] migration/memory: Add hotplug READD_MULTIPLE
2018-10-16 19:57 ` Tyrel Datwyler
@ 2018-10-16 21:31 ` Michael Bringmann
2018-10-17 0:48 ` Michael Ellerman
2018-10-17 0:43 ` Michael Ellerman
1 sibling, 1 reply; 8+ messages in thread
From: Michael Bringmann @ 2018-10-16 21:31 UTC (permalink / raw)
To: Tyrel Datwyler, Michael Ellerman, linuxppc-dev
Cc: Nathan Fontenot, Juliet Kim, Thomas Falcon
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
--
Michael W. Bringmann
Linux Technology Center
IBM Corporation
Tie-Line 363-5196
External: (512) 286-5196
Cell: (512) 466-0650
mwb@linux.vnet.ibm.com
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v06 3/5] migration/memory: Add hotplug READD_MULTIPLE
2018-10-16 19:57 ` Tyrel Datwyler
2018-10-16 21:31 ` Michael Bringmann
@ 2018-10-17 0:43 ` Michael Ellerman
1 sibling, 0 replies; 8+ messages in thread
From: Michael Ellerman @ 2018-10-17 0:43 UTC (permalink / raw)
To: Tyrel Datwyler, Michael Bringmann, linuxppc-dev
Cc: Nathan Fontenot, Juliet Kim, Thomas Falcon
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v06 3/5] migration/memory: Add hotplug READD_MULTIPLE
2018-10-16 21:31 ` Michael Bringmann
@ 2018-10-17 0:48 ` Michael Ellerman
2018-10-17 15:12 ` Michael Bringmann
0 siblings, 1 reply; 8+ messages in thread
From: Michael Ellerman @ 2018-10-17 0:48 UTC (permalink / raw)
To: Michael Bringmann, Tyrel Datwyler, linuxppc-dev
Cc: Nathan Fontenot, Juliet Kim, Thomas Falcon
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v06 3/5] migration/memory: Add hotplug READD_MULTIPLE
2018-10-17 0:48 ` Michael Ellerman
@ 2018-10-17 15:12 ` Michael Bringmann
0 siblings, 0 replies; 8+ messages in thread
From: Michael Bringmann @ 2018-10-17 15:12 UTC (permalink / raw)
To: Michael Ellerman, Tyrel Datwyler, linuxppc-dev
Cc: Nathan Fontenot, Juliet Kim, Thomas Falcon
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.
--
Michael W. Bringmann
Linux Technology Center
IBM Corporation
Tie-Line 363-5196
External: (512) 286-5196
Cell: (512) 466-0650
mwb@linux.vnet.ibm.com
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-10-17 15:14 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20181015194738.5705.88953.stgit@ltcalpine2-lp9.aus.stglabs.ibm.com>
2018-10-15 19:53 ` [PATCH v06 3/5] migration/memory: Add hotplug READD_MULTIPLE Michael Bringmann
2018-10-16 0:39 ` Michael Ellerman
2018-10-16 14:07 ` Michael Bringmann
2018-10-16 19:57 ` Tyrel Datwyler
2018-10-16 21:31 ` Michael Bringmann
2018-10-17 0:48 ` Michael Ellerman
2018-10-17 15:12 ` Michael Bringmann
2018-10-17 0:43 ` 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).