Message ID | 86cca913-fb4d-0c58-66d0-3b7b58e2a75e@redhat.com |
---|---|
State | Not Applicable |
Headers | show |
2016-12-22 3:05 GMT-08:00 Kevin Traynor <ktraynor@redhat.com>: > On 12/22/2016 10:02 AM, Kevin Traynor wrote: >> On 12/21/2016 07:35 PM, Daniele Di Proietto wrote: >>> 2016-12-21 10:18 GMT-08:00 Kevin Traynor <ktraynor@redhat.com>: >>>> On 12/21/2016 03:02 AM, Daniele Di Proietto wrote: >>>>> 2016-12-20 14:08 GMT-08:00 Kevin Traynor <ktraynor@redhat.com>: >>>>>> On 12/15/2016 11:54 AM, Ciara Loftus wrote: >>>>>>> 'dpdk' ports no longer have naming restrictions. Now, instead of >>>>>>> specifying the dpdk port ID as part of the name, the PCI address of the >>>>>>> device must be specified via the 'dpdk-devargs' option. eg. >>>>>>> >>>>>>> ovs-vsctl add-port br0 my-port >>>>>>> ovs-vsctl set Interface my-port type=dpdk >>>>>>> ovs-vsctl set Interface my-port options:dpdk-devargs=0000:06:00.3 >>>>>> >>>>>> I wouldn't encourage people to split up commands like above as they'll >>>>>> see errors and warnings. >>>>> >>>>> Good point >>>>> >>>>>> >>>>>> If you use the old command (which people surely will), it's not >>>>>> intuitive that it's now still a valid cmd but incomplete for setting up >>>>>> the port: >>>>>> >>>>>> []# ovs-vsctl add-port br0 dpdk0 -- set Interface dpdk0 type=dpdk >>>>>> 2016-12-20T19:53:54Z|00051|netdev|WARN|dpdk0: could not set >>>>>> configuration (Invalid argument) >>>>>> ovs-vsctl: Error detected while setting up 'dpdk0'. See ovs-vswitchd >>>>>> log for details. >>>>>> >>>>>> It would be nice if this was just a warning and more informative - this >>>>>> could be an incremental change also. >>>>> >>>>> You're right, vsctl errors are pretty obscure in this case. I think a first >>>>> step is to improve what ovs-vsctl reports to the user. I sent a patch here: >>>>> >>>>> https://mail.openvswitch.org/pipermail/ovs-dev/2016-December/326542.html >>>>> >>>>> The second step would be to allow netdev_dpdk_set_config() to return an error >>>>> string, that can be printed by ovs-vsctl. I'm fine with doing that later. >>>>> What do you guys think? >>>> >>>> sounds like a good plan to me. >>>> >>>> I've done some testing with this patch today and I can't seem to get >>>> hotplug working after applying 2/3. It works with 1/3 but I'm seeing >>>> hangs with the new scheme in 2/3 and a dpdk seg fault with 3/3. I think >>>> maybe my dpdk build is bad or my steps are wrong but it would be good if >>>> someone else could test too. >>>> >>>> - dpdk-devbind.py -u 0000:01:00.0 0000:01:00.1 >>>> - start vswitchd and add bridge >>>> - dpdk-devbind.py -b igb_uio 0000:01:00.0 0000:01:00.1 >>>> - ovs-vsctl add-port br0 dpdk0 -- set Interface dpdk0 type=dpdk >>>> options:dpdk-devargs=0000:01:00.0 >>>> >>> >>> I think there's a bug in DPDK 16.11 that should be fixed by this commit: >>> >>> f9ae888b1e19("ethdev: fix port lookup if none"). >>> >>> Does it work if you include the fix in your DPDK build? >> >> yes - that was the issue I was hitting for the seg fault, thanks. The >> hang is fixed too, I think my igb_uio was out of date. >> >> It is a reasonable that someone might try to mistakenly add a port when >> there are no devices in dpdk but the dpdk fix won't be available until a >> 16.11.1, so we need the equivalent check in OVS. I tested attach/detach >> when no device with/without this incremental and it works now. > > pah...I didn't test when there was a single device bound after vswitchd > starts and of course it breaks, so it should be: > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index 75559fe..11c007d 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -1097,8 +1097,9 @@ netdev_dpdk_process_devargs(const char *devargs) > { > uint8_t new_port_id = UINT8_MAX; > > - if (rte_eth_dev_get_port_by_name(devargs, &new_port_id) > - || !rte_eth_dev_is_valid_port(new_port_id)) { > + if (!rte_eth_dev_count() > + || rte_eth_dev_get_port_by_name(devargs, &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 */ > @@ -2397,7 +2398,7 @@ netdev_dpdk_detach(struct unixctl_conn *conn, int > argc OVS_UNUSED, > > ovs_mutex_lock(&dpdk_mutex); > > - if (rte_eth_dev_get_port_by_name(argv[1], &port_id)) { > + if (!rte_eth_dev_count() || rte_eth_dev_get_port_by_name(argv[1], > &port_id)) { > response = xasprintf("Device '%s' not found in DPDK", argv[1]); > goto error; > } I haven't thought about working around it in ovs. This is, of course, a good idea and I think should be incorporated in the patch. Thanks, Daniele > > > >> >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >> index 75559fe..bedff86 100644 >> --- a/lib/netdev-dpdk.c >> +++ b/lib/netdev-dpdk.c >> @@ -1097,6 +1097,10 @@ netdev_dpdk_process_devargs(const char *devargs) >> { >> uint8_t new_port_id = UINT8_MAX; >> >> + if (!rte_eth_dev_count()) { >> + goto out; >> + } >> + >> if (rte_eth_dev_get_port_by_name(devargs, &new_port_id) >> || !rte_eth_dev_is_valid_port(new_port_id)) { >> /* Device not found in DPDK, attempt to attach it */ >> @@ -1109,6 +1113,7 @@ netdev_dpdk_process_devargs(const char *devargs) >> } >> } >> >> +out: >> return new_port_id; >> } >> >> @@ -2397,7 +2402,7 @@ netdev_dpdk_detach(struct unixctl_conn *conn, int >> argc OVS_UNUSED, >> >> ovs_mutex_lock(&dpdk_mutex); >> >> - if (rte_eth_dev_get_port_by_name(argv[1], &port_id)) { >> + if (!rte_eth_dev_count() || rte_eth_dev_get_port_by_name(argv[1], >> &port_id)) { >> response = xasprintf("Device '%s' not found in DPDK", argv[1]); >> goto error; >> } >> >> >>> >>>> >>>>> >>>>>> >>>>>>> >>>>>>> The user must no longer hotplug attach DPDK ports by issuing the >>>>>>> specific ovs-appctl netdev-dpdk/attach command. The hotplug is now >>>>>>> automatically invoked when a valid PCI address is set in the >>>>>>> dpdk-devargs. The format for ovs-appctl netdev-dpdk/detach command >>>>>>> has changed in that the user now must specify the relevant PCI address >>>>>>> as input instead of the port name. >>>>>>> >>>>>>> Signed-off-by: Ciara Loftus <ciara.loftus@intel.com> >>>>>>> Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com> >>>>>>> Co-authored-by: Daniele Di Proietto <diproiettod@vmware.com> >>>>> >>>>> I think netdev_dpdk_set_config() should compare new_devargs with dev->devargs >>>>> and avoid calling netdev_dpdk_process_devargs() if they're equal and >>>>> if the port_id >>>>> is valid. I've noticed that rte_eth_dev_get_port_by_name() sometimes >>>>> doesn't find >>>>> an existing device and I think comparing the strings will fix the problem. >>>>> >>>>> Also, could you add a log message if rte_eth_dev_attach fails? >>>>> >>>>> I've been playing with this for a while and I guess I'm fine with the >>>>> rest of the series. >>>>> >>>>> Thanks, >>>>> >>>>> Daniele >>>>> >>>>>>> --- >>>>>>> Changelog: >>>>>>> * Keep port-detach appctl function - use PCI as input arg >>>>>>> * Add requires_mutex to devargs processing functions >>>>>>> * use reconfigure infrastructure for devargs changes >>>>>>> * process devargs even if valid portid ie. device already configured >>>>>>> * report err if dpdk-devargs is not specified >>>>>>> * Add Daniele's incremental patch & Sign-off + Co-author tags >>>>>>> * Update details of detach command to reflect PCI is needed instead of >>>>>>> port name >>>>>>> * Update NEWS to mention that the new naming scheme is not backwards >>>>>>> compatible >>>>>>> * Use existing DPDk functions to get port ID from PCI address/devname >>>>>>> * Merged process_devargs and process_pdevargs functions >>>>>>> * Removed unnecessary requires_mutex from devargs processing fn >>>>>>> * Fix case where port is re-attached after detach >>>>>>> * Add note to documentation that devices won't work until devargs set. >>>>>>> >>>>>>> Documentation/intro/install/dpdk-advanced.rst | 5 +- >>>>>>> Documentation/intro/install/dpdk.rst | 15 ++- >>>>>>> NEWS | 5 + >>>>>>> lib/netdev-dpdk.c | 169 +++++++++++++++++--------- >>>>>>> vswitchd/vswitch.xml | 8 ++ >>>>>>> 5 files changed, 138 insertions(+), 64 deletions(-) >>>> >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 75559fe..11c007d 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -1097,8 +1097,9 @@ netdev_dpdk_process_devargs(const char *devargs) { uint8_t new_port_id = UINT8_MAX; - if (rte_eth_dev_get_port_by_name(devargs, &new_port_id) - || !rte_eth_dev_is_valid_port(new_port_id)) { + if (!rte_eth_dev_count() + || rte_eth_dev_get_port_by_name(devargs, &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 */ @@ -2397,7 +2398,7 @@ netdev_dpdk_detach(struct unixctl_conn *conn, int argc OVS_UNUSED, ovs_mutex_lock(&dpdk_mutex); - if (rte_eth_dev_get_port_by_name(argv[1], &port_id)) { + if (!rte_eth_dev_count() || rte_eth_dev_get_port_by_name(argv[1], &port_id)) { response = xasprintf("Device '%s' not found in DPDK", argv[1]);