[PATCH v06 3/5] migration/memory: Add hotplug READD_MULTIPLE

[PATCH v06 3/5] migration/memory: Add hotplug READD_MULTIPLE

[Michael Bringmann]

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;

Re: [PATCH v06 3/5] migration/memory: Add hotplug READD_MULTIPLE

[Michael Ellerman]

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

Re: [PATCH v06 3/5] migration/memory: Add hotplug READD_MULTIPLE

[Michael Bringmann]

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

Re: [PATCH v06 3/5] migration/memory: Add hotplug READD_MULTIPLE

[Tyrel Datwyler]

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
> 

Re: [PATCH v06 3/5] migration/memory: Add hotplug READD_MULTIPLE

[Michael Bringmann]

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

Re: [PATCH v06 3/5] migration/memory: Add hotplug READD_MULTIPLE

[Michael Ellerman]

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

Re: [PATCH v06 3/5] migration/memory: Add hotplug READD_MULTIPLE

[Michael Ellerman]

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

Re: [PATCH v06 3/5] migration/memory: Add hotplug READD_MULTIPLE

[Michael Bringmann]

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