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: Fri, 18 Aug 2017 11:52:50 +0200 [thread overview]
Message-ID: <20170818095250.GT8124@bidouze.vm.6wind.com> (raw)
In-Reply-To: <VI1PR05MB3149A7373A62CD39552E57E2C3830@VI1PR05MB3149.eurprd05.prod.outlook.com>
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
prev parent reply other threads:[~2017-08-18 9:53 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
2017-08-17 6:04 ` Shahaf Shuler
2017-08-18 9:52 ` Gaëtan Rivet [this message]
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=20170818095250.GT8124@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).