Re: [ovs-dev] [PATCH v3] conntrack: Remove nat_conn introducing key directionality.
Simon Horman writes: > + Xavier > > On Thu, Aug 31, 2023 at 02:52:59PM -0400, Aaron Conole wrote: >> Ilya Maximets writes: >> >> > On 8/31/23 09:15, Frode Nordahl wrote: >> >> On Wed, Aug 30, 2023 at 9:30 PM Paolo Valerio wrote: >> >>> >> >>> From: hepeng >> >>> >> >>> The patch avoids the extra allocation for nat_conn. >> >>> 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_DIRS] member as per Aaron's suggestion in the conn which >> >>> consists of a key, direction, and a cmap_node for hash lookup so >> >>> addressing the feedback received by the original patch [0]. >> >>> >> >>> [0] >> >>> https://patchwork.ozlabs.org/project/openvswitch/patch/20201129033255.64647-2-hepeng.0...@bytedance.com/ >> >>> >> >>> Signed-off-by: Peng He >> >>> Co-authored-by: Paolo Valerio >> >>> Signed-off-by: Paolo Valerio >> >> >> >> Thanks alot for working on this, should we perhaps reference the >> >> original bug report, i.e: >> >> Reported-by: Michael Plato >> >> Reported-at: >> >> https://mail.openvswitch.org/pipermail/ovs-discuss/2022-September/052065.html >> > >> > Can be added while applying, I think. It also may be worth adding >> > a sentence about fixing the assertion to the commit message. >> >> Done. >> >> >> >> >> We have a reproducer for the issue and we no longer see it occurring >> >> with this patch. >> >> Tested-by: Frode Nordahl >> > >> > Thanks! >> >> Thanks everyone! I've applied and backported down to branch-3.0, and >> will work on the backport to branch-2.17. > > Hi Aaron, > > while working on [1] I notice that this patch did not seem to be > backported to branch-3.2. I will plan on doing so as part of > my backports of [1]. > > [1] [ovs-dev,v3] conntrack: Fix flush not flushing all elements. > > https://patchwork.ozlabs.org/project/openvswitch/patch/20240304152159.1710977-1-xsimo...@redhat.com/ Strange - I would have thought I had applied it. Glad to see this get resolved. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3] conntrack: Remove nat_conn introducing key directionality.
+ Xavier On Thu, Aug 31, 2023 at 02:52:59PM -0400, Aaron Conole wrote: > Ilya Maximets writes: > > > On 8/31/23 09:15, Frode Nordahl wrote: > >> On Wed, Aug 30, 2023 at 9:30 PM Paolo Valerio wrote: > >>> > >>> From: hepeng > >>> > >>> The patch avoids the extra allocation for nat_conn. > >>> 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_DIRS] member as per Aaron's suggestion in the conn which > >>> consists of a key, direction, and a cmap_node for hash lookup so > >>> addressing the feedback received by the original patch [0]. > >>> > >>> [0] > >>> https://patchwork.ozlabs.org/project/openvswitch/patch/20201129033255.64647-2-hepeng.0...@bytedance.com/ > >>> > >>> Signed-off-by: Peng He > >>> Co-authored-by: Paolo Valerio > >>> Signed-off-by: Paolo Valerio > >> > >> Thanks alot for working on this, should we perhaps reference the > >> original bug report, i.e: > >> Reported-by: Michael Plato > >> Reported-at: > >> https://mail.openvswitch.org/pipermail/ovs-discuss/2022-September/052065.html > > > > Can be added while applying, I think. It also may be worth adding > > a sentence about fixing the assertion to the commit message. > > Done. > > >> > >> We have a reproducer for the issue and we no longer see it occurring > >> with this patch. > >> Tested-by: Frode Nordahl > > > > Thanks! > > Thanks everyone! I've applied and backported down to branch-3.0, and > will work on the backport to branch-2.17. Hi Aaron, while working on [1] I notice that this patch did not seem to be backported to branch-3.2. I will plan on doing so as part of my backports of [1]. [1] [ovs-dev,v3] conntrack: Fix flush not flushing all elements. https://patchwork.ozlabs.org/project/openvswitch/patch/20240304152159.1710977-1-xsimo...@redhat.com/ ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3] conntrack: Remove nat_conn introducing key directionality.
Ilya Maximets writes: > On 8/31/23 09:15, Frode Nordahl wrote: >> On Wed, Aug 30, 2023 at 9:30 PM Paolo Valerio wrote: >>> >>> From: hepeng >>> >>> The patch avoids the extra allocation for nat_conn. >>> 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_DIRS] member as per Aaron's suggestion in the conn which >>> consists of a key, direction, and a cmap_node for hash lookup so >>> addressing the feedback received by the original patch [0]. >>> >>> [0] >>> https://patchwork.ozlabs.org/project/openvswitch/patch/20201129033255.64647-2-hepeng.0...@bytedance.com/ >>> >>> Signed-off-by: Peng He >>> Co-authored-by: Paolo Valerio >>> Signed-off-by: Paolo Valerio >> >> Thanks alot for working on this, should we perhaps reference the >> original bug report, i.e: >> Reported-by: Michael Plato >> Reported-at: >> https://mail.openvswitch.org/pipermail/ovs-discuss/2022-September/052065.html > > Can be added while applying, I think. It also may be worth adding > a sentence about fixing the assertion to the commit message. Done. >> >> We have a reproducer for the issue and we no longer see it occurring >> with this patch. >> Tested-by: Frode Nordahl > > Thanks! Thanks everyone! I've applied and backported down to branch-3.0, and will work on the backport to branch-2.17. >> >> Is there a plan for a backport, we have users on OVS 2.17 that would >> be interested in having this fixed, and AFAICT this patch does not >> cleanly backport. > > Yes, the plan was to get this one reviewed, accepted and backported > down to 3.0. 3.1 and 3.0 seems to require only minor rebase (no support > for SCTP and parent key dumps). Then post a backport patch for 2.17 to > get it reviewed, since it will have some extra modifications. > > Aaron, I suppose your Ack from v2 still mostly relevant, but could > you please take another look at v3? > > From my side the code looks fine: > > Acked-by: Ilya Maximets > > Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3] conntrack: Remove nat_conn introducing key directionality.
On 8/31/23 09:15, Frode Nordahl wrote: > On Wed, Aug 30, 2023 at 9:30 PM Paolo Valerio wrote: >> >> From: hepeng >> >> The patch avoids the extra allocation for nat_conn. >> 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_DIRS] member as per Aaron's suggestion in the conn which >> consists of a key, direction, and a cmap_node for hash lookup so >> addressing the feedback received by the original patch [0]. >> >> [0] >> https://patchwork.ozlabs.org/project/openvswitch/patch/20201129033255.64647-2-hepeng.0...@bytedance.com/ >> >> Signed-off-by: Peng He >> Co-authored-by: Paolo Valerio >> Signed-off-by: Paolo Valerio > > Thanks alot for working on this, should we perhaps reference the > original bug report, i.e: > Reported-by: Michael Plato > Reported-at: > https://mail.openvswitch.org/pipermail/ovs-discuss/2022-September/052065.html Can be added while applying, I think. It also may be worth adding a sentence about fixing the assertion to the commit message. > > We have a reproducer for the issue and we no longer see it occurring > with this patch. > Tested-by: Frode Nordahl Thanks! > > Is there a plan for a backport, we have users on OVS 2.17 that would > be interested in having this fixed, and AFAICT this patch does not > cleanly backport. Yes, the plan was to get this one reviewed, accepted and backported down to 3.0. 3.1 and 3.0 seems to require only minor rebase (no support for SCTP and parent key dumps). Then post a backport patch for 2.17 to get it reviewed, since it will have some extra modifications. Aaron, I suppose your Ack from v2 still mostly relevant, but could you please take another look at v3? From my side the code looks fine: Acked-by: Ilya Maximets Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3] conntrack: Remove nat_conn introducing key directionality.
On Wed, Aug 30, 2023 at 9:30 PM Paolo Valerio wrote:
>
> From: hepeng
>
> The patch avoids the extra allocation for nat_conn.
> 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_DIRS] member as per Aaron's suggestion in the conn which
> consists of a key, direction, and a cmap_node for hash lookup so
> addressing the feedback received by the original patch [0].
>
> [0]
> https://patchwork.ozlabs.org/project/openvswitch/patch/20201129033255.64647-2-hepeng.0...@bytedance.com/
>
> Signed-off-by: Peng He
> Co-authored-by: Paolo Valerio
> Signed-off-by: Paolo Valerio
Thanks alot for working on this, should we perhaps reference the
original bug report, i.e:
Reported-by: Michael Plato
Reported-at:
https://mail.openvswitch.org/pipermail/ovs-discuss/2022-September/052065.html
We have a reproducer for the issue and we no longer see it occurring
with this patch.
Tested-by: Frode Nordahl
Is there a plan for a backport, we have users on OVS 2.17 that would
be interested in having this fixed, and AFAICT this patch does not
cleanly backport.
--
Frode Nordahl
> ---
> v3:
> - resolved a potentially UB with offsetof() and integer constant
> expression (Ilya)
> - int to bool assignment (Ilya)
> - check the direction early in conntrack_dump_next() to avoid
> unneeded operations (Ilya)
> - unrelated change added that turns the branch:
> if (!conn_lookup()) { return true; } else { return false; }
> into return !conn_lookup() (Ilya)
> - cosmetic/coding style changes (Ilya)
>
> v2:
> - use enum value instead of bool (Aaron).
> - s/conn_for_expectation/conn_for_exp/ in process_ftp_ctl_v6()
> to avoid long line.
> - removed CT_CONN_TYPE_* reference in two comments.
> ---
> lib/conntrack-private.h | 19 +-
> lib/conntrack-tp.c |6 +
> lib/conntrack.c | 366
> +++
> 3 files changed, 164 insertions(+), 227 deletions(-)
>
> diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
> index bb326868e..3fd5fccd3 100644
> --- a/lib/conntrack-private.h
> +++ b/lib/conntrack-private.h
> @@ -49,6 +49,12 @@ 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_DIRS,
> +};
> +
> /* Changes to this structure need to be reflected in conn_key_hash()
> * and conn_key_cmp(). */
> struct conn_key {
> @@ -112,20 +118,18 @@ enum ct_timeout {
>
> #define N_EXP_LISTS 100
>
> -enum OVS_PACKED_ENUM ct_conn_type {
> -CT_CONN_TYPE_DEFAULT,
> -CT_CONN_TYPE_UN_NAT,
> +struct conn_key_node {
> +enum key_dir dir;
> +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_DIRS];
> struct conn_key parent_key; /* Only used for orig_tuple support. */
> -struct cmap_node cm_node;
> uint16_t nat_action;
> char *alg;
> -struct conn *nat_conn; /* The NAT 'conn' context, if there is one. */
> atomic_flag reclaimed; /* False during the lifetime of the connection,
> * True as soon as a thread has started freeing
> * its memory. */
> @@ -150,7 +154,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 89cb2704a..2149fdc73 100644
> --- a/lib/conntrack-tp.c
> +++ b/lib/conntrack-tp.c
> @@ -253,7 +253,8 @@ conn_update_expiration(struct conntrack *ct, struct conn
> *conn,
> }
> 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);
>
> atomic_store_relaxed(>expiration, now + val * 1000);
> }
> @@ -273,7 +274,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->expiration = now + val * 1000;
> }
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 5f1176d33..47a443fba 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -103,7 +103,7 @@ static enum ct_update_res conn_update(struct conntrack
> *ct, struct
Re: [ovs-dev] [PATCH v3] conntrack: Remove nat_conn introducing key directionality.
Bleep bloop. Greetings Paolo Valerio, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: ERROR: Author hepeng needs to sign off. WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Peng He Lines checked: 899, Warnings: 1, Errors: 1 Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v3] conntrack: Remove nat_conn introducing key directionality.
From: hepeng
The patch avoids the extra allocation for nat_conn.
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_DIRS] member as per Aaron's suggestion in the conn which
consists of a key, direction, and a cmap_node for hash lookup so
addressing the feedback received by the original patch [0].
[0]
https://patchwork.ozlabs.org/project/openvswitch/patch/20201129033255.64647-2-hepeng.0...@bytedance.com/
Signed-off-by: Peng He
Co-authored-by: Paolo Valerio
Signed-off-by: Paolo Valerio
---
v3:
- resolved a potentially UB with offsetof() and integer constant
expression (Ilya)
- int to bool assignment (Ilya)
- check the direction early in conntrack_dump_next() to avoid
unneeded operations (Ilya)
- unrelated change added that turns the branch:
if (!conn_lookup()) { return true; } else { return false; }
into return !conn_lookup() (Ilya)
- cosmetic/coding style changes (Ilya)
v2:
- use enum value instead of bool (Aaron).
- s/conn_for_expectation/conn_for_exp/ in process_ftp_ctl_v6()
to avoid long line.
- removed CT_CONN_TYPE_* reference in two comments.
---
lib/conntrack-private.h | 19 +-
lib/conntrack-tp.c |6 +
lib/conntrack.c | 366 +++
3 files changed, 164 insertions(+), 227 deletions(-)
diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
index bb326868e..3fd5fccd3 100644
--- a/lib/conntrack-private.h
+++ b/lib/conntrack-private.h
@@ -49,6 +49,12 @@ 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_DIRS,
+};
+
/* Changes to this structure need to be reflected in conn_key_hash()
* and conn_key_cmp(). */
struct conn_key {
@@ -112,20 +118,18 @@ enum ct_timeout {
#define N_EXP_LISTS 100
-enum OVS_PACKED_ENUM ct_conn_type {
-CT_CONN_TYPE_DEFAULT,
-CT_CONN_TYPE_UN_NAT,
+struct conn_key_node {
+enum key_dir dir;
+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_DIRS];
struct conn_key parent_key; /* Only used for orig_tuple support. */
-struct cmap_node cm_node;
uint16_t nat_action;
char *alg;
-struct conn *nat_conn; /* The NAT 'conn' context, if there is one. */
atomic_flag reclaimed; /* False during the lifetime of the connection,
* True as soon as a thread has started freeing
* its memory. */
@@ -150,7 +154,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 89cb2704a..2149fdc73 100644
--- a/lib/conntrack-tp.c
+++ b/lib/conntrack-tp.c
@@ -253,7 +253,8 @@ conn_update_expiration(struct conntrack *ct, struct conn
*conn,
}
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);
atomic_store_relaxed(>expiration, now + val * 1000);
}
@@ -273,7 +274,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->expiration = now + val * 1000;
}
diff --git a/lib/conntrack.c b/lib/conntrack.c
index 5f1176d33..47a443fba 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -103,7 +103,7 @@ static enum ct_update_res conn_update(struct conntrack *ct,
struct conn *conn,
struct conn_lookup_ctx *ctx,
long long now);
static long long int conn_expiration(const struct conn *);
-static bool conn_expired(struct conn *, long long now);
+static bool conn_expired(const struct conn *, long long now);
static void conn_expire_push_front(struct conntrack *ct, struct conn *conn);
static void set_mark(struct dp_packet *, struct conn *,
uint32_t val, uint32_t mask);
@@ -113,8 +113,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
