From: Shahaf Shuler <shahafs@mellanox.com>
To: "Gaëtan Rivet" <gaetan.rivet@6wind.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: Thu, 17 Aug 2017 06:04:27 +0000 [thread overview]
Message-ID: <VI1PR05MB3149A7373A62CD39552E57E2C3830@VI1PR05MB3149.eurprd05.prod.outlook.com> (raw)
In-Reply-To: <20170816152607.GO8124@bidouze.vm.6wind.com>
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?
next prev parent reply other threads:[~2017-08-17 6:04 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 [this message]
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=VI1PR05MB3149A7373A62CD39552E57E2C3830@VI1PR05MB3149.eurprd05.prod.outlook.com \
--to=shahafs@mellanox.com \
--cc=dev@dpdk.org \
--cc=gaetan.rivet@6wind.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).