| Message ID | 20220112162353.1468468-1-harry.van.haaren@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] dpif-netdev: fix handling of 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 |
On 12 Jan 2022, at 17:23, 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. > > Fixes: aa85a25095 ("dpif-netdev/mfex: Add more AVX512 traffic profiles.") > > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com> > > --- > > This patch should be applied to 2.16 as well. I expect it applies cleanly, but > volunteer to rebase/fixup on 2.16 release and send new patch if required. > > --- > > lib/dpif-netdev-extract-avx512.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/lib/dpif-netdev-extract-avx512.c b/lib/dpif-netdev-extract-avx512.c > index d23349482..7b21a3af9 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, 0xFF, 0xFF, 0xFF) I assume the original idea was not to include the may fragment bit, which I think should be fine to ignore. But the previous mask was 0xFE was masking of bit in the “Fragment offset”, so I think setting this to 0xDF would accomplish making sure this is not a fragment (or reserved bit set). > #define PATTERN_IPV4_UDP PATTERN_IPV4_GEN(0x45, 0, 0, 0x11) > #define PATTERN_IPV4_TCP PATTERN_IPV4_GEN(0x45, 0, 0, 0x06) > > @@ -389,7 +389,7 @@ static const struct mfex_profile mfex_profiles[PROFILE_COUNT] = > .dp_pkt_offs = { > 14, UINT16_MAX, 18, 38, > }, > - .dp_pkt_min_size = 46, > + .dp_pkt_min_size = 58, ACK > }, > }; > > -- > 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: Friday, January 14, 2022 10:03 AM > 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: fix handling of vlan and ipv4 parsing > in avx512 <snip patch contents> > > -#define PATTERN_IPV4_MASK PATTERN_IPV4_GEN(0xFF, 0xFE, 0xFF, 0xFF) > > +#define PATTERN_IPV4_MASK PATTERN_IPV4_GEN(0xFF, 0xFF, 0xFF, 0xFF) > > I assume the original idea was not to include the may fragment bit, which I think > should be fine to ignore. Yes - good point. > But the previous mask was 0xFE was masking of bit in the “Fragment offset”, so I > think setting this to 0xDF would accomplish making sure this is not a fragment (or > reserved bit set). Yes - will validate and respin a v2. > > #define PATTERN_IPV4_UDP PATTERN_IPV4_GEN(0x45, 0, 0, 0x11) > > #define PATTERN_IPV4_TCP PATTERN_IPV4_GEN(0x45, 0, 0, 0x06) > > > > @@ -389,7 +389,7 @@ static const struct mfex_profile > mfex_profiles[PROFILE_COUNT] = > > .dp_pkt_offs = { > > 14, UINT16_MAX, 18, 38, > > }, > > - .dp_pkt_min_size = 46, > > + .dp_pkt_min_size = 58, > > ACK Thanks for review, -Harry
> -----Original Message----- > From: Van Haaren, Harry > Sent: Monday, January 24, 2022 3:43 PM > To: Eelco Chaudron <echaudro@redhat.com> > Cc: ovs-dev@openvswitch.org; i.maximets@ovn.org; Ferriter, Cian > <Cian.Ferriter@intel.com> > Subject: RE: [ovs-dev] [PATCH] dpif-netdev: fix handling of vlan and ipv4 parsing > in avx512 > > > -----Original Message----- > > From: Eelco Chaudron <echaudro@redhat.com> > > Sent: Friday, January 14, 2022 10:03 AM > > 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: fix handling of vlan and ipv4 > parsing > > in avx512 > > <snip patch contents> > > > > -#define PATTERN_IPV4_MASK PATTERN_IPV4_GEN(0xFF, 0xFE, 0xFF, 0xFF) > > > +#define PATTERN_IPV4_MASK PATTERN_IPV4_GEN(0xFF, 0xFF, 0xFF, 0xFF) > > > > I assume the original idea was not to include the may fragment bit, which I think > > should be fine to ignore. > > Yes - good point. > > > But the previous mask was 0xFE was masking of bit in the “Fragment offset”, > > so I think setting this to 0xDF would accomplish making sure this is not a > > fragment (or reserved bit set). > > Yes - will validate and respin a v2. The idea of masking away the "Don’t Frag" bit (DF) is a good one. Identifying the correct bit is unfortunately more complex than one would initially think. The reason is that BE and LE, as well as "written formats" tend to disagree on exactly how things work. The suggested 0xDF above isn't correct. For example, the Wikipedia article mentions bits, but doesn't state BE/LE, or which direction to count the bits in (its left to right, aka MSB to lsb): https://en.wikipedia.org/wiki/IPv4#Flags Looking at the OVS/FreeBSD source gives a better idea, as it uses u16 values, and 0x4000 style hex values: https://www.leidinger.net/FreeBSD/dox/netinet/html/da/d2f/ip_8h_source.html#l00065 All in all, the 0xFF initially proposed will not handle packets with the DF bit set, so is not ideal. The correct hex-value to use to mask-away the "DF" bit is 0xBF: bin 0100 0000 : Location of the DF bit in the u16 frag-offset in the IPv4 header (network byte order) bin 1011 1111 : Mask value to ignore that DF bit. Binary 1011 1111 = hex 0xBF. With the DF bit now appropriately ignored, we can *match* against an IPv4 packets, however the DF bit is actually not *tracked* by the miniflow data-structure at all! (The scalar code extracts the nw_frag info from the frag_offset field, but masks away DF bit, and never pushes it into miniflow). The avx512 code must emulate that behaviour; mfex-autovalidator immediately pin-pointed the issue. The "fixup" for IPv4 DF bit is not applicable to other protocols[1], so we do not want to add it to the generic MFEX processing infrastructure. A simple bitwise AND of the blocks is all that is required, and can be done in the IPv4 parts of the switch on the protocol stack. Patch to implement & handle that DF bit-stripping on the way. -Harry [1] IPv6 Protocol Additions https://patchwork.ozlabs.org/project/openvswitch/cover/20211207110425.3873101-1-kumar.amber@intel.com/ <snip remainder of patch which didn't have new comments>
diff --git a/lib/dpif-netdev-extract-avx512.c b/lib/dpif-netdev-extract-avx512.c index d23349482..7b21a3af9 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, 0xFF, 0xFF, 0xFF) #define PATTERN_IPV4_UDP PATTERN_IPV4_GEN(0x45, 0, 0, 0x11) #define PATTERN_IPV4_TCP PATTERN_IPV4_GEN(0x45, 0, 0, 0x06) @@ -389,7 +389,7 @@ static const struct mfex_profile mfex_profiles[PROFILE_COUNT] = .dp_pkt_offs = { 14, UINT16_MAX, 18, 38, }, - .dp_pkt_min_size = 46, + .dp_pkt_min_size = 58, }, };
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. Fixes: aa85a25095 ("dpif-netdev/mfex: Add more AVX512 traffic profiles.") Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com> --- This patch should be applied to 2.16 as well. I expect it applies cleanly, but volunteer to rebase/fixup on 2.16 release and send new patch if required. --- lib/dpif-netdev-extract-avx512.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)