| 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. | expand |
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, \ >
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, \ > >
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, \ >>>
>-----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, \ >>>>
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, \ >>>>>
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, \
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(-)