[ovs-dev] [PATCH V7 13/13] netdev-dpdk-offload: Add vxlan pattern matching function.

Ilya Maximets i.maximets at ovn.org
Wed Jul 7 15:05:42 UTC 2021


On 7/6/21 3:03 PM, Van Haaren, Harry wrote:
>> -----Original Message-----
>> From: Ilya Maximets <i.maximets at ovn.org>
>> Sent: Friday, July 2, 2021 2:34 PM
>> To: Van Haaren, Harry <harry.van.haaren at intel.com>; Eli Britstein
>> <elibr at nvidia.com>; dev at openvswitch.org; Ilya Maximets <i.maximets at ovn.org>
>> Cc: Ivan Malov <Ivan.Malov at oktetlabs.ru>; Majd Dibbiny <majd at 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
> 



More information about the dev mailing list