Drivers: hv: hv_balloon: Fix a deadlock in the hot-add path.

PATCH In review K. Y. Srinivasan 3 participants 12 messages Dec 5, 2014 – Dec 14, 2014

Subsystems (3)

Memory management Driver core Hyper-V/Azure core and drivers

lore

Share & tools

lore

K. Y. Srinivasan

author

2014-12-05 23:31 UTC #0 lore raw

cover Drivers: hv: hv_balloon: Fix a deadlock in the hot-add path.

Fix a deadlock in the hot-add path in the Hyper-V balloon driver.

K. Y. Srinivasan (2):
Drivers: base: core: Export functions to lock/unlock device hotplug
lock
Drivers: hv: balloon: Fix the deadlock issue in the memory hot-add
code

 drivers/base/core.c     |    2 ++
 drivers/hv/hv_balloon.c |    4 ++++
 2 files changed, 6 insertions(+), 0 deletions(-)

Signature · 7 lines

1.7.4.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

K. Y. Srinivasan

author

2014-12-05 23:31 UTC #1 lore raw

PATCH 1/2 Drivers: base: core: Export functions to lock/unlock device hotplug lock

The Hyper-V balloon driver does memory hot-add. The device_hotplug_lock is designed to address AB BA deadlock issues between the hot-add path and the sysfs path. Export the APIs to acquire and release the device_hotplug_lock for use by loadable modules that want to hot-add memory or CPU.

Signed-off-by K. Y. Srinivasan <[email protected]>

---
 drivers/base/core.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

drivers/base/core.c +2 -0

index 97e2baf..b3073af 100644
@@ -55,11 +55,13 @@ void lock_device_hotplug(void)
 {
 	mutex_lock(&device_hotplug_lock);
 }
+EXPORT_SYMBOL_GPL(lock_device_hotplug);
 
 void unlock_device_hotplug(void)
 {
 	mutex_unlock(&device_hotplug_lock);
 }
