On 6/6/17, 5:46 AM, "[email protected] on behalf of Ilya
Maximets" <[email protected] on behalf of [email protected]>
wrote:
On 06.06.2017 11:30, Darrell Ball wrote:
> On port deletion, device resources cannot be cleaned
> up if the device was attached when vswitchd was started.
> The issue is mainly due to introduction of the 'attached'
> field for the netdev_dpdk device context. The logic
> setting the 'attached' field did not include startup
> handling. The use of an extra 'attached' field was
> introduced to guard against detaching a device that could
> not be detached, but should not be necessary as the rte and
> eal keep the attached state, as they must, and also filter
> trying to detach a device that is not detachable. If
> there were any bugs in this regard, which I did not find
> with basic testing, then they should be fixed in rte/eal.
Why you want to free resources of these devices?
It's, actually, was a default behaviour of OVS for years.
If you want to detach hotplugable devices than you may just not
attach them using cmdline.
Some testing:
1) Using a hotpluggable device; it is using the uio_pci_generic
driver.
a) I restart OVS with the device already bound.
When I do del-port to remove it, the code skips resource cleanup,
namely *_close and *_detach, bcoz the new internal OVS variable ‘attached’ is
not true, even though rte/eal considers it ‘attached.’
b) If I use the exact same device, but hot plug attach it this time and then do
del-port,
the device resources are cleaned up.
I would expect ‘a’ to do cleanup and also be consistent with ‘b’.
Moreover, this worked before
5dcde09c80a8 ("netdev-dpdk: Fix device leak on port deletion."),
as netdev_dpdk_detach() could be used to do the cleanup in the exact same way
for both ‘a’ and ‘b’, as this new patch can do, without the old API.
2) Now, let us move to the second point.
Not hotplugable devices will not
be detached anyway.
Correct, I mentioned in the commit message that the rte/eal code should block
this
and it does.
From the other side this patch will allow following scenario
for not detachable devices:
- add_port(05.00.0)
- configure()
- stop()
- close()
- detach(05.00.0) -----> Failure
Correct/expected.
- add_port(05.00.0)
- configure()
- start()
At this point OvS will get closed device by name and will
try to configure it and start again.
My concern is that we can't be sure that it will continue
work properly after close.
According to DPDK API, *device can not be started back after
close*. Unfortunately, there is no documentation about
trying to reinitialize closed device. So, we can't
rely on your assumption that device will work properly
after that.
Using my patch with VFIO and 16.11; VFIO is not detachable in this environment.
There is no problem seen; packet rates are the same before and after
del-port/re-add-port.
Using your previous patch, this is what I see:
In this case, ‘attached’ will be false and on del-port, no resources will be
freed -
_close will not be called and resources leaked.
So, you want to achieve blocking the _close call to free resources, in case we
want
to add the port back and because we are not sure if the device can be restarted
after _close?
Since we deleted the port, it is likely we won’t use it again, but we will want
to use
other ports, presumably.
Also, this would be a bug in rte/eal, if it was an issue, since we should be
able to free
resources on deleting a port, right ? Freeing unused resources should not make
it
impossible to reuse the device later by reattaching it, right ?
However, my earlier test shows this works with VFIO.
> Fixes: 5dcde09c80a8 ("netdev-dpdk: Fix device leak on port deletion.")
> CC: Ilya Maximets <[email protected]>
> Signed-off-by: Darrell Ball [email protected] ter
> ---
> lib/netdev-dpdk.c | 14 +++-----------
> 1 file changed, 3 insertions(+), 11 deletions(-)
>
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index b770b70..753345d 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -365,9 +365,6 @@ struct netdev_dpdk {
> /* Device arguments for dpdk ports */
> char *devargs;
>
> - /* If true, device was attached by rte_eth_dev_attach(). */
> - bool attached;
> -
> /* In dpdk_list. */
> struct ovs_list list_node OVS_GUARDED_BY(dpdk_mutex);
>
> @@ -862,7 +859,6 @@ common_construct(struct netdev *netdev, dpdk_port_t
port_no,
> dev->max_packet_len = MTU_TO_FRAME_LEN(dev->mtu);
> ovsrcu_index_init(&dev->vid, -1);
> dev->vhost_reconfigured = false;
> - dev->attached = false;
>
> ovsrcu_init(&dev->qos_conf, NULL);
>
> @@ -1016,7 +1012,7 @@ netdev_dpdk_destruct(struct netdev *netdev)
>
> rte_eth_dev_stop(dev->port_id);
>
> - if (dev->attached) {
> + if (rte_eth_dev_is_valid_port(dev->port_id)) {
> rte_eth_dev_close(dev->port_id);
> if (rte_eth_dev_detach(dev->port_id, devname) < 0) {
> VLOG_ERR("Device '%s' can not be detached", dev->devargs);
> @@ -1136,8 +1132,7 @@ netdev_dpdk_lookup_by_port_id(dpdk_port_t port_id)
> }
>
> static dpdk_port_t
> -netdev_dpdk_process_devargs(struct netdev_dpdk *dev,
> - const char *devargs, char **errp)
> +netdev_dpdk_process_devargs(const char *devargs, char **errp)
> {
> /* Get the name up to the first comma. */
> char *name = xmemdup0(devargs, strcspn(devargs, ","));
> @@ -1148,8 +1143,6 @@ netdev_dpdk_process_devargs(struct netdev_dpdk *dev,
> || !rte_eth_dev_is_valid_port(new_port_id)) {
> /* Device not found in DPDK, attempt to attach it */
> if (!rte_eth_dev_attach(devargs, &new_port_id)) {
> - /* Attach successful */
> - dev->attached = true;
> VLOG_INFO("Device '%s' attached to DPDK", devargs);
> } else {
> /* Attach unsuccessful */
> @@ -1236,8 +1229,7 @@ netdev_dpdk_set_config(struct netdev *netdev, const
struct smap *args,
> * is valid */
> if (!(dev->devargs && !strcmp(dev->devargs, new_devargs)
> && rte_eth_dev_is_valid_port(dev->port_id))) {
> - dpdk_port_t new_port_id = netdev_dpdk_process_devargs(dev,
> -
new_devargs,
> + dpdk_port_t new_port_id =
netdev_dpdk_process_devargs(new_devargs,
> errp);
> if (!rte_eth_dev_is_valid_port(new_port_id)) {
> err = EINVAL;
>
_______________________________________________
dev mailing list
[email protected]
https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=G8QfrUVNm6ggIVwO3aMkZCC7YjM9GV0vFsv3waiJ0C0&s=cXxznk6aRq6m4xUyF4hzRow5hhSTQPC7gj0ahmI7LZs&e=
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
