Message-ID: <2023082246-lumping-rebate-4142@gregkh>
Date:   Tue, 22 Aug 2023 13:48:03 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Saurabh Singh Sengar <ssengar@...rosoft.com>
Cc:     Saurabh Sengar <ssengar@...ux.microsoft.com>,
        KY Srinivasan <kys@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        "wei.liu@...nel.org" <wei.liu@...nel.org>,
        Dexuan Cui <decui@...rosoft.com>,
        "Michael Kelley (LINUX)" <mikelley@...rosoft.com>,
        "corbet@....net" <corbet@....net>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>
Subject: Re: [EXTERNAL] Re: [PATCH v4 0/3] UIO driver for low speed Hyper-V
 devices

On Mon, Aug 21, 2023 at 07:36:18AM +0000, Saurabh Singh Sengar wrote:
> 
> 
> > -----Original Message-----
> > From: Greg KH <gregkh@...uxfoundation.org>
> > Sent: Saturday, August 12, 2023 4:45 PM
> > To: Saurabh Sengar <ssengar@...ux.microsoft.com>
> > Cc: KY Srinivasan <kys@...rosoft.com>; Haiyang Zhang
> > <haiyangz@...rosoft.com>; wei.liu@...nel.org; Dexuan Cui
> > <decui@...rosoft.com>; Michael Kelley (LINUX) <mikelley@...rosoft.com>;
> > corbet@....net; linux-kernel@...r.kernel.org; linux-hyperv@...r.kernel.org;
> > linux-doc@...r.kernel.org
> > Subject: [EXTERNAL] Re: [PATCH v4 0/3] UIO driver for low speed Hyper-V
> > devices
> > 
> > On Fri, Aug 04, 2023 at 12:09:53AM -0700, Saurabh Sengar wrote:
> > > Hyper-V is adding multiple low speed "speciality" synthetic devices.
> > > Instead of writing a new kernel-level VMBus driver for each device,
> > > make the devices accessible to user space through a UIO-based
> > > hv_vmbus_client driver. Each device can then be supported by a user
> > > space driver. This approach optimizes the development process and
> > > provides flexibility to user space applications to control the key
> > > interactions with the VMBus ring buffer.
> > 
> > Why is it faster to write userspace drivers here?  Where are those new drivers,
> > and why can't they be proper kernel drivers?  Are all hyper-v drivers going to
> > move to userspace now?
> 
> Hi Greg,
> 
> You are correct; it isn't faster. However, the developers working on these userspace
> drivers can concentrate entirely on the business logic of these devices. The more
> intricate aspects of the kernel, such as interrupt management and host communication,
> can be encapsulated within the uio driver.

Yes, kernel drivers are hard, we all know that.

But if you do it right, it doesn't have to be, saying "it's too hard for
our programmers to write good code for our platform" isn't exactly a
good endorcement of either your programmers, or your platform :)

> The quantity of Hyper-V devices is substantial, and their numbers are consistently
> increasing. Presently, all of these drivers are in a development/planning phase and
> rely significantly on the acceptance of this UIO driver as a prerequisite.

Don't make my acceptance of something that you haven't submitted before
a business decision that I need to make, that's disenginous.

> Not all hyper-v drivers will move to userspace, but many a new slow Hyperv-V
> devices will use this framework and will avoid introducing a new kernel driver. We
> will also plan to remove some of the existing drivers like kvp/vss.

Define "slow" please.

> > > The new synthetic devices are low speed devices that don't support
> > > VMBus monitor bits, and so they must use vmbus_setevent() to notify
> > > the host of ring buffer updates. The new driver provides this
> > > functionality along with a configurable ring buffer size.
> > >
> > > Moreover, this series of patches incorporates an update to the fcopy
> > > application, enabling it to seamlessly utilize the new interface. The
> > > older fcopy driver and application will be phased out gradually.
> > > Development of other similar userspace drivers is still underway.
> > >
> > > Moreover, this patch series adds a new implementation of the fcopy
> > > application that uses the new UIO driver. The older fcopy driver and
> > > application will be phased out gradually. Development of other similar
> > > userspace drivers is still underway.
> > 
> > You are adding a new user api with the "ring buffer" size api, which is odd for
> > normal UIO drivers as that's not something that UIO was designed for.
> > 
> > Why not just make you own generic type uiofs type kernel api if you really
> > want to do all of this type of thing in userspace instead of in the kernel?
> 
> Could you please elaborate more on this suggestion. I couldn't understand it
> completely.

Why is uio the requirement here?  Why not make your own framework to
write hv drivers in userspace that fits in better with the overall goal?
Call it "hvfs" or something like that, much like we have usbfs for
writing usb drivers in userspace.

Bolting on HV drivers to UIO seems very odd as that is not what this
framework is supposed to be providing at all.  UIO was to enable "pass
through" memory-mapped drivers that only wanted an interrupt and access
to raw memory locations in the hardware.

Now you are adding ring buffer managment and all other sorts of things
just for your platform.  So make it a real subsystem tuned exactly for
what you need and NOT try to force it into the UIO interface (which
should know nothing about ring buffers...)

> > And finally, if this is going to replace the fcopy driver, then it should be part of
> > this series as well, removing it to show that you really mean it when you say it
> > will be deleted, otherwise we all know that will never happen.
> 
> I was hesitant about disrupting the backward compatibility, but I suppose I could
> rename the new fcopy daemon to match the old one.

If you can't drop the driver today due to backwards compatibility, then
you will not be able to drop it in the future either.

Please work to come up with a sane framework if you are going to insist
on writing drivers in userspace for your platform, that works well and
it not bolted onto the UIO api.  Consider this a chance to fix the
issues that the UIO api currently has that we can't change due to
compatiblity reasons :)

thanks,

greg k-h

Powered by blists - more mailing lists