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

Wed Dec 21 19:35:10 UTC 2016

2016-12-21 10:18 GMT-08:00 Kevin Traynor <ktraynor at redhat.com>:
> On 12/21/2016 03:02 AM, Daniele Di Proietto wrote:
>> 2016-12-20 14:08 GMT-08:00 Kevin Traynor <ktraynor at 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?

>
>>
>>>
>>>>
>>>> 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 at intel.com>
>>>> Signed-off-by: Daniele Di Proietto <diproiettod at vmware.com>
>>>> Co-authored-by: Daniele Di Proietto <diproiettod at 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(-)
>