On 4/5/22 15:51, Stokes, Ian wrote: > > >> -----Original Message----- >> From: Amber, Kumar <[email protected]> >> Sent: Friday, April 1, 2022 12:24 PM >> To: [email protected] >> Cc: Stokes, Ian <[email protected]>; [email protected]; Ferriter, Cian >> <[email protected]>; [email protected]; Van Haaren, Harry >> <[email protected]>; Amber, Kumar <[email protected]> >> Subject: [PATCH v8 1/4] dpif-netdev/mfex: Add ipv4 profile based hashing. >> >> For packets which don't already have a hash calculated, >> miniflow_hash_5tuple() calculates the hash of a packet >> using the previously built miniflow. >> >> This commit adds IPv4 profile specific hashing which >> uses fixed offsets into the packet to improve hashing >> performance. >> >> Signed-off-by: Kumar Amber <[email protected]> >> Signed-off-by: Harry van Haaren <[email protected]> >> Co-authored-by: Harry van Haaren <[email protected]> > > Thanks for working on this Amber/Harry and the reviews /testing Cian. > > Looking at this it seems to be in a good sate. Will apply to master unless > there are any objection?
Hmm. On a quick glance over the first patch I see a lot of unaligned memory accesses that are clearly an undefined behavior from the compiler point of view. Some of them are actually done for odd (!) memory addresses, e.g. &((uint8_t)pkt)[27] is converted to a uint32_t pointer and dereferenced. And unlike some other memory accesses in this file, alignment for these ones can actually easily be pre-calculated by a compiler potentially causing all sorts of fun stuff. It's not a coincidence that most of memory accesses in the generic flow.c are performed with memcpy() or get_16aligned_be32() or similar functions. This helps to avoid issues with incorrect compiler assumptions and potentially broken logic. Not only to support various different architectures. We just cleared a big pile of undefined behavior issues that were causing real mis-compilations in the code and some more fixes are coming, including enabling of UBsan in CI. So, I don't think the series should be accepted in the form it is now. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
