LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Alex Williamson <alex.williamson@redhat.com>
To: Cornelia Huck <cohuck@redhat.com>
Cc: kwankhede@nvidia.com, kvm@vger.kernel.org,
	linux-kernel@vger.kernel.org, pasic@linux.ibm.com,
	zhenyuw@linux.intel.com, zhenyuw@linux.intel.com,
	intel-gvt-dev@lists.freedesktop.org,
	intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v4 1/2] vfio/mdev: Check globally for duplicate devices
Date: Tue, 22 May 2018 09:53:37 -0600	[thread overview]
Message-ID: <20180522095337.1a3043e6@w520.home> (raw)
In-Reply-To: <20180522101346.3442e1af.cohuck@redhat.com>

[Cc +GVT-g maintainers/lists]

On Tue, 22 May 2018 10:13:46 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Fri, 18 May 2018 13:10:25 -0600
> Alex Williamson <alex.williamson@redhat.com> wrote:
> 
> > When we create an mdev device, we check for duplicates against the
> > parent device and return -EEXIST if found, but the mdev device
> > namespace is global since we'll link all devices from the bus.  We do
> > catch this later in sysfs_do_create_link_sd() to return -EEXIST, but
> > with it comes a kernel warning and stack trace for trying to create
> > duplicate sysfs links, which makes it an undesirable response.
> > 
> > Therefore we should really be looking for duplicates across all mdev
> > parent devices, or as implemented here, against our mdev device list.
> > Using mdev_list to prevent duplicates means that we can remove
> > mdev_parent.lock, but in order not to serialize mdev device creation
> > and removal globally, we add mdev_device.active which allows UUIDs to
> > be reserved such that we can drop the mdev_list_lock before the mdev
> > device is fully in place.
> > 
> > Two behavioral notes; first, mdev_parent.lock had the side-effect of
> > serializing mdev create and remove ops per parent device.  This was
> > an implementation detail, not an intentional guarantee provided to
> > the mdev vendor drivers.  Vendor drivers can trivially provide this
> > serialization internally if necessary.  Second, review comments note
> > the new -EAGAIN behavior when the device, and in particular the remove
> > attribute, becomes visible in sysfs.  If a remove is triggered prior
> > to completion of mdev_device_create() the user will see a -EAGAIN
> > error.  While the errno is different, receiving an error during this
> > period is not, the previous implementation returned -ENODEV for the
> > same condition.  Furthermore, the consistency to the user is improved
> > in the case where mdev_device_remove_ops() returns error.  Previously
> > concurrent calls to mdev_device_remove() could see the device
> > disappear with -ENODEV and return in the case of error.  Now a user
> > would see -EAGAIN while the device is in this transitory state.
> > 
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > ---
> >  Documentation/vfio-mediated-device.txt |    5 ++
> >  drivers/vfio/mdev/mdev_core.c          |  102 +++++++++++---------------------
> >  drivers/vfio/mdev/mdev_private.h       |    2 -
> >  3 files changed, 42 insertions(+), 67 deletions(-)  
> 
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> 
> I think it is better to deal with any possible vendor driver
> implications on top of this (I still believe that vfio-ccw is fine).

Thanks Cornelia.  So if vfio-ccw is fine, presumably NVIDIA is fine,
then this leaves GVT-g to see if there's any fallout.  Zhenyu & Zhi,
I've linked the series under discussion here below[1].  The question to
you is the first of the two behavioral notes listed above, does GVT-g
have any dependency on the mdev core providing serialization per mdev
parent device for the create and remove callbacks within the
mdev_parent_ops?  This was never an intended feature of the
implementation and as noted it should be trivial for for an mdev vendor
driver to provide equivalent course grained serialization if
necessary.  Of course it would be better to implement that sooner
rather than later if required.

I see that __intel_gvt_create_vgpu() makes use of gvt->lock, which
would seem to already provide this level of per-parent locking. The
remove path makes use of this same lock, so I think we're ok, but
looking for an explicit ack so there are no surprises.  I'd like
to queue this series for v4.18.  Thanks,

Alex

[1] https://lkml.org/lkml/2018/5/18/1035

  reply	other threads:[~2018-05-22 15:53 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-18 19:10 [PATCH v4 0/2] vfio/mdev: Device namespace protection Alex Williamson
2018-05-18 19:10 ` [PATCH v4 1/2] vfio/mdev: Check globally for duplicate devices Alex Williamson
2018-05-18 19:37   ` Kirti Wankhede
2018-05-22  8:13   ` Cornelia Huck
2018-05-22 15:53     ` Alex Williamson [this message]
2018-05-23  4:53       ` Zhenyu Wang
2018-05-18 19:10 ` [PATCH v4 2/2] vfio/mdev: Re-order sysfs attribute creation Alex Williamson
2018-05-18 19:38   ` Kirti Wankhede
2018-05-22  8:14   ` Cornelia Huck
2018-05-18 19:37 ` [PATCH v4 0/2] vfio/mdev: Device namespace protection Kirti Wankhede
2018-05-22 17:17 ` Halil Pasic
2018-05-22 18:38   ` Alex Williamson
2018-05-23  8:56     ` Cornelia Huck
2018-05-23 12:29       ` Halil Pasic
2018-05-23 13:34         ` Cornelia Huck

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180522095337.1a3043e6@w520.home \
    --to=alex.williamson@redhat.com \
    --cc=cohuck@redhat.com \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=intel-gvt-dev@lists.freedesktop.org \
    --cc=kvm@vger.kernel.org \
    --cc=kwankhede@nvidia.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=pasic@linux.ibm.com \
    --cc=zhenyuw@linux.intel.com \
    --subject='Re: [PATCH v4 1/2] vfio/mdev: Check globally for duplicate devices' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).