| Message ID | 20210623155252.10975-14-elibr@nvidia.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Netdev vxlan-decap offload | expand |
Bleep bloop. Greetings Eli Britstein, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Ilya Maximets <i.maximets@ovn.org> Lines checked: 217, Warnings: 1, Errors: 0 Please check this out. If you feel there has been an error, please email aconole@redhat.com Thanks, 0-day Robot
> -----Original Message----- > From: dev <ovs-dev-bounces@openvswitch.org> On Behalf Of Eli Britstein > Sent: Wednesday, June 23, 2021 4:53 PM > To: dev@openvswitch.org; Ilya Maximets <i.maximets@ovn.org> > Cc: Eli Britstein <elibr@nvidia.com>; Ivan Malov <Ivan.Malov@oktetlabs.ru>; Majd > Dibbiny <majd@nvidia.com> > Subject: [ovs-dev] [PATCH V7 13/13] netdev-dpdk-offload: Add vxlan pattern > matching function. > > For VXLAN offload, matches should be done on outer header for tunnel > properties as well as inner packet matches. Add a function for parsing > VXLAN tunnel matches. > > Signed-off-by: Eli Britstein <elibr@nvidia.com> > Reviewed-by: Gaetan Rivet <gaetanr@nvidia.com> > Acked-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com> > Tested-by: Emma Finn <emma.finn@intel.com> > Tested-by: Marko Kovacevic <marko.kovacevic@intel.com> > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > --- > NEWS | 2 + > lib/netdev-offload-dpdk.c | 155 +++++++++++++++++++++++++++++++++++++- > 2 files changed, 156 insertions(+), 1 deletion(-) > > diff --git a/NEWS b/NEWS > index 6f8e62f7f..10b3ab053 100644 > --- a/NEWS > +++ b/NEWS > @@ -19,6 +19,8 @@ Post-v2.15.0 > * New debug appctl command 'dpdk/get-malloc-stats'. > * Add hardware offload support for tunnel pop action (experimental). > Available only if DPDK experimantal APIs enabled during the build. > + * Add hardware offload support for VXLAN flows (experimental). > + Available only if DPDK experimantal APIs enabled during the build. > - ovsdb-tool: > * New option '--election-timer' to the 'create-cluster' command to set the > leader election timer during cluster creation. > diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c > index 6220394de..6bd5b6c9f 100644 > --- a/lib/netdev-offload-dpdk.c > +++ b/lib/netdev-offload-dpdk.c > @@ -372,6 +372,20 @@ dump_flow_pattern(struct ds *s, > ipv6_mask->hdr.hop_limits); > } > ds_put_cstr(s, "/ "); > + } else if (item->type == RTE_FLOW_ITEM_TYPE_VXLAN) { > + const struct rte_flow_item_vxlan *vxlan_spec = item->spec; > + const struct rte_flow_item_vxlan *vxlan_mask = item->mask; > + > + ds_put_cstr(s, "vxlan "); > + if (vxlan_spec) { > + if (!vxlan_mask) { > + vxlan_mask = &rte_flow_item_vxlan_mask; > + } > + DUMP_PATTERN_ITEM(vxlan_mask->vni, "vni", "%"PRIu32, > + ntohl(*(ovs_be32 *) vxlan_spec->vni) >> 8, > + ntohl(*(ovs_be32 *) vxlan_mask->vni) >> 8); > + } > + ds_put_cstr(s, "/ "); > } else { > ds_put_format(s, "unknown rte flow pattern (%d)\n", item->type); > } > @@ -865,15 +879,154 @@ out: > return ret; > } Hi All, Is there a build failure in OVS master branch since this has been merged? It seems the above casts (ovs_be32 *) are causing issues with increasing alignment? From DPDK's struct rte_flow_item_vxlan, the vni is a "uint8_t vni[3];" The VNI is not a 32-bit value, so is not required to be aligned on 32-bits. The cast here technically requires the alignment of the casted pointer to be 32-bit/4 byte, which is not guaranteed to be true. I think the compiler rightly flags a cast alignment issue? Casting through a (void *) might solve, or else using some of the BE/LE conversion and alignment helpers in byte-order.h and unaligned.h could perhaps work? My testing as follows below. Regards, -Harry Configure: ./configure CFLAGS="-O3 -g -msse4.2 -mpopcnt -Wall -Wextra" --with-dpdk=static --enable-Werror Compiler, from Ubuntu 21.04: gcc (Ubuntu 10.3.0-1ubuntu1) 10.3.0 Build Failure: lib/netdev-offload-dpdk.c: In function 'dump_flow_pattern': lib/netdev-offload-dpdk.c:385:38: error: cast increases required alignment of target type [-Werror=cast-align] 385 | ntohl(*(ovs_be32 *) vxlan_spec->vni) >> 8, | ^ lib/netdev-offload-dpdk.c:189:48: note: in definition of macro 'DUMP_PATTERN_ITEM' 189 | ds_put_format(s, field " is " fmt " ", spec_pri); \ | ^~~~~~~~ lib/netdev-offload-dpdk.c:385:38: error: cast increases required alignment of target type [-Werror=cast-align] 385 | ntohl(*(ovs_be32 *) vxlan_spec->vni) >> 8, | ^ lib/netdev-offload-dpdk.c:192:23: note: in definition of macro 'DUMP_PATTERN_ITEM' 192 | spec_pri, mask_pri); \ | ^~~~~~~~ lib/netdev-offload-dpdk.c:386:38: error: cast increases required alignment of target type [-Werror=cast-align] 386 | ntohl(*(ovs_be32 *) vxlan_mask->vni) >> 8); | ^ lib/netdev-offload-dpdk.c:192:33: note: in definition of macro 'DUMP_PATTERN_ITEM' 192 | spec_pri, mask_pri); \ | ^~~~~~~~ In file included from /usr/include/netinet/ip6.h:22, from lib/netdev-offload-dpdk.c:20: lib/netdev-offload-dpdk.c: In function 'dump_vxlan_encap': lib/netdev-offload-dpdk.c:421:30: error: cast increases required alignment of target type [-Werror=cast-align] 421 | ntohl(*(ovs_be32 *) vxlan->vni) >> 8); | ^ lib/netdev-offload-dpdk.c: In function 'dump_flow_action': lib/netdev-offload-dpdk.c:572:30: error: cast increases required alignment of target type [-Werror=cast-align] 572 | ipv6_format_addr((struct in6_addr *) &set_ipv6->ipv6_addr, s); | ^ lib/netdev-offload-dpdk.c: In function 'parse_vxlan_match': lib/netdev-offload-dpdk.c:1002:24: error: cast increases required alignment of target type [-Werror=cast-align] 1002 | put_unaligned_be32((ovs_be32 *) vx_spec->vni, | ^ lib/netdev-offload-dpdk.c:1004:24: error: cast increases required alignment of target type [-Werror=cast-align] 1004 | put_unaligned_be32((ovs_be32 *) vx_mask->vni, | ^ cc1: all warnings being treated as errors
On 7/1/21 6:36 PM, Van Haaren, Harry wrote: >> -----Original Message----- >> From: dev <ovs-dev-bounces@openvswitch.org> On Behalf Of Eli Britstein >> Sent: Wednesday, June 23, 2021 4:53 PM >> To: dev@openvswitch.org; Ilya Maximets <i.maximets@ovn.org> >> Cc: Eli Britstein <elibr@nvidia.com>; Ivan Malov <Ivan.Malov@oktetlabs.ru>; Majd >> Dibbiny <majd@nvidia.com> >> Subject: [ovs-dev] [PATCH V7 13/13] netdev-dpdk-offload: Add vxlan pattern >> matching function. >> >> For VXLAN offload, matches should be done on outer header for tunnel >> properties as well as inner packet matches. Add a function for parsing >> VXLAN tunnel matches. >> >> Signed-off-by: Eli Britstein <elibr@nvidia.com> >> Reviewed-by: Gaetan Rivet <gaetanr@nvidia.com> >> Acked-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com> >> Tested-by: Emma Finn <emma.finn@intel.com> >> Tested-by: Marko Kovacevic <marko.kovacevic@intel.com> >> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >> --- >> NEWS | 2 + >> lib/netdev-offload-dpdk.c | 155 +++++++++++++++++++++++++++++++++++++- >> 2 files changed, 156 insertions(+), 1 deletion(-) >> >> diff --git a/NEWS b/NEWS >> index 6f8e62f7f..10b3ab053 100644 >> --- a/NEWS >> +++ b/NEWS >> @@ -19,6 +19,8 @@ Post-v2.15.0 >> * New debug appctl command 'dpdk/get-malloc-stats'. >> * Add hardware offload support for tunnel pop action (experimental). >> Available only if DPDK experimantal APIs enabled during the build. >> + * Add hardware offload support for VXLAN flows (experimental). >> + Available only if DPDK experimantal APIs enabled during the build. >> - ovsdb-tool: >> * New option '--election-timer' to the 'create-cluster' command to set the >> leader election timer during cluster creation. >> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c >> index 6220394de..6bd5b6c9f 100644 >> --- a/lib/netdev-offload-dpdk.c >> +++ b/lib/netdev-offload-dpdk.c >> @@ -372,6 +372,20 @@ dump_flow_pattern(struct ds *s, >> ipv6_mask->hdr.hop_limits); >> } >> ds_put_cstr(s, "/ "); >> + } else if (item->type == RTE_FLOW_ITEM_TYPE_VXLAN) { >> + const struct rte_flow_item_vxlan *vxlan_spec = item->spec; >> + const struct rte_flow_item_vxlan *vxlan_mask = item->mask; >> + >> + ds_put_cstr(s, "vxlan "); >> + if (vxlan_spec) { >> + if (!vxlan_mask) { >> + vxlan_mask = &rte_flow_item_vxlan_mask; >> + } >> + DUMP_PATTERN_ITEM(vxlan_mask->vni, "vni", "%"PRIu32, >> + ntohl(*(ovs_be32 *) vxlan_spec->vni) >> 8, >> + ntohl(*(ovs_be32 *) vxlan_mask->vni) >> 8); >> + } >> + ds_put_cstr(s, "/ "); >> } else { >> ds_put_format(s, "unknown rte flow pattern (%d)\n", item->type); >> } >> @@ -865,15 +879,154 @@ out: >> return ret; >> } > > Hi All, > > Is there a build failure in OVS master branch since this has been merged? I didn't notice any build failures. The main factor here is that you just can't build with DPDK without -Wno-cast-align anyway with neither modern gcc nor clang. So, this should not be an issue. gcc 10.3.1 on fedora generates myriads of cast-align warnings in DPDK headers: ./configure CC=gcc --enable-Werror --with-dpdk=static Part of the build log: ... /dpdk/build/include/rte_memcpy.h: In function ‘rte_mov16’: /dpdk/build/include/rte_memcpy.h:499:42: error: cast increases required alignment of target type [-Werror=cast-align] 499 | xmm0 = _mm_loadu_si128((const __m128i *)(const __m128i *)src); | ^ /dpdk/build/include/rte_memcpy.h:500:19: error: cast increases required alignment of target type [-Werror=cast-align] 500 | _mm_storeu_si128((__m128i *)dst, xmm0); | ^ /dpdk/build/include/rte_memcpy.h: In function ‘rte_memcpy_generic’: /dpdk/build/include/rte_memcpy.h:584:32: error: cast increases required alignment of target type [-Werror=cast-align] 584 | xmm0 = _mm_loadu_si128((const __m128i *)((const uint8_t *)src - offset + 0 * 16)); \ ... clang always complained about them, as far as I remember. Also, gcc on ubuntu 18.04 in our GHA doesn't seem to generate warnings that you're seeing, but it doesn't generate warnings for DPDK headers either, so it maybe just not that smart. > It seems the above casts (ovs_be32 *) are causing issues with increasing alignment? > > From DPDK's struct rte_flow_item_vxlan, the vni is a "uint8_t vni[3];" > > The VNI is not a 32-bit value, so is not required to be aligned on 32-bits. The cast here > technically requires the alignment of the casted pointer to be 32-bit/4 byte, which is > not guaranteed to be true. I think the compiler rightly flags a cast alignment issue? I briefly inspected the resulted binary and I didn't notice any actual changes in alignments, so this, likely, doesn't cause any real problems, at least on x86. But, I agree that we, probably, should fix that to have a more correct code. > > Casting through a (void *) might solve, or else using some of the BE/LE conversion > and alignment helpers in byte-order.h and unaligned.h could perhaps work? I think, get_unaligned_be32() is a way to go here. Feel free to submit a patch. > > My testing as follows below. Regards, -Harry > > > Configure: > ./configure CFLAGS="-O3 -g -msse4.2 -mpopcnt -Wall -Wextra" --with-dpdk=static --enable-Werror > > Compiler, from Ubuntu 21.04: > gcc (Ubuntu 10.3.0-1ubuntu1) 10.3.0 > > Build Failure: > lib/netdev-offload-dpdk.c: In function 'dump_flow_pattern': > lib/netdev-offload-dpdk.c:385:38: error: cast increases required alignment of target type [-Werror=cast-align] > 385 | ntohl(*(ovs_be32 *) vxlan_spec->vni) >> 8, > | ^ > lib/netdev-offload-dpdk.c:189:48: note: in definition of macro 'DUMP_PATTERN_ITEM' > 189 | ds_put_format(s, field " is " fmt " ", spec_pri); \ > | ^~~~~~~~ > lib/netdev-offload-dpdk.c:385:38: error: cast increases required alignment of target type [-Werror=cast-align] > 385 | ntohl(*(ovs_be32 *) vxlan_spec->vni) >> 8, > | ^ > lib/netdev-offload-dpdk.c:192:23: note: in definition of macro 'DUMP_PATTERN_ITEM' > 192 | spec_pri, mask_pri); \ > | ^~~~~~~~ > lib/netdev-offload-dpdk.c:386:38: error: cast increases required alignment of target type [-Werror=cast-align] > 386 | ntohl(*(ovs_be32 *) vxlan_mask->vni) >> 8); > | ^ > lib/netdev-offload-dpdk.c:192:33: note: in definition of macro 'DUMP_PATTERN_ITEM' > 192 | spec_pri, mask_pri); \ > | ^~~~~~~~ > In file included from /usr/include/netinet/ip6.h:22, > from lib/netdev-offload-dpdk.c:20: > lib/netdev-offload-dpdk.c: In function 'dump_vxlan_encap': > lib/netdev-offload-dpdk.c:421:30: error: cast increases required alignment of target type [-Werror=cast-align] > 421 | ntohl(*(ovs_be32 *) vxlan->vni) >> 8); > | ^ > lib/netdev-offload-dpdk.c: In function 'dump_flow_action': > lib/netdev-offload-dpdk.c:572:30: error: cast increases required alignment of target type [-Werror=cast-align] > 572 | ipv6_format_addr((struct in6_addr *) &set_ipv6->ipv6_addr, s); > | ^ > lib/netdev-offload-dpdk.c: In function 'parse_vxlan_match': > lib/netdev-offload-dpdk.c:1002:24: error: cast increases required alignment of target type [-Werror=cast-align] > 1002 | put_unaligned_be32((ovs_be32 *) vx_spec->vni, > | ^ > lib/netdev-offload-dpdk.c:1004:24: error: cast increases required alignment of target type [-Werror=cast-align] > 1004 | put_unaligned_be32((ovs_be32 *) vx_mask->vni, > | ^ > cc1: all warnings being treated as errors >
> -----Original Message----- > From: Ilya Maximets <i.maximets@ovn.org> > Sent: Friday, July 2, 2021 2:34 PM > To: Van Haaren, Harry <harry.van.haaren@intel.com>; Eli Britstein > <elibr@nvidia.com>; dev@openvswitch.org; Ilya Maximets <i.maximets@ovn.org> > Cc: Ivan Malov <Ivan.Malov@oktetlabs.ru>; Majd Dibbiny <majd@nvidia.com> > Subject: Re: [ovs-dev] [PATCH V7 13/13] netdev-dpdk-offload: Add vxlan pattern > matching function. > > On 7/1/21 6:36 PM, Van Haaren, Harry wrote: <snip commit message> > > Is there a build failure in OVS master branch since this has been merged? > > I didn't notice any build failures. The main factor here is that you just > can't build with DPDK without -Wno-cast-align anyway with neither modern gcc > nor clang. Actually, yes it is possible to build OVS with DPDK, I've just tested this with a fresh clone: git clone https://github.com/openvswitch/ovs (commit 780b2bde8 on master) cd ovs ./boot.sh ./configure --enable-Werror --with-dpdk=static make The above steps *fail to compile* due to the code changes introduce in this patch. This above steps *pass* if the code here (and in another vxlan offload commit) are fixed. (See void* casting suggestion below, which was used to fix the compile issues here). If DPDK has issues in cast-align, then those should be addressed in that community. Saying that it's OK to introduce issues around cast-align in OVS because DPDK has them is not a good argument. If I could point out a DPDK segfault, would it be OK to introduce segfaulting code in OVS? No. Bugs originating from cast-align warnings are no different. > So, this should not be an issue. This is an issue, and the fact that an OVS maintainer is downplaying breaking the build when the issue is pointed out is frustrating. > gcc 10.3.1 on fedora generates myriads of cast-align warnings in DPDK headers: > > ./configure CC=gcc --enable-Werror --with-dpdk=static Perhaps on the system being tested there, but this works fine here. If there are issues in DPDK, then file a bug there. Do not use potential issues in other projects to try cover up having introduced code that breaks the OVS build. <snip> > > It seems the above casts (ovs_be32 *) are causing issues with increasing > alignment? > > > > From DPDK's struct rte_flow_item_vxlan, the vni is a "uint8_t vni[3];" > > > > The VNI is not a 32-bit value, so is not required to be aligned on 32-bits. The cast > here > > technically requires the alignment of the casted pointer to be 32-bit/4 byte, which > is > > not guaranteed to be true. I think the compiler rightly flags a cast alignment issue? > > I briefly inspected the resulted binary and I didn't notice any actual > changes in alignments, so this, likely, doesn't cause any real problems, > at least on x86. But, I agree that we, probably, should fix that to > have a more correct code. I agree that the code on x86-64 is likely to be the same with a (void *) cast. I disagree that we "probably, should" fix this. Its an issue, it must be fixed. It breaks the build when compiled with -Werror enabled (as shown above). > > Casting through a (void *) might solve, or else using some of the BE/LE conversion > > and alignment helpers in byte-order.h and unaligned.h could perhaps work? > > I think, get_unaligned_be32() is a way to go here. A (void *) cast will likely have no impact on resulting ASM, but just mutes the warning. get_unaligned_be32() may result in 4x byte loads and shifts and ORs, instead of a 4-byte load. > Feel free to submit a patch. I have no intention of fixing bugs introduced in a patch that was merged while there were open outstanding issues to be resolved. Consider this email a "Reported-by". For anybody not aware of the context, refer to this thread: https://mail.openvswitch.org/pipermail/ovs-dev/2021-July/thread.html#384738
On 7/6/21 3:03 PM, Van Haaren, Harry wrote: >> -----Original Message----- >> From: Ilya Maximets <i.maximets@ovn.org> >> Sent: Friday, July 2, 2021 2:34 PM >> To: Van Haaren, Harry <harry.van.haaren@intel.com>; Eli Britstein >> <elibr@nvidia.com>; dev@openvswitch.org; Ilya Maximets <i.maximets@ovn.org> >> Cc: Ivan Malov <Ivan.Malov@oktetlabs.ru>; Majd Dibbiny <majd@nvidia.com> >> Subject: Re: [ovs-dev] [PATCH V7 13/13] netdev-dpdk-offload: Add vxlan pattern >> matching function. >> >> On 7/1/21 6:36 PM, Van Haaren, Harry wrote: > > <snip commit message> > >>> Is there a build failure in OVS master branch since this has been merged? >> >> I didn't notice any build failures. The main factor here is that you just >> can't build with DPDK without -Wno-cast-align anyway with neither modern gcc >> nor clang. > > Actually, yes it is possible to build OVS with DPDK, I've just tested this with a fresh clone: > git clone https://github.com/openvswitch/ovs (commit 780b2bde8 on master) > cd ovs > ./boot.sh > ./configure --enable-Werror --with-dpdk=static > make > > The above steps *fail to compile* due to the code changes introduce in this patch. > This above steps *pass* if the code here (and in another vxlan offload commit) are fixed. > (See void* casting suggestion below, which was used to fix the compile issues here). > > If DPDK has issues in cast-align, then those should be addressed in that community. > Saying that it's OK to introduce issues around cast-align in OVS because DPDK has them > is not a good argument. If I could point out a DPDK segfault, would it be OK to introduce > segfaulting code in OVS? No. Bugs originating from cast-align warnings are no different. I'm not saying that we should introduce issues existed in other projects, I'm just saying that it's very easy to miss this issue because of flags that needs to be passed in order to build OVS. I have no compilers that can build this code without -fno-cast-align, same for our CI. > > >> So, this should not be an issue. > > This is an issue, and the fact that an OVS maintainer is downplaying breaking the build > when the issue is pointed out is frustrating. I don't have a system where this change leads to a build failure, same for our CI. I understand that certain compilers might handle things differently and I'm not suggesting to keep things as is. If you didn't notice, I suggested to fix that in may email below. > > >> gcc 10.3.1 on fedora generates myriads of cast-align warnings in DPDK headers: >> >> ./configure CC=gcc --enable-Werror --with-dpdk=static > > Perhaps on the system being tested there, but this works fine here. If there are issues in > DPDK, then file a bug there. DPDK project is fully aware of this and has no intention to fix. And they have -fno-cast-align in their CI systems. > Do not use potential issues in other projects to try cover up > having introduced code that breaks the OVS build. I didn't suggest that. > > <snip> > > >>> It seems the above casts (ovs_be32 *) are causing issues with increasing >> alignment? >>> >>> From DPDK's struct rte_flow_item_vxlan, the vni is a "uint8_t vni[3];" >>> >>> The VNI is not a 32-bit value, so is not required to be aligned on 32-bits. The cast >> here >>> technically requires the alignment of the casted pointer to be 32-bit/4 byte, which >> is >>> not guaranteed to be true. I think the compiler rightly flags a cast alignment issue? >> >> I briefly inspected the resulted binary and I didn't notice any actual >> changes in alignments, so this, likely, doesn't cause any real problems, >> at least on x86. But, I agree that we, probably, should fix that to >> have a more correct code. > > I agree that the code on x86-64 is likely to be the same with a (void *) cast. > > I disagree that we "probably, should" fix this. Its an issue, it must be fixed. > It breaks the build when compiled with -Werror enabled (as shown above). When I'm saying "probably, should", I usually mean "we need to". That is just a more mild way to put that. Sorry for my English, if that wasn't clear. > > >>> Casting through a (void *) might solve, or else using some of the BE/LE conversion >>> and alignment helpers in byte-order.h and unaligned.h could perhaps work? >> >> I think, get_unaligned_be32() is a way to go here. > > A (void *) cast will likely have no impact on resulting ASM, but just mutes the warning. > get_unaligned_be32() may result in 4x byte loads and shifts and ORs, instead of a 4-byte load. So why you're suggesting to just silence the warning instead of fixing the problem here? In the same way you may just ignore the warning with -fno-cast-align. Please, be consistent. > > >> Feel free to submit a patch. > > I have no intention of fixing bugs introduced in a patch that was merged while there > were open outstanding issues to be resolved. Consider this email a "Reported-by". I have no issues with Reported-by and nobody should be obligated to fix the issue that they found, that is perfectly fine. Best regards, Ilya Maximets. > > For anybody not aware of the context, refer to this thread: > https://mail.openvswitch.org/pipermail/ovs-dev/2021-July/thread.html#384738 >
> -----Original Message----- > From: Ilya Maximets <i.maximets@ovn.org> > Sent: Wednesday, July 7, 2021 4:06 PM > To: Van Haaren, Harry <harry.van.haaren@intel.com>; Ilya Maximets > <i.maximets@ovn.org>; Eli Britstein <elibr@nvidia.com>; dev@openvswitch.org > Cc: Ivan Malov <Ivan.Malov@oktetlabs.ru>; Majd Dibbiny <majd@nvidia.com> > Subject: Re: [ovs-dev] [PATCH V7 13/13] netdev-dpdk-offload: Add vxlan pattern > matching function. > > On 7/6/21 3:03 PM, Van Haaren, Harry wrote: > >> -----Original Message----- > >> From: Ilya Maximets <i.maximets@ovn.org> > >> Sent: Friday, July 2, 2021 2:34 PM > >> To: Van Haaren, Harry <harry.van.haaren@intel.com>; Eli Britstein > >> <elibr@nvidia.com>; dev@openvswitch.org; Ilya Maximets > <i.maximets@ovn.org> > >> Cc: Ivan Malov <Ivan.Malov@oktetlabs.ru>; Majd Dibbiny > <majd@nvidia.com> > >> Subject: Re: [ovs-dev] [PATCH V7 13/13] netdev-dpdk-offload: Add vxlan > pattern > >> matching function. > >> > >> On 7/1/21 6:36 PM, Van Haaren, Harry wrote: > > > > <snip commit message> > > > >>> Is there a build failure in OVS master branch since this has been merged? > >> > >> I didn't notice any build failures. The main factor here is that you just > >> can't build with DPDK without -Wno-cast-align anyway with neither modern > gcc > >> nor clang. > > > > Actually, yes it is possible to build OVS with DPDK, I've just tested this with a > fresh clone: > > git clone https://github.com/openvswitch/ovs (commit 780b2bde8 on master) > > cd ovs > > ./boot.sh > > ./configure --enable-Werror --with-dpdk=static > > make > > > > The above steps *fail to compile* due to the code changes introduce in this > patch. > > This above steps *pass* if the code here (and in another vxlan offload commit) > are fixed. > > (See void* casting suggestion below, which was used to fix the compile issues > here). > > > > If DPDK has issues in cast-align, then those should be addressed in that > community. > > Saying that it's OK to introduce issues around cast-align in OVS because DPDK > has them > > is not a good argument. If I could point out a DPDK segfault, would it be OK to > introduce > > segfaulting code in OVS? No. Bugs originating from cast-align warnings are no > different. > > I'm not saying that we should introduce issues existed in other projects, > I'm just saying that it's very easy to miss this issue because of flags > that needs to be passed in order to build OVS. I have no compilers that > can build this code without -fno-cast-align, same for our CI. > > > > > > >> So, this should not be an issue. > > > > This is an issue, and the fact that an OVS maintainer is downplaying breaking the > build > > when the issue is pointed out is frustrating. > > I don't have a system where this change leads to a build failure, same > for our CI. I understand that certain compilers might handle things > differently and I'm not suggesting to keep things as is. If you didn't > notice, I suggested to fix that in may email below. > > > > > > >> gcc 10.3.1 on fedora generates myriads of cast-align warnings in DPDK > headers: > >> > >> ./configure CC=gcc --enable-Werror --with-dpdk=static > > > > Perhaps on the system being tested there, but this works fine here. If there are > issues in > > DPDK, then file a bug there. > > DPDK project is fully aware of this and has no intention to fix. And they > have -fno-cast-align in their CI systems. > > > Do not use potential issues in other projects to try cover up > > having introduced code that breaks the OVS build. > > I didn't suggest that. > > > > > <snip> > > > > > >>> It seems the above casts (ovs_be32 *) are causing issues with increasing > >> alignment? > >>> > >>> From DPDK's struct rte_flow_item_vxlan, the vni is a "uint8_t vni[3];" > >>> > >>> The VNI is not a 32-bit value, so is not required to be aligned on 32-bits. The > cast > >> here > >>> technically requires the alignment of the casted pointer to be 32-bit/4 byte, > which > >> is > >>> not guaranteed to be true. I think the compiler rightly flags a cast alignment > issue? > >> > >> I briefly inspected the resulted binary and I didn't notice any actual > >> changes in alignments, so this, likely, doesn't cause any real problems, > >> at least on x86. But, I agree that we, probably, should fix that to > >> have a more correct code. > > > > I agree that the code on x86-64 is likely to be the same with a (void *) cast. > > > > I disagree that we "probably, should" fix this. Its an issue, it must be fixed. > > It breaks the build when compiled with -Werror enabled (as shown above). > > When I'm saying "probably, should", I usually mean "we need to". > That is just a more mild way to put that. Sorry for my English, > if that wasn't clear. OK, no problem. > >>> Casting through a (void *) might solve, or else using some of the BE/LE > conversion > >>> and alignment helpers in byte-order.h and unaligned.h could perhaps work? > >> > >> I think, get_unaligned_be32() is a way to go here. > > > > A (void *) cast will likely have no impact on resulting ASM, but just mutes the > warning. > > get_unaligned_be32() may result in 4x byte loads and shifts and ORs, instead of > a 4-byte load. > > So why you're suggesting to just silence the warning instead of > fixing the problem here? In the same way you may just ignore > the warning with -fno-cast-align. Please, be consistent. My comments above are technical comments on the ASM output of two methods of fixing the build, I don't recommend merging the (void*) cast. I'm not familiar with the context here, should be BE/LE conversion here or not, what does rte_flow provide, what do we want to print to the string. Somebody who was involved in the code authoring/review is a better candidate to fix. > >> Feel free to submit a patch. > > > > I have no intention of fixing bugs introduced in a patch that was merged while > there > > were open outstanding issues to be resolved. Consider this email a "Reported- > by". > > I have no issues with Reported-by and nobody should be obligated > to fix the issue that they found, that is perfectly fine. In this case can I ask you Eli (as author of the original patch) to fix the issue? > Best regards, Ilya Maximets. Regards, -Harry <snip>
diff --git a/NEWS b/NEWS index 6f8e62f7f..10b3ab053 100644 --- a/NEWS +++ b/NEWS @@ -19,6 +19,8 @@ Post-v2.15.0 * New debug appctl command 'dpdk/get-malloc-stats'. * Add hardware offload support for tunnel pop action (experimental). Available only if DPDK experimantal APIs enabled during the build. + * Add hardware offload support for VXLAN flows (experimental). + Available only if DPDK experimantal APIs enabled during the build. - ovsdb-tool: * New option '--election-timer' to the 'create-cluster' command to set the leader election timer during cluster creation. diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c index 6220394de..6bd5b6c9f 100644 --- a/lib/netdev-offload-dpdk.c +++ b/lib/netdev-offload-dpdk.c @@ -372,6 +372,20 @@ dump_flow_pattern(struct ds *s, ipv6_mask->hdr.hop_limits); } ds_put_cstr(s, "/ "); + } else if (item->type == RTE_FLOW_ITEM_TYPE_VXLAN) { + const struct rte_flow_item_vxlan *vxlan_spec = item->spec; + const struct rte_flow_item_vxlan *vxlan_mask = item->mask; + + ds_put_cstr(s, "vxlan "); + if (vxlan_spec) { + if (!vxlan_mask) { + vxlan_mask = &rte_flow_item_vxlan_mask; + } + DUMP_PATTERN_ITEM(vxlan_mask->vni, "vni", "%"PRIu32, + ntohl(*(ovs_be32 *) vxlan_spec->vni) >> 8, + ntohl(*(ovs_be32 *) vxlan_mask->vni) >> 8); + } + ds_put_cstr(s, "/ "); } else { ds_put_format(s, "unknown rte flow pattern (%d)\n", item->type); } @@ -865,15 +879,154 @@ out: return ret; } +static int +parse_tnl_ip_match(struct flow_patterns *patterns, + struct match *match, + uint8_t proto) +{ + struct flow *consumed_masks; + + consumed_masks = &match->wc.masks; + /* IP v4 */ + if (match->wc.masks.tunnel.ip_src || match->wc.masks.tunnel.ip_dst) { + struct rte_flow_item_ipv4 *spec, *mask; + + spec = xzalloc(sizeof *spec); + mask = xzalloc(sizeof *mask); + + spec->hdr.type_of_service = match->flow.tunnel.ip_tos; + spec->hdr.time_to_live = match->flow.tunnel.ip_ttl; + spec->hdr.next_proto_id = proto; + spec->hdr.src_addr = match->flow.tunnel.ip_src; + spec->hdr.dst_addr = match->flow.tunnel.ip_dst; + + mask->hdr.type_of_service = match->wc.masks.tunnel.ip_tos; + mask->hdr.time_to_live = match->wc.masks.tunnel.ip_ttl; + mask->hdr.next_proto_id = UINT8_MAX; + mask->hdr.src_addr = match->wc.masks.tunnel.ip_src; + mask->hdr.dst_addr = match->wc.masks.tunnel.ip_dst; + + consumed_masks->tunnel.ip_tos = 0; + consumed_masks->tunnel.ip_ttl = 0; + consumed_masks->tunnel.ip_src = 0; + consumed_masks->tunnel.ip_dst = 0; + + add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_IPV4, spec, mask); + } else if (!is_all_zeros(&match->wc.masks.tunnel.ipv6_src, + sizeof(struct in6_addr)) || + !is_all_zeros(&match->wc.masks.tunnel.ipv6_dst, + sizeof(struct in6_addr))) { + /* IP v6 */ + struct rte_flow_item_ipv6 *spec, *mask; + + spec = xzalloc(sizeof *spec); + mask = xzalloc(sizeof *mask); + + spec->hdr.proto = proto; + spec->hdr.hop_limits = match->flow.tunnel.ip_ttl; + spec->hdr.vtc_flow = htonl((uint32_t) match->flow.tunnel.ip_tos << + RTE_IPV6_HDR_TC_SHIFT); + memcpy(spec->hdr.src_addr, &match->flow.tunnel.ipv6_src, + sizeof spec->hdr.src_addr); + memcpy(spec->hdr.dst_addr, &match->flow.tunnel.ipv6_dst, + sizeof spec->hdr.dst_addr); + + mask->hdr.proto = UINT8_MAX; + mask->hdr.hop_limits = match->wc.masks.tunnel.ip_ttl; + mask->hdr.vtc_flow = htonl((uint32_t) match->wc.masks.tunnel.ip_tos << + RTE_IPV6_HDR_TC_SHIFT); + memcpy(mask->hdr.src_addr, &match->wc.masks.tunnel.ipv6_src, + sizeof mask->hdr.src_addr); + memcpy(mask->hdr.dst_addr, &match->wc.masks.tunnel.ipv6_dst, + sizeof mask->hdr.dst_addr); + + consumed_masks->tunnel.ip_tos = 0; + consumed_masks->tunnel.ip_ttl = 0; + memset(&consumed_masks->tunnel.ipv6_src, 0, + sizeof consumed_masks->tunnel.ipv6_src); + memset(&consumed_masks->tunnel.ipv6_dst, 0, + sizeof consumed_masks->tunnel.ipv6_dst); + + add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_IPV6, spec, mask); + } else { + VLOG_ERR_RL(&rl, "Tunnel L3 protocol is neither IPv4 nor IPv6"); + return -1; + } + + return 0; +} + +static void +parse_tnl_udp_match(struct flow_patterns *patterns, + struct match *match) +{ + struct flow *consumed_masks; + struct rte_flow_item_udp *spec, *mask; + + consumed_masks = &match->wc.masks; + + spec = xzalloc(sizeof *spec); + mask = xzalloc(sizeof *mask); + + spec->hdr.src_port = match->flow.tunnel.tp_src; + spec->hdr.dst_port = match->flow.tunnel.tp_dst; + + mask->hdr.src_port = match->wc.masks.tunnel.tp_src; + mask->hdr.dst_port = match->wc.masks.tunnel.tp_dst; + + consumed_masks->tunnel.tp_src = 0; + consumed_masks->tunnel.tp_dst = 0; + + add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_UDP, spec, mask); +} + +static int +parse_vxlan_match(struct flow_patterns *patterns, + struct match *match) +{ + struct rte_flow_item_vxlan *vx_spec, *vx_mask; + struct flow *consumed_masks; + int ret; + + ret = parse_tnl_ip_match(patterns, match, IPPROTO_UDP); + if (ret) { + return -1; + } + parse_tnl_udp_match(patterns, match); + + consumed_masks = &match->wc.masks; + /* VXLAN */ + vx_spec = xzalloc(sizeof *vx_spec); + vx_mask = xzalloc(sizeof *vx_mask); + + put_unaligned_be32((ovs_be32 *) vx_spec->vni, + htonl(ntohll(match->flow.tunnel.tun_id) << 8)); + put_unaligned_be32((ovs_be32 *) vx_mask->vni, + htonl(ntohll(match->wc.masks.tunnel.tun_id) << 8)); + + consumed_masks->tunnel.tun_id = 0; + consumed_masks->tunnel.flags = 0; + + add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_VXLAN, vx_spec, vx_mask); + return 0; +} + static int OVS_UNUSED parse_flow_tnl_match(struct netdev *tnldev, struct flow_patterns *patterns, odp_port_t orig_in_port, - struct match *match OVS_UNUSED) + struct match *match) { int ret; ret = add_vport_match(patterns, orig_in_port, tnldev); + if (ret) { + return ret; + } + + if (!strcmp(netdev_get_type(tnldev), "vxlan")) { + ret = parse_vxlan_match(patterns, match); + } return ret; }