diff mbox

[ovs-dev,v3,2/3] netdev-dpdk: Arbitrary 'dpdk' port naming

Message ID 23e03654-fd08-6544-7814-371207d4e9eb@redhat.com
State Not Applicable
Headers show

Commit Message

Kevin Traynor Dec. 22, 2016, 10:02 a.m. UTC 
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.

         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(-)
>>
diff mbox

Patch

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]);