| Message ID | 168192964179.4031872.15675810711997662503.stgit@fed.void |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | conntrack: Fix failed assertion in conn_update_state(). | 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 | success | test: success |
Paolo Valerio <pvalerio@redhat.com> writes: > Connections that need to be removed, e.g. while forcing a direction, > were invalidated forcing them to be expired. > This is not actually needed, as it's typically a one-time > operation. > The patch replaces a call to conn_force_expire() with a call to > conn_clean(). > > Signed-off-by: Paolo Valerio <pvalerio@redhat.com> > --- Is there a possible contention issue now where the conn update can also take the ct lock? IE: before, we would rely on the expiration timer processing, but now we directly release which requires the ct lock. Maybe since it is a rare enough event, this isn't as big a deal? > lib/conntrack.c | 10 ++-------- > 1 file changed, 2 insertions(+), 8 deletions(-) > > diff --git a/lib/conntrack.c b/lib/conntrack.c > index ce8a63de5..7e1fc4b1f 100644 > --- a/lib/conntrack.c > +++ b/lib/conntrack.c > @@ -514,12 +514,6 @@ conn_clean(struct conntrack *ct, struct conn *conn) > atomic_count_dec(&ct->n_conn); > } > > -static void > -conn_force_expire(struct conn *conn) > -{ > - atomic_store_relaxed(&conn->expiration, 0); > -} > - > /* 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). */ > @@ -1089,7 +1083,7 @@ conn_update_state(struct conntrack *ct, struct dp_packet *pkt, > break; > case CT_UPDATE_NEW: > if (conn_lookup(ct, &conn->key, now, NULL, NULL)) { > - conn_force_expire(conn); > + conn_clean(ct, conn); > } > create_new_conn = true; > break; > @@ -1299,7 +1293,7 @@ process_one(struct conntrack *ct, struct dp_packet *pkt, > /* Delete found entry if in wrong direction. 'force' implies commit. */ > if (OVS_UNLIKELY(force && ctx->reply && conn)) { > if (conn_lookup(ct, &conn->key, now, NULL, NULL)) { > - conn_force_expire(conn); > + conn_clean(ct, conn); > } > conn = NULL; > }
Aaron Conole <aconole@redhat.com> writes: > Paolo Valerio <pvalerio@redhat.com> writes: > >> Connections that need to be removed, e.g. while forcing a direction, >> were invalidated forcing them to be expired. >> This is not actually needed, as it's typically a one-time >> operation. >> The patch replaces a call to conn_force_expire() with a call to >> conn_clean(). >> >> Signed-off-by: Paolo Valerio <pvalerio@redhat.com> >> --- > > Is there a possible contention issue now where the conn update can also > take the ct lock? IE: before, we would rely on the expiration timer > processing, but now we directly release which requires the ct lock. > > Maybe since it is a rare enough event, this isn't as big a deal? > That's a fair point and mostly the reason I opted to split this one from the next. Assuming as common the scenario where, e.g. many connections are in TIME_WAIT and new connections with the same 5-tuple are initiated while the sweeper is actually deleting, yes. The advantage with this patch is that nconns is lowered earlier instead of waiting for the next sweep interval, and, assuming it is an actual upside, the load on the sweeper thread is reduced for those deletions. The reason I included it is that forcing the expiration makes the reported issue theoretically possible for those use case, but doesn't solve it for all the cases as the second patch should. I guess it's fine to drop this, at least for the time being. >> lib/conntrack.c | 10 ++-------- >> 1 file changed, 2 insertions(+), 8 deletions(-) >> >> diff --git a/lib/conntrack.c b/lib/conntrack.c >> index ce8a63de5..7e1fc4b1f 100644 >> --- a/lib/conntrack.c >> +++ b/lib/conntrack.c >> @@ -514,12 +514,6 @@ conn_clean(struct conntrack *ct, struct conn *conn) >> atomic_count_dec(&ct->n_conn); >> } >> >> -static void >> -conn_force_expire(struct conn *conn) >> -{ >> - atomic_store_relaxed(&conn->expiration, 0); >> -} >> - >> /* 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). */ >> @@ -1089,7 +1083,7 @@ conn_update_state(struct conntrack *ct, struct dp_packet *pkt, >> break; >> case CT_UPDATE_NEW: >> if (conn_lookup(ct, &conn->key, now, NULL, NULL)) { >> - conn_force_expire(conn); >> + conn_clean(ct, conn); >> } >> create_new_conn = true; >> break; >> @@ -1299,7 +1293,7 @@ process_one(struct conntrack *ct, struct dp_packet *pkt, >> /* Delete found entry if in wrong direction. 'force' implies commit. */ >> if (OVS_UNLIKELY(force && ctx->reply && conn)) { >> if (conn_lookup(ct, &conn->key, now, NULL, NULL)) { >> - conn_force_expire(conn); >> + conn_clean(ct, conn); >> } >> conn = NULL; >> }
diff --git a/lib/conntrack.c b/lib/conntrack.c index ce8a63de5..7e1fc4b1f 100644 --- a/lib/conntrack.c +++ b/lib/conntrack.c @@ -514,12 +514,6 @@ conn_clean(struct conntrack *ct, struct conn *conn) atomic_count_dec(&ct->n_conn); } -static void -conn_force_expire(struct conn *conn) -{ - atomic_store_relaxed(&conn->expiration, 0); -} - /* 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). */ @@ -1089,7 +1083,7 @@ conn_update_state(struct conntrack *ct, struct dp_packet *pkt, break; case CT_UPDATE_NEW: if (conn_lookup(ct, &conn->key, now, NULL, NULL)) { - conn_force_expire(conn); + conn_clean(ct, conn); } create_new_conn = true; break; @@ -1299,7 +1293,7 @@ process_one(struct conntrack *ct, struct dp_packet *pkt, /* Delete found entry if in wrong direction. 'force' implies commit. */ if (OVS_UNLIKELY(force && ctx->reply && conn)) { if (conn_lookup(ct, &conn->key, now, NULL, NULL)) { - conn_force_expire(conn); + conn_clean(ct, conn); } conn = NULL; }
Connections that need to be removed, e.g. while forcing a direction, were invalidated forcing them to be expired. This is not actually needed, as it's typically a one-time operation. The patch replaces a call to conn_force_expire() with a call to conn_clean(). Signed-off-by: Paolo Valerio <pvalerio@redhat.com> --- lib/conntrack.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)