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

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

[Shahaf Shuler]

Currently device state moves between ATTACHED when device was
successfully probed to UNUSED when device is detached or released.

The device state following rte_eth_dev_close() operation is inconsist,
The device is still in ATTACHED state, however it cannot be used
in any way till it will be probed again.

Fixing it by changing the state to UNUSED.

Fixes: d52268a8b24b ("ethdev: expose device states")
Cc: gaetan.rivet@6wind.com
Cc: stable@dpdk.org

Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
---
 lib/librte_ether/rte_ethdev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 0597641ee..98d9e929c 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -992,6 +992,8 @@ rte_eth_dev_close(uint8_t port_id)
 	dev->data->nb_tx_queues = 0;
 	rte_free(dev->data->tx_queues);
 	dev->data->tx_queues = NULL;
+
+	dev->state = RTE_ETH_DEV_UNUSED;
 }
 
 int
-- 
2.12.0

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

[Gaëtan Rivet]

Hello Shahaf,

On Wed, Aug 16, 2017 at 02:43:08PM +0300, Shahaf Shuler wrote:
> Currently device state moves between ATTACHED when device was
> successfully probed to UNUSED when device is detached or released.
> 
> The device state following rte_eth_dev_close() operation is inconsist,
> The device is still in ATTACHED state, however it cannot be used
> in any way till it will be probed again.
> 
> Fixing it by changing the state to UNUSED.
> 

You are right that simply closing the device leaves it in a unusable
state.

However it seems to be by design.
Most drivers call `rte_eth_dev_release_port` when being removed, which
sets the state to RTE_ETH_DEV_UNUSED.

If I'm not mistaken, the API of rte_eth_dev_close is that the only
available action should then be to detach the driver. At least PCI and
vdev buses expects a `remove` callback from their driver, which can be
called by the user (previously using specific API like
`rte_eal_vdev_uninit` for example, now using `rte_eal_hotplug_remove` or
`rte_eth_dev_detach` from the ether layer).

So, it seems that this burden lies with the driver which should call the
proper API when removing their device.

Maybe Thomas will have a better insight about the scope of the
`rte_eth_dev_close` function. But IMO the API is respected.
After all, until the proper `dev_detach` function is called, the device
is still attached, even if closed.

If you disagree, there might possibly be an argument to make about
either adding finer-grained device states or streamlining the API. This
is however a discussion about API design and not about its implementation
anymore.

> Fixes: d52268a8b24b ("ethdev: expose device states")
> Cc: gaetan.rivet@6wind.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> ---
>  lib/librte_ether/rte_ethdev.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index 0597641ee..98d9e929c 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -992,6 +992,8 @@ rte_eth_dev_close(uint8_t port_id)
>  	dev->data->nb_tx_queues = 0;
>  	rte_free(dev->data->tx_queues);
>  	dev->data->tx_queues = NULL;
> +
> +	dev->state = RTE_ETH_DEV_UNUSED;
>  }
>  
>  int
> -- 
> 2.12.0
> 

-- 
Gaëtan Rivet
6WIND

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

[Shahaf Shuler]

Wednesday, August 16, 2017 3:42 PM, Gaëtan Rivet:
> On Wed, Aug 16, 2017 at 02:43:08PM +0300, Shahaf Shuler wrote:
> > Currently device state moves between ATTACHED when device was
> > successfully probed to UNUSED when device is detached or released.
> >
> > The device state following rte_eth_dev_close() operation is inconsist,
> > The device is still in ATTACHED state, however it cannot be used in
> > any way till it will be probed again.
> >
> > Fixing it by changing the state to UNUSED.
> >
> 
> You are right that simply closing the device leaves it in a unusable state.
> 
> However it seems to be by design.
> Most drivers call `rte_eth_dev_release_port` when being removed, which
> sets the state to RTE_ETH_DEV_UNUSED.
> 
> If I'm not mistaken, the API of rte_eth_dev_close is that the only available
> action should then be to detach the driver. At least PCI and vdev buses
> expects a `remove` callback from their driver, which can be called by the user
> (previously using specific API like `rte_eal_vdev_uninit` for example, now
> using `rte_eal_hotplug_remove` or `rte_eth_dev_detach` from the ether
> layer).
> 
> So, it seems that this burden lies with the driver which should call the proper
> API when removing their device.

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. 

> 
> Maybe Thomas will have a better insight about the scope of the
> `rte_eth_dev_close` function. But IMO the API is respected.
> After all, until the proper `dev_detach` function is called, the device is still
> attached, even if closed.
> 
> If you disagree, there might possibly be an argument to make about either
> adding finer-grained device states or streamlining the API. This is however a
> discussion about API design and not about its implementation anymore.

Well my first thought when I created this patch was to add fine-grained device states. However then I read the commit log which changed the device states, specifically :

" "DEV_DETACHED" is renamed "RTE_ETH_DEV_UNUSED" to better reflect that
the emptiness of a slot is not necessarily the result of detaching a
device."

