[ovs-dev,v1] netdev-vport: Check DPDK_NETDEV for offloaded API.
diff mbox series

Message ID 1554196661-16307-1-git-send-email-ophirmu@mellanox.com
State Superseded
Headers show
Series
  • [ovs-dev,v1] netdev-vport: Check DPDK_NETDEV for offloaded API.
Related show

Commit Message

Ophir Munk April 2, 2019, 9:18 a.m. UTC
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@mellanox.com>
---
 lib/netdev-vport.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Ilya Maximets April 8, 2019, 7:25 a.m. UTC | #1
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@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, you need to *check* if offloading
supported by *each particular netdev-vport in runtime*, probably based on the
datapath type they assigned to. 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,                        \
>
Ophir Munk April 9, 2019, 10:30 p.m. UTC | #2
Hi Ilya,
Please see comments inline

> -----Original Message-----
> From: Ilya Maximets <i.maximets@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@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?

> 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,                        \
> >
Ilya Maximets April 10, 2019, 2:20 p.m. UTC | #3
On 10.04.2019 1:30, Ophir Munk wrote:
> Hi Ilya,
> Please see comments inline
> 
>> -----Original Message-----
>> From: Ilya Maximets <i.maximets@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@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.

> 
>> 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,                        \
>>>
Roni Bar Yanai April 10, 2019, 7:56 p.m. UTC | #4
>-----Original Message-----
>From: Ilya Maximets <i.maximets@samsung.com>
>Sent: Wednesday, April 10, 2019 5:21 PM
>To: Ophir Munk <ophirmu@mellanox.com>; ovs-dev@openvswitch.org
>Cc: Ian Stokes <ian.stokes@intel.com>; Olga Shern <olgas@mellanox.com>;
>Kevin Traynor <ktraynor@redhat.com>; Asaf Penso <asafp@mellanox.com>;
>Roni Bar Yanai <roniba@mellanox.com>; Paul Blakey <paulb@mellanox.com>;
>Roi Dayan <roid@mellanox.com>; Flavio Leitner <fbl@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@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@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,                        \
>>>>
Ilya Maximets April 11, 2019, 5:46 a.m. UTC | #5
On 10.04.2019 22:56, Roni Bar Yanai wrote:
> 
> 
>> -----Original Message-----
>> From: Ilya Maximets <i.maximets@samsung.com>
>> Sent: Wednesday, April 10, 2019 5:21 PM
>> To: Ophir Munk <ophirmu@mellanox.com>; ovs-dev@openvswitch.org
>> Cc: Ian Stokes <ian.stokes@intel.com>; Olga Shern <olgas@mellanox.com>;
>> Kevin Traynor <ktraynor@redhat.com>; Asaf Penso <asafp@mellanox.com>;
>> Roni Bar Yanai <roniba@mellanox.com>; Paul Blakey <paulb@mellanox.com>;
>> Roi Dayan <roid@mellanox.com>; Flavio Leitner <fbl@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@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@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.

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,                        \
>>>>>

Patch
diff mbox series

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,                        \