| Message ID | 20220106114551.3779260-1-harry.van.haaren@intel.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] dpif-netdev: improve loading of packet data for undersized packets | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
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 <i.maximets@ovn.org> > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com> The change looks fine to me, can’t test, as I lack an AVX machine. However, one small comment below. //Eelco > --- > 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? > + v_pkt0 = _mm512_loadu_si512(pkt); > + } else { > + uint64_t load_kmask = (1ULL << size) - 1; > + v_pkt0 = _mm512_maskz_loadu_epi8(load_kmask, pkt); > + } > + > __m512i v_pkt0_masked = _mm512_and_si512(v_pkt0, v_mask); > __mmask64 k_cmp = _mm512_cmpeq_epi8_mask(v_pkt0_masked, v_vals); > if (k_cmp != UINT64_MAX) { > -- > 2.25.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> -----Original Message----- > From: Eelco Chaudron <echaudro@redhat.com> > Sent: Thursday, January 6, 2022 1:01 PM > To: Van Haaren, Harry <harry.van.haaren@intel.com> > Cc: ovs-dev@openvswitch.org; i.maximets@ovn.org > 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 <i.maximets@ovn.org> > > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com> > > 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>
On 6 Jan 2022, at 14:32, Van Haaren, Harry wrote: >> -----Original Message----- >> From: Eelco Chaudron <echaudro@redhat.com> >> Sent: Thursday, January 6, 2022 1:01 PM >> To: Van Haaren, Harry <harry.van.haaren@intel.com> >> Cc: ovs-dev@openvswitch.org; i.maximets@ovn.org >> 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 <i.maximets@ovn.org> >>> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com> >> >> 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 <echaudro@redhat.com> > Thanks for read-review & checking though! > > <snip>
On 1/6/22 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. This statement is not correct. Few reasons: 1. Nitpick: there is no such thing as 'DPDK datapath'. 2. The issue is easily reproducible in production environments, i.e. it's not test-only. The reason for that is netdev-linux and other ports which are present in every OVS setup (at least the bridge port in userspace datapath is a tap interface). In a vast majority of setups these ports are actually up and has ip addresses. E.g. OpenStack is using tap/veth interfaces for DHCP and other stuff. And locally delivered packets (packets that never left the hypervisor) are not obliged to be padded up to 64 bytes. You may find that local ARP packets, for example, are typically 42 bytes long and that triggers the memory over-read in our case. > 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 <i.maximets@ovn.org> > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com> > --- > lib/dpif-netdev-extract-avx512.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) Thanks, Harry and Eelco. The change itself looks good to me and it fixes ASAN errors while running a few tests under SDE. So, I fixed the commit message and applied the patch. Also backported to 2.16. Best regards, Ilya Maximets.
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) { + v_pkt0 = _mm512_loadu_si512(pkt); + } else { + uint64_t load_kmask = (1ULL << size) - 1; + v_pkt0 = _mm512_maskz_loadu_epi8(load_kmask, pkt); + } + __m512i v_pkt0_masked = _mm512_and_si512(v_pkt0, v_mask); __mmask64 k_cmp = _mm512_cmpeq_epi8_mask(v_pkt0_masked, v_vals); if (k_cmp != UINT64_MAX) {
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 <i.maximets@ovn.org> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com> --- lib/dpif-netdev-extract-avx512.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)