ACK: [SRU][F:linux-bluefield][PATCH v1 1/2] netfilter: conntrack: remove offload_pickup sysctl again
Zachary Tahenakos
zachary.tahenakos at canonical.com
Tue Jun 21 15:05:29 UTC 2022
Hi Bodong,
Yes it isn't merged to master-next on purpose. This patch would have
landed for 2022.06.20, however the previous cycle is being extended out.
We are prioritizing the urgent patches for the 2022.05.30 cycle to
ensure those can get out the door smoothly and then once the cycles
catch up, we can merge this in, among other patches that need to come in
as well.
-Zack
On 6/21/22 10:57 AM, Bodong Wang wrote:
> 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
More information about the kernel-team
mailing list