Skip to contentSkip to conversation

vfio: Fix WARNING "do not call blocking ops when !TASK_RUNNING"

PATCH Farhan Ali 2 participants 5 messages Apr 9, 2019 – Apr 23, 2019 2 messages not in the archive

Subsystems (2)
KVM VFIO driver
lore
Share & tools
lore

Farhan Ali

author
#2 lore raw
In reply to a message not in the archive
Quoted 60 lines · Farhan Ali

vfio_dev_present() which is the condition to wait_event_interruptible_timeout(), will call vfio_group_get_device and try to acquire the mutex group->device_lock.

wait_event_interruptible_timeout() will set the state of the current task to TASK_INTERRUPTIBLE, before doing the condition check. This means that we will try to accquire the mutex while already in a sleeping state. The scheduler warns us by giving the following warning:

[ 4050.264464] ------------[ cut here ]------------
[ 4050.264508] do not call blocking ops when !TASK_RUNNING; state=1 set at [<00000000b33c00e2>] prepare_to_wait_event+0x14a/0x188
[ 4050.264529] WARNING: CPU: 12 PID: 35924 at kernel/sched/core.c:6112 __might_sleep+0x76/0x90

....

4050.264756] Call Trace:

[ 4050.264765] ([<000000000017bbaa>] __might_sleep+0x72/0x90)
[ 4050.264774]  [<0000000000b97edc>] __mutex_lock+0x44/0x8c0
[ 4050.264782]  [<0000000000b9878a>] mutex_lock_nested+0x32/0x40
[ 4050.264793]  [<000003ff800d7abe>] vfio_group_get_device+0x36/0xa8 [vfio]
[ 4050.264803]  [<000003ff800d87c0>] vfio_del_group_dev+0x238/0x378 [vfio]
[ 4050.264813]  [<000003ff8015f67c>] mdev_remove+0x3c/0x68 [mdev]
[ 4050.264825]  [<00000000008e01b0>] device_release_driver_internal+0x168/0x268
[ 4050.264834]  [<00000000008de692>] bus_remove_device+0x162/0x190
[ 4050.264843]  [<00000000008daf42>] device_del+0x1e2/0x368
[ 4050.264851]  [<00000000008db12c>] device_unregister+0x64/0x88
[ 4050.264862]  [<000003ff8015ed84>] mdev_device_remove+0xec/0x130 [mdev]
[ 4050.264872]  [<000003ff8015f074>] remove_store+0x6c/0xa8 [mdev]
[ 4050.264881]  [<000000000046f494>] kernfs_fop_write+0x14c/0x1f8
[ 4050.264890]  [<00000000003c1530>] __vfs_write+0x38/0x1a8
[ 4050.264899]  [<00000000003c187c>] vfs_write+0xb4/0x198
[ 4050.264908]  [<00000000003c1af2>] ksys_write+0x5a/0xb0
[ 4050.264916]  [<0000000000b9e270>] system_call+0xdc/0x2d8
[ 4050.264925] 4 locks held by sh/35924:
[ 4050.264933]  #0: 000000001ef90325 (sb_writers#4){.+.+}, at: vfs_write+0x9e/0x198
[ 4050.264948]  #1: 000000005c1ab0b3 (&of->mutex){+.+.}, at: kernfs_fop_write+0x1cc/0x1f8
[ 4050.264963]  #2: 0000000034831ab8 (kn->count#297){++++}, at: kernfs_remove_self+0x12e/0x150
[ 4050.264979]  #3: 00000000e152484f (&dev->mutex){....}, at: device_release_driver_internal+0x5c/0x268
[ 4050.264993] Last Breaking-Event-Address:
[ 4050.265002]  [<000000000017bbaa>] __might_sleep+0x72/0x90
[ 4050.265010] irq event stamp: 7039
[ 4050.265020] hardirqs last  enabled at (7047): [<00000000001cee7a>] console_unlock+0x6d2/0x740
[ 4050.265029] hardirqs last disabled at (7054): [<00000000001ce87e>] console_unlock+0xd6/0x740
[ 4050.265040] softirqs last  enabled at (6416): [<0000000000b8fe26>] __udelay+0xb6/0x100
[ 4050.265049] softirqs last disabled at (6415): [<0000000000b8fe06>] __udelay+0x96/0x100
[ 4050.265057] ---[ end trace d04a07d39d99a9f9 ]---

Let's fix this as described in the articlehttps://lwn.net/Articles/628628/.

