[ovs-dev,v2,1/2] conntrack: remove the IP iterations in nat_get_unique_l4

From: wenxu <wenxu@chinatelecom.cn>

Removing the IP iterations, and just picking the IP address
with the hash base on the least-used src-ip/dst-ip/proto triple.

Signed-off-by: wenxu <wenxu@chinatelecom.cn>
---
 lib/conntrack.c | 80 ++++++++-------------------------------------------------
 1 file changed, 10 insertions(+), 70 deletions(-)

wenxu@chinatelecom.cn writes:

> From: wenxu <wenxu@chinatelecom.cn>
>
> Removing the IP iterations, and just picking the IP address
> with the hash base on the least-used src-ip/dst-ip/proto triple.
>
> Signed-off-by: wenxu <wenxu@chinatelecom.cn>
> ---
>  lib/conntrack.c | 80 ++++++++-------------------------------------------------
>  1 file changed, 10 insertions(+), 70 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 08da4dd..5556aad 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -2322,10 +2322,10 @@ 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,
> -                 const struct nat_action_info_t *nat_info)
> +find_addr(const struct conn *conn, union ct_addr *min,
> +          union ct_addr *max, union ct_addr *curr,
> +          uint32_t hash, bool ipv4,
> +          const struct nat_action_info_t *nat_info)
>  {
>      const union ct_addr zero_ip = {0};
>  
> @@ -2352,51 +2352,6 @@ store_addr_to_key(union ct_addr *addr, struct conn_key *key,
>      }
>  }
>  
> -static void
> -next_addr_in_range(union ct_addr *curr, union ct_addr *min,
> -                   union ct_addr *max, bool ipv4)
> -{
> -    if (ipv4) {
> -        /* This check could be unified with IPv6, but let's avoid
> -         * an unneeded memcmp() in case of IPv4. */
> -        if (min->ipv4 == max->ipv4) {
> -            return;
> -        }
> -
> -        curr->ipv4 = (curr->ipv4 == max->ipv4) ? min->ipv4
> -                                               : htonl(ntohl(curr->ipv4) + 1);
> -    } else {
> -        if (!memcmp(min, max, sizeof *min)) {
> -            return;
> -        }
> -
> -        if (!memcmp(curr, max, sizeof *curr)) {
> -            *curr = *min;
> -            return;
> -        }
> -
> -        nat_ipv6_addr_increment(&curr->ipv6, 1);
> -    }
> -}
> -
> -static bool
> -next_addr_in_range_guarded(union ct_addr *curr, union ct_addr *min,
> -                           union ct_addr *max, union ct_addr *guard,
> -                           bool ipv4)
> -{
> -    bool exhausted;
> -
> -    next_addr_in_range(curr, min, max, ipv4);
> -
> -    if (ipv4) {
> -        exhausted = (curr->ipv4 == guard->ipv4);
> -    } else {
> -        exhausted = !memcmp(curr, guard, sizeof *curr);
> -    }
> -
> -    return exhausted;
> -}
> -
>  static bool
>  nat_get_unique_l4(struct conntrack *ct, struct conn *nat_conn,
>                    ovs_be16 *port, uint16_t curr, uint16_t min,
> @@ -2443,9 +2398,8 @@ nat_get_unique_tuple(struct conntrack *ct, const struct conn *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, nat_info);
> +    union ct_addr min_addr = {0}, max_addr = {0}, addr = {0};
>      bool pat_proto = conn->key.nw_proto == IPPROTO_TCP ||
>                       conn->key.nw_proto == IPPROTO_UDP;
>      uint16_t min_dport, max_dport, curr_dport;
> @@ -2454,12 +2408,8 @@ nat_get_unique_tuple(struct conntrack *ct, const struct conn *conn,
>      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)), nat_info);
> -
> -    /* Save the address we started from so that
> -     * we can stop once we reach it. */
> -    guard_addr = curr_addr;
> +    find_addr(conn, &min_addr, &max_addr, &addr, hash,
> +              (conn->key.dl_type == htons(ETH_TYPE_IP)), nat_info);
>  
>      set_sport_range(nat_info, &conn->key, hash, &curr_sport,
>                      &min_sport, &max_sport);
> @@ -2471,8 +2421,7 @@ nat_get_unique_tuple(struct conntrack *ct, const struct conn *conn,

I overlooked the comments related to this function.
I think we have to replace the following (for both src and dst):

- For each src IP address in the range (if any).

with something like:

- pick a src IP address in the range.

while at it, it's probably better to change the comment on
get_addr_in_range():

- Gets the initial in range address based on the hash. ...

with something like:

- Gets an in range address based on the hash. ...

Feel free to change them as you prefer.
With those fixed:

Acked-by: Paolo Valerio <pvalerio@redhat.com>

>          nat_conn->rev_key.dst.port = htons(curr_sport);
>      }
>  
> -another_round:
> -    store_addr_to_key(&curr_addr, &nat_conn->rev_key,
> +    store_addr_to_key(&addr, &nat_conn->rev_key,
>                        nat_info->nat_action);
>  
>      if (!pat_proto) {
> @@ -2481,7 +2430,7 @@ another_round:
>              return true;
>          }
>  
> -        goto next_addr;
> +        return false;
>      }
>  
>      bool found = false;
> @@ -2499,16 +2448,7 @@ another_round:
>          return true;
>      }
>  
> -    /* Check if next IP is in range and respin. Otherwise, notify
> -     * exhaustion to the caller. */
> -next_addr:
> -    if (next_addr_in_range_guarded(&curr_addr, &min_addr,
> -                                   &max_addr, &guard_addr,
> -                                   conn->key.dl_type == htons(ETH_TYPE_IP))) {
> -        return false;
> -    }
> -
> -    goto another_round;
> +    return false;
>  }
>  
>  static enum ct_update_res
> -- 
> 1.8.3.1