[ovs-dev] [PATCH v4 1/4] netdev: Dynamic per-port Flow API.

Roni Bar Yanai roniba at mellanox.com
Mon Jun 10 07:02:02 UTC 2019 

Hi Ilya,

See inline

>-----Original Message-----
>From: Ilya Maximets <i.maximets at samsung.com>
>Sent: Thursday, June 6, 2019 2:19 PM
>To: Roni Bar Yanai <roniba at mellanox.com>; Ophir Munk
><ophirmu at mellanox.com>; Roi Dayan <roid 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>; 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>; Majd Dibbiny
><majd at mellanox.com>; Oz Shlomo <ozsh at mellanox.com>
>Subject: Re: [PATCH v4 1/4] netdev: Dynamic per-port Flow API.
>
>On 06.06.2019 13:38, Roni Bar Yanai wrote:
>> Hi Ilya,
>> was curious myself. Mark & RSS is working  (didn't test with representors yet).
>
>Hi. Thanks for testing!
>
>> I've tested offload is supported on vport using !has_system_port, do you think
>failing in this test is enough to say that this is netdev bridge port?
>
>I think it's OK for now. '!has_system_port' allows to say that it's not a
>system vport and, since 'dummy' flow API doesn't support vports offloading,
>we could be sure that only dpdk flow API could be used (if allowed).
>
>> When I returned supported, "dpdk_put" was called as expected (after removing
>the disallow).
>>
>> One thing I've encounters is while addin a tap to the dpdk bridge. I got this:
>>
>> 2019-06-06T10:18:21.865Z|00055|netdev_offload_tc|INFO|probe tc: block
>offload is supported.
>> 2019-06-06T10:18:21.866Z|00056|netdev_offload_tc|INFO|added ingress qdisc
>to veth0
>> 2019-06-06T10:18:21.866Z|00057|netdev_offload|INFO|veth0: Assigned flow
>API 'linux_tc'.
>> 2019-06-06T10:18:21.866Z|00058|bridge|INFO|bridge br-int: added interface
>veth0 on port 1
>> 2019-06-06T10:18:21.867Z|00059|bridge|INFO|bridge br-phy: added interface
>br-phy on port 65534
>>
>> Seems that we should block this as well.
>
>I don't think that we should block this, because we should allow
>'linux_tc' offloading for linux interfaces. For example, someone
>could open two linux ports from the userspace datapath and expect
>that flows could be offloaded to HW/tc layer. I agree that it will
>not fully work in case of mixing port types within a single OVS
>bridge, however this should be a valid case. Packets from linux to
>DPDK ports and backwards will go through OVS, because such flows
>could not be offloaded.
>
>Curious fact is that working this way (opening physical ports via
>netdev-linux) could be even faster than using DPDK ports in some
>cases, because 'linux_tc' supports full HW offloading unlike DPDK.

Can you explain what exactly are the use cases?
DPDK supports full offload when using port representors, it is just a matter of integrating it into OVS.

>
>We'll also have AF_XDP support some time soon which will give even
>more benefits to linux interfaces (XDP bypasses tc layer, however
>it might be possible to install flows to HW and handle unmatched
>packets in userspace).
>
>Best regards, Ilya Maximets.
>
>>
>> BR,
>> Roni
>>
>>> -----Original Message-----
>>> From: Ilya Maximets <i.maximets at samsung.com>
>>> Sent: Monday, June 3, 2019 6:30 PM
>>> To: Ophir Munk <ophirmu at mellanox.com>; Roi Dayan <roid 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>; Majd Dibbiny <majd at mellanox.com>; Oz
>Shlomo
>>> <ozsh at mellanox.com>
>>> Subject: Re: [PATCH v4 1/4] netdev: Dynamic per-port Flow API.
>>>
>>> Hi Ophir.
>>> I'm curious, what is the current status of your testing?
>>>
>>> I'd like to apply this series to not block further development.
>>>
>>> Best regards, Ilya Maximets.
>>>
>>> On 23.05.2019 16:54, Ilya Maximets wrote:
>>>> On 23.05.2019 16:53, Ilya Maximets wrote:
>>>>> On 23.05.2019 16:18, Ophir Munk wrote:
>>>>>>
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Ilya Maximets <i.maximets at samsung.com>
>>>>>>> Sent: Wednesday, May 22, 2019 2:50 PM
>>>>>>> To: Ophir Munk <ophirmu at mellanox.com>; Roi Dayan
>>>>>>> <roid 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>; Majd
>>>>>>> Dibbiny <majd at mellanox.com>
>>>>>>> Subject: Re: [PATCH v4 1/4] netdev: Dynamic per-port Flow API.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 22.05.2019 13:15, Ilya Maximets wrote:
>>>>>>>> On 22.05.2019 1:12, Ophir Munk wrote:
>>>>>>>>>
>>>>>>>>>> -----Original Message-----
>>>>>>>>>> From: Roi Dayan
>>>>>>>>>> Sent: Tuesday, May 21, 2019 7:48 PM
>>>>>>>>>> To: Ilya Maximets <i.maximets at samsung.com>; ovs-
>>>>>>> dev at openvswitch.org
>>>>>>>>>> Cc: Ian Stokes <ian.stokes at intel.com>; Flavio Leitner
>>>>>>>>>> <fbl at sysclose.org>; Ophir Munk <ophirmu at mellanox.com>; 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>
>>>>>>>>>> Subject: Re: [PATCH v4 1/4] netdev: Dynamic per-port Flow API.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Acked-by: Roi Dayan <roid at mellanox.com>
>>>>>>>>>
>>>>>>>>> Hi Ilya,
>>>>>>>>> Can you please send a patch for the detection of netdev vport on top
>of
>>>>>>> this series (as you have already started suggesting in ML discussions)?
>>>>>>>>> I will then test it and will make sure it's applicable with this series. I
>think it
>>>>>>> is better to do that before series acceptance.
>>>>>>>>> What do you think?
>>>>>>>>
>>>>>>>> Hi.
>>>>>>>> Actually patches are already on a list. You only need to add few lines
>>>>>>>> to make them allow vxlan for netdev-offload-dpdk.
>>>>>>>>
>>>>>>>> Apply following patch sets on top of this one:
>>>>>>>>
>>>>>>>>
>>>
>https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork
>>>
>.ozlabs.org%2Fproject%2Fopenvswitch%2Flist%2F%3Fseries%3D107534&amp;dat
>>>
>a=02%7C01%7Croniba%40mellanox.com%7C90cd4c9ba95b4629884f08d6e8384c41
>>>
>%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636951725824527442&am
>>>
>p;sdata=n%2FVMz34ICfk0dvGOP77Ip4L008RRKdraRXMG%2FtFRM64%3D&amp;re
>>> served=0
>>>>>>
>>>>>> FYI - applying this patch succeeded, however applying the next patch failed
>>> unless I applied
>>>>>> patches 2/4, 3/4/ 4/4 first and only then I applied the next patch.
>>>>>
>>>>> Yes. That is expected.
>>>>>
>>>>>>
>>>>>>>>
>>>
>https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork
>>>
>.ozlabs.org%2Fproject%2Fopenvswitch%2Flist%2F%3Fseries%3D107545&amp;dat
>>>
>a=02%7C01%7Croniba%40mellanox.com%7C90cd4c9ba95b4629884f08d6e8384c41
>>>
>%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636951725824527442&am
>>>
>p;sdata=pfTmeLEuMW1DyJck3T%2BypfDI7ZlZ7K2%2FO6810tis4PQ%3D&amp;reser
>>> ved=0
>>>>>>>>
>>>>>>>>
>>>>>>>> Change below should than allow you to use dpdk offloading for vxlan
>ports:
>>>>>>
>>>>>> Why do you want to use dpdk offloading for vxlan ports?
>>>>>
>>>>> Sorry for misunderstanding, but I thought that you're implementing
>>>>> vxlan offloading as part of dpdk offloading. If it'll be a separate
>>>>> module, it's even better.
>>>>>
>>>>>> We need to use vport-netdev offloading for vxlan-netdev ports.
>>>>>> We need to use dpdk offloading for dpdk ports.
>>>>>> Vxlan-netdev offloading and dpdk offloading have a different
>implementation
>>>>>> (unlike the system case where vxlan-system offloading and system
>offloading
>>> are identical).
>>>>>>
>>>>>> I see four required offloading APIs:
>>>>>> 1. system
>>>>>> 2. dpdk
>>>>>> 3. vport under system (currently it is identical to system API)
>>>>>> 4. New vport under netdev.
>>>>>>
>>>>>> The first three APIs exist. The last (vxlan-netdev) will be sent soon.
>>>>>>
>>>>>> I see two options for adding vxlan-netdev API.
>>>>>> 1. Create a new dedicated vport-netdev offload class.
>>>>>>
>>>>>> 2. Having vport-netdev API to be identical to dpdk API but since the
>>> implementations are different we will have to know the type ("dpdk" versus
>>> "vxlan").
>>>>>> In pseudo code:
>>>>>> If (type=="dpdk")
>>>>>> {
>>>>>>    // handle dpdk offloading
>>>>>> }
>>>>>> If (type=="vxlan")
>>>>>> {
>>>>>>    // handle vxlan offloading
>>>>>> }
>>>>>>
>>>>>> I prefer the first option.
>>>>>
>>>>> Yes. Sure. I alse prefer the separate class if it has separate implementation
>>>>> anyway.
>>>>>
>>>>>
>>>>> So, with all above patches applied you just need to make a new file:
>>>>> netdev-offload-vport-dpdk.c:
>>>>>
>>>>> <...>
>>>>> static int netdev_offload_vport_dpdk_flow_put(...)
>>>>> {
>>>>>     ...
>>>>> }
>>>>>
>>>>> static int netdev_offload_vport_dpdk_flow_del(...)
>>>>> {
>>>>>     ...
>>>>> }
>>>>>
>>>>> static int
>>>>> netdev_offload_vport_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 strcmp(netdev_get_type(netdev), "vxlan");
>>>>> }
>>>>>
>>>>> const struct netdev_flow_api netdev_offload_vport_dpdk = {
>>>>>     .type = "dpdk_flow_api",
>>>>
>>>> s/type = "dpdk_flow_api"/type = "vport_dpdk_flow_api"/
>>>>
>>>>>     .flow_put = netdev_offload_vport_dpdk_flow_put,
>>>>>     .flow_del = netdev_offload_vport_dpdk_flow_del,
>>>>>     .init_flow_api = netdev_offload_vport_dpdk_init_flow_api,
>>>>> };
>>>>>
>>>>>
>>>>> And add following line to lib/dpdk.c:
>>>>>
>>>>> netdev_register_flow_api_provider(&netdev_offload_vport_dpdk);
>>>>>
>>>>>
>>>>> And following to lib/netdev-offload-provider.h:
>>>>>
>>>>> extern const struct netdev_flow_api netdev_offload_vport_dpdk;
>>>>>
>>>>>>
>>>>>>>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
>>>>>>>> index b7b0616ec..32f23c401 100644
>>>>>>>> --- a/lib/netdev-offload-dpdk.c
>>>>>>>> +++ b/lib/netdev-offload-dpdk.c
>>>>>>>> @@ -760,6 +760,10 @@ netdev_offload_dpdk_init_flow_api(struct
>netdev
>>>>>>> *netdev)
>>>>>>>>          return EOPNOTSUPP;
>>>>>>>>      }
>>>>>>>>
>>>>>>>> +    if (!strcmp(netdev_get_name(netdev), "vxlan")) {
>>>>>>>
>>>>>>> Sorry,
>>>>>>> s/netdev_get_name/netdev_get_type/
>>>>>>>
>>>>>>>> +        return 0;
>>>>>>
>>>>>> Having said all the above - we still need a way to correctly select between
>>> vport-netdev API versus vport-system API.
>>>>>> Reading your suggestion I am still not sure we have a solution here. Say we
>>> have a system bridge and a netdev bridge both with a vxlan port.
>>>>>> When the vxlan-netdev is checked first by the system-init API it will pass
>the
>>> checking and it will be added as a vxlan-system. Right?
>>>>>
>>>>> No. See the 'netdev_vport_has_system_port' function from patch
>>>>> "[RFC 2/2] netdev-offload: Disallow offloading to unrelated vports.".
>>>>>
>>>>>> Can you please advise?
>>>>>
>>>>> See the code snippet above.
>>>>>
>>>>> Best regards, Ilya Maximets.

More information about the dev mailing list