Hi Cian,
Please find the comments inline.
> > /* Do a batch minfilow extract into keys. */
> > - /* Do a batch minfilow extract into keys, but only for outer packets.
> > */
>
> In the earlier DPIF part of this patchset, I guess you add the above comment
> line that you are removing here. But when you add it, I don't think it should
> be duplicating the line above. Just add the ", but only for outer packets."
> part
> in the earlier patchset and remove it here, rather than adding a whole line
> then removing in a later patch.
>
Fixed in earlier patches.
> > uint32_t mf_mask = 0;
> > - if (recirc_depth == 0) {
> > - miniflow_extract_func mfex_func;
> > - atomic_read_relaxed(&pmd->miniflow_extract_opt, &mfex_func);
> > - if (mfex_func) {
> > - mf_mask = mfex_func(packets, keys, batch_size, in_port, pmd,
> > + miniflow_extract_func mfex_func;
> > + atomic_read_relaxed(&pmd->miniflow_extract_opt, &mfex_func);
> > + miniflow_extract_func mfex_inner_func;
> > + atomic_read_relaxed(&pmd->miniflow_extract_inner_opt,
> &mfex_inner_func);
> > + if (md_is_valid && mfex_inner_func) {
> > + mf_mask = mfex_inner_func(packets, keys, batch_size, in_port, pmd,
> > + md_is_valid);
> > + } else if (!md_is_valid && mfex_func) {
> > + mf_mask = mfex_func(packets, keys, batch_size, in_port, pmd,
> > md_is_valid);
>
> Align the above line with the 'p' in 'packets' from 2 lines above.
>
Done.
> > - }
> > }
> >
> > uint32_t iter = lookup_pkts_bitmask; diff --git
> > a/lib/dpif-netdev-extract-avx512.c b/lib/dpif-netdev-extract-avx512.c
> > index 833e9bd31..4c62bd911 100644
> > --- a/lib/dpif-netdev-extract-avx512.c
> > +++ b/lib/dpif-netdev-extract-avx512.c
> > @@ -360,6 +360,53 @@
> _mm512_maskz_permutexvar_epi8_selector(__mmask64 k_shuf, __m512i
> v_shuf,
> > MF_WORD(ipv6_dst, 2) | MF_BIT(tp_src) |
> > MF_BIT(tp_dst))
> > #define MF_IPV6_TCP (MF_IPV6_UDP | MF_BIT(tcp_flags) |
> MF_BIT(arp_tha.ea[2]))
> >
> > +#define MF_TUNNEL MF_WORD(tunnel, offsetof(struct flow_tnl,
> metadata) / 8)
> > +
> > +#define MF_ETH_TUNNEL (MF_TUNNEL | MF_ETH) #define
> MF_ETH_VLAN_TUNNEL
> > +(MF_TUNNEL | MF_ETH_VLAN)
> > +
> > +/* Block offsets represents the offsets into the blocks array of
> > +miniflow
> > + * and are derived experimentally. Scalar miniflow parses the header
> > + * in a fixed order and sequentially in a dynamic fashion thus
> > +incrementing
> > + * pointer and copying data is enough but in AVX512 since the headers
> > +are
> > + * parsed using pre-defined masks we need these magic offsets to
> > +write
> > + * some of the data items at the correct loaction in the blocks array
> > + * using below magic numbers.
> > + */
> > +#define BLK_META_DATA_OFFS 9
>
> We could use something like the below instead of hardcoding 9, right?
> offsetof(struct flow_tnl, metadata) / sizeof(uint64_t)
>
> That is the number of words that the scalar miniflow_extract() passes to
> miniflow_push_words() for the tunnel metadata.
>
> > +#define BLK_IPv4_TCP_FLAG 6
> > +#define BLK_VLAN_IPv4_TCP_FLAG 7
> > +#define BLK_VLAN_PCP 4
> > +#define BLK_IPv6_HDR_OFFS 8
> > +#define BLK_VLAN_IPv6_HDR_OFFS 9
> > +#define BLK_IPv6_TCP_FLAG 9
> > +#define BLK_VLAN_IPv6_TCP_FLAG 10
> > +#define BLK_L4_UDP_OFFS 9
> > +#define BLK_L4_TCP_OFFS 10
> > +#define BLK_VLAN_L4_UDP_OFFS 10
> > +#define BLK_VLAN_L4_TCP_OFFS 11
>
>
> I spent some time thinking about these #defines and whether we can
> generate them in a more dynamic and robust way like with other #defines in
> the file. I think it's tricky since they aren't as straight forward as
> figuring out
> "sizeof()" since the scalar miniflow_extract() pushes some but maybe not all
> of a protocol header. miniflow_extract() also calls miniflow_pad_to_64() for
> some protocol headers. Maybe we could build up the values using individual
> header #defines, like this:
> #define BLK_ETH_HEADER 2
> #define BLK_IPv4_HEADER 2
> #define BLK_TCP_FLAG 2
>
> #define BLK_IPv4_TCP_FLAG (BLK_ETH_HEADER + BLK_IPv4_HEADER +
> BLK_TCP_FLAG)
>
> This might make it a little more clear where the values are coming from.
>
> We could make the #defines a little more related to the protocol headers
> with something like this:
> #define BLK_ETH_HEADER ROUND_UP(sizeof(struct eth_header),
> 8)/sizeof(uint64_t)
>
> This should give the value 2 as well, but shows where it's coming from since
> the ROUND_UP is essentially what the miniflow_pad_to_64() ends up doing.
>
> I'm not sure if this would work for all headers though.
>
> Also, maybe counting the miniflow bits field which we know ahead of time
> for each MFEX impl could give us these values. I'll investigate this a bit
> more
> to see if there are any better solutions.
>
> Let me know if you have any thoughts on this.
>
I have removed the magic bits as much I can, and the patches builds on the
offsets using packet
Header lengths .
> > +
> > +/* Below Offsets simply shifts the offsets by 9 blocks as
> > + * in the tunneling case the first 9 blocks are reserved and
> > + * written with the outer tunnel data.
> > + */
> > +#define BLK_TUN_IPv6_HDR_OFFS (BLK_IPv6_HDR_OFFS +
> BLK_META_DATA_OFFS)
> > +#define BLK_TUN_VLAN_IPv6_HDR_OFFS (BLK_VLAN_IPv6_HDR_OFFS + \
> > + BLK_META_DATA_OFFS)
> > +#define BLK_TUN_IPv6_TCP_FLAG (BLK_IPv6_TCP_FLAG +
> BLK_META_DATA_OFFS)
> > +#define BLK_TUN_VLAN_IPv6_TCP_FLAG (BLK_VLAN_IPv6_TCP_FLAG + \
> > + BLK_META_DATA_OFFS)
> > +#define BLK_TUN_L4_UDP_OFFS (BLK_L4_UDP_OFFS +
> BLK_META_DATA_OFFS)
> > +#define BLK_TUN_L4_TCP_OFFS (BLK_L4_TCP_OFFS +
> BLK_META_DATA_OFFS)
> > +#define BLK_TUN_VLAN_L4_UDP_OFFS (BLK_VLAN_L4_UDP_OFFS + \
> > + BLK_META_DATA_OFFS)
> > +#define BLK_TUN_VLAN_L4_TCP_OFFS (BLK_VLAN_L4_TCP_OFFS + \
> > + BLK_META_DATA_OFFS)
> > +#define BLK_TUN_IPv4_TCP_FLAG (BLK_IPv4_TCP_FLAG +
> BLK_META_DATA_OFFS)
> > +#define BLK_TUN_VLAN_PCP (BLK_VLAN_PCP +
> BLK_META_DATA_OFFS)
> > +#define BLK_TUN_VLAN_IPv4_TCP_FLAG (BLK_VLAN_IPv4_TCP_FLAG + \
> > + BLK_META_DATA_OFFS)
> > +
> > #define PATTERN_STRIP_IPV6_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, \
> > @@ -744,7 +791,7 @@ mfex_avx512_process(struct dp_packet_batch
> *packets,
> > uint32_t keys_size OVS_UNUSED,
> > odp_port_t in_port,
> > void *pmd_handle OVS_UNUSED,
> > - bool md_is_valid OVS_UNUSED,
> > + bool md_is_valid,
> > const enum MFEX_PROFILES profile_id,
> > const uint32_t use_vbmi OVS_UNUSED) { @@ -770,6
> > +817,15 @@ mfex_avx512_process(struct dp_packet_batch *packets,
> > __m128i v_blocks01 = _mm_insert_epi32(v_zeros,
> > odp_to_u32(in_port), 1);
> >
> > DP_PACKET_BATCH_FOR_EACH (i, packet, packets) {
> > + /* Handle meta-data init in the loop. */
> > + if (!md_is_valid) {
> > + pkt_metadata_init(&packet->md, in_port);
> > + }
>
> Please see my comments on the patch 7/9 for whether we need this
> md_is_valid check.
>
Done remove tunnel check.
> > + const struct pkt_metadata *md = &packet->md;
> > + /* Dummy pmd dont always pass correct md_is_valid and hence
> > + * need to check the tunnel data to ensure correct behaviour.
> > + */
> > + bool tunnel = flow_tnl_dst_is_set(&md->tunnel);
> > /* If the packet is smaller than the probe size, skip it. */
> > const uint32_t size = dp_packet_size(packet);
> > if (size < dp_pkt_min_size) { @@ -808,7 +864,16 @@
> > mfex_avx512_process(struct dp_packet_batch *packets,
> >
> > use_vbmi);
> >
> > __m512i v_blk0_strip = _mm512_and_si512(v_blk0, v_strp);
> > - _mm512_storeu_si512(&blocks[2], v_blk0_strip);
> > + /* Handle inner meta-data if valid. */
> > + if (!tunnel) {
> > + _mm512_storeu_si512(&blocks[2], v_blk0_strip);
> > + } else {
> > + __m512i v_tun = _mm512_loadu_si512(&md->tunnel);
> > + _mm512_storeu_si512(&blocks[0], v_tun);
> > + _mm512_storeu_si512(&blocks[11], v_blk0_strip);
> > + blocks[BLK_META_DATA_OFFS] = md->dp_hash |
> > + ((uint64_t) odp_to_u32(md->in_port.odp_port) <<
> > 32);
> > + }
> >
> > /* Perform "post-processing" per profile, handling details not
> > easily
> > * handled in the above generic AVX512 code. Examples include
> > TCP flag @@ -820,38 +885,45 @@ mfex_avx512_process(struct
> dp_packet_batch *packets,
> > break;
> >
> > case PROFILE_ETH_VLAN_IPV4_TCP: {
> > - mfex_vlan_pcp(pkt[14], &keys[i].buf[4]);
> > -
> > uint32_t size_from_ipv4 = size - VLAN_ETH_HEADER_LEN;
> > struct ip_header *nh = (void *)&pkt[VLAN_ETH_HEADER_LEN];
> > if (mfex_ipv4_set_l2_pad_size(packet, nh, size_from_ipv4,
> > TCP_HEADER_LEN)) {
> > continue;
> > }
> > -
> > /* Process TCP flags, and store to blocks. */
> > const struct tcp_header *tcp = (void *)&pkt[38];
> > - mfex_handle_tcp_flags(tcp, &blocks[7]);
> > + uint32_t vlan_pcp_off = BLK_VLAN_PCP;
> > + uint32_t tcp_flag_off = BLK_VLAN_IPv4_TCP_FLAG;
> > +
> > + if (tunnel) {
> > + vlan_pcp_off = BLK_TUN_VLAN_PCP;
> > + tcp_flag_off = BLK_TUN_VLAN_IPv4_TCP_FLAG;
> > + mf->map.bits[0] = MF_ETH_VLAN_TUNNEL;
> > + }
>
> I like this pattern to reuse the existing MFEX impls for the tunnel case,
> since
> we just need to conditionally adjust offsets like you are doing and it will
> work
> for tunnel and non-tunnel cases. Avoids double the number of impls, nice
> job.
>
Thanks .
> > + mfex_vlan_pcp(pkt[14], &keys[i].buf[vlan_pcp_off]);
> > + mfex_handle_tcp_flags(tcp, &blocks[tcp_flag_off]);
> > dp_packet_update_rss_hash_ipv4_tcp_udp(packet);
> > } break;
> >
>
> <snip the rest of the MFEX impls>
>
> > diff --git a/lib/dpif-netdev-private-extract.c
> > b/lib/dpif-netdev-private-extract.c
> > index 12ac8ecce..5f7f1b6d3 100644
> > --- a/lib/dpif-netdev-private-extract.c
> > +++ b/lib/dpif-netdev-private-extract.c
> > @@ -362,7 +362,9 @@ dpif_miniflow_extract_autovalidator(struct
> > dp_packet_batch *packets,
> >
> > /* Run scalar miniflow_extract to get default result. */
> > DP_PACKET_BATCH_FOR_EACH (i, packet, packets) {
> > - pkt_metadata_init(&packet->md, in_port);
> > + if (!md_is_valid) {
> > + pkt_metadata_init(&packet->md, in_port);
> > + }
>
> Could the " bool tunnel = flow_tnl_dst_is_set(&md->tunnel);" type check
> be used here too?
>
Yes kept the md_is_valid as it's a better solution.
Regards
Amber
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
