| Message ID | 20240617140837.508342-1-emma.finn@intel.com |
|---|---|
| State | Accepted |
| Commit | 48118494497040e71c0c60f59ab5664c5b00464c |
| Delegated to: | Eelco Chaudron |
| Headers | show |
| Series | [ovs-dev,v2] odp-execute: Check IPv4 checksum offload flag in AVX. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On 17 Jun 2024, at 16:08, Emma Finn wrote: > The AVX implementation for IPv4 action did not check whether > the IPv4 checksum offload flag has been set and was incorrectly > calculating checksums in software. Adding a check to skip AVX > checksum calculation when offload flags are set. > > Signed-off-by: Emma Finn <emma.finn@intel.com> > Reported-by: Eelco Chaudron <echaudro@redhat.com> This is missing a fixes tag. And maybe you can also add which test is failing, so people reviewing know what to look for. ‘nsh - triangle PTAP bridge setup with NSH over vxlan-gpe’ > --- > lib/odp-execute-avx512.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/odp-execute-avx512.c b/lib/odp-execute-avx512.c > index 569ea789e..54bd556e1 100644 > --- a/lib/odp-execute-avx512.c > +++ b/lib/odp-execute-avx512.c > @@ -473,7 +473,7 @@ action_avx512_ipv4_set_addrs(struct dp_packet_batch *batch, > * (v_pkt_masked). */ > __m256i v_new_hdr = _mm256_or_si256(v_key_shuf, v_pkt_masked); > > - if (dp_packet_hwol_tx_ip_csum(packet)) { > + if (dp_packet_hwol_l3_ipv4(packet)) { I’m trying to understand why this change is needed. The scaler implementation is working fine with this check. Is something not initialized correctly in the AVX implementation? > dp_packet_ol_reset_ip_csum_good(packet); > } else { > ovs_be16 old_csum = ~nh->ip_csum; > -- > 2.34.1
> -----Original Message----- > From: Eelco Chaudron <echaudro@redhat.com> > Sent: Thursday, June 20, 2024 9:48 AM > To: Finn, Emma <emma.finn@intel.com> > Cc: ovs-dev@openvswitch.org; mkp@redhat.com > Subject: Re: [v2] odp-execute: Check IPv4 checksum offload flag in AVX. > > On 17 Jun 2024, at 16:08, Emma Finn wrote: > > > The AVX implementation for IPv4 action did not check whether the IPv4 > > checksum offload flag has been set and was incorrectly calculating > > checksums in software. Adding a check to skip AVX checksum calculation > > when offload flags are set. > > > > Signed-off-by: Emma Finn <emma.finn@intel.com> > > Reported-by: Eelco Chaudron <echaudro@redhat.com> > > This is missing a fixes tag. And maybe you can also add which test is failing, so > people reviewing know what to look for. > > ‘nsh - triangle PTAP bridge setup with NSH over vxlan-gpe’ > > > --- > > lib/odp-execute-avx512.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/lib/odp-execute-avx512.c b/lib/odp-execute-avx512.c index > > 569ea789e..54bd556e1 100644 > > --- a/lib/odp-execute-avx512.c > > +++ b/lib/odp-execute-avx512.c > > @@ -473,7 +473,7 @@ action_avx512_ipv4_set_addrs(struct > dp_packet_batch *batch, > > * (v_pkt_masked). */ > > __m256i v_new_hdr = _mm256_or_si256(v_key_shuf, > > v_pkt_masked); > > > > - if (dp_packet_hwol_tx_ip_csum(packet)) { > > + if (dp_packet_hwol_l3_ipv4(packet)) { > > I’m trying to understand why this change is needed. The scaler > implementation is working fine with this check. Is something not initialized > correctly in the AVX implementation? Sure, I can send v3 with fixes tag and failing test info. The test nsh - triangle PTAP bridge setup with NSH over vxlan-gpe, was failing the autovalidator because the AVX implementation was calculating a checksum for the outer IPv4 header when it shouldn't have been. Previously with just checking dp_packet_hwol_tx_ip_csum(), the AVX implementation was only checking if packet was marked for IPv4 csum offload. It was never checking if the packet was encapsulated AND the outer layer is marked for IPv4 checksum offload. The scalar does this check also in packet_set_ipv4_addr(). Thanks, Emma > > > dp_packet_ol_reset_ip_csum_good(packet); > > } else { > > ovs_be16 old_csum = ~nh->ip_csum; > > -- > > 2.34.1
On 20 Jun 2024, at 13:01, Finn, Emma wrote: >> -----Original Message----- >> From: Eelco Chaudron <echaudro@redhat.com> >> Sent: Thursday, June 20, 2024 9:48 AM >> To: Finn, Emma <emma.finn@intel.com> >> Cc: ovs-dev@openvswitch.org; mkp@redhat.com >> Subject: Re: [v2] odp-execute: Check IPv4 checksum offload flag in AVX. >> >> On 17 Jun 2024, at 16:08, Emma Finn wrote: >> >>> The AVX implementation for IPv4 action did not check whether the IPv4 >>> checksum offload flag has been set and was incorrectly calculating >>> checksums in software. Adding a check to skip AVX checksum calculation >>> when offload flags are set. >>> >>> Signed-off-by: Emma Finn <emma.finn@intel.com> >>> Reported-by: Eelco Chaudron <echaudro@redhat.com> >> >> This is missing a fixes tag. And maybe you can also add which test is failing, so >> people reviewing know what to look for. >> >> ‘nsh - triangle PTAP bridge setup with NSH over vxlan-gpe’ >> >>> --- >>> lib/odp-execute-avx512.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/lib/odp-execute-avx512.c b/lib/odp-execute-avx512.c index >>> 569ea789e..54bd556e1 100644 >>> --- a/lib/odp-execute-avx512.c >>> +++ b/lib/odp-execute-avx512.c >>> @@ -473,7 +473,7 @@ action_avx512_ipv4_set_addrs(struct >> dp_packet_batch *batch, >>> * (v_pkt_masked). */ >>> __m256i v_new_hdr = _mm256_or_si256(v_key_shuf, >>> v_pkt_masked); >>> >>> - if (dp_packet_hwol_tx_ip_csum(packet)) { >>> + if (dp_packet_hwol_l3_ipv4(packet)) { >> >> I’m trying to understand why this change is needed. The scaler >> implementation is working fine with this check. Is something not initialized >> correctly in the AVX implementation? > > Sure, I can send v3 with fixes tag and failing test info. > > The test nsh - triangle PTAP bridge setup with NSH over vxlan-gpe, was failing the autovalidator because the AVX implementation was calculating a checksum for the outer IPv4 header when it shouldn't have been. > Previously with just checking dp_packet_hwol_tx_ip_csum(), the AVX implementation was only checking if packet was marked for IPv4 csum offload. > It was never checking if the packet was encapsulated AND the outer layer is marked for IPv4 checksum offload. The scalar does this check also in packet_set_ipv4_addr(). You are right I misread the diff :( Let’s wait with the v3 as I know Mike was going to take a look at this also. If he finds nothing, I can make a suggestion for the change and apply. Cheers, Eelco >> >>> dp_packet_ol_reset_ip_csum_good(packet); >>> } else { >>> ovs_be16 old_csum = ~nh->ip_csum; >>> -- >>> 2.34.1
On Mon, Jun 17, 2024 at 10:08 AM Emma Finn <emma.finn@intel.com> wrote: > > The AVX implementation for IPv4 action did not check whether > the IPv4 checksum offload flag has been set and was incorrectly > calculating checksums in software. Adding a check to skip AVX > checksum calculation when offload flags are set. > > Signed-off-by: Emma Finn <emma.finn@intel.com> > Reported-by: Eelco Chaudron <echaudro@redhat.com> This brings things inline with the scalar odp-execute. Fixes: 5d11c47d3ebe ("userspace: Enable IP checksum offloading by default.") Acked-by: Mike Pattrick <mkp@redhat.com> Cheers, M
On 20 Jun 2024, at 20:48, Mike Pattrick wrote: > On Mon, Jun 17, 2024 at 10:08 AM Emma Finn <emma.finn@intel.com> wrote: >> >> The AVX implementation for IPv4 action did not check whether >> the IPv4 checksum offload flag has been set and was incorrectly >> calculating checksums in software. Adding a check to skip AVX >> checksum calculation when offload flags are set. >> >> Signed-off-by: Emma Finn <emma.finn@intel.com> >> Reported-by: Eelco Chaudron <echaudro@redhat.com> > > This brings things inline with the scalar odp-execute. > > Fixes: 5d11c47d3ebe ("userspace: Enable IP checksum offloading by default.") > Acked-by: Mike Pattrick <mkp@redhat.com> Thanks Mike for the review! I’ll wait till next week, and if I get no more comments/feedback I’ll apply both AVX512 patches with the suggested modifications. Cheers, Eelco
On 17 Jun 2024, at 16:08, Emma Finn wrote: > The AVX implementation for IPv4 action did not check whether > the IPv4 checksum offload flag has been set and was incorrectly > calculating checksums in software. Adding a check to skip AVX > checksum calculation when offload flags are set. > > Signed-off-by: Emma Finn <emma.finn@intel.com> > Reported-by: Eelco Chaudron <echaudro@redhat.com> Thanks Emma and Mike, the patch was committed to main and the 3.3 branch. Cheers, Eelco
diff --git a/lib/odp-execute-avx512.c b/lib/odp-execute-avx512.c index 569ea789e..54bd556e1 100644 --- a/lib/odp-execute-avx512.c +++ b/lib/odp-execute-avx512.c @@ -473,7 +473,7 @@ action_avx512_ipv4_set_addrs(struct dp_packet_batch *batch, * (v_pkt_masked). */ __m256i v_new_hdr = _mm256_or_si256(v_key_shuf, v_pkt_masked); - if (dp_packet_hwol_tx_ip_csum(packet)) { + if (dp_packet_hwol_l3_ipv4(packet)) { dp_packet_ol_reset_ip_csum_good(packet); } else { ovs_be16 old_csum = ~nh->ip_csum;
The AVX implementation for IPv4 action did not check whether the IPv4 checksum offload flag has been set and was incorrectly calculating checksums in software. Adding a check to skip AVX checksum calculation when offload flags are set. Signed-off-by: Emma Finn <emma.finn@intel.com> Reported-by: Eelco Chaudron <echaudro@redhat.com> --- lib/odp-execute-avx512.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)