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.ri...@6wind.com
> > > Cc: sta...@dpdk.org
> > >
> > > Signed-off-by: Shahaf Shuler <shah...@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
