Re: [dpdk-dev] [RFC] eal: allow hotplug to skip an already probed device

[Ophir Munk <ophirmu@mellanox.com>]

> -----Original Message-----
> From: Ophir Munk
> Sent: Thursday, September 13, 2018 9:30 AM
> To: Thomas Monjalon <thomas@monjalon.net>; dev@dpdk.org
> Cc: gaetan.rivet@6wind.com; Olga Shern <olgas@mellanox.com>; Shahaf
> Shuler <shahafs@mellanox.com>; Asaf Penso <asafp@mellanox.com>
> Subject: RE: [dpdk-dev] [RFC] eal: allow hotplug to skip an already probed
> device
> 
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> > Sent: Saturday, September 08, 2018 2:10 AM
> > To: dev@dpdk.org
> > Cc: gaetan.rivet@6wind.com
> > Subject: [dpdk-dev] [RFC] eal: allow hotplug to skip an already probed
> > device
> >
> > In the devargs syntax for device representors, it is possible to add
> > several devices at once: -w dbdf,representor=[0-3] It will become a
> > more frequent case when introducing wildcards and ranges in the new
> devargs syntax.
> >
> > If a devargs string is provided for probing, and updated with a bigger
> > range for a new probing, then we do not want it to fail because part
> > of this range was already probed previously.
> >
> 
> When having devargs with representors ("dbdf,representor=<args>") there is
> actually just one PCI device to probe (whose address is "dbdf", the master)
> while the representors themselves are net devices all using the same PCI
> "dbdf" address.
> The way to see it: when running "lspci": only the "dpdf" PCI device appears
> while when executing "ifconfig" - all representors are shown as net devices.
> When calling rte_eal_hotplug_add() for the first time there is a flow which
> eventually calls the PMD probe callback (e.g. mlx5_pci_probe() in case of
> mlx5 PMD).
> When calling rte_eal_hotplug_add() for several times we should skip failures
> till we reach the PMD probe callback.
> 
> Skipping failures can be done as follows:
> 1. In file ./lib/librte_eal/common/eal_common_dev.c, function:
> rte_eal_hotplug_add(), remove the following code:
> 
> if (dev->driver != NULL) {
> 	RTE_LOG(ERR, EAL, "Device is already plugged\n");
> 	return -EEXIST}
> 
> 2. In file ./drivers/bus/pci/pci_common.cm function: pci_probe_all_drivers(),
> remove the following code:
> 
> /* Check if a driver is already loaded */ if (dev->driver != NULL)
> 	return -1;
> 
> 
> However the substantial major changes are in each individual PMD probe
> callback when it is called several times with different devargs. For example it
> should not fail an already probed PCI device and just create new eth devices
> for new representors.
> 
> 
> > On the opposite, we could require rte_eal_hotplug_add() to try to add
> > all matching devices, and fail if one is already probed.
> >
> > That's why a new parameter is added to specify if the function must
> > fail or not when trying to add an already probed device.
> >
> 
> Please note this new parameter ("fail_existing") will have to be propagated
> to all PMD probe callbacks.
> Otherwise, in case (fail_existing == false) a second call to
> rte_eal_hotplug_add() will call the PMD probe callback, which may fail unless
> it is aware of "fail_existing" parameter.
> Alternatively "fail_existing" may be better named "enable_multi_probes".
> Anyway - if the PMD probe() callback has to be updated to return a
> success/failure value (for more than one probe) - maybe we do not need a
> new parameter and can rely on the PMD probe() callback the take the
> decision by returning success/failure value.
> 
> The counter part of rte_eal_hotplug_add() is rte_eal_hotplug_remove()
> which must be updated as well. For example when representors 1 and 2 exist
> - then removing just representor 1 will have to make sure that the PCI device
> used for both representors is not unplugged since representor 2 is not
> removed and it uses the same PCI device as representor 1.
> 

To make it clearer: the flow initiated by calling rte_eal_hotplug_removed() should eventually call the PMD remote() callback which in turn should manage a reference count to all representor devices and decide which ones should be closed.

> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > ---
> > This patch contains only the change in the function itself as RFC.
> >
> > This idea was presented at Dublin during the "hotplug talk".
> > ---
> >  lib/librte_eal/common/eal_common_dev.c  | 4 +++-
> > lib/librte_eal/common/include/rte_dev.h | 5 ++++-
> >  2 files changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/librte_eal/common/eal_common_dev.c
> > b/lib/librte_eal/common/eal_common_dev.c
> > index 678dbcac7..17d7e9089 100644
> > --- a/lib/librte_eal/common/eal_common_dev.c
> > +++ b/lib/librte_eal/common/eal_common_dev.c
> > @@ -128,7 +128,7 @@ int rte_eal_dev_detach(struct rte_device *dev)  }
> >
> >  int __rte_experimental rte_eal_hotplug_add(const char *busname, const
> > char *devname,
> > -			const char *devargs)
> > +			const char *devargs, bool fail_existing)
> >  {
> >  	struct rte_bus *bus;
> >  	struct rte_device *dev;
> > @@ -173,6 +173,8 @@ int __rte_experimental rte_eal_hotplug_add(const
> > char *busname, const char *devn
> >  	}
> >
> >  	if (dev->driver != NULL) {
> > +		if (!fail_existing)
> > +			return 0;
> >  		RTE_LOG(ERR, EAL, "Device is already plugged\n");
> >  		return -EEXIST;
> >  	}
> > diff --git a/lib/librte_eal/common/include/rte_dev.h
> > b/lib/librte_eal/common/include/rte_dev.h
> > index b80a80598..10a1cd2b4 100644
> > --- a/lib/librte_eal/common/include/rte_dev.h
> > +++ b/lib/librte_eal/common/include/rte_dev.h
> > @@ -201,11 +201,14 @@ int rte_eal_dev_detach(struct rte_device *dev);
> >   *   capable of handling it and pass it to the driver probing function.
> >   * @param devargs
> >   *   Device arguments to be passed to the driver.
> > + * @param fail_existing
> > + *   If true and a matching device is already probed, then return -EEXIST.
> > + *   If false, then skip the already probed device without returning an
> error.
> >   * @return
> >   *   0 on success, negative on error.
> >   */
> >  int __rte_experimental rte_eal_hotplug_add(const char *busname, const
> > char *devname,
> > -			const char *devargs);
> > +			const char *devargs, bool fail_existing);
> >
> >  /**
> >   * @warning
> > --
> > 2.18.0