[ovs-dev] [PATCH] conntrack: remove the nat_action_info from the conn

Michael Santana msantana at redhat.com
Tue Aug 10 13:32:39 UTC 2021


On Mon, Aug 9, 2021 at 11:41 PM wenxu <wenxu at ucloud.cn> wrote:
>
>
>
>
>
>
>
>
> From: Michael Santana <msantana at redhat.com>
> Date: 2021-08-09 23:17:06
> To:  wenxu at ucloud.cn
> Cc:  blp at ovn.org,Ilya Maximets <i.maximets at ovn.org>,dlu998 at gmail.com,dev at openvswitch.org
> Subject: Re: [ovs-dev] [PATCH] conntrack: remove the nat_action_info from the conn>nat_info->nat_action
> >
> >On Wed, Aug 4, 2021 at 11:20 PM <wenxu at ucloud.cn> wrote:
> >>
> >> From: wenxu <wenxu at ucloud.cn>
> >>
> >> Only the nat_action in the nat_action_info is used for conn
> >> packet forward and other item such as min/max_ip/port only
> >> used for the new conn operation. So the whole nat_ction_info
> >> no need store in conn. This will also avoid unnecessary memory
> >> allocation.
> >This seems like a good change. There are like 20 or so instances of
> >nat_info->nat_action compared to the 6 or so instances of
> >min/max/ip/port that I could find.
> >Seems you covered all the instances that needed to be changed which is
> >good. The one thing that is not clear to me is why you changed certain
> >variable declarations to const.
>
> The nat_action_info pass to the nat_get_unique_tuple is const one, The functions
> get the data from nat_action_info also should be const one. Or there are some compile
> warnning.

Ok, makes sense

