diff mbox series

[ovs-dev,v2,2/2] conntrack: Handle persistent selection for IP addresses.

Message ID 20240207173808.1475540-2-pvalerio@redhat.com
State Superseded
Delegated to: Simon Horman
Headers show
Series [ovs-dev,v2,1/2] conntrack: Handle random selection for port ranges. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/intel-ovs-compilation success test: success

Commit Message

Paolo Valerio Feb. 7, 2024, 5:38 p.m. UTC 
The patch, when 'persistent' flag is specified, makes the IP selection
in a range persistent across reboots.

Signed-off-by: Paolo Valerio <pvalerio@redhat.com>
---
 NEWS              |  3 ++-
 lib/conntrack.c   | 27 +++++++++++++++++++++------
 lib/conntrack.h   |  1 +
 lib/dpif-netdev.c |  2 ++
 4 files changed, 26 insertions(+), 7 deletions(-)

Comments

Simon Horman Feb. 15, 2024, 12:34 p.m. UTC | #1 
On Wed, Feb 07, 2024 at 06:38:08PM +0100, Paolo Valerio wrote:
> The patch, when 'persistent' flag is specified, makes the IP selection
> in a range persistent across reboots.
> 
> Signed-off-by: Paolo Valerio <pvalerio@redhat.com>

Hi Paolo,

I have some minor nits below - which you can feel free to take or leave.
But overall this looks good to me.

Acked-by: Simon Horman <horms@ovn.org>

...

> diff --git a/lib/conntrack.c b/lib/conntrack.c

...

