> -----Original Message----- > From: Ilya Maximets <i.maxim...@ovn.org> > Sent: Wednesday, July 7, 2021 4:06 PM > To: Van Haaren, Harry <harry.van.haa...@intel.com>; Ilya Maximets > <i.maxim...@ovn.org>; Eli Britstein <el...@nvidia.com>; d...@openvswitch.org > Cc: Ivan Malov <ivan.ma...@oktetlabs.ru>; Majd Dibbiny <m...@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.maxim...@ovn.org> > >> Sent: Friday, July 2, 2021 2:34 PM > >> To: Van Haaren, Harry <harry.van.haa...@intel.com>; Eli Britstein > >> <el...@nvidia.com>; d...@openvswitch.org; Ilya Maximets > <i.maxim...@ovn.org> > >> Cc: Ivan Malov <ivan.ma...@oktetlabs.ru>; Majd Dibbiny > <m...@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> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
