Re: [dpdk-dev] [PATCH v2 00/11] bus: attach / detach API

["Gaëtan Rivet" <gaetan.rivet@6wind.com>]

Hi Stephen,

On Wed, May 31, 2017 at 08:34:26AM -0700, Stephen Hemminger wrote:
> On Wed, 31 May 2017 15:17:45 +0200
> Gaetan Rivet <gaetan.rivet@6wind.com> wrote:
> 
> > Following the work from Jan:
> > 
> > This patchset introduces the attach / detach API to rte_bus.
> > The rte_device structure is used as the generic device representation.
> > 
> > This API is implemented for the virtual bus.
> > The functions rte_eal_dev_attach and rte_eal_dev_detach are updated to
> > use this new interface.
> > 
> > --
> > 
> > 0. API rework
> > -------------
> > 
> > I would like to propose an evolution on the API developed by Jan.
> > 
> > The attach / detach rte_bus API is necessary to support the attach/detach
> > rte_dev API. Those are two different levels for one similar functionality.
> > 
> > Attach / detach does not allow true hotplugging, because the attach
> > function expects the devices operated upon to already exist within the
> > buses / sub-layers. This means that this API expects devices meta-datas
> > (bus-internal device representation and associated device information
> > read from the system) to be present upon attach. This part of the work
> > is done during scanning.
> > 
> > While it is best to avoid changing the public rte_dev API as it already
> > exists, nothing prevents this new rte_bus API from superseeding it.
> > It has been said during the previous release cycle that device hotplug
> > was a feature that interested users. True hotplug is not allowed by the
> > current attach / detach API. Worse, this API hinders the effort to bring
> > this new functionality by squatting its semantic field.
> > 
> > Thus, I propose to rename rte_bus attach / detach; plug / unplug. As it
> > is a superset of the attach / detach functionality, it can be used to
> > implement rte_dev attach / detach. Now is the right time to pivot to
> > this new feature.
> > 
> > This should help maintainers understanding the aim of this API and the
> > differences with the APIs higher-up, clarify the field and allow a new
> > functionality to be proposed.
> > 
> > The vdev bus is inherently supporting the new API, however it has been
> > made explicit. My implementation in the PCI bus in further patchset also
> > follows the rte_bus hotplug API instead of only attach / detach.
> > 
> > One remaining problem with the vdev bus is the rte_dev attach
> > implementation, which needs the rte_devargs rework to be properly fixed.
> > 
> > 1. Additional evolutions in the patchset
> > ----------------------------------------
> > 
> > The RTE_VERIFY on the find_device is too stringent I think and forces
> > all buses to implement a public device iterator. While it could be
> > argued that it would push for quicker support for the functionality, I
> > think it's possible that some buses are not interested at all in it and
> > should simply be ignored.
> > 
> > The bus devices iterator has been fixed.
> > 
> > The internal rte_device handle was not properly setup within the
> > net_ring PMD.
> > 
> > Gaetan Rivet (2):
> >   vdev: implement hotplug functionality
> >   net/ring: fix dev handle in eth_dev
> > 
> > Jan Blunck (9):
> >   bus: add bus iterator to find a particular bus
> >   bus: add device iterator
> >   bus: add helper to find bus for a particular device
> >   bus: add bus helper iterator to find a particular device
> >   bus: introduce hotplug functionality
> >   vdev: implement find_device bus operation
> >   vdev: implement unplug bus operation
> >   eal: make virtual driver probe and remove take rte_vdev_device
> >   ethdev: Use embedded rte_device to detach driver
> > 
> >  drivers/net/ring/rte_eth_ring.c                 |   7 ++
> >  lib/librte_eal/bsdapp/eal/rte_eal_version.map   |   4 +
> >  lib/librte_eal/common/eal_common_bus.c          |  65 +++++++++++++++
> >  lib/librte_eal/common/eal_common_dev.c          | 100 ++++++++++++++++++------
> >  lib/librte_eal/common/eal_common_vdev.c         |  27 +++++++
> >  lib/librte_eal/common/include/rte_bus.h         |  87 +++++++++++++++++++++
> >  lib/librte_eal/common/include/rte_dev.h         |  26 ++++++
> >  lib/librte_eal/linuxapp/eal/rte_eal_version.map |   3 +
> >  lib/librte_ether/rte_ethdev.c                   |   3 +-
> >  9 files changed, 299 insertions(+), 23 deletions(-)
> > 
> 
> LGTM
> 
> Maybe we should evolve it by having both rte_bus and  rte_dev API for one release and mark
> the rte_dev API for attach/detach as deprecated?

Sorry for the late response.

I think that if the hotplug API is correctly designed, the rte_dev API
could indeed be deprecated and then removed. I do not see a reason right
now to keep it.

-- 
Gaëtan Rivet
6WIND