[ovs-dev,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 | 76 +++++++--------------------------------------------------
 1 file changed, 9 insertions(+), 67 deletions(-)

Hello wenxu,

Overall, I'm ok with the change. I think we should consider the case of
, e.g. ICMP (identifier), as in that scenario, the avoidance is solely
based on the randomness of the originating ends. Probably we may want to
consider solving a potential clash for that case too (implies further
changes and should be handled in a different patch).

Ideally, it would be nice to hear other opinions on the overall idea and
the ICMP case.

Some nits below.

wenx05124561@163.com 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 | 76 +++++++--------------------------------------------------
>  1 file changed, 9 insertions(+), 67 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 08da4dd..ac95d0f 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -2322,7 +2322,7 @@ get_addr_in_range(union ct_addr *min, union ct_addr *max,
>  }
>  

nit: I'm not convinced by the name. It's not the actual best addr
(e.g. the least used IP addr). What about something like get_addr() or
find_addr().
If you prefer something else, it'll work too.

>  static void
> -get_initial_addr(const struct conn *conn, union ct_addr *min,
> +find_best_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)
> @@ -2336,6 +2336,8 @@ get_initial_addr(const struct conn *conn, union ct_addr *min,
>          } else if (nat_info->nat_action & NAT_ACTION_DST) {
>              *curr = conn->key.dst.addr;
>          }

can you explain why this is needed?

> +    } else if (!memcmp(min, max, sizeof *min)) {
> +        *curr = *min;
>      } else {
>          get_addr_in_range(min, max, curr, hash, ipv4);
>      }
> @@ -2352,51 +2354,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 +2400,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}, best_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 +2410,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;

Same for best_addr. Doesn't seem to really describe the variable. What do you
think about just addr or address or something similar?

