10/01/2019 15:23, Ilya Maximets:
> On 10.01.2019 16:42, Thomas Monjalon wrote:
> > Hi,
> >
> > 10/01/2019 12:32, Ilya Maximets:
> >> On 08.01.2019 12:31, Ophir Munk wrote:
> >>> if (dev->attached) {
> >>> + /* Remove the port eth device. */
> >>> rte_eth_dev_close(dev->port_id);
> >>> - rte_eth_dev_info_get(dev->port_id, &dev_info);
> >>> - if (dev_info.device && !rte_dev_remove(dev_info.device)) {
> >>> - VLOG_INFO("Device '%s' has been detached", dev->devargs);
> >>
> >> Please keep this log message for the successful case.
> >>
> >>> - } else {
> >>> - VLOG_ERR("Device '%s' can not be detached", dev->devargs);
> >>> + VLOG_INFO("Device '%s' has been removed", dev->devargs);
> >>> + /* If this is the last port_id using this rte device
> >>> + * remove this rte device and all its eth devices.
> >>> + * FIXME: avoid direct access to DPDK internal array
> >>> rte_eth_devices.
> >>> + */
> >>> + rte_dev = rte_eth_devices[dev->port_id].device;
> >>> + if (netdev_dpdk_get_num_ports(rte_dev) == 1) {
> >>
> >> rte_eth_dev_close() sets the RTE_ETH_DEV_UNUSED state for PMDs with
> >> RTE_ETH_DEV_CLOSE_REMOVE flag.
> >> The function netdev_dpdk_get_num_ports() skips unused ports.
> >> The result should not be equal to 1. We need the other way to detect
> >> the siblings or close the port after calculating them.
> >> In this case we'll probably do not need to have check for UNUSED state
> >> inside the _get_num_ports().
> >
> > Maybe I don't understand correctly.
> > Why do you want to count closed sibling ports?
>
> Proposed version of 'netdev_dpdk_get_num_ports' iterates over
> 'dpdk_list' which contains only ports that currently opened by OVS.
> So, the only port that could have UNUSED state in that list is the
> port that we're just closed here. If we'll count number of siblings
> before closing the port where should be no UNUSED ports in that list.
>
> The issue I tried to raise here is that rte_eth_dev_close() changed the
> state only for PMDs with RTE_ETH_DEV_CLOSE_REMOVE. So, the result
> of 'netdev_dpdk_get_num_ports' will be different for different PMDs.
Yes, that's true.
You can manage it two ways:
- you rely on OVS list of open ports
- you remove the device completely (as before)
for PMDs not supporting RTE_ETH_DEV_CLOSE_REMOVE
> >>> + if (rte_dev_remove(rte_dev) < 0) {
> >>> + VLOG_ERR("Device '%s' can not be detached",
> >>> dev->devargs);
> >>> + }
> >>> }
> >>> }
> >
> > [...]
> >> Also, do we still need netdev_dpdk_get_port_by_mac() if we're using
> >> devargs iterator ?
> >
> > The devargs iterator manages "mac=" property in DPDK 18.11.
>
> Good.
>
> >
> > [...]
> >>> - rte_eth_dev_close(port_id);
> >>> + /* FIXME: avoid direct access to DPDK internal array
> >>> rte_eth_devices. */
> >>> + rte_dev = rte_eth_devices[port_id].device;
> >>> + if (netdev_dpdk_get_num_ports(rte_dev)) {
> >>
> >> Probably same here.
> >> DPDK does not set RTE_ETH_DEV_UNUSED state for PMDs without
> >> RTE_ETH_DEV_CLOSE_REMOVE flag.
> >
> > If RTE_ETH_DEV_CLOSE_REMOVE is not set, you should call rte_dev_remove()
> > just after rte_eth_dev_close().
>
> And what if we call rte_dev_remove() after rte_eth_dev_close() for
> PMD with RTE_ETH_DEV_CLOSE_REMOVE ?
Then you release the full device, as before with the old "detach" API.
It makes no difference for single-port device.
But it is bad for multi-port devices (which should already implement
RTE_ETH_DEV_CLOSE_REMOVE).
> >>> + response = xasprintf("Device '%s' is being shared with other "
> >>> + "interfaces. Remove them before detaching.",
> >>> + argv[1]);
> >>> + goto error;
> >>> + }
> >>>
> >>> - rte_eth_dev_info_get(port_id, &dev_info);
> >>> - if (!dev_info.device || rte_dev_remove(dev_info.device)) {
> >>> - response = xasprintf("Device '%s' can not be detached", argv[1]);
> >>> + rte_eth_dev_close(port_id);
> >>> + if (rte_dev_remove(rte_dev) < 0) {
> >>> + response = xasprintf("Device '%s' can not be removed", argv[1]);
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
