| Message ID | 20230217055517.1828-1-liangmc1@chinatelecom.cn |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] conntrack:fix conntrack_clean may access the same exp_list each time be called | 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 |
Hello Liang, Liang Mancang <liangmc1@chinatelecom.cn> writes: > when a exp_list contains more than the clean_end's number of nodes, > and these nodes will not expire immediately. Then, every times we > call conntrack_clean, it use the same next_sweep to get exp_list. > Yes, in general, if the previous count exceeds the clean_end, it should not make the sweeper restart from a list just swept, but it should not happen that a single list contains more than n_conn_limit / 64. Did you observe a single exp_list containing more than n_conn_limit / 64 entries? > Actually, we should add i every times after we call ct_sweep. > > Signed-off-by: Liang Mancang <liangmc1@chinatelecom.cn> > --- > lib/conntrack.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/lib/conntrack.c b/lib/conntrack.c > index 524670e45..5029b2cda 100644 > --- a/lib/conntrack.c > +++ b/lib/conntrack.c > @@ -1512,12 +1512,13 @@ conntrack_clean(struct conntrack *ct, long long now) > clean_end = n_conn_limit / 64; > > for (i = ct->next_sweep; i < N_EXP_LISTS; i++) { > - count += ct_sweep(ct, &ct->exp_lists[i], now); > - > if (count > clean_end) { > next_wakeup = 0; > + This new line is not needed, and a Fixes tag could be added: Fixes: 3d9c1b855a5f ("conntrack: Replace timeout based expiration lists with rculists.") The patch LGTM, > break; > } > + > + count += ct_sweep(ct, &ct->exp_lists[i], now); > } > > ct->next_sweep = (i < N_EXP_LISTS) ? i : 0; > -- > 2.30.0.windows.2 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Paolo Valerio <pvalerio@redhat.com> writes: > Hello Liang, > > Liang Mancang <liangmc1@chinatelecom.cn> writes: > >> when a exp_list contains more than the clean_end's number of nodes, >> and these nodes will not expire immediately. Then, every times we >> call conntrack_clean, it use the same next_sweep to get exp_list. >> > > Yes, in general, if the previous count exceeds the clean_end, it should > not make the sweeper restart from a list just swept, but it should not > happen that a single list contains more than n_conn_limit / 64. > > Did you observe a single exp_list containing more than n_conn_limit / 64 > entries? > >> Actually, we should add i every times after we call ct_sweep. >> >> Signed-off-by: Liang Mancang <liangmc1@chinatelecom.cn> >> --- >> lib/conntrack.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/lib/conntrack.c b/lib/conntrack.c >> index 524670e45..5029b2cda 100644 >> --- a/lib/conntrack.c >> +++ b/lib/conntrack.c >> @@ -1512,12 +1512,13 @@ conntrack_clean(struct conntrack *ct, long long now) >> clean_end = n_conn_limit / 64; >> >> for (i = ct->next_sweep; i < N_EXP_LISTS; i++) { >> - count += ct_sweep(ct, &ct->exp_lists[i], now); >> - >> if (count > clean_end) { >> next_wakeup = 0; >> + > > This new line is not needed, and a Fixes tag could be added: > > Fixes: 3d9c1b855a5f ("conntrack: Replace timeout based expiration lists with rculists.") > > The patch LGTM, > Sorry, the last line slipped out. Please consider my question and the other comments. I will explicitly tag the patch once we're done. >> break; >> } >> + >> + count += ct_sweep(ct, &ct->exp_lists[i], now); >> } >> >> ct->next_sweep = (i < N_EXP_LISTS) ? i : 0; >> -- >> 2.30.0.windows.2 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Mon, Feb 20, 2023 at 07:38:39PM +0100, Paolo Valerio wrote: > Paolo Valerio <pvalerio@redhat.com> writes: > > > Hello Liang, > > > > Liang Mancang <liangmc1@chinatelecom.cn> writes: > > > >> when a exp_list contains more than the clean_end's number of nodes, > >> and these nodes will not expire immediately. Then, every times we > >> call conntrack_clean, it use the same next_sweep to get exp_list. > >> > > > > Yes, in general, if the previous count exceeds the clean_end, it should > > not make the sweeper restart from a list just swept, but it should not > > happen that a single list contains more than n_conn_limit / 64. > > > > Did you observe a single exp_list containing more than n_conn_limit / 64 > > entries? We only select exp_list for a conntrack entry when createing it, but never move them when update their expires or delete them. So the number of each exp_list will become unbalanced after long-time running. > > > >> Actually, we should add i every times after we call ct_sweep. > >> > >> Signed-off-by: Liang Mancang <liangmc1@chinatelecom.cn> > >> --- > >> lib/conntrack.c | 5 +++-- > >> 1 file changed, 3 insertions(+), 2 deletions(-) > >> > >> diff --git a/lib/conntrack.c b/lib/conntrack.c > >> index 524670e45..5029b2cda 100644 > >> --- a/lib/conntrack.c > >> +++ b/lib/conntrack.c > >> @@ -1512,12 +1512,13 @@ conntrack_clean(struct conntrack *ct, long long now) > >> clean_end = n_conn_limit / 64; > >> > >> for (i = ct->next_sweep; i < N_EXP_LISTS; i++) { > >> - count += ct_sweep(ct, &ct->exp_lists[i], now); > >> - > >> if (count > clean_end) { > >> next_wakeup = 0; > >> + > > > > This new line is not needed, and a Fixes tag could be added: > > > > Fixes: 3d9c1b855a5f ("conntrack: Replace timeout based expiration lists with rculists.") > > > > The patch LGTM, > > > > Sorry, the last line slipped out. Please consider my question and the > other comments. I will explicitly tag the patch once we're done. > I sent v2 for this. > >> break; > >> } > >> + > >> + count += ct_sweep(ct, &ct->exp_lists[i], now); > >> } > >> > >> ct->next_sweep = (i < N_EXP_LISTS) ? i : 0; > >> -- > >> 2.30.0.windows.2 > >> > >> _______________________________________________ > >> dev mailing list > >> dev@openvswitch.org
Liang Mancang <liangmc1@chinatelecom.cn> writes: > On Mon, Feb 20, 2023 at 07:38:39PM +0100, Paolo Valerio wrote: >> Paolo Valerio <pvalerio@redhat.com> writes: >> >> > Hello Liang, >> > >> > Liang Mancang <liangmc1@chinatelecom.cn> writes: >> > >> >> when a exp_list contains more than the clean_end's number of nodes, >> >> and these nodes will not expire immediately. Then, every times we >> >> call conntrack_clean, it use the same next_sweep to get exp_list. >> >> >> > >> > Yes, in general, if the previous count exceeds the clean_end, it should >> > not make the sweeper restart from a list just swept, but it should not >> > happen that a single list contains more than n_conn_limit / 64. >> > >> > Did you observe a single exp_list containing more than n_conn_limit / 64 >> > entries? > We only select exp_list for a conntrack entry when createing it, but never move > them when update their expires or delete them. So the number of each exp_list > will become unbalanced after long-time running. of course, if not balanced that could happen. >> > >> >> Actually, we should add i every times after we call ct_sweep. >> >> >> >> Signed-off-by: Liang Mancang <liangmc1@chinatelecom.cn> >> >> --- >> >> lib/conntrack.c | 5 +++-- >> >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> >> >> diff --git a/lib/conntrack.c b/lib/conntrack.c >> >> index 524670e45..5029b2cda 100644 >> >> --- a/lib/conntrack.c >> >> +++ b/lib/conntrack.c >> >> @@ -1512,12 +1512,13 @@ conntrack_clean(struct conntrack *ct, long long now) >> >> clean_end = n_conn_limit / 64; >> >> >> >> for (i = ct->next_sweep; i < N_EXP_LISTS; i++) { >> >> - count += ct_sweep(ct, &ct->exp_lists[i], now); >> >> - >> >> if (count > clean_end) { >> >> next_wakeup = 0; >> >> + >> > >> > This new line is not needed, and a Fixes tag could be added: >> > >> > Fixes: 3d9c1b855a5f ("conntrack: Replace timeout based expiration lists with rculists.") >> > >> > The patch LGTM, >> > >> >> Sorry, the last line slipped out. Please consider my question and the >> other comments. I will explicitly tag the patch once we're done. >> > I sent v2 for this. Thanks. >> >> break; >> >> } >> >> + >> >> + count += ct_sweep(ct, &ct->exp_lists[i], now); >> >> } >> >> >> >> ct->next_sweep = (i < N_EXP_LISTS) ? i : 0; >> >> -- >> >> 2.30.0.windows.2 >> >> >> >> _______________________________________________ >> >> dev mailing list >> >> dev@openvswitch.org
diff --git a/lib/conntrack.c b/lib/conntrack.c index 524670e45..5029b2cda 100644 --- a/lib/conntrack.c +++ b/lib/conntrack.c @@ -1512,12 +1512,13 @@ conntrack_clean(struct conntrack *ct, long long now) clean_end = n_conn_limit / 64; for (i = ct->next_sweep; i < N_EXP_LISTS; i++) { - count += ct_sweep(ct, &ct->exp_lists[i], now); - if (count > clean_end) { next_wakeup = 0; + break; } + + count += ct_sweep(ct, &ct->exp_lists[i], now); } ct->next_sweep = (i < N_EXP_LISTS) ? i : 0;
when a exp_list contains more than the clean_end's number of nodes, and these nodes will not expire immediately. Then, every times we call conntrack_clean, it use the same next_sweep to get exp_list. Actually, we should add i every times after we call ct_sweep. Signed-off-by: Liang Mancang <liangmc1@chinatelecom.cn> --- lib/conntrack.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)