Re: [ovs-dev] [PATCH RFC 3/5] conntrack: Replaces nat_conn introducing key directionality.
Hi Peng,
贺鹏 writes:
> Hi, Paolo
>
> well done.
>
> the patch looks good to me.
> and I am going through your other patches.
thanks for the initial work and for the review, I appreciate.
> I wonder maybe finally we need a *conn_flags* to store all the informations
> containing NAT, cleaned flag, etc.
>
IMO, that is a good idea and it is worth posting it as separate
patch.
> Paolo Valerio 于2021年11月30日周二 02:06写道：
>
> From: Peng He
>
> Currently, when doing NAT, the userspace conntrack will use an extra
> conn for the two directions in a flow. However, each conn has actually
> the two keys for both orig and rev directions. This patch introduces a
> key_node[CT_DIR_MAX] member in the conn which consists of key and a
> cmap_node for hash lookup. Both keys can now be accessed in the
> following way:
>
> conn->key_node[CT_DIR_{FWD,REV}].key
>
> similarly to what Aaron Conole suggested.
>
> This patch avoids the extra allocation for nat_conn, and makes
> userspace code cleaner.
>
> Signed-off-by: Peng He
> Co-authored-by: Paolo Valerio
> Signed-off-by: Paolo Valerio
> ---
> Initial patch:
>
>
> https://patchwork.ozlabs.org/project/openvswitch/patch/20201129033255.64647-2-hepeng.0...@bytedance.com/
> ---
> lib/conntrack-private.h | 20 +-
> lib/conntrack-tp.c | 6 -
> lib/conntrack.c | 499
> ---
> 3 files changed, 229 insertions(+), 296 deletions(-)
>
> diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
> index 34c688821..ea5ba3d9e 100644
> --- a/lib/conntrack-private.h
> +++ b/lib/conntrack-private.h
> @@ -48,9 +48,16 @@ struct ct_endpoint {
> * hashing in ct_endpoint_hash_add(). */
> BUILD_ASSERT_DECL(sizeof(struct ct_endpoint) == sizeof(union ct_addr) +
> 4);
>
> +enum key_dir {
> + CT_DIR_FWD = 0,
> + CT_DIR_REV,
> + CT_DIR_MAX,
> +};
> +
> /* Changes to this structure need to be reflected in conn_key_hash()
> * and conn_key_cmp(). */
> struct conn_key {
> + enum key_dir dir;
> struct ct_endpoint src;
> struct ct_endpoint dst;
>
> @@ -86,21 +93,19 @@ struct alg_exp_node {
> bool nat_rpl_dst;
> };
>
> -enum OVS_PACKED_ENUM ct_conn_type {
> - CT_CONN_TYPE_DEFAULT,
> - CT_CONN_TYPE_UN_NAT,
> +struct conn_key_node {
> + struct conn_key key;
> + struct cmap_node cm_node;
> };
>
> struct conn {
> /* Immutable data. */
> - struct conn_key key;
> - struct conn_key rev_key;
> + struct conn_key_node key_node[CT_DIR_MAX];
> struct conn_key parent_key; /* Only used for orig_tuple support. */
> struct ovs_list exp_node;
> - struct cmap_node cm_node;
> +
> uint16_t nat_action;
> char *alg;
> - struct conn *nat_conn; /* The NAT 'conn' context, if there is one. */
>
> /* Mutable data. */
> struct ovs_mutex lock; /* Guards all mutable fields. */
> @@ -120,7 +125,6 @@ struct conn {
>
> /* Immutable data. */
> bool alg_related; /* True if alg data connection. */
> - enum ct_conn_type conn_type;
>
> uint32_t tp_id; /* Timeout policy ID. */
> };
> diff --git a/lib/conntrack-tp.c b/lib/conntrack-tp.c
> index c2245038b..9ecb06978 100644
> --- a/lib/conntrack-tp.c
> +++ b/lib/conntrack-tp.c
> @@ -282,7 +282,8 @@ conn_update_expiration(struct conntrack *ct, struct
> conn *conn,
> ovs_mutex_lock(>lock);
> VLOG_DBG_RL(, "Update timeout %s zone=%u with policy id=%d "
> "val=%u sec.",
> - ct_timeout_str[tm], conn->key.zone, conn->tp_id, val);
> + ct_timeout_str[tm], conn->key_node[CT_DIR_FWD].key.zone,
> + conn->tp_id, val);
>
> conn_update_expiration__(ct, conn, tm, now, val);
> }
> @@ -313,7 +314,8 @@ conn_init_expiration(struct conntrack *ct, struct
> conn *conn,
> }
>
> VLOG_DBG_RL(, "Init timeout %s zone=%u with policy id=%d val=%u
> sec.",
> - ct_timeout_str[tm], conn->key.zone, conn->tp_id, val);
> + ct_timeout_str[tm], conn->key_node[CT_DIR_FWD].key.zone,
> + conn->tp_id, val);
>
> conn_init_expiration__(ct, conn, tm, now, val);
> }
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 983f824d2..a284c57c0 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -96,7 +96,6 @@ static struct conn *new_conn(struct conntrack *ct,
> struct dp_packet *pkt,
> uint32_t tp_id);
> static void delete_conn_cmn(struct conn *);
> static void delete_conn(struct conn *);
> -static void
Re: [ovs-dev] [PATCH RFC 3/5] conntrack: Replaces nat_conn introducing key directionality.
Hi, Paolo
well done.
the patch looks good to me.
and I am going through your other patches.
I wonder maybe finally we need a *conn_flags* to store all the informations
containing NAT, cleaned flag, etc.
Paolo Valerio 于2021年11月30日周二 02:06写道：
> From: Peng He
>
> Currently, when doing NAT, the userspace conntrack will use an extra
> conn for the two directions in a flow. However, each conn has actually
> the two keys for both orig and rev directions. This patch introduces a
> key_node[CT_DIR_MAX] member in the conn which consists of key and a
> cmap_node for hash lookup. Both keys can now be accessed in the
> following way:
>
> conn->key_node[CT_DIR_{FWD,REV}].key
>
> similarly to what Aaron Conole suggested.
>
> This patch avoids the extra allocation for nat_conn, and makes
> userspace code cleaner.
>
> Signed-off-by: Peng He
> Co-authored-by: Paolo Valerio
> Signed-off-by: Paolo Valerio
> ---
> Initial patch:
>
>
> https://patchwork.ozlabs.org/project/openvswitch/patch/20201129033255.64647-2-hepeng.0...@bytedance.com/
> ---
> lib/conntrack-private.h | 20 +-
> lib/conntrack-tp.c |6 -
> lib/conntrack.c | 499
> ---
> 3 files changed, 229 insertions(+), 296 deletions(-)
>
> diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
> index 34c688821..ea5ba3d9e 100644
> --- a/lib/conntrack-private.h
> +++ b/lib/conntrack-private.h
> @@ -48,9 +48,16 @@ struct ct_endpoint {
> * hashing in ct_endpoint_hash_add(). */
> BUILD_ASSERT_DECL(sizeof(struct ct_endpoint) == sizeof(union ct_addr) +
> 4);
>
> +enum key_dir {
> +CT_DIR_FWD = 0,
> +CT_DIR_REV,
> +CT_DIR_MAX,
> +};
> +
> /* Changes to this structure need to be reflected in conn_key_hash()
> * and conn_key_cmp(). */
> struct conn_key {
> +enum key_dir dir;
> struct ct_endpoint src;
> struct ct_endpoint dst;
>
> @@ -86,21 +93,19 @@ struct alg_exp_node {
> bool nat_rpl_dst;
> };
>
> -enum OVS_PACKED_ENUM ct_conn_type {
> -CT_CONN_TYPE_DEFAULT,
> -CT_CONN_TYPE_UN_NAT,
> +struct conn_key_node {
> +struct conn_key key;
> +struct cmap_node cm_node;
> };
>
> struct conn {
> /* Immutable data. */
> -struct conn_key key;
> -struct conn_key rev_key;
> +struct conn_key_node key_node[CT_DIR_MAX];
> struct conn_key parent_key; /* Only used for orig_tuple support. */
> struct ovs_list exp_node;
> -struct cmap_node cm_node;
> +
> uint16_t nat_action;
> char *alg;
> -struct conn *nat_conn; /* The NAT 'conn' context, if there is one. */
>
> /* Mutable data. */
> struct ovs_mutex lock; /* Guards all mutable fields. */
> @@ -120,7 +125,6 @@ struct conn {
>
> /* Immutable data. */
> bool alg_related; /* True if alg data connection. */
> -enum ct_conn_type conn_type;
>
> uint32_t tp_id; /* Timeout policy ID. */
> };
> diff --git a/lib/conntrack-tp.c b/lib/conntrack-tp.c
> index c2245038b..9ecb06978 100644
> --- a/lib/conntrack-tp.c
> +++ b/lib/conntrack-tp.c
> @@ -282,7 +282,8 @@ conn_update_expiration(struct conntrack *ct, struct
> conn *conn,
> ovs_mutex_lock(>lock);
> VLOG_DBG_RL(, "Update timeout %s zone=%u with policy id=%d "
> "val=%u sec.",
> -ct_timeout_str[tm], conn->key.zone, conn->tp_id, val);
> +ct_timeout_str[tm], conn->key_node[CT_DIR_FWD].key.zone,
> +conn->tp_id, val);
>
> conn_update_expiration__(ct, conn, tm, now, val);
> }
> @@ -313,7 +314,8 @@ conn_init_expiration(struct conntrack *ct, struct conn
> *conn,
> }
>
> VLOG_DBG_RL(, "Init timeout %s zone=%u with policy id=%d val=%u
> sec.",
> -ct_timeout_str[tm], conn->key.zone, conn->tp_id, val);
> +ct_timeout_str[tm], conn->key_node[CT_DIR_FWD].key.zone,
> +conn->tp_id, val);
>
> conn_init_expiration__(ct, conn, tm, now, val);
> }
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 983f824d2..a284c57c0 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -96,7 +96,6 @@ static struct conn *new_conn(struct conntrack *ct,
> struct dp_packet *pkt,
> uint32_t tp_id);
> static void delete_conn_cmn(struct conn *);
> static void delete_conn(struct conn *);
> -static void delete_conn_one(struct conn *conn);
> static enum ct_update_res conn_update(struct conntrack *ct, struct conn
> *conn,
>struct dp_packet *pkt,
>struct conn_lookup_ctx *ctx,
> @@ -110,8 +109,7 @@ static void set_label(struct dp_packet *, struct conn
> *,
> static void *clean_thread_main(void *f_);
>
> static bool
> -nat_get_unique_tuple(struct conntrack *ct, const struct conn *conn,
> - struct conn *nat_conn,
> +nat_get_unique_tuple(struct conntrack *ct, struct conn *conn,
> const struct nat_action_info_t *nat_info);
>
> static uint8_t
>
[ovs-dev] [PATCH RFC 3/5] conntrack: Replaces nat_conn introducing key directionality.
From: Peng He
Currently, when doing NAT, the userspace conntrack will use an extra
conn for the two directions in a flow. However, each conn has actually
the two keys for both orig and rev directions. This patch introduces a
key_node[CT_DIR_MAX] member in the conn which consists of key and a
cmap_node for hash lookup. Both keys can now be accessed in the
following way:
conn->key_node[CT_DIR_{FWD,REV}].key
similarly to what Aaron Conole suggested.
This patch avoids the extra allocation for nat_conn, and makes
userspace code cleaner.
Signed-off-by: Peng He
Co-authored-by: Paolo Valerio
Signed-off-by: Paolo Valerio
---
Initial patch:
https://patchwork.ozlabs.org/project/openvswitch/patch/20201129033255.64647-2-hepeng.0...@bytedance.com/
---
lib/conntrack-private.h | 20 +-
lib/conntrack-tp.c |6 -
lib/conntrack.c | 499 ---
3 files changed, 229 insertions(+), 296 deletions(-)
diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
index 34c688821..ea5ba3d9e 100644
--- a/lib/conntrack-private.h
+++ b/lib/conntrack-private.h
@@ -48,9 +48,16 @@ struct ct_endpoint {
* hashing in ct_endpoint_hash_add(). */
BUILD_ASSERT_DECL(sizeof(struct ct_endpoint) == sizeof(union ct_addr) + 4);
+enum key_dir {
+CT_DIR_FWD = 0,
+CT_DIR_REV,
+CT_DIR_MAX,
+};
+
/* Changes to this structure need to be reflected in conn_key_hash()
* and conn_key_cmp(). */
struct conn_key {
+enum key_dir dir;
struct ct_endpoint src;
struct ct_endpoint dst;
@@ -86,21 +93,19 @@ struct alg_exp_node {
bool nat_rpl_dst;
};
-enum OVS_PACKED_ENUM ct_conn_type {
-CT_CONN_TYPE_DEFAULT,
-CT_CONN_TYPE_UN_NAT,
+struct conn_key_node {
+struct conn_key key;
+struct cmap_node cm_node;
};
struct conn {
/* Immutable data. */
-struct conn_key key;
-struct conn_key rev_key;
+struct conn_key_node key_node[CT_DIR_MAX];
struct conn_key parent_key; /* Only used for orig_tuple support. */
struct ovs_list exp_node;
-struct cmap_node cm_node;
+
uint16_t nat_action;
char *alg;
-struct conn *nat_conn; /* The NAT 'conn' context, if there is one. */
/* Mutable data. */
struct ovs_mutex lock; /* Guards all mutable fields. */
@@ -120,7 +125,6 @@ struct conn {
/* Immutable data. */
bool alg_related; /* True if alg data connection. */
-enum ct_conn_type conn_type;
uint32_t tp_id; /* Timeout policy ID. */
};
diff --git a/lib/conntrack-tp.c b/lib/conntrack-tp.c
index c2245038b..9ecb06978 100644
--- a/lib/conntrack-tp.c
+++ b/lib/conntrack-tp.c
@@ -282,7 +282,8 @@ conn_update_expiration(struct conntrack *ct, struct conn
*conn,
ovs_mutex_lock(>lock);
VLOG_DBG_RL(, "Update timeout %s zone=%u with policy id=%d "
"val=%u sec.",
-ct_timeout_str[tm], conn->key.zone, conn->tp_id, val);
+ct_timeout_str[tm], conn->key_node[CT_DIR_FWD].key.zone,
+conn->tp_id, val);
conn_update_expiration__(ct, conn, tm, now, val);
}
@@ -313,7 +314,8 @@ conn_init_expiration(struct conntrack *ct, struct conn
*conn,
}
VLOG_DBG_RL(, "Init timeout %s zone=%u with policy id=%d val=%u sec.",
-ct_timeout_str[tm], conn->key.zone, conn->tp_id, val);
+ct_timeout_str[tm], conn->key_node[CT_DIR_FWD].key.zone,
+conn->tp_id, val);
conn_init_expiration__(ct, conn, tm, now, val);
}
diff --git a/lib/conntrack.c b/lib/conntrack.c
index 983f824d2..a284c57c0 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -96,7 +96,6 @@ static struct conn *new_conn(struct conntrack *ct, struct
dp_packet *pkt,
uint32_t tp_id);
static void delete_conn_cmn(struct conn *);
static void delete_conn(struct conn *);
-static void delete_conn_one(struct conn *conn);
static enum ct_update_res conn_update(struct conntrack *ct, struct conn *conn,
struct dp_packet *pkt,
struct conn_lookup_ctx *ctx,
@@ -110,8 +109,7 @@ static void set_label(struct dp_packet *, struct conn *,
static void *clean_thread_main(void *f_);
static bool
-nat_get_unique_tuple(struct conntrack *ct, const struct conn *conn,
- struct conn *nat_conn,
+nat_get_unique_tuple(struct conntrack *ct, struct conn *conn,
const struct nat_action_info_t *nat_info);
static uint8_t
@@ -231,61 +229,6 @@ conn_key_cmp(const struct conn_key *key1, const struct
conn_key *key2)
return 1;
}
-static void
-ct_print_conn_info(const struct conn *c, const char *log_msg,
- enum vlog_level vll, bool force, bool rl_on)
-{
-#define CT_VLOG(RL_ON, LEVEL, ...) \
-do {\
-if (RL_ON) {\
-
