On 10.04.2019 1:30, Ophir Munk wrote:
> Hi Ilya,
> Please see comments inline
>
>> -----Original Message-----
>> From: Ilya Maximets <[email protected]>
> ...
>> 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 <[email protected]>
>>> ---
>>
>> 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.
>
>> 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, \
>>>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
