[ovs-dev,v8,2/6] dpif-netdev: retrieve flow directly from the flow mark

Message ID eb929eeebdf95c04a0bc0414cfd2310b5e554d36.1522136948.git.shahafs@mellanox.com
State Superseded
Headers show
Series
  • OVS-DPDK flow offload with rte_flow
Related show

Commit Message

Shahaf Shuler March 27, 2018, 7:54 a.m.
From: Yuanhan Liu <yliu@fridaylinux.org>

So that we could skip some very costly CPU operations, including but
not limiting to miniflow_extract, emc lookup, dpcls lookup, etc. Thus,
performance could be greatly improved.

A PHY-PHY forwarding with 1000 mega flows (udp,tp_src=1000-1999) and
1 million streams (tp_src=1000-1999, tp_dst=2000-2999) show more that
260% performance boost.

Note that though the heavy miniflow_extract is skipped, we still have
to do per packet checking, due to we have to check the tcp_flags.

Co-authored-by: Finn Christensen <fc@napatech.com>
Signed-off-by: Yuanhan Liu <yliu@fridaylinux.org>
Signed-off-by: Finn Christensen <fc@napatech.com>
Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
---
 lib/dp-packet.h   |  13 +++++
 lib/dpif-netdev.c |  44 ++++++++++++--
 lib/flow.c        | 155 +++++++++++++++++++++++++++++++++++++++----------
 lib/flow.h        |   1 +
 4 files changed, 175 insertions(+), 38 deletions(-)

Comments

