| Message ID | 20190430224937.57156-10-parav@mellanox.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio/mdev: Improve vfio/mdev core module | expand |
On Tue, 30 Apr 2019 17:49:36 -0500 Parav Pandit <parav@mellanox.com> wrote: > If device is removal is initiated by two threads as below, mdev core > attempts to create a syfs remove file on stale device. > During this flow, below [1] call trace is observed. > > cpu-0 cpu-1 > ----- ----- > mdev_unregister_device() > device_for_each_child > mdev_device_remove_cb > mdev_device_remove > user_syscall > remove_store() > mdev_device_remove() > [..] > unregister device(); > /* not found in list or > * active=false. > */ > sysfs_create_file() > ..Call trace > > Now that mdev core follows correct device removal system of the linux > bus model, remove shouldn't fail in normal cases. If it fails, there is > no point of creating a stale file or checking for specific error status. Which error cases are left? Is there anything that does not indicate that something got terribly messed up internally? > > kernel: WARNING: CPU: 2 PID: 9348 at fs/sysfs/file.c:327 > sysfs_create_file_ns+0x7f/0x90 > kernel: CPU: 2 PID: 9348 Comm: bash Kdump: loaded Not tainted > 5.1.0-rc6-vdevbus+ #6 > kernel: Hardware name: Supermicro SYS-6028U-TR4+/X10DRU-i+, BIOS 2.0b > 08/09/2016 > kernel: RIP: 0010:sysfs_create_file_ns+0x7f/0x90 > kernel: Call Trace: > kernel: remove_store+0xdc/0x100 [mdev] > kernel: kernfs_fop_write+0x113/0x1a0 > kernel: vfs_write+0xad/0x1b0 > kernel: ksys_write+0x5a/0xe0 > kernel: do_syscall_64+0x5a/0x210 > kernel: entry_SYSCALL_64_after_hwframe+0x49/0xbe > > Signed-off-by: Parav Pandit <parav@mellanox.com> > --- > drivers/vfio/mdev/mdev_sysfs.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/drivers/vfio/mdev/mdev_sysfs.c b/drivers/vfio/mdev/mdev_sysfs.c > index 9f774b91d275..ffa3dcebf201 100644 > --- a/drivers/vfio/mdev/mdev_sysfs.c > +++ b/drivers/vfio/mdev/mdev_sysfs.c > @@ -237,10 +237,8 @@ static ssize_t remove_store(struct device *dev, struct device_attribute *attr, > int ret; > > ret = mdev_device_remove(dev); > - if (ret) { > - device_create_file(dev, attr); > + if (ret) Should you merge this into the previous patch? > return ret; > - } > } > > return count;
> -----Original Message----- > From: Cornelia Huck <cohuck@redhat.com> > Sent: Wednesday, May 8, 2019 12:17 PM > To: Parav Pandit <parav@mellanox.com> > Cc: kvm@vger.kernel.org; linux-kernel@vger.kernel.org; > kwankhede@nvidia.com; alex.williamson@redhat.com; cjia@nvidia.com > Subject: Re: [PATCHv2 09/10] vfio/mdev: Avoid creating sysfs remove file on > stale device removal > > On Tue, 30 Apr 2019 17:49:36 -0500 > Parav Pandit <parav@mellanox.com> wrote: > > > If device is removal is initiated by two threads as below, mdev core > > attempts to create a syfs remove file on stale device. > > During this flow, below [1] call trace is observed. > > > > cpu-0 cpu-1 > > ----- ----- > > mdev_unregister_device() > > device_for_each_child > > mdev_device_remove_cb > > mdev_device_remove > > user_syscall > > remove_store() > > mdev_device_remove() > > [..] > > unregister device(); > > /* not found in list or > > * active=false. > > */ > > sysfs_create_file() > > ..Call trace > > > > Now that mdev core follows correct device removal system of the linux > > bus model, remove shouldn't fail in normal cases. If it fails, there > > is no point of creating a stale file or checking for specific error status. > > Which error cases are left? Is there anything that does not indicate that > something got terribly messed up internally? > Few reasons I can think of that can fail remove are: 1. Some device removal requires allocating memory too as it needs to issue commands to device. If on the path, such allocation fails, remove can fail. However such fail to allocate memory will probably result into more serious warnings before this. 2. if the device firmware has crashed, device removal commands will likely timeout and return such error upto user. 3. If user tries to remove a device, while parent is already in removal path, this call will eventually fail as it won't find the device in the internal list. > > > > kernel: WARNING: CPU: 2 PID: 9348 at fs/sysfs/file.c:327 > > sysfs_create_file_ns+0x7f/0x90 > > kernel: CPU: 2 PID: 9348 Comm: bash Kdump: loaded Not tainted > > 5.1.0-rc6-vdevbus+ #6 > > kernel: Hardware name: Supermicro SYS-6028U-TR4+/X10DRU-i+, BIOS 2.0b > > 08/09/2016 > > kernel: RIP: 0010:sysfs_create_file_ns+0x7f/0x90 > > kernel: Call Trace: > > kernel: remove_store+0xdc/0x100 [mdev] > > kernel: kernfs_fop_write+0x113/0x1a0 > > kernel: vfs_write+0xad/0x1b0 > > kernel: ksys_write+0x5a/0xe0 > > kernel: do_syscall_64+0x5a/0x210 > > kernel: entry_SYSCALL_64_after_hwframe+0x49/0xbe > > > > Signed-off-by: Parav Pandit <parav@mellanox.com> > > --- > > drivers/vfio/mdev/mdev_sysfs.c | 4 +--- > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > diff --git a/drivers/vfio/mdev/mdev_sysfs.c > > b/drivers/vfio/mdev/mdev_sysfs.c index 9f774b91d275..ffa3dcebf201 > > 100644 > > --- a/drivers/vfio/mdev/mdev_sysfs.c > > +++ b/drivers/vfio/mdev/mdev_sysfs.c > > @@ -237,10 +237,8 @@ static ssize_t remove_store(struct device *dev, > struct device_attribute *attr, > > int ret; > > > > ret = mdev_device_remove(dev); > > - if (ret) { > > - device_create_file(dev, attr); > > + if (ret) > > Should you merge this into the previous patch? > I am not sure. Previous patch changes the sequence. I think that deserved an own patch by itself. This change is making use of that sequence. So its easier to review? Alex had comment in v0 to split into more logical patches, so... Specially to capture a different call trace, I cut into different patch. Otherwise previous patch's commit message is too long. > > return ret; > > - } > > } > > > > return count;
On Wed, 8 May 2019 22:13:28 +0000 Parav Pandit <parav@mellanox.com> wrote: > > -----Original Message----- > > From: Cornelia Huck <cohuck@redhat.com> > > Sent: Wednesday, May 8, 2019 12:17 PM > > To: Parav Pandit <parav@mellanox.com> > > Cc: kvm@vger.kernel.org; linux-kernel@vger.kernel.org; > > kwankhede@nvidia.com; alex.williamson@redhat.com; cjia@nvidia.com > > Subject: Re: [PATCHv2 09/10] vfio/mdev: Avoid creating sysfs remove file on > > stale device removal > > > > On Tue, 30 Apr 2019 17:49:36 -0500 > > Parav Pandit <parav@mellanox.com> wrote: > > > > > If device is removal is initiated by two threads as below, mdev core > > > attempts to create a syfs remove file on stale device. > > > During this flow, below [1] call trace is observed. > > > > > > cpu-0 cpu-1 > > > ----- ----- > > > mdev_unregister_device() > > > device_for_each_child > > > mdev_device_remove_cb > > > mdev_device_remove > > > user_syscall > > > remove_store() > > > mdev_device_remove() > > > [..] > > > unregister device(); > > > /* not found in list or > > > * active=false. > > > */ > > > sysfs_create_file() > > > ..Call trace > > > > > > Now that mdev core follows correct device removal system of the linux > > > bus model, remove shouldn't fail in normal cases. If it fails, there > > > is no point of creating a stale file or checking for specific error status. > > > > Which error cases are left? Is there anything that does not indicate that > > something got terribly messed up internally? > > > Few reasons I can think of that can fail remove are: > > 1. Some device removal requires allocating memory too as it needs to issue commands to device. > If on the path, such allocation fails, remove can fail. However such fail to allocate memory will probably result into more serious warnings before this. Nod. If we're OOM, we probably have some bigger problems anyway. > 2. if the device firmware has crashed, device removal commands will likely timeout and return such error upto user. In that case, I'd consider the device pretty much unusable in any case. > 3. If user tries to remove a device, while parent is already in removal path, this call will eventually fail as it won't find the device in the internal list. This should be benign, I think. > > > > > > > kernel: WARNING: CPU: 2 PID: 9348 at fs/sysfs/file.c:327 > > > sysfs_create_file_ns+0x7f/0x90 > > > kernel: CPU: 2 PID: 9348 Comm: bash Kdump: loaded Not tainted > > > 5.1.0-rc6-vdevbus+ #6 > > > kernel: Hardware name: Supermicro SYS-6028U-TR4+/X10DRU-i+, BIOS 2.0b > > > 08/09/2016 > > > kernel: RIP: 0010:sysfs_create_file_ns+0x7f/0x90 > > > kernel: Call Trace: > > > kernel: remove_store+0xdc/0x100 [mdev] > > > kernel: kernfs_fop_write+0x113/0x1a0 > > > kernel: vfs_write+0xad/0x1b0 > > > kernel: ksys_write+0x5a/0xe0 > > > kernel: do_syscall_64+0x5a/0x210 > > > kernel: entry_SYSCALL_64_after_hwframe+0x49/0xbe > > > > > > Signed-off-by: Parav Pandit <parav@mellanox.com> > > > --- > > > drivers/vfio/mdev/mdev_sysfs.c | 4 +--- > > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > > > diff --git a/drivers/vfio/mdev/mdev_sysfs.c > > > b/drivers/vfio/mdev/mdev_sysfs.c index 9f774b91d275..ffa3dcebf201 > > > 100644 > > > --- a/drivers/vfio/mdev/mdev_sysfs.c > > > +++ b/drivers/vfio/mdev/mdev_sysfs.c > > > @@ -237,10 +237,8 @@ static ssize_t remove_store(struct device *dev, > > struct device_attribute *attr, > > > int ret; > > > > > > ret = mdev_device_remove(dev); > > > - if (ret) { > > > - device_create_file(dev, attr); > > > + if (ret) > > > > Should you merge this into the previous patch? > > > I am not sure. Previous patch changes the sequence. I think that deserved an own patch by itself. > This change is making use of that sequence. > So its easier to review? Alex had comment in v0 to split into more logical patches, so... > Specially to capture a different call trace, I cut into different patch. > Otherwise previous patch's commit message is too long. I'm not sure if splitting out this one is worth it... your call. > > > > return ret; > > > - } > > > } > > > > > > return count; >
> -----Original Message----- > From: Cornelia Huck <cohuck@redhat.com> > Sent: Thursday, May 9, 2019 4:18 AM > To: Parav Pandit <parav@mellanox.com> > Cc: kvm@vger.kernel.org; linux-kernel@vger.kernel.org; > kwankhede@nvidia.com; alex.williamson@redhat.com; cjia@nvidia.com > Subject: Re: [PATCHv2 09/10] vfio/mdev: Avoid creating sysfs remove file on > stale device removal > > On Wed, 8 May 2019 22:13:28 +0000 > Parav Pandit <parav@mellanox.com> wrote: > > > > -----Original Message----- > > > From: Cornelia Huck <cohuck@redhat.com> > > > Sent: Wednesday, May 8, 2019 12:17 PM > > > To: Parav Pandit <parav@mellanox.com> > > > Cc: kvm@vger.kernel.org; linux-kernel@vger.kernel.org; > > > kwankhede@nvidia.com; alex.williamson@redhat.com; cjia@nvidia.com > > > Subject: Re: [PATCHv2 09/10] vfio/mdev: Avoid creating sysfs remove > > > file on stale device removal > > > > > > On Tue, 30 Apr 2019 17:49:36 -0500 > > > Parav Pandit <parav@mellanox.com> wrote: > > > > > > > If device is removal is initiated by two threads as below, mdev > > > > core attempts to create a syfs remove file on stale device. > > > > During this flow, below [1] call trace is observed. > > > > > > > > cpu-0 cpu-1 > > > > ----- ----- > > > > mdev_unregister_device() > > > > device_for_each_child > > > > mdev_device_remove_cb > > > > mdev_device_remove > > > > user_syscall > > > > remove_store() > > > > mdev_device_remove() > > > > [..] > > > > unregister device(); > > > > /* not found in list or > > > > * active=false. > > > > */ > > > > sysfs_create_file() > > > > ..Call trace > > > > > > > > Now that mdev core follows correct device removal system of the > > > > linux bus model, remove shouldn't fail in normal cases. If it > > > > fails, there is no point of creating a stale file or checking for specific > error status. > > > > > > Which error cases are left? Is there anything that does not indicate > > > that something got terribly messed up internally? > > > > > Few reasons I can think of that can fail remove are: > > > > 1. Some device removal requires allocating memory too as it needs to issue > commands to device. > > If on the path, such allocation fails, remove can fail. However such fail to > allocate memory will probably result into more serious warnings before this. > > Nod. If we're OOM, we probably have some bigger problems anyway. > > > 2. if the device firmware has crashed, device removal commands will likely > timeout and return such error upto user. > > In that case, I'd consider the device pretty much unusable in any case. > Right. > > 3. If user tries to remove a device, while parent is already in removal path, > this call will eventually fail as it won't find the device in the internal list. > > This should be benign, I think. > Right. > > > > > > > > > > kernel: WARNING: CPU: 2 PID: 9348 at fs/sysfs/file.c:327 > > > > sysfs_create_file_ns+0x7f/0x90 > > > > kernel: CPU: 2 PID: 9348 Comm: bash Kdump: loaded Not tainted > > > > 5.1.0-rc6-vdevbus+ #6 > > > > kernel: Hardware name: Supermicro SYS-6028U-TR4+/X10DRU-i+, BIOS > > > > 2.0b > > > > 08/09/2016 > > > > kernel: RIP: 0010:sysfs_create_file_ns+0x7f/0x90 > > > > kernel: Call Trace: > > > > kernel: remove_store+0xdc/0x100 [mdev] > > > > kernel: kernfs_fop_write+0x113/0x1a0 > > > > kernel: vfs_write+0xad/0x1b0 > > > > kernel: ksys_write+0x5a/0xe0 > > > > kernel: do_syscall_64+0x5a/0x210 > > > > kernel: entry_SYSCALL_64_after_hwframe+0x49/0xbe > > > > > > > > Signed-off-by: Parav Pandit <parav@mellanox.com> > > > > --- > > > > drivers/vfio/mdev/mdev_sysfs.c | 4 +--- > > > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/vfio/mdev/mdev_sysfs.c > > > > b/drivers/vfio/mdev/mdev_sysfs.c index 9f774b91d275..ffa3dcebf201 > > > > 100644 > > > > --- a/drivers/vfio/mdev/mdev_sysfs.c > > > > +++ b/drivers/vfio/mdev/mdev_sysfs.c > > > > @@ -237,10 +237,8 @@ static ssize_t remove_store(struct device > > > > *dev, > > > struct device_attribute *attr, > > > > int ret; > > > > > > > > ret = mdev_device_remove(dev); > > > > - if (ret) { > > > > - device_create_file(dev, attr); > > > > + if (ret) > > > > > > Should you merge this into the previous patch? > > > > > I am not sure. Previous patch changes the sequence. I think that deserved > an own patch by itself. > > This change is making use of that sequence. > > So its easier to review? Alex had comment in v0 to split into more logical > patches, so... > > Specially to capture a different call trace, I cut into different patch. > > Otherwise previous patch's commit message is too long. > > I'm not sure if splitting out this one is worth it... your call. > Ok. either ways... > > > > > > return ret; > > > > - } > > > > } > > > > > > > > return count; > >
diff --git a/drivers/vfio/mdev/mdev_sysfs.c b/drivers/vfio/mdev/mdev_sysfs.c index 9f774b91d275..ffa3dcebf201 100644 --- a/drivers/vfio/mdev/mdev_sysfs.c +++ b/drivers/vfio/mdev/mdev_sysfs.c @@ -237,10 +237,8 @@ static ssize_t remove_store(struct device *dev, struct device_attribute *attr, int ret; ret = mdev_device_remove(dev); - if (ret) { - device_create_file(dev, attr); + if (ret) return ret; - } } return count;
If device is removal is initiated by two threads as below, mdev core attempts to create a syfs remove file on stale device. During this flow, below [1] call trace is observed. cpu-0 cpu-1 ----- ----- mdev_unregister_device() device_for_each_child mdev_device_remove_cb mdev_device_remove user_syscall remove_store() mdev_device_remove() [..] unregister device(); /* not found in list or * active=false. */ sysfs_create_file() ..Call trace Now that mdev core follows correct device removal system of the linux bus model, remove shouldn't fail in normal cases. If it fails, there is no point of creating a stale file or checking for specific error status. kernel: WARNING: CPU: 2 PID: 9348 at fs/sysfs/file.c:327 sysfs_create_file_ns+0x7f/0x90 kernel: CPU: 2 PID: 9348 Comm: bash Kdump: loaded Not tainted 5.1.0-rc6-vdevbus+ #6 kernel: Hardware name: Supermicro SYS-6028U-TR4+/X10DRU-i+, BIOS 2.0b 08/09/2016 kernel: RIP: 0010:sysfs_create_file_ns+0x7f/0x90 kernel: Call Trace: kernel: remove_store+0xdc/0x100 [mdev] kernel: kernfs_fop_write+0x113/0x1a0 kernel: vfs_write+0xad/0x1b0 kernel: ksys_write+0x5a/0xe0 kernel: do_syscall_64+0x5a/0x210 kernel: entry_SYSCALL_64_after_hwframe+0x49/0xbe Signed-off-by: Parav Pandit <parav@mellanox.com> --- drivers/vfio/mdev/mdev_sysfs.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)