Re: [PATCH v2 0/2] Simplify mtty driver and mdev core

[Alex Williamson <alex.williamson@redhat.com>]

On Tue, 13 Aug 2019 14:40:02 +0000
Parav Pandit <parav@mellanox.com> wrote:

> > -----Original Message-----
> > From: Kirti Wankhede <kwankhede@nvidia.com>
> > Sent: Monday, August 12, 2019 5:06 PM
> > To: Alex Williamson <alex.williamson@redhat.com>; Parav Pandit
> > <parav@mellanox.com>
> > Cc: kvm@vger.kernel.org; linux-kernel@vger.kernel.org; cohuck@redhat.com;
> > cjia@nvidia.com
> > Subject: Re: [PATCH v2 0/2] Simplify mtty driver and mdev core
> > 
> > 
> > 
> > On 8/9/2019 4:32 AM, Alex Williamson wrote:  
> > > On Thu,  8 Aug 2019 09:12:53 -0500
> > > Parav Pandit <parav@mellanox.com> wrote:
> > >  
> > >> Currently mtty sample driver uses mdev state and UUID in convoluated
> > >> way to generate an interrupt.
> > >> It uses several translations from mdev_state to mdev_device to mdev uuid.
> > >> After which it does linear search of long uuid comparision to find
> > >> out mdev_state in mtty_trigger_interrupt().
> > >> mdev_state is already available while generating interrupt from which
> > >> all such translations are done to reach back to mdev_state.
> > >>
> > >> This translations are done during interrupt generation path.
> > >> This is unnecessary and reduandant.  
> > >
> > > Is the interrupt handling efficiency of this particular sample driver
> > > really relevant, or is its purpose more to illustrate the API and
> > > provide a proof of concept?  If we go to the trouble to optimize the
> > > sample driver and remove this interface from the API, what do we lose?
> > >
> > > This interface was added via commit:
> > >
> > > 99e3123e3d72 vfio-mdev: Make mdev_device private and abstract
> > > interfaces
> > >
> > > Where the goal was to create a more formal interface and abstract
> > > driver access to the struct mdev_device.  In part this served to make
> > > out-of-tree mdev vendor drivers more supportable; the object is
> > > considered opaque and access is provided via an API rather than
> > > through direct structure fields.
> > >
> > > I believe that the NVIDIA GRID mdev driver does make use of this
> > > interface and it's likely included in the sample driver specifically
> > > so that there is an in-kernel user for it (ie. specifically to avoid
> > > it being removed so casually).  An interesting feature of the NVIDIA
> > > mdev driver is that I believe it has portions that run in userspace.
> > > As we know, mdevs are named with a UUID, so I can imagine there are
> > > some efficiencies to be gained in having direct access to the UUID for
> > > a device when interacting with userspace, rather than repeatedly
> > > parsing it from a device name.  
> > 
> > That's right.
> >   
> > >  Is that really something we want to make more difficult in order to
> > > optimize a sample driver?  Knowing that an mdev device uses a UUID for
> > > it's name, as tools like libvirt and mdevctl expect, is it really
> > > worthwhile to remove such a trivial API?
> > >  
> > >> Hence,
> > >> Patch-1 simplifies mtty sample driver to directly use mdev_state.
> > >>
> > >> Patch-2, Since no production driver uses mdev_uuid(), simplifies and
> > >> removes redandant mdev_uuid() exported symbol.  
> > >
> > > s/no production driver/no in-kernel production driver/
> > >
> > > I'd be interested to hear how the NVIDIA folks make use of this API
> > > interface.  Thanks,
> > >  
> > 
> > Yes, NVIDIA mdev driver do use this interface. I don't agree on removing
> > mdev_uuid() interface.
> >   
> We need to ask Greg or Linus on the kernel policy on whether an API
> should exist without in-kernel driver. We don't add such API in
> netdev, rdma and possibly other subsystem. Where can we find this
> mdev driver in-tree?

We probably would not have added the API only for an out of tree
driver, but we do have a sample driver that uses it, even if it's
rather convoluted.  The sample driver is showing an example of using the
API, which is rather its purpose more so than absolutely efficient
interrupt handling.  Also, let's not overstate what this particular
API callback provides, it's simply access to the uuid of the device,
which is a fundamental property of a mediated device.  This API was
added simply to provide data abstraction, allowing the struct
mdev_device to be opaque to vendor drivers.  Thanks,

Alex