* [PATCH net 0/2] netfilter: conntrack: Fix CT offload timeout on heavily loaded systems @ 2020-07-28 11:57 Roi Dayan 2020-07-28 11:57 ` [PATCH net 1/2] netfilter: conntrack: Move nf_ct_offload_timeout to header file Roi Dayan 2020-07-28 11:57 ` [PATCH net 2/2] net/sched: act_ct: Set offload timeout when setting the offload bit Roi Dayan 0 siblings, 2 replies; 7+ messages in thread From: Roi Dayan @ 2020-07-28 11:57 UTC (permalink / raw) To: netdev; +Cc: pablo, Paul Blakey, Oz Shlomo, Roi Dayan 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. First commit is to expose the function that updates the timeout. Second commit is to use it from act_ct. Roi Dayan (2): netfilter: conntrack: Move nf_ct_offload_timeout to header file net/sched: act_ct: Set offload timeout when setting the offload bit include/net/netfilter/nf_conntrack.h | 12 ++++++++++++ net/netfilter/nf_conntrack_core.c | 12 ------------ net/sched/act_ct.c | 2 ++ 3 files changed, 14 insertions(+), 12 deletions(-) -- 2.8.4 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH net 1/2] netfilter: conntrack: Move nf_ct_offload_timeout to header file 2020-07-28 11:57 [PATCH net 0/2] netfilter: conntrack: Fix CT offload timeout on heavily loaded systems Roi Dayan @ 2020-07-28 11:57 ` Roi Dayan 2020-07-28 11:57 ` [PATCH net 2/2] net/sched: act_ct: Set offload timeout when setting the offload bit Roi Dayan 1 sibling, 0 replies; 7+ messages in thread From: Roi Dayan @ 2020-07-28 11:57 UTC (permalink / raw) To: netdev; +Cc: pablo, Paul Blakey, Oz Shlomo, Roi Dayan To be used by callers from other modules. Signed-off-by: Roi Dayan <roid@mellanox.com> Reviewed-by: Oz Shlomo <ozsh@mellanox.com> --- include/net/netfilter/nf_conntrack.h | 12 ++++++++++++ net/netfilter/nf_conntrack_core.c | 12 ------------ 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/include/net/netfilter/nf_conntrack.h b/include/net/netfilter/nf_conntrack.h index 90690e37a56f..8481819ff632 100644 --- a/include/net/netfilter/nf_conntrack.h +++ b/include/net/netfilter/nf_conntrack.h @@ -279,6 +279,18 @@ static inline bool nf_ct_should_gc(const struct nf_conn *ct) !nf_ct_is_dying(ct); } +#define DAY (86400 * HZ) + +/* Set an arbitrary timeout large enough not to ever expire, this save + * us a check for the IPS_OFFLOAD_BIT from the packet path via + * nf_ct_is_expired(). + */ +static inline void nf_ct_offload_timeout(struct nf_conn *ct) +{ + if (nf_ct_expires(ct) < DAY / 2) + ct->timeout = nfct_time_stamp + DAY; +} + struct kernel_param; int nf_conntrack_set_hashsize(const char *val, const struct kernel_param *kp); diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c index 79cd9dde457b..947c6d9437c3 100644 --- a/net/netfilter/nf_conntrack_core.c +++ b/net/netfilter/nf_conntrack_core.c @@ -1344,18 +1344,6 @@ static bool gc_worker_can_early_drop(const struct nf_conn *ct) return false; } -#define DAY (86400 * HZ) - -/* Set an arbitrary timeout large enough not to ever expire, this save - * us a check for the IPS_OFFLOAD_BIT from the packet path via - * nf_ct_is_expired(). - */ -static void nf_ct_offload_timeout(struct nf_conn *ct) -{ - if (nf_ct_expires(ct) < DAY / 2) - ct->timeout = nfct_time_stamp + DAY; -} - static void gc_worker(struct work_struct *work) { unsigned int min_interval = max(HZ / GC_MAX_BUCKETS_DIV, 1u); -- 2.8.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH net 2/2] net/sched: act_ct: Set offload timeout when setting the offload bit 2020-07-28 11:57 [PATCH net 0/2] netfilter: conntrack: Fix CT offload timeout on heavily loaded systems Roi Dayan 2020-07-28 11:57 ` [PATCH net 1/2] netfilter: conntrack: Move nf_ct_offload_timeout to header file Roi Dayan @ 2020-07-28 11:57 ` Roi Dayan 2020-07-28 14:42 ` Marcelo Ricardo Leitner 1 sibling, 1 reply; 7+ messages in thread From: Roi Dayan @ 2020-07-28 11:57 UTC (permalink / raw) To: netdev; +Cc: pablo, Paul Blakey, Oz Shlomo, Roi Dayan 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, if (err) goto err_add; + nf_ct_offload_timeout(ct); + return; err_add: -- 2.8.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net 2/2] net/sched: act_ct: Set offload timeout when setting the offload bit 2020-07-28 11:57 ` [PATCH net 2/2] net/sched: act_ct: Set offload timeout when setting the offload bit Roi Dayan @ 2020-07-28 14:42 ` Marcelo Ricardo Leitner 2020-07-29 12:55 ` Roi Dayan 0 siblings, 1 reply; 7+ messages in thread From: Marcelo Ricardo Leitner @ 2020-07-28 14:42 UTC (permalink / raw) To: Roi Dayan; +Cc: netdev, pablo, Paul Blakey, Oz Shlomo 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net 2/2] net/sched: act_ct: Set offload timeout when setting the offload bit 2020-07-28 14:42 ` Marcelo Ricardo Leitner @ 2020-07-29 12:55 ` Roi Dayan 2020-07-29 17:10 ` Marcelo Ricardo Leitner 0 siblings, 1 reply; 7+ messages in thread From: Roi Dayan @ 2020-07-29 12:55 UTC (permalink / raw) To: Marcelo Ricardo Leitner; +Cc: netdev, pablo, Paul Blakey, Oz Shlomo 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 >> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net 2/2] net/sched: act_ct: Set offload timeout when setting the offload bit 2020-07-29 12:55 ` Roi Dayan @ 2020-07-29 17:10 ` Marcelo Ricardo Leitner 2020-08-03 7:21 ` Roi Dayan 0 siblings, 1 reply; 7+ messages in thread From: Marcelo Ricardo Leitner @ 2020-07-29 17:10 UTC (permalink / raw) To: Roi Dayan; +Cc: netdev, pablo, Paul Blakey, Oz Shlomo 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 > >> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net 2/2] net/sched: act_ct: Set offload timeout when setting the offload bit 2020-07-29 17:10 ` Marcelo Ricardo Leitner @ 2020-08-03 7:21 ` Roi Dayan 0 siblings, 0 replies; 7+ messages in thread From: Roi Dayan @ 2020-08-03 7:21 UTC (permalink / raw) To: Marcelo Ricardo Leitner; +Cc: netdev, pablo, Paul Blakey, Oz Shlomo 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 >>>> ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-08-03 7:22 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-28 11:57 [PATCH net 0/2] netfilter: conntrack: Fix CT offload timeout on heavily loaded systems Roi Dayan 2020-07-28 11:57 ` [PATCH net 1/2] netfilter: conntrack: Move nf_ct_offload_timeout to header file Roi Dayan 2020-07-28 11:57 ` [PATCH net 2/2] net/sched: act_ct: Set offload timeout when setting the offload bit Roi Dayan 2020-07-28 14:42 ` Marcelo Ricardo Leitner 2020-07-29 12:55 ` Roi Dayan 2020-07-29 17:10 ` Marcelo Ricardo Leitner 2020-08-03 7:21 ` Roi Dayan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).