+EXPORT_SYMBOL_GPL(unlock_device_hotplug);
 
 int lock_device_hotplug_sysfs(void)
 {

Signature · 7 lines

1.7.4.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

K. Y. Srinivasan

author

2014-12-05 23:31 UTC #2 lore raw

PATCH 2/2 Drivers: hv: balloon: Fix the deadlock issue in the memory hot-add code

Andy Whitcroft <[email protected]> initially saw this deadlock. We have seen this as well. Here is the original description of the problem (and a potential solution) from Andy:

https://lkml.org/lkml/2014/3/14/451

Here is an excerpt from that mail:

"We are seeing machines lockup with what appears to be an ABBA
deadlock in the memory hotplug system. These are from the 3.13.6 based Ubuntu kernels. The hv_balloon driver is adding memory using add_memory() which takes
the hotplug lock, and then emits a udev event, and then attempts to
lock the sysfs device. In response to the udev event udev opens the sysfs device and locks it, then attempts to grab the hotplug lock to online the memory. This seems to be inverted nesting in the two cases, leading to the hangs below:

[  240.608612] INFO: task kworker/0:2:861 blocked for more than 120 seconds.
[  240.608705] INFO: task systemd-udevd:1906 blocked for more than 120 seconds.

I note that the device hotplug locking allows complete retries (via ERESTARTSYS) and if we could detect this at the online stage it could be used to get us out. But before I go down this road I wanted to make sure I am reading this right. Or indeed if the hv_balloon driver is just doing this wrong."

This patch is based on the suggestion from Yasuaki Ishimatsu <[email protected]>

Signed-off-by K. Y. Srinivasan <[email protected]>

---
 drivers/hv/hv_balloon.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

drivers/hv/hv_balloon.c +4 -0

index afdb0d5..f525a62 100644
@@ -22,6 +22,7 @@
 #include <linux/jiffies.h>
 #include <linux/mman.h>
 #include <linux/delay.h>
+#include <linux/device.h>
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/slab.h>

@@ -649,8 +650,11 @@ static void hv_mem_hot_add(unsigned long start, unsigned long size,
 
 		release_region_mutex(false);
 		nid = memory_add_physaddr_to_nid(PFN_PHYS(start_pfn));
+
+		lock_device_hotplug();
 		ret = add_memory(nid, PFN_PHYS((start_pfn)),
 				(HA_CHUNK << PAGE_SHIFT));
+		unlock_device_hotplug();
 
 		if (ret) {
 			pr_info("hot_add memory failed error is %d\n", ret);

Signature · 7 lines

1.7.4.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Michal Hocko

2014-12-08 15:04 UTC #3 lore raw

Re: [PATCH 2/2] Drivers: hv: balloon: Fix the deadlock issue in the memory hot-add code

Quoted 4 lines · K. Y. Srinivasan

Andy Whitcroft <[email protected]> initially saw this deadlock. We have seen this as well. Here is the original description of the problem (and a potential solution) from Andy:

Quoted 8 lines

https://lkml.org/lkml/2014/3/14/451
Here is an excerpt from that mail:
"We are seeing machines lockup with what appears to be an ABBA
deadlock in the memory hotplug system. These are from the 3.13.6 based Ubuntu kernels. The hv_balloon driver is adding memory using add_memory() which takes
the hotplug lock

Do you mean mem_hotplug_begin?

Quoted 3 lines

and then emits a udev event, and then attempts to
lock the sysfs device. In response to the udev event udev opens the sysfs device and locks it, then attempts to grab the hotplug lock to online the memory.

Cannot we simply teach online_pages to fail with EBUSY when the memory hotplug is on the way. We shouldn't try to online something that is not initialized yet, no? The memory hotplug log is global so we can get false positives but that should be easier to deal with than exporting lock_device_hotplug and adding yet another lock dependency.

Quoted 8 lines

This seems to be inverted nesting in the two cases, leading to the hangs below:
[  240.608612] INFO: task kworker/0:2:861 blocked for more than 120 seconds.
[  240.608705] INFO: task systemd-udevd:1906 blocked for more than 120 seconds.
I note that the device hotplug locking allows complete retries (via ERESTARTSYS) and if we could detect this at the online stage it could be used to get us out.

I am not sure I understand this but it suggests EBUSY above?

Quoted 6 lines

But before I go down this road I wanted to
make sure I am reading this right. Or indeed if the hv_balloon driver is just doing this wrong."
This patch is based on the suggestion from Yasuaki Ishimatsu <[email protected]>

This changelog doesn't explain us much. And boy this whole thing is so convoluted. E.g. I have hard time to see why ACPI hotplug is working correctly. My trail got lost at acpi_memory_device_add level which is a callback while acpi_device_hotplug is holding lock_device_hotplug but then again the rest is hidden by callbacks. I cannot seem to find any documentation which would explain all the locking here.

So why other callers of add_memory don't need the same treatment and if they do then why don't we use the lock at add_memory level?

Quoted 1 line

Signed-off-by K. Y. Srinivasan <[email protected]>

Nak to this without proper explanation and I really think that it should be the onlining code which should deal with the parallel add_memory and back off until the full initialization is done.

Quoted 36 lines

---
 drivers/hv/hv_balloon.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

drivers/hv/hv_balloon.c +4 -0

index afdb0d5..f525a62 100644
@@ -22,6 +22,7 @@
 #include <linux/jiffies.h>
 #include <linux/mman.h>
 #include <linux/delay.h>
+#include <linux/device.h>
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/slab.h>

@@ -649,8 +650,11 @@ static void hv_mem_hot_add(unsigned long start, unsigned long size,
 
 		release_region_mutex(false);
 		nid = memory_add_physaddr_to_nid(PFN_PHYS(start_pfn));
+
+		lock_device_hotplug();
 		ret = add_memory(nid, PFN_PHYS((start_pfn)),
 				(HA_CHUNK << PAGE_SHIFT));
+		unlock_device_hotplug();
 
 		if (ret) {
 			pr_info("hot_add memory failed error is %d\n", ret);

Signature · 7 lines

1.7.4.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Signature · 8 lines

Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Yasuaki Ishimatsu

2014-12-09 01:43 UTC #4 lore raw

Re: [PATCH 2/2] Drivers: hv: balloon: Fix the deadlock issue in the memory hot-add code

Quoted 17 lines · Michal Hocko

Quoted 4 lines · K. Y. Srinivasan
Andy Whitcroft <[email protected]> initially saw this deadlock. We have seen this as well. Here is the original description of the problem (and a potential solution) from Andy:
Quoted 8 lines
https://lkml.org/lkml/2014/3/14/451
Here is an excerpt from that mail:
"We are seeing machines lockup with what appears to be an ABBA
deadlock in the memory hotplug system. These are from the 3.13.6 based Ubuntu kernels. The hv_balloon driver is adding memory using add_memory() which takes
the hotplug lock
Do you mean mem_hotplug_begin?

Quoted 7 lines

Quoted 3 lines
and then emits a udev event, and then attempts to
lock the sysfs device. In response to the udev event udev opens the sysfs device and locks it, then attempts to grab the hotplug lock to online the memory.
Cannot we simply teach online_pages to fail with EBUSY when the memory hotplug is on the way. We shouldn't try to online something that is not initialized yet, no?

Yes. Memory online shouldn't try before initializing it. Then memory online should wait for initializing it, not easily fails. Generally, kernel sends memory ONLINE event to userland by kobject_uevent() during initializing memory and udev makes memory online after catching the event. Onlining memory by udev almost run during initializing memory. So if memory online easily fails, onlining memory by udev almost fails.

Quoted 19 lines

The memory hotplug log is global so we can get
false positives but that should be easier to deal with than exporting lock_device_hotplug and adding yet another lock dependency.
Quoted 8 lines
This seems to be inverted nesting in the two cases, leading to the hangs below:
[  240.608612] INFO: task kworker/0:2:861 blocked for more than 120 seconds.
[  240.608705] INFO: task systemd-udevd:1906 blocked for more than 120 seconds.
I note that the device hotplug locking allows complete retries (via ERESTARTSYS) and if we could detect this at the online stage it could be used to get us out.
I am not sure I understand this but it suggests EBUSY above?
Quoted 4 lines
But before I go down this road I wanted to
make sure I am reading this right. Or indeed if the hv_balloon driver is just doing this wrong."

Quoted 8 lines

Quoted 2 lines
This patch is based on the suggestion from Yasuaki Ishimatsu <[email protected]>
This changelog doesn't explain us much. And boy this whole thing is so convoluted. E.g. I have hard time to see why ACPI hotplug is working correctly. My trail got lost at acpi_memory_device_add level which is a callback while acpi_device_hotplug is holding lock_device_hotplug but then again the rest is hidden by callbacks.

Commit 0f1cfe9d0d06 (mm/hotplug: remove stop_machine() from try_offline_node()) said:

---
lock_device_hotplug() serializes hotplug & online/offline operations. The lock is held in common sysfs online/offline interfaces and ACPI hotplug code paths.

And here are the code paths:

- CPU & Mem online/offline via sysfs online
store_online()->lock_device_hotplug()

- Mem online via sysfs state:
store_mem_state()->lock_device_hotplug()

- ACPI CPU & Mem hot-add:
acpi_scan_bus_device_check()->lock_device_hotplug()

- ACPI CPU & Mem hot-delete:
acpi_scan_hot_remove()->lock_device_hotplug()
---

CPU & Memory online/offline/hotplug are serialized by lock_device_hotplug().

Quoted 2 lines

I cannot seem to find any
documentation which would explain all the locking here.

Yes. I need the documentation.

Thanks,
Yasuaki Ishimatsu

Quoted 41 lines

So why other callers of add_memory don't need the same treatment and if they do then why don't we use the lock at add_memory level?
Quoted 1 line
Signed-off-by K. Y. Srinivasan <[email protected]>
Nak to this without proper explanation and I really think that it should be the onlining code which should deal with the parallel add_memory and back off until the full initialization is done.
Quoted 31 lines
---
  drivers/hv/hv_balloon.c |    4 ++++
  1 files changed, 4 insertions(+), 0 deletions(-)
drivers/hv/hv_balloon.c +4 -1
index afdb0d5..f525a62 100644
@@ -22,6 +22,7 @@
  #include <linux/jiffies.h>
  #include <linux/mman.h>
  #include <linux/delay.h>
+#include <linux/device.h>
  #include <linux/init.h>
  #include <linux/module.h>
  #include <linux/slab.h>
@@ -649,8 +650,11 @@ static void hv_mem_hot_add(unsigned long start, unsigned long size,
 
  		release_region_mutex(false);
  		nid = memory_add_physaddr_to_nid(PFN_PHYS(start_pfn));
+
+		lock_device_hotplug();
  		ret = add_memory(nid, PFN_PHYS((start_pfn)),
  				(HA_CHUNK << PAGE_SHIFT));
+		unlock_device_hotplug();
 
  		if (ret) {
  			pr_info("hot_add memory failed error is %d\n", ret);
--
1.7.4.1

Quoted 6 lines

Quoted 5 lines
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Michal Hocko

2014-12-09 09:08 UTC #5 lore raw

Re: [PATCH 2/2] Drivers: hv: balloon: Fix the deadlock issue in the memory hot-add code

Quoted 26 lines · Yasuaki Ishimatsu

Quoted 17 lines · Michal Hocko
Quoted 4 lines · K. Y. Srinivasan
Andy Whitcroft <[email protected]> initially saw this deadlock. We have seen this as well. Here is the original description of the problem (and a potential solution) from Andy:
Quoted 8 lines
https://lkml.org/lkml/2014/3/14/451
Here is an excerpt from that mail:
"We are seeing machines lockup with what appears to be an ABBA
deadlock in the memory hotplug system. These are from the 3.13.6 based Ubuntu kernels. The hv_balloon driver is adding memory using add_memory() which takes
the hotplug lock
Do you mean mem_hotplug_begin?
Quoted 7 lines
Quoted 3 lines
and then emits a udev event, and then attempts to
lock the sysfs device. In response to the udev event udev opens the sysfs device and locks it, then attempts to grab the hotplug lock to online the memory.
Cannot we simply teach online_pages to fail with EBUSY when the memory hotplug is on the way. We shouldn't try to online something that is not initialized yet, no?

Quoted 5 lines

Yes. Memory online shouldn't try before initializing it. Then memory online should wait for initializing it, not easily fails. Generally, kernel sends memory ONLINE event to userland by kobject_uevent() during initializing memory and udev makes memory online after catching the event. Onlining memory by udev almost run during initializing memory.

I guess this is because the event is sent after a mem section is initialized while the overal hotplug operation is still not completed.

Quoted 2 lines

So if memory online easily fails, onlining memory by udev almost fails.

Doesn't udev retry the operation if it gets EBUSY or EAGAIN?

Quoted 43 lines

Quoted 19 lines
The memory hotplug log is global so we can get
false positives but that should be easier to deal with than exporting lock_device_hotplug and adding yet another lock dependency.
Quoted 8 lines
This seems to be inverted nesting in the two cases, leading to the hangs below:
[  240.608612] INFO: task kworker/0:2:861 blocked for more than 120 seconds.
[  240.608705] INFO: task systemd-udevd:1906 blocked for more than 120 seconds.
I note that the device hotplug locking allows complete retries (via ERESTARTSYS) and if we could detect this at the online stage it could be used to get us out.
I am not sure I understand this but it suggests EBUSY above?
Quoted 4 lines
But before I go down this road I wanted to
make sure I am reading this right. Or indeed if the hv_balloon driver is just doing this wrong."
Quoted 8 lines
Quoted 2 lines
This patch is based on the suggestion from Yasuaki Ishimatsu <[email protected]>
This changelog doesn't explain us much. And boy this whole thing is so convoluted. E.g. I have hard time to see why ACPI hotplug is working correctly. My trail got lost at acpi_memory_device_add level which is a callback while acpi_device_hotplug is holding lock_device_hotplug but then again the rest is hidden by callbacks.
Commit 0f1cfe9d0d06 (mm/hotplug: remove stop_machine() from try_offline_node()) said:
---
lock_device_hotplug() serializes hotplug & online/offline operations. The lock is held in common sysfs online/offline interfaces and ACPI hotplug code paths.
And here are the code paths:
- CPU & Mem online/offline via sysfs online
store_online()->lock_device_hotplug()
- Mem online via sysfs state:
store_mem_state()->lock_device_hotplug()

Quoted 8 lines

- ACPI CPU & Mem hot-add:
acpi_scan_bus_device_check()->lock_device_hotplug()
- ACPI CPU & Mem hot-delete:
acpi_scan_hot_remove()->lock_device_hotplug()
---
CPU & Memory online/offline/hotplug are serialized by lock_device_hotplug().

OK, this patch aimed at the complete nodes hotplug. I am not familiar with the code enough to tell whether this is really needed but it sounds like an overkill when we are interested only in the memory hotplug. Why would we need stop_machine or anything for memory that is guaranteed to be not used at the time of both online and offline.
And again, why cannot we simply make the onlining fail or try_lock and retry internally if the event consumer cannot cope with errors? And even if that is not possible then do not export lock_device_hotplug but export a memory hotplug functions which use it properly so that other consumers (xen ballon seem to rely on add_memory as well) do not need the same change as well.

Quoted 4 lines

Quoted 2 lines
I cannot seem to find any documentation which would explain all the locking here.
Yes. I need the documentation.

Yes please! This code is incredibly hard to follow and deduce all the hidden requirements and dependencies is even harder.

Signature · 8 lines

Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Yasuaki Ishimatsu

2014-12-09 10:28 UTC #6 lore raw

Re: [PATCH 2/2] Drivers: hv: balloon: Fix the deadlock issue in the memory hot-add code

Quoted 27 lines · Michal Hocko

Quoted 26 lines · Yasuaki Ishimatsu
Quoted 24 lines · Michal Hocko
Quoted 4 lines · K. Y. Srinivasan
Andy Whitcroft <[email protected]> initially saw this deadlock. We have seen this as well. Here is the original description of the problem (and a potential solution) from Andy:
Quoted 8 lines
https://lkml.org/lkml/2014/3/14/451
Here is an excerpt from that mail:
"We are seeing machines lockup with what appears to be an ABBA
deadlock in the memory hotplug system. These are from the 3.13.6 based Ubuntu kernels. The hv_balloon driver is adding memory using add_memory() which takes
the hotplug lock
Do you mean mem_hotplug_begin?
Quoted 3 lines
and then emits a udev event, and then attempts to
lock the sysfs device. In response to the udev event udev opens the sysfs device and locks it, then attempts to grab the hotplug lock to online the memory.
Cannot we simply teach online_pages to fail with EBUSY when the memory hotplug is on the way. We shouldn't try to online something that is not initialized yet, no?

Quoted 8 lines

Quoted 5 lines
Yes. Memory online shouldn't try before initializing it. Then memory online should wait for initializing it, not easily fails. Generally, kernel sends memory ONLINE event to userland by kobject_uevent() during initializing memory and udev makes memory online after catching the event. Onlining memory by udev almost run during initializing memory.
I guess this is because the event is sent after a mem section is initialized while the overal hotplug operation is still not completed.

right.

Quoted 5 lines

Quoted 3 lines
So if memory online easily fails, onlining memory by udev almost fails.
Doesn't udev retry the operation if it gets EBUSY or EAGAIN?

It depend on implementation of udev.rules. So we can retry online/offline operation in udev.rules.

Quoted 51 lines

Quoted 51 lines
Quoted 20 lines
The memory hotplug log is global so we can get
false positives but that should be easier to deal with than exporting lock_device_hotplug and adding yet another lock dependency.
Quoted 8 lines
This seems to be inverted nesting in the two cases, leading to the hangs below:
[  240.608612] INFO: task kworker/0:2:861 blocked for more than 120 seconds.
[  240.608705] INFO: task systemd-udevd:1906 blocked for more than 120 seconds.
I note that the device hotplug locking allows complete retries (via ERESTARTSYS) and if we could detect this at the online stage it could be used to get us out.
I am not sure I understand this but it suggests EBUSY above?
Quoted 4 lines
But before I go down this road I wanted to
make sure I am reading this right. Or indeed if the hv_balloon driver is just doing this wrong."
Quoted 8 lines
Quoted 2 lines
This patch is based on the suggestion from Yasuaki Ishimatsu <[email protected]>
This changelog doesn't explain us much. And boy this whole thing is so convoluted. E.g. I have hard time to see why ACPI hotplug is working correctly. My trail got lost at acpi_memory_device_add level which is a callback while acpi_device_hotplug is holding lock_device_hotplug but then again the rest is hidden by callbacks.
Commit 0f1cfe9d0d06 (mm/hotplug: remove stop_machine() from try_offline_node()) said:
---
lock_device_hotplug() serializes hotplug & online/offline operations. The lock is held in common sysfs online/offline interfaces and ACPI hotplug code paths.
And here are the code paths:
- CPU & Mem online/offline via sysfs online
store_online()->lock_device_hotplug()
- Mem online via sysfs state:
store_mem_state()->lock_device_hotplug()
- ACPI CPU & Mem hot-add:
acpi_scan_bus_device_check()->lock_device_hotplug()
- ACPI CPU & Mem hot-delete:
acpi_scan_hot_remove()->lock_device_hotplug()
---

Quoted 7 lines

Quoted 1 line
CPU & Memory online/offline/hotplug are serialized by lock_device_hotplug().
OK, this patch aimed at the complete nodes hotplug. I am not familiar with the code enough to tell whether this is really needed but it sounds like an overkill when we are interested only in the memory hotplug. Why would we need stop_machine or anything for memory that is guaranteed to be not used at the time of both online and offline.

As Srinivasan mentioned, there is ABBA issue. So to evade it, lock_device_hotplug() is needed.

Quoted 2 lines

And again, why cannot we simply make the onlining fail or try_lock and retry internally if the event consumer cannot cope with errors?

Did you mean the following Srinivasan's first patch looks good to you?
https://lkml.org/lkml/2014/12/2/662

Thanks,
Yasuaki Ishimatsu

Quoted 7 lines

And even if that is not possible then do not export lock_device_hotplug but export a memory hotplug functions which use it properly so that other consumers (xen ballon seem to rely on add_memory as well) do not need the same change as well.

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
I cannot seem to find any documentation which would explain all the locking here.
Yes. I need the documentation.
Yes please! This code is incredibly hard to follow and deduce all the hidden requirements and dependencies is even harder.

Michal Hocko

2014-12-09 10:55 UTC #7 lore raw

Re: [PATCH 2/2] Drivers: hv: balloon: Fix the deadlock issue in the memory hot-add code

Quoted 1 line · Yasuaki Ishimatsu

(2014/12/09 18:08), Michal Hocko wrote:

[...]

Quoted 4 lines

Quoted 1 line
Doesn't udev retry the operation if it gets EBUSY or EAGAIN?
It depend on implementation of udev.rules. So we can retry online/offline operation in udev.rules.

[...]

# Memory hotadd request
SUBSYSTEM=="memory", ACTION=="add", DEVPATH=="/devices/system/memory/memory*[0-9]", TEST=="/sys$devpath/state", RUN+="/bin/sh -c 'echo online > /sys$devpath/state'"

OK so this is not prepared for a temporary failures and retries.

Quoted 5 lines

Quoted 2 lines
And again, why cannot we simply make the onlining fail or try_lock and retry internally if the event consumer cannot cope with errors?
Did you mean the following Srinivasan's first patch looks good to you?
https://lkml.org/lkml/2014/12/2/662

Heh, I was just about to post this. Because I haven't noticed the previous patch yet. Yeah, Something like that. Except that I would expect EAGAIN or EBUSY rather than ERESTARTSYS which should never leak into userspace. And that would happen here AFAICS because signal_pending will not be true usually.

So there are two options. Either make the udev rule more robust and retry within RUN section or do the retry withing online_pages (try_lock and go into interruptible sleep which gets signaled by finished add_memory()). The later option is safer wrt. the userspace because the operation wouldn't fail unexpectedly.
Another option would be generating the sysfs file after all the internal initialization is done and call it outside of the memory hotplug lock.

Signature · 8 lines

Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

KY Srinivasan

author

2014-12-11 00:21 UTC #8 lore raw

RE: [PATCH 2/2] Drivers: hv: balloon: Fix the deadlock issue in the memory hot-add code

Quoted 32 lines

-----Original Message-----
From: Michal Hocko [mailto:[email protected]]
Sent: Tuesday, December 9, 2014 2:56 AM
To: Yasuaki Ishimatsu
Cc: KY Srinivasan; [email protected]; linux- [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH 2/2] Drivers: hv: balloon: Fix the deadlock issue in the memory hot-add code
Quoted 1 line · Yasuaki Ishimatsu
(2014/12/09 18:08), Michal Hocko wrote:
[...]
Quoted 4 lines
Quoted 1 line
Doesn't udev retry the operation if it gets EBUSY or EAGAIN?
It depend on implementation of udev.rules. So we can retry online/offline operation in udev.rules.
[...]
# Memory hotadd request
SUBSYSTEM=="memory", ACTION=="add", DEVPATH=="/devices/system/memory/memory*[0-9]", TEST=="/sys$devpath/state", RUN+="/bin/sh -c 'echo online > /sys$devpath/state'"
OK so this is not prepared for a temporary failures and retries.
Quoted 3 lines
Quoted 2 lines
And again, why cannot we simply make the onlining fail or try_lock and retry internally if the event consumer cannot cope with errors?

Quoted 8 lines

Quoted 2 lines
Did you mean the following Srinivasan's first patch looks good to you?
https://lkml.org/lkml/2014/12/2/662
Heh, I was just about to post this. Because I haven't noticed the previous patch yet. Yeah, Something like that. Except that I would expect EAGAIN or EBUSY rather than ERESTARTSYS which should never leak into userspace. And that would happen here AFAICS because signal_pending will not be true usually.

Michal,

I agree that the fix to this problem must be outside the clients of add_memory() and that is the reason I had sent that patch: https://lkml.org/lkml/2014/12/2/662. Let me know if you want me to resend this patch with the correct return value.

Regards,

K. Y

Quoted 9 lines

So there are two options. Either make the udev rule more robust and retry within RUN section or do the retry withing online_pages (try_lock and go into interruptible sleep which gets signaled by finished add_memory()). The later option is safer wrt. the userspace because the operation wouldn't fail unexpectedly.
Another option would be generating the sysfs file after all the internal initialization is done and call it outside of the memory hotplug lock.

Quoted 3 lines

--
Michal Hocko
SUSE Labs

Michal Hocko

2014-12-11 12:58 UTC #9 lore raw

Re: [PATCH 2/2] Drivers: hv: balloon: Fix the deadlock issue in the memory hot-add code

Quoted 40 lines · KY Srinivasan

Quoted 32 lines
-----Original Message-----
From: Michal Hocko [mailto:[email protected]]
Sent: Tuesday, December 9, 2014 2:56 AM
To: Yasuaki Ishimatsu
Cc: KY Srinivasan; [email protected]; linux- [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH 2/2] Drivers: hv: balloon: Fix the deadlock issue in the memory hot-add code
Quoted 1 line · Yasuaki Ishimatsu
(2014/12/09 18:08), Michal Hocko wrote:
[...]
Quoted 4 lines
Quoted 1 line
Doesn't udev retry the operation if it gets EBUSY or EAGAIN?
It depend on implementation of udev.rules. So we can retry online/offline operation in udev.rules.
[...]
# Memory hotadd request
SUBSYSTEM=="memory", ACTION=="add", DEVPATH=="/devices/system/memory/memory*[0-9]", TEST=="/sys$devpath/state", RUN+="/bin/sh -c 'echo online > /sys$devpath/state'"
OK so this is not prepared for a temporary failures and retries.
Quoted 3 lines
Quoted 2 lines
And again, why cannot we simply make the onlining fail or try_lock and retry internally if the event consumer cannot cope with errors?
Quoted 8 lines
Quoted 2 lines
Did you mean the following Srinivasan's first patch looks good to you?
https://lkml.org/lkml/2014/12/2/662
Heh, I was just about to post this. Because I haven't noticed the previous patch yet. Yeah, Something like that. Except that I would expect EAGAIN or EBUSY rather than ERESTARTSYS which should never leak into userspace. And that would happen here AFAICS because signal_pending will not be true usually.

Quoted 6 lines

Michal,
I agree that the fix to this problem must be outside the clients of add_memory() and that is the reason I had sent that patch:
https://lkml.org/lkml/2014/12/2/662. Let me know if you want me to resend this patch with the correct return value.

Please think about the other suggested options as well.

Quoted 15 lines

Regards,
K. Y
Quoted 12 lines
So there are two options. Either make the udev rule more robust and retry within RUN section or do the retry withing online_pages (try_lock and go into interruptible sleep which gets signaled by finished add_memory()). The later option is safer wrt. the userspace because the operation wouldn't fail unexpectedly.
Another option would be generating the sysfs file after all the internal initialization is done and call it outside of the memory hotplug lock.
--
Michal Hocko
SUSE Labs

Signature · 8 lines

Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

KY Srinivasan

author

2014-12-14 18:45 UTC #10 lore raw

RE: [PATCH 2/2] Drivers: hv: balloon: Fix the deadlock issue in the memory hot-add code

Quoted 54 lines

-----Original Message-----
From: Michal Hocko [mailto:[email protected]]
Sent: Thursday, December 11, 2014 4:58 AM
To: KY Srinivasan
Cc: Yasuaki Ishimatsu; [email protected]; linux- [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH 2/2] Drivers: hv: balloon: Fix the deadlock issue in the memory hot-add code
Quoted 27 lines · KY Srinivasan
Quoted 27 lines
-----Original Message-----
From: Michal Hocko [mailto:[email protected]]
Sent: Tuesday, December 9, 2014 2:56 AM
To: Yasuaki Ishimatsu
Cc: KY Srinivasan; [email protected]; linux- [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH 2/2] Drivers: hv: balloon: Fix the deadlock issue in the memory hot-add code
Quoted 1 line · Yasuaki Ishimatsu
(2014/12/09 18:08), Michal Hocko wrote:
[...]
Quoted 4 lines
Quoted 1 line
Doesn't udev retry the operation if it gets EBUSY or EAGAIN?
It depend on implementation of udev.rules. So we can retry online/offline operation in udev.rules.
[...]
# Memory hotadd request
SUBSYSTEM=="memory", ACTION=="add", DEVPATH=="/devices/system/memory/memory*[0-9]", TEST=="/sys$devpath/state", RUN+="/bin/sh -c 'echo online > /sys$devpath/state'"
Quoted 4 lines
Quoted 4 lines
OK so this is not prepared for a temporary failures and retries.
Quoted 2 lines
Quoted 2 lines
And again, why cannot we simply make the onlining fail or try_lock and retry internally if the event consumer cannot cope with
errors?
Quoted 9 lines
Quoted 9 lines
Quoted 3 lines
Did you mean the following Srinivasan's first patch looks good to you?
https://lkml.org/lkml/2014/12/2/662
Heh, I was just about to post this. Because I haven't noticed the previous patch yet. Yeah, Something like that. Except that I would expect EAGAIN or EBUSY rather than ERESTARTSYS which should never leak into userspace. And that would happen here AFAICS because signal_pending will not be true usually.

Quoted 8 lines

Quoted 6 lines
Michal,
I agree that the fix to this problem must be outside the clients of add_memory() and that is the reason I had sent that patch:
https://lkml.org/lkml/2014/12/2/662. Let me know if you want me to resend this patch with the correct return value.
Please think about the other suggested options as well.

Thanks Michal. I will look at the other options you have listed as well.

K. Y

Quoted 14 lines

Quoted 12 lines
Regards,
K. Y
Quoted 8 lines
So there are two options. Either make the udev rule more robust and retry within RUN section or do the retry withing online_pages (try_lock and go into interruptible sleep which gets signaled by finished add_memory()). The later option is safer wrt. the userspace because the operation wouldn't fail unexpectedly.
Another option would be generating the sysfs file after all the internal initialization is done and call it outside of the memory hotplug
lock.
Quoted 1 line
Quoted 1 line

Quoted 7 lines

Quoted 3 lines
Quoted 3 lines
--
Michal Hocko
SUSE Labs
--
Michal Hocko
SUSE Labs

Yasuaki Ishimatsu

Reviewed-by on the v1 series

2014-12-08 00:07 UTC #11 lore raw

In reply to K. Y. Srinivasan

Quoted 8 lines · K. Y. Srinivasan

Fix a deadlock in the hot-add path in the Hyper-V balloon driver.
K. Y. Srinivasan (2):
Drivers: base: core: Export functions to lock/unlock device hotplug
lock
Drivers: hv: balloon: Fix the deadlock issue in the memory hot-add
code

Looks good to me.

Reviewed-by Yasuaki Ishimatsu <[email protected]>

Thanks,
Yasauaki Ishimatsu

Quoted 5 lines

  drivers/base/core.c     |    2 ++
  drivers/hv/hv_balloon.c |    4 ++++
  2 files changed, 6 insertions(+), 0 deletions(-)