> @@ -2386,12 +2390,23 @@ nat_get_unique_tuple(struct conntrack *ct, struct conn *conn,
>      bool pat_proto = fwd_key->nw_proto == IPPROTO_TCP ||
>                       fwd_key->nw_proto == IPPROTO_UDP ||
>                       fwd_key->nw_proto == IPPROTO_SCTP;
> +    uint32_t hash, port_off, basis = ct->hash_basis;
>      uint16_t min_dport, max_dport, curr_dport;
>      uint16_t min_sport, max_sport, curr_sport;
> -    uint32_t hash, port_off;
>  
> -    hash = nat_range_hash(fwd_key, ct->hash_basis, nat_info);
> -    port_off = nat_info->nat_flags & NAT_RANGE_RANDOM ? random_uint32() : hash;
> +    if (nat_info->nat_flags & NAT_PERSISTENT) {
> +        basis = 0;
> +    }

nit: maybe it is nicer to set basis only once.

    basis = (nat_info->nat_flags & NAT_PERSISTENT) ? 0 : ct->hash_basis;

> +
> +    hash = nat_range_hash(fwd_key, basis, nat_info);
> +
> +    if (nat_info->nat_flags & NAT_RANGE_RANDOM) {
> +        port_off = random_uint32();
> +    } else {
> +        port_off =
> +            basis ? hash : nat_range_hash(fwd_key, ct->hash_basis, nat_info);
> +    }
> +

nit: maybe this is a little easier on the eyes (completely untested!)?

    if (nat_info->nat_flags & NAT_RANGE_RANDOM) {
        port_off = random_uint32();
    } else if (basis) {
        port_off = hash;
    } else {
        port_off = nat_range_hash(fwd_key, ct->hash_basis, nat_info);
    }

>      min_addr = nat_info->min_addr;
>      max_addr = nat_info->max_addr;
>  

...
Aaron Conole Feb. 15, 2024, 2:32 p.m. UTC | #2 
Paolo Valerio <pvalerio@redhat.com> writes:

> The patch, when 'persistent' flag is specified, makes the IP selection
> in a range persistent across reboots.
>
> Signed-off-by: Paolo Valerio <pvalerio@redhat.com>
> ---
>  NEWS              |  3 ++-
>  lib/conntrack.c   | 27 +++++++++++++++++++++------
>  lib/conntrack.h   |  1 +
>  lib/dpif-netdev.c |  2 ++
>  4 files changed, 26 insertions(+), 7 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 93046b963..0c86bba81 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -2,7 +2,8 @@ Post-v3.3.0
>  --------------------
>     - Userspace datapath:
>       * Conntrack now supports 'random' flag for selecting ports in a range
> -       while natting.
> +       while natting and 'persistent' flag for selection of the IP address
> +       from a range.
>  
>  
>  v3.3.0 - xx xxx xxxx
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index e09ecdf33..7868a67f7 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -2202,17 +2202,21 @@ nat_range_hash(const struct conn_key *key, uint32_t basis,
>  {
>      uint32_t hash = basis;
>  
> +    if (!basis) {
> +        hash = ct_addr_hash_add(hash, &key->src.addr);
> +    } else {
> +        hash = ct_endpoint_hash_add(hash, &key->src);
> +        hash = ct_endpoint_hash_add(hash, &key->dst);
> +    }
> +
>      hash = ct_addr_hash_add(hash, &nat_info->min_addr);
>      hash = ct_addr_hash_add(hash, &nat_info->max_addr);
>      hash = hash_add(hash,
>                      ((uint32_t) nat_info->max_port << 16)
>                      | nat_info->min_port);
> -    hash = ct_endpoint_hash_add(hash, &key->src);
> -    hash = ct_endpoint_hash_add(hash, &key->dst);
>      hash = hash_add(hash, (OVS_FORCE uint32_t) key->dl_type);
>      hash = hash_add(hash, key->nw_proto);
>      hash = hash_add(hash, key->zone);
> -
>      /* The purpose of the second parameter is to distinguish hashes of data of
>       * different length; our data always has the same length so there is no
>       * value in counting. */
> @@ -2386,12 +2390,23 @@ nat_get_unique_tuple(struct conntrack *ct, struct conn *conn,
>      bool pat_proto = fwd_key->nw_proto == IPPROTO_TCP ||
>                       fwd_key->nw_proto == IPPROTO_UDP ||
>                       fwd_key->nw_proto == IPPROTO_SCTP;
> +    uint32_t hash, port_off, basis = ct->hash_basis;
>      uint16_t min_dport, max_dport, curr_dport;
>      uint16_t min_sport, max_sport, curr_sport;
> -    uint32_t hash, port_off;
>  
> -    hash = nat_range_hash(fwd_key, ct->hash_basis, nat_info);
> -    port_off = nat_info->nat_flags & NAT_RANGE_RANDOM ? random_uint32() : hash;
> +    if (nat_info->nat_flags & NAT_PERSISTENT) {
> +        basis = 0;
> +    }
> +
> +    hash = nat_range_hash(fwd_key, basis, nat_info);
> +
> +    if (nat_info->nat_flags & NAT_RANGE_RANDOM) {
> +        port_off = random_uint32();
> +    } else {
> +        port_off =
> +            basis ? hash : nat_range_hash(fwd_key, ct->hash_basis, nat_info);
> +    }
> +

I'm probably misreading this, but the multiple 'if's is confusing me.

Maybe it would be better to write something like:

bool persist = !!(nat_info->nat_flags & NAT_PERSISTENT)

if (nat_info->nat_flags & NAT_RANGE_RANDOM) {
   port_off = random_uint32();
} else {
   port_off = !persist ?
       nat_range_hash(fwd_key, ct->hash_basis, nat_info) :
       nat_range_hash(fwd_key, 0, nat_info);
}

Especially because in the above code, ct->hash_basis == '0' would look
the same as persistent (which isn't obvious from the code).

At least, the multiple nested ifs make it difficult to follow what is
going on with basis call and nat_range_hash

Just read Simon's comments, and it's similar kind of comment I guess
:)

>      min_addr = nat_info->min_addr;
>      max_addr = nat_info->max_addr;
>  
> diff --git a/lib/conntrack.h b/lib/conntrack.h
> index 9b0c6aa88..ee7da099e 100644
> --- a/lib/conntrack.h
> +++ b/lib/conntrack.h
> @@ -79,6 +79,7 @@ enum nat_action_e {
>  
>  enum nat_flags_e {
>      NAT_RANGE_RANDOM = 1 << 0,
> +    NAT_PERSISTENT = 1 << 1,
>  };
>  
>  struct nat_action_info_t {
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index c3334c667..fbf7ccabd 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -9413,6 +9413,8 @@ dp_execute_cb(void *aux_, struct dp_packet_batch *packets_,
>                          nat_action_info.nat_flags |= NAT_RANGE_RANDOM;
>                          break;
>                      case OVS_NAT_ATTR_PERSISTENT:
> +                        nat_action_info.nat_flags |= NAT_PERSISTENT;
> +                        break;
>                      case OVS_NAT_ATTR_PROTO_HASH:
>                          break;
>                      case OVS_NAT_ATTR_UNSPEC:
Paolo Valerio Feb. 16, 2024, 5:20 p.m. UTC | #3 
Simon Horman <horms@ovn.org> writes:

> On Wed, Feb 07, 2024 at 06:38:08PM +0100, Paolo Valerio wrote:
>> The patch, when 'persistent' flag is specified, makes the IP selection
>> in a range persistent across reboots.
>> 
>> Signed-off-by: Paolo Valerio <pvalerio@redhat.com>
>
> Hi Paolo,
>
> I have some minor nits below - which you can feel free to take or leave.
> But overall this looks good to me.
>
> Acked-by: Simon Horman <horms@ovn.org>
>
> ...
>
>> diff --git a/lib/conntrack.c b/lib/conntrack.c
>
> ...
>
>> @@ -2386,12 +2390,23 @@ nat_get_unique_tuple(struct conntrack *ct, struct conn *conn,
>>      bool pat_proto = fwd_key->nw_proto == IPPROTO_TCP ||
>>                       fwd_key->nw_proto == IPPROTO_UDP ||
>>                       fwd_key->nw_proto == IPPROTO_SCTP;
>> +    uint32_t hash, port_off, basis = ct->hash_basis;
>>      uint16_t min_dport, max_dport, curr_dport;
>>      uint16_t min_sport, max_sport, curr_sport;
>> -    uint32_t hash, port_off;
>>  
>> -    hash = nat_range_hash(fwd_key, ct->hash_basis, nat_info);
>> -    port_off = nat_info->nat_flags & NAT_RANGE_RANDOM ? random_uint32() : hash;
>> +    if (nat_info->nat_flags & NAT_PERSISTENT) {
>> +        basis = 0;
>> +    }
>
> nit: maybe it is nicer to set basis only once.
>
>     basis = (nat_info->nat_flags & NAT_PERSISTENT) ? 0 : ct->hash_basis;
>
>> +
>> +    hash = nat_range_hash(fwd_key, basis, nat_info);
>> +
>> +    if (nat_info->nat_flags & NAT_RANGE_RANDOM) {
>> +        port_off = random_uint32();
>> +    } else {
>> +        port_off =
>> +            basis ? hash : nat_range_hash(fwd_key, ct->hash_basis, nat_info);
>> +    }
>> +
>
> nit: maybe this is a little easier on the eyes (completely untested!)?
>
>     if (nat_info->nat_flags & NAT_RANGE_RANDOM) {
>         port_off = random_uint32();
>     } else if (basis) {
>         port_off = hash;
>     } else {
>         port_off = nat_range_hash(fwd_key, ct->hash_basis, nat_info);
>     }
>

thanks Simon for taking a look.
Agreed, looks easier on the eyes. I included your suggestions and your
acks in v3.

I guess the above solve Aaron's suggestions as well.

>>      min_addr = nat_info->min_addr;
>>      max_addr = nat_info->max_addr;
>>  
>
> ...
diff mbox series

Patch

diff --git a/NEWS b/NEWS
index 93046b963..0c86bba81 100644
--- a/NEWS
+++ b/NEWS
@@ -2,7 +2,8 @@  Post-v3.3.0
 --------------------
    - Userspace datapath:
      * Conntrack now supports 'random' flag for selecting ports in a range
-       while natting.
+       while natting and 'persistent' flag for selection of the IP address
+       from a range.
 
 
 v3.3.0 - xx xxx xxxx
diff --git a/lib/conntrack.c b/lib/conntrack.c
index e09ecdf33..7868a67f7 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -2202,17 +2202,21 @@  nat_range_hash(const struct conn_key *key, uint32_t basis,
 {
     uint32_t hash = basis;
 
+    if (!basis) {
+        hash = ct_addr_hash_add(hash, &key->src.addr);
+    } else {
+        hash = ct_endpoint_hash_add(hash, &key->src);
+        hash = ct_endpoint_hash_add(hash, &key->dst);
+    }
+
     hash = ct_addr_hash_add(hash, &nat_info->min_addr);
     hash = ct_addr_hash_add(hash, &nat_info->max_addr);
     hash = hash_add(hash,
                     ((uint32_t) nat_info->max_port << 16)
                     | nat_info->min_port);
-    hash = ct_endpoint_hash_add(hash, &key->src);
-    hash = ct_endpoint_hash_add(hash, &key->dst);
     hash = hash_add(hash, (OVS_FORCE uint32_t) key->dl_type);
     hash = hash_add(hash, key->nw_proto);
     hash = hash_add(hash, key->zone);
-
     /* The purpose of the second parameter is to distinguish hashes of data of
      * different length; our data always has the same length so there is no
      * value in counting. */
@@ -2386,12 +2390,23 @@  nat_get_unique_tuple(struct conntrack *ct, struct conn *conn,
     bool pat_proto = fwd_key->nw_proto == IPPROTO_TCP ||
                      fwd_key->nw_proto == IPPROTO_UDP ||
                      fwd_key->nw_proto == IPPROTO_SCTP;
+    uint32_t hash, port_off, basis = ct->hash_basis;
     uint16_t min_dport, max_dport, curr_dport;
     uint16_t min_sport, max_sport, curr_sport;
-    uint32_t hash, port_off;
 
-    hash = nat_range_hash(fwd_key, ct->hash_basis, nat_info);
-    port_off = nat_info->nat_flags & NAT_RANGE_RANDOM ? random_uint32() : hash;
+    if (nat_info->nat_flags & NAT_PERSISTENT) {
+        basis = 0;
+    }
+
+    hash = nat_range_hash(fwd_key, basis, nat_info);
+
+    if (nat_info->nat_flags & NAT_RANGE_RANDOM) {
+        port_off = random_uint32();
+    } else {
+        port_off =
+            basis ? hash : nat_range_hash(fwd_key, ct->hash_basis, nat_info);
+    }
+
     min_addr = nat_info->min_addr;
     max_addr = nat_info->max_addr;
 
diff --git a/lib/conntrack.h b/lib/conntrack.h
index 9b0c6aa88..ee7da099e 100644
--- a/lib/conntrack.h
+++ b/lib/conntrack.h
@@ -79,6 +79,7 @@  enum nat_action_e {
 
 enum nat_flags_e {
     NAT_RANGE_RANDOM = 1 << 0,
+    NAT_PERSISTENT = 1 << 1,
 };
 
 struct nat_action_info_t {
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index c3334c667..fbf7ccabd 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -9413,6 +9413,8 @@  dp_execute_cb(void *aux_, struct dp_packet_batch *packets_,
                         nat_action_info.nat_flags |= NAT_RANGE_RANDOM;
                         break;
                     case OVS_NAT_ATTR_PERSISTENT:
+                        nat_action_info.nat_flags |= NAT_PERSISTENT;
+                        break;
                     case OVS_NAT_ATTR_PROTO_HASH:
                         break;
                     case OVS_NAT_ATTR_UNSPEC: