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

[Ophir Munk <ophirmu@mellanox.com>]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Saturday, September 08, 2018 12: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 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) the 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_probe".
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. 

> 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