On Mon, Jan 08, 2018 at 01:10:10PM +0000, Stokes, Ian wrote: > Hi Yuanhan, > > Thanks for working on this, I've done some testing with ani40e device and a > review of the current patch, please find comments below. > > > On 12/20/2017 04:03 PM, Yuanhan Liu wrote: > > > Some NICs have only one PCI address associated with multiple ports. > > > This patch extends the dpdk-devargs option's format to cater for such > > devices. > > > > > > To achieve that, this patch uses a new syntax that will be adapted and > > > implemented in future DPDK release (likely, v18.05): > > > http://dpdk.org/ml/archives/dev/2017-December/084234.html > > > > > > And since it's the DPDK duty to parse the (complete and full) syntax > > > and this patch is more likely to serve as an intermediate workaround, > > > here I take a simpler and shorter syntax from it (note it's allowed to > > > have only one category being provided): > > > class=eth,mac=00:11:22:33:44:55:66 > > > > > > Also, old compatibility is kept. Users can still go on with using the > > > PCI id to add a port (if that's enough for them). Meaning, this patch > > > will not break anything. > > I've validated that both the old method/new method both work with i40e > devices as expected.
Thanks a lot for testing! > > Hi Yuanhan, I think there would need to be some doc updates also for a new > > syntax. > > Fully agree, would need updates to the relevant OVS DPDK related docs as well > as an explanation regarding why there are 2 approaches to adding ports. Sure, I will add it in v2. Documentation/howto/dpdk.rst is the one to update, right? > > > > How settled/agreed is the syntax in DPDK now? Ideally it is totally > > settled and we use it for these types of devices. It's not 100% settled, but I think we are really close to it. > > But if not...then considering we will continue to keep compatibility with > > older simpler "pci" anyway, maybe it would be safer to add something > > simple now like "pci","mac" or just "mac" and keep compatibility for that > > when the new syntax is finally agreed in DPDK. It may mean some parsing to > > distinguish pci from mac, or vdev from pci,mac though. > > > > Anyway, agreed syntax in DPDK is better so hopefully that can be done. > > > > > This patch is basically based on the one from Ciara: > > > > > > https://mail.openvswitch.org/pipermail/ovs-dev/2017-October/339496.htm > > > l > > > > > > Cc: Loftus Ciara <[email protected]> > > > Cc: Thomas Monjalon <[email protected]> > > > Cc: Kevin Traynor <[email protected]> > > > Signed-off-by: Yuanhan Liu <[email protected]> > > > --- > > > lib/netdev-dpdk.c | 77 > > > ++++++++++++++++++++++++++++++++++++++++++++----------- > > > 1 file changed, 62 insertions(+), 15 deletions(-) > > > > > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index > > > 45fcc74..4e5cc25 100644 > > > --- a/lib/netdev-dpdk.c > > > +++ b/lib/netdev-dpdk.c > > > @@ -1205,30 +1205,77 @@ netdev_dpdk_lookup_by_port_id(dpdk_port_t > > port_id) > > > return NULL; > > > } > > > > > > +static int > > > +netdev_dpdk_str_to_ether(const char *mac, struct ether_addr *ea) { > > > + unsigned int bytes[6]; > > > + int i; > > > + > > > + if (sscanf(mac, "%x:%x:%x:%x:%x:%x", > > > + &bytes[0], &bytes[1], &bytes[2], > > > + &bytes[3], &bytes[4], &bytes[5]) != 6) { > > > + return -1; > > Should an error be logged here? I thought an error (which will be threw out when port finding is failed) is enough? Just like we didn't do the sanity check to pci id. Anyway, I have no objection to add an error log to make it more explicit. > I flag the same question when checking the return type for this function > later but something to think about. The error log could be here or after the > return type check but I do think it's useful. I will put the error log here (inside the if clause). > > > + } > > > + > > > + for (i = 0; i < 6; i++) { > > > + ea->addr_bytes[i] = bytes[i]; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +static dpdk_port_t > > > +netdev_dpdk_get_port_by_mac(const char *mac_str) { > > > + int i; > > > + struct ether_addr mac; > > > + struct ether_addr port_mac; > > > + > > > + netdev_dpdk_str_to_ether(mac_str, &mac); > > I think there should be an error check for the call to > netdev_dpdk_str_to_ether() above and an associated error log, in the case > where sscanf fails within netdev_dpdk_str_to_ether() (i.e. Mac is too long, > too short etc.) it will help with the logs to zero in on the issue. Right, I missed the sanity check here. > > > + for (i = 0; i < RTE_MAX_ETHPORTS; i++) { > > > + if (!rte_eth_dev_is_valid_port(i)) { > > > + continue; > > > + } > > > + > > > + rte_eth_macaddr_get(i, &port_mac); > > > + if (is_same_ether_addr(&mac, &port_mac)) { > > > + return i; > > > + } > > > + } > > > + > > > + return DPDK_ETH_PORT_ID_INVALID; > > > +} > > > + > > In general for this function I'd like to see a comment added explaining the > behavior now that there are 2 methods to add ports. Sure, will do that in v2. > > > static dpdk_port_t > > > netdev_dpdk_process_devargs(struct netdev_dpdk *dev, > > > const char *devargs, char **errp) { > > > - /* Get the name up to the first comma. */ > > > - char *name = xmemdup0(devargs, strcspn(devargs, ",")); > > > + char *name; > > > dpdk_port_t new_port_id = DPDK_ETH_PORT_ID_INVALID; > > > > > > - 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_eth_dev_attach(devargs, &new_port_id)) { > > > - /* Attach successful */ > > > - dev->attached = true; > > > - VLOG_INFO("Device '%s' attached to DPDK", devargs); > > > - } else { > > > - /* Attach unsuccessful */ > > > - new_port_id = DPDK_ETH_PORT_ID_INVALID; > > > - VLOG_WARN_BUF(errp, "Error attaching device '%s' to DPDK", > > > - devargs); > > > + if (strncmp(devargs, "class=eth,mac=", 14) == 0) { > > > + new_port_id = netdev_dpdk_get_port_by_mac(&devargs[14]); > > > + } else { > > > > It means that hotplug attach will not work for these devices - that needs > > to be documented too. Ian also mentioned concerns about how detach would > > work in one of the previous threads > > https://mail.openvswitch.org/pipermail/ovs-dev/2017-October/339738.html > > +1 on hotplug functionality, needs to be documented when it is available and > how different port attach methods will interact with it. Unfortunately, it could not support hotplug functionality at this stage. I think it will be resolved (naturally) when we have adapt the new syntax in DPDK (which is targeted to v18.05 release). I will also mention this in the doc. Thanks for the review! --yliu _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
