On 07.06.2017 04:46, Darrell Ball wrote:
>
>
> 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.’
At first, I already told in discussion of my previous patch that
'dev->attached' is not the copy of internal state of device inside dpdk.
They are completely different.
About your testing.
I tried this:
1. Start ovs-vswitchd
2. ovs-vsctl add-port br1 dpdk0 -- set Interface dpdk0 type=dpdk
options:dpdk-devargs=0000:88:00.0
--> |netdev_dpdk|INFO|Device '0000:88:00.0' attached to DPDK
3. Restart ovs-vswitchd
4. ovs-vsctl del-port dpdk0
--> |netdev_dpdk|INFO|Device '0000:88:00.0' detached
So, all resources freed as expected.
What I'm doing wrong?
Or port was bound using whitelist in 'other_ocnfig:dpdk-extra' ? In this case
it's expected behaviour that it will not be detached.
It's reasonable because changes in 'dpdk-extra' requires restart and device
will not be attached after restart if removed from list in 'dpdk-extra'.
> 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.
Resources are not actually leaked because you're able to reopen the device.
> 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?
Yes. I found at least one device that doesn't support hotplug and will not work
after close.
It is 'Chelsio T5' NIC (cxgbe). On close it will call 't4_fw_bye' to close
connection with NICs firmware and it will not open it back because 't4_fw_hello'
can be called only on 'eth_cxgbe_pci_probe'. Such behaviour reasonable for this
driver because of some unusual NIC architecture (It has only 1 pci address for
all it's ports).
So, with this patch applied it will be impossible to use 'Chelsio T5' NIC with
OvS.
> Since we deleted the port, it is likely we won’t use it again, but we will
> want to use
> other ports, presumably.
That is not true because there are some actions that leads to removing and
subsequent
re-adding ports to the datapath. For example, setting the 'ofport_request':
ovs-vsctl set Interface dpdk4 ofport_request=42
---> |bridge|INFO|bridge br10_4f0: deleted interface dpdk4 on port 1
|netdev_dpdk|INFO|Device '0000:88:00.0' detached
|netdev_dpdk|INFO|Device '0000:88:00.0' attached to DPDK
|bridge|INFO|bridge br10_4f0: added interface dpdk4 on port 42
>
> 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 ?
I don't think so because we passed this device as cmdline argument and we,
actually,
need to restart application to change cmdline arguments. And we have no issues
with resource freeing if device wasn't passed via cmdline.
> Freeing unused resources should not make it
> impossible to reuse the device later by reattaching it, right ?
The key word here is 'reattaching', but we're not reattaching it.
You're just tries to reconfigure closed device while DPDK API
says *"The device cannot be restarted!"*.
> However, my earlier test shows this works with VFIO.
At least 'cxgbe' will not work. I have no such NIC for testing, but it's
clear from the code.
>
> > 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]
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
