From: "Gaëtan Rivet" <gaetan.rivet@6wind.com>
To: Shahaf Shuler <shahafs@mellanox.com>
Cc: Thomas Monjalon <thomas@monjalon.net>,
"dev@dpdk.org" <dev@dpdk.org>,
"stable@dpdk.org" <stable@dpdk.org>
Subject: Re: [dpdk-stable] [PATCH] ethdev: fix device state on close
Date: Wed, 16 Aug 2017 17:26:07 +0200 [thread overview]
Message-ID: <20170816152607.GO8124@bidouze.vm.6wind.com> (raw)
In-Reply-To: <VI1PR05MB3149204676C8AA95E2163396C3820@VI1PR05MB3149.eurprd05.prod.outlook.com>
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
next prev parent reply other threads:[~2017-08-16 15:26 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-16 11:43 Shahaf Shuler
2017-08-16 12:41 ` Gaëtan Rivet
2017-08-16 14:17 ` Shahaf Shuler
2017-08-16 15:26 ` Gaëtan Rivet [this message]
2017-08-17 6:04 ` Shahaf Shuler
2017-08-18 9:52 ` Gaëtan Rivet
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170816152607.GO8124@bidouze.vm.6wind.com \
--to=gaetan.rivet@6wind.com \
--cc=dev@dpdk.org \
--cc=shahafs@mellanox.com \
--cc=stable@dpdk.org \
--cc=thomas@monjalon.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).