[ovs-dev] [PATCH] netdev: Dynamic per-port Flow API.

Ilya Maximets i.maximets at ovn.org
Fri Nov 8 15:57:15 UTC 2019


On 06.11.2019 18:11, Ophir Munk wrote:
> Hi Ilya,
> A few months ago we discussed the missing functionality of vports offloading under user space bridges.
> Commit [1] was added to explicitly avoid installing userspace vport flows (to avoid confusion with the vport kernel).
> When reverting commit [1] - we are left with this missing functionality.
> Applying your suggested patch netdev_vport_has_system_port() API)  does not seem to work.
> It always fails when it tries to look for the interface name (say "vxlan10") in the system list where vxlan interfaces are renamed (say "vxlan_sys_4789").

Hi Ophir,

No-one ever tried to run this code, so I'm not surprised.
I could take a look at this issue on next week.

Best regards, Ilya Maximets.

> 
> You wrote:
>> On master, after applying dynamic flow API patch-set, we'll be able to use
>> patch that I suggested in previous mail to properly handle this situation and
>> enable vport offloading.
> 
> It would be appreciated if we can resume the efforts in fixing this missing functionality.
> Please advise on the next steps.
> 
> [1]
> Commit 0da667e345 ("dpif-netdev: Forbid vport offloading attempts.")
> 
>>
>> -----Original Message-----
>> From: Ilya Maximets <i.maximets at samsung.com>
>> Sent: Monday, May 13, 2019 3:33 PM
>> To: Ophir Munk <ophirmu at mellanox.com>; ovs-dev at openvswitch.org
>> Cc: Ian Stokes <ian.stokes at intel.com>; Flavio Leitner <fbl at sysclose.org>;
>> Kevin Traynor <ktraynor at redhat.com>; Roni Bar Yanai
>> <roniba at mellanox.com>; Finn Christensen <fc at napatech.com>; Ben Pfaff
>> <blp at ovn.org>; Simon Horman <simon.horman at netronome.com>; Olga
>> Shern <olgas at mellanox.com>; Asaf Penso <asafp at mellanox.com>; Oz
>> Shlomo <ozsh at mellanox.com>; Majd Dibbiny <majd at mellanox.com>
>> Subject: Re: [PATCH] netdev: Dynamic per-port Flow API.
>>
>> On 13.05.2019 14:41, Ophir Munk wrote:
>>> Hi Ilya,
>>>
>>>> -----Original Message-----
>>>> From: Ilya Maximets <i.maximets at samsung.com>
>>>> Sent: Monday, May 13, 2019 12:22 PM
>>>> To: Ophir Munk <ophirmu at mellanox.com>; ovs-dev at openvswitch.org
>>>> Cc: Ian Stokes <ian.stokes at intel.com>; Flavio Leitner
>>>> <fbl at sysclose.org>; Kevin Traynor <ktraynor at redhat.com>; Roni Bar
>>>> Yanai <roniba at mellanox.com>; Finn Christensen <fc at napatech.com>;
>> Ben
>>>> Pfaff <blp at ovn.org>; Simon Horman <simon.horman at netronome.com>;
>> Olga
>>>> Shern <olgas at mellanox.com>; Asaf Penso <asafp at mellanox.com>; Oz
>>>> Shlomo <ozsh at mellanox.com>
>>>> Subject: Re: [PATCH] netdev: Dynamic per-port Flow API.
>>>>
>>>> On 09.05.2019 1:59, Ophir Munk wrote:
>>>>> Hi Ilya,
>>>>>
>>> ...
>>>>> I am recreating this scenario in my setup.
>>>>
>>>> I see. Yes, you're right. And I think that this case could be
>>>> reproduced on current master without any patches. So, it's a bug that we
>> need to fix.
>>>> Otherwise userspace datapath will try to offload its flows to the
>>>> unrelated system interfaces. For now we could just forbid offloading
>>>> to vports in dpif- netdev. I'll prepare the patch. This fix also should be
>> backported.
>>>>
>>>
>>> We need a way to enable offloading of vports in dpif-netdev. So if you
>>> can address It with your new patch - that would be appreciated.
>>
>> I'm suggesting disabling because it'll be easy to backport to older branches.
>> On master, after applying dynamic flow API patch-set, we'll be able to use
>> patch that I suggested in previous mail to properly handle this situation and
>> enable vport offloading.
>>
> 
>>>
>>>>>
>>>>> This patch series is important but one of its main goals is to
>>>>> enable different
>>>> offloads for different vports under Kernel/userspace.
>>>>> Can you please advise how this goal can be achieved?
>>>>
>>>> It looks like there is no way to distinguish system and userspace
>>>> vports by looking only on netdev. We should check with dpif type.
>>>
>>> Agreed. But then looking at your sample patch below you are basing
>>> your decision on the existence of system port (and not on dpif type).
>>
>> Right now 'ifindex' is used for checking, so this is equally racy.
>>
>>>   I think it is risky because
>>> you are dependent on the order of operations: Creation of the system port
>> versus checking for system port existence.
>>> Which was first (under different scenarios)?
>>
>> Actually, port creation and checking will always happen in the same thread
>> context, so creation and checking will be serialized. Kernel should guarantee
>> the port existence. No races here.
>>
>>>
>>> What do you say about the following patch:
>>>
>>> diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c int
>>> netdev_ports_insert(struct netdev *netdev, const struct dpif_class
>>> *dpif_class,
>>>                       struct dpif_port *dpif_port) @@ -547,8 +555,9 @@
>>> netdev_ports_insert(struct netdev *netdev, const struct dpif_class
>>> *dpif_class,
>>>                   netdev_ports_hash(dpif_port->port_no, dpif_class));
>>>       hmap_insert(&ifindex_to_port, &data->ifindex_node, ifindex);
>>>       ovs_mutex_unlock(&netdev_hmap_mutex);
>>>
>>> -    netdev_init_flow_api(netdev);
>>> +    netdev_init_flow_api(netdev, dpif_class->type); /* Pass class
>>> + type "netdev" or "syste" */
>>>
>>>       return 0;
>>> }
>>>
>>> It's not a full solution. It is just a hint how to pass the dpif type ("netdev" or
>> "system")  when initializing the flow api.
>>> We can use the dpif type when initializing vport offload.
>>
>> This is, actually, the first thing I tried. However, this requires exposing the
>> internals of 'dpif-provider', which is bad from the point of code architecture.
>>
>> For the below patch code, I missed actual port requesting from the datapath.
>> Following incremental needed:
>>
>> ---
>> diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c index
>> 82943c102..1c88a91ed 100644
>> --- a/lib/netdev-vport.c
>> +++ b/lib/netdev-vport.c
>> @@ -124,13 +124,28 @@ netdev_vport_class_get_dpif_port(const struct
>> netdev_class *class)  bool  netdev_vport_has_system_port(const struct
>> netdev *netdev)  {
>> -    bool found;
>> +    bool found = false;
>> +    const char *name;
>> +    const char *type = "system";
>>       struct sset names = SSET_INITIALIZER(&names);
>>
>>       ovs_assert(is_vport_class(netdev_get_class(netdev)));
>>
>> -    dp_enumerate_names("system", &names);
>> -    found = sset_contains(&names, netdev_get_name(netdev));
>> +    dp_enumerate_names(type, &names);
>> +    SSET_FOR_EACH (name, &names) {
>> +        struct dpif *dpifp = NULL;
>> +
>> +        if (dpif_open(name, type, &dpifp)) {
>> +            continue;
>> +        }
>> +
>> +        found = dpif_port_exists(dpifp, netdev_get_name(netdev));
>> +
>> +        dpif_close(dpifp);
>> +        if (found) {
>> +            break;
>> +        }
>> +    }
>>       sset_destroy(&names);
>>
>>       return found;
>> ---
>>
>>>
>>>> Something like this (not tested):
>>>>
>>>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
>>>> index 01e900461..b7b0616ec 100644
>>>> --- a/lib/netdev-offload-dpdk.c
>>>> +++ b/lib/netdev-offload-dpdk.c
>>>> @@ -22,6 +22,7 @@
>>>>   #include "dpif-netdev.h"
>>>>   #include "netdev-offload-provider.h"
>>>>   #include "netdev-provider.h"
>>>> +#include "netdev-vport.h"
>>>>   #include "openvswitch/match.h"
>>>>   #include "openvswitch/vlog.h"
>>>>   #include "packets.h"
>>>> @@ -752,6 +753,13 @@ netdev_offload_dpdk_flow_del(struct netdev
>>>> *netdev, const ovs_u128 *ufid,  static int
>>>> netdev_offload_dpdk_init_flow_api(struct netdev *netdev)  {
>>>> +    if (netdev_vport_is_vport_class(netdev->netdev_class)
>>>> +        && netdev_vport_has_system_port(netdev)) {
>>>> +        VLOG_DBG("%s: vport has backing system interface. Skipping.",
>>>> +                 netdev_get_name(netdev));
>>>> +        return EOPNOTSUPP;
>>>> +    }
>>>> +
>>>>       return netdev_dpdk_flow_api_supported(netdev) ? 0 : EOPNOTSUPP;
>>>> }
>>>>
>>>> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c index
>>>> 498aae369..e4d121ed7 100644
>>>> --- a/lib/netdev-offload-tc.c
>>>> +++ b/lib/netdev-offload-tc.c
>>>> @@ -31,6 +31,7 @@
>>>>   #include "netdev-linux.h"
>>>>   #include "netdev-offload-provider.h"
>>>>   #include "netdev-provider.h"
>>>> +#include "netdev-vport.h"
>>>>   #include "netlink.h"
>>>>   #include "netlink-socket.h"
>>>>   #include "odp-netlink.h"
>>>> @@ -1560,6 +1561,13 @@ netdev_tc_init_flow_api(struct netdev
>> *netdev)
>>>>       int ifindex;
>>>>       int error;
>>>>
>>>> +    if (netdev_vport_is_vport_class(netdev->netdev_class)
>>>> +        && !netdev_vport_has_system_port(netdev)) {
>>>> +        VLOG_DBG("%s: vport has no backing system interface. Skipping.",
>>>> +                 netdev_get_name(netdev));
>>>> +        return EOPNOTSUPP;
>>>> +    }
>>>> +
>>>>       ifindex = netdev_get_ifindex(netdev);
>>>>       if (ifindex < 0) {
>>>>           VLOG_INFO("init: failed to get ifindex for %s: %s", diff
>>>> --git a/lib/netdev- vport.c b/lib/netdev-vport.c index
>>>> 92a256af1..82943c102 100644
>>>> --- a/lib/netdev-vport.c
>>>> +++ b/lib/netdev-vport.c
>>>> @@ -44,6 +44,7 @@
>>>>   #include "simap.h"
>>>>   #include "smap.h"
>>>>   #include "socket-util.h"
>>>> +#include "sset.h"
>>>>   #include "unaligned.h"
>>>>   #include "unixctl.h"
>>>>   #include "openvswitch/vlog.h"
>>>> @@ -120,6 +121,21 @@ netdev_vport_class_get_dpif_port(const struct
>>>> netdev_class *class)
>>>>       return is_vport_class(class) ?
>>>> vport_class_cast(class)->dpif_port : NULL;  }
>>>>
>>>> +bool
>>>> +netdev_vport_has_system_port(const struct netdev *netdev) {
>>>> +    bool found;
>>>> +    struct sset names = SSET_INITIALIZER(&names);
>>>> +
>>>> +    ovs_assert(is_vport_class(netdev_get_class(netdev)));
>>>> +
>>>> +    dp_enumerate_names("system", &names);
>>>> +    found = sset_contains(&names, netdev_get_name(netdev));
>>>> +    sset_destroy(&names);
>>>> +
>>>> +    return found;
>>>> +}
>>>> +
>>>>   const char *
>>>>   netdev_vport_get_dpif_port(const struct netdev *netdev,
>>>>                              char namebuf[], size_t bufsize) diff
>>>> --git a/lib/netdev-vport.h b/lib/netdev-vport.h index
>>>> 9d756a265..4be1b92ec 100644
>>>> --- a/lib/netdev-vport.h
>>>> +++ b/lib/netdev-vport.h
>>>> @@ -40,6 +40,7 @@ void netdev_vport_inc_tx(const struct netdev *,
>>>>                            const struct dpif_flow_stats *);
>>>>
>>>>   bool netdev_vport_is_vport_class(const struct netdev_class *);
>>>> +bool netdev_vport_has_system_port(const struct netdev *);
>>>>   const char *netdev_vport_class_get_dpif_port(const struct
>>>> netdev_class *);
>>>>
>>>>   #ifndef _WIN32
>>>> ---
>>>>
>>>> I'll format this as a proper patch and send along with patch for
>>>> enabling offloading for ports without correct ifindex.
>>>>
>>>> Best regards, Ilya Maximets.
>>>
>>> Best regards,
>>> Ophir
>>>


More information about the dev mailing list