ACK: [SRU][F:linux-bluefield][PATCH v1 1/2] netfilter: conntrack: remove offload_pickup sysctl again

Bodong Wang bodong at nvidia.com
Tue Jun 21 14:57:01 UTC 2022


Hi Tim/Zach,

Seems like this patch series is ACKed but not merged to master-next. Could you please check?

Thanks,
Bodong

-----Original Message-----
From: Tim Gardner <tim.gardner at canonical.com> 
Sent: Thursday, June 16, 2022 10:10 AM
To: Bodong Wang <bodong at nvidia.com>; kernel-team at lists.ubuntu.com
Cc: Vladimir Sokolovsky <vlad at nvidia.com>; Maor Dickman <maord at nvidia.com>; Oz Shlomo <ozsh at nvidia.com>
Subject: ACK: [SRU][F:linux-bluefield][PATCH v1 1/2] netfilter: conntrack: remove offload_pickup sysctl again

On 5/26/22 07:03, Bodong Wang wrote:
> From: Florian Westphal <fw at strlen.de>
> 
> BugLink: https://bugs.launchpad.net/bugs/1975820
> 
> These two sysctls were added because the hardcoded defaults (2 
> minutes, tcp, 30 seconds, udp) turned out to be too low for some setups.
> 
> They appeared in 5.14-rc1 so it should be fine to remove it again.
> 
> Marcelo convinced me that there should be no difference between a flow 
> that was offloaded vs. a flow that was not wrt. timeout handling.
> Thus the default is changed to those for TCP established and UDP 
> stream,
> 5 days and 120 seconds, respectively.
> 
> Marcelo also suggested to account for the timeout value used for the 
> offloading, this avoids increase beyond the value in the 
> conntrack-sysctl and will also instantly expire the conntrack entry with altered sysctls.
> 
> Example:
>     nf_conntrack_udp_timeout_stream=60
>     nf_flowtable_udp_timeout=60
> 
> This will remove offloaded udp flows after one minute, rather than two.
> 
> An earlier version of this patch also cleared the ASSURED bit to allow 
> nf_conntrack to evict the entry via early_drop (i.e., table full).
> However, it looks like we can safely assume that connection timed out 
> via HW is still in established state, so this isn't needed.
> 
> Quoting Oz:
>   [..] the hardware sends all packets with a set FIN flags to sw.
>   [..] Connections that are aged in hardware are expected to be in the
>   established state.
> 
> In case it turns out that back-to-sw-path transition can occur for 
> 'dodgy' connections too (e.g., one side disappeared while 
> software-path would have been in RETRANS timeout), we can adjust this later.
> 
> Cc: Oz Shlomo <ozsh at nvidia.com>
> Cc: Paul Blakey <paulb at nvidia.com>
> Suggested-by: Marcelo Ricardo Leitner <marcelo.leitner at gmail.com>
> Signed-off-by: Florian Westphal <fw at strlen.de>
> Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner at gmail.com>
> Reviewed-by: Oz Shlomo <ozsh at nvidia.com>
> Signed-off-by: Pablo Neira Ayuso <pablo at netfilter.org> (backported 
> from commit 4592ee7f525c4683ec9e290381601fdee50ae110)
> Signed-off-by: Bodong Wang <bodong at nvidia.com>
> [bodong: remove doc file]
> 
> Conflicts:
> 	Documentation/networking/nf_conntrack-sysctl.rst
> ---
>   include/net/netns/conntrack.h           |  2 --
>   net/netfilter/nf_conntrack_proto_tcp.c  |  1 -
>   net/netfilter/nf_conntrack_proto_udp.c  |  1 -
>   net/netfilter/nf_conntrack_standalone.c | 16 ----------------
>   net/netfilter/nf_flow_table_core.c      | 11 ++++++++---
>   5 files changed, 8 insertions(+), 23 deletions(-)
> 
> diff --git a/include/net/netns/conntrack.h 
> b/include/net/netns/conntrack.h index 67bbaa6..9e3963c8 100644
> --- a/include/net/netns/conntrack.h
> +++ b/include/net/netns/conntrack.h
> @@ -29,7 +29,6 @@ struct nf_tcp_net {
>   	int tcp_max_retrans;
>   #if IS_ENABLED(CONFIG_NF_FLOW_TABLE)
>   	unsigned int offload_timeout;
> -	unsigned int offload_pickup;
>   #endif
>   };
>   
> @@ -43,7 +42,6 @@ struct nf_udp_net {
>   	unsigned int timeouts[UDP_CT_MAX];
>   #if IS_ENABLED(CONFIG_NF_FLOW_TABLE)
>   	unsigned int offload_timeout;
> -	unsigned int offload_pickup;
>   #endif
>   };
>   
> diff --git a/net/netfilter/nf_conntrack_proto_tcp.c 
> b/net/netfilter/nf_conntrack_proto_tcp.c
> index d8e554c..0e51fb7 100644
> --- a/net/netfilter/nf_conntrack_proto_tcp.c
> +++ b/net/netfilter/nf_conntrack_proto_tcp.c
> @@ -1435,7 +1435,6 @@ void nf_conntrack_tcp_init_net(struct net *net)
>   
>   #if IS_ENABLED(CONFIG_NF_FLOW_TABLE)
>   	tn->offload_timeout = 30 * HZ;
> -	tn->offload_pickup = 120 * HZ;
>   #endif
>   }
>   
> diff --git a/net/netfilter/nf_conntrack_proto_udp.c 
> b/net/netfilter/nf_conntrack_proto_udp.c
> index b3e135e..ee336d7 100644
> --- a/net/netfilter/nf_conntrack_proto_udp.c
> +++ b/net/netfilter/nf_conntrack_proto_udp.c
> @@ -282,7 +282,6 @@ void nf_conntrack_udp_init_net(struct net *net)
>   
>   #if IS_ENABLED(CONFIG_NF_FLOW_TABLE)
>   	un->offload_timeout = 30 * HZ;
> -	un->offload_pickup = 30 * HZ;
>   #endif
>   }
>   
> diff --git a/net/netfilter/nf_conntrack_standalone.c 
> b/net/netfilter/nf_conntrack_standalone.c
> index ac8f12b..1e78ad8 100644
> --- a/net/netfilter/nf_conntrack_standalone.c
> +++ b/net/netfilter/nf_conntrack_standalone.c
> @@ -569,7 +569,6 @@ enum nf_ct_sysctl_index {
>   	NF_SYSCTL_CT_PROTO_TIMEOUT_TCP_UNACK,
>   #if IS_ENABLED(CONFIG_NF_FLOW_TABLE)
>   	NF_SYSCTL_CT_PROTO_TIMEOUT_TCP_OFFLOAD,
> -	NF_SYSCTL_CT_PROTO_TIMEOUT_TCP_OFFLOAD_PICKUP,
>   #endif
>   	NF_SYSCTL_CT_PROTO_TCP_LOOSE,
>   	NF_SYSCTL_CT_PROTO_TCP_LIBERAL,
> @@ -578,7 +577,6 @@ enum nf_ct_sysctl_index {
>   	NF_SYSCTL_CT_PROTO_TIMEOUT_UDP_STREAM,
>   #if IS_ENABLED(CONFIG_NF_FLOW_TABLE)
>   	NF_SYSCTL_CT_PROTO_TIMEOUT_UDP_OFFLOAD,
> -	NF_SYSCTL_CT_PROTO_TIMEOUT_UDP_OFFLOAD_PICKUP,
>   #endif
>   	NF_SYSCTL_CT_PROTO_TIMEOUT_ICMP,
>   	NF_SYSCTL_CT_PROTO_TIMEOUT_ICMPV6,
> @@ -773,12 +771,6 @@ enum nf_ct_sysctl_index {
>   		.mode		= 0644,
>   		.proc_handler	= proc_dointvec_jiffies,
>   	},
> -	[NF_SYSCTL_CT_PROTO_TIMEOUT_TCP_OFFLOAD_PICKUP] = {
> -		.procname	= "nf_flowtable_tcp_pickup",
> -		.maxlen		= sizeof(unsigned int),
> -		.mode		= 0644,
> -		.proc_handler	= proc_dointvec_jiffies,
> -	},
>   #endif
>   	[NF_SYSCTL_CT_PROTO_TCP_LOOSE] = {
>   		.procname	= "nf_conntrack_tcp_loose",
> @@ -821,12 +813,6 @@ enum nf_ct_sysctl_index {
>   		.mode		= 0644,
>   		.proc_handler	= proc_dointvec_jiffies,
>   	},
> -	[NF_SYSCTL_CT_PROTO_TIMEOUT_UDP_OFFLOAD_PICKUP] = {
> -		.procname	= "nf_flowtable_udp_pickup",
> -		.maxlen		= sizeof(unsigned int),
> -		.mode		= 0644,
> -		.proc_handler	= proc_dointvec_jiffies,
> -	},
>   #endif
>   	[NF_SYSCTL_CT_PROTO_TIMEOUT_ICMP] = {
>   		.procname	= "nf_conntrack_icmp_timeout",
> @@ -1006,7 +992,6 @@ static void 
> nf_conntrack_standalone_init_tcp_sysctl(struct net *net,
>   
>   #if IS_ENABLED(CONFIG_NF_FLOW_TABLE)
>   	table[NF_SYSCTL_CT_PROTO_TIMEOUT_TCP_OFFLOAD].data = &tn->offload_timeout;
> -	table[NF_SYSCTL_CT_PROTO_TIMEOUT_TCP_OFFLOAD_PICKUP].data = &tn->offload_pickup;
>   #endif
>   
>   }
> @@ -1098,7 +1083,6 @@ static int nf_conntrack_standalone_init_sysctl(struct net *net)
>   	table[NF_SYSCTL_CT_PROTO_TIMEOUT_UDP_STREAM].data = &un->timeouts[UDP_CT_REPLIED];
>   #if IS_ENABLED(CONFIG_NF_FLOW_TABLE)
>   	table[NF_SYSCTL_CT_PROTO_TIMEOUT_UDP_OFFLOAD].data = &un->offload_timeout;
> -	table[NF_SYSCTL_CT_PROTO_TIMEOUT_UDP_OFFLOAD_PICKUP].data = &un->offload_pickup;
>   #endif
>   
>   	nf_conntrack_standalone_init_tcp_sysctl(net, table); diff --git 
> a/net/netfilter/nf_flow_table_core.c 
> b/net/netfilter/nf_flow_table_core.c
> index 8ed2752..ba86bc0 100644
> --- a/net/netfilter/nf_flow_table_core.c
> +++ b/net/netfilter/nf_flow_table_core.c
> @@ -136,7 +136,7 @@ static void flow_offload_fixup_ct_timeout(struct nf_conn *ct)
>   	const struct nf_conntrack_l4proto *l4proto;
>   	struct net *net = nf_ct_net(ct);
>   	int l4num = nf_ct_protonum(ct);
> -	unsigned int timeout;
> +	s32 timeout;
>   
>   	l4proto = nf_ct_l4proto_find(l4num);
>   	if (!l4proto)
> @@ -145,15 +145,20 @@ static void flow_offload_fixup_ct_timeout(struct nf_conn *ct)
>   	if (l4num == IPPROTO_TCP) {
>   		struct nf_tcp_net *tn = nf_tcp_pernet(net);
>   
> -		timeout = tn->offload_pickup;
> +		timeout = tn->timeouts[TCP_CONNTRACK_ESTABLISHED];
> +		timeout -= tn->offload_timeout;
>   	} else if (l4num == IPPROTO_UDP) {
>   		struct nf_udp_net *tn = nf_udp_pernet(net);
>   
> -		timeout = tn->offload_pickup;
> +		timeout = tn->timeouts[UDP_CT_REPLIED];
> +		timeout -= tn->offload_timeout;
>   	} else {
>   		return;
>   	}
>   
> +	if (timeout < 0)
> +		timeout = 0;
> +
>   	if (nf_flow_timeout_delta(ct->timeout) > (__s32)timeout)
>   		ct->timeout = nfct_time_stamp + timeout;
>   }
Acked-by: Tim Gardner <tim.gardner at canonical.com>

--
-----------
Tim Gardner
Canonical, Inc
-------------- next part --------------
An embedded message was scrubbed...
From: Bodong Wang <bodong at nvidia.com>
Subject: [SRU][F:linux-bluefield][PATCH v1 2/2] netfilter: flowtable: fix TCP flow teardown
Date: Thu, 26 May 2022 13:03:04 +0000
Size: 12935
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20220621/f3ba1d2d/attachment-0001.mht>


More information about the kernel-team mailing list