> +    find_best_addr(conn, &min_addr, &max_addr, &best_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 +2423,7 @@ nat_get_unique_tuple(struct conntrack *ct, const struct conn *conn,
>          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(&best_addr, &nat_conn->rev_key,
>                        nat_info->nat_action);
>  
>      if (!pat_proto) {
> @@ -2481,7 +2432,7 @@ another_round:
>              return true;
>          }
>  
> -        goto next_addr;
> +        return false;
>      }
>  
>      bool found = false;
> @@ -2499,16 +2450,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

On 2022/5/5 0:55, Paolo Valerio wrote:
> Hello wenxu,
>
> Overall, I'm ok with the change. I think we should consider the case of
> , e.g. ICMP (identifier), as in that scenario, the avoidance is solely
> based on the randomness of the originating ends. Probably we may want to
> consider solving a potential clash for that case too (implies further
> changes and should be handled in a different patch).

Yes, It should be improvement for icmp case. For the normal case, two server snat

to only one ip, there are the problem the two icmp connection with the same icmp.id

in the two server.  For resolved this, the icmp.id can be the port_key like tcp and udp.

This can be improvement in the future patch.

>
> Ideally, it would be nice to hear other opinions on the overall idea and
> the ICMP case.
>
> Some nits below.
>
> wenx05124561@163.com 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 | 76 +++++++--------------------------------------------------
>>  1 file changed, 9 insertions(+), 67 deletions(-)
>>
>> diff --git a/lib/conntrack.c b/lib/conntrack.c
>> index 08da4dd..ac95d0f 100644
>> --- a/lib/conntrack.c
>> +++ b/lib/conntrack.c
>> @@ -2322,7 +2322,7 @@ get_addr_in_range(union ct_addr *min, union ct_addr *max,
>>  }
>>  
> nit: I'm not convinced by the name. It's not the actual best addr
> (e.g. the least used IP addr). What about something like get_addr() or
> find_addr().
> If you prefer something else, it'll work too.
>
>>  static void
>> -get_initial_addr(const struct conn *conn, union ct_addr *min,
>> +find_best_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)
>> @@ -2336,6 +2336,8 @@ get_initial_addr(const struct conn *conn, union ct_addr *min,
>>          } else if (nat_info->nat_action & NAT_ACTION_DST) {
>>              *curr = conn->key.dst.addr;
>>          }
> can you explain why this is needed?
This is unnecessary.
>
>> +    } else if (!memcmp(min, max, sizeof *min)) {
>> +        *curr = *min;
>>      } else {
>>          get_addr_in_range(min, max, curr, hash, ipv4);
>>      }
>> @@ -2352,51 +2354,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 +2400,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}, best_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 +2410,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;
> Same for best_addr. Doesn't seem to really describe the variable. What do you
> think about just addr or address or something similar?
>
>> +    find_best_addr(conn, &min_addr, &max_addr, &best_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 +2423,7 @@ nat_get_unique_tuple(struct conntrack *ct, const struct conn *conn,
>>          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(&best_addr, &nat_conn->rev_key,
>>                        nat_info->nat_action);
>>  
>>      if (!pat_proto) {
>> @@ -2481,7 +2432,7 @@ another_round:
>>              return true;
>>          }
>>  
>> -        goto next_addr;
>> +        return false;
>>      }
>>  
>>      bool found = false;
>> @@ -2499,16 +2450,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

wenxu <wenxu@chinatelecom.cn> writes:

> On 2022/5/5 0:55, Paolo Valerio wrote:
>> Hello wenxu,
>>
>> Overall, I'm ok with the change. I think we should consider the case of
>> , e.g. ICMP (identifier), as in that scenario, the avoidance is solely
>> based on the randomness of the originating ends. Probably we may want to
>> consider solving a potential clash for that case too (implies further
>> changes and should be handled in a different patch).
>
> Yes, It should be improvement for icmp case. For the normal case, two server snat
>
> to only one ip, there are the problem the two icmp connection with the same icmp.id
>
> in the two server.  For resolved this, the icmp.id can be the port_key like tcp and udp.
>
> This can be improvement in the future patch.
>

I agree. I also tend to consider this unrelated to your series as the
potential problem is already existing.

>>
>> Ideally, it would be nice to hear other opinions on the overall idea and
>> the ICMP case.
>>
>> Some nits below.
>>
>> wenx05124561@163.com 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 | 76 +++++++--------------------------------------------------
>>>  1 file changed, 9 insertions(+), 67 deletions(-)
>>>
>>> diff --git a/lib/conntrack.c b/lib/conntrack.c
>>> index 08da4dd..ac95d0f 100644
>>> --- a/lib/conntrack.c
>>> +++ b/lib/conntrack.c
>>> @@ -2322,7 +2322,7 @@ get_addr_in_range(union ct_addr *min, union ct_addr *max,
>>>  }
>>>  
>> nit: I'm not convinced by the name. It's not the actual best addr
>> (e.g. the least used IP addr). What about something like get_addr() or
>> find_addr().
>> If you prefer something else, it'll work too.
>>
>>>  static void
>>> -get_initial_addr(const struct conn *conn, union ct_addr *min,
>>> +find_best_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)
>>> @@ -2336,6 +2336,8 @@ get_initial_addr(const struct conn *conn, union ct_addr *min,
>>>          } else if (nat_info->nat_action & NAT_ACTION_DST) {
>>>              *curr = conn->key.dst.addr;
>>>          }
>> can you explain why this is needed?
> This is unnecessary.

thanks for confirming it, please let's drop it.

>>
>>> +    } else if (!memcmp(min, max, sizeof *min)) {
>>> +        *curr = *min;
>>>      } else {
>>>          get_addr_in_range(min, max, curr, hash, ipv4);
>>>      }
>>> @@ -2352,51 +2354,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 +2400,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}, best_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 +2410,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;
>> Same for best_addr. Doesn't seem to really describe the variable. What do you
>> think about just addr or address or something similar?
>>
>>> +    find_best_addr(conn, &min_addr, &max_addr, &best_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 +2423,7 @@ nat_get_unique_tuple(struct conntrack *ct, const struct conn *conn,
>>>          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(&best_addr, &nat_conn->rev_key,
>>>                        nat_info->nat_action);
>>>  
>>>      if (!pat_proto) {
>>> @@ -2481,7 +2432,7 @@ another_round:
>>>              return true;
>>>          }
>>>  
>>> -        goto next_addr;
>>> +        return false;
>>>      }
>>>  
>>>      bool found = false;
>>> @@ -2499,16 +2450,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