Re: [dpdk-dev] [PATCH v4 04/12] net/failsafe: add fail-safe PMD

[Stephen Hemminger <stephen@networkplumber.org>]

On Mon, 29 May 2017 15:42:16 +0200
Gaetan Rivet <gaetan.rivet@6wind.com> wrote:
> +Fail-safe poll mode driver library
> +==================================
> +
> +The Fail-safe poll mode driver library (**librte_pmd_failsafe**) is a virtual
> +device that allows using any device supporting hotplug (sudden device removal
> +and plugging on its bus), without modifying other components relying on such
> +device (application, other PMDs).

What about the case of Hyper-V where the components of the Fail Safe PMD may
arrive later. An example would be a NFV server that starts on boot. The synthetic
device will be present at boot, but the associated VF device may be plugged
in later (by checking SR-IOV on host console) or removed (by unchecking).

There doesn't appear to be a way to manage slave devices that get added
and removed through CLI management model.



> +Using the Fail-safe PMD from the EAL command line
> +-------------------------------------------------
> +
> +The Fail-safe PMD can be used like most other DPDK virtual devices, by passing a
> +``--vdev`` parameter to the EAL when starting the application. The device name
> +must start with the *net_failsafe* prefix, followed by numbers or letters. This
> +name must be unique for each device. Each fail-safe instance must have at least one
> +sub-device, up to ``RTE_MAX_ETHPORTS-1``.
> +
> +A sub-device can be any legal DPDK device, including possibly another fail-safe
> +instance.

Configuring fail-safe (or any other device) from command line is difficult in a real
world application. The EAL command line is difficult API to manipulate programmatically.
Why not have a real API?

> +static int
> +fs_link_update(struct rte_eth_dev *dev,
> +		int wait_to_complete)
> +{
> +	struct sub_device *sdev;
> +	uint8_t i;
> +	int ret;
> +
> +	FOREACH_SUBDEV_ST(sdev, i, dev, DEV_ACTIVE) {
> +		DEBUG("Calling link_update on sub_device %d", i);
> +		ret = (SUBOPS(sdev, link_update))(ETH(sdev), wait_to_complete);
> +		if (ret && ret != -1) {
> +			ERROR("Link update failed for sub_device %d with error %d",
> +			      i, ret);
> +			return ret;
> +		}
> +	}
> +	if (TX_SUBDEV(dev)) {
> +		struct rte_eth_link *l1;
> +		struct rte_eth_link *l2;
> +
> +		l1 = &dev->data->dev_link;
> +		l2 = &ETH(TX_SUBDEV(dev))->data->dev_link;
> +		if (memcmp(l1, l2, sizeof(*l1))) {
> +			*l1 = *l2;
> +			return 0;
> +		}
> +	}
> +	return -1;
> +}

memcmp here is a potential problem since rte_eth_link maybe padded and have holes.
Why compare anyway? if *l1 == *l2 the assignment would be a nop.
What if links are down?


> +static void
> +fs_stats_get(struct rte_eth_dev *dev,
> +	     struct rte_eth_stats *stats)
> +{
> +	memset(stats, 0, sizeof(*stats));

memset here is unnecessary, already done by rte_eth_stats_get