Which convince me to reuse the UNUSED state to reflect that this device cannot longer be used (even though it is still attached). 

> 
> > Fixes: d52268a8b24b ("ethdev: expose device states")
> > Cc: gaetan.rivet@6wind.com
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> > ---
> >  lib/librte_ether/rte_ethdev.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/lib/librte_ether/rte_ethdev.c
> > b/lib/librte_ether/rte_ethdev.c index 0597641ee..98d9e929c 100644
> > --- a/lib/librte_ether/rte_ethdev.c
> > +++ b/lib/librte_ether/rte_ethdev.c
> > @@ -992,6 +992,8 @@ rte_eth_dev_close(uint8_t port_id)
> >  	dev->data->nb_tx_queues = 0;
> >  	rte_free(dev->data->tx_queues);
> >  	dev->data->tx_queues = NULL;
> > +
> > +	dev->state = RTE_ETH_DEV_UNUSED;
> >  }
> >
> >  int
> > --
> > 2.12.0
> >
> 
> --
> Gaëtan Rivet
> 6WIND

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

[Gaëtan Rivet]

On Wed, Aug 16, 2017 at 02:17:16PM +0000, Shahaf Shuler wrote:
> Wednesday, August 16, 2017 3:42 PM, Gaëtan Rivet:
> > On Wed, Aug 16, 2017 at 02:43:08PM +0300, Shahaf Shuler wrote:
> > > Currently device state moves between ATTACHED when device was
> > > successfully probed to UNUSED when device is detached or released.
> > >
> > > The device state following rte_eth_dev_close() operation is inconsist,
> > > The device is still in ATTACHED state, however it cannot be used in
> > > any way till it will be probed again.
> > >
> > > Fixing it by changing the state to UNUSED.
> > >
> > 
> > You are right that simply closing the device leaves it in a unusable state.
> > 
> > However it seems to be by design.
> > Most drivers call `rte_eth_dev_release_port` when being removed, which
> > sets the state to RTE_ETH_DEV_UNUSED.
> > 
> > If I'm not mistaken, the API of rte_eth_dev_close is that the only available
> > action should then be to detach the driver. At least PCI and vdev buses
> > expects a `remove` callback from their driver, which can be called by the user
> > (previously using specific API like `rte_eal_vdev_uninit` for example, now
> > using `rte_eal_hotplug_remove` or `rte_eth_dev_detach` from the ether
> > layer).
> > 
> > So, it seems that this burden lies with the driver which should call the proper
> > API when removing their device.
> 
> 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.

> > 
> > Maybe Thomas will have a better insight about the scope of the
> > `rte_eth_dev_close` function. But IMO the API is respected.
> > After all, until the proper `dev_detach` function is called, the device is still
> > attached, even if closed.
> > 
> > If you disagree, there might possibly be an argument to make about either
> > adding finer-grained device states or streamlining the API. This is however a
> > discussion about API design and not about its implementation anymore.
> 
> Well my first thought when I created this patch was to add fine-grained device states. However then I read the commit log which changed the device states, specifically :
> 
> " "DEV_DETACHED" is renamed "RTE_ETH_DEV_UNUSED" to better reflect that
> the emptiness of a slot is not necessarily the result of detaching a
> device."
> 
> Which convince me to reuse the UNUSED state to reflect that this device cannot longer be used (even though it is still attached). 
> 

When the device is closed, it could still be in the
`RTE_ETH_DEV_DEFERRED` state. This state means that the device is
managed by a third party (application or whatever). It is forbidden for
the ethdev layer to change the state of the device unless said
third-party releases it.

The only place where the device state could unequivocally be set to
UNUSED is upon the proper release of the device. As far as the ethdev
layer is concerned, this is currently within rte_eth_dev_detach.

> > 
> > > Fixes: d52268a8b24b ("ethdev: expose device states")
> > > Cc: gaetan.rivet@6wind.com
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> > > ---
> > >  lib/librte_ether/rte_ethdev.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/lib/librte_ether/rte_ethdev.c
> > > b/lib/librte_ether/rte_ethdev.c index 0597641ee..98d9e929c 100644
> > > --- a/lib/librte_ether/rte_ethdev.c
> > > +++ b/lib/librte_ether/rte_ethdev.c
> > > @@ -992,6 +992,8 @@ rte_eth_dev_close(uint8_t port_id)
> > >  	dev->data->nb_tx_queues = 0;
> > >  	rte_free(dev->data->tx_queues);
> > >  	dev->data->tx_queues = NULL;
> > > +
> > > +	dev->state = RTE_ETH_DEV_UNUSED;
> > >  }
> > >
> > >  int
> > > --
> > > 2.12.0
> > >
> > 
> > --
> > Gaëtan Rivet
> > 6WIND

-- 
Gaëtan Rivet
6WIND

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

[Shahaf Shuler]

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? 

Re: [dpdk-stable] [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