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 <ktray...@redhat.com>:
>>> On 12/21/2016 03:02 AM, Daniele Di Proietto wrote:
>>>> 2016-12-20 14:08 GMT-08:00 Kevin Traynor <ktray...@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;
}
>
> 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.lof...@intel.com>
>>>>>> Signed-off-by: Daniele Di Proietto <diproiet...@vmware.com>
>>>>>> Co-authored-by: Daniele Di Proietto <diproiet...@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
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
