| Message ID | 20220131135453.3239792-1-harry.van.haaren@intel.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v3] dpif-netdev: fix vlan and ipv4 parsing in avx512 | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
Hi Harry, Tested Again and looks ohk now in random testing. Regards Amber > -----Original Message----- > From: Van Haaren, Harry <harry.van.haaren@intel.com> > Sent: Monday, January 31, 2022 7:25 PM > To: ovs-dev@openvswitch.org > Cc: Ferriter, Cian <cian.ferriter@intel.com>; Stokes, Ian > <ian.stokes@intel.com>; i.maximets@ovn.org; echaudro@redhat.com; Amber, > Kumar <kumar.amber@intel.com>; Van Haaren, Harry > <harry.van.haaren@intel.com> > Subject: [PATCH v3] dpif-netdev: fix vlan and ipv4 parsing in avx512 > > This commit fixes the minimum packet size for the vlan/ipv4/tcp traffic profile, > which was previously incorrectly set. > > This commit also disallows any fragmented IPv4 packets from being matched in > the optimized miniflow-extract, avoiding complexity of handling fragmented > packets and using scalar fallback instead. > The DF (don't fragment) bit is now ignored, and stripped from the resulting > miniflow. > > Fixes: aa85a25095 ("dpif-netdev/mfex: Add more AVX512 traffic profiles.") > > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com> > > --- > > Testing this patch becomes easier if the MFEX/DPIF patch by Amber here is > applied, as it ensures the AVX512 DPIF is active (and hence MFEX-autovalidator > actually executes in the datapath always, or the test gets skipped if the ISA is not > available). > https://patchwork.ozlabs.org/project/openvswitch/patch/20220131105149.147 > 1184-1-kumar.amber@intel.com/ > > v3: > - Rework AVX512 impl to be more generic, adding "strip_mask" to profile > - Use #define NC for 0xFF value generation in bitmask (Eelco) > - Use previous store method (not in separate function) (Eelco/Harry) > - Handle VLAN/Dot1Q appropriately to pass MFEX Autovalidation (Amber) > > v2: > - Fixup the "frag-offset" mask from incorrect value, to ignore DF bit (Eelco) > - The OVS_UNLIKELY() is added as the extra instructions/inline-func-call > was confusing the compiler here, resulting in slow code. By marking > the branch as unlikely, the code sequence generated is optimal again. > --- > lib/dpif-netdev-extract-avx512.c | 36 +++++++++++++++++++++++++++----- > 1 file changed, 31 insertions(+), 5 deletions(-) > > diff --git a/lib/dpif-netdev-extract-avx512.c b/lib/dpif-netdev-extract-avx512.c > index d23349482..c1c1fefb6 100644 > --- a/lib/dpif-netdev-extract-avx512.c > +++ b/lib/dpif-netdev-extract-avx512.c > @@ -157,7 +157,7 @@ _mm512_maskz_permutexvar_epi8_wrap(__mmask64 > kmask, __m512i idx, __m512i a) > 0, 0, 0, 0, /* Src IP */ \ > 0, 0, 0, 0, /* Dst IP */ > > -#define PATTERN_IPV4_MASK PATTERN_IPV4_GEN(0xFF, 0xFE, 0xFF, 0xFF) > +#define PATTERN_IPV4_MASK PATTERN_IPV4_GEN(0xFF, 0xBF, 0xFF, 0xFF) > #define PATTERN_IPV4_UDP PATTERN_IPV4_GEN(0x45, 0, 0, 0x11) #define > PATTERN_IPV4_TCP PATTERN_IPV4_GEN(0x45, 0, 0, 0x06) > > @@ -226,6 +226,25 @@ _mm512_maskz_permutexvar_epi8_wrap(__mmask64 > kmask, __m512i idx, __m512i a) #define PATTERN_DT1Q_IPV4_TCP_KMASK \ > (KMASK_ETHER | (KMASK_DT1Q << 16) | (KMASK_IPV4 << 24) | (KMASK_TCP > << 40)) > > +/* Miniflow Strip post-processing masks. > + * This allows unsetting specific bits from the resulting miniflow. It > +is used > + * for e.g. IPv4 where the "DF" bit is never pushed to the miniflow itself. > + * The NC define is for "No Change", allowing the bits to pass through. > + */ > +#define NC 0xFF > + > +#define PATTERN_STRIP_IPV4_MASK \ > + NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, \ > + NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, 0xBF, NC, NC, NC, \ > + NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, \ > + NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC > + > +#define PATTERN_STRIP_DOT1Q_IPV4_MASK \ > + NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, \ > + NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, \ > + NC, NC, NC, NC, 0xBF, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, \ > + NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC > + > /* This union allows initializing static data as u8, but easily loading it > * into AVX512 registers too. The union ensures proper alignment for the zmm. > */ > @@ -250,8 +269,9 @@ struct mfex_profile { > union mfex_data probe_mask; > union mfex_data probe_data; > > - /* Required for reshaping packet into miniflow. */ > + /* Required for reshaping packet into miniflow and post-processing > + it. */ > union mfex_data store_shuf; > + union mfex_data strip_mask; > __mmask64 store_kmsk; > > /* Constant data to set in mf.bits and dp_packet data on hit. */ @@ -319,6 > +339,7 @@ static const struct mfex_profile mfex_profiles[PROFILE_COUNT] = > .probe_data.u8_data = { PATTERN_ETHERTYPE_IPV4 PATTERN_IPV4_UDP}, > > .store_shuf.u8_data = { PATTERN_IPV4_UDP_SHUFFLE }, > + .strip_mask.u8_data = { PATTERN_STRIP_IPV4_MASK }, > .store_kmsk = PATTERN_IPV4_UDP_KMASK, > > .mf_bits = { 0x18a0000000000000, 0x0000000000040401}, @@ -341,6 > +362,7 @@ static const struct mfex_profile mfex_profiles[PROFILE_COUNT] = > }, > > .store_shuf.u8_data = { PATTERN_IPV4_TCP_SHUFFLE }, > + .strip_mask.u8_data = { PATTERN_STRIP_IPV4_MASK }, > .store_kmsk = PATTERN_IPV4_TCP_KMASK, > > .mf_bits = { 0x18a0000000000000, 0x0000000000044401}, @@ -359,6 > +381,7 @@ static const struct mfex_profile mfex_profiles[PROFILE_COUNT] = > }, > > .store_shuf.u8_data = { PATTERN_DT1Q_IPV4_UDP_SHUFFLE }, > + .strip_mask.u8_data = { PATTERN_STRIP_DOT1Q_IPV4_MASK }, > .store_kmsk = PATTERN_DT1Q_IPV4_UDP_KMASK, > > .mf_bits = { 0x38a0000000000000, 0x0000000000040401}, @@ -383,13 > +406,14 @@ static const struct mfex_profile mfex_profiles[PROFILE_COUNT] = > }, > > .store_shuf.u8_data = { PATTERN_DT1Q_IPV4_TCP_SHUFFLE }, > + .strip_mask.u8_data = { PATTERN_STRIP_DOT1Q_IPV4_MASK }, > .store_kmsk = PATTERN_DT1Q_IPV4_TCP_KMASK, > > .mf_bits = { 0x38a0000000000000, 0x0000000000044401}, > .dp_pkt_offs = { > 14, UINT16_MAX, 18, 38, > }, > - .dp_pkt_min_size = 46, > + .dp_pkt_min_size = 58, > }, > }; > > @@ -471,6 +495,7 @@ mfex_avx512_process(struct dp_packet_batch > *packets, > __m512i v_vals = _mm512_loadu_si512(&profile->probe_data); > __m512i v_mask = _mm512_loadu_si512(&profile->probe_mask); > __m512i v_shuf = _mm512_loadu_si512(&profile->store_shuf); > + __m512i v_strp = _mm512_loadu_si512(&profile->strip_mask); > > __mmask64 k_shuf = profile->store_kmsk; > __m128i v_bits = _mm_loadu_si128((void *) &profile->mf_bits); @@ -498,7 > +523,7 @@ mfex_avx512_process(struct dp_packet_batch *packets, > > __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) { > + if (OVS_UNLIKELY(k_cmp != UINT64_MAX)) { > continue; > } > > @@ -526,8 +551,9 @@ mfex_avx512_process(struct dp_packet_batch > *packets, > v_blk0 = _mm512_maskz_permutex2var_epi8_skx(k_shuf, v_pkt0, > v_shuf, v512_zeros); > } > - _mm512_storeu_si512(&blocks[2], v_blk0); > > + __m512i v_blk0_strip = _mm512_and_si512(v_blk0, v_strp); > + _mm512_storeu_si512(&blocks[2], v_blk0_strip); > > /* Perform "post-processing" per profile, handling details not easily > * handled in the above generic AVX512 code. Examples include TCP flag > -- > 2.25.1 Tested-by: Kumar Amber <kumar.amber@intel.com>
On 31 Jan 2022, at 14:54, Harry van Haaren wrote: > This commit fixes the minimum packet size for the vlan/ipv4/tcp > traffic profile, which was previously incorrectly set. > > This commit also disallows any fragmented IPv4 packets from being > matched in the optimized miniflow-extract, avoiding complexity of > handling fragmented packets and using scalar fallback instead. > The DF (don't fragment) bit is now ignored, and stripped from the > resulting miniflow. > > Fixes: aa85a25095 ("dpif-netdev/mfex: Add more AVX512 traffic profiles.") > > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com> > > --- Hi Harry, All the changes look good to me! But with one exception :( I borrowed an AVX machine and I get the “OVS-DPDK - MFEX Autovalidator Fuzzy” test to fail every run. The cause is these log messages being present in the system log: +2022-02-04T09:09:45.705Z|00002|ofproto_dpif_upcall(pmd-c58/id:69)|WARN|upcall: datapath reached the dynamic limit of 10000 flows. +2022-02-04T09:09:45.705Z|00003|ofproto_dpif_upcall(pmd-c58/id:69)|WARN|upcall: datapath reached the dynamic limit of 10000 flows. +2022-02-04T09:09:45.705Z|00004|ofproto_dpif_upcall(pmd-c58/id:69)|WARN|upcall: datapath reached the dynamic limit of 10000 flows. +2022-02-04T09:09:45.705Z|00005|ofproto_dpif_upcall(pmd-c58/id:69)|WARN|upcall: datapath reached the dynamic limit of 10000 flows. +2022-02-04T09:09:45.705Z|00006|ofproto_dpif_upcall(pmd-c58/id:69)|WARN|upcall: datapath reached the dynamic limit of 10000 flows. I think this has nothing to do with this specific change, and it can be easily fixed by adding this specific message to the OVS_VSWITCHD_STOP allowed logs for this specific test case. Guess you can either fix it in this patchset, or a separate one, as without the patch it’s also failing. Acked-by: Eelco Chaudron <echaudro@redhat.com> Thanks, Eelco
Hi Eelco, I have created a patch to add the warning to the system log http://patchwork.ozlabs.org/project/openvswitch/patch/20220207110008.2054074-1-kumar.amber@intel.com/ I am not able to reproduce the warning, it will be good if you can test it and let me know if something more needs to be Added here Regards Amber > -----Original Message----- > From: Eelco Chaudron <echaudro@redhat.com> > Sent: Friday, February 4, 2022 2:54 PM > To: Van Haaren, Harry <harry.van.haaren@intel.com> > Cc: ovs-dev@openvswitch.org; Ferriter, Cian <cian.ferriter@intel.com>; Stokes, > Ian <ian.stokes@intel.com>; i.maximets@ovn.org; Amber, Kumar > <kumar.amber@intel.com> > Subject: Re: [PATCH v3] dpif-netdev: fix vlan and ipv4 parsing in avx512 > > > > On 31 Jan 2022, at 14:54, Harry van Haaren wrote: > > > This commit fixes the minimum packet size for the vlan/ipv4/tcp > > traffic profile, which was previously incorrectly set. > > > > This commit also disallows any fragmented IPv4 packets from being > > matched in the optimized miniflow-extract, avoiding complexity of > > handling fragmented packets and using scalar fallback instead. > > The DF (don't fragment) bit is now ignored, and stripped from the > > resulting miniflow. > > > > Fixes: aa85a25095 ("dpif-netdev/mfex: Add more AVX512 traffic > > profiles.") > > > > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com> > > > > --- > > Hi Harry, > > All the changes look good to me! > > But with one exception :( I borrowed an AVX machine and I get the “OVS-DPDK - > MFEX Autovalidator Fuzzy” test to fail every run. > The cause is these log messages being present in the system log: > > > +2022-02-04T09:09:45.705Z|00002|ofproto_dpif_upcall(pmd- > c58/id:69)|WARN|upcall: datapath reached the dynamic limit of 10000 flows. > +2022-02-04T09:09:45.705Z|00003|ofproto_dpif_upcall(pmd- > c58/id:69)|WARN|upcall: datapath reached the dynamic limit of 10000 flows. > +2022-02-04T09:09:45.705Z|00004|ofproto_dpif_upcall(pmd- > c58/id:69)|WARN|upcall: datapath reached the dynamic limit of 10000 flows. > +2022-02-04T09:09:45.705Z|00005|ofproto_dpif_upcall(pmd- > c58/id:69)|WARN|upcall: datapath reached the dynamic limit of 10000 flows. > +2022-02-04T09:09:45.705Z|00006|ofproto_dpif_upcall(pmd- > c58/id:69)|WARN|upcall: datapath reached the dynamic limit of 10000 flows. > > > I think this has nothing to do with this specific change, and it can be easily fixed > by adding this specific message to the OVS_VSWITCHD_STOP allowed logs for > this specific test case. > > Guess you can either fix it in this patchset, or a separate one, as without the > patch it’s also failing. > > Acked-by: Eelco Chaudron <echaudro@redhat.com> > > Thanks, > > Eelco >
On 7 Feb 2022, at 12:41, Amber, Kumar wrote: > Hi Eelco, > > I have created a patch to add the warning to the system log > http://patchwork.ozlabs.org/project/openvswitch/patch/20220207110008.2054074-1-kumar.amber@intel.com/ > > I am not able to reproduce the warning, it will be good if you can test it and let me know if something more needs to be > Added here I lost my AVX system, but the changes look good, so I acked it as it. //Eelco >> -----Original Message----- >> From: Eelco Chaudron <echaudro@redhat.com> >> Sent: Friday, February 4, 2022 2:54 PM >> To: Van Haaren, Harry <harry.van.haaren@intel.com> >> Cc: ovs-dev@openvswitch.org; Ferriter, Cian <cian.ferriter@intel.com>; Stokes, >> Ian <ian.stokes@intel.com>; i.maximets@ovn.org; Amber, Kumar >> <kumar.amber@intel.com> >> Subject: Re: [PATCH v3] dpif-netdev: fix vlan and ipv4 parsing in avx512 >> >> >> >> On 31 Jan 2022, at 14:54, Harry van Haaren wrote: >> >>> This commit fixes the minimum packet size for the vlan/ipv4/tcp >>> traffic profile, which was previously incorrectly set. >>> >>> This commit also disallows any fragmented IPv4 packets from being >>> matched in the optimized miniflow-extract, avoiding complexity of >>> handling fragmented packets and using scalar fallback instead. >>> The DF (don't fragment) bit is now ignored, and stripped from the >>> resulting miniflow. >>> >>> Fixes: aa85a25095 ("dpif-netdev/mfex: Add more AVX512 traffic >>> profiles.") >>> >>> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com> >>> >>> --- >> >> Hi Harry, >> >> All the changes look good to me! >> >> But with one exception :( I borrowed an AVX machine and I get the “OVS-DPDK - >> MFEX Autovalidator Fuzzy” test to fail every run. >> The cause is these log messages being present in the system log: >> >> >> +2022-02-04T09:09:45.705Z|00002|ofproto_dpif_upcall(pmd- >> c58/id:69)|WARN|upcall: datapath reached the dynamic limit of 10000 flows. >> +2022-02-04T09:09:45.705Z|00003|ofproto_dpif_upcall(pmd- >> c58/id:69)|WARN|upcall: datapath reached the dynamic limit of 10000 flows. >> +2022-02-04T09:09:45.705Z|00004|ofproto_dpif_upcall(pmd- >> c58/id:69)|WARN|upcall: datapath reached the dynamic limit of 10000 flows. >> +2022-02-04T09:09:45.705Z|00005|ofproto_dpif_upcall(pmd- >> c58/id:69)|WARN|upcall: datapath reached the dynamic limit of 10000 flows. >> +2022-02-04T09:09:45.705Z|00006|ofproto_dpif_upcall(pmd- >> c58/id:69)|WARN|upcall: datapath reached the dynamic limit of 10000 flows. >> >> >> I think this has nothing to do with this specific change, and it can be easily fixed >> by adding this specific message to the OVS_VSWITCHD_STOP allowed logs for >> this specific test case. >> >> Guess you can either fix it in this patchset, or a separate one, as without the >> patch it’s also failing. >> >> Acked-by: Eelco Chaudron <echaudro@redhat.com> >> >> Thanks, >> >> Eelco >>
Top posting summary/status of this patch;
1) Code Acked by Eelco, Tested-by Amber
2) Unit test Issue reported by Eelco below is being fixed in Amber's patch/thread here
https://patchwork.ozlabs.org/project/openvswitch/patch/20220207110008.2054074-1-kumar.amber@intel.com/
Patch should apply cleanly to 2.17 and 2.16. If required, I can backport a 2.16 version.
I would like to see this getting merged soon, as the 2.17 release is planned for next week.
> -----Original Message-----
> From: Eelco Chaudron <echaudro@redhat.com>
> Sent: Monday, February 7, 2022 11:44 AM
> To: Amber, Kumar <kumar.amber@intel.com>
> Cc: ovs-dev@openvswitch.org; Ferriter, Cian <cian.ferriter@intel.com>; Stokes,
> Ian <ian.stokes@intel.com>; i.maximets@ovn.org; Van Haaren, Harry
> <harry.van.haaren@intel.com>
> Subject: Re: [PATCH v3] dpif-netdev: fix vlan and ipv4 parsing in avx512
>
>
>
> On 7 Feb 2022, at 12:41, Amber, Kumar wrote:
>
> > Hi Eelco,
> >
> > I have created a patch to add the warning to the system log
> >
> http://patchwork.ozlabs.org/project/openvswitch/patch/20220207110008.20540
> 74-1-kumar.amber@intel.com/
> >
> > I am not able to reproduce the warning, it will be good if you can test it and let
> me know if something more needs to be
> > Added here
>
> I lost my AVX system, but the changes look good, so I acked it as it.
>
> //Eelco
>
>
> >> -----Original Message-----
> >> From: Eelco Chaudron <echaudro@redhat.com>
> >> Sent: Friday, February 4, 2022 2:54 PM
> >> To: Van Haaren, Harry <harry.van.haaren@intel.com>
> >> Cc: ovs-dev@openvswitch.org; Ferriter, Cian <cian.ferriter@intel.com>;
> Stokes,
> >> Ian <ian.stokes@intel.com>; i.maximets@ovn.org; Amber, Kumar
> >> <kumar.amber@intel.com>
> >> Subject: Re: [PATCH v3] dpif-netdev: fix vlan and ipv4 parsing in avx512
> >>
> >>
> >>
> >> On 31 Jan 2022, at 14:54, Harry van Haaren wrote:
> >>
> >>> This commit fixes the minimum packet size for the vlan/ipv4/tcp
> >>> traffic profile, which was previously incorrectly set.
> >>>
> >>> This commit also disallows any fragmented IPv4 packets from being
> >>> matched in the optimized miniflow-extract, avoiding complexity of
> >>> handling fragmented packets and using scalar fallback instead.
> >>> The DF (don't fragment) bit is now ignored, and stripped from the
> >>> resulting miniflow.
> >>>
> >>> Fixes: aa85a25095 ("dpif-netdev/mfex: Add more AVX512 traffic
> >>> profiles.")
> >>>
> >>> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> >>>
> >>> ---
> >>
> >> Hi Harry,
> >>
> >> All the changes look good to me!
> >>
> >> But with one exception :( I borrowed an AVX machine and I get the “OVS-
> DPDK -
> >> MFEX Autovalidator Fuzzy” test to fail every run.
> >> The cause is these log messages being present in the system log:
> >>
> >>
> >> +2022-02-04T09:09:45.705Z|00002|ofproto_dpif_upcall(pmd-
> >> c58/id:69)|WARN|upcall: datapath reached the dynamic limit of 10000 flows.
> >> +2022-02-04T09:09:45.705Z|00003|ofproto_dpif_upcall(pmd-
> >> c58/id:69)|WARN|upcall: datapath reached the dynamic limit of 10000 flows.
> >> +2022-02-04T09:09:45.705Z|00004|ofproto_dpif_upcall(pmd-
> >> c58/id:69)|WARN|upcall: datapath reached the dynamic limit of 10000 flows.
> >> +2022-02-04T09:09:45.705Z|00005|ofproto_dpif_upcall(pmd-
> >> c58/id:69)|WARN|upcall: datapath reached the dynamic limit of 10000 flows.
> >> +2022-02-04T09:09:45.705Z|00006|ofproto_dpif_upcall(pmd-
> >> c58/id:69)|WARN|upcall: datapath reached the dynamic limit of 10000 flows.
> >>
> >>
> >> I think this has nothing to do with this specific change, and it can be easily fixed
> >> by adding this specific message to the OVS_VSWITCHD_STOP allowed logs for
> >> this specific test case.
> >>
> >> Guess you can either fix it in this patchset, or a separate one, as without the
> >> patch it’s also failing.
> >>
> >> Acked-by: Eelco Chaudron <echaudro@redhat.com>
> >>
> >> Thanks,
> >>
> >> Eelco
> >>
> -----Original Message----- > From: Van Haaren, Harry <harry.van.haaren@intel.com> > Sent: Tuesday, February 8, 2022 10:17 AM > To: Eelco Chaudron <echaudro@redhat.com>; Amber, Kumar > <kumar.amber@intel.com> > Cc: ovs-dev@openvswitch.org; Ferriter, Cian <cian.ferriter@intel.com>; Stokes, > Ian <ian.stokes@intel.com>; i.maximets@ovn.org > Subject: RE: [PATCH v3] dpif-netdev: fix vlan and ipv4 parsing in avx512 > > Top posting summary/status of this patch; > 1) Code Acked by Eelco, Tested-by Amber > 2) Unit test Issue reported by Eelco below is being fixed in Amber's patch/thread > here > > https://patchwork.ozlabs.org/project/openvswitch/patch/20220207110008.205 > 4074-1-kumar.amber@intel.com/ > > Patch should apply cleanly to 2.17 and 2.16. If required, I can backport a 2.16 > version. > > I would like to see this getting merged soon, as the 2.17 release is planned for > next week. Thanks all for the reviews and testing on this, I've pushed this patch to master, 2.17 and 2.16. Thanks Ian > > > > -----Original Message----- > > From: Eelco Chaudron <echaudro@redhat.com> > > Sent: Monday, February 7, 2022 11:44 AM > > To: Amber, Kumar <kumar.amber@intel.com> > > Cc: ovs-dev@openvswitch.org; Ferriter, Cian <cian.ferriter@intel.com>; > Stokes, > > Ian <ian.stokes@intel.com>; i.maximets@ovn.org; Van Haaren, Harry > > <harry.van.haaren@intel.com> > > Subject: Re: [PATCH v3] dpif-netdev: fix vlan and ipv4 parsing in avx512 > > > > > > > > On 7 Feb 2022, at 12:41, Amber, Kumar wrote: > > > > > Hi Eelco, > > > > > > I have created a patch to add the warning to the system log > > > > > > http://patchwork.ozlabs.org/project/openvswitch/patch/20220207110008.2054 > 0 > > 74-1-kumar.amber@intel.com/ > > > > > > I am not able to reproduce the warning, it will be good if you can test it and > let > > me know if something more needs to be > > > Added here > > > > I lost my AVX system, but the changes look good, so I acked it as it. > > > > //Eelco > > > > > > >> -----Original Message----- > > >> From: Eelco Chaudron <echaudro@redhat.com> > > >> Sent: Friday, February 4, 2022 2:54 PM > > >> To: Van Haaren, Harry <harry.van.haaren@intel.com> > > >> Cc: ovs-dev@openvswitch.org; Ferriter, Cian <cian.ferriter@intel.com>; > > Stokes, > > >> Ian <ian.stokes@intel.com>; i.maximets@ovn.org; Amber, Kumar > > >> <kumar.amber@intel.com> > > >> Subject: Re: [PATCH v3] dpif-netdev: fix vlan and ipv4 parsing in avx512 > > >> > > >> > > >> > > >> On 31 Jan 2022, at 14:54, Harry van Haaren wrote: > > >> > > >>> This commit fixes the minimum packet size for the vlan/ipv4/tcp > > >>> traffic profile, which was previously incorrectly set. > > >>> > > >>> This commit also disallows any fragmented IPv4 packets from being > > >>> matched in the optimized miniflow-extract, avoiding complexity of > > >>> handling fragmented packets and using scalar fallback instead. > > >>> The DF (don't fragment) bit is now ignored, and stripped from the > > >>> resulting miniflow. > > >>> > > >>> Fixes: aa85a25095 ("dpif-netdev/mfex: Add more AVX512 traffic > > >>> profiles.") > > >>> > > >>> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com> > > >>> > > >>> --- > > >> > > >> Hi Harry, > > >> > > >> All the changes look good to me! > > >> > > >> But with one exception :( I borrowed an AVX machine and I get the “OVS- > > DPDK - > > >> MFEX Autovalidator Fuzzy” test to fail every run. > > >> The cause is these log messages being present in the system log: > > >> > > >> > > >> +2022-02-04T09:09:45.705Z|00002|ofproto_dpif_upcall(pmd- > > >> c58/id:69)|WARN|upcall: datapath reached the dynamic limit of 10000 > flows. > > >> +2022-02-04T09:09:45.705Z|00003|ofproto_dpif_upcall(pmd- > > >> c58/id:69)|WARN|upcall: datapath reached the dynamic limit of 10000 > flows. > > >> +2022-02-04T09:09:45.705Z|00004|ofproto_dpif_upcall(pmd- > > >> c58/id:69)|WARN|upcall: datapath reached the dynamic limit of 10000 > flows. > > >> +2022-02-04T09:09:45.705Z|00005|ofproto_dpif_upcall(pmd- > > >> c58/id:69)|WARN|upcall: datapath reached the dynamic limit of 10000 > flows. > > >> +2022-02-04T09:09:45.705Z|00006|ofproto_dpif_upcall(pmd- > > >> c58/id:69)|WARN|upcall: datapath reached the dynamic limit of 10000 > flows. > > >> > > >> > > >> I think this has nothing to do with this specific change, and it can be easily > fixed > > >> by adding this specific message to the OVS_VSWITCHD_STOP allowed logs > for > > >> this specific test case. > > >> > > >> Guess you can either fix it in this patchset, or a separate one, as without the > > >> patch it’s also failing. > > >> > > >> Acked-by: Eelco Chaudron <echaudro@redhat.com> > > >> > > >> Thanks, > > >> > > >> Eelco > > >> >
diff --git a/lib/dpif-netdev-extract-avx512.c b/lib/dpif-netdev-extract-avx512.c index d23349482..c1c1fefb6 100644 --- a/lib/dpif-netdev-extract-avx512.c +++ b/lib/dpif-netdev-extract-avx512.c @@ -157,7 +157,7 @@ _mm512_maskz_permutexvar_epi8_wrap(__mmask64 kmask, __m512i idx, __m512i a) 0, 0, 0, 0, /* Src IP */ \ 0, 0, 0, 0, /* Dst IP */ -#define PATTERN_IPV4_MASK PATTERN_IPV4_GEN(0xFF, 0xFE, 0xFF, 0xFF) +#define PATTERN_IPV4_MASK PATTERN_IPV4_GEN(0xFF, 0xBF, 0xFF, 0xFF) #define PATTERN_IPV4_UDP PATTERN_IPV4_GEN(0x45, 0, 0, 0x11) #define PATTERN_IPV4_TCP PATTERN_IPV4_GEN(0x45, 0, 0, 0x06) @@ -226,6 +226,25 @@ _mm512_maskz_permutexvar_epi8_wrap(__mmask64 kmask, __m512i idx, __m512i a) #define PATTERN_DT1Q_IPV4_TCP_KMASK \ (KMASK_ETHER | (KMASK_DT1Q << 16) | (KMASK_IPV4 << 24) | (KMASK_TCP << 40)) +/* Miniflow Strip post-processing masks. + * This allows unsetting specific bits from the resulting miniflow. It is used + * for e.g. IPv4 where the "DF" bit is never pushed to the miniflow itself. + * The NC define is for "No Change", allowing the bits to pass through. + */ +#define NC 0xFF + +#define PATTERN_STRIP_IPV4_MASK \ + NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, \ + NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, 0xBF, NC, NC, NC, \ + NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, \ + NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC + +#define PATTERN_STRIP_DOT1Q_IPV4_MASK \ + NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, \ + NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, \ + NC, NC, NC, NC, 0xBF, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, \ + NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC, NC + /* This union allows initializing static data as u8, but easily loading it * into AVX512 registers too. The union ensures proper alignment for the zmm. */ @@ -250,8 +269,9 @@ struct mfex_profile { union mfex_data probe_mask; union mfex_data probe_data; - /* Required for reshaping packet into miniflow. */ + /* Required for reshaping packet into miniflow and post-processing it. */ union mfex_data store_shuf; + union mfex_data strip_mask; __mmask64 store_kmsk; /* Constant data to set in mf.bits and dp_packet data on hit. */ @@ -319,6 +339,7 @@ static const struct mfex_profile mfex_profiles[PROFILE_COUNT] = .probe_data.u8_data = { PATTERN_ETHERTYPE_IPV4 PATTERN_IPV4_UDP}, .store_shuf.u8_data = { PATTERN_IPV4_UDP_SHUFFLE }, + .strip_mask.u8_data = { PATTERN_STRIP_IPV4_MASK }, .store_kmsk = PATTERN_IPV4_UDP_KMASK, .mf_bits = { 0x18a0000000000000, 0x0000000000040401}, @@ -341,6 +362,7 @@ static const struct mfex_profile mfex_profiles[PROFILE_COUNT] = }, .store_shuf.u8_data = { PATTERN_IPV4_TCP_SHUFFLE }, + .strip_mask.u8_data = { PATTERN_STRIP_IPV4_MASK }, .store_kmsk = PATTERN_IPV4_TCP_KMASK, .mf_bits = { 0x18a0000000000000, 0x0000000000044401}, @@ -359,6 +381,7 @@ static const struct mfex_profile mfex_profiles[PROFILE_COUNT] = }, .store_shuf.u8_data = { PATTERN_DT1Q_IPV4_UDP_SHUFFLE }, + .strip_mask.u8_data = { PATTERN_STRIP_DOT1Q_IPV4_MASK }, .store_kmsk = PATTERN_DT1Q_IPV4_UDP_KMASK, .mf_bits = { 0x38a0000000000000, 0x0000000000040401}, @@ -383,13 +406,14 @@ static const struct mfex_profile mfex_profiles[PROFILE_COUNT] = }, .store_shuf.u8_data = { PATTERN_DT1Q_IPV4_TCP_SHUFFLE }, + .strip_mask.u8_data = { PATTERN_STRIP_DOT1Q_IPV4_MASK }, .store_kmsk = PATTERN_DT1Q_IPV4_TCP_KMASK, .mf_bits = { 0x38a0000000000000, 0x0000000000044401}, .dp_pkt_offs = { 14, UINT16_MAX, 18, 38, }, - .dp_pkt_min_size = 46, + .dp_pkt_min_size = 58, }, }; @@ -471,6 +495,7 @@ mfex_avx512_process(struct dp_packet_batch *packets, __m512i v_vals = _mm512_loadu_si512(&profile->probe_data); __m512i v_mask = _mm512_loadu_si512(&profile->probe_mask); __m512i v_shuf = _mm512_loadu_si512(&profile->store_shuf); + __m512i v_strp = _mm512_loadu_si512(&profile->strip_mask); __mmask64 k_shuf = profile->store_kmsk; __m128i v_bits = _mm_loadu_si128((void *) &profile->mf_bits); @@ -498,7 +523,7 @@ mfex_avx512_process(struct dp_packet_batch *packets, __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) { + if (OVS_UNLIKELY(k_cmp != UINT64_MAX)) { continue; } @@ -526,8 +551,9 @@ mfex_avx512_process(struct dp_packet_batch *packets, v_blk0 = _mm512_maskz_permutex2var_epi8_skx(k_shuf, v_pkt0, v_shuf, v512_zeros); } - _mm512_storeu_si512(&blocks[2], v_blk0); + __m512i v_blk0_strip = _mm512_and_si512(v_blk0, v_strp); + _mm512_storeu_si512(&blocks[2], v_blk0_strip); /* Perform "post-processing" per profile, handling details not easily * handled in the above generic AVX512 code. Examples include TCP flag
This commit fixes the minimum packet size for the vlan/ipv4/tcp traffic profile, which was previously incorrectly set. This commit also disallows any fragmented IPv4 packets from being matched in the optimized miniflow-extract, avoiding complexity of handling fragmented packets and using scalar fallback instead. The DF (don't fragment) bit is now ignored, and stripped from the resulting miniflow. Fixes: aa85a25095 ("dpif-netdev/mfex: Add more AVX512 traffic profiles.") Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com> --- Testing this patch becomes easier if the MFEX/DPIF patch by Amber here is applied, as it ensures the AVX512 DPIF is active (and hence MFEX-autovalidator actually executes in the datapath always, or the test gets skipped if the ISA is not available). https://patchwork.ozlabs.org/project/openvswitch/patch/20220131105149.1471184-1-kumar.amber@intel.com/ v3: - Rework AVX512 impl to be more generic, adding "strip_mask" to profile - Use #define NC for 0xFF value generation in bitmask (Eelco) - Use previous store method (not in separate function) (Eelco/Harry) - Handle VLAN/Dot1Q appropriately to pass MFEX Autovalidation (Amber) v2: - Fixup the "frag-offset" mask from incorrect value, to ignore DF bit (Eelco) - The OVS_UNLIKELY() is added as the extra instructions/inline-func-call was confusing the compiler here, resulting in slow code. By marking the branch as unlikely, the code sequence generated is optimal again. --- lib/dpif-netdev-extract-avx512.c | 36 +++++++++++++++++++++++++++----- 1 file changed, 31 insertions(+), 5 deletions(-)