Signed-off-by Farhan Ali<[email protected]>

---

I have already tested in my environment and the warning goes away for me with this patch. But appreciate further testing and review feedback on the patch.

Thanks
Farhan

Quoted 7 lines

ChangeLog
---------
v1 -> v2
- Keep the same behavior as before, so the task goes into TASK_UNINTERRUPTIBLE state after being interrupted once

---

Hi Alex,

Polite ping :)

Do you have any other feedback regarding the patch?

Thanks
Farhan

#3 lore raw
Quoted 71 lines · On Tue, 9 Apr 2019 09:13:48 -0400 Farhan Ali <[email protected]> wrote:
Quoted 60 lines · Farhan Ali

vfio_dev_present() which is the condition to wait_event_interruptible_timeout(), will call vfio_group_get_device and try to acquire the mutex group->device_lock.

wait_event_interruptible_timeout() will set the state of the current task to TASK_INTERRUPTIBLE, before doing the condition check. This means that we will try to accquire the mutex while already in a sleeping state. The scheduler warns us by giving the following warning:

[ 4050.264464] ------------[ cut here ]------------
[ 4050.264508] do not call blocking ops when !TASK_RUNNING; state=1 set at [<00000000b33c00e2>] prepare_to_wait_event+0x14a/0x188
[ 4050.264529] WARNING: CPU: 12 PID: 35924 at kernel/sched/core.c:6112 __might_sleep+0x76/0x90

....

4050.264756] Call Trace:

[ 4050.264765] ([<000000000017bbaa>] __might_sleep+0x72/0x90)
[ 4050.264774]  [<0000000000b97edc>] __mutex_lock+0x44/0x8c0
[ 4050.264782]  [<0000000000b9878a>] mutex_lock_nested+0x32/0x40
[ 4050.264793]  [<000003ff800d7abe>] vfio_group_get_device+0x36/0xa8 [vfio]
[ 4050.264803]  [<000003ff800d87c0>] vfio_del_group_dev+0x238/0x378 [vfio]
[ 4050.264813]  [<000003ff8015f67c>] mdev_remove+0x3c/0x68 [mdev]
[ 4050.264825]  [<00000000008e01b0>] device_release_driver_internal+0x168/0x268
[ 4050.264834]  [<00000000008de692>] bus_remove_device+0x162/0x190
[ 4050.264843]  [<00000000008daf42>] device_del+0x1e2/0x368
[ 4050.264851]  [<00000000008db12c>] device_unregister+0x64/0x88
[ 4050.264862]  [<000003ff8015ed84>] mdev_device_remove+0xec/0x130 [mdev]
[ 4050.264872]  [<000003ff8015f074>] remove_store+0x6c/0xa8 [mdev]
[ 4050.264881]  [<000000000046f494>] kernfs_fop_write+0x14c/0x1f8
[ 4050.264890]  [<00000000003c1530>] __vfs_write+0x38/0x1a8
[ 4050.264899]  [<00000000003c187c>] vfs_write+0xb4/0x198
[ 4050.264908]  [<00000000003c1af2>] ksys_write+0x5a/0xb0
[ 4050.264916]  [<0000000000b9e270>] system_call+0xdc/0x2d8
[ 4050.264925] 4 locks held by sh/35924:
[ 4050.264933]  #0: 000000001ef90325 (sb_writers#4){.+.+}, at: vfs_write+0x9e/0x198
[ 4050.264948]  #1: 000000005c1ab0b3 (&of->mutex){+.+.}, at: kernfs_fop_write+0x1cc/0x1f8
[ 4050.264963]  #2: 0000000034831ab8 (kn->count#297){++++}, at: kernfs_remove_self+0x12e/0x150
[ 4050.264979]  #3: 00000000e152484f (&dev->mutex){....}, at: device_release_driver_internal+0x5c/0x268
[ 4050.264993] Last Breaking-Event-Address:
[ 4050.265002]  [<000000000017bbaa>] __might_sleep+0x72/0x90
[ 4050.265010] irq event stamp: 7039
[ 4050.265020] hardirqs last  enabled at (7047): [<00000000001cee7a>] console_unlock+0x6d2/0x740
[ 4050.265029] hardirqs last disabled at (7054): [<00000000001ce87e>] console_unlock+0xd6/0x740
[ 4050.265040] softirqs last  enabled at (6416): [<0000000000b8fe26>] __udelay+0xb6/0x100
[ 4050.265049] softirqs last disabled at (6415): [<0000000000b8fe06>] __udelay+0x96/0x100
[ 4050.265057] ---[ end trace d04a07d39d99a9f9 ]---

