Messages in this thread | | | From | Parav Pandit <> | Subject | RE: [PATCH v2 0/2] Simplify mtty driver and mdev core | Date | Tue, 13 Aug 2019 16:28:53 +0000 |
| |
> -----Original Message----- > From: Alex Williamson <alex.williamson@redhat.com> > Sent: Tuesday, August 13, 2019 8:23 PM > To: Parav Pandit <parav@mellanox.com> > Cc: Kirti Wankhede <kwankhede@nvidia.com>; 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 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. For showing API use, it doesn't have to convoluted that too in interrupt handling code. It could be just dev_info(" UUID print..) But the whole point is to have useful API that non sample driver need to use. And there is none. In bigger objective, I wanted to discuss post this cleanup patch, is to expand mdev to have more user friendly device names.
Before we reach there, I should include a patch that eliminates storing UUID itself in the mdev_device.
> 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 fundamental property is available in form of device name already.
> API was added simply to provide data abstraction, allowing the struct > mdev_device to be opaque to vendor drivers. Thanks, > I get that part. I prefer to remove the UUID itself from the structure and therefore removing this API makes lot more sense?
| |