On 6 Jan 2022, at 14:32, Van Haaren, Harry wrote:
>> -----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 Interesting, if I get some time I’ll try it out! >> 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. This is what I was thinking about, this is an “ERROR” case in normal operations, and this path is only taken in the test cases, which are not high performance (or performance should not matter). But anyway I’m fine, assuming you have not seen any performance impact with the change. As it compiles and runs :) Acked-by: Eelco Chaudron <[email protected]> > Thanks for read-review & checking though! > > <snip> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