Let's fix this as described in the articlehttps://lwn.net/Articles/628628/.

Signed-off-by Farhan Ali<[email protected]>

---

I have already tested in my environment and the warning goes away for me with this patch. But appreciate further testing and review feedback on the patch.

Thanks
Farhan

Quoted 7 lines

ChangeLog
---------
v1 -> v2
- Keep the same behavior as before, so the task goes into TASK_UNINTERRUPTIBLE state after being interrupted once

---

Quoted 5 lines

Hi Alex,

Polite ping :)

Do you have any other feedback regarding the patch?

Hi Farhan,

It looks fine to me, it's pending integration and further testing on my end. I'll target this for v5.2. Thanks,

Alex

#4 lore raw
In reply to a message not in the archive
Quoted 107 lines · On Wed, 3 Apr 2019 14:22:27 -0400 Farhan Ali <[email protected]> wrote:

vfio_dev_present() which is the condition to wait_event_interruptible_timeout(), will call vfio_group_get_device and try to acquire the mutex group->device_lock.

wait_event_interruptible_timeout() will set the state of the current task to TASK_INTERRUPTIBLE, before doing the condition check. This means that we will try to accquire the mutex while already in a sleeping state. The scheduler warns us by giving the following warning:

[ 4050.264464] ------------[ cut here ]------------
[ 4050.264508] do not call blocking ops when !TASK_RUNNING; state=1 set at [<00000000b33c00e2>] prepare_to_wait_event+0x14a/0x188
[ 4050.264529] WARNING: CPU: 12 PID: 35924 at kernel/sched/core.c:6112 __might_sleep+0x76/0x90

....

4050.264756] Call Trace:

[ 4050.264765] ([<000000000017bbaa>] __might_sleep+0x72/0x90)
[ 4050.264774]  [<0000000000b97edc>] __mutex_lock+0x44/0x8c0
[ 4050.264782]  [<0000000000b9878a>] mutex_lock_nested+0x32/0x40
[ 4050.264793]  [<000003ff800d7abe>] vfio_group_get_device+0x36/0xa8 [vfio]
[ 4050.264803]  [<000003ff800d87c0>] vfio_del_group_dev+0x238/0x378 [vfio]
[ 4050.264813]  [<000003ff8015f67c>] mdev_remove+0x3c/0x68 [mdev]
[ 4050.264825]  [<00000000008e01b0>] device_release_driver_internal+0x168/0x268
[ 4050.264834]  [<00000000008de692>] bus_remove_device+0x162/0x190
[ 4050.264843]  [<00000000008daf42>] device_del+0x1e2/0x368
[ 4050.264851]  [<00000000008db12c>] device_unregister+0x64/0x88
[ 4050.264862]  [<000003ff8015ed84>] mdev_device_remove+0xec/0x130 [mdev]
[ 4050.264872]  [<000003ff8015f074>] remove_store+0x6c/0xa8 [mdev]
[ 4050.264881]  [<000000000046f494>] kernfs_fop_write+0x14c/0x1f8
[ 4050.264890]  [<00000000003c1530>] __vfs_write+0x38/0x1a8
[ 4050.264899]  [<00000000003c187c>] vfs_write+0xb4/0x198
[ 4050.264908]  [<00000000003c1af2>] ksys_write+0x5a/0xb0
[ 4050.264916]  [<0000000000b9e270>] system_call+0xdc/0x2d8
[ 4050.264925] 4 locks held by sh/35924:
[ 4050.264933]  #0: 000000001ef90325 (sb_writers#4){.+.+}, at: vfs_write+0x9e/0x198
[ 4050.264948]  #1: 000000005c1ab0b3 (&of->mutex){+.+.}, at: kernfs_fop_write+0x1cc/0x1f8
[ 4050.264963]  #2: 0000000034831ab8 (kn->count#297){++++}, at: kernfs_remove_self+0x12e/0x150
[ 4050.264979]  #3: 00000000e152484f (&dev->mutex){....}, at: device_release_driver_internal+0x5c/0x268
[ 4050.264993] Last Breaking-Event-Address:
[ 4050.265002]  [<000000000017bbaa>] __might_sleep+0x72/0x90
[ 4050.265010] irq event stamp: 7039
[ 4050.265020] hardirqs last  enabled at (7047): [<00000000001cee7a>] console_unlock+0x6d2/0x740
[ 4050.265029] hardirqs last disabled at (7054): [<00000000001ce87e>] console_unlock+0xd6/0x740
[ 4050.265040] softirqs last  enabled at (6416): [<0000000000b8fe26>] __udelay+0xb6/0x100
[ 4050.265049] softirqs last disabled at (6415): [<0000000000b8fe06>] __udelay+0x96/0x100
[ 4050.265057] ---[ end trace d04a07d39d99a9f9 ]---

