[dpdk-dev] [PATCH] net/failsafe: do not probe device if plugged out

Gaëtan Rivet gaetan.rivet at 6wind.com
Wed Jul 12 22:39:57 CEST 2017


Hi Thomas,

Nice idea. A few remarks below:

On Wed, Jul 12, 2017 at 08:28:12PM +0200, Thomas Monjalon wrote:
> When probing a sub-devices which does not exist in the system,
> some errors are logged:
> 	EAL: Cannot find unplugged device (0002:00:02.0)
> 	PMD: net_failsafe: ERROR: sub_device 0 probe failed
> 
> It is normal to have these errors when initializing the failsafe
> device and its sub-devices.
> But we do not need to log them at each probe.
> And it is even less relevant if the sub-device has been plugged out.
> 
> The unavailable devices are skipped after the first probe, considering
> two exceptions:
> - The vdevs are always probed because they do not really exist on the
> virtual bus before probing them.
> - The sub-device list given by an executed command line may change
> from one call to the next one, therefore it is considered to be always
> the first one. Anyway, such external command should check the
> device availability before passing it to the failsafe PMD.
> 
> Signed-off-by: Thomas Monjalon <thomas at monjalon.net>
> ---
> This patch is on top of the failsafe series which is
> pending for integration in 17.08-rc2.
> ---
>  drivers/net/failsafe/failsafe_eal.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/net/failsafe/failsafe_eal.c b/drivers/net/failsafe/failsafe_eal.c
> index 3321dda71..5181066ed 100644
> --- a/drivers/net/failsafe/failsafe_eal.c
> +++ b/drivers/net/failsafe/failsafe_eal.c
> @@ -49,6 +49,11 @@ fs_find_ethdev(const struct rte_device *dev)

This needs to be rebased on the latest version.
Sorry I forgot to supersede the previous one.

>  	return NULL;
>  }
>  
> +static int cmp_dev_name(const struct rte_device *dev, const void *name)
> +{
> +	return strcmp(dev->name, name);
> +}
> +
>  static int
>  fs_bus_init(struct rte_eth_dev *dev)
>  {
> @@ -56,12 +61,24 @@ fs_bus_init(struct rte_eth_dev *dev)
>  	struct rte_device *rdev;
>  	struct rte_devargs *da;
>  	uint8_t i;
> +	static int first_init = 1;

I did not do that before within this PMD, but a boolean would be better
there.

>  	int ret;
>  
>  	FOREACH_SUBDEV(sdev, i, dev) {
>  		if (sdev->state != DEV_PARSED)
>  			continue;
>  		da = &sdev->devargs;
> +

Superfluous line.

> +		/* skip plugged out devices */
> +		if (! first_init
> +				&& sdev->cmdline == NULL
> +				&& strcmp(da->bus->name, "vdev") != 0) {

Use first_init == false instead of negation.
&& should be at the end of the line instead of the start of the next
one.
Indentation is wrong.

> +			da->bus->scan();
> +			if (bus->find_device(NULL, cmp_dev_name, da->name) == NULL)
> +				continue; /* device not found */

da->bus->find_device instead of bus->find_device.
This function cannot find the device back currently on the PCI bus,
blocking the plugging of VF.

The PCI bus will scan the VF while no rte_devargs exists to
describe it within the global list. If the device exists, it will
detect it, allocate it and then set its name.
Without any rte_devargs, the name of a PCI device falls back to its
canonical name (DomBDF instead of BDF). The name comparison with
da->name can only succeed if the slave was declared using the DomBDF
format.

The fix is to do a deep copy of the rte_devargs (the API has been
sent previously with the rte_devargs rework but I have since removed
it) and insert it using rte_eal_devargs_insert(). This is essentially
the solution I used for the rte_eal_hotplug_add() fix[1].

The alternative fix is to propose an API for buses to transform device
names into their canonical form on demand... And it would certainly only
be useful for the PCI bus.

The issue is discussed there:
[1]: http://dpdk.org/ml/archives/dev/2017-July/071155.html

> +		}
> +
> +		/* probe device */
>  		rdev = rte_eal_hotplug_add(da->bus->name,
>  					   da->name,
>  					   da->args);
> @@ -84,6 +101,10 @@ fs_bus_init(struct rte_eth_dev *dev)
>  		ETH(sdev)->state = RTE_ETH_DEV_DEFERRED;
>  		sdev->state = DEV_PROBED;
>  	}
> +

Superfluous line.

> +	if (first_init)
> +		first_init = 0;

first_init = false;

> +
>  	return 0;
>  }
>  
> -- 
> 2.13.2
> 

-- 
Gaëtan Rivet
6WIND


More information about the dev mailing list