wenxu <[email protected]> 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. >> >> [email protected] writes: >> >>> From: wenxu <[email protected]> >>> >>> 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 <[email protected]> >>> --- >>> 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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
