On 10.04.2019 22:56, Roni Bar Yanai wrote:
>
>
>> -----Original Message-----
>> From: Ilya Maximets <[email protected]>
>> Sent: Wednesday, April 10, 2019 5:21 PM
>> To: Ophir Munk <[email protected]>; [email protected]
>> Cc: Ian Stokes <[email protected]>; Olga Shern <[email protected]>;
>> Kevin Traynor <[email protected]>; Asaf Penso <[email protected]>;
>> Roni Bar Yanai <[email protected]>; Paul Blakey <[email protected]>;
>> Roi Dayan <[email protected]>; Flavio Leitner <[email protected]>
>> 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 <[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.
>
> 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.
There is only one instance of each 'netdev_class'. So, all the netdevs
has same pointer to 'netdev_class'. That's why I mentioned duplicating.
Please, be sure that offloading will work if both datapaths will have
tunneling ports at the same time.
> 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, \
>>>>>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
