| Message ID | 1543422714-100901-6-git-send-email-dlu998@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | conntrack: Optimize and refactor. | expand |
Darrell Ball <dlu998@gmail.com> writes: > In most cases, recirculations through conntrack can be much less costly. > > Signed-off-by: Darrell Ball <dlu998@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. > }; > > 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()
On Wed, Nov 28, 2018 at 1:59 PM Aaron Conole <aconole@redhat.com> wrote: > Darrell Ball <dlu998@gmail.com> writes: > > > In most cases, recirculations through conntrack can be much less costly. > > > > Signed-off-by: Darrell Ball <dlu998@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. > > }; > > > > 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() >
Darrell Ball <dlu998@gmail.com> writes: > On Wed, Nov 28, 2018 at 1:59 PM Aaron Conole <aconole@redhat.com> wrote: > > Darrell Ball <dlu998@gmail.com> writes: > > > In most cases, recirculations through conntrack can be much less costly. > > > > Signed-off-by: Darrell Ball <dlu998@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()
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; }; 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()
In most cases, recirculations through conntrack can be much less costly. Signed-off-by: Darrell Ball <dlu998@gmail.com> --- lib/conntrack.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++--- lib/packets.h | 4 ++++ 2 files changed, 51 insertions(+), 3 deletions(-)