Re: [dpdk-dev] [PATCH v10 03/11] net/failsafe: add fail-safe PMD

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

On Mon, Jul 17, 2017 at 02:56:54PM +0100, Ferruh Yigit wrote:
> On 7/15/2017 6:57 PM, Gaetan Rivet wrote:
> > Introduce the fail-safe poll mode driver initialization and enable its
> > build infrastructure.
> > 
> > This PMD allows for applications to benefit from true hot-plugging
> > support without having to implement it.
> > 
> > It intercepts and manages Ethernet device removal events issued by
> > slave PMDs and re-initializes them transparently when brought back.
> > It also allows defining a contingency to the removal of a device, by
> > designating a fail-over device that will take on transmitting operations
> > if the preferred device is removed.
> > 
> > Applications only see a fail-safe instance, without caring for
> > underlying activity ensuring their continued operations.
> 
> All PMD in a single patch is hard to review, I am sure some details
> missed during the review, but taking account the histroy of the PMD I
> accept this as it is, but I will rely on your support to fix issues in
> the future.
> 

Sure, sorry for having this one first big patch.
I thought about having a skeleton patch first, but found it made little
sense. I tried to restrict this version to the bare functionalities,
adding the others afterward.

I will fix any issues. From what I've seen I agree with almost all of your
remarks and will send a new version shortly. In the meantime, I will answer
in this email a few clarifying questions.

<...>

> > +VLAN filter          = Y
> > +Packet type parsing  = Y
> 
> I am not sure how to document some of these features, because they
> depends on sub-device capability. I guess if sub-device doesn't support
> packet type parsing, this feature won't be supported?
> 

Yes, supporting a feature for the fail-safe means that there is some
verification and synchronization code related to this feature. All
sub_device should have feature parity, and the features of the fail-safe are limited
to those of the sub_devices.

I thought advertizing the support made sense as there was some code
related to it in the fail-safe.

> > +int
> > +failsafe_args_parse(struct rte_eth_dev *dev, const char *params)
> > +{
> > +	struct fs_priv *priv;
> > +	char mut_params[DEVARGS_MAXLEN] = "";
> 
> Out of curiosity, what does "mut" stands for?
> 

This is the mutable version of params.

<...>

> > +	n = snprintf(mut_params, sizeof(mut_params), "%s", params);
> > +	if (n >= sizeof(mut_params)) {
> > +		ERROR("Parameter string too long (>=%zu)",
> > +				sizeof(mut_params));
> > +		return -ENOMEM;
> > +	}
> > +	ret = fs_parse_sub_devices(fs_parse_device_param,
> > +				   dev, params);
> 
> Why the device argument is not defined as dev=xxx, instead of current
> dev(xxx).
> 
> "dev=xxx" will be compatible with rest of the argument usage, and it
> will be possible to use kvargs to parse it, which will make this code
> simpler I believe.
> 
> What is the reason of using different syntax?
> 

Using the dev() syntax allows the user to explicitly set the limits of
the sub_device declaration, clarifying for which device each kvargs is.

The issue is that the kvargs library does not allow to set state
informations in the parser depending on the position in the kvlist. An
alternative would have been for example to restrict the kvargs to that
of the last declared dev=, however, this means multi-stage kvargs
parsing, which mean pre-processing of the parameter list, etc...

An example:

net_failsafe0,dev=net_tap0,iface=tap0,mac=00:01:02:03:04:05
net_failsafe0,dev(net_tap0,iface=tap0),mac=00:01:02:03:04:05

This is much simpler to parse this way, and much clearer I think
for users.

The kvargs library was not designed with recursive PMDs in mind.

<...>

> > +static int
> > +fs_bus_init(struct rte_eth_dev *dev)
> > +{
> > +	struct sub_device *sdev;
> > +	struct rte_devargs *da;
> > +	uint8_t i;
> > +	int ret;
> > +
> > +	FOREACH_SUBDEV(sdev, i, dev) {
> 
> Can FOREACH_SUBDEV_ST(..., DEV_PARSED) be used here?
> 

I could use it, this would restrict the iteration only to sub_devices
being at least of the state DEV_PARSED. However, in the check just
below:

+		if (sdev->state != DEV_PARSED)
+			continue;

I would have to pass on any device being in a state higher than
DEV_PARSED. Thus, using FOREACH_SUBDEV_ST would not simplify the code
flow. By using FOREACH_SUBDEV() directly, the reader at least has a
simpler parsing to do of my intent:

foreach subdev not "parsed".

> And what do you think renaming "FOREACH_SUBDEV_ST" to
> "FOREACH_SUBDEV_STATE"?
> 

Sure, I pushed for brievity but it might be easier to read.

<...>

> > +	FOREACH_SUBDEV_ST(sdev, i, dev, DEV_ACTIVE) {
> > +		DEBUG("Closing sub_device %d", i);
> > +		rte_eth_dev_close(PORT_ID(sdev));
> > +		sdev->state = DEV_ACTIVE - 1;
> 
> Should it be better to set state to DEV_PROBED? Instead of calculation.
> 

I wanted to be able to add / remove device states without having to
rewrite each of those state changes (there are a few in several places).
If I insert a new device state between ACTIVE and PROBED, setting to
DEV_PROBED would still be valid (no compile error), but it would be a
bug. It would be very easy to miss a reference to this specific state.

Those states bugs are a little hard to find at runtime, they usually
have subtle side-effects.

I can change it if you prefer, but I would probably introduce a helper
in the form of fs_dev_state_prev/next, thus having a single place to
check for any changes.

-- 
Gaëtan Rivet
6WIND