Acked-By Michael Santana <msantana at redhat.com>
>
> >
> >Otherwise LGTM
> >>
> >> Signed-off-by: wenxu <wenxu at ucloud.cn>
> >> ---
> >>  lib/conntrack-private.h |   2 +-
> >>  lib/conntrack.c         | 101 +++++++++++++++++++++++++-----------------------
> >>  2 files changed, 53 insertions(+), 50 deletions(-)
> >>
> >> diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
> >> index 169ace5..dfdf4e6 100644
> >> --- a/lib/conntrack-private.h
> >> +++ b/lib/conntrack-private.h
> >> @@ -98,7 +98,7 @@ struct conn {
> >>      struct conn_key parent_key; /* Only used for orig_tuple support. */
> >>      struct ovs_list exp_node;
> >>      struct cmap_node cm_node;
> >> -    struct nat_action_info_t *nat_info;
> >> +    uint16_t nat_action;
> >>      char *alg;
> >>      struct conn *nat_conn; /* The NAT 'conn' context, if there is one. */
> >>
> >> diff --git a/lib/conntrack.c b/lib/conntrack.c
> >> index 551c206..33a1a92 100644
> >> --- a/lib/conntrack.c
> >> +++ b/lib/conntrack.c
> >> @@ -111,7 +111,8 @@ static void *clean_thread_main(void *f_);
> >>
> >>  static bool
> >>  nat_get_unique_tuple(struct conntrack *ct, const struct conn *conn,
> >> -                     struct conn *nat_conn);
> >> +                     struct conn *nat_conn,
> >> +                     const struct nat_action_info_t *nat_info);
> >>
> >>  static uint8_t
> >>  reverse_icmp_type(uint8_t type);
> >> @@ -724,7 +725,7 @@ handle_alg_ctl(struct conntrack *ct, const struct conn_lookup_ctx *ctx,
> >>  static void
> >>  pat_packet(struct dp_packet *pkt, const struct conn *conn)
> >>  {
> >> -    if (conn->nat_info->nat_action & NAT_ACTION_SRC) {
> >> +    if (conn->nat_action & NAT_ACTION_SRC) {
> >>          if (conn->key.nw_proto == IPPROTO_TCP) {
> >>              struct tcp_header *th = dp_packet_l4(pkt);
> >>              packet_set_tcp_port(pkt, conn->rev_key.dst.port, th->tcp_dst);
> >> @@ -732,7 +733,7 @@ pat_packet(struct dp_packet *pkt, const struct conn *conn)
> >>              struct udp_header *uh = dp_packet_l4(pkt);
> >>              packet_set_udp_port(pkt, conn->rev_key.dst.port, uh->udp_dst);
> >>          }
> >> -    } else if (conn->nat_info->nat_action & NAT_ACTION_DST) {
> >> +    } else if (conn->nat_action & NAT_ACTION_DST) {
> >>          if (conn->key.nw_proto == IPPROTO_TCP) {
> >>              packet_set_tcp_port(pkt, conn->rev_key.dst.port,
> >>                                  conn->rev_key.src.port);
> >> @@ -746,7 +747,7 @@ pat_packet(struct dp_packet *pkt, const struct conn *conn)
> >>  static void
> >>  nat_packet(struct dp_packet *pkt, const struct conn *conn, bool related)
> >>  {
> >> -    if (conn->nat_info->nat_action & NAT_ACTION_SRC) {
> >> +    if (conn->nat_action & NAT_ACTION_SRC) {
> >>          pkt->md.ct_state |= CS_SRC_NAT;
> >>          if (conn->key.dl_type == htons(ETH_TYPE_IP)) {
> >>              struct ip_header *nh = dp_packet_l3(pkt);
> >> @@ -761,7 +762,7 @@ nat_packet(struct dp_packet *pkt, const struct conn *conn, bool related)
> >>          if (!related) {
> >>              pat_packet(pkt, conn);
> >>          }
> >> -    } else if (conn->nat_info->nat_action & NAT_ACTION_DST) {
> >> +    } else if (conn->nat_action & NAT_ACTION_DST) {
> >>          pkt->md.ct_state |= CS_DST_NAT;
> >>          if (conn->key.dl_type == htons(ETH_TYPE_IP)) {
> >>              struct ip_header *nh = dp_packet_l3(pkt);
> >> @@ -782,7 +783,7 @@ nat_packet(struct dp_packet *pkt, const struct conn *conn, bool related)
> >>  static void
> >>  un_pat_packet(struct dp_packet *pkt, const struct conn *conn)
> >>  {
> >> -    if (conn->nat_info->nat_action & NAT_ACTION_SRC) {
> >> +    if (conn->nat_action & NAT_ACTION_SRC) {
> >>          if (conn->key.nw_proto == IPPROTO_TCP) {
> >>              struct tcp_header *th = dp_packet_l4(pkt);
> >>              packet_set_tcp_port(pkt, th->tcp_src, conn->key.src.port);
> >> @@ -790,7 +791,7 @@ un_pat_packet(struct dp_packet *pkt, const struct conn *conn)
> >>              struct udp_header *uh = dp_packet_l4(pkt);
> >>              packet_set_udp_port(pkt, uh->udp_src, conn->key.src.port);
> >>          }
> >> -    } else if (conn->nat_info->nat_action & NAT_ACTION_DST) {
> >> +    } else if (conn->nat_action & NAT_ACTION_DST) {
> >>          if (conn->key.nw_proto == IPPROTO_TCP) {
> >>              packet_set_tcp_port(pkt, conn->key.dst.port, conn->key.src.port);
> >>          } else if (conn->key.nw_proto == IPPROTO_UDP) {
> >> @@ -802,7 +803,7 @@ un_pat_packet(struct dp_packet *pkt, const struct conn *conn)
> >>  static void
> >>  reverse_pat_packet(struct dp_packet *pkt, const struct conn *conn)
> >>  {
> >> -    if (conn->nat_info->nat_action & NAT_ACTION_SRC) {
> >> +    if (conn->nat_action & NAT_ACTION_SRC) {
> >>          if (conn->key.nw_proto == IPPROTO_TCP) {
> >>              struct tcp_header *th_in = dp_packet_l4(pkt);
> >>              packet_set_tcp_port(pkt, conn->key.src.port,
> >> @@ -812,7 +813,7 @@ reverse_pat_packet(struct dp_packet *pkt, const struct conn *conn)
> >>              packet_set_udp_port(pkt, conn->key.src.port,
> >>                                  uh_in->udp_dst);
> >>          }
> >> -    } else if (conn->nat_info->nat_action & NAT_ACTION_DST) {
> >> +    } else if (conn->nat_action & NAT_ACTION_DST) {
> >>          if (conn->key.nw_proto == IPPROTO_TCP) {
> >>              packet_set_tcp_port(pkt, conn->key.src.port,
> >>                                  conn->key.dst.port);
> >> @@ -844,10 +845,10 @@ reverse_nat_packet(struct dp_packet *pkt, const struct conn *conn)
> >>          pkt->l3_ofs += (char *) inner_l3 - (char *) nh;
> >>          pkt->l4_ofs += inner_l4 - (char *) icmp;
> >>
> >> -        if (conn->nat_info->nat_action & NAT_ACTION_SRC) {
> >> +        if (conn->nat_action & NAT_ACTION_SRC) {
> >>              packet_set_ipv4_addr(pkt, &inner_l3->ip_src,
> >>                                   conn->key.src.addr.ipv4);
> >> -        } else if (conn->nat_info->nat_action & NAT_ACTION_DST) {
> >> +        } else if (conn->nat_action & NAT_ACTION_DST) {
> >>              packet_set_ipv4_addr(pkt, &inner_l3->ip_dst,
> >>                                   conn->key.dst.addr.ipv4);
> >>          }
> >> @@ -868,11 +869,11 @@ reverse_nat_packet(struct dp_packet *pkt, const struct conn *conn)
> >>          pkt->l3_ofs += (char *) inner_l3_6 - (char *) nh6;
> >>          pkt->l4_ofs += inner_l4 - (char *) icmp6;
> >>
> >> -        if (conn->nat_info->nat_action & NAT_ACTION_SRC) {
> >> +        if (conn->nat_action & NAT_ACTION_SRC) {
> >>              packet_set_ipv6_addr(pkt, conn->key.nw_proto,
> >>                                   inner_l3_6->ip6_src.be32,
> >>                                   &conn->key.src.addr.ipv6, true);
> >> -        } else if (conn->nat_info->nat_action & NAT_ACTION_DST) {
> >> +        } else if (conn->nat_action & NAT_ACTION_DST) {
> >>              packet_set_ipv6_addr(pkt, conn->key.nw_proto,
> >>                                   inner_l3_6->ip6_dst.be32,
> >>                                   &conn->key.dst.addr.ipv6, true);
> >> @@ -890,7 +891,7 @@ static void
> >>  un_nat_packet(struct dp_packet *pkt, const struct conn *conn,
> >>                bool related)
> >>  {
> >> -    if (conn->nat_info->nat_action & NAT_ACTION_SRC) {
> >> +    if (conn->nat_action & NAT_ACTION_SRC) {
> >>          pkt->md.ct_state |= CS_DST_NAT;
> >>          if (conn->key.dl_type == htons(ETH_TYPE_IP)) {
> >>              struct ip_header *nh = dp_packet_l3(pkt);
> >> @@ -908,7 +909,7 @@ un_nat_packet(struct dp_packet *pkt, const struct conn *conn,
> >>          } else {
> >>              un_pat_packet(pkt, conn);
> >>          }
> >> -    } else if (conn->nat_info->nat_action & NAT_ACTION_DST) {
> >> +    } else if (conn->nat_action & NAT_ACTION_DST) {
> >>          pkt->md.ct_state |= CS_SRC_NAT;
> >>          if (conn->key.dl_type == htons(ETH_TYPE_IP)) {
> >>              struct ip_header *nh = dp_packet_l3(pkt);
> >> @@ -1018,20 +1019,21 @@ conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
> >>          }
> >>
> >>          if (nat_action_info) {
> >> -            nc->nat_info = xmemdup(nat_action_info, sizeof *nc->nat_info);
> >> +            nc->nat_action = nat_action_info->nat_action;
> >>              nat_conn = xzalloc(sizeof *nat_conn);
> >>
> >>              if (alg_exp) {
> >>                  if (alg_exp->nat_rpl_dst) {
> >>                      nc->rev_key.dst.addr = alg_exp->alg_nat_repl_addr;
> >> -                    nc->nat_info->nat_action = NAT_ACTION_SRC;
> >> +                    nc->nat_action = NAT_ACTION_SRC;
> >>                  } else {
> >>                      nc->rev_key.src.addr = alg_exp->alg_nat_repl_addr;
> >> -                    nc->nat_info->nat_action = NAT_ACTION_DST;
> >> +                    nc->nat_action = NAT_ACTION_DST;
> >>                  }
> >>              } else {
> >>                  memcpy(nat_conn, nc, sizeof *nat_conn);
> >> -                bool nat_res = nat_get_unique_tuple(ct, nc, nat_conn);
> >> +                bool nat_res = nat_get_unique_tuple(ct, nc, nat_conn,
> >> +                                                    nat_action_info);
> >>
> >>                  if (!nat_res) {
> >>                      goto nat_res_exhaustion;
> >> @@ -1046,7 +1048,7 @@ conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
> >>              memcpy(&nat_conn->key, &nc->rev_key, sizeof nat_conn->key);
> >>              memcpy(&nat_conn->rev_key, &nc->key, sizeof nat_conn->rev_key);
> >>              nat_conn->conn_type = CT_CONN_TYPE_UN_NAT;
> >> -            nat_conn->nat_info = NULL;
> >> +            nat_conn->nat_action = 0;
> >>              nat_conn->alg = NULL;
> >>              nat_conn->nat_conn = NULL;
> >>              uint32_t nat_hash = conn_key_hash(&nat_conn->key, ct->hash_basis);
> >> @@ -1138,7 +1140,7 @@ static void
> >>  handle_nat(struct dp_packet *pkt, struct conn *conn,
> >>             uint16_t zone, bool reply, bool related)
> >>  {
> >> -    if (conn->nat_info &&
> >> +    if (conn->nat_action &&
> >>          (!(pkt->md.ct_state & (CS_SRC_NAT | CS_DST_NAT)) ||
> >>            (pkt->md.ct_state & (CS_SRC_NAT | CS_DST_NAT) &&
> >>             zone != pkt->md.ct_zone))) {
> >> @@ -2165,12 +2167,13 @@ conn_key_reverse(struct conn_key *key)
> >>  }
> >>
> >>  static uint32_t
> >> -nat_ipv6_addrs_delta(struct in6_addr *ipv6_min, struct in6_addr *ipv6_max)
> >> +nat_ipv6_addrs_delta(const struct in6_addr *ipv6_min,
> >> +                     const struct in6_addr *ipv6_max)
> >>  {
> >> -    uint8_t *ipv6_min_hi = &ipv6_min->s6_addr[0];
> >> -    uint8_t *ipv6_min_lo = &ipv6_min->s6_addr[0] +  sizeof(uint64_t);
> >> -    uint8_t *ipv6_max_hi = &ipv6_max->s6_addr[0];
> >> -    uint8_t *ipv6_max_lo = &ipv6_max->s6_addr[0] + sizeof(uint64_t);
> >> +    const uint8_t *ipv6_min_hi = &ipv6_min->s6_addr[0];
> >> +    const uint8_t *ipv6_min_lo = &ipv6_min->s6_addr[0] +  sizeof(uint64_t);
> >> +    const uint8_t *ipv6_max_hi = &ipv6_max->s6_addr[0];
> >> +    const uint8_t *ipv6_max_lo = &ipv6_max->s6_addr[0] + sizeof(uint64_t);
> >>
> >>      ovs_be64 addr6_64_min_hi;
> >>      ovs_be64 addr6_64_min_lo;
> >> @@ -2231,15 +2234,16 @@ nat_ipv6_addr_increment(struct in6_addr *ipv6, uint32_t increment)
> >>  }
> >>
> >>  static uint32_t
> >> -nat_range_hash(const struct conn *conn, uint32_t basis)
> >> +nat_range_hash(const struct conn *conn, uint32_t basis,
> >> +               const struct nat_action_info_t *nat_info)
> >>  {
> >>      uint32_t hash = basis;
> >>
> >> -    hash = ct_addr_hash_add(hash, &conn->nat_info->min_addr);
> >> -    hash = ct_addr_hash_add(hash, &conn->nat_info->max_addr);
> >> +    hash = ct_addr_hash_add(hash, &nat_info->min_addr);
> >> +    hash = ct_addr_hash_add(hash, &nat_info->max_addr);
> >>      hash = hash_add(hash,
> >> -                    (conn->nat_info->max_port << 16)
> >> -                    | conn->nat_info->min_port);
> >> +                    (nat_info->max_port << 16)
> >> +                    | nat_info->min_port);
> >>      hash = ct_endpoint_hash_add(hash, &conn->key.src);
> >>      hash = ct_endpoint_hash_add(hash, &conn->key.dst);
> >>      hash = hash_add(hash, (OVS_FORCE uint32_t) conn->key.dl_type);
> >> @@ -2254,7 +2258,7 @@ nat_range_hash(const struct conn *conn, uint32_t basis)
> >>
> >>  /* Ports are stored in host byte order for convenience. */
> >>  static void
> >> -set_sport_range(struct nat_action_info_t *ni, const struct conn_key *k,
> >> +set_sport_range(const struct nat_action_info_t *ni, const struct conn_key *k,
> >>                  uint32_t hash, uint16_t *curr, uint16_t *min,
> >>                  uint16_t *max)
> >>  {
> >> @@ -2271,7 +2275,7 @@ set_sport_range(struct nat_action_info_t *ni, const struct conn_key *k,
> >>  }
> >>
> >>  static void
> >> -set_dport_range(struct nat_action_info_t *ni, const struct conn_key *k,
> >> +set_dport_range(const struct nat_action_info_t *ni, const struct conn_key *k,
> >>                  uint32_t hash, uint16_t *curr, uint16_t *min,
> >>                  uint16_t *max)
> >>  {
> >> @@ -2312,15 +2316,16 @@ get_addr_in_range(union ct_addr *min, union ct_addr *max,
> >>  static void
> >>  get_initial_addr(const struct conn *conn, union ct_addr *min,
> >>                   union ct_addr *max, union ct_addr *curr,
> >> -                 uint32_t hash, bool ipv4)
> >> +                 uint32_t hash, bool ipv4,
> >> +                 const struct nat_action_info_t *nat_info)
> >>  {
> >>      const union ct_addr zero_ip = {0};
> >>
> >>      /* All-zero case. */
> >>      if (!memcmp(min, &zero_ip, sizeof *min)) {
> >> -        if (conn->nat_info->nat_action & NAT_ACTION_SRC) {
> >> +        if (nat_info->nat_action & NAT_ACTION_SRC) {
> >>              *curr = conn->key.src.addr;
> >> -        } else if (conn->nat_info->nat_action & NAT_ACTION_DST) {
> >> +        } else if (nat_info->nat_action & NAT_ACTION_DST) {
> >>              *curr = conn->key.dst.addr;
> >>          }
> >>      } else {
> >> @@ -2405,34 +2410,35 @@ next_addr_in_range_guarded(union ct_addr *curr, union ct_addr *min,
> >>   * If none can be found, return exhaustion to the caller. */
> >>  static bool
> >>  nat_get_unique_tuple(struct conntrack *ct, const struct conn *conn,
> >> -                     struct conn *nat_conn)
> >> +                     struct conn *nat_conn,
> >> +                     const struct nat_action_info_t *nat_info)
> >>  {
> >>      union ct_addr min_addr = {0}, max_addr = {0}, curr_addr = {0},
> >>                    guard_addr = {0};
> >> -    uint32_t hash = nat_range_hash(conn, ct->hash_basis);
> >> +    uint32_t hash = nat_range_hash(conn, ct->hash_basis, nat_info);
> >>      bool pat_proto = conn->key.nw_proto == IPPROTO_TCP ||
> >>                       conn->key.nw_proto == IPPROTO_UDP;
> >>      uint16_t min_dport, max_dport, curr_dport;
> >>      uint16_t min_sport, max_sport, curr_sport;
> >>
> >> -    min_addr = conn->nat_info->min_addr;
> >> -    max_addr = conn->nat_info->max_addr;
> >> +    min_addr = nat_info->min_addr;
> >> +    max_addr = nat_info->max_addr;
> >>
> >>      get_initial_addr(conn, &min_addr, &max_addr, &curr_addr, hash,
> >> -                     (conn->key.dl_type == htons(ETH_TYPE_IP)));
> >> +                     (conn->key.dl_type == htons(ETH_TYPE_IP)), nat_info);
> >>
> >>      /* Save the address we started from so that
> >>       * we can stop once we reach it. */
> >>      guard_addr = curr_addr;
> >>
> >> -    set_sport_range(conn->nat_info, &conn->key, hash, &curr_sport,
> >> +    set_sport_range(nat_info, &conn->key, hash, &curr_sport,
> >>                      &min_sport, &max_sport);
> >> -    set_dport_range(conn->nat_info, &conn->key, hash, &curr_dport,
> >> +    set_dport_range(nat_info, &conn->key, hash, &curr_dport,
> >>                      &min_dport, &max_dport);
> >>
> >>  another_round:
> >>      store_addr_to_key(&curr_addr, &nat_conn->rev_key,
> >> -                      conn->nat_info->nat_action);
> >> +                      nat_info->nat_action);
> >>
> >>      if (!pat_proto) {
> >>          if (!conn_lookup(ct, &nat_conn->rev_key,
> >> @@ -2506,7 +2512,6 @@ new_conn(struct conntrack *ct, struct dp_packet *pkt, struct conn_key *key,
> >>  static void
> >>  delete_conn_cmn(struct conn *conn)
> >>  {
> >> -    free(conn->nat_info);
> >>      free(conn->alg);
> >>      free(conn);
> >>  }
> >> @@ -2883,8 +2888,7 @@ expectation_create(struct conntrack *ct, ovs_be16 dst_port,
> >>          alg_exp_node->nat_rpl_dst = true;
> >>          if (skip_nat) {
> >>              alg_nat_repl_addr = dst_addr;
> >> -        } else if (parent_conn->nat_info &&
> >> -                   parent_conn->nat_info->nat_action & NAT_ACTION_DST) {
> >> +        } else if (parent_conn->nat_action & NAT_ACTION_DST) {
> >>              alg_nat_repl_addr = parent_conn->rev_key.src.addr;
> >>              alg_exp_node->nat_rpl_dst = false;
> >>          } else {
> >> @@ -2896,8 +2900,7 @@ expectation_create(struct conntrack *ct, ovs_be16 dst_port,
> >>          alg_exp_node->nat_rpl_dst = false;
> >>          if (skip_nat) {
> >>              alg_nat_repl_addr = src_addr;
> >> -        } else if (parent_conn->nat_info &&
> >> -                   parent_conn->nat_info->nat_action & NAT_ACTION_DST) {
> >> +        } else if (parent_conn->nat_action & NAT_ACTION_DST) {
> >>              alg_nat_repl_addr = parent_conn->key.dst.addr;
> >>              alg_exp_node->nat_rpl_dst = true;
> >>          } else {
> >> --
> >> 1.8.3.1
> >>
> >> _______________________________________________
> >> dev mailing list
> >> dev at openvswitch.org
> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >>
> >
>
>



More information about the dev mailing list