[PATCH v2 0/2] memory-hotplug: cleanup register_memory_resource()

[PATCH v2 0/2] memory-hotplug: cleanup register_memory_resource()

[Vitaly Kuznetsov]

This series is a successor for the previously sent "[PATCH] memory-hotplug:
 don't BUG() in register_memory_resource()". Changes since it are:
- Use ERR_PTR/PTR_ERR/IS_ERR() [David Rientjes]
- Add "memory-hotplug: keep the request_resource() error code" patch
  [Andrew Morton]

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: David Rientjes <rientjes@google.com>
Cc: Tang Chen <tangchen@cn.fujitsu.com>
Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Cc: Xishi Qiu <qiuxishi@huawei.com>
Cc: Sheng Yong <shengyong1@huawei.com>
Cc: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: David Vrabel <david.vrabel@citrix.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Len Brown <lenb@kernel.org>

Vitaly Kuznetsov (2):
  memory-hotplug: don't BUG() in register_memory_resource()
  memory-hotplug: keep the request_resource() error code

 drivers/acpi/acpi_memhotplug.c |  4 ++--
 mm/memory_hotplug.c            | 13 ++++++++-----
 2 files changed, 10 insertions(+), 7 deletions(-)

-- 
2.4.3

[PATCH v2 1/2] memory-hotplug: don't BUG() in register_memory_resource()

[Vitaly Kuznetsov]

Out of memory condition is not a bug and while we can't add new memory in
such case crashing the system seems wrong. Propagating the return value
from register_memory_resource() requires interface change.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: David Rientjes <rientjes@google.com>
Cc: Tang Chen <tangchen@cn.fujitsu.com>
Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Cc: Xishi Qiu <qiuxishi@huawei.com>
Cc: Sheng Yong <shengyong1@huawei.com>
Cc: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: David Vrabel <david.vrabel@citrix.com>
Cc: Igor Mammedov <imammedo@redhat.com>
---
Changes since v1:
- Use ERR_PTR/PTR_ERR/IS_ERR() [David Rientjes]
---
 mm/memory_hotplug.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index a042a9d..92f9595 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -131,7 +131,8 @@ static struct resource *register_memory_resource(u64 start, u64 size)
 {
 	struct resource *res;
 	res = kzalloc(sizeof(struct resource), GFP_KERNEL);
-	BUG_ON(!res);
+	if (!res)
+		return ERR_PTR(-ENOMEM);
 
 	res->name = "System RAM";
 	res->start = start;
@@ -140,7 +141,7 @@ static struct resource *register_memory_resource(u64 start, u64 size)
 	if (request_resource(&iomem_resource, res) < 0) {
 		pr_debug("System RAM resource %pR cannot be added\n", res);
 		kfree(res);
-		res = NULL;
+		return ERR_PTR(-EEXIST);
 	}
 	return res;
 }
@@ -1312,8 +1313,8 @@ int __ref add_memory(int nid, u64 start, u64 size)
 	int ret;
 
 	res = register_memory_resource(start, size);
-	if (!res)
-		return -EEXIST;
+	if (IS_ERR(res))
+		return PTR_ERR(res);
 
 	ret = add_memory_resource(nid, res);
 	if (ret < 0)
-- 
2.4.3

[PATCH v2 2/2] memory-hotplug: keep the request_resource() error code

[Vitaly Kuznetsov]

Don't overwrite the request_resource() return value with -EEXIST in
register_memory_resource(), just propagate the return value. As we return
-EBUSY instead of -EEXIST when the desired resource is already occupied
now we need to adapt acpi_memory_enable_device(). -EBUSY is currently the
only possible error returned by request_resource() so this is just a
cleanup, no functional changes intended.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: David Rientjes <rientjes@google.com>
Cc: Tang Chen <tangchen@cn.fujitsu.com>
Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: David Vrabel <david.vrabel@citrix.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Len Brown <lenb@kernel.org>
Suggested-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 drivers/acpi/acpi_memhotplug.c | 4 ++--
 mm/memory_hotplug.c            | 6 ++++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
index 6b0d3ef..e367e4b 100644
--- a/drivers/acpi/acpi_memhotplug.c
+++ b/drivers/acpi/acpi_memhotplug.c
@@ -232,10 +232,10 @@ static int acpi_memory_enable_device(struct acpi_memory_device *mem_device)
 
 		/*
 		 * If the memory block has been used by the kernel, add_memory()
-		 * returns -EEXIST. If add_memory() returns the other error, it
+		 * returns -EBUSY. If add_memory() returns the other error, it
 		 * means that this memory block is not used by the kernel.
 		 */
