Hi Ilya, Please find my replies Inline.
> -----Original Message----- > From: Ilya Maximets <[email protected]> > Sent: Wednesday, April 6, 2022 3:38 AM > To: Stokes, Ian <[email protected]>; Amber, Kumar > <[email protected]>; [email protected] > Cc: [email protected]; [email protected]; Aaron Conole > <[email protected]>; Adrian Moreno <[email protected]>; Dumitru > Ceara <[email protected]>; Ferriter, Cian <[email protected]>; Van > Haaren, Harry <[email protected]> > Subject: Re: [ovs-dev] [PATCH v8 1/4] dpif-netdev/mfex: Add ipv4 profile > based hashing. > > 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. > Thanks a lot Ilya for these valuable comments . Based on the suggestions have changed the typecast to unit32_t to memcpy. Although ASM for both typecast and memcpy generated same remains same . so, there is no actual real-world bug here (in x86-64 , which is the only platform this code will run on). But memcpy is a safe bet for all the compiler. Regards Amber > Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
