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. [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? > + } 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
