Hi Ilya,
Thanks for the quick review.
Please find comments inline.
> -----Original Message-----
> From: Ilya Maximets <[email protected]>
> Sent: Wednesday, January 16, 2019 5:42 PM
> To: Ophir Munk <[email protected]>; [email protected]; Ian
> Stokes <[email protected]>
> Cc: Asaf Penso <[email protected]>; Shahaf Shuler
> <[email protected]>; Thomas Monjalon <[email protected]>; Olga
> Shern <[email protected]>; Kevin Traynor <[email protected]>; Aaron
> Conole <[email protected]>
> Subject: Re: [PATCH v5] netdev-dpdk: support port representors
>
> Not a full review.
> Comments inline.
>
> Best regards, Ilya Maximets.
>
> >
> > @@ -1361,12 +1382,32 @@ netdev_dpdk_destruct(struct netdev *netdev)
> > dev->started = false;
> >
> > if (dev->attached) {
> > + /* Retrieve eth device data before closing it.
> > + * FIXME: avoid direct access to DPDK internal array
> > rte_eth_devices.
> > + */
> > + eth_dev = &rte_eth_devices[dev->port_id];
> > + uint32_t remove_on_close =
> > + eth_dev->data->dev_flags & RTE_ETH_DEV_CLOSE_REMOVE;
>
> All other variables declared at the start of this function.
> Please, be consistent.
>
In v6
> > + rte_dev = eth_dev->device;
> > +
> > + /* Remove the 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);
> > +
> > + /* Remove this rte device and all its eth devices if flag
> > + * RTE_ETH_DEV_CLOSE_REMOVE is not supported (which means
> representors
> > + * are not supported), or if all the eth devices belonging to the
> > rte
> > + * device are closed.
> > + */
> > + if (!remove_on_close || !netdev_dpdk_get_num_ports(rte_dev)) {
> > + if (rte_dev_remove(rte_dev) < 0) {
> > + VLOG_ERR("Device '%s' can not be detached", dev->devargs);
> > + } else {
> > + /* Device was closed and detached. */
> > + VLOG_INFO("Device '%s' has been detached", dev->devargs);
> > + }
> > } else {
> > - VLOG_ERR("Device '%s' can not be detached", dev->devargs);
> > + /* Device was only closed. rte_dev_remove() was not called. */
> > + VLOG_INFO("Device '%s' has been removed", dev->devargs);
>
> IMHO, this should be printed regardless of detaching. (Before it)
>
As I suggested to Kevin in a recent review - "Device has been detached" was
changed to "Device has been removed and detached".
This will accurately reflect the operations on the device.
> > +
> > +static int
> > +netdev_dpdk_check_dup_dev(dpdk_port_t port_id, struct netdev *netdev,
> > + const char *devargs, char **errp) {
>
> OVS_REQUIRES(dpdk_mutex)
>
> Also, the '{' should be on a new line in function definitions.
>
This function is removed in v6
> > + struct netdev_dpdk *dup_dev;
> > +
> > + dup_dev = netdev_dpdk_lookup_by_port_id(port_id);
> > + if (dup_dev) {
> > + VLOG_WARN_BUF(errp, "'%s' is trying to use device '%s' "
> > + "which is already in use by '%s'",
> > + netdev_get_name(netdev), devargs,
> > + netdev_get_name(&dup_dev->up));
> > + return EADDRINUSE;
> > + }
> > + return 0;
>
> Indents are off in the previous 3 lines.
>
In v6
> > +}
> > +
> > /*
> > - * Normally, a PCI id is enough for identifying a specific DPDK port.
> > + * Normally, a PCI id (optionally followed by a representor number)
> > + * is enough for identifying a specific DPDK port.
> > * However, for some NICs having multiple ports sharing the same PCI
> > * id, using PCI id won't work then.
> > *
> > @@ -1641,32 +1728,35 @@ netdev_dpdk_get_port_by_mac(const char
> *mac_str)
> > *
> > * Note that the compatibility is fully kept: user can still use the
> > * PCI id for adding ports (when it's enough for them).
> > + *
> > + * In order to avoid clang errors add OVS_REQUIRES(dpdk_mutex) to
> > + this function
> > + * since it is required by netdev_dpdk_lookup_by_port_id() while the
> > + dpdk_mutex
> > + * is taken outside of this function.
>
> Agree with Aaron that the comment is not needed.
>
Comment removed in v6
> > */
> > static dpdk_port_t
> > netdev_dpdk_process_devargs(struct netdev_dpdk *dev,
> > const char *devargs, char **errp)
> > + OVS_REQUIRES(dpdk_mutex)
> > {
> > - char *name;
> > dpdk_port_t new_port_id = DPDK_ETH_PORT_ID_INVALID;
> >
> > if (strncmp(devargs, "class=eth,mac=", 14) == 0) {
> > new_port_id = netdev_dpdk_get_port_by_mac(&devargs[14]);
> > } else {
> > - name = xmemdup0(devargs, strcspn(devargs, ","));
> > - if (rte_eth_dev_get_port_by_name(name, &new_port_id)
> > - || !rte_eth_dev_is_valid_port(new_port_id)) {
> > - /* Device not found in DPDK, attempt to attach it */
> > - if (!rte_dev_probe(devargs)
> > - && !rte_eth_dev_get_port_by_name(name, &new_port_id)) {
> > - /* Attach successful */
> > - dev->attached = true;
> > - VLOG_INFO("Device '%s' attached to DPDK", devargs);
> > + new_port_id = netdev_dpdk_probe_port_by_devargs(devargs);
> > + if (!rte_eth_dev_is_valid_port(new_port_id)) {
> > + new_port_id = DPDK_ETH_PORT_ID_INVALID;
> > + } else {
> > + if (netdev_dpdk_check_dup_dev(new_port_id, &dev->up,
> > + devargs, errp)) {
> > + new_port_id = DPDK_ETH_PORT_ID_INVALID;
>
> Still don't understand why you need to check duplicated ports twice.
> Upper layer should handle this case.
> Also, indents are off.
Duplicate check is removed in v6
> > +1846,9 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct
> smap *args,
> > /* Already configured, do not reconfigure again */
> > err = 0;
> > } else {
> > - struct netdev_dpdk *dup_dev;
> > -
> > - dup_dev = netdev_dpdk_lookup_by_port_id(new_port_id);
> > - if (dup_dev) {
> > - VLOG_WARN_BUF(errp, "'%s' is trying to use device '%s'
> > "
> > - "which is already in use by '%s'",
> > - netdev_get_name(netdev), new_devargs,
> > - netdev_get_name(&dup_dev->up));
> > - err = EADDRINUSE;
> > - } else {
> > + err = netdev_dpdk_check_dup_dev(new_port_id, netdev,
> > + new_devargs, errp);
>
> Indents off.
> It's better to be:
>
> err = netdev_dpdk_check_dup_dev(new_port_id, netdev,
> new_devargs, errp);
>
In v6 this function call is replaced by the function contents.
> > @@ -3253,15 +3342,26 @@ netdev_dpdk_detach(struct unixctl_conn *conn,
> int argc OVS_UNUSED,
> > goto error;
> > }
> >
> > - rte_eth_dev_close(port_id);
> > + /* FIXME: avoid direct access to DPDK internal array rte_eth_devices.
> > */
> > + eth_dev = &rte_eth_devices[port_id];
> > + rte_dev = eth_dev->device;
> > +
> > + if ((eth_dev->data->dev_flags & RTE_ETH_DEV_CLOSE_REMOVE) &&
> > + (netdev_dpdk_get_num_ports(rte_dev) > 1)) {
This line requires a fix. It should be ">=1" instead of ">1".
The two lines above are rewritten in v6.
>
> We still need to close the port here because if you'll have 2 sibling ports
> you'll
> not be able do detach any of them because another one still open.
>
> Anyway, Logic seems wrong here.
>
I will try to explain the logic
> The function intended to detach pci devices. And the usecase is detaching
> devices that was added by the whitelist and not attached in
> 'netdev_dpdk_process_devargs', i.e. has 'dev->attached = false'.
>
Correct
> The workflow should be:
>
> * Iterate over all the siblings of the desired device.
> * if port_id still used by OVS (netdev_dpdk_lookup_by_port_id(port_id)
> !=
> NULL)
> print "Device '%s' is being used by interface '%s'. Remove it
> before
> detaching"
>
For PMDs not supporting representors the code is backward compatible with
previous versions.
For PMDs supporting representors and in cases we have dozens of multi-ports
(representors/interfaces) - is it desirable to print dozens of such messages?
The current patch suggests one message for all cases:
"Device '%s' is being shared with other interfaces. Remove them before
detaching.",
IMHO, I would leave it this way for this patch. If you still think it is useful
to have a message per interface - I suggest adding it in a follow up patch.
What do you think?
> * If some ports are still used by OVS
> return
>
> * If none of the ports still used by OVS
> Iterate over all the siblings and close them.
> (rte_eth_dev_close(port_id))
> Try to remove the device. (rte_dev_remove(rte_dev))
The responsibility for closing all eth devices before detaching the rte device
should be embedded in the rte_dev_remove() call itself.
In other words, when calling rte_dev_remove() - all opened eth devices should
be automatically closed and then the rte device should be unplugged/detached.
It requires a PMD supporting representors to implement the remove() callback as
part of struct rte_pci_driver.
Hence, if none of the sibling ports is used by OVS - it is sufficient to call
rte_dev_remove(rte_dev) and there is no need to iterate over all the siblings
and close them.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
