[ovs-dev] [patch v2 5/5] conntrack: Optimize recirculations.

Aaron Conole aconole at redhat.com
Thu Nov 29 19:25:11 UTC 2018


Darrell Ball <dlu998 at gmail.com> writes:

> On Wed, Nov 28, 2018 at 1:59 PM Aaron Conole <aconole at redhat.com> wrote:
>
>  Darrell Ball <dlu998 at gmail.com> writes:
>
>  > In most cases, recirculations through conntrack can be much less costly.
>  >
>  > Signed-off-by: Darrell Ball <dlu998 at gmail.com>
>  > ---
>  >  lib/conntrack.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++---
>  >  lib/packets.h   |  4 ++++
>  >  2 files changed, 51 insertions(+), 3 deletions(-)
>  >
>  > diff --git a/lib/conntrack.c b/lib/conntrack.c
>  > index 9d10b14..f9c4d90 100644
>  > --- a/lib/conntrack.c
>  > +++ b/lib/conntrack.c
>  > @@ -51,6 +51,7 @@ struct conn_lookup_ctx {
>  >      uint32_t hash;
>  >      bool reply;
>  >      bool icmp_related;
>  > +    bool nat;
>
>  Is this already available via testing for conn->nat_info?  Maybe I'm
>  misunderstanding something.  Probably I am.
>
> conn can be NULL at this point, struct conn_lookup_ctx is a parameter type and overall size was unaffected.
> It was just for convenience to avoid (conn && conn->nat_info) syntax; nothing more.
> I can remove it and replace with parameter check on nat_action_info to achieve almost same effect.

No problem, I thought conn was already valid in all the cases it was
used, but I probably missed some.

>  >  };
>  >  
>  >  enum ftp_ctl_pkt {
>  > @@ -954,7 +955,7 @@ conn_not_found(struct dp_packet *pkt, struct conn_lookup_ctx *ctx,
>  >              }
>  >              nc->ext_nat->nat_conn = nat_conn;
>  >              nat_packet(pkt, nc, ctx->icmp_related);
>  > -
>  > +            ctx->nat = true;
>  >              nat_conn->key = nc->rev_key;
>  >              nat_conn->rev_key = nc->key;
>  >              nat_conn->conn_type = CT_CONN_TYPE_UN_NAT;
>  > @@ -1193,6 +1194,7 @@ process_one(struct dp_packet *pkt, struct conn_lookup_ctx *ctx, uint16_t
>  zone,
>  >          }
>  >          if (nat_action_info && !create_new_conn) {
>  >              handle_nat(pkt, conn, zone, ctx->reply, ctx->icmp_related);
>  > +            ctx->nat = true;
>  >          }
>  >      } else if (check_orig_tuple(pkt, ctx, now, &conn, nat_action_info)) {
>  >          create_new_conn = conn_update_state(pkt, ctx, conn, now);
>  > @@ -1237,6 +1239,39 @@ process_one(struct dp_packet *pkt, struct conn_lookup_ctx *ctx, uint16_t
>  zone,
>  >      }
>  >  
>  >      handle_alg_ctl(ctx, pkt, ct_alg_ctl, conn, now, !!nat_action_info);
>  > +
>  > +    if (!ctx->nat && ct_alg_ctl == CT_ALG_CTL_NONE) {
>  > +        pkt->md.conn = conn;
>  > +        pkt->md.reply = ctx->reply;
>  > +        pkt->md.icmp_related = ctx->icmp_related;
>  > +    } else {
>  > +        pkt->md.conn = NULL;
>  > +    }
>  > +}
>  > +
>  > +static inline void
>  > +process_one_fast(struct dp_packet *pkt, uint16_t zone,
>  > +                 const uint32_t *setmark,
>  > +                 const struct ovs_key_ct_labels *setlabel,
>  > +                 const struct nat_action_info_t *nat_action_info,
>  > +                 struct conn *conn)
>  > +{
>  > +    if (nat_action_info) {
>  > +        handle_nat(pkt, conn, zone, pkt->md.reply, pkt->md.icmp_related);
>  > +        pkt->md.conn = NULL;
>  > +    }
>  > +
>  > +    pkt->md.ct_zone = zone;
>  > +    pkt->md.ct_mark = conn->mark;
>  > +    pkt->md.ct_label = conn->label;
>  > +
>  > +    if (setmark) {
>  > +        set_mark(pkt, conn, setmark[0], setmark[1]);
>  > +    }
>  > +
>  > +    if (setlabel) {
>  > +        set_label(pkt, conn, &setlabel[0], &setlabel[1]);
>  > +    }
>  >  }
>  >  
>  >  /* Sends the packets in '*pkt_batch' through the connection tracker 'ct'.  All
>  > @@ -1260,8 +1295,16 @@ conntrack_execute(struct dp_packet_batch *pkt_batch, ovs_be16 dl_type,
>  >      struct conn_lookup_ctx ctx;
>  >  
>  >      DP_PACKET_BATCH_FOR_EACH (i, packet, pkt_batch) {
>  > -        if (packet->md.ct_state == CS_INVALID
>  > -            || !conn_key_extract(packet, dl_type, &ctx, zone)) {
>  > +        struct conn *conn = packet->md.conn;
>  > +        if (OVS_UNLIKELY(packet->md.ct_state == CS_INVALID)) {
>  > +            write_ct_md(packet, zone, NULL, NULL, NULL);
>  > +                continue;
>  > +        } else if (conn && !force && !commit && conn->key.zone == zone) {
>  > +            process_one_fast(packet, zone, setmark, setlabel, nat_action_info,
>  > +                             packet->md.conn);
>  > +            continue;
>  > +        } else if (OVS_UNLIKELY(!conn_key_extract(packet, dl_type, &ctx,
>  > +                                zone))) {
>  >              packet->md.ct_state = CS_INVALID;
>  >              write_ct_md(packet, zone, NULL, NULL, NULL);
>  >              continue;
>  > @@ -1279,6 +1322,7 @@ conntrack_clear(struct dp_packet *packet)
>  >      /* According to pkt_metadata_init(), ct_state == 0 is enough to make all of
>  >       * the conntrack fields invalid. */
>  >      packet->md.ct_state = 0;
>  > +    packet->md.conn = NULL;
>  >  }
>  >  
>  >  static void
>  > diff --git a/lib/packets.h b/lib/packets.h
>  > index 09a0ac3..a88d1ad 100644
>  > --- a/lib/packets.h
>  > +++ b/lib/packets.h
>  > @@ -108,6 +108,9 @@ PADDED_MEMBERS_CACHELINE_MARKER(CACHE_LINE_SIZE, cacheline0,
>  >      uint32_t ct_mark;           /* Connection mark. */
>  >      ovs_u128 ct_label;          /* Connection label. */
>  >      union flow_in_port in_port; /* Input port. */
>  > +    void *conn;
>  > +    bool reply;
>  > +    bool icmp_related;
>  >  );
>  >  
>  >  PADDED_MEMBERS_CACHELINE_MARKER(CACHE_LINE_SIZE, cacheline1,
>  > @@ -157,6 +160,7 @@ pkt_metadata_init(struct pkt_metadata *md, odp_port_t port)
>  >      md->tunnel.ip_dst = 0;
>  >      md->tunnel.ipv6_dst = in6addr_any;
>  >      md->in_port.odp_port = port;
>  > +    md->conn = NULL;
>  >  }
>  >  
>  >  /* This function prefetches the cachelines touched by pkt_metadata_init()


More information about the dev mailing list