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

2024-02-16 Thread Paolo Valerio 
Simon Horman  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 
>
> 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 
>
> ...
>
>> 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;
>>  
>
> ...

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

2024-02-15 Thread Aaron Conole 
Paolo Valerio  writes:

> The patch, when 'persistent' flag is specified, makes the IP selection
> in a range persistent across reboots.
>
> Signed-off-by: Paolo Valerio 
> ---
>  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 
> 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, >src.addr);
> +} else {
> +hash = ct_endpoint_hash_add(hash, >src);
> +hash = ct_endpoint_hash_add(hash, >dst);
> +}
> +
>  hash = ct_addr_hash_add(hash, _info->min_addr);
>  hash = ct_addr_hash_add(hash, _info->max_addr);
>  hash = hash_add(hash,
>  ((uint32_t) nat_info->max_port << 16)
>  | nat_info->min_port);
> -hash = ct_endpoint_hash_add(hash, >src);
> -hash = ct_endpoint_hash_add(hash, >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:

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

2024-02-15 Thread Simon Horman 
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 

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 

...

> 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;
>  

...
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

2024-02-07 Thread Paolo Valerio 
The patch, when 'persistent' flag is specified, makes the IP selection
in a range persistent across reboots.

Signed-off-by: Paolo Valerio 
---
 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 
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, >src.addr);
+} else {
+hash = ct_endpoint_hash_add(hash, >src);
+hash = ct_endpoint_hash_add(hash, >dst);
+}
+
 hash = ct_addr_hash_add(hash, _info->min_addr);
 hash = ct_addr_hash_add(hash, _info->max_addr);
 hash = hash_add(hash,
 ((uint32_t) nat_info->max_port << 16)
 | nat_info->min_port);
-hash = ct_endpoint_hash_add(hash, >src);
-hash = ct_endpoint_hash_add(hash, >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:
-- 
2.43.0

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev