| Message ID | 20161218012900.18142-11-aik@ozlabs.ru |
|---|---|
| State | Superseded |
| Headers | show |
On 12/18/2016 09:28 AM, Alexey Kardashevskiy wrote: > This moves a check for unregistered notifiers from fops release > callback to the place where the group will actually be released. > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > --- > > This is going to be used in the following patch in cleanup > path. Since the next patch is RFC, this one might not be needed. Hi Alexey, I didn't find any use in the following patch 11/11, did you mean something else? BTW the warning in vfio_group_release seems too late, the user should actually unregister everything by close()? -- Thanks, Jike > --- > drivers/vfio/vfio.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > index 6b9a98508939..083b581e87c0 100644 > --- a/drivers/vfio/vfio.c > +++ b/drivers/vfio/vfio.c > @@ -403,6 +403,8 @@ static void vfio_group_release(struct kref *kref) > struct iommu_group *iommu_group = group->iommu_group; > > WARN_ON(!list_empty(&group->device_list)); > + /* Any user didn't unregister? */ > + WARN_ON(group->notifier.head); > > list_for_each_entry_safe(unbound, tmp, > &group->unbound_list, unbound_next) { > @@ -1584,9 +1586,6 @@ static int vfio_group_fops_release(struct inode *inode, struct file *filep) > > filep->private_data = NULL; > > - /* Any user didn't unregister? */ > - WARN_ON(group->notifier.head); > - > vfio_group_try_dissolve_container(group); > > atomic_dec(&group->opened); > -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 19 Dec 2016 18:41:05 +0800 Jike Song <jike.song@intel.com> wrote: > On 12/18/2016 09:28 AM, Alexey Kardashevskiy wrote: > > This moves a check for unregistered notifiers from fops release > > callback to the place where the group will actually be released. > > > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > > --- > > > > This is going to be used in the following patch in cleanup > > path. Since the next patch is RFC, this one might not be needed. Alexey, this is intended to be a bug fix, it should be sent separately, not buried in an unrelated patch series. > I didn't find any use in the following patch 11/11, did you mean > something else? > > BTW the warning in vfio_group_release seems too late, the user > should actually unregister everything by close()? The thing is, it's not the user that registered the notifiers, it's the vendor driver. The vendor driver should know via the device release to unregister the notifier, which we're counting on to happen before the group release. Can we rely on that ordering even in the case where a user is SIGKILL'd? > > --- > > drivers/vfio/vfio.c | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > > index 6b9a98508939..083b581e87c0 100644 > > --- a/drivers/vfio/vfio.c > > +++ b/drivers/vfio/vfio.c > > @@ -403,6 +403,8 @@ static void vfio_group_release(struct kref *kref) > > struct iommu_group *iommu_group = group->iommu_group; > > > > WARN_ON(!list_empty(&group->device_list)); > > + /* Any user didn't unregister? */ > > + WARN_ON(group->notifier.head); Even if this is a bug, this is the wrong fix. This is when the group is being destroyed. Yes, it would be a bug to still have any notifiers here, but the intention is to make sure there are no notifiers when the group is idle and unused. > > > > list_for_each_entry_safe(unbound, tmp, > > &group->unbound_list, unbound_next) { > > @@ -1584,9 +1586,6 @@ static int vfio_group_fops_release(struct inode *inode, struct file *filep) > > > > filep->private_data = NULL; > > > > - /* Any user didn't unregister? */ > > - WARN_ON(group->notifier.head); > > - > > vfio_group_try_dissolve_container(group); > > > > atomic_dec(&group->opened); > > -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 20/12/16 03:28, Alex Williamson wrote: > On Mon, 19 Dec 2016 18:41:05 +0800 > Jike Song <jike.song@intel.com> wrote: > >> On 12/18/2016 09:28 AM, Alexey Kardashevskiy wrote: >>> This moves a check for unregistered notifiers from fops release >>> callback to the place where the group will actually be released. >>> >>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >>> --- >>> >>> This is going to be used in the following patch in cleanup >>> path. Since the next patch is RFC, this one might not be needed. > > Alexey, this is intended to be a bug fix, it should be sent separately, > not buried in an unrelated patch series. Well, I was pretty unsure this is a correct fix and I was sure that it would make sense without the context which is quite weird :) > >> I didn't find any use in the following patch 11/11, did you mean >> something else? >> >> BTW the warning in vfio_group_release seems too late, the user >> should actually unregister everything by close()? > > The thing is, it's not the user that registered the notifiers, it's the > vendor driver. The vendor driver should know via the device release to > unregister the notifier, which we're counting on to happen before the > group release. Can we rely on that ordering even in the case where a > user is SIGKILL'd? > >>> --- >>> drivers/vfio/vfio.c | 5 ++--- >>> 1 file changed, 2 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c >>> index 6b9a98508939..083b581e87c0 100644 >>> --- a/drivers/vfio/vfio.c >>> +++ b/drivers/vfio/vfio.c >>> @@ -403,6 +403,8 @@ static void vfio_group_release(struct kref *kref) >>> struct iommu_group *iommu_group = group->iommu_group; >>> >>> WARN_ON(!list_empty(&group->device_list)); >>> + /* Any user didn't unregister? */ >>> + WARN_ON(group->notifier.head); > > Even if this is a bug, this is the wrong fix. This is when the group > is being destroyed. Yes, it would be a bug to still have any notifiers > here, but the intention is to make sure there are no notifiers when the > group is idle and unused. Out of curiosity - vendor drivers are supposed to hold a group file open, not just reference vfio_grop/iommu_group objects? > >>> >>> list_for_each_entry_safe(unbound, tmp, >>> &group->unbound_list, unbound_next) { >>> @@ -1584,9 +1586,6 @@ static int vfio_group_fops_release(struct inode *inode, struct file *filep) >>> >>> filep->private_data = NULL; >>> >>> - /* Any user didn't unregister? */ >>> - WARN_ON(group->notifier.head); >>> - >>> vfio_group_try_dissolve_container(group); >>> >>> atomic_dec(&group->opened); >>> >
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c index 6b9a98508939..083b581e87c0 100644 --- a/drivers/vfio/vfio.c +++ b/drivers/vfio/vfio.c @@ -403,6 +403,8 @@ static void vfio_group_release(struct kref *kref) struct iommu_group *iommu_group = group->iommu_group; WARN_ON(!list_empty(&group->device_list)); + /* Any user didn't unregister? */ + WARN_ON(group->notifier.head); list_for_each_entry_safe(unbound, tmp, &group->unbound_list, unbound_next) { @@ -1584,9 +1586,6 @@ static int vfio_group_fops_release(struct inode *inode, struct file *filep) filep->private_data = NULL; - /* Any user didn't unregister? */ - WARN_ON(group->notifier.head); - vfio_group_try_dissolve_container(group); atomic_dec(&group->opened);
This moves a check for unregistered notifiers from fops release callback to the place where the group will actually be released. Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- This is going to be used in the following patch in cleanup path. Since the next patch is RFC, this one might not be needed. --- drivers/vfio/vfio.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)