| Message ID | 20240905123246.802622-1-aconole@redhat.com |
|---|---|
| State | Superseded |
| Delegated to: | aaron conole |
| Headers | show |
| Series | [ovs-dev] conntrack: Disambiguate the cleaned count log. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On Thu, Sep 05, 2024 at 08:32:46AM -0400, Aaron Conole wrote: > After 3d9c1b855a5f ("conntrack: Replace timeout based expiration lists with rculists.") > the conntrack cleanup log reports the number of connections it checked > rather than the number of connections it cleaned. This patch includes the > count of connections cleaned during expiration sweeping. > > Reported-by: Cheng Li <lic121@chinatelecom.cn> > Suggested-by: Cheng Li <lic121@chinatelecom.cn> > Fixes: 3d9c1b855a5f ("conntrack: Replace timeout based expiration lists with rculists.") > Signed-off-by: Aaron Conole <aconole@redhat.com> Acked-by: Simon Horman <horms@ovn.org>
On 5 Sep 2024, at 14:32, Aaron Conole wrote: > After 3d9c1b855a5f ("conntrack: Replace timeout based expiration lists with rculists.") > the conntrack cleanup log reports the number of connections it checked > rather than the number of connections it cleaned. This patch includes the > count of connections cleaned during expiration sweeping. The patch looks good to me, however one comments on the log message itself. //Eelco > Reported-by: Cheng Li <lic121@chinatelecom.cn> > Suggested-by: Cheng Li <lic121@chinatelecom.cn> > Fixes: 3d9c1b855a5f ("conntrack: Replace timeout based expiration lists with rculists.") > Signed-off-by: Aaron Conole <aconole@redhat.com> > --- > lib/conntrack.c | 18 +++++++++++++++--- > 1 file changed, 15 insertions(+), 3 deletions(-) > > diff --git a/lib/conntrack.c b/lib/conntrack.c > index db44f82374..e90c2ac19f 100644 > --- a/lib/conntrack.c > +++ b/lib/conntrack.c > @@ -1443,20 +1443,27 @@ conntrack_get_sweep_interval(struct conntrack *ct) > } > > static size_t > -ct_sweep(struct conntrack *ct, struct rculist *list, long long now) > +ct_sweep(struct conntrack *ct, struct rculist *list, long long now, > + size_t *cleaned_count) > OVS_NO_THREAD_SAFETY_ANALYSIS > { > struct conn *conn; > + size_t cleaned = 0; > size_t count = 0; > > RCULIST_FOR_EACH (conn, node, list) { > if (conn_expired(conn, now)) { > conn_clean(ct, conn); > + cleaned++; > } > > count++; > } > > + if (cleaned_count) { > + *cleaned_count = cleaned; > + } > + > return count; > } > > @@ -1469,22 +1476,27 @@ conntrack_clean(struct conntrack *ct, long long now) > long long next_wakeup = now + conntrack_get_sweep_interval(ct); > unsigned int n_conn_limit, i; > size_t clean_end, count = 0; > + size_t total_cleaned = 0; > > atomic_read_relaxed(&ct->n_conn_limit, &n_conn_limit); > clean_end = n_conn_limit / 64; > > for (i = ct->next_sweep; i < N_EXP_LISTS; i++) { > + size_t cleaned; > + > if (count > clean_end) { > next_wakeup = 0; > break; > } > > - count += ct_sweep(ct, &ct->exp_lists[i], now); > + count += ct_sweep(ct, &ct->exp_lists[i], now, &cleaned); > + total_cleaned += cleaned; > } > > ct->next_sweep = (i < N_EXP_LISTS) ? i : 0; > > - VLOG_DBG("conntrack cleanup %"PRIuSIZE" entries in %lld msec", count, > + VLOG_DBG("conntrack cleaned %"PRIuSIZE" / %"PRIuSIZE > + " entries in %lld msec", total_cleaned, count, > time_msec() - now); Can we make the log message a bit more clear? Maybe something like: “conntrack cleaned %"PRIuSIZE" entries out of %"PRIuSIZE" in %lld msec” > > return next_wakeup; > -- > 2.45.2 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/lib/conntrack.c b/lib/conntrack.c index db44f82374..e90c2ac19f 100644 --- a/lib/conntrack.c +++ b/lib/conntrack.c @@ -1443,20 +1443,27 @@ conntrack_get_sweep_interval(struct conntrack *ct) } static size_t -ct_sweep(struct conntrack *ct, struct rculist *list, long long now) +ct_sweep(struct conntrack *ct, struct rculist *list, long long now, + size_t *cleaned_count) OVS_NO_THREAD_SAFETY_ANALYSIS { struct conn *conn; + size_t cleaned = 0; size_t count = 0; RCULIST_FOR_EACH (conn, node, list) { if (conn_expired(conn, now)) { conn_clean(ct, conn); + cleaned++; } count++; } + if (cleaned_count) { + *cleaned_count = cleaned; + } + return count; } @@ -1469,22 +1476,27 @@ conntrack_clean(struct conntrack *ct, long long now) long long next_wakeup = now + conntrack_get_sweep_interval(ct); unsigned int n_conn_limit, i; size_t clean_end, count = 0; + size_t total_cleaned = 0; atomic_read_relaxed(&ct->n_conn_limit, &n_conn_limit); clean_end = n_conn_limit / 64; for (i = ct->next_sweep; i < N_EXP_LISTS; i++) { + size_t cleaned; + if (count > clean_end) { next_wakeup = 0; break; } - count += ct_sweep(ct, &ct->exp_lists[i], now); + count += ct_sweep(ct, &ct->exp_lists[i], now, &cleaned); + total_cleaned += cleaned; } ct->next_sweep = (i < N_EXP_LISTS) ? i : 0; - VLOG_DBG("conntrack cleanup %"PRIuSIZE" entries in %lld msec", count, + VLOG_DBG("conntrack cleaned %"PRIuSIZE" / %"PRIuSIZE + " entries in %lld msec", total_cleaned, count, time_msec() - now); return next_wakeup;
After 3d9c1b855a5f ("conntrack: Replace timeout based expiration lists with rculists.") the conntrack cleanup log reports the number of connections it checked rather than the number of connections it cleaned. This patch includes the count of connections cleaned during expiration sweeping. Reported-by: Cheng Li <lic121@chinatelecom.cn> Suggested-by: Cheng Li <lic121@chinatelecom.cn> Fixes: 3d9c1b855a5f ("conntrack: Replace timeout based expiration lists with rculists.") Signed-off-by: Aaron Conole <aconole@redhat.com> --- lib/conntrack.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)