| Message ID | 20200728115759.426667-3-roid@mellanox.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | netfilter: conntrack: Fix CT offload timeout on heavily loaded systems | expand |
On Tue, Jul 28, 2020 at 02:57:59PM +0300, Roi Dayan wrote: > On heavily loaded systems the GC can take time to go over all existing > conns and reset their timeout. At that time other calls like from > nf_conntrack_in() can call of nf_ct_is_expired() and see the conn as > expired. To fix this when we set the offload bit we should also reset > the timeout instead of counting on GC to finish first iteration over > all conns before the initial timeout. > > Fixes: 64ff70b80fd4 ("net/sched: act_ct: Offload established connections to flow table") > Signed-off-by: Roi Dayan <roid@mellanox.com> > Reviewed-by: Oz Shlomo <ozsh@mellanox.com> > --- > net/sched/act_ct.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c > index e9f3576cbf71..650c2d78a346 100644 > --- a/net/sched/act_ct.c > +++ b/net/sched/act_ct.c > @@ -366,6 +366,8 @@ static void tcf_ct_flow_table_add(struct tcf_ct_flow_table *ct_ft, Extra context line: err = flow_offload_add(&ct_ft->nf_ft, entry); > if (err) > goto err_add; > > + nf_ct_offload_timeout(ct); > + What about adding this to flow_offload_add() instead? It is already adjusting the flow_offload timeout there and then it also effective for nft. > return; > > err_add: > -- > 2.8.4 >
On 2020-07-28 5:42 PM, Marcelo Ricardo Leitner wrote: > On Tue, Jul 28, 2020 at 02:57:59PM +0300, Roi Dayan wrote: >> On heavily loaded systems the GC can take time to go over all existing >> conns and reset their timeout. At that time other calls like from >> nf_conntrack_in() can call of nf_ct_is_expired() and see the conn as >> expired. To fix this when we set the offload bit we should also reset >> the timeout instead of counting on GC to finish first iteration over >> all conns before the initial timeout. >> >> Fixes: 64ff70b80fd4 ("net/sched: act_ct: Offload established connections to flow table") >> Signed-off-by: Roi Dayan <roid@mellanox.com> >> Reviewed-by: Oz Shlomo <ozsh@mellanox.com> >> --- >> net/sched/act_ct.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c >> index e9f3576cbf71..650c2d78a346 100644 >> --- a/net/sched/act_ct.c >> +++ b/net/sched/act_ct.c >> @@ -366,6 +366,8 @@ static void tcf_ct_flow_table_add(struct tcf_ct_flow_table *ct_ft, > > Extra context line: > err = flow_offload_add(&ct_ft->nf_ft, entry); >> if (err) >> goto err_add; >> >> + nf_ct_offload_timeout(ct); >> + > > What about adding this to flow_offload_add() instead? > It is already adjusting the flow_offload timeout there and then it > also effective for nft. > As you said, in flow_offload_add() we adjust the flow timeout. Here we adjust the conn timeout. So it's outside flow_offload_add() which only touch the flow struct. I guess it's like conn offload bit is set outside here and for nft. What do you think? >> return; >> >> err_add: >> -- >> 2.8.4 >>
On Wed, Jul 29, 2020 at 03:55:53PM +0300, Roi Dayan wrote: > > > On 2020-07-28 5:42 PM, Marcelo Ricardo Leitner wrote: > > On Tue, Jul 28, 2020 at 02:57:59PM +0300, Roi Dayan wrote: > >> On heavily loaded systems the GC can take time to go over all existing > >> conns and reset their timeout. At that time other calls like from > >> nf_conntrack_in() can call of nf_ct_is_expired() and see the conn as > >> expired. To fix this when we set the offload bit we should also reset > >> the timeout instead of counting on GC to finish first iteration over > >> all conns before the initial timeout. > >> > >> Fixes: 64ff70b80fd4 ("net/sched: act_ct: Offload established connections to flow table") > >> Signed-off-by: Roi Dayan <roid@mellanox.com> > >> Reviewed-by: Oz Shlomo <ozsh@mellanox.com> > >> --- > >> net/sched/act_ct.c | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c > >> index e9f3576cbf71..650c2d78a346 100644 > >> --- a/net/sched/act_ct.c > >> +++ b/net/sched/act_ct.c > >> @@ -366,6 +366,8 @@ static void tcf_ct_flow_table_add(struct tcf_ct_flow_table *ct_ft, > > > > Extra context line: > > err = flow_offload_add(&ct_ft->nf_ft, entry); > >> if (err) > >> goto err_add; > >> > >> + nf_ct_offload_timeout(ct); > >> + > > > > What about adding this to flow_offload_add() instead? > > It is already adjusting the flow_offload timeout there and then it > > also effective for nft. > > > > As you said, in flow_offload_add() we adjust the flow timeout. > Here we adjust the conn timeout. > So it's outside flow_offload_add() which only touch the flow struct. > I guess it's like conn offload bit is set outside here and for nft. Right, but > What do you think? I don't see why it can't update both. flow_offload_fixup_ct_timeout(), called by flow_offload_del(), is updating ct->timeout already. It looks consistent to me to update it in _add as well then. > > >> return; > >> > >> err_add: > >> -- > >> 2.8.4 > >>
On 2020-07-29 8:10 PM, Marcelo Ricardo Leitner wrote: > On Wed, Jul 29, 2020 at 03:55:53PM +0300, Roi Dayan wrote: >> >> >> On 2020-07-28 5:42 PM, Marcelo Ricardo Leitner wrote: >>> On Tue, Jul 28, 2020 at 02:57:59PM +0300, Roi Dayan wrote: >>>> On heavily loaded systems the GC can take time to go over all existing >>>> conns and reset their timeout. At that time other calls like from >>>> nf_conntrack_in() can call of nf_ct_is_expired() and see the conn as >>>> expired. To fix this when we set the offload bit we should also reset >>>> the timeout instead of counting on GC to finish first iteration over >>>> all conns before the initial timeout. >>>> >>>> Fixes: 64ff70b80fd4 ("net/sched: act_ct: Offload established connections to flow table") >>>> Signed-off-by: Roi Dayan <roid@mellanox.com> >>>> Reviewed-by: Oz Shlomo <ozsh@mellanox.com> >>>> --- >>>> net/sched/act_ct.c | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c >>>> index e9f3576cbf71..650c2d78a346 100644 >>>> --- a/net/sched/act_ct.c >>>> +++ b/net/sched/act_ct.c >>>> @@ -366,6 +366,8 @@ static void tcf_ct_flow_table_add(struct tcf_ct_flow_table *ct_ft, >>> >>> Extra context line: >>> err = flow_offload_add(&ct_ft->nf_ft, entry); >>>> if (err) >>>> goto err_add; >>>> >>>> + nf_ct_offload_timeout(ct); >>>> + >>> >>> What about adding this to flow_offload_add() instead? >>> It is already adjusting the flow_offload timeout there and then it >>> also effective for nft. >>> >> >> As you said, in flow_offload_add() we adjust the flow timeout. >> Here we adjust the conn timeout. >> So it's outside flow_offload_add() which only touch the flow struct. >> I guess it's like conn offload bit is set outside here and for nft. > > Right, but > >> What do you think? > > I don't see why it can't update both. flow_offload_fixup_ct_timeout(), > called by flow_offload_del(), is updating ct->timeout already. It > looks consistent to me to update it in _add as well then. > I don't mind. just add is not consistent with del. del also clears the ips_offload_bit but add doesn't add it. i'll send v2 with your suggestion. >> >>>> return; >>>> >>>> err_add: >>>> -- >>>> 2.8.4 >>>>
diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c index e9f3576cbf71..650c2d78a346 100644 --- a/net/sched/act_ct.c +++ b/net/sched/act_ct.c @@ -366,6 +366,8 @@ static void tcf_ct_flow_table_add(struct tcf_ct_flow_table *ct_ft, if (err) goto err_add; + nf_ct_offload_timeout(ct); + return; err_add:
On heavily loaded systems the GC can take time to go over all existing conns and reset their timeout. At that time other calls like from nf_conntrack_in() can call of nf_ct_is_expired() and see the conn as expired. To fix this when we set the offload bit we should also reset the timeout instead of counting on GC to finish first iteration over all conns before the initial timeout. Fixes: 64ff70b80fd4 ("net/sched: act_ct: Offload established connections to flow table") Signed-off-by: Roi Dayan <roid@mellanox.com> Reviewed-by: Oz Shlomo <ozsh@mellanox.com> --- net/sched/act_ct.c | 2 ++ 1 file changed, 2 insertions(+)