[dpdk-dev] [PATCH] ethdev: fix device state on close

[Gaëtan Rivet]

On Thu, Aug 17, 2017 at 06:04:27AM +0000, Shahaf Shuler wrote:
> Wednesday, August 16, 2017 6:26 PM, Gaëtan Rivet:
> > > Even though it is reasonable for driver to call the
> > rte_eth_dev_port_release, I still think the ethdev layer should protect from
> > such bad behavior from the application side.
> > > It is more robust than counting on the different PMD to implement such
> > release.
> > >
> > 
> > The ethdev layer does so in the rte_eth_dev_detach function, which is
> > currently the only way to detach a device from the ethdev level.
> > 
> > `rte_eth_dev_detach` setting the device state seems to me to be a crutch
> > against badly implemented drivers. If I was nitpicky I would actually remove it
> > and ideally everyone should enforce the call of rte_eth_dev_release_port
> > from device removal functions when reviewing PMD implementations.
> > 
> > The hotplug API is available to applications and the only way to have
> > consistent devices states after calling rte_eal_hotplug_remove is to have
> > drivers using a hook in the ethdev layer allowing to clean-up resources upon
> > release. While it is trivial in its current state, such entry-point in the ethdev
> > layer will be useful and should be kept and enforced IMO.
> > 
> > I'm thinking about the 16-bit portid scheduled for v17.11, which implies an
> > arbitrary number of port available. This would imply a dynamic allocation of
> > rte_eth_devices, which would *need* such release hook to be available.
> > Well the API should not be designed from conjectures or speculations of
> > course, but I think it should be useful and there is no reason to remove it.
> > 
> > Going further, I find it dangerous to have two points where the port is
> > semantically released (device state set to UNUSED). If the API of the port
> > release changes, we now have two points where we need to enforce the
> > changes. While trivial concerning an enum, it could become more complex /
> > dangerous if this veered toward memory management.
> 
> Those are valid concerns, and you convinced me the RTE_ETH_DEV_UNUSED cannot be set after device close.
> 
> I still think the ethdev layer missing protection against driver calls (other than detach) following a device close. The API not allows, but the ethdev should enforce it.
> Considering some driver memset to 0 their control structs after device close, with no protection such bad calls might lead to segfault, which is not what we want even under bad behavior. 
> 
> One could suggest to protect it inside the driver by replacing the vtable of the driver ops, however it will introduce a lot of duplicated code which can easily be avoided if ethdev would not pass such
> Calls after device close.
> 
> So, how about adding another state which will indicate the device is closed, cannot be used, yet still attached? 
> 

It's feasible. It might be useful. Adding finer device states seems
logical to me, I made those explicit in the fail-safe.

However, in the fail-safe I had the need to make automatic the handling
of devices, meaning that these device states had to be properly
abstracted and parseable.

Adding finer device states in ethdev:

Pros:

  - Easier to follow API and to spot usage mistakes.
  - Increasing the rigidity of the API might make apparent mistakes from
    PMD implementations. Helps PMD maintainers.

Cons:

  - Some of those "mistakes" might be due to hardware-specific
    limitations that cannot be bypassed. I'm thinking about the
    flow-isolation implementation in MLX4 that had specific requirements
    about when it could be enabled, that would actually be forbidden by
    this proposal as it had to be done before rte_eth_dev_configure.
  - Handling special cases could quickly become a mess of edge-cases
    with workarounds here and there. Making the API evolve would imply
    untangling this mess at times.

I'm considering there a full-blown device state implementation. I know
you are only proposing adding an intermediate device state allowing to
protect the user from using a closed device.

But this proposal should be examined on the direction it would lead us
into. It might be simpler for example to introduce a flag "dev_closed"
in rte_eth_dev_data (along dev_started) for example, and checking only
on this in the relevant functions.

I don't have a strong opinion about the finer-grained device states.
I only wanted to lay out what I saw as possible longer-term effects of
choosing this solution and a possible alternative.

-- 
Gaëtan Rivet
6WIND