[ovs-dev] [PATCH v3 2/3] netdev-dpdk: Arbitrary 'dpdk' port naming
Daniele Di Proietto
diproiettod at ovn.org
Fri Dec 23 02:38:43 UTC 2016
2016-12-22 3:05 GMT-08:00 Kevin Traynor <ktraynor at 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 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?
>>
>> 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 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(-)
>>>>
>>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>
More information about the dev
mailing list