Let's fix this as described in the article https://lwn.net/Articles/628628/.

Signed-off-by Farhan Ali <[email protected]>

---

I have already tested in my environment and the warning goes away for me with this patch. But appreciate further testing and review feedback on the patch.

Thanks
Farhan

ChangeLog
---------
v1 -> v2
- Keep the same behavior as before, so the task goes into TASK_UNINTERRUPTIBLE state after being interrupted once

---

 drivers/vfio/vfio.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)
drivers/vfio/vfio.c +4 -1
index 6483387..62f9637 100644
@@ -34,6 +34,7 @@
 #include <linux/uaccess.h>
 #include <linux/vfio.h>
 #include <linux/wait.h>
+#include <linux/sched/signal.h>
 
 #define DRIVER_VERSION	"0.3"
 #define DRIVER_AUTHOR	"Alex Williamson <[email protected]>"

@@ -922,12 +923,12 @@ static bool vfio_dev_present(struct vfio_group *group, struct device *dev)
  * removed.  Open file descriptors for the device... */
 void *vfio_del_group_dev(struct device *dev)
 {
+	DEFINE_WAIT_FUNC(wait, woken_wake_function);
 	struct vfio_device *device = dev_get_drvdata(dev);
 	struct vfio_group *group = device->group;
 	void *device_data = device->device_data;
 	struct vfio_unbound_dev *unbound;
 	unsigned int i = 0;
-	long ret;
 	bool interrupted = false;
 
 	/*

@@ -964,6 +965,8 @@ void *vfio_del_group_dev(struct device *dev)
 	 * interval with counter to allow the driver to take escalating
 	 * measures to release the device if it has the ability to do so.
 	 */
+	add_wait_queue(&vfio.release_q, &wait);
+
 	do {
 		device = vfio_group_get_device(group, dev);
 		if (!device)

drivers/vfio/vfio.c void *vfio_del_group_dev(struct device *dev) · line 981

Quoted 5 lines
drivers/vfio/vfio.c +2 -0
@@ -974,13 +977,14 @@ void *vfio_del_group_dev(struct device *dev)
 
 		vfio_device_put(device);
 
+		if (!vfio_dev_present(group, dev))
+			break;

Hi Farhan,

Sorry for the delay, this seems to work well, but I think the above vfio_dev_present() check is redundant. The code above this test is:

                device = vfio_group_get_device(group, dev);
                if (!device)
                        break;

                if (device->ops->request)
                        device->ops->request(device_data, i++);

                vfio_device_put(device);

And vfio_dev_present() is:

static bool vfio_dev_present(struct vfio_group *group, struct device *dev)

{
        struct vfio_device *device;

        device = vfio_group_get_device(group, dev);
        if (!device)
                return false;

        vfio_device_put(device);
        return true;
}

vfio_dev_present() exists entirely to support the wait_event_timeout() calls, so I think we can simply drop it and delete the function with your conversion to this new method. If you agree, I can go ahead and make the change below on commit. Thanks,

Alex

drivers/vfio/vfio.c +0 -16
index fb7cc8f11ce5..82fcf07fa9ea 100644
@@ -902,19 +902,6 @@ void *vfio_device_data(struct vfio_device *device)
 }
 EXPORT_SYMBOL_GPL(vfio_device_data);
 
-/* Given a referenced group, check if it contains the device */
-static bool vfio_dev_present(struct vfio_group *group, struct device *dev)
-{
-	struct vfio_device *device;
-
-	device = vfio_group_get_device(group, dev);
-	if (!device)
-		return false;
-
-	vfio_device_put(device);
-	return true;
-}
-
 /*
  * Decrement the device reference count and wait for the device to be
  * removed.  Open file descriptors for the device... */

@@ -974,9 +961,6 @@ void *vfio_del_group_dev(struct device *dev)
 
 		vfio_device_put(device);
 
-		if (!vfio_dev_present(group, dev))
-			break;
-
 		if (interrupted) {
 			wait_woken(&wait, TASK_UNINTERRUPTIBLE, HZ * 10);
 		} else {
author
#5 lore raw
Quoted 179 lines · Alex Williamson
Quoted 107 lines · On Wed, 3 Apr 2019 14:22:27 -0400 Farhan Ali <[email protected]> wrote:

vfio_dev_present() which is the condition to wait_event_interruptible_timeout(), will call vfio_group_get_device and try to acquire the mutex group->device_lock.

wait_event_interruptible_timeout() will set the state of the current task to TASK_INTERRUPTIBLE, before doing the condition check. This means that we will try to accquire the mutex while already in a sleeping state. The scheduler warns us by giving the following warning:

[ 4050.264464] ------------[ cut here ]------------
[ 4050.264508] do not call blocking ops when !TASK_RUNNING; state=1 set at [<00000000b33c00e2>] prepare_to_wait_event+0x14a/0x188
[ 4050.264529] WARNING: CPU: 12 PID: 35924 at kernel/sched/core.c:6112 __might_sleep+0x76/0x90

....

4050.264756] Call Trace:

[ 4050.264765] ([<000000000017bbaa>] __might_sleep+0x72/0x90)
[ 4050.264774]  [<0000000000b97edc>] __mutex_lock+0x44/0x8c0
[ 4050.264782]  [<0000000000b9878a>] mutex_lock_nested+0x32/0x40
[ 4050.264793]  [<000003ff800d7abe>] vfio_group_get_device+0x36/0xa8 [vfio]
[ 4050.264803]  [<000003ff800d87c0>] vfio_del_group_dev+0x238/0x378 [vfio]
[ 4050.264813]  [<000003ff8015f67c>] mdev_remove+0x3c/0x68 [mdev]
[ 4050.264825]  [<00000000008e01b0>] device_release_driver_internal+0x168/0x268
[ 4050.264834]  [<00000000008de692>] bus_remove_device+0x162/0x190
[ 4050.264843]  [<00000000008daf42>] device_del+0x1e2/0x368
[ 4050.264851]  [<00000000008db12c>] device_unregister+0x64/0x88
[ 4050.264862]  [<000003ff8015ed84>] mdev_device_remove+0xec/0x130 [mdev]
[ 4050.264872]  [<000003ff8015f074>] remove_store+0x6c/0xa8 [mdev]
[ 4050.264881]  [<000000000046f494>] kernfs_fop_write+0x14c/0x1f8
[ 4050.264890]  [<00000000003c1530>] __vfs_write+0x38/0x1a8
[ 4050.264899]  [<00000000003c187c>] vfs_write+0xb4/0x198
[ 4050.264908]  [<00000000003c1af2>] ksys_write+0x5a/0xb0
[ 4050.264916]  [<0000000000b9e270>] system_call+0xdc/0x2d8
[ 4050.264925] 4 locks held by sh/35924:
[ 4050.264933]  #0: 000000001ef90325 (sb_writers#4){.+.+}, at: vfs_write+0x9e/0x198
[ 4050.264948]  #1: 000000005c1ab0b3 (&of->mutex){+.+.}, at: kernfs_fop_write+0x1cc/0x1f8
[ 4050.264963]  #2: 0000000034831ab8 (kn->count#297){++++}, at: kernfs_remove_self+0x12e/0x150
[ 4050.264979]  #3: 00000000e152484f (&dev->mutex){....}, at: device_release_driver_internal+0x5c/0x268
[ 4050.264993] Last Breaking-Event-Address:
[ 4050.265002]  [<000000000017bbaa>] __might_sleep+0x72/0x90
[ 4050.265010] irq event stamp: 7039
[ 4050.265020] hardirqs last  enabled at (7047): [<00000000001cee7a>] console_unlock+0x6d2/0x740
[ 4050.265029] hardirqs last disabled at (7054): [<00000000001ce87e>] console_unlock+0xd6/0x740
[ 4050.265040] softirqs last  enabled at (6416): [<0000000000b8fe26>] __udelay+0xb6/0x100
[ 4050.265049] softirqs last disabled at (6415): [<0000000000b8fe06>] __udelay+0x96/0x100
[ 4050.265057] ---[ end trace d04a07d39d99a9f9 ]---

Let's fix this as described in the article https://lwn.net/Articles/628628/.

Signed-off-by Farhan Ali <[email protected]>

---

I have already tested in my environment and the warning goes away for me with this patch. But appreciate further testing and review feedback on the patch.

Thanks
Farhan

ChangeLog
---------
v1 -> v2
- Keep the same behavior as before, so the task goes into TASK_UNINTERRUPTIBLE state after being interrupted once

---

  drivers/vfio/vfio.c | 20 +++++++++++++-------
  1 file changed, 13 insertions(+), 7 deletions(-)
drivers/vfio/vfio.c +4 -1
index 6483387..62f9637 100644
@@ -34,6 +34,7 @@
  #include <linux/uaccess.h>
  #include <linux/vfio.h>
  #include <linux/wait.h>
+#include <linux/sched/signal.h>
 
  #define DRIVER_VERSION	"0.3"
  #define DRIVER_AUTHOR	"Alex Williamson <[email protected]>"

@@ -922,12 +923,12 @@ static bool vfio_dev_present(struct vfio_group *group, struct device *dev)
   * removed.  Open file descriptors for the device... */
  void *vfio_del_group_dev(struct device *dev)
  {
+	DEFINE_WAIT_FUNC(wait, woken_wake_function);
  	struct vfio_device *device = dev_get_drvdata(dev);
  	struct vfio_group *group = device->group;
  	void *device_data = device->device_data;
  	struct vfio_unbound_dev *unbound;
  	unsigned int i = 0;
-	long ret;
  	bool interrupted = false;
 
  	/*

@@ -964,6 +965,8 @@ void *vfio_del_group_dev(struct device *dev)
  	 * interval with counter to allow the driver to take escalating
  	 * measures to release the device if it has the ability to do so.
  	 */
+	add_wait_queue(&vfio.release_q, &wait);
+
  	do {
  		device = vfio_group_get_device(group, dev);
  		if (!device)

drivers/vfio/vfio.c void *vfio_del_group_dev(struct device *dev) · line 981

Quoted 5 lines
drivers/vfio/vfio.c +2 -0
@@ -974,13 +977,14 @@ void *vfio_del_group_dev(struct device *dev)
 
  		vfio_device_put(device);
 
+		if (!vfio_dev_present(group, dev))
+			break;

Hi Farhan,

Sorry for the delay, this seems to work well, but I think the above vfio_dev_present() check is redundant. The code above this test is:

                 device = vfio_group_get_device(group, dev);
                 if (!device)
                         break;

                 if (device->ops->request)
                         device->ops->request(device_data, i++);

                 vfio_device_put(device);

And vfio_dev_present() is:

static bool vfio_dev_present(struct vfio_group *group, struct device *dev)

{
         struct vfio_device *device;

         device = vfio_group_get_device(group, dev);
         if (!device)
                 return false;

         vfio_device_put(device);
         return true;
}

vfio_dev_present() exists entirely to support the wait_event_timeout() calls, so I think we can simply drop it and delete the function with your conversion to this new method. If you agree, I can go ahead and make the change below on commit. Thanks,

Alex

drivers/vfio/vfio.c +0 -13
index fb7cc8f11ce5..82fcf07fa9ea 100644
@@ -902,19 +902,6 @@ void *vfio_device_data(struct vfio_device *device)
  }
  EXPORT_SYMBOL_GPL(vfio_device_data);
 
-/* Given a referenced group, check if it contains the device */
-static bool vfio_dev_present(struct vfio_group *group, struct device *dev)
-{
-	struct vfio_device *device;
-
-	device = vfio_group_get_device(group, dev);
-	if (!device)
-		return false;
-
-	vfio_device_put(device);
-	return true;
-}
-
  /*
   * Decrement the device reference count and wait for the device to be
   * removed.  Open file descriptors for the device... */

drivers/vfio/vfio.c void *vfio_del_group_dev(struct device *dev) · line 966

Quoted 9 lines
drivers/vfio/vfio.c +0 -3
@@ -974,9 +961,6 @@ void *vfio_del_group_dev(struct device *dev)
 
  		vfio_device_put(device);
 
-		if (!vfio_dev_present(group, dev))
-			break;
-
  		if (interrupted) {
  			wait_woken(&wait, TASK_UNINTERRUPTIBLE, HZ * 10);
  		} else {

Hi Alex,

You are right and this change would remove redundant code. I like it :) and I am fine with the change.

Thanks
Farhan

#6 lore raw
Quoted 192 lines · On Fri, 19 Apr 2019 08:05:00 -0400 Farhan Ali <[email protected]> wrote:
Quoted 179 lines · Alex Williamson
Quoted 107 lines · On Wed, 3 Apr 2019 14:22:27 -0400 Farhan Ali <[email protected]> wrote:

vfio_dev_present() which is the condition to wait_event_interruptible_timeout(), will call vfio_group_get_device and try to acquire the mutex group->device_lock.

wait_event_interruptible_timeout() will set the state of the current task to TASK_INTERRUPTIBLE, before doing the condition check. This means that we will try to accquire the mutex while already in a sleeping state. The scheduler warns us by giving the following warning:

[ 4050.264464] ------------[ cut here ]------------
[ 4050.264508] do not call blocking ops when !TASK_RUNNING; state=1 set at [<00000000b33c00e2>] prepare_to_wait_event+0x14a/0x188
[ 4050.264529] WARNING: CPU: 12 PID: 35924 at kernel/sched/core.c:6112 __might_sleep+0x76/0x90

....

4050.264756] Call Trace:

[ 4050.264765] ([<000000000017bbaa>] __might_sleep+0x72/0x90)
[ 4050.264774]  [<0000000000b97edc>] __mutex_lock+0x44/0x8c0
[ 4050.264782]  [<0000000000b9878a>] mutex_lock_nested+0x32/0x40
[ 4050.264793]  [<000003ff800d7abe>] vfio_group_get_device+0x36/0xa8 [vfio]
[ 4050.264803]  [<000003ff800d87c0>] vfio_del_group_dev+0x238/0x378 [vfio]
[ 4050.264813]  [<000003ff8015f67c>] mdev_remove+0x3c/0x68 [mdev]
[ 4050.264825]  [<00000000008e01b0>] device_release_driver_internal+0x168/0x268
[ 4050.264834]  [<00000000008de692>] bus_remove_device+0x162/0x190
[ 4050.264843]  [<00000000008daf42>] device_del+0x1e2/0x368
[ 4050.264851]  [<00000000008db12c>] device_unregister+0x64/0x88
[ 4050.264862]  [<000003ff8015ed84>] mdev_device_remove+0xec/0x130 [mdev]
[ 4050.264872]  [<000003ff8015f074>] remove_store+0x6c/0xa8 [mdev]
[ 4050.264881]  [<000000000046f494>] kernfs_fop_write+0x14c/0x1f8
[ 4050.264890]  [<00000000003c1530>] __vfs_write+0x38/0x1a8
[ 4050.264899]  [<00000000003c187c>] vfs_write+0xb4/0x198
[ 4050.264908]  [<00000000003c1af2>] ksys_write+0x5a/0xb0
[ 4050.264916]  [<0000000000b9e270>] system_call+0xdc/0x2d8
[ 4050.264925] 4 locks held by sh/35924:
[ 4050.264933]  #0: 000000001ef90325 (sb_writers#4){.+.+}, at: vfs_write+0x9e/0x198
[ 4050.264948]  #1: 000000005c1ab0b3 (&of->mutex){+.+.}, at: kernfs_fop_write+0x1cc/0x1f8
[ 4050.264963]  #2: 0000000034831ab8 (kn->count#297){++++}, at: kernfs_remove_self+0x12e/0x150
[ 4050.264979]  #3: 00000000e152484f (&dev->mutex){....}, at: device_release_driver_internal+0x5c/0x268
[ 4050.264993] Last Breaking-Event-Address:
[ 4050.265002]  [<000000000017bbaa>] __might_sleep+0x72/0x90
[ 4050.265010] irq event stamp: 7039
[ 4050.265020] hardirqs last  enabled at (7047): [<00000000001cee7a>] console_unlock+0x6d2/0x740
[ 4050.265029] hardirqs last disabled at (7054): [<00000000001ce87e>] console_unlock+0xd6/0x740
[ 4050.265040] softirqs last  enabled at (6416): [<0000000000b8fe26>] __udelay+0xb6/0x100
[ 4050.265049] softirqs last disabled at (6415): [<0000000000b8fe06>] __udelay+0x96/0x100
[ 4050.265057] ---[ end trace d04a07d39d99a9f9 ]---

Let's fix this as described in the article https://lwn.net/Articles/628628/.

Signed-off-by Farhan Ali <[email protected]>

---

I have already tested in my environment and the warning goes away for me with this patch. But appreciate further testing and review feedback on the patch.

Thanks
Farhan

ChangeLog
---------
v1 -> v2
- Keep the same behavior as before, so the task goes into TASK_UNINTERRUPTIBLE state after being interrupted once

---

  drivers/vfio/vfio.c | 20 +++++++++++++-------
  1 file changed, 13 insertions(+), 7 deletions(-)
drivers/vfio/vfio.c +4 -1
index 6483387..62f9637 100644
@@ -34,6 +34,7 @@
  #include <linux/uaccess.h>
  #include <linux/vfio.h>
  #include <linux/wait.h>
+#include <linux/sched/signal.h>
 
  #define DRIVER_VERSION	"0.3"
  #define DRIVER_AUTHOR	"Alex Williamson <[email protected]>"

@@ -922,12 +923,12 @@ static bool vfio_dev_present(struct vfio_group *group, struct device *dev)
   * removed.  Open file descriptors for the device... */
  void *vfio_del_group_dev(struct device *dev)
  {
+	DEFINE_WAIT_FUNC(wait, woken_wake_function);
  	struct vfio_device *device = dev_get_drvdata(dev);
  	struct vfio_group *group = device->group;
  	void *device_data = device->device_data;
  	struct vfio_unbound_dev *unbound;
  	unsigned int i = 0;
-	long ret;
  	bool interrupted = false;
 
  	/*

@@ -964,6 +965,8 @@ void *vfio_del_group_dev(struct device *dev)
  	 * interval with counter to allow the driver to take escalating
  	 * measures to release the device if it has the ability to do so.
  	 */
+	add_wait_queue(&vfio.release_q, &wait);
+
  	do {
  		device = vfio_group_get_device(group, dev);
  		if (!device)

drivers/vfio/vfio.c void *vfio_del_group_dev(struct device *dev) · line 981

Quoted 5 lines
drivers/vfio/vfio.c +2 -0
@@ -974,13 +977,14 @@ void *vfio_del_group_dev(struct device *dev)
 
  		vfio_device_put(device);
 
+		if (!vfio_dev_present(group, dev))
+			break;

Hi Farhan,

Sorry for the delay, this seems to work well, but I think the above vfio_dev_present() check is redundant. The code above this test is:

                 device = vfio_group_get_device(group, dev);
                 if (!device)
                         break;

                 if (device->ops->request)
                         device->ops->request(device_data, i++);

                 vfio_device_put(device);

And vfio_dev_present() is:

static bool vfio_dev_present(struct vfio_group *group, struct device *dev)

{
         struct vfio_device *device;

         device = vfio_group_get_device(group, dev);
         if (!device)
                 return false;

         vfio_device_put(device);
         return true;
}

vfio_dev_present() exists entirely to support the wait_event_timeout() calls, so I think we can simply drop it and delete the function with your conversion to this new method. If you agree, I can go ahead and make the change below on commit. Thanks,

Alex

drivers/vfio/vfio.c +0 -13
index fb7cc8f11ce5..82fcf07fa9ea 100644
@@ -902,19 +902,6 @@ void *vfio_device_data(struct vfio_device *device)
  }
  EXPORT_SYMBOL_GPL(vfio_device_data);
 
-/* Given a referenced group, check if it contains the device */
-static bool vfio_dev_present(struct vfio_group *group, struct device *dev)
-{
-	struct vfio_device *device;
-
-	device = vfio_group_get_device(group, dev);
-	if (!device)
-		return false;
-
-	vfio_device_put(device);
-	return true;
-}
-
  /*
   * Decrement the device reference count and wait for the device to be
   * removed.  Open file descriptors for the device... */

drivers/vfio/vfio.c void *vfio_del_group_dev(struct device *dev) · line 966

Quoted 9 lines
drivers/vfio/vfio.c +0 -3
@@ -974,9 +961,6 @@ void *vfio_del_group_dev(struct device *dev)
 
  		vfio_device_put(device);
 
-		if (!vfio_dev_present(group, dev))
-			break;
-
  		if (interrupted) {
  			wait_woken(&wait, TASK_UNINTERRUPTIBLE, HZ * 10);
  		} else {

Hi Alex,

Quoted 2 lines

You are right and this change would remove redundant code. I like it :) and I am fine with the change.

Great, folded into patch and applied to vfio next branch for v5.2. Thanks!

Alex