> -----Original Message----- > From: Ilya Maximets <[email protected]> > Sent: Friday, July 2, 2021 2:34 PM > To: Van Haaren, Harry <[email protected]>; Eli Britstein > <[email protected]>; [email protected]; Ilya Maximets <[email protected]> > Cc: Ivan Malov <[email protected]>; Majd Dibbiny <[email protected]> > 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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
