| Message ID | 20231003190557.423232-1-aconole@redhat.com |
|---|---|
| State | Accepted |
| Commit | f179c7c07fa07485575e7f5b5e4073986179366f |
| Headers | show |
| Series | [ovs-dev,v3,branch-2.17,1/2] conntrack: simplify cleanup path | expand |
| 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 | fail | test: fail |
| ovsrobot/intel-ovs-compilation | success | test: success |
On Tue, Oct 3, 2023 at 9:06 PM Aaron Conole <aconole@redhat.com> wrote: > > The conntrack cleanup and allocation code is spread across multiple > list invocations. This was changed in mainline code when the timeout > expiration lists were refactored, but backporting those fixes would > be a rather large effort. Instead, take only the changes we need > to backport "contrack: Remove nat_conn introducing key directionality" > into branch-2.17. Thanks alot for your help in backporting this patch. We have a managed customer environment where circumstances make the issue trigger with a rate of 70% when performing a certain action. Up until now they have been running with a temporary package containing the patches from https://patchwork.ozlabs.org/project/openvswitch/list/?series=351579&state=* To test this series, they have first re-confirmed that they see the issue with a packaged version of OVS 2.17.7, and then switched to a packaged version of OVS 2.17.7 with these patches and confirmed that the issue is no longer occurring. The same package has been in production use for the past week, being exposed to real world traffic. No side effects or incidents to report. Tested-by: Frode Nordahl <frode.nordahl@canonical.com>
On Thu, Oct 12, 2023 at 09:45:05AM +0200, Frode Nordahl wrote: > On Tue, Oct 3, 2023 at 9:06 PM Aaron Conole <aconole@redhat.com> wrote: > > > > The conntrack cleanup and allocation code is spread across multiple > > list invocations. This was changed in mainline code when the timeout > > expiration lists were refactored, but backporting those fixes would > > be a rather large effort. Instead, take only the changes we need > > to backport "contrack: Remove nat_conn introducing key directionality" > > into branch-2.17. > > Thanks alot for your help in backporting this patch. > > We have a managed customer environment where circumstances make the > issue trigger with a rate of 70% when performing a certain action. Up > until now they have been running with a temporary package containing > the patches from > https://patchwork.ozlabs.org/project/openvswitch/list/?series=351579&state=* > > To test this series, they have first re-confirmed that they see the > issue with a packaged version of OVS 2.17.7, and then switched to a > packaged version of OVS 2.17.7 with these patches and confirmed that > the issue is no longer occurring. The same package has been in > production use for the past week, being exposed to real world traffic. > No side effects or incidents to report. > > Tested-by: Frode Nordahl <frode.nordahl@canonical.com> Thanks Frode, I think we can go ahead and apply this series to branch-2.17. Acked-by: Simon Horman <horms@ovn.org>
Frode Nordahl <frode.nordahl@canonical.com> writes: > On Tue, Oct 3, 2023 at 9:06 PM Aaron Conole <aconole@redhat.com> wrote: >> >> The conntrack cleanup and allocation code is spread across multiple >> list invocations. This was changed in mainline code when the timeout >> expiration lists were refactored, but backporting those fixes would >> be a rather large effort. Instead, take only the changes we need >> to backport "contrack: Remove nat_conn introducing key directionality" >> into branch-2.17. > > Thanks alot for your help in backporting this patch. > > We have a managed customer environment where circumstances make the > issue trigger with a rate of 70% when performing a certain action. Up > until now they have been running with a temporary package containing > the patches from > https://patchwork.ozlabs.org/project/openvswitch/list/?series=351579&state=* > > To test this series, they have first re-confirmed that they see the > issue with a packaged version of OVS 2.17.7, and then switched to a > packaged version of OVS 2.17.7 with these patches and confirmed that > the issue is no longer occurring. The same package has been in > production use for the past week, being exposed to real world traffic. > No side effects or incidents to report. > > Tested-by: Frode Nordahl <frode.nordahl@canonical.com> > Thanks Frode, Aaron and Simon. On my side, I don't see any issues with the series, both patches look good to me. > -- > Frode Nordahl > >> Signed-off-by: Aaron Conole <aconole@redhat.com> >> Co-authored-by: Paolo Valerio <pvalerio@redhat.com> >> Signed-off-by: Paolo Valerio <pvalerio@redhat.com> >> --- >> lib/conntrack.c | 60 +++++++++++++++---------------------------------- >> 1 file changed, 18 insertions(+), 42 deletions(-) >> >> diff --git a/lib/conntrack.c b/lib/conntrack.c >> index fff8e77db1..83a73995d6 100644 >> --- a/lib/conntrack.c >> +++ b/lib/conntrack.c >> @@ -94,9 +94,8 @@ static bool valid_new(struct dp_packet *pkt, struct conn_key *); >> static struct conn *new_conn(struct conntrack *ct, struct dp_packet *pkt, >> struct conn_key *, long long now, >> uint32_t tp_id); >> -static void delete_conn_cmn(struct conn *); >> +static void delete_conn__(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, >> @@ -444,9 +443,11 @@ zone_limit_delete(struct conntrack *ct, uint16_t zone) >> } >> >> static void >> -conn_clean_cmn(struct conntrack *ct, struct conn *conn) >> +conn_clean(struct conntrack *ct, struct conn *conn) >> OVS_REQUIRES(ct->ct_lock) >> { >> + ovs_assert(conn->conn_type == CT_CONN_TYPE_DEFAULT); >> + >> if (conn->alg) { >> expectation_clean(ct, &conn->key); >> } >> @@ -458,19 +459,9 @@ conn_clean_cmn(struct conntrack *ct, struct conn *conn) >> if (zl && zl->czl.zone_limit_seq == conn->zone_limit_seq) { >> zl->czl.count--; >> } >> -} >> >> -/* Must be called with 'conn' of 'conn_type' CT_CONN_TYPE_DEFAULT. Also >> - * removes the associated nat 'conn' from the lookup datastructures. */ >> -static void >> -conn_clean(struct conntrack *ct, struct conn *conn) >> - OVS_REQUIRES(ct->ct_lock) >> -{ >> - ovs_assert(conn->conn_type == CT_CONN_TYPE_DEFAULT); >> - >> - conn_clean_cmn(ct, conn); >> if (conn->nat_conn) { >> - uint32_t hash = conn_key_hash(&conn->nat_conn->key, ct->hash_basis); >> + hash = conn_key_hash(&conn->nat_conn->key, ct->hash_basis); >> cmap_remove(&ct->conns, &conn->nat_conn->cm_node, hash); >> } >> ovs_list_remove(&conn->exp_node); >> @@ -479,19 +470,6 @@ conn_clean(struct conntrack *ct, struct conn *conn) >> atomic_count_dec(&ct->n_conn); >> } >> >> -static void >> -conn_clean_one(struct conntrack *ct, struct conn *conn) >> - OVS_REQUIRES(ct->ct_lock) >> -{ >> - conn_clean_cmn(ct, conn); >> - if (conn->conn_type == CT_CONN_TYPE_DEFAULT) { >> - ovs_list_remove(&conn->exp_node); >> - conn->cleaned = true; >> - atomic_count_dec(&ct->n_conn); >> - } >> - ovsrcu_postpone(delete_conn_one, conn); >> -} >> - >> /* Destroys the connection tracker 'ct' and frees all the allocated memory. >> * The caller of this function must already have shut down packet input >> * and PMD threads (which would have been quiesced). */ >> @@ -505,7 +483,11 @@ conntrack_destroy(struct conntrack *ct) >> >> ovs_mutex_lock(&ct->ct_lock); >> CMAP_FOR_EACH (conn, cm_node, &ct->conns) { >> - conn_clean_one(ct, conn); >> + if (conn->conn_type != CT_CONN_TYPE_DEFAULT) { >> + continue; >> + } >> + >> + conn_clean(ct, conn); >> } >> cmap_destroy(&ct->conns); >> >> @@ -1009,7 +991,7 @@ conn_not_found(struct conntrack *ct, struct dp_packet *pkt, >> nat_res_exhaustion: >> free(nat_conn); >> ovs_list_remove(&nc->exp_node); >> - delete_conn_cmn(nc); >> + delete_conn__(nc); >> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5); >> VLOG_WARN_RL(&rl, "Unable to NAT due to tuple space exhaustion - " >> "if DoS attack, use firewalling and/or zone partitioning."); >> @@ -2475,7 +2457,7 @@ new_conn(struct conntrack *ct, struct dp_packet *pkt, struct conn_key *key, >> } >> >> static void >> -delete_conn_cmn(struct conn *conn) >> +delete_conn__(struct conn *conn) >> { >> free(conn->alg); >> free(conn); >> @@ -2487,17 +2469,7 @@ delete_conn(struct conn *conn) >> ovs_assert(conn->conn_type == CT_CONN_TYPE_DEFAULT); >> ovs_mutex_destroy(&conn->lock); >> free(conn->nat_conn); >> - delete_conn_cmn(conn); >> -} >> - >> -/* Only used by conn_clean_one(). */ >> -static void >> -delete_conn_one(struct conn *conn) >> -{ >> - if (conn->conn_type == CT_CONN_TYPE_DEFAULT) { >> - ovs_mutex_destroy(&conn->lock); >> - } >> - delete_conn_cmn(conn); >> + delete_conn__(conn); >> } >> >> /* Convert a conntrack address 'a' into an IP address 'b' based on 'dl_type'. >> @@ -2673,8 +2645,12 @@ conntrack_flush(struct conntrack *ct, const uint16_t *zone) >> >> ovs_mutex_lock(&ct->ct_lock); >> CMAP_FOR_EACH (conn, cm_node, &ct->conns) { >> + if (conn->conn_type != CT_CONN_TYPE_DEFAULT) { >> + continue; >> + } >> + >> if (!zone || *zone == conn->key.zone) { >> - conn_clean_one(ct, conn); >> + conn_clean(ct, conn); >> } >> } >> ovs_mutex_unlock(&ct->ct_lock); >> -- >> 2.40.1 >>
Simon Horman <horms@ovn.org> writes: > On Thu, Oct 12, 2023 at 09:45:05AM +0200, Frode Nordahl wrote: >> On Tue, Oct 3, 2023 at 9:06 PM Aaron Conole <aconole@redhat.com> wrote: >> > >> > The conntrack cleanup and allocation code is spread across multiple >> > list invocations. This was changed in mainline code when the timeout >> > expiration lists were refactored, but backporting those fixes would >> > be a rather large effort. Instead, take only the changes we need >> > to backport "contrack: Remove nat_conn introducing key directionality" >> > into branch-2.17. >> >> Thanks alot for your help in backporting this patch. >> >> We have a managed customer environment where circumstances make the >> issue trigger with a rate of 70% when performing a certain action. Up >> until now they have been running with a temporary package containing >> the patches from >> https://patchwork.ozlabs.org/project/openvswitch/list/?series=351579&state=* >> >> To test this series, they have first re-confirmed that they see the >> issue with a packaged version of OVS 2.17.7, and then switched to a >> packaged version of OVS 2.17.7 with these patches and confirmed that >> the issue is no longer occurring. The same package has been in >> production use for the past week, being exposed to real world traffic. >> No side effects or incidents to report. >> >> Tested-by: Frode Nordahl <frode.nordahl@canonical.com> > > Thanks Frode, > > I think we can go ahead and apply this series to branch-2.17. > > Acked-by: Simon Horman <horms@ovn.org> Thanks Simon, Frode, and Paolo - I've merged this series to branch-2.17.
diff --git a/lib/conntrack.c b/lib/conntrack.c index fff8e77db1..83a73995d6 100644 --- a/lib/conntrack.c +++ b/lib/conntrack.c @@ -94,9 +94,8 @@ static bool valid_new(struct dp_packet *pkt, struct conn_key *); static struct conn *new_conn(struct conntrack *ct, struct dp_packet *pkt, struct conn_key *, long long now, uint32_t tp_id); -static void delete_conn_cmn(struct conn *); +static void delete_conn__(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, @@ -444,9 +443,11 @@ zone_limit_delete(struct conntrack *ct, uint16_t zone) } static void -conn_clean_cmn(struct conntrack *ct, struct conn *conn) +conn_clean(struct conntrack *ct, struct conn *conn) OVS_REQUIRES(ct->ct_lock) { + ovs_assert(conn->conn_type == CT_CONN_TYPE_DEFAULT); + if (conn->alg) { expectation_clean(ct, &conn->key); } @@ -458,19 +459,9 @@ conn_clean_cmn(struct conntrack *ct, struct conn *conn) if (zl && zl->czl.zone_limit_seq == conn->zone_limit_seq) { zl->czl.count--; } -} -/* Must be called with 'conn' of 'conn_type' CT_CONN_TYPE_DEFAULT. Also - * removes the associated nat 'conn' from the lookup datastructures. */ -static void -conn_clean(struct conntrack *ct, struct conn *conn) - OVS_REQUIRES(ct->ct_lock) -{ - ovs_assert(conn->conn_type == CT_CONN_TYPE_DEFAULT); - - conn_clean_cmn(ct, conn); if (conn->nat_conn) { - uint32_t hash = conn_key_hash(&conn->nat_conn->key, ct->hash_basis); + hash = conn_key_hash(&conn->nat_conn->key, ct->hash_basis); cmap_remove(&ct->conns, &conn->nat_conn->cm_node, hash); } ovs_list_remove(&conn->exp_node); @@ -479,19 +470,6 @@ conn_clean(struct conntrack *ct, struct conn *conn) atomic_count_dec(&ct->n_conn); } -static void -conn_clean_one(struct conntrack *ct, struct conn *conn) - OVS_REQUIRES(ct->ct_lock) -{ - conn_clean_cmn(ct, conn); - if (conn->conn_type == CT_CONN_TYPE_DEFAULT) { - ovs_list_remove(&conn->exp_node); - conn->cleaned = true; - atomic_count_dec(&ct->n_conn); - } - ovsrcu_postpone(delete_conn_one, conn); -} - /* Destroys the connection tracker 'ct' and frees all the allocated memory. * The caller of this function must already have shut down packet input * and PMD threads (which would have been quiesced). */ @@ -505,7 +483,11 @@ conntrack_destroy(struct conntrack *ct) ovs_mutex_lock(&ct->ct_lock); CMAP_FOR_EACH (conn, cm_node, &ct->conns) { - conn_clean_one(ct, conn); + if (conn->conn_type != CT_CONN_TYPE_DEFAULT) { + continue; + } + + conn_clean(ct, conn); } cmap_destroy(&ct->conns); @@ -1009,7 +991,7 @@ conn_not_found(struct conntrack *ct, struct dp_packet *pkt, nat_res_exhaustion: free(nat_conn); ovs_list_remove(&nc->exp_node); - delete_conn_cmn(nc); + delete_conn__(nc); static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5); VLOG_WARN_RL(&rl, "Unable to NAT due to tuple space exhaustion - " "if DoS attack, use firewalling and/or zone partitioning."); @@ -2475,7 +2457,7 @@ new_conn(struct conntrack *ct, struct dp_packet *pkt, struct conn_key *key, } static void -delete_conn_cmn(struct conn *conn) +delete_conn__(struct conn *conn) { free(conn->alg); free(conn); @@ -2487,17 +2469,7 @@ delete_conn(struct conn *conn) ovs_assert(conn->conn_type == CT_CONN_TYPE_DEFAULT); ovs_mutex_destroy(&conn->lock); free(conn->nat_conn); - delete_conn_cmn(conn); -} - -/* Only used by conn_clean_one(). */ -static void -delete_conn_one(struct conn *conn) -{ - if (conn->conn_type == CT_CONN_TYPE_DEFAULT) { - ovs_mutex_destroy(&conn->lock); - } - delete_conn_cmn(conn); + delete_conn__(conn); } /* Convert a conntrack address 'a' into an IP address 'b' based on 'dl_type'. @@ -2673,8 +2645,12 @@ conntrack_flush(struct conntrack *ct, const uint16_t *zone) ovs_mutex_lock(&ct->ct_lock); CMAP_FOR_EACH (conn, cm_node, &ct->conns) { + if (conn->conn_type != CT_CONN_TYPE_DEFAULT) { + continue; + } + if (!zone || *zone == conn->key.zone) { - conn_clean_one(ct, conn); + conn_clean(ct, conn); } } ovs_mutex_unlock(&ct->ct_lock);