On Wed, 3 Apr 2019 14:22:27 -0400
Farhan Ali <alifm@xxxxxxxxxxxxx> 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 <alifm@xxxxxxxxxxxxx>
> ---
>
> 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(-)
>
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index 6483387..62f9637 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -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 <alex.williamson@xxxxxxxxxx>"
> @@ -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)
> @@ -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
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index fb7cc8f11ce5..82fcf07fa9ea 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -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 {
