Re: [dpdk-dev] [PATCH v1 2/3] net/hyperv: implement core functionality

[Thomas Monjalon <thomas@monjalon.net>]

19/12/2017 21:44, Ferruh Yigit:
> On 12/19/2017 7:06 AM, Adrien Mazarguil wrote:
> > On Mon, Dec 18, 2017 at 05:54:45PM -0800, Ferruh Yigit wrote:
> >> On 12/18/2017 8:46 AM, Adrien Mazarguil wrote:
> >>> As described in more details in the attached documentation (see patch
> >>> contents), this virtual device driver manages NetVSC interfaces in virtual
> >>> machines hosted by Hyper-V/Azure platforms.
> >>>
> >>> This driver does not manage traffic nor Ethernet devices directly; it acts
> >>> as a thin configuration layer that automatically instantiates and controls
> >>> fail-safe PMD instances combining tap and PCI sub-devices, so that each
> >>> NetVSC interface is exposed as a single consolidated port to DPDK
> >>> applications.
> >>>
> >>> PCI sub-devices being hot-pluggable (e.g. during VM migration),
> >>> applications automatically benefit from increased throughput when present
> >>> and automatic fallback on NetVSC otherwise without interruption thanks to
> >>> fail-safe's hot-plug handling.
> >>>
> >>> Once initialized, the sole job of the hyperv driver is to regularly scan
> >>> for PCI devices to associate with NetVSC interfaces and feed their
> >>> addresses to corresponding fail-safe instances.
> >>>
> >>> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> >>
> >> <...>
> >>
> >>> +	RTE_ETH_FOREACH_DEV(port_id) {
> >> <..>
> >>> +			ret = rte_eal_hotplug_remove(bus->name, dev->name);
> >> <..>
> >>> +	ret = rte_eal_hotplug_add("vdev", ctx->devname, ctx->devargs);
> >>
> >> Overall why this logic implemented as network PMD?
> >> Yes technically you can implement *anything* as PMD :), but should we?
> >>
> >> This code does eal level work (scans bus, add/remove devices), and for control
> >> path, and not a generic solution either (specific to netvsc and failsafe).
> >>
> >> Only device argument part of a PMD seems used, rest is unrelated to being a PMD.
> >> Scans netvsc changes in background and reflects them into failsafe PMD...
> >>
> >> Why this is implemented as PMD, not another entity, like bus driver perhaps?
> >>
> >> Or indeed why this in DPDK instead of being in application?
> > 
> > I'll address that last question first: the point of this driver is enabling
> > existing applications to run within a Hyper-V environment unmodified,
> > because they'd otherwise need to manage two driver instances correctly on
> > their own in addition to hot-plug events during VM migration.
> > 
> > Some kind of driver generating a front end to what otherwise appears as two
> > distinct ethdev to applications is therefore necessary.
> > 
> > Currently without it, users have to manually configure failsafe properly for
> > each NetVSC interface on their system. Besides the inconvenience, it's not
> > even a possibility with DPDK applications that don't rely on EAL
> > command-line arguments.
> > 
> > As such it's more correctly defined as a "platform" driver rather than a
> > true PMD. It leaves VF device handling to their respective PMDs while
> > automatically managing the platform-specific part itself. There's no simpler
> > alternative when running in blacklist mode (i.e. not specifying any device
> > parameters on the command line).
> > 
> > Regarding its presence in drivers/net rather than drivers/bus, the end
> > result from an application standpoint is that each instance exposes a single
> > ethdev, even if not its own (failsafe's). Busses don't do that. It also
> > allows passing arguments to individual devices through --vdev if needed.
> > 
> > You're right about putting device detection at the bus level though, and I
> > think there's work in progress to do just that, this driver will be updated
> > to benefit from it once applied. In the meantime, the code as submitted
> > works fine with the current DPDK code base and addresses an existing use
> > case for which there is no solution at this point.
> 
> This may be working but this looks like a hack to me.
> 
> If we need a platform driver why not properly work on it. If we need to improve
> eal hotplug, this is a good motivation to improve it.

I agree this code looks to be a platform driver.
It is the first one of this kind.
Usually, things are managed either in a device driver, a bus driver,
or in EAL.
I also agree that hotplug should be managed in EAL and bus drivers.

> And if this logic needs to be in application let it be, your argument is to not
> change the existing application but this logic may lead implementing many
> unrelated things as PMD to not change application, what is the line here.

The line is hardware management.
The application should not have to implement device-specific or
platform-specific code.
The same application should be able to work on any platform.

> What is the work in progress, exact list, that will replace this solution? If
> this hackish solution will prevent that real work, I am against this solution.
> Is there a way to ensure this will be a temporary solution and that real work
> will happen?

I think we should explicitly mark this code as temporary, or use the
EXPERIMENTAL tag. It should motivate us to implement what is needed
to completely remove this code later.

About the work in progress:
- When hotplug will be fully supported in EAL and bus drivers,
  the scan part of this platform driver should be removed.
- When ethdev probe notifications will be integrated, it may
  also clean a part of this code.
- We may also think how the future port ownership can improve
  the behaviour of this driver.
- NetVSC is currently supported by the TAP PMD, but it may be
  replaced by a new NetVSC PMD (VMBUS driver is already sent).
- We should also continue the work on the configuration file.
  Such user configuration may help for platform behaviours.

As a conclusion, there are a lot of improvements in progress,
and I am really happy to see Hyper-V supported in DPDK.
I think this driver must be only a step towards a first class support,
like KVM/Qemu/vhost/virtio.
As there is no API implied here, I am OK to progress step by step.