[ovs-dev] [PATCH v1] netdev-vport: Check DPDK_NETDEV for offloaded API.

[Roni Bar Yanai]

>-----Original Message-----
>From: Ilya Maximets <i.maximets at samsung.com>
>Sent: Wednesday, April 10, 2019 5:21 PM
>To: Ophir Munk <ophirmu at mellanox.com>; ovs-dev at openvswitch.org
>Cc: Ian Stokes <ian.stokes at intel.com>; Olga Shern <olgas at mellanox.com>;
>Kevin Traynor <ktraynor at redhat.com>; Asaf Penso <asafp at mellanox.com>;
>Roni Bar Yanai <roniba at mellanox.com>; Paul Blakey <paulb at mellanox.com>;
>Roi Dayan <roid at mellanox.com>; Flavio Leitner <fbl at sysclose.org>
>Subject: Re: [PATCH v1] netdev-vport: Check DPDK_NETDEV for offloaded
>API.
>
>On 10.04.2019 1:30, Ophir Munk wrote:
>> Hi Ilya,
>> Please see comments inline
>>
>>> -----Original Message-----
>>> From: Ilya Maximets <i.maximets at samsung.com>
>> ...
>>> On 02.04.2019 12:18, Ophir Munk wrote:
>>>> vport offloaded functions should have a different implementation for
>>>> linux based OVS versus dpdk based OVS. Currently there is only support
>>>> for linux based offloaded API. The code in the file named
>>>> netdev-vport.c checks 'ifdef __linux__' to select the linux based
>>>> offloaded functions, without checking 'ifndef DPDK_NETDEV' as well.
>>>> '__linux__' is a pre-defined compiler macro, indicating that a source
>>>> code is compiled on a linux based system. Any code inside a __linux__
>>>> definition will be excluded on a windows based system, for example.
>>>> 'DPDK_NETDEV' is a macro defined by autoconf tools when configuring
>>>> OVS to be dpdk based as shown in [1].
>>>> Before this commit and in case hw-offload=true - using a vport
>>>> interface with a dpdk based OVS daemon running on a linux machine
>>>> resulted in an error since the vport linux based offloaded APIs were
>>>> called instead of returning EOPNOTSUPP. Luckily the linux offloaded
>>>> API returned immediately on a get_ifindex() failure, which caused no
>>>> harm. An example of the failure message is shown in [2].
>>>>
>>>> [1]
>>>> configure --with-dpdk=<dpdk root tree>/<target architecture>
>>>>
>>>> [2]
>>>> ovs|00002|netdev_tc_offloads(dp_netdev_flow_5)|ERR|flow_put:
>failed
>>> to
>>>> get ifindex for vxlan_sys_4789: No such device
>>>>
>>>> Fixes: 01b257860c89 ("netdev-vport: Use common offloads interface")
>>>> Signed-off-by: Ophir Munk <ophirmu at mellanox.com>
>>>> ---
>>>
>>> Hi.
>>>
>>> We already discussed this with Roni some time ago. You can't just disable
>>> vport offloading on compile time. Compiling with DPDK support doesn't
>>> mean that it will be used only with userspace datapath. DPDK support is
>just
>>> an additional feature. You could still use kernel datapath and even use
>kernel
>>> and userpace datapaths at the same time under control of the same ovs-
>>> vswitchd process. If those error messages are annoying,
>>
>> The main issue I am concerned with is that unexpected/unplanned kernel
>targeted code is executed
>> as part of dpdk run. It may now end with annoying messages, but I think
>kernel code should be avoided
>> in the first place in case of dpdk datapath. I will send a new patch to address
>it.
>>
>>> you need to
>>> *check* if offloading supported by *each particular netdev-vport in
>>> runtime*, probably based on the datapath type they assigned to.
>>
>> Thank you for this advice. Do you have pointers into the code where such
>checks occur?
>
>The simplest way is to duplicate vport types stripping the offloading
>and update 'dpif_netdev_port_open_type' to return names of duplicated
>ones.
>It probably be good to add '-user' suffix like 'geneve-user' or 'erspan-user'.
>
>I thought that Roni already has some solution for his tunneling offload
>patches.
>You may ask how he done that.

Thanks Ilya, I did a patch but haven't submit it yet.
I did it by changing the pointers according to the bridge type the port was added to, I didn't duplicate.
Will discuss with Ophir.
Thanks

>
>>
>>> In case of
>>> simultaneous running of different datapaths some vports will have backing
>>> linux devices and some will not.
>>>
>>> Compiling out offloading based on enabling of DPDK support will just break
>>> offloading for kernel datapath. At least, this will cause issues for distros
>that
>>> will have to decide if they want DPDK support or vport offloading in kernel.
>>>
>>> Best regards, Ilya Maximets.
>>>
>>>>  lib/netdev-vport.c | 8 ++++----
>>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c index
>>>> 808a43f..5ba7455 100644
>>>> --- a/lib/netdev-vport.c
>>>> +++ b/lib/netdev-vport.c
>>>> @@ -47,8 +47,8 @@
>>>>  #include "unaligned.h"
>>>>  #include "unixctl.h"
>>>>  #include "openvswitch/vlog.h"
>>>> +#if defined(__linux__) && !defined(DPDK_NETDEV)
>>>>  #include "netdev-tc-offloads.h"
>>>> -#ifdef __linux__
>>>>  #include "netdev-linux.h"
>>>>  #endif
>>>>
>>>> @@ -1093,7 +1093,7 @@ netdev_vport_get_pt_mode(const struct
>netdev
>>>> *netdev)
>>>>
>>>>
>>>>
>>
>>>> -#ifdef __linux__
>>>> +#if defined(__linux__) && !defined(DPDK_NETDEV)
>>>>  static int
>>>>  netdev_vport_get_ifindex(const struct netdev *netdev_)  { @@ -
>1105,10
>>>> +1105,10 @@ netdev_vport_get_ifindex(const struct netdev *netdev_)
>>>>
>>>>  #define NETDEV_VPORT_GET_IFINDEX netdev_vport_get_ifindex
>>> #define
>>>> NETDEV_FLOW_OFFLOAD_API , LINUX_FLOW_OFFLOAD_API -#else /*
>>> !__linux__
>>>> */
>>>> +#else
>>>>  #define NETDEV_VPORT_GET_IFINDEX NULL  #define
>>>> NETDEV_FLOW_OFFLOAD_API -#endif /* __linux__ */
>>>> +#endif
>>>>
>>>>  #define VPORT_FUNCTIONS_COMMON                      \
>>>>      .run = netdev_vport_run,                        \
>>>>