Hi Ilya, Thanks for code styling fixes. It looks better. Was not able to run git apply so I updated them manually and now sending v7.
Regards, Ophir > -----Original Message----- > From: Ilya Maximets <[email protected]> > Sent: Thursday, January 17, 2019 7:28 PM > To: Ophir Munk <[email protected]>; [email protected] > Cc: Ian Stokes <[email protected]>; Olga Shern <[email protected]>; > Kevin Traynor <[email protected]> > Subject: Re: [PATCH v6] netdev-dpdk: support port representors > > Not a full review. > > There are few style issues. Also same code snippets appears 3 times in a code, > so, it's better to make a function from them. > > I prepared the incremental for those changes. (Compile tested only) Please, > take a look: > > ---- > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 7ce067d0a..53ef55865 > 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -1231,7 +1231,7 @@ netdev_dpdk_get_num_ports(struct rte_device > *device) > > LIST_FOR_EACH (dev, list_node, &dpdk_list) { > if (rte_eth_devices[dev->port_id].device == device > - && rte_eth_devices[dev->port_id].state != RTE_ETH_DEV_UNUSED) { > + && rte_eth_devices[dev->port_id].state != > + RTE_ETH_DEV_UNUSED) { > count++; > } > } > @@ -1676,6 +1676,23 @@ netdev_dpdk_get_port_by_mac(const char > *mac_str) > return DPDK_ETH_PORT_ID_INVALID; > } > > +/* Return the first DPDK port id matching the devargs pattern. */ > +static dpdk_port_t netdev_dpdk_get_port_by_devargs(const char *devargs) > + OVS_REQUIRES(dpdk_mutex) > +{ > + dpdk_port_t port_id; > + struct rte_dev_iterator iterator; > + > + RTE_ETH_FOREACH_MATCHING_DEV (port_id, devargs, &iterator) { > + /* If a break is done - must call rte_eth_iterator_cleanup. */ > + rte_eth_iterator_cleanup(&iterator); > + break; > + } > + > + return port_id; > +} > + > /* > * Normally, a PCI id (optionally followed by a representor number) > * is enough for identifying a specific DPDK port. > @@ -1692,29 +1709,18 @@ netdev_dpdk_process_devargs(struct netdev_dpdk > *dev, > const char *devargs, char **errp) > OVS_REQUIRES(dpdk_mutex) > { > - struct rte_dev_iterator iterator; > dpdk_port_t new_port_id; > > if (strncmp(devargs, "class=eth,mac=", 14) == 0) { > new_port_id = netdev_dpdk_get_port_by_mac(&devargs[14]); > } else { > - /* Return the first DPDK port id matching the devargs pattern. */ > - RTE_ETH_FOREACH_MATCHING_DEV (new_port_id, devargs, &iterator) { > - /* If a break is done - must call rte_eth_iterator_cleanup. */ > - rte_eth_iterator_cleanup(&iterator); > - break; > - } > + new_port_id = netdev_dpdk_get_port_by_devargs(devargs); > if (!rte_eth_dev_is_valid_port(new_port_id)) { > /* Device not found in DPDK, attempt to attach it */ > if (rte_dev_probe(devargs)) { > new_port_id = DPDK_ETH_PORT_ID_INVALID; > } else { > - RTE_ETH_FOREACH_MATCHING_DEV (new_port_id, \ > - devargs, &iterator) { > - /* If a break is done - call rte_eth_iterator_cleanup. */ > - rte_eth_iterator_cleanup(&iterator); > - break; > - } > + new_port_id = netdev_dpdk_get_port_by_devargs(devargs); > if (rte_eth_dev_is_valid_port(new_port_id)) { > /* Attach successful */ > dev->attached = true; @@ -1725,7 +1731,7 @@ > netdev_dpdk_process_devargs(struct netdev_dpdk *dev, > } > } > } > - } > + } > > if (new_port_id == DPDK_ETH_PORT_ID_INVALID) { > VLOG_WARN_BUF(errp, "Error attaching device '%s' to DPDK", devargs); > @@ -3295,18 +3301,13 @@ netdev_dpdk_detach(struct unixctl_conn *conn, > int argc OVS_UNUSED, > dpdk_port_t port_id; > struct netdev_dpdk *dev; > struct rte_device *rte_dev; > - struct rte_dev_iterator iterator; > struct ds used_interfaces = DS_EMPTY_INITIALIZER; > bool used = false; > > ovs_mutex_lock(&dpdk_mutex); > > - RTE_ETH_FOREACH_MATCHING_DEV (port_id, argv[1], &iterator) { > - /* If a break is done - must call rte_eth_iterator_cleanup. */ > - rte_eth_iterator_cleanup(&iterator); > - break; > - } > - if (port_id == DPDK_ETH_PORT_ID_INVALID) { > + port_id = netdev_dpdk_get_port_by_devargs(argv[1]); > + if (!rte_eth_dev_is_valid_port(port_id)) { > response = xasprintf("Device '%s' not found in DPDK", argv[1]); > goto error; > } > @@ -3319,7 +3320,7 @@ netdev_dpdk_detach(struct unixctl_conn *conn, int > argc OVS_UNUSED, > LIST_FOR_EACH (dev, list_node, &dpdk_list) { > /* FIXME: avoid direct access to DPDK array rte_eth_devices. */ > if (rte_eth_devices[dev->port_id].device == rte_dev > - && rte_eth_devices[dev->port_id].state != RTE_ETH_DEV_UNUSED) { > + && rte_eth_devices[dev->port_id].state != > + RTE_ETH_DEV_UNUSED) { > used = true; > ds_put_format(&used_interfaces, " %s", > netdev_get_name(&dev->up)); @@ -3341,7 +3342,7 @@ > netdev_dpdk_detach(struct unixctl_conn *conn, int argc OVS_UNUSED, > } > > response = xasprintf("All devices shared with device '%s' " > - "have been detached", argv[1]); > + "have been detached", argv[1]); > > ovs_mutex_unlock(&dpdk_mutex); > unixctl_command_reply(conn, response); > ---- > > Best regards, Ilya Maximets. > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
