Re: [ovs-dev] [patch_v6 4/8] dpdk: Userspace Datapath: Introduce NAT Support.

[Daniele Di Proietto]

2017-03-09 10:21 GMT-08:00 Darrell Ball :
>
>
> On 3/8/17, 6:14 PM, "ovs-dev-boun...@openvswitch.org on behalf of Daniele Di 
> Proietto"  
> wrote:
>
> 2017-02-16 0:47 GMT-08:00 Darrell Ball :
> > This patch introduces NAT support for the userspace datapath.
> > The conntrack module changes are in this patch.
> >
> > The per packet scope of lookups for NAT and un_NAT is at
> > the bucket level rather than global. One hash table is
> > introduced to support create/delete handling. The create/delete
> > events may be further optimized, if the need becomes clear.
> >
> > Some NAT options with limited utility (persistent, random) are
> > not supported yet, but will be supported in a later patch.
> >
> > Signed-off-by: Darrell Ball 
>
> Thanks for the patch,  I'll keep looking at this, but since you're
> about to send another version I had one comment below.
>
> > ---
> >  lib/conntrack-private.h |  16 +-
> >  lib/conntrack.c | 782 
> ++--
> >  lib/conntrack.h |  46 +++
> >  3 files changed, 751 insertions(+), 93 deletions(-)
> >
> > diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
> > index 493865f..a7c2ae4 100644
> > --- a/lib/conntrack-private.h
> > +++ b/lib/conntrack-private.h
> > @@ -51,14 +51,23 @@ struct conn_key {
> >  uint16_t zone;
> >  };
> >
> > +struct nat_conn_key_node {
> > +struct hmap_node node;
> > +struct conn_key key;
> > +struct conn_key value;
> > +};
> > +
> >  struct conn {
> >  struct conn_key key;
> >  struct conn_key rev_key;
> >  long long expiration;
> >  struct ovs_list exp_node;
> >  struct hmap_node node;
> > -uint32_t mark;
> >  ovs_u128 label;
> > +/* XXX: consider flattening. */
> > +struct nat_action_info_t *nat_info;
> > +uint32_t mark;
> > +uint8_t conn_type;
> >  };
> >
> >  enum ct_update_res {
> > @@ -67,6 +76,11 @@ enum ct_update_res {
> >  CT_UPDATE_NEW,
> >  };
> >
> > +enum ct_conn_type {
> > +CT_CONN_TYPE_DEFAULT,
> > +CT_CONN_TYPE_UN_NAT,
> > +};
> > +
> >  struct ct_l4_proto {
> >  struct conn *(*new_conn)(struct conntrack_bucket *, struct 
> dp_packet *pkt,
> >   long long now);
> > diff --git a/lib/conntrack.c b/lib/conntrack.c
> > index d0e106f..49760c0 100644
> > --- a/lib/conntrack.c
> > +++ b/lib/conntrack.c
> > @@ -76,6 +76,20 @@ static void set_label(struct dp_packet *, struct 
> conn *,
> >const struct ovs_key_ct_labels *mask);
> >  static void *clean_thread_main(void *f_);
> >
> > +static struct nat_conn_key_node *
> > +nat_conn_keys_lookup(struct hmap *nat_conn_keys,
> > + const struct conn_key *key,
> > + uint32_t basis);
> > +
> > +static void
> > +nat_conn_keys_remove(struct hmap *nat_conn_keys,
> > +const struct conn_key *key,
> > +uint32_t basis);
> > +
> > +static bool
> > +nat_select_range_tuple(struct conntrack *ct, const struct conn *conn,
> > +   struct conn *nat_conn);
> > +
> >  static struct ct_l4_proto *l4_protos[] = {
> >  [IPPROTO_TCP] = _proto_tcp,
> >  [IPPROTO_UDP] = _proto_other,
> > @@ -90,7 +104,7 @@ long long ct_timeout_val[] = {
> >  };
> >
> >  /* If the total number of connections goes above this value, no new 
> connections
> > - * are accepted */
> > + * are accepted; this is for CT_CONN_TYPE_DEFAULT connections. */
> >  #define DEFAULT_N_CONN_LIMIT 300
> >
> >  /* Initializes the connection tracker 'ct'.  The caller is responsible 
> for
> > @@ -101,6 +115,11 @@ conntrack_init(struct conntrack *ct)
> >  unsigned i, j;
> >  long long now = time_msec();
> >
> > +ct_rwlock_init(>nat_resources_lock);
> > +ct_rwlock_wrlock(>nat_resources_lock);
> > +hmap_init(>nat_conn_keys);
> > +ct_rwlock_unlock(>nat_resources_lock);
> > +
> >  for (i = 0; i < CONNTRACK_BUCKETS; i++) {
> >  struct conntrack_bucket *ctb = >buckets[i];
> >
> > @@ -139,13 +158,24 @@ conntrack_destroy(struct conntrack *ct)
> >  ovs_mutex_destroy(>cleanup_mutex);
> >  ct_lock_lock(>lock);
> >  HMAP_FOR_EACH_POP(conn, node, >connections) {
> > -atomic_count_dec(>n_conn);
> > +if (conn->conn_type == CT_CONN_TYPE_DEFAULT) {
> > +atomic_count_dec(>n_conn);
> > +}
> >   

Re: [ovs-dev] [patch_v6 4/8] dpdk: Userspace Datapath: Introduce NAT Support.

[Darrell Ball]

On 3/8/17, 6:14 PM, "ovs-dev-boun...@openvswitch.org on behalf of Daniele Di 
Proietto"  
wrote:

2017-02-16 0:47 GMT-08:00 Darrell Ball :
> This patch introduces NAT support for the userspace datapath.
> The conntrack module changes are in this patch.
>
> The per packet scope of lookups for NAT and un_NAT is at
> the bucket level rather than global. One hash table is
> introduced to support create/delete handling. The create/delete
> events may be further optimized, if the need becomes clear.
>
> Some NAT options with limited utility (persistent, random) are
> not supported yet, but will be supported in a later patch.
>
> Signed-off-by: Darrell Ball 

Thanks for the patch,  I'll keep looking at this, but since you're
about to send another version I had one comment below.

> ---
>  lib/conntrack-private.h |  16 +-
>  lib/conntrack.c | 782 
++--
>  lib/conntrack.h |  46 +++
>  3 files changed, 751 insertions(+), 93 deletions(-)
>
> diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
> index 493865f..a7c2ae4 100644
> --- a/lib/conntrack-private.h
> +++ b/lib/conntrack-private.h
> @@ -51,14 +51,23 @@ struct conn_key {
>  uint16_t zone;
>  };
>
> +struct nat_conn_key_node {
> +struct hmap_node node;
> +struct conn_key key;
> +struct conn_key value;
> +};
> +
>  struct conn {
>  struct conn_key key;
>  struct conn_key rev_key;
>  long long expiration;
>  struct ovs_list exp_node;
>  struct hmap_node node;
> -uint32_t mark;
>  ovs_u128 label;
> +/* XXX: consider flattening. */
> +struct nat_action_info_t *nat_info;
> +uint32_t mark;
> +uint8_t conn_type;
>  };
>
>  enum ct_update_res {
> @@ -67,6 +76,11 @@ enum ct_update_res {
>  CT_UPDATE_NEW,
>  };
>
> +enum ct_conn_type {
> +CT_CONN_TYPE_DEFAULT,
> +CT_CONN_TYPE_UN_NAT,
> +};
> +
>  struct ct_l4_proto {
>  struct conn *(*new_conn)(struct conntrack_bucket *, struct dp_packet 
*pkt,
>   long long now);
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index d0e106f..49760c0 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -76,6 +76,20 @@ static void set_label(struct dp_packet *, struct conn 
*,
>const struct ovs_key_ct_labels *mask);
>  static void *clean_thread_main(void *f_);
>
> +static struct nat_conn_key_node *
> +nat_conn_keys_lookup(struct hmap *nat_conn_keys,
> + const struct conn_key *key,
> + uint32_t basis);
> +
> +static void
> +nat_conn_keys_remove(struct hmap *nat_conn_keys,
> +const struct conn_key *key,
> +uint32_t basis);
> +
> +static bool
> +nat_select_range_tuple(struct conntrack *ct, const struct conn *conn,
> +   struct conn *nat_conn);
> +
>  static struct ct_l4_proto *l4_protos[] = {
>  [IPPROTO_TCP] = _proto_tcp,
>  [IPPROTO_UDP] = _proto_other,
> @@ -90,7 +104,7 @@ long long ct_timeout_val[] = {
>  };
>
>  /* If the total number of connections goes above this value, no new 
connections
> - * are accepted */
> + * are accepted; this is for CT_CONN_TYPE_DEFAULT connections. */
>  #define DEFAULT_N_CONN_LIMIT 300
>
>  /* Initializes the connection tracker 'ct'.  The caller is responsible 
for
> @@ -101,6 +115,11 @@ conntrack_init(struct conntrack *ct)
>  unsigned i, j;
>  long long now = time_msec();
>
> +ct_rwlock_init(>nat_resources_lock);
> +ct_rwlock_wrlock(>nat_resources_lock);
> +hmap_init(>nat_conn_keys);
> +ct_rwlock_unlock(>nat_resources_lock);
> +
>  for (i = 0; i < CONNTRACK_BUCKETS; i++) {
>  struct conntrack_bucket *ctb = >buckets[i];
>
> @@ -139,13 +158,24 @@ conntrack_destroy(struct conntrack *ct)
>  ovs_mutex_destroy(>cleanup_mutex);
>  ct_lock_lock(>lock);
>  HMAP_FOR_EACH_POP(conn, node, >connections) {
> -atomic_count_dec(>n_conn);
> +if (conn->conn_type == CT_CONN_TYPE_DEFAULT) {
> +atomic_count_dec(>n_conn);
> +}
>  delete_conn(conn);
>  }
>  hmap_destroy(>connections);
>  ct_lock_unlock(>lock);
>  ct_lock_destroy(>lock);
>  }
> +ct_rwlock_wrlock(>nat_resources_lock);
> +struct nat_conn_key_node *nat_conn_key_node;
> +

Re: [ovs-dev] [patch_v6 4/8] dpdk: Userspace Datapath: Introduce NAT Support.

[Flavio Leitner]

On Wed, Feb 22, 2017 at 04:29:09AM +, Darrell Ball wrote:
> 
> 
> On 2/21/17, 1:12 PM, "ovs-dev-boun...@openvswitch.org on behalf of Flavio 
> Leitner"  
> wrote:
> 
> On Thu, Feb 16, 2017 at 12:47:35AM -0800, Darrell Ball wrote:
> > This patch introduces NAT support for the userspace datapath.
> > The conntrack module changes are in this patch.
> > 
> > The per packet scope of lookups for NAT and un_NAT is at
> > the bucket level rather than global. One hash table is
> > introduced to support create/delete handling. The create/delete
> > events may be further optimized, if the need becomes clear.
> > 
> > Some NAT options with limited utility (persistent, random) are
> > not supported yet, but will be supported in a later patch.
> > 
> > Signed-off-by: Darrell Ball 
> 
> 
> It works for me :-)
> I was worried with the two memcpy() in sequence in nat_packet()
> but turns out that gcc is smart enough to optimize them.
> 
> Thanks for pointing this out.
> I removed the unnecessary memcpy and associated local variable in all
> these instances.
> Fortuitously, while looking at the associated code, I noticed the V6 DNAT
> address replacement had a “.dst.” instead of a “.src.”.
> I fixed it.

Nice catch, I was sure I had looked at those variables because it's easy
to swap them and yet I missed.  :)


> I added a new V6 DNAT test to cover this case.

> 
> Another thing is use of OVS_NO_THREAD_SAFETY_ANALYSIS, but it
> doesn't look like fixing it will get any better.
> 
> There are some unrelated changes like cleanups or code movement
> in process_one(), but that's fine I guess.
> 
> Acked-by: Flavio Leitner 
> 
> 
> ___
> dev mailing list
> d...@openvswitch.org
> 
> https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev=DwICAg=uilaK90D4TOVoH58JNXRgQ=BVhFA09CGX7JQ5Ih-uZnsw=O2Clkd8rWWs5ciGhLS-rNrdZ1khoZwU9j7nFrQlraOk=wUwh32XoPIHc4lG8BpDYmKyI_wC6E68q0s2chKWnAQo=
>  
> 
> 

-- 
Flavio

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

Re: [ovs-dev] [patch_v6 4/8] dpdk: Userspace Datapath: Introduce NAT Support.

[Darrell Ball]

On 2/21/17, 1:12 PM, "ovs-dev-boun...@openvswitch.org on behalf of Flavio 
Leitner"  wrote:

On Thu, Feb 16, 2017 at 12:47:35AM -0800, Darrell Ball wrote:
> This patch introduces NAT support for the userspace datapath.
> The conntrack module changes are in this patch.
> 
> The per packet scope of lookups for NAT and un_NAT is at
> the bucket level rather than global. One hash table is
> introduced to support create/delete handling. The create/delete
> events may be further optimized, if the need becomes clear.
> 
> Some NAT options with limited utility (persistent, random) are
> not supported yet, but will be supported in a later patch.
> 
> Signed-off-by: Darrell Ball 


It works for me :-)
I was worried with the two memcpy() in sequence in nat_packet()
but turns out that gcc is smart enough to optimize them.

Thanks for pointing this out.
I removed the unnecessary memcpy and associated local variable in all
these instances.
Fortuitously, while looking at the associated code, I noticed the V6 DNAT
address replacement had a “.dst.” instead of a “.src.”.
I fixed it.
I added a new V6 DNAT test to cover this case.

Another thing is use of OVS_NO_THREAD_SAFETY_ANALYSIS, but it
doesn't look like fixing it will get any better.

There are some unrelated changes like cleanups or code movement
in process_one(), but that's fine I guess.

Acked-by: Flavio Leitner 


___
dev mailing list
d...@openvswitch.org

https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev=DwICAg=uilaK90D4TOVoH58JNXRgQ=BVhFA09CGX7JQ5Ih-uZnsw=O2Clkd8rWWs5ciGhLS-rNrdZ1khoZwU9j7nFrQlraOk=wUwh32XoPIHc4lG8BpDYmKyI_wC6E68q0s2chKWnAQo=
 


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

[ovs-dev] [patch_v6 4/8] dpdk: Userspace Datapath: Introduce NAT Support.

[Darrell Ball]

This patch introduces NAT support for the userspace datapath.
The conntrack module changes are in this patch.

The per packet scope of lookups for NAT and un_NAT is at
the bucket level rather than global. One hash table is
introduced to support create/delete handling. The create/delete
events may be further optimized, if the need becomes clear.

Some NAT options with limited utility (persistent, random) are
not supported yet, but will be supported in a later patch.

Signed-off-by: Darrell Ball 
---
 lib/conntrack-private.h |  16 +-
 lib/conntrack.c | 782 ++--
 lib/conntrack.h |  46 +++
 3 files changed, 751 insertions(+), 93 deletions(-)

diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
index 493865f..a7c2ae4 100644
--- a/lib/conntrack-private.h
+++ b/lib/conntrack-private.h
@@ -51,14 +51,23 @@ struct conn_key {
 uint16_t zone;
 };
 
+struct nat_conn_key_node {
+struct hmap_node node;
+struct conn_key key;
+struct conn_key value;
+};
+
 struct conn {
 struct conn_key key;
 struct conn_key rev_key;
 long long expiration;
 struct ovs_list exp_node;
 struct hmap_node node;
-uint32_t mark;
 ovs_u128 label;
+/* XXX: consider flattening. */
+struct nat_action_info_t *nat_info;
+uint32_t mark;
+uint8_t conn_type;
 };
 
 enum ct_update_res {
@@ -67,6 +76,11 @@ enum ct_update_res {
 CT_UPDATE_NEW,
 };
 
+enum ct_conn_type {
+CT_CONN_TYPE_DEFAULT,
+CT_CONN_TYPE_UN_NAT,
+};
+
 struct ct_l4_proto {
 struct conn *(*new_conn)(struct conntrack_bucket *, struct dp_packet *pkt,
  long long now);
diff --git a/lib/conntrack.c b/lib/conntrack.c
index d0e106f..49760c0 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -76,6 +76,20 @@ static void set_label(struct dp_packet *, struct conn *,
   const struct ovs_key_ct_labels *mask);
 static void *clean_thread_main(void *f_);
 
+static struct nat_conn_key_node *
+nat_conn_keys_lookup(struct hmap *nat_conn_keys,
+ const struct conn_key *key,
+ uint32_t basis);
+
+static void
+nat_conn_keys_remove(struct hmap *nat_conn_keys,
+const struct conn_key *key,
+uint32_t basis);
+
+static bool
+nat_select_range_tuple(struct conntrack *ct, const struct conn *conn,
+   struct conn *nat_conn);
+
 static struct ct_l4_proto *l4_protos[] = {
 [IPPROTO_TCP] = _proto_tcp,
 [IPPROTO_UDP] = _proto_other,
@@ -90,7 +104,7 @@ long long ct_timeout_val[] = {
 };
 
 /* If the total number of connections goes above this value, no new connections
- * are accepted */
+ * are accepted; this is for CT_CONN_TYPE_DEFAULT connections. */
 #define DEFAULT_N_CONN_LIMIT 300
 
 /* Initializes the connection tracker 'ct'.  The caller is responsible for
@@ -101,6 +115,11 @@ conntrack_init(struct conntrack *ct)
 unsigned i, j;
 long long now = time_msec();
 
+ct_rwlock_init(>nat_resources_lock);
+ct_rwlock_wrlock(>nat_resources_lock);
+hmap_init(>nat_conn_keys);
+ct_rwlock_unlock(>nat_resources_lock);
+
 for (i = 0; i < CONNTRACK_BUCKETS; i++) {
 struct conntrack_bucket *ctb = >buckets[i];
 
@@ -139,13 +158,24 @@ conntrack_destroy(struct conntrack *ct)
 ovs_mutex_destroy(>cleanup_mutex);
 ct_lock_lock(>lock);
 HMAP_FOR_EACH_POP(conn, node, >connections) {
-atomic_count_dec(>n_conn);
+if (conn->conn_type == CT_CONN_TYPE_DEFAULT) {
+atomic_count_dec(>n_conn);
+}
 delete_conn(conn);
 }
 hmap_destroy(>connections);
 ct_lock_unlock(>lock);
 ct_lock_destroy(>lock);
 }
+ct_rwlock_wrlock(>nat_resources_lock);
+struct nat_conn_key_node *nat_conn_key_node;
+HMAP_FOR_EACH_POP(nat_conn_key_node, node, >nat_conn_keys) {
+free(nat_conn_key_node);
+}
+hmap_destroy(>nat_conn_keys);
+ct_rwlock_unlock(>nat_resources_lock);
+ct_rwlock_destroy(>nat_resources_lock);
+
 }
 
 static unsigned hash_to_bucket(uint32_t hash)
@@ -158,29 +188,186 @@ static unsigned hash_to_bucket(uint32_t hash)
 }
 
 static void
-write_ct_md(struct dp_packet *pkt, uint16_t state, uint16_t zone,
-uint32_t mark, ovs_u128 label)
+write_ct_md(struct dp_packet *pkt, uint16_t zone, uint32_t mark,
+ovs_u128 label)
 {
-pkt->md.ct_state = state | CS_TRACKED;
+pkt->md.ct_state |= CS_TRACKED;
 pkt->md.ct_zone = zone;
 pkt->md.ct_mark = mark;
 pkt->md.ct_label = label;
 }
 
+static void
+nat_packet(struct dp_packet *pkt, const struct conn *conn)
+{
+if (conn->nat_info->nat_action & NAT_ACTION_SRC) {
+pkt->md.ct_state |= CS_SRC_NAT;
+if (conn->key.dl_type == htons(ETH_TYPE_IP)) {
+struct ip_header *nh = dp_packet_l3(pkt);
+packet_set_ipv4_addr(pkt, >ip_src,
+