Ian Stokes April 10, 2018, 7:58 p.m. | #1
> Subject: [PATCH v8 2/6] dpif-netdev: retrieve flow directly from the flow
> mark
> 
> From: Yuanhan Liu <yliu@fridaylinux.org>
> 
> So that we could skip some very costly CPU operations, including but not
> limiting to miniflow_extract, emc lookup, dpcls lookup, etc. Thus,
> performance could be greatly improved.
> 
> A PHY-PHY forwarding with 1000 mega flows (udp,tp_src=1000-1999) and
> 1 million streams (tp_src=1000-1999, tp_dst=2000-2999) show more that 260%
> performance boost.
> 
> Note that though the heavy miniflow_extract is skipped, we still have to
> do per packet checking, due to we have to check the tcp_flags.
> 
> Co-authored-by: Finn Christensen <fc@napatech.com>
> Signed-off-by: Yuanhan Liu <yliu@fridaylinux.org>
> Signed-off-by: Finn Christensen <fc@napatech.com>
> Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> ---
>  lib/dp-packet.h   |  13 +++++
>  lib/dpif-netdev.c |  44 ++++++++++++--
>  lib/flow.c        | 155 +++++++++++++++++++++++++++++++++++++++----------
>  lib/flow.h        |   1 +
>  4 files changed, 175 insertions(+), 38 deletions(-)
> 
> diff --git a/lib/dp-packet.h b/lib/dp-packet.h index 21c8ca5..dd3f17b
> 100644
> --- a/lib/dp-packet.h
> +++ b/lib/dp-packet.h
> @@ -691,6 +691,19 @@ reset_dp_packet_checksum_ol_flags(struct dp_packet
> *p)  #define reset_dp_packet_checksum_ol_flags(arg)
>  #endif
> 
> +static inline bool
> +dp_packet_has_flow_mark(struct dp_packet *p OVS_UNUSED,
> +                        uint32_t *mark OVS_UNUSED) { #ifdef DPDK_NETDEV
> +    if (p->mbuf.ol_flags & PKT_RX_FDIR_ID) {
> +        *mark = p->mbuf.hash.fdir.hi;
> +        return true;
> +    }
> +#endif
> +    return false;
> +}
> +
>  enum { NETDEV_MAX_BURST = 32 }; /* Maximum number packets in a batch. */
> 
>  struct dp_packet_batch {
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 2fdb6ef..7489a2f
> 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -2015,6 +2015,23 @@ flow_mark_flush(struct dp_netdev_pmd_thread *pmd)
>      }
>  }
> 
> +static struct dp_netdev_flow *
> +mark_to_flow_find(const struct dp_netdev_pmd_thread *pmd,
> +                  const uint32_t mark)
> +{
> +    struct dp_netdev_flow *flow;
> +
> +    CMAP_FOR_EACH_WITH_HASH (flow, mark_node, hash_int(mark, 0),
> +                             &flow_mark.mark_to_flow) {
> +        if (flow->mark == mark && flow->pmd_id == pmd->core_id &&
> +            flow->dead == false) {
> +            return flow;
> +        }
> +    }
> +
> +    return NULL;
> +}
> +
>  static void
>  dp_netdev_pmd_remove_flow(struct dp_netdev_pmd_thread *pmd,
>                            struct dp_netdev_flow *flow) @@ -5204,10
> +5221,10 @@ struct packet_batch_per_flow {  static inline void
> packet_batch_per_flow_update(struct packet_batch_per_flow *batch,
>                               struct dp_packet *packet,
> -                             const struct miniflow *mf)
> +                             uint16_t tcp_flags)
>  {
>      batch->byte_count += dp_packet_size(packet);
> -    batch->tcp_flags |= miniflow_get_tcp_flags(mf);
> +    batch->tcp_flags |= tcp_flags;
>      batch->array.packets[batch->array.count++] = packet;  }
> 
> @@ -5241,7 +5258,7 @@ packet_batch_per_flow_execute(struct
> packet_batch_per_flow *batch,
> 
>  static inline void
>  dp_netdev_queue_batches(struct dp_packet *pkt,
> -                        struct dp_netdev_flow *flow, const struct
> miniflow *mf,
> +                        struct dp_netdev_flow *flow, uint16_t
> + tcp_flags,
>                          struct packet_batch_per_flow *batches,
>                          size_t *n_batches)  { @@ -5252,7 +5269,7 @@
> dp_netdev_queue_batches(struct dp_packet *pkt,
>          packet_batch_per_flow_init(batch, flow);
>      }
> 
> -    packet_batch_per_flow_update(batch, pkt, mf);
> +    packet_batch_per_flow_update(batch, pkt, tcp_flags);
>  }
> 
>  /* Try to process all ('cnt') the 'packets' using only the exact match
> cache @@ -5283,6 +5300,7 @@ emc_processing(struct dp_netdev_pmd_thread
> *pmd,
>      const size_t cnt = dp_packet_batch_size(packets_);
>      uint32_t cur_min;
>      int i;
> +    uint16_t tcp_flags;
> 
>      atomic_read_relaxed(&pmd->dp->emc_insert_min, &cur_min);
>      pmd_perf_update_counter(&pmd->perf_stats,
> @@ -5291,6 +5309,7 @@ emc_processing(struct dp_netdev_pmd_thread *pmd,
> 
>      DP_PACKET_BATCH_REFILL_FOR_EACH (i, cnt, packet, packets_) {
>          struct dp_netdev_flow *flow;
> +        uint32_t mark;
> 
>          if (OVS_UNLIKELY(dp_packet_size(packet) < ETH_HEADER_LEN)) {
>              dp_packet_delete(packet);
> @@ -5298,6 +5317,16 @@ emc_processing(struct dp_netdev_pmd_thread *pmd,
>              continue;
>          }
> 
> +        if (dp_packet_has_flow_mark(packet, &mark)) {
> +            flow = mark_to_flow_find(pmd, mark);
> +            if (flow) {
> +                tcp_flags = parse_tcp_flags(packet);
> +                dp_netdev_queue_batches(packet, flow, tcp_flags, batches,
> +                                        n_batches);
> +                continue;
> +            }
> +        }
> +
>          if (i != cnt - 1) {
>              struct dp_packet **packets = packets_->packets;
>              /* Prefetch next packet data and metadata. */ @@ -5323,7
> +5352,8 @@ emc_processing(struct dp_netdev_pmd_thread *pmd,
>              flow = NULL;
>          }
>          if (OVS_LIKELY(flow)) {
> -            dp_netdev_queue_batches(packet, flow, &key->mf, batches,
> +            tcp_flags = miniflow_get_tcp_flags(&key->mf);
> +            dp_netdev_queue_batches(packet, flow, tcp_flags, batches,
>                                      n_batches);
>          } else {
>              /* Exact match cache missed. Group missed packets together at
> @@ -5501,7 +5531,9 @@ fast_path_processing(struct dp_netdev_pmd_thread
> *pmd,
>          flow = dp_netdev_flow_cast(rules[i]);
> 
>          emc_probabilistic_insert(pmd, &keys[i], flow);
> -        dp_netdev_queue_batches(packet, flow, &keys[i].mf, batches,
> n_batches);
> +        dp_netdev_queue_batches(packet, flow,
> +                                miniflow_get_tcp_flags(&keys[i].mf),
> +                                batches, n_batches);
>      }
> 
>      pmd_perf_update_counter(&pmd->perf_stats, PMD_STAT_MASKED_HIT, diff -
> -git a/lib/flow.c b/lib/flow.c index 38ff29c..6c74dd3 100644
> --- a/lib/flow.c
> +++ b/lib/flow.c
> @@ -624,6 +624,70 @@ flow_extract(struct dp_packet *packet, struct flow
> *flow)
>      miniflow_expand(&m.mf, flow);
>  }
> 
> +static inline bool
> +ipv4_sanity_check(const struct ip_header *nh, size_t size,
> +                  int *ip_lenp, uint16_t *tot_lenp) {
> +    int ip_len;
> +    uint16_t tot_len;
> +
> +    if (OVS_UNLIKELY(size < IP_HEADER_LEN)) {
> +        return false;
> +    }
> +    ip_len = IP_IHL(nh->ip_ihl_ver) * 4;
> +
> +    if (OVS_UNLIKELY(ip_len < IP_HEADER_LEN || size < ip_len)) {
> +        return false;
> +    }
> +
> +    tot_len = ntohs(nh->ip_tot_len);
> +    if (OVS_UNLIKELY(tot_len > size || ip_len > tot_len ||
> +                size - tot_len > UINT8_MAX)) {
> +        return false;
> +    }
> +
> +    *ip_lenp = ip_len;
> +    *tot_lenp = tot_len;
> +
> +    return true;
> +}
> +
> +static inline uint8_t
> +ipv4_get_nw_frag(const struct ip_header *nh) {
> +    uint8_t nw_frag = 0;
> +
> +    if (OVS_UNLIKELY(IP_IS_FRAGMENT(nh->ip_frag_off))) {
> +        nw_frag = FLOW_NW_FRAG_ANY;
> +        if (nh->ip_frag_off & htons(IP_FRAG_OFF_MASK)) {
> +            nw_frag |= FLOW_NW_FRAG_LATER;
> +        }
> +    }
> +
> +    return nw_frag;
> +}
> +
> +static inline bool
> +ipv6_sanity_check(const struct ovs_16aligned_ip6_hdr *nh, size_t size)
> +{
> +    uint16_t plen;
> +
> +    if (OVS_UNLIKELY(size < sizeof *nh)) {
> +        return false;
> +    }
> +
> +    plen = ntohs(nh->ip6_plen);
> +    if (OVS_UNLIKELY(plen > size)) {
> +        return false;
> +    }
> +    /* Jumbo Payload option not supported yet. */
> +    if (OVS_UNLIKELY(size - plen > UINT8_MAX)) {
> +        return false;
> +    }
> +
> +    return true;
> +}
> +
>  /* Caller is responsible for initializing 'dst' with enough storage for
>   * FLOW_U64S * 8 bytes. */
>  void
> @@ -748,22 +812,7 @@ miniflow_extract(struct dp_packet *packet, struct
> miniflow *dst)
>          int ip_len;
>          uint16_t tot_len;
> 
> -        if (OVS_UNLIKELY(size < IP_HEADER_LEN)) {
> -            goto out;
> -        }
> -        ip_len = IP_IHL(nh->ip_ihl_ver) * 4;
> -
> -        if (OVS_UNLIKELY(ip_len < IP_HEADER_LEN)) {
> -            goto out;
> -        }
> -        if (OVS_UNLIKELY(size < ip_len)) {
> -            goto out;
> -        }
> -        tot_len = ntohs(nh->ip_tot_len);
> -        if (OVS_UNLIKELY(tot_len > size || ip_len > tot_len)) {
> -            goto out;
> -        }
> -        if (OVS_UNLIKELY(size - tot_len > UINT8_MAX)) {
> +        if (OVS_UNLIKELY(!ipv4_sanity_check(nh, size, &ip_len,
> + &tot_len))) {

You may have missed my comment in the v7 regarding this.

There's no mention in commit message of the introduction of IPv4/IPv6/fragmentation sanity checks.

This affects minflow extract regardless of HWOL usage. The miniflow extract refactor should  to use these should be introduced as a separate patch to this series so it's clear that it is being modified.

>              goto out;
>          }
>          dp_packet_set_l2_pad_size(packet, size - tot_len); @@ -786,31
> +835,19 @@ miniflow_extract(struct dp_packet *packet, struct miniflow
> *dst)
>          nw_tos = nh->ip_tos;
>          nw_ttl = nh->ip_ttl;
>          nw_proto = nh->ip_proto;
> -        if (OVS_UNLIKELY(IP_IS_FRAGMENT(nh->ip_frag_off))) {
> -            nw_frag = FLOW_NW_FRAG_ANY;
> -            if (nh->ip_frag_off & htons(IP_FRAG_OFF_MASK)) {
> -                nw_frag |= FLOW_NW_FRAG_LATER;
> -            }
> -        }
> +        nw_frag = ipv4_get_nw_frag(nh);
>          data_pull(&data, &size, ip_len);
>      } else if (dl_type == htons(ETH_TYPE_IPV6)) {
> -        const struct ovs_16aligned_ip6_hdr *nh;
> +        const struct ovs_16aligned_ip6_hdr *nh = data;
>          ovs_be32 tc_flow;
>          uint16_t plen;
> 
> -        if (OVS_UNLIKELY(size < sizeof *nh)) {
> +        if (OVS_UNLIKELY(!ipv6_sanity_check(nh, size))) {
>              goto out;
>          }
> -        nh = data_pull(&data, &size, sizeof *nh);
> +        data_pull(&data, &size, sizeof *nh);
> 
>          plen = ntohs(nh->ip6_plen);
> -        if (OVS_UNLIKELY(plen > size)) {
> -            goto out;
> -        }
> -        /* Jumbo Payload option not supported yet. */
> -        if (OVS_UNLIKELY(size - plen > UINT8_MAX)) {
> -            goto out;
> -        }
>          dp_packet_set_l2_pad_size(packet, size - plen);
>          size = plen;   /* Never pull padding. */
> 
> @@ -982,6 +1019,60 @@ parse_dl_type(const struct eth_header *data_, size_t
> size)
>      return parse_ethertype(&data, &size);  }
> 
> +uint16_t
> +parse_tcp_flags(struct dp_packet *packet) {
> +    const void *data = dp_packet_data(packet);
> +    size_t size = dp_packet_size(packet);
> +    ovs_be16 dl_type;
> +    uint8_t nw_frag = 0, nw_proto = 0;
> +
> +    if (packet->packet_type != htonl(PT_ETH)) {
> +        return 0;
> +    }
> +
> +    data_pull(&data, &size, ETH_ADDR_LEN * 2);
> +    dl_type = parse_ethertype(&data, &size);
> +    if (OVS_LIKELY(dl_type == htons(ETH_TYPE_IP))) {
> +        const struct ip_header *nh = data;
> +        int ip_len;
> +        uint16_t tot_len;
> +
> +        if (OVS_UNLIKELY(!ipv4_sanity_check(nh, size, &ip_len,
> &tot_len))) {
> +            return 0;
> +        }
> +        nw_proto = nh->ip_proto;
> +        nw_frag = ipv4_get_nw_frag(nh);
> +
> +        size = tot_len;   /* Never pull padding. */
> +        data_pull(&data, &size, ip_len);
> +    } else if (dl_type == htons(ETH_TYPE_IPV6)) {
> +        const struct ovs_16aligned_ip6_hdr *nh = data;
> +
> +        if (OVS_UNLIKELY(!ipv6_sanity_check(nh, size))) {
> +            return 0;
> +        }
> +        data_pull(&data, &size, sizeof *nh);
> +
> +        size = ntohs(nh->ip6_plen); /* Never pull padding. */
> +        if (!parse_ipv6_ext_hdrs__(&data, &size, &nw_proto, &nw_frag)) {
> +            return 0;
> +        }
> +        nw_proto = nh->ip6_nxt;
> +    } else {
> +        return 0;
> +    }
> +
> +    if (!(nw_frag & FLOW_NW_FRAG_LATER) && nw_proto == IPPROTO_TCP &&
> +        size >= TCP_HEADER_LEN) {
> +        const struct tcp_header *tcp = data;
> +
> +        return TCP_FLAGS_BE32(tcp->tcp_ctl);

Will cause compilation failure with sparse when compiling without DPDK.

lib/flow.c:1070:16: error: incorrect type in return expression (different base types)
lib/flow.c:1070:16:    expected unsigned short
lib/flow.c:1070:16:    got restricted ovs_be32 [usertype] <noident>

https://travis-ci.org/istokes/ovs/jobs/364709985

> +    }
> +
> +    return 0;
> +}
> +
>  /* For every bit of a field that is wildcarded in 'wildcards', sets the
>   * corresponding bit in 'flow' to zero. */  void diff --git a/lib/flow.h
> b/lib/flow.h index 770a07a..0adecbf 100644
> --- a/lib/flow.h
> +++ b/lib/flow.h
> @@ -132,6 +132,7 @@ bool parse_ipv6_ext_hdrs(const void **datap, size_t
> *sizep, uint8_t *nw_proto,
>                           uint8_t *nw_frag);
>  ovs_be16 parse_dl_type(const struct eth_header *data_, size_t size);
> bool parse_nsh(const void **datap, size_t *sizep, struct ovs_key_nsh
> *key);
> +uint16_t parse_tcp_flags(struct dp_packet *packet);
> 
>  static inline uint64_t
>  flow_get_xreg(const struct flow *flow, int idx)
> --
> 2.7.4
Shahaf Shuler April 11, 2018, 12:18 p.m. | #2
Tuesday, April 10, 2018 10:58 PM, Stokes, Ian:
> Subject: RE: [PATCH v8 2/6] dpif-netdev: retrieve flow directly from the flow
> mark
> 
> > Subject: [PATCH v8 2/6] dpif-netdev: retrieve flow directly from the
> > flow mark
> >
> > From: Yuanhan Liu <yliu@fridaylinux.org>
> >
> > So that we could skip some very costly CPU operations, including but
> > not limiting to miniflow_extract, emc lookup, dpcls lookup, etc. Thus,
> > performance could be greatly improved.
> >
> > A PHY-PHY forwarding with 1000 mega flows (udp,tp_src=1000-1999) and
> > 1 million streams (tp_src=1000-1999, tp_dst=2000-2999) show more that
> > 260% performance boost.
> >
> > Note that though the heavy miniflow_extract is skipped, we still have
> > to do per packet checking, due to we have to check the tcp_flags.
> >
> > Co-authored-by: Finn Christensen <fc@napatech.com>
> > Signed-off-by: Yuanhan Liu <yliu@fridaylinux.org>
> > Signed-off-by: Finn Christensen <fc@napatech.com>
> > Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> > ---
> >  lib/dp-packet.h   |  13 +++++
> >  lib/dpif-netdev.c |  44 ++++++++++++--
> >  lib/flow.c        | 155 +++++++++++++++++++++++++++++++++++++++-----
> -----
> >  lib/flow.h        |   1 +
> >  4 files changed, 175 insertions(+), 38 deletions(-)
> >
> > diff --git a/lib/dp-packet.h b/lib/dp-packet.h index 21c8ca5..dd3f17b
> > 100644
> > --- a/lib/dp-packet.h
> > +++ b/lib/dp-packet.h
> > @@ -691,6 +691,19 @@ reset_dp_packet_checksum_ol_flags(struct
> > dp_packet
> > *p)  #define reset_dp_packet_checksum_ol_flags(arg)
> >  #endif
> >
> > +static inline bool
> > +dp_packet_has_flow_mark(struct dp_packet *p OVS_UNUSED,
> > +                        uint32_t *mark OVS_UNUSED) { #ifdef DPDK_NETDEV
> > +    if (p->mbuf.ol_flags & PKT_RX_FDIR_ID) {
> > +        *mark = p->mbuf.hash.fdir.hi;
> > +        return true;
> > +    }
> > +#endif
> > +    return false;
> > +}
> > +
> >  enum { NETDEV_MAX_BURST = 32 }; /* Maximum number packets in a
> batch.
> > */
> >
> >  struct dp_packet_batch {
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index
> > 2fdb6ef..7489a2f
> > 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -2015,6 +2015,23 @@ flow_mark_flush(struct
> dp_netdev_pmd_thread *pmd)
> >      }
> >  }
> >
> > +static struct dp_netdev_flow *
> > +mark_to_flow_find(const struct dp_netdev_pmd_thread *pmd,
> > +                  const uint32_t mark) {
> > +    struct dp_netdev_flow *flow;
> > +
> > +    CMAP_FOR_EACH_WITH_HASH (flow, mark_node, hash_int(mark, 0),
> > +                             &flow_mark.mark_to_flow) {
> > +        if (flow->mark == mark && flow->pmd_id == pmd->core_id &&
> > +            flow->dead == false) {
> > +            return flow;
> > +        }
> > +    }
> > +
> > +    return NULL;
> > +}
> > +
> >  static void
> >  dp_netdev_pmd_remove_flow(struct dp_netdev_pmd_thread *pmd,
> >                            struct dp_netdev_flow *flow) @@ -5204,10
> > +5221,10 @@ struct packet_batch_per_flow {  static inline void
> > packet_batch_per_flow_update(struct packet_batch_per_flow *batch,
> >                               struct dp_packet *packet,
> > -                             const struct miniflow *mf)
> > +                             uint16_t tcp_flags)
> >  {
> >      batch->byte_count += dp_packet_size(packet);
> > -    batch->tcp_flags |= miniflow_get_tcp_flags(mf);
> > +    batch->tcp_flags |= tcp_flags;
> >      batch->array.packets[batch->array.count++] = packet;  }
> >
> > @@ -5241,7 +5258,7 @@ packet_batch_per_flow_execute(struct
> > packet_batch_per_flow *batch,
> >
> >  static inline void
> >  dp_netdev_queue_batches(struct dp_packet *pkt,
> > -                        struct dp_netdev_flow *flow, const struct
> > miniflow *mf,
> > +                        struct dp_netdev_flow *flow, uint16_t
> > + tcp_flags,
> >                          struct packet_batch_per_flow *batches,
> >                          size_t *n_batches)  { @@ -5252,7 +5269,7 @@
> > dp_netdev_queue_batches(struct dp_packet *pkt,
> >          packet_batch_per_flow_init(batch, flow);
> >      }
> >
> > -    packet_batch_per_flow_update(batch, pkt, mf);
> > +    packet_batch_per_flow_update(batch, pkt, tcp_flags);
> >  }
> >
> >  /* Try to process all ('cnt') the 'packets' using only the exact
> > match cache @@ -5283,6 +5300,7 @@ emc_processing(struct
> > dp_netdev_pmd_thread *pmd,
> >      const size_t cnt = dp_packet_batch_size(packets_);
> >      uint32_t cur_min;
> >      int i;
> > +    uint16_t tcp_flags;
> >
> >      atomic_read_relaxed(&pmd->dp->emc_insert_min, &cur_min);
> >      pmd_perf_update_counter(&pmd->perf_stats,
> > @@ -5291,6 +5309,7 @@ emc_processing(struct dp_netdev_pmd_thread
> *pmd,
> >
> >      DP_PACKET_BATCH_REFILL_FOR_EACH (i, cnt, packet, packets_) {
> >          struct dp_netdev_flow *flow;
> > +        uint32_t mark;
> >
> >          if (OVS_UNLIKELY(dp_packet_size(packet) < ETH_HEADER_LEN)) {
> >              dp_packet_delete(packet); @@ -5298,6 +5317,16 @@
> > emc_processing(struct dp_netdev_pmd_thread *pmd,
> >              continue;
> >          }
> >
> > +        if (dp_packet_has_flow_mark(packet, &mark)) {
> > +            flow = mark_to_flow_find(pmd, mark);
> > +            if (flow) {
> > +                tcp_flags = parse_tcp_flags(packet);
> > +                dp_netdev_queue_batches(packet, flow, tcp_flags, batches,
> > +                                        n_batches);
> > +                continue;
> > +            }
> > +        }
> > +
> >          if (i != cnt - 1) {
> >              struct dp_packet **packets = packets_->packets;
> >              /* Prefetch next packet data and metadata. */ @@ -5323,7
> > +5352,8 @@ emc_processing(struct dp_netdev_pmd_thread *pmd,
> >              flow = NULL;
> >          }
> >          if (OVS_LIKELY(flow)) {
> > -            dp_netdev_queue_batches(packet, flow, &key->mf, batches,
> > +            tcp_flags = miniflow_get_tcp_flags(&key->mf);
> > +            dp_netdev_queue_batches(packet, flow, tcp_flags, batches,
> >                                      n_batches);
> >          } else {
> >              /* Exact match cache missed. Group missed packets
> > together at @@ -5501,7 +5531,9 @@ fast_path_processing(struct
> > dp_netdev_pmd_thread *pmd,
> >          flow = dp_netdev_flow_cast(rules[i]);
> >
> >          emc_probabilistic_insert(pmd, &keys[i], flow);
> > -        dp_netdev_queue_batches(packet, flow, &keys[i].mf, batches,
> > n_batches);
> > +        dp_netdev_queue_batches(packet, flow,
> > +                                miniflow_get_tcp_flags(&keys[i].mf),
> > +                                batches, n_batches);
> >      }
> >
> >      pmd_perf_update_counter(&pmd->perf_stats,
> PMD_STAT_MASKED_HIT,
> > diff - -git a/lib/flow.c b/lib/flow.c index 38ff29c..6c74dd3 100644
> > --- a/lib/flow.c
> > +++ b/lib/flow.c
> > @@ -624,6 +624,70 @@ flow_extract(struct dp_packet *packet, struct
> > flow
> > *flow)
> >      miniflow_expand(&m.mf, flow);
> >  }
> >
> > +static inline bool
> > +ipv4_sanity_check(const struct ip_header *nh, size_t size,
> > +                  int *ip_lenp, uint16_t *tot_lenp) {
> > +    int ip_len;
> > +    uint16_t tot_len;
> > +
> > +    if (OVS_UNLIKELY(size < IP_HEADER_LEN)) {
> > +        return false;
> > +    }
> > +    ip_len = IP_IHL(nh->ip_ihl_ver) * 4;
> > +
> > +    if (OVS_UNLIKELY(ip_len < IP_HEADER_LEN || size < ip_len)) {
> > +        return false;
> > +    }
> > +
> > +    tot_len = ntohs(nh->ip_tot_len);
> > +    if (OVS_UNLIKELY(tot_len > size || ip_len > tot_len ||
> > +                size - tot_len > UINT8_MAX)) {
> > +        return false;
> > +    }
> > +
> > +    *ip_lenp = ip_len;
> > +    *tot_lenp = tot_len;
> > +
> > +    return true;
> > +}
> > +
> > +static inline uint8_t
> > +ipv4_get_nw_frag(const struct ip_header *nh) {
> > +    uint8_t nw_frag = 0;
> > +
> > +    if (OVS_UNLIKELY(IP_IS_FRAGMENT(nh->ip_frag_off))) {
> > +        nw_frag = FLOW_NW_FRAG_ANY;
> > +        if (nh->ip_frag_off & htons(IP_FRAG_OFF_MASK)) {
> > +            nw_frag |= FLOW_NW_FRAG_LATER;
> > +        }
> > +    }
> > +
> > +    return nw_frag;
> > +}
> > +
> > +static inline bool
> > +ipv6_sanity_check(const struct ovs_16aligned_ip6_hdr *nh, size_t
> > +size) {
> > +    uint16_t plen;
> > +
> > +    if (OVS_UNLIKELY(size < sizeof *nh)) {
> > +        return false;
> > +    }
> > +
> > +    plen = ntohs(nh->ip6_plen);
> > +    if (OVS_UNLIKELY(plen > size)) {
> > +        return false;
> > +    }
> > +    /* Jumbo Payload option not supported yet. */
> > +    if (OVS_UNLIKELY(size - plen > UINT8_MAX)) {
> > +        return false;
> > +    }
> > +
> > +    return true;
> > +}
> > +
> >  /* Caller is responsible for initializing 'dst' with enough storage for
> >   * FLOW_U64S * 8 bytes. */
> >  void
> > @@ -748,22 +812,7 @@ miniflow_extract(struct dp_packet *packet, struct
> > miniflow *dst)
> >          int ip_len;
> >          uint16_t tot_len;
> >
> > -        if (OVS_UNLIKELY(size < IP_HEADER_LEN)) {
> > -            goto out;
> > -        }
> > -        ip_len = IP_IHL(nh->ip_ihl_ver) * 4;
> > -
> > -        if (OVS_UNLIKELY(ip_len < IP_HEADER_LEN)) {
> > -            goto out;
> > -        }
> > -        if (OVS_UNLIKELY(size < ip_len)) {
> > -            goto out;
> > -        }
> > -        tot_len = ntohs(nh->ip_tot_len);
> > -        if (OVS_UNLIKELY(tot_len > size || ip_len > tot_len)) {
> > -            goto out;
> > -        }
> > -        if (OVS_UNLIKELY(size - tot_len > UINT8_MAX)) {
> > +        if (OVS_UNLIKELY(!ipv4_sanity_check(nh, size, &ip_len,
> > + &tot_len))) {
> 
> You may have missed my comment in the v7 regarding this.
> 
> There's no mention in commit message of the introduction of
> IPv4/IPv6/fragmentation sanity checks.
> 
> This affects minflow extract regardless of HWOL usage. The miniflow extract
> refactor should  to use these should be introduced as a separate patch to this
> series so it's clear that it is being modified.
> 
> >              goto out;
> >          }
> >          dp_packet_set_l2_pad_size(packet, size - tot_len); @@ -786,31
> > +835,19 @@ miniflow_extract(struct dp_packet *packet, struct miniflow
> > *dst)
> >          nw_tos = nh->ip_tos;
> >          nw_ttl = nh->ip_ttl;
> >          nw_proto = nh->ip_proto;
> > -        if (OVS_UNLIKELY(IP_IS_FRAGMENT(nh->ip_frag_off))) {
> > -            nw_frag = FLOW_NW_FRAG_ANY;
> > -            if (nh->ip_frag_off & htons(IP_FRAG_OFF_MASK)) {
> > -                nw_frag |= FLOW_NW_FRAG_LATER;
> > -            }
> > -        }
> > +        nw_frag = ipv4_get_nw_frag(nh);
> >          data_pull(&data, &size, ip_len);
> >      } else if (dl_type == htons(ETH_TYPE_IPV6)) {
> > -        const struct ovs_16aligned_ip6_hdr *nh;
> > +        const struct ovs_16aligned_ip6_hdr *nh = data;
> >          ovs_be32 tc_flow;
> >          uint16_t plen;
> >
> > -        if (OVS_UNLIKELY(size < sizeof *nh)) {
> > +        if (OVS_UNLIKELY(!ipv6_sanity_check(nh, size))) {
> >              goto out;
> >          }
> > -        nh = data_pull(&data, &size, sizeof *nh);
> > +        data_pull(&data, &size, sizeof *nh);
> >
> >          plen = ntohs(nh->ip6_plen);
> > -        if (OVS_UNLIKELY(plen > size)) {
> > -            goto out;
> > -        }
> > -        /* Jumbo Payload option not supported yet. */
> > -        if (OVS_UNLIKELY(size - plen > UINT8_MAX)) {
> > -            goto out;
> > -        }
> >          dp_packet_set_l2_pad_size(packet, size - plen);
> >          size = plen;   /* Never pull padding. */
> >
> > @@ -982,6 +1019,60 @@ parse_dl_type(const struct eth_header *data_,
> > size_t
> > size)
> >      return parse_ethertype(&data, &size);  }
> >
> > +uint16_t
> > +parse_tcp_flags(struct dp_packet *packet) {
> > +    const void *data = dp_packet_data(packet);
> > +    size_t size = dp_packet_size(packet);
> > +    ovs_be16 dl_type;
> > +    uint8_t nw_frag = 0, nw_proto = 0;
> > +
> > +    if (packet->packet_type != htonl(PT_ETH)) {
> > +        return 0;
> > +    }
> > +
> > +    data_pull(&data, &size, ETH_ADDR_LEN * 2);
> > +    dl_type = parse_ethertype(&data, &size);
> > +    if (OVS_LIKELY(dl_type == htons(ETH_TYPE_IP))) {
> > +        const struct ip_header *nh = data;
> > +        int ip_len;
> > +        uint16_t tot_len;
> > +
> > +        if (OVS_UNLIKELY(!ipv4_sanity_check(nh, size, &ip_len,
> > &tot_len))) {
> > +            return 0;
> > +        }
> > +        nw_proto = nh->ip_proto;
> > +        nw_frag = ipv4_get_nw_frag(nh);
> > +
> > +        size = tot_len;   /* Never pull padding. */
> > +        data_pull(&data, &size, ip_len);
> > +    } else if (dl_type == htons(ETH_TYPE_IPV6)) {
> > +        const struct ovs_16aligned_ip6_hdr *nh = data;
> > +
> > +        if (OVS_UNLIKELY(!ipv6_sanity_check(nh, size))) {
> > +            return 0;
> > +        }
> > +        data_pull(&data, &size, sizeof *nh);
> > +
> > +        size = ntohs(nh->ip6_plen); /* Never pull padding. */
> > +        if (!parse_ipv6_ext_hdrs__(&data, &size, &nw_proto, &nw_frag)) {
> > +            return 0;
> > +        }
> > +        nw_proto = nh->ip6_nxt;
> > +    } else {
> > +        return 0;
> > +    }
> > +
> > +    if (!(nw_frag & FLOW_NW_FRAG_LATER) && nw_proto ==
> IPPROTO_TCP &&
> > +        size >= TCP_HEADER_LEN) {
> > +        const struct tcp_header *tcp = data;
> > +
> > +        return TCP_FLAGS_BE32(tcp->tcp_ctl);
> 
> Will cause compilation failure with sparse when compiling without DPDK.
> 
> lib/flow.c:1070:16: error: incorrect type in return expression (different base
> types)
> lib/flow.c:1070:16:    expected unsigned short
> lib/flow.c:1070:16:    got restricted ovs_be32 [usertype] <noident>

Thanks,

Is there a way for me to re-run travis build so I can verify it is fixed? 

> 
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftra
> vis-
> ci.org%2Fistokes%2Fovs%2Fjobs%2F364709985&data=02%7C01%7Cshahafs%
> 40mellanox.com%7C624a50f6dcd64230e11c08d59f1d6f66%7Ca652971c7d2e4
> d9ba6a4d149256f461b%7C0%7C0%7C636589871173668041&sdata=bZ6FwLkX
> B0%2FphhOaQBb6tBjAkNaA8to%2BSWiuK6zdBew%3D&reserved=0
> 
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >  /* For every bit of a field that is wildcarded in 'wildcards', sets the
> >   * corresponding bit in 'flow' to zero. */  void diff --git
> > a/lib/flow.h b/lib/flow.h index 770a07a..0adecbf 100644
> > --- a/lib/flow.h
> > +++ b/lib/flow.h
> > @@ -132,6 +132,7 @@ bool parse_ipv6_ext_hdrs(const void **datap,
> > size_t *sizep, uint8_t *nw_proto,
> >                           uint8_t *nw_frag);
> >  ovs_be16 parse_dl_type(const struct eth_header *data_, size_t size);
> > bool parse_nsh(const void **datap, size_t *sizep, struct ovs_key_nsh
> > *key);
> > +uint16_t parse_tcp_flags(struct dp_packet *packet);
> >
> >  static inline uint64_t
> >  flow_get_xreg(const struct flow *flow, int idx)
> > --
> > 2.7.4
Ian Stokes April 12, 2018, 1:39 p.m. | #3
> Tuesday, April 10, 2018 10:58 PM, Stokes, Ian:
> > Subject: RE: [PATCH v8 2/6] dpif-netdev: retrieve flow directly from
> > the flow mark
> >
> > > Subject: [PATCH v8 2/6] dpif-netdev: retrieve flow directly from the
> > > flow mark
> > >
> > > From: Yuanhan Liu <yliu@fridaylinux.org>
> > >
> > > So that we could skip some very costly CPU operations, including but
> > > not limiting to miniflow_extract, emc lookup, dpcls lookup, etc.
> > > Thus, performance could be greatly improved.
> > >
> > > A PHY-PHY forwarding with 1000 mega flows (udp,tp_src=1000-1999) and
> > > 1 million streams (tp_src=1000-1999, tp_dst=2000-2999) show more
> > > that 260% performance boost.
> > >
> > > Note that though the heavy miniflow_extract is skipped, we still
> > > have to do per packet checking, due to we have to check the tcp_flags.
> > >
> > > Co-authored-by: Finn Christensen <fc@napatech.com>
> > > Signed-off-by: Yuanhan Liu <yliu@fridaylinux.org>
> > > Signed-off-by: Finn Christensen <fc@napatech.com>
> > > Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> > > ---
> > >  lib/dp-packet.h   |  13 +++++
> > >  lib/dpif-netdev.c |  44 ++++++++++++--
> > >  lib/flow.c        | 155 +++++++++++++++++++++++++++++++++++++++-----
> > -----
> > >  lib/flow.h        |   1 +
> > >  4 files changed, 175 insertions(+), 38 deletions(-)
> > >
> > > diff --git a/lib/dp-packet.h b/lib/dp-packet.h index
> > > 21c8ca5..dd3f17b
> > > 100644
> > > --- a/lib/dp-packet.h
> > > +++ b/lib/dp-packet.h
> > > @@ -691,6 +691,19 @@ reset_dp_packet_checksum_ol_flags(struct
> > > dp_packet
> > > *p)  #define reset_dp_packet_checksum_ol_flags(arg)
> > >  #endif
> > >
> > > +static inline bool
> > > +dp_packet_has_flow_mark(struct dp_packet *p OVS_UNUSED,
> > > +                        uint32_t *mark OVS_UNUSED) { #ifdef
> DPDK_NETDEV
> > > +    if (p->mbuf.ol_flags & PKT_RX_FDIR_ID) {
> > > +        *mark = p->mbuf.hash.fdir.hi;
> > > +        return true;
> > > +    }
> > > +#endif
> > > +    return false;
> > > +}
> > > +
> > >  enum { NETDEV_MAX_BURST = 32 }; /* Maximum number packets in a
> > batch.
> > > */
> > >
> > >  struct dp_packet_batch {
> > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index
> > > 2fdb6ef..7489a2f
> > > 100644
> > > --- a/lib/dpif-netdev.c
> > > +++ b/lib/dpif-netdev.c
> > > @@ -2015,6 +2015,23 @@ flow_mark_flush(struct
> > dp_netdev_pmd_thread *pmd)
> > >      }
> > >  }
> > >
> > > +static struct dp_netdev_flow *
> > > +mark_to_flow_find(const struct dp_netdev_pmd_thread *pmd,
> > > +                  const uint32_t mark) {
> > > +    struct dp_netdev_flow *flow;
> > > +
> > > +    CMAP_FOR_EACH_WITH_HASH (flow, mark_node, hash_int(mark, 0),
> > > +                             &flow_mark.mark_to_flow) {
> > > +        if (flow->mark == mark && flow->pmd_id == pmd->core_id &&
> > > +            flow->dead == false) {
> > > +            return flow;
> > > +        }
> > > +    }
> > > +
> > > +    return NULL;
> > > +}
> > > +
> > >  static void
> > >  dp_netdev_pmd_remove_flow(struct dp_netdev_pmd_thread *pmd,
> > >                            struct dp_netdev_flow *flow) @@ -5204,10
> > > +5221,10 @@ struct packet_batch_per_flow {  static inline void
> > > packet_batch_per_flow_update(struct packet_batch_per_flow *batch,
> > >                               struct dp_packet *packet,
> > > -                             const struct miniflow *mf)
> > > +                             uint16_t tcp_flags)
> > >  {
> > >      batch->byte_count += dp_packet_size(packet);
> > > -    batch->tcp_flags |= miniflow_get_tcp_flags(mf);
> > > +    batch->tcp_flags |= tcp_flags;
> > >      batch->array.packets[batch->array.count++] = packet;  }
> > >
> > > @@ -5241,7 +5258,7 @@ packet_batch_per_flow_execute(struct
> > > packet_batch_per_flow *batch,
> > >
> > >  static inline void
> > >  dp_netdev_queue_batches(struct dp_packet *pkt,
> > > -                        struct dp_netdev_flow *flow, const struct
> > > miniflow *mf,
> > > +                        struct dp_netdev_flow *flow, uint16_t
> > > + tcp_flags,
> > >                          struct packet_batch_per_flow *batches,
> > >                          size_t *n_batches)  { @@ -5252,7 +5269,7 @@
> > > dp_netdev_queue_batches(struct dp_packet *pkt,
> > >          packet_batch_per_flow_init(batch, flow);
> > >      }
> > >
> > > -    packet_batch_per_flow_update(batch, pkt, mf);
> > > +    packet_batch_per_flow_update(batch, pkt, tcp_flags);
> > >  }
> > >
> > >  /* Try to process all ('cnt') the 'packets' using only the exact
> > > match cache @@ -5283,6 +5300,7 @@ emc_processing(struct
> > > dp_netdev_pmd_thread *pmd,
> > >      const size_t cnt = dp_packet_batch_size(packets_);
> > >      uint32_t cur_min;
> > >      int i;
> > > +    uint16_t tcp_flags;
> > >
> > >      atomic_read_relaxed(&pmd->dp->emc_insert_min, &cur_min);
> > >      pmd_perf_update_counter(&pmd->perf_stats,
> > > @@ -5291,6 +5309,7 @@ emc_processing(struct dp_netdev_pmd_thread
> > *pmd,
> > >
> > >      DP_PACKET_BATCH_REFILL_FOR_EACH (i, cnt, packet, packets_) {
> > >          struct dp_netdev_flow *flow;
> > > +        uint32_t mark;
> > >
> > >          if (OVS_UNLIKELY(dp_packet_size(packet) < ETH_HEADER_LEN)) {
> > >              dp_packet_delete(packet); @@ -5298,6 +5317,16 @@
> > > emc_processing(struct dp_netdev_pmd_thread *pmd,
> > >              continue;
> > >          }
> > >
> > > +        if (dp_packet_has_flow_mark(packet, &mark)) {
> > > +            flow = mark_to_flow_find(pmd, mark);
> > > +            if (flow) {
> > > +                tcp_flags = parse_tcp_flags(packet);
> > > +                dp_netdev_queue_batches(packet, flow, tcp_flags,
> batches,
> > > +                                        n_batches);
> > > +                continue;
> > > +            }
> > > +        }
> > > +
> > >          if (i != cnt - 1) {
> > >              struct dp_packet **packets = packets_->packets;
> > >              /* Prefetch next packet data and metadata. */ @@
> > > -5323,7
> > > +5352,8 @@ emc_processing(struct dp_netdev_pmd_thread *pmd,
> > >              flow = NULL;
> > >          }
> > >          if (OVS_LIKELY(flow)) {
> > > -            dp_netdev_queue_batches(packet, flow, &key->mf, batches,
> > > +            tcp_flags = miniflow_get_tcp_flags(&key->mf);
> > > +            dp_netdev_queue_batches(packet, flow, tcp_flags,
> > > + batches,
> > >                                      n_batches);
> > >          } else {
> > >              /* Exact match cache missed. Group missed packets
> > > together at @@ -5501,7 +5531,9 @@ fast_path_processing(struct
> > > dp_netdev_pmd_thread *pmd,
> > >          flow = dp_netdev_flow_cast(rules[i]);
> > >
> > >          emc_probabilistic_insert(pmd, &keys[i], flow);
> > > -        dp_netdev_queue_batches(packet, flow, &keys[i].mf, batches,
> > > n_batches);
> > > +        dp_netdev_queue_batches(packet, flow,
> > > +                                miniflow_get_tcp_flags(&keys[i].mf),
> > > +                                batches, n_batches);
> > >      }
> > >
> > >      pmd_perf_update_counter(&pmd->perf_stats,
> > PMD_STAT_MASKED_HIT,
> > > diff - -git a/lib/flow.c b/lib/flow.c index 38ff29c..6c74dd3 100644
> > > --- a/lib/flow.c
> > > +++ b/lib/flow.c
> > > @@ -624,6 +624,70 @@ flow_extract(struct dp_packet *packet, struct
> > > flow
> > > *flow)
> > >      miniflow_expand(&m.mf, flow);
> > >  }
> > >
> > > +static inline bool
> > > +ipv4_sanity_check(const struct ip_header *nh, size_t size,
> > > +                  int *ip_lenp, uint16_t *tot_lenp) {
> > > +    int ip_len;
> > > +    uint16_t tot_len;
> > > +
> > > +    if (OVS_UNLIKELY(size < IP_HEADER_LEN)) {
> > > +        return false;
> > > +    }
> > > +    ip_len = IP_IHL(nh->ip_ihl_ver) * 4;
> > > +
> > > +    if (OVS_UNLIKELY(ip_len < IP_HEADER_LEN || size < ip_len)) {
> > > +        return false;
> > > +    }
> > > +
> > > +    tot_len = ntohs(nh->ip_tot_len);
> > > +    if (OVS_UNLIKELY(tot_len > size || ip_len > tot_len ||
> > > +                size - tot_len > UINT8_MAX)) {
> > > +        return false;
> > > +    }
> > > +
> > > +    *ip_lenp = ip_len;
> > > +    *tot_lenp = tot_len;
> > > +
> > > +    return true;
> > > +}
> > > +
> > > +static inline uint8_t
> > > +ipv4_get_nw_frag(const struct ip_header *nh) {
> > > +    uint8_t nw_frag = 0;
> > > +
> > > +    if (OVS_UNLIKELY(IP_IS_FRAGMENT(nh->ip_frag_off))) {
> > > +        nw_frag = FLOW_NW_FRAG_ANY;
> > > +        if (nh->ip_frag_off & htons(IP_FRAG_OFF_MASK)) {
> > > +            nw_frag |= FLOW_NW_FRAG_LATER;
> > > +        }
> > > +    }
> > > +
> > > +    return nw_frag;
> > > +}
> > > +
> > > +static inline bool
> > > +ipv6_sanity_check(const struct ovs_16aligned_ip6_hdr *nh, size_t
> > > +size) {
> > > +    uint16_t plen;
> > > +
> > > +    if (OVS_UNLIKELY(size < sizeof *nh)) {
> > > +        return false;
> > > +    }
> > > +
> > > +    plen = ntohs(nh->ip6_plen);
> > > +    if (OVS_UNLIKELY(plen > size)) {
> > > +        return false;
> > > +    }
> > > +    /* Jumbo Payload option not supported yet. */
> > > +    if (OVS_UNLIKELY(size - plen > UINT8_MAX)) {
> > > +        return false;
> > > +    }
> > > +
> > > +    return true;
> > > +}
> > > +
> > >  /* Caller is responsible for initializing 'dst' with enough storage
> for
> > >   * FLOW_U64S * 8 bytes. */
> > >  void
> > > @@ -748,22 +812,7 @@ miniflow_extract(struct dp_packet *packet,
> > > struct miniflow *dst)
> > >          int ip_len;
> > >          uint16_t tot_len;
> > >
> > > -        if (OVS_UNLIKELY(size < IP_HEADER_LEN)) {
> > > -            goto out;
> > > -        }
> > > -        ip_len = IP_IHL(nh->ip_ihl_ver) * 4;
> > > -
> > > -        if (OVS_UNLIKELY(ip_len < IP_HEADER_LEN)) {
> > > -            goto out;
> > > -        }
> > > -        if (OVS_UNLIKELY(size < ip_len)) {
> > > -            goto out;
> > > -        }
> > > -        tot_len = ntohs(nh->ip_tot_len);
> > > -        if (OVS_UNLIKELY(tot_len > size || ip_len > tot_len)) {
> > > -            goto out;
> > > -        }
> > > -        if (OVS_UNLIKELY(size - tot_len > UINT8_MAX)) {
> > > +        if (OVS_UNLIKELY(!ipv4_sanity_check(nh, size, &ip_len,
> > > + &tot_len))) {
> >
> > You may have missed my comment in the v7 regarding this.
> >
> > There's no mention in commit message of the introduction of
> > IPv4/IPv6/fragmentation sanity checks.
> >
> > This affects minflow extract regardless of HWOL usage. The miniflow
> > extract refactor should  to use these should be introduced as a
> > separate patch to this series so it's clear that it is being modified.
> >
> > >              goto out;
> > >          }
> > >          dp_packet_set_l2_pad_size(packet, size - tot_len); @@
> > > -786,31
> > > +835,19 @@ miniflow_extract(struct dp_packet *packet, struct
> > > +miniflow
> > > *dst)
> > >          nw_tos = nh->ip_tos;
> > >          nw_ttl = nh->ip_ttl;
> > >          nw_proto = nh->ip_proto;
> > > -        if (OVS_UNLIKELY(IP_IS_FRAGMENT(nh->ip_frag_off))) {
> > > -            nw_frag = FLOW_NW_FRAG_ANY;
> > > -            if (nh->ip_frag_off & htons(IP_FRAG_OFF_MASK)) {
> > > -                nw_frag |= FLOW_NW_FRAG_LATER;
> > > -            }
> > > -        }
> > > +        nw_frag = ipv4_get_nw_frag(nh);
> > >          data_pull(&data, &size, ip_len);
> > >      } else if (dl_type == htons(ETH_TYPE_IPV6)) {
> > > -        const struct ovs_16aligned_ip6_hdr *nh;
> > > +        const struct ovs_16aligned_ip6_hdr *nh = data;
> > >          ovs_be32 tc_flow;
> > >          uint16_t plen;
> > >
> > > -        if (OVS_UNLIKELY(size < sizeof *nh)) {
> > > +        if (OVS_UNLIKELY(!ipv6_sanity_check(nh, size))) {
> > >              goto out;
> > >          }
> > > -        nh = data_pull(&data, &size, sizeof *nh);
> > > +        data_pull(&data, &size, sizeof *nh);
> > >
> > >          plen = ntohs(nh->ip6_plen);
> > > -        if (OVS_UNLIKELY(plen > size)) {
> > > -            goto out;
> > > -        }
> > > -        /* Jumbo Payload option not supported yet. */
> > > -        if (OVS_UNLIKELY(size - plen > UINT8_MAX)) {
> > > -            goto out;
> > > -        }
> > >          dp_packet_set_l2_pad_size(packet, size - plen);
> > >          size = plen;   /* Never pull padding. */
> > >
> > > @@ -982,6 +1019,60 @@ parse_dl_type(const struct eth_header *data_,
> > > size_t
> > > size)
> > >      return parse_ethertype(&data, &size);  }
> > >
> > > +uint16_t
> > > +parse_tcp_flags(struct dp_packet *packet) {
> > > +    const void *data = dp_packet_data(packet);
> > > +    size_t size = dp_packet_size(packet);
> > > +    ovs_be16 dl_type;
> > > +    uint8_t nw_frag = 0, nw_proto = 0;
> > > +
> > > +    if (packet->packet_type != htonl(PT_ETH)) {
> > > +        return 0;
> > > +    }
> > > +
> > > +    data_pull(&data, &size, ETH_ADDR_LEN * 2);
> > > +    dl_type = parse_ethertype(&data, &size);
> > > +    if (OVS_LIKELY(dl_type == htons(ETH_TYPE_IP))) {
> > > +        const struct ip_header *nh = data;
> > > +        int ip_len;
> > > +        uint16_t tot_len;
> > > +
> > > +        if (OVS_UNLIKELY(!ipv4_sanity_check(nh, size, &ip_len,
> > > &tot_len))) {
> > > +            return 0;
> > > +        }
> > > +        nw_proto = nh->ip_proto;
> > > +        nw_frag = ipv4_get_nw_frag(nh);
> > > +
> > > +        size = tot_len;   /* Never pull padding. */
> > > +        data_pull(&data, &size, ip_len);
> > > +    } else if (dl_type == htons(ETH_TYPE_IPV6)) {
> > > +        const struct ovs_16aligned_ip6_hdr *nh = data;
> > > +
> > > +        if (OVS_UNLIKELY(!ipv6_sanity_check(nh, size))) {
> > > +            return 0;
> > > +        }
> > > +        data_pull(&data, &size, sizeof *nh);
> > > +
> > > +        size = ntohs(nh->ip6_plen); /* Never pull padding. */
> > > +        if (!parse_ipv6_ext_hdrs__(&data, &size, &nw_proto,
> &nw_frag)) {
> > > +            return 0;
> > > +        }
> > > +        nw_proto = nh->ip6_nxt;
> > > +    } else {
> > > +        return 0;
> > > +    }
> > > +
> > > +    if (!(nw_frag & FLOW_NW_FRAG_LATER) && nw_proto ==
> > IPPROTO_TCP &&
> > > +        size >= TCP_HEADER_LEN) {
> > > +        const struct tcp_header *tcp = data;
> > > +
> > > +        return TCP_FLAGS_BE32(tcp->tcp_ctl);
> >
> > Will cause compilation failure with sparse when compiling without DPDK.
> >
> > lib/flow.c:1070:16: error: incorrect type in return expression
> > (different base
> > types)
> > lib/flow.c:1070:16:    expected unsigned short
> > lib/flow.c:1070:16:    got restricted ovs_be32 [usertype] <noident>
> 
> Thanks,
> 
> Is there a way for me to re-run travis build so I can verify it is fixed?

Sure, Travis can link to a github account. The OVS test doc has steps you can follow to set it up:

http://docs.openvswitch.org/en/latest/topics/testing/#continuous-integration-with-travis-ci

Ian

> 
> >
> > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftra
> > vis-
> > ci.org%2Fistokes%2Fovs%2Fjobs%2F364709985&data=02%7C01%7Cshahafs%
> > 40mellanox.com%7C624a50f6dcd64230e11c08d59f1d6f66%7Ca652971c7d2e4
> > d9ba6a4d149256f461b%7C0%7C0%7C636589871173668041&sdata=bZ6FwLkX
> > B0%2FphhOaQBb6tBjAkNaA8to%2BSWiuK6zdBew%3D&reserved=0
> >
> > > +    }
> > > +
> > > +    return 0;
> > > +}
> > > +
> > >  /* For every bit of a field that is wildcarded in 'wildcards', sets
> the
> > >   * corresponding bit in 'flow' to zero. */  void diff --git
> > > a/lib/flow.h b/lib/flow.h index 770a07a..0adecbf 100644
> > > --- a/lib/flow.h
> > > +++ b/lib/flow.h
> > > @@ -132,6 +132,7 @@ bool parse_ipv6_ext_hdrs(const void **datap,
> > > size_t *sizep, uint8_t *nw_proto,
> > >                           uint8_t *nw_frag);
> > >  ovs_be16 parse_dl_type(const struct eth_header *data_, size_t
> > > size); bool parse_nsh(const void **datap, size_t *sizep, struct
> > > ovs_key_nsh *key);
> > > +uint16_t parse_tcp_flags(struct dp_packet *packet);
> > >
> > >  static inline uint64_t
> > >  flow_get_xreg(const struct flow *flow, int idx)
> > > --
> > > 2.7.4
Shahaf Shuler April 15, 2018, 12:40 p.m. | #4
Thursday, April 12, 2018 4:39 PM, Stokes, Ian:
> Subject: RE: [PATCH v8 2/6] dpif-netdev: retrieve flow directly from the flow
> mark
> > > > +
> > > > +    if (!(nw_frag & FLOW_NW_FRAG_LATER) && nw_proto ==
> > > IPPROTO_TCP &&
> > > > +        size >= TCP_HEADER_LEN) {
> > > > +        const struct tcp_header *tcp = data;
> > > > +
> > > > +        return TCP_FLAGS_BE32(tcp->tcp_ctl);
> > >
> > > Will cause compilation failure with sparse when compiling without DPDK.
> > >
> > > lib/flow.c:1070:16: error: incorrect type in return expression
> > > (different base
> > > types)
> > > lib/flow.c:1070:16:    expected unsigned short
> > > lib/flow.c:1070:16:    got restricted ovs_be32 [usertype] <noident>
> >
> > Thanks,
> >
> > Is there a way for me to re-run travis build so I can verify it is fixed?
> 
> Sure, Travis can link to a github account. The OVS test doc has steps you can
> follow to set it up:
> 
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdoc
> s.openvswitch.org%2Fen%2Flatest%2Ftopics%2Ftesting%2F%23continuous-
> integration-with-travis-
> ci&data=02%7C01%7Cshahafs%40mellanox.com%7C4ab1d89f766f4969a93d0
> 8d5a07aea0c%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636591
> 372144565253&sdata=GnJlyU0Sob%2BQGEODZcfSo%2BOwu0R0YgESLqVd4Ks
> IQyQ%3D&reserved=0
> 

Am getting this error[1] when enabling the sparse compiler. 

I thought it should be included in the Travis builder. 

[1]
https://travis-ci.org/shahafsh/ovs/jobs/366766006
Ben Pfaff April 16, 2018, 6:25 p.m. | #5
On Sun, Apr 15, 2018 at 12:40:28PM +0000, Shahaf Shuler wrote:
> Thursday, April 12, 2018 4:39 PM, Stokes, Ian:
> > Subject: RE: [PATCH v8 2/6] dpif-netdev: retrieve flow directly from the flow
> > mark
> > > > > +
> > > > > +    if (!(nw_frag & FLOW_NW_FRAG_LATER) && nw_proto ==
> > > > IPPROTO_TCP &&
> > > > > +        size >= TCP_HEADER_LEN) {
> > > > > +        const struct tcp_header *tcp = data;
> > > > > +
> > > > > +        return TCP_FLAGS_BE32(tcp->tcp_ctl);
> > > >
> > > > Will cause compilation failure with sparse when compiling without DPDK.
> > > >
> > > > lib/flow.c:1070:16: error: incorrect type in return expression
> > > > (different base
> > > > types)
> > > > lib/flow.c:1070:16:    expected unsigned short
> > > > lib/flow.c:1070:16:    got restricted ovs_be32 [usertype] <noident>
> > >
> > > Thanks,
> > >
> > > Is there a way for me to re-run travis build so I can verify it is fixed?
> > 
> > Sure, Travis can link to a github account. The OVS test doc has steps you can
> > follow to set it up:
> > 
> > https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdoc
> > s.openvswitch.org%2Fen%2Flatest%2Ftopics%2Ftesting%2F%23continuous-
> > integration-with-travis-
> > ci&data=02%7C01%7Cshahafs%40mellanox.com%7C4ab1d89f766f4969a93d0
> > 8d5a07aea0c%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636591
> > 372144565253&sdata=GnJlyU0Sob%2BQGEODZcfSo%2BOwu0R0YgESLqVd4Ks
> > IQyQ%3D&reserved=0
> > 
> 
> Am getting this error[1] when enabling the sparse compiler. 
> 
> I thought it should be included in the Travis builder. 
> 
> [1]
> https://travis-ci.org/shahafsh/ovs/jobs/366766006

The build enables sparse already, see .travis/linux-build.sh.  You don't
need it as a separate compiler.

Patch

diff --git a/lib/dp-packet.h b/lib/dp-packet.h
index 21c8ca5..dd3f17b 100644
--- a/lib/dp-packet.h
+++ b/lib/dp-packet.h
@@ -691,6 +691,19 @@  reset_dp_packet_checksum_ol_flags(struct dp_packet *p)
 #define reset_dp_packet_checksum_ol_flags(arg)
 #endif
 
+static inline bool
+dp_packet_has_flow_mark(struct dp_packet *p OVS_UNUSED,
+                        uint32_t *mark OVS_UNUSED)
+{
+#ifdef DPDK_NETDEV
+    if (p->mbuf.ol_flags & PKT_RX_FDIR_ID) {
+        *mark = p->mbuf.hash.fdir.hi;
+        return true;
+    }
+#endif
+    return false;
+}
+
 enum { NETDEV_MAX_BURST = 32 }; /* Maximum number packets in a batch. */
 
 struct dp_packet_batch {
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 2fdb6ef..7489a2f 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -2015,6 +2015,23 @@  flow_mark_flush(struct dp_netdev_pmd_thread *pmd)
     }
 }
 
+static struct dp_netdev_flow *
+mark_to_flow_find(const struct dp_netdev_pmd_thread *pmd,
+                  const uint32_t mark)
+{
+    struct dp_netdev_flow *flow;
+
+    CMAP_FOR_EACH_WITH_HASH (flow, mark_node, hash_int(mark, 0),
+                             &flow_mark.mark_to_flow) {
+        if (flow->mark == mark && flow->pmd_id == pmd->core_id &&
+            flow->dead == false) {
+            return flow;
+        }
+    }
+
+    return NULL;
+}
+
 static void
 dp_netdev_pmd_remove_flow(struct dp_netdev_pmd_thread *pmd,
                           struct dp_netdev_flow *flow)
@@ -5204,10 +5221,10 @@  struct packet_batch_per_flow {
 static inline void
 packet_batch_per_flow_update(struct packet_batch_per_flow *batch,
                              struct dp_packet *packet,
-                             const struct miniflow *mf)
+                             uint16_t tcp_flags)
 {
     batch->byte_count += dp_packet_size(packet);
-    batch->tcp_flags |= miniflow_get_tcp_flags(mf);
+    batch->tcp_flags |= tcp_flags;
     batch->array.packets[batch->array.count++] = packet;
 }
 
@@ -5241,7 +5258,7 @@  packet_batch_per_flow_execute(struct packet_batch_per_flow *batch,
 
 static inline void
 dp_netdev_queue_batches(struct dp_packet *pkt,
-                        struct dp_netdev_flow *flow, const struct miniflow *mf,
+                        struct dp_netdev_flow *flow, uint16_t tcp_flags,
                         struct packet_batch_per_flow *batches,
                         size_t *n_batches)
 {
@@ -5252,7 +5269,7 @@  dp_netdev_queue_batches(struct dp_packet *pkt,
         packet_batch_per_flow_init(batch, flow);
     }
 
-    packet_batch_per_flow_update(batch, pkt, mf);
+    packet_batch_per_flow_update(batch, pkt, tcp_flags);
 }
 
 /* Try to process all ('cnt') the 'packets' using only the exact match cache
@@ -5283,6 +5300,7 @@  emc_processing(struct dp_netdev_pmd_thread *pmd,
     const size_t cnt = dp_packet_batch_size(packets_);
     uint32_t cur_min;
     int i;
+    uint16_t tcp_flags;
 
     atomic_read_relaxed(&pmd->dp->emc_insert_min, &cur_min);
     pmd_perf_update_counter(&pmd->perf_stats,
@@ -5291,6 +5309,7 @@  emc_processing(struct dp_netdev_pmd_thread *pmd,
 
     DP_PACKET_BATCH_REFILL_FOR_EACH (i, cnt, packet, packets_) {
         struct dp_netdev_flow *flow;
+        uint32_t mark;
 
         if (OVS_UNLIKELY(dp_packet_size(packet) < ETH_HEADER_LEN)) {
             dp_packet_delete(packet);
@@ -5298,6 +5317,16 @@  emc_processing(struct dp_netdev_pmd_thread *pmd,
             continue;
         }
 
+        if (dp_packet_has_flow_mark(packet, &mark)) {
+            flow = mark_to_flow_find(pmd, mark);
+            if (flow) {
+                tcp_flags = parse_tcp_flags(packet);
+                dp_netdev_queue_batches(packet, flow, tcp_flags, batches,
+                                        n_batches);
+                continue;
+            }
+        }
+
         if (i != cnt - 1) {
             struct dp_packet **packets = packets_->packets;
             /* Prefetch next packet data and metadata. */
@@ -5323,7 +5352,8 @@  emc_processing(struct dp_netdev_pmd_thread *pmd,
             flow = NULL;
         }
         if (OVS_LIKELY(flow)) {
-            dp_netdev_queue_batches(packet, flow, &key->mf, batches,
+            tcp_flags = miniflow_get_tcp_flags(&key->mf);
+            dp_netdev_queue_batches(packet, flow, tcp_flags, batches,
                                     n_batches);
         } else {
             /* Exact match cache missed. Group missed packets together at
@@ -5501,7 +5531,9 @@  fast_path_processing(struct dp_netdev_pmd_thread *pmd,
         flow = dp_netdev_flow_cast(rules[i]);
 
         emc_probabilistic_insert(pmd, &keys[i], flow);
-        dp_netdev_queue_batches(packet, flow, &keys[i].mf, batches, n_batches);
+        dp_netdev_queue_batches(packet, flow,
+                                miniflow_get_tcp_flags(&keys[i].mf),
+                                batches, n_batches);
     }
 
     pmd_perf_update_counter(&pmd->perf_stats, PMD_STAT_MASKED_HIT,
diff --git a/lib/flow.c b/lib/flow.c
index 38ff29c..6c74dd3 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -624,6 +624,70 @@  flow_extract(struct dp_packet *packet, struct flow *flow)
     miniflow_expand(&m.mf, flow);
 }
 
+static inline bool
+ipv4_sanity_check(const struct ip_header *nh, size_t size,
+                  int *ip_lenp, uint16_t *tot_lenp)
+{
+    int ip_len;
+    uint16_t tot_len;
+
+    if (OVS_UNLIKELY(size < IP_HEADER_LEN)) {
+        return false;
+    }
+    ip_len = IP_IHL(nh->ip_ihl_ver) * 4;
+
+    if (OVS_UNLIKELY(ip_len < IP_HEADER_LEN || size < ip_len)) {
+        return false;
+    }
+
+    tot_len = ntohs(nh->ip_tot_len);
+    if (OVS_UNLIKELY(tot_len > size || ip_len > tot_len ||
+                size - tot_len > UINT8_MAX)) {
+        return false;
+    }
+
+    *ip_lenp = ip_len;
+    *tot_lenp = tot_len;
+
+    return true;
+}
+
+static inline uint8_t
+ipv4_get_nw_frag(const struct ip_header *nh)
+{
+    uint8_t nw_frag = 0;
+
+    if (OVS_UNLIKELY(IP_IS_FRAGMENT(nh->ip_frag_off))) {
+        nw_frag = FLOW_NW_FRAG_ANY;
+        if (nh->ip_frag_off & htons(IP_FRAG_OFF_MASK)) {
+            nw_frag |= FLOW_NW_FRAG_LATER;
+        }
+    }
+
+    return nw_frag;
+}
+
+static inline bool
+ipv6_sanity_check(const struct ovs_16aligned_ip6_hdr *nh, size_t size)
+{
+    uint16_t plen;
+
+    if (OVS_UNLIKELY(size < sizeof *nh)) {
+        return false;
+    }
+
+    plen = ntohs(nh->ip6_plen);
+    if (OVS_UNLIKELY(plen > size)) {
+        return false;
+    }
+    /* Jumbo Payload option not supported yet. */
+    if (OVS_UNLIKELY(size - plen > UINT8_MAX)) {
+        return false;
+    }
+
+    return true;
+}
+
 /* Caller is responsible for initializing 'dst' with enough storage for
  * FLOW_U64S * 8 bytes. */
 void
@@ -748,22 +812,7 @@  miniflow_extract(struct dp_packet *packet, struct miniflow *dst)
         int ip_len;
         uint16_t tot_len;
 
-        if (OVS_UNLIKELY(size < IP_HEADER_LEN)) {
-            goto out;
-        }
-        ip_len = IP_IHL(nh->ip_ihl_ver) * 4;
-
-        if (OVS_UNLIKELY(ip_len < IP_HEADER_LEN)) {
-            goto out;
-        }
-        if (OVS_UNLIKELY(size < ip_len)) {
-            goto out;
-        }
-        tot_len = ntohs(nh->ip_tot_len);
-        if (OVS_UNLIKELY(tot_len > size || ip_len > tot_len)) {
-            goto out;
-        }
-        if (OVS_UNLIKELY(size - tot_len > UINT8_MAX)) {
+        if (OVS_UNLIKELY(!ipv4_sanity_check(nh, size, &ip_len, &tot_len))) {
             goto out;
         }
         dp_packet_set_l2_pad_size(packet, size - tot_len);
@@ -786,31 +835,19 @@  miniflow_extract(struct dp_packet *packet, struct miniflow *dst)
         nw_tos = nh->ip_tos;
         nw_ttl = nh->ip_ttl;
         nw_proto = nh->ip_proto;
-        if (OVS_UNLIKELY(IP_IS_FRAGMENT(nh->ip_frag_off))) {
-            nw_frag = FLOW_NW_FRAG_ANY;
-            if (nh->ip_frag_off & htons(IP_FRAG_OFF_MASK)) {
-                nw_frag |= FLOW_NW_FRAG_LATER;
-            }
-        }
+        nw_frag = ipv4_get_nw_frag(nh);
         data_pull(&data, &size, ip_len);
     } else if (dl_type == htons(ETH_TYPE_IPV6)) {
-        const struct ovs_16aligned_ip6_hdr *nh;
+        const struct ovs_16aligned_ip6_hdr *nh = data;
         ovs_be32 tc_flow;
         uint16_t plen;
 
-        if (OVS_UNLIKELY(size < sizeof *nh)) {
+        if (OVS_UNLIKELY(!ipv6_sanity_check(nh, size))) {
             goto out;
         }
-        nh = data_pull(&data, &size, sizeof *nh);
+        data_pull(&data, &size, sizeof *nh);
 
         plen = ntohs(nh->ip6_plen);
-        if (OVS_UNLIKELY(plen > size)) {
-            goto out;
-        }
-        /* Jumbo Payload option not supported yet. */
-        if (OVS_UNLIKELY(size - plen > UINT8_MAX)) {
-            goto out;
-        }
         dp_packet_set_l2_pad_size(packet, size - plen);
         size = plen;   /* Never pull padding. */
 
@@ -982,6 +1019,60 @@  parse_dl_type(const struct eth_header *data_, size_t size)
     return parse_ethertype(&data, &size);
 }
 
+uint16_t
+parse_tcp_flags(struct dp_packet *packet)
+{
+    const void *data = dp_packet_data(packet);
+    size_t size = dp_packet_size(packet);
+    ovs_be16 dl_type;
+    uint8_t nw_frag = 0, nw_proto = 0;
+
+    if (packet->packet_type != htonl(PT_ETH)) {
+        return 0;
+    }
+
+    data_pull(&data, &size, ETH_ADDR_LEN * 2);
+    dl_type = parse_ethertype(&data, &size);
+    if (OVS_LIKELY(dl_type == htons(ETH_TYPE_IP))) {
+        const struct ip_header *nh = data;
+        int ip_len;
+        uint16_t tot_len;
+
+        if (OVS_UNLIKELY(!ipv4_sanity_check(nh, size, &ip_len, &tot_len))) {
+            return 0;
+        }
+        nw_proto = nh->ip_proto;
+        nw_frag = ipv4_get_nw_frag(nh);
+
+        size = tot_len;   /* Never pull padding. */
+        data_pull(&data, &size, ip_len);
+    } else if (dl_type == htons(ETH_TYPE_IPV6)) {
+        const struct ovs_16aligned_ip6_hdr *nh = data;
+
+        if (OVS_UNLIKELY(!ipv6_sanity_check(nh, size))) {
+            return 0;
+        }
+        data_pull(&data, &size, sizeof *nh);
+
+        size = ntohs(nh->ip6_plen); /* Never pull padding. */
+        if (!parse_ipv6_ext_hdrs__(&data, &size, &nw_proto, &nw_frag)) {
+            return 0;
+        }
+        nw_proto = nh->ip6_nxt;
+    } else {
+        return 0;
+    }
+
+    if (!(nw_frag & FLOW_NW_FRAG_LATER) && nw_proto == IPPROTO_TCP &&
+        size >= TCP_HEADER_LEN) {
+        const struct tcp_header *tcp = data;
+
+        return TCP_FLAGS_BE32(tcp->tcp_ctl);
+    }
+
+    return 0;
+}
+
 /* For every bit of a field that is wildcarded in 'wildcards', sets the
  * corresponding bit in 'flow' to zero. */
 void
diff --git a/lib/flow.h b/lib/flow.h
index 770a07a..0adecbf 100644
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -132,6 +132,7 @@  bool parse_ipv6_ext_hdrs(const void **datap, size_t *sizep, uint8_t *nw_proto,
                          uint8_t *nw_frag);
 ovs_be16 parse_dl_type(const struct eth_header *data_, size_t size);
 bool parse_nsh(const void **datap, size_t *sizep, struct ovs_key_nsh *key);
+uint16_t parse_tcp_flags(struct dp_packet *packet);
 
 static inline uint64_t
 flow_get_xreg(const struct flow *flow, int idx)