> -----Original Message----- > From: Eelco Chaudron <[email protected]> > Sent: Thursday, January 6, 2022 1:01 PM > To: Van Haaren, Harry <[email protected]> > Cc: [email protected]; [email protected] > Subject: Re: [ovs-dev] [PATCH] dpif-netdev: improve loading of packet data for > undersized packets > > > > On 6 Jan 2022, at 12:45, Harry van Haaren wrote: > > > This commit improves handling of packets where the allocated memory > > is less than 64 bytes. In the DPDK datapath this never matters, as > > an mbuf always pre-allocates enough space, however this can occur in > > test environments such as the dummy netdev. The fix is required to > > ensure ASAN enabled builds don't error on testing this, hence the > > fix is valuable. > > > > The solution implemented uses a mask-to-zero if the available buffer > > size is less than 64 bytes, and a branch for which type of load is used. > > > > Fixes: 250ceddcc2d0 ("dpif-netdev/mfex: Add AVX512 based optimized > miniflow extract") > > > > Reported-by: Ilya Maximets <[email protected]> > > Signed-off-by: Harry van Haaren <[email protected]> > > The change looks fine to me, can’t test, as I lack an AVX machine.
For those interested, the SDE tool should allow testing this if hardware is lacking; https://www.intel.com/content/www/us/en/developer/articles/tool/software-development-emulator.html > However, one small comment below. > > //Eelco Thanks for having a look, detailed reply below. Regards, -Harry > > --- > > lib/dpif-netdev-extract-avx512.c | 9 ++++++++- > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/lib/dpif-netdev-extract-avx512.c > > b/lib/dpif-netdev-extract-avx512.c > > index e060ab14a..d23349482 100644 > > --- a/lib/dpif-netdev-extract-avx512.c > > +++ b/lib/dpif-netdev-extract-avx512.c > > @@ -488,7 +488,14 @@ mfex_avx512_process(struct dp_packet_batch > *packets, > > > > /* Load packet data and probe with AVX512 mask & compare. */ > > const uint8_t *pkt = dp_packet_data(packet); > > - __m512i v_pkt0 = _mm512_loadu_si512(pkt); > > + __m512i v_pkt0; > > + if (size >= 64) { > > Does it make sense to add an OVS_LIKELY() here? Nope, not really a good candidate in my opinion. So OVS_LIKELY() does not influence *runtime* branch prediction, it influences the compilers generated code layout. As a result, LIKELY() basically says "put this on the linear-instructions path, and *jump far away* for the unlikely case (because its unlikely, perf shouldn't matter!) In the case of this branch, packet-after-packet could be taken/not-taken, so we really just don't know which is better. Often the compiler will interleave the two code-paths, and both will have a jump (one "into" its start point, the other "out"). Overally, LIKELY() should only be used when we *know* something to be an error condition, and is *invalid* to occur on the datapath. This isn't branch does not handle any invalid case, so no LIKELY/UNLIKELY here. Thanks for read-review & checking though! <snip> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