-		if (result && result != -EEXIST)
+		if (result && result != -EBUSY)
 			continue;
 
 		result = acpi_bind_memory_blocks(info, mem_device->device);
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 92f9595..07eab2c 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -130,6 +130,7 @@ void mem_hotplug_done(void)
 static struct resource *register_memory_resource(u64 start, u64 size)
 {
 	struct resource *res;
+	int ret;
 	res = kzalloc(sizeof(struct resource), GFP_KERNEL);
 	if (!res)
 		return ERR_PTR(-ENOMEM);
@@ -138,10 +139,11 @@ static struct resource *register_memory_resource(u64 start, u64 size)
 	res->start = start;
 	res->end = start + size - 1;
 	res->flags = IORESOURCE_MEM | IORESOURCE_BUSY;
-	if (request_resource(&iomem_resource, res) < 0) {
+	ret = request_resource(&iomem_resource, res);
+	if (ret < 0) {
 		pr_debug("System RAM resource %pR cannot be added\n", res);
 		kfree(res);
-		return ERR_PTR(-EEXIST);
+		return ERR_PTR(ret);
 	}
 	return res;
 }
-- 
2.4.3

Re: [PATCH v2 2/2] memory-hotplug: keep the request_resource() error code

[Rafael J. Wysocki]

On Monday, January 04, 2016 05:17:31 PM Vitaly Kuznetsov wrote:
> Don't overwrite the request_resource() return value with -EEXIST in
> register_memory_resource(), just propagate the return value. As we return
> -EBUSY instead of -EEXIST when the desired resource is already occupied
> now we need to adapt acpi_memory_enable_device(). -EBUSY is currently the
> only possible error returned by request_resource() so this is just a
> cleanup, no functional changes intended.
> 
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Tang Chen <tangchen@cn.fujitsu.com>
> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: David Vrabel <david.vrabel@citrix.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Len Brown <lenb@kernel.org>
> Suggested-by: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

for the ACPI part and I'm assumig that this will go in through the mm tree.

> ---
>  drivers/acpi/acpi_memhotplug.c | 4 ++--
>  mm/memory_hotplug.c            | 6 ++++--
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
> index 6b0d3ef..e367e4b 100644
> --- a/drivers/acpi/acpi_memhotplug.c
> +++ b/drivers/acpi/acpi_memhotplug.c
> @@ -232,10 +232,10 @@ static int acpi_memory_enable_device(struct acpi_memory_device *mem_device)
>  
>  		/*
>  		 * If the memory block has been used by the kernel, add_memory()
> -		 * returns -EEXIST. If add_memory() returns the other error, it
> +		 * returns -EBUSY. If add_memory() returns the other error, it
>  		 * means that this memory block is not used by the kernel.
>  		 */
> -		if (result && result != -EEXIST)
> +		if (result && result != -EBUSY)
>  			continue;
>  
>  		result = acpi_bind_memory_blocks(info, mem_device->device);
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 92f9595..07eab2c 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -130,6 +130,7 @@ void mem_hotplug_done(void)
>  static struct resource *register_memory_resource(u64 start, u64 size)
>  {
>  	struct resource *res;
> +	int ret;
>  	res = kzalloc(sizeof(struct resource), GFP_KERNEL);
>  	if (!res)
>  		return ERR_PTR(-ENOMEM);
> @@ -138,10 +139,11 @@ static struct resource *register_memory_resource(u64 start, u64 size)
>  	res->start = start;
>  	res->end = start + size - 1;
>  	res->flags = IORESOURCE_MEM | IORESOURCE_BUSY;
> -	if (request_resource(&iomem_resource, res) < 0) {
> +	ret = request_resource(&iomem_resource, res);
> +	if (ret < 0) {
>  		pr_debug("System RAM resource %pR cannot be added\n", res);
>  		kfree(res);
> -		return ERR_PTR(-EEXIST);
> +		return ERR_PTR(ret);
>  	}
>  	return res;
>  }
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v2 1/2] memory-hotplug: don't BUG() in register_memory_resource()

[David Rientjes]

On Mon, 4 Jan 2016, Vitaly Kuznetsov wrote:

> Out of memory condition is not a bug and while we can't add new memory in
> such case crashing the system seems wrong. Propagating the return value
> from register_memory_resource() requires interface change.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Tang Chen <tangchen@cn.fujitsu.com>
> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> Cc: Xishi Qiu <qiuxishi@huawei.com>
> Cc: Sheng Yong <shengyong1@huawei.com>
> Cc: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: David Vrabel <david.vrabel@citrix.com>
> Cc: Igor Mammedov <imammedo@redhat.com>

Acked-by: David Rientjes <rientjes@google.com>

Re: [PATCH v2 2/2] memory-hotplug: keep the request_resource() error code

[David Rientjes]

On Mon, 4 Jan 2016, Vitaly Kuznetsov wrote:

> diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
> index 6b0d3ef..e367e4b 100644
> --- a/drivers/acpi/acpi_memhotplug.c
> +++ b/drivers/acpi/acpi_memhotplug.c
> @@ -232,10 +232,10 @@ static int acpi_memory_enable_device(struct acpi_memory_device *mem_device)
>  
>  		/*
>  		 * If the memory block has been used by the kernel, add_memory()
> -		 * returns -EEXIST. If add_memory() returns the other error, it
> +		 * returns -EBUSY. If add_memory() returns the other error, it
>  		 * means that this memory block is not used by the kernel.
>  		 */
> -		if (result && result != -EEXIST)
> +		if (result && result != -EBUSY)
>  			continue;
>  
>  		result = acpi_bind_memory_blocks(info, mem_device->device);
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 92f9595..07eab2c 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -130,6 +130,7 @@ void mem_hotplug_done(void)
>  static struct resource *register_memory_resource(u64 start, u64 size)
>  {
>  	struct resource *res;
> +	int ret;
>  	res = kzalloc(sizeof(struct resource), GFP_KERNEL);
>  	if (!res)
>  		return ERR_PTR(-ENOMEM);
> @@ -138,10 +139,11 @@ static struct resource *register_memory_resource(u64 start, u64 size)
>  	res->start = start;
>  	res->end = start + size - 1;
>  	res->flags = IORESOURCE_MEM | IORESOURCE_BUSY;
> -	if (request_resource(&iomem_resource, res) < 0) {
> +	ret = request_resource(&iomem_resource, res);
> +	if (ret < 0) {
>  		pr_debug("System RAM resource %pR cannot be added\n", res);
>  		kfree(res);
> -		return ERR_PTR(-EEXIST);
> +		return ERR_PTR(ret);
>  	}
>  	return res;
>  }

The result of this change is that add_memory() returns -EBUSY instead of 
-EEXIST for overlapping ranges.  This patch breaks hv_mem_hot_add() since 
it strictly uses a return value of -EEXIST to indicate a non-transient 
failure, so NACK.  It also changes the return value of both the "probe" 
and "dlpar" (on ppc) userspace triggers, which I could imagine is tested 
from existing userspace scripts.

Re: [PATCH v2 2/2] memory-hotplug: keep the request_resource() error code

[Vitaly Kuznetsov]

David Rientjes <rientjes@google.com> writes:

> On Mon, 4 Jan 2016, Vitaly Kuznetsov wrote:
>
>> diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
>> index 6b0d3ef..e367e4b 100644
>> --- a/drivers/acpi/acpi_memhotplug.c
>> +++ b/drivers/acpi/acpi_memhotplug.c
>> @@ -232,10 +232,10 @@ static int acpi_memory_enable_device(struct acpi_memory_device *mem_device)
>>  
>>  		/*
>>  		 * If the memory block has been used by the kernel, add_memory()
>> -		 * returns -EEXIST. If add_memory() returns the other error, it
>> +		 * returns -EBUSY. If add_memory() returns the other error, it
>>  		 * means that this memory block is not used by the kernel.
>>  		 */
>> -		if (result && result != -EEXIST)
>> +		if (result && result != -EBUSY)
>>  			continue;
>>  
>>  		result = acpi_bind_memory_blocks(info, mem_device->device);
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index 92f9595..07eab2c 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -130,6 +130,7 @@ void mem_hotplug_done(void)
>>  static struct resource *register_memory_resource(u64 start, u64 size)
>>  {
>>  	struct resource *res;
>> +	int ret;
>>  	res = kzalloc(sizeof(struct resource), GFP_KERNEL);
>>  	if (!res)
>>  		return ERR_PTR(-ENOMEM);
>> @@ -138,10 +139,11 @@ static struct resource *register_memory_resource(u64 start, u64 size)
>>  	res->start = start;
>>  	res->end = start + size - 1;
>>  	res->flags = IORESOURCE_MEM | IORESOURCE_BUSY;
>> -	if (request_resource(&iomem_resource, res) < 0) {
>> +	ret = request_resource(&iomem_resource, res);
>> +	if (ret < 0) {
>>  		pr_debug("System RAM resource %pR cannot be added\n", res);
>>  		kfree(res);
>> -		return ERR_PTR(-EEXIST);
>> +		return ERR_PTR(ret);
>>  	}
>>  	return res;
>>  }
>
> The result of this change is that add_memory() returns -EBUSY instead of 
> -EEXIST for overlapping ranges.  This patch breaks hv_mem_hot_add() since 
> it strictly uses a return value of -EEXIST to indicate a non-transient 
> failure, so NACK.

While this is shameful but fixable ...

> It also changes the return value of both the "probe" 
> and "dlpar" (on ppc) userspace triggers, which I could imagine is tested 
> from existing userspace scripts.

this is probably a show-stopper as we're bound by "we don't break
userspace" rule (and it's not worth it anyway). Thanks for pointing this
out!

Andrew, please drop this patch from your queue permanently. The patch 1
of the series is worthwhile (and is acked by David).

Thanks,

-- 
  Vitaly