From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1A1C7ECAAA1 for ; Tue, 6 Sep 2022 23:25:16 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229704AbiIFXZO (ORCPT ); Tue, 6 Sep 2022 19:25:14 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:58532 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229509AbiIFXZL (ORCPT ); Tue, 6 Sep 2022 19:25:11 -0400 Received: from smtp-fw-80006.amazon.com (smtp-fw-80006.amazon.com [99.78.197.217]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 87F0573904 for ; Tue, 6 Sep 2022 16:25:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=amazon.com; i=@amazon.com; q=dns/txt; s=amazon201209; t=1662506711; x=1694042711; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=DvlUd1QRKfClwvY0YVzalUw54m7T3knqsKG1APKZTvI=; b=m2fzCV/K3+SkMHEhhgbqeC/43k3x/J1IiOCS9hfHhCmfbJCnbs+4YjTF pBi//wdG8JmYyj+Z79B4rHr0yjPK++eu4Sos02gI7VhsWK/Jv7nqBkgGA YC8SaLfn2mj2pBEIiVf5GqnYyvJ5Aphbj0/4JxcVnb8oSrGlpb3ZyIIc4 A=; X-IronPort-AV: E=Sophos;i="5.93,294,1654560000"; d="scan'208";a="127340388" Received: from pdx4-co-svc-p1-lb2-vlan2.amazon.com (HELO email-inbound-relay-pdx-2c-72dc3927.us-west-2.amazon.com) ([10.25.36.210]) by smtp-border-fw-80006.pdx80.corp.amazon.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 06 Sep 2022 23:24:57 +0000 Received: from EX13MTAUWB001.ant.amazon.com (pdx1-ws-svc-p6-lb9-vlan2.pdx.amazon.com [10.236.137.194]) by email-inbound-relay-pdx-2c-72dc3927.us-west-2.amazon.com (Postfix) with ESMTPS id 19C8A45001; Tue, 6 Sep 2022 23:24:56 +0000 (UTC) Received: from EX19D004ANA001.ant.amazon.com (10.37.240.138) by EX13MTAUWB001.ant.amazon.com (10.43.161.249) with Microsoft SMTP Server (TLS) id 15.0.1497.38; Tue, 6 Sep 2022 23:24:55 +0000 Received: from 88665a182662.ant.amazon.com.com (10.43.162.89) by EX19D004ANA001.ant.amazon.com (10.37.240.138) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA) id 15.2.1118.12; Tue, 6 Sep 2022 23:24:52 +0000 From: Kuniyuki Iwashima To: CC: , , , , , , Subject: Re: [PATCH v4 net-next 2/6] tcp: Don't allocate tcp_death_row outside of struct netns_ipv4. Date: Tue, 6 Sep 2022 16:24:44 -0700 Message-ID: <20220906232444.68846-1-kuniyu@amazon.com> X-Mailer: git-send-email 2.30.2 In-Reply-To: <20220906231150.68045-1-kuniyu@amazon.com> References: <20220906231150.68045-1-kuniyu@amazon.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain X-Originating-IP: [10.43.162.89] X-ClientProxiedBy: EX13D15UWB004.ant.amazon.com (10.43.161.61) To EX19D004ANA001.ant.amazon.com (10.37.240.138) Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org From: Kuniyuki Iwashima Date: Tue, 6 Sep 2022 16:11:50 -0700 > From: Eric Dumazet > Date: Tue, 6 Sep 2022 14:26:30 -0700 > > On 9/6/22 09:24, Kuniyuki Iwashima wrote: > > > We will soon introduce an optional per-netns ehash and access hash > > > tables via net->ipv4.tcp_death_row->hashinfo instead of &tcp_hashinfo > > > in most places. > > > > > > It could harm the fast path because dereferences of two fields in net > > > and tcp_death_row might incur two extra cache line misses. To save one > > > dereference, let's place tcp_death_row back in netns_ipv4 and fetch > > > hashinfo via net->ipv4.tcp_death_row"."hashinfo. > > > > > > Note tcp_death_row was initially placed in netns_ipv4, and commit > > > fbb8295248e1 ("tcp: allocate tcp_death_row outside of struct netns_ipv4") > > > changed it to a pointer so that we can fire TIME_WAIT timers after freeing > > > net. However, we don't do so after commit 04c494e68a13 ("Revert "tcp/dccp: > > > get rid of inet_twsk_purge()""), so we need not define tcp_death_row as a > > > pointer. > > > > > > Signed-off-by: Kuniyuki Iwashima > > > --- > > > include/net/netns/ipv4.h | 3 ++- > > > net/dccp/minisocks.c | 2 +- > > > net/ipv4/inet_timewait_sock.c | 4 +--- > > > net/ipv4/proc.c | 2 +- > > > net/ipv4/sysctl_net_ipv4.c | 8 ++------ > > > net/ipv4/tcp_ipv4.c | 14 +++----------- > > > net/ipv4/tcp_minisocks.c | 2 +- > > > net/ipv6/tcp_ipv6.c | 2 +- > > > 8 files changed, 12 insertions(+), 25 deletions(-) > > > > > > diff --git a/include/net/netns/ipv4.h b/include/net/netns/ipv4.h > > > index 6320a76cefdc..2c7df93e3403 100644 > > > --- a/include/net/netns/ipv4.h > > > +++ b/include/net/netns/ipv4.h > > > @@ -34,6 +34,7 @@ struct inet_hashinfo; > > > struct inet_timewait_death_row { > > > refcount_t tw_refcount; > > > > > > + /* Padding to avoid false sharing, tw_refcount can be often written */ > > > struct inet_hashinfo *hashinfo ____cacheline_aligned_in_smp; > > > int sysctl_max_tw_buckets; > > > }; > > > @@ -41,7 +42,7 @@ struct inet_timewait_death_row { > > > struct tcp_fastopen_context; > > > > > > struct netns_ipv4 { > > > - struct inet_timewait_death_row *tcp_death_row; > > > + struct inet_timewait_death_row tcp_death_row; > > > > > > #ifdef CONFIG_SYSCTL > > > struct ctl_table_header *forw_hdr; > > > diff --git a/net/dccp/minisocks.c b/net/dccp/minisocks.c > > > index 64d805b27add..39f408d44da5 100644 > > > --- a/net/dccp/minisocks.c > > > +++ b/net/dccp/minisocks.c > > > @@ -22,7 +22,7 @@ > > > #include "feat.h" > > > > > > struct inet_timewait_death_row dccp_death_row = { > > > - .tw_refcount = REFCOUNT_INIT(1), > > > + .tw_refcount = REFCOUNT_INIT(0), > > > > > > I do not see how this can possibly work. > > > > If the initial (TCP/DCCP) tw_refcount value is 0, then the first attempt > > > > doing a refcount_inc() will trigger a warning/crash. > > > > Have you looked at dmesg/syslog when testing your patch ? > > > > It should contain a loud warning... > > Ah.. right... I'll keep the base as 1. > > > > > > > > .sysctl_max_tw_buckets = NR_FILE * 2, > > > .hashinfo = &dccp_hashinfo, > > > }; > > > diff --git a/net/ipv4/inet_timewait_sock.c b/net/ipv4/inet_timewait_sock.c > > > index 47ccc343c9fb..71d3bb0abf6c 100644 > > > --- a/net/ipv4/inet_timewait_sock.c > > > +++ b/net/ipv4/inet_timewait_sock.c > > > @@ -59,9 +59,7 @@ static void inet_twsk_kill(struct inet_timewait_sock *tw) > > > inet_twsk_bind_unhash(tw, hashinfo); > > > spin_unlock(&bhead->lock); > > > > > > - if (refcount_dec_and_test(&tw->tw_dr->tw_refcount)) > > > - kfree(tw->tw_dr); > > > - > > > + refcount_dec(&tw->tw_dr->tw_refcount); > > > inet_twsk_put(tw); > > > } > > > > > > diff --git a/net/ipv4/proc.c b/net/ipv4/proc.c > > > index 0088a4c64d77..37508be97393 100644 > > > --- a/net/ipv4/proc.c > > > +++ b/net/ipv4/proc.c > > > @@ -59,7 +59,7 @@ static int sockstat_seq_show(struct seq_file *seq, void *v) > > > socket_seq_show(seq); > > > seq_printf(seq, "TCP: inuse %d orphan %d tw %d alloc %d mem %ld\n", > > > sock_prot_inuse_get(net, &tcp_prot), orphans, > > > - refcount_read(&net->ipv4.tcp_death_row->tw_refcount) - 1, > > > > > > I think your patch changes too many things. > > > > Please leave the refcount base value at 1, not 0 :/ > > Will do. > > > > > + refcount_read(&net->ipv4.tcp_death_row.tw_refcount), > > > sockets, proto_memory_allocated(&tcp_prot)); > > > seq_printf(seq, "UDP: inuse %d mem %ld\n", > > > sock_prot_inuse_get(net, &udp_prot), > > > diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c > > > index 5490c285668b..4d7c110c772f 100644 > > > --- a/net/ipv4/sysctl_net_ipv4.c > > > +++ b/net/ipv4/sysctl_net_ipv4.c > > > @@ -530,10 +530,9 @@ static struct ctl_table ipv4_table[] = { > > > }; > > > > > > static struct ctl_table ipv4_net_table[] = { > > > - /* tcp_max_tw_buckets must be first in this table. */ > > > { > > > .procname = "tcp_max_tw_buckets", > > > -/* .data = &init_net.ipv4.tcp_death_row.sysctl_max_tw_buckets, */ > > > + .data = &init_net.ipv4.tcp_death_row.sysctl_max_tw_buckets, > > > .maxlen = sizeof(int), > > > .mode = 0644, > > > .proc_handler = proc_dointvec > > > @@ -1361,8 +1360,7 @@ static __net_init int ipv4_sysctl_init_net(struct net *net) > > > if (!table) > > > goto err_alloc; > > > > > > - /* skip first entry (sysctl_max_tw_buckets) */ > > > - for (i = 1; i < ARRAY_SIZE(ipv4_net_table) - 1; i++) { > > > + for (i = 0; i < ARRAY_SIZE(ipv4_net_table) - 1; i++) { > > > if (table[i].data) { > > > /* Update the variables to point into > > > * the current struct net > > > @@ -1377,8 +1375,6 @@ static __net_init int ipv4_sysctl_init_net(struct net *net) > > > } > > > } > > > > > > - table[0].data = &net->ipv4.tcp_death_row->sysctl_max_tw_buckets; > > > - > > > net->ipv4.ipv4_hdr = register_net_sysctl(net, "net/ipv4", table); > > > if (!net->ipv4.ipv4_hdr) > > > goto err_reg; > > > diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c > > > index a07243f66d4c..e2a6511218f8 100644 > > > --- a/net/ipv4/tcp_ipv4.c > > > +++ b/net/ipv4/tcp_ipv4.c > > > @@ -292,7 +292,7 @@ int tcp_v4_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len) > > > * complete initialization after this. > > > */ > > > tcp_set_state(sk, TCP_SYN_SENT); > > > - tcp_death_row = net->ipv4.tcp_death_row; > > > + tcp_death_row = &net->ipv4.tcp_death_row; > > > err = inet_hash_connect(tcp_death_row, sk); > > > if (err) > > > goto failure; > > > @@ -3091,13 +3091,9 @@ EXPORT_SYMBOL(tcp_prot); > > > > > > static void __net_exit tcp_sk_exit(struct net *net) > > > { > > > - struct inet_timewait_death_row *tcp_death_row = net->ipv4.tcp_death_row; > > > - > > > if (net->ipv4.tcp_congestion_control) > > > bpf_module_put(net->ipv4.tcp_congestion_control, > > > net->ipv4.tcp_congestion_control->owner); > > > - if (refcount_dec_and_test(&tcp_death_row->tw_refcount)) > > > - kfree(tcp_death_row); > > > > We might add a debug check about tw_refcount being 1 at this point. > > > > WARN_ON_ONCE(!refcount_dec_and_test(&net->ipv4.tcp_death_row.tw_refcount))); > > I'll add it. In tcp_sk_exit(), we could still have tw sockets and inet_twsk_purge() could get rid of them. So, I'll add the check in tcp_sk_exit_batch(). > > Thank you! > > > > > } > > > > > > static int __net_init tcp_sk_init(struct net *net) > > > @@ -3129,13 +3125,9 @@ static int __net_init tcp_sk_init(struct net *net) > > > net->ipv4.sysctl_tcp_tw_reuse = 2; > > > net->ipv4.sysctl_tcp_no_ssthresh_metrics_save = 1; > > > > > > - net->ipv4.tcp_death_row = kzalloc(sizeof(struct inet_timewait_death_row), GFP_KERNEL); > > > - if (!net->ipv4.tcp_death_row) > > > - return -ENOMEM; > > > - refcount_set(&net->ipv4.tcp_death_row->tw_refcount, 1); > > > cnt = tcp_hashinfo.ehash_mask + 1; > > > - net->ipv4.tcp_death_row->sysctl_max_tw_buckets = cnt / 2; > > > - net->ipv4.tcp_death_row->hashinfo = &tcp_hashinfo; > > > + net->ipv4.tcp_death_row.sysctl_max_tw_buckets = cnt / 2; > > > + net->ipv4.tcp_death_row.hashinfo = &tcp_hashinfo; > > > > > > net->ipv4.sysctl_max_syn_backlog = max(128, cnt / 128); > > > net->ipv4.sysctl_tcp_sack = 1; > > > diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c > > > index 80ce27f8f77e..8bddb2a78b21 100644 > > > --- a/net/ipv4/tcp_minisocks.c > > > +++ b/net/ipv4/tcp_minisocks.c > > > @@ -250,7 +250,7 @@ void tcp_time_wait(struct sock *sk, int state, int timeo) > > > struct net *net = sock_net(sk); > > > struct inet_timewait_sock *tw; > > > > > > - tw = inet_twsk_alloc(sk, net->ipv4.tcp_death_row, state); > > > + tw = inet_twsk_alloc(sk, &net->ipv4.tcp_death_row, state); > > > > > > if (tw) { > > > struct tcp_timewait_sock *tcptw = tcp_twsk((struct sock *)tw); > > > diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c > > > index 5c562d69fddf..eb1da7a63fbb 100644 > > > --- a/net/ipv6/tcp_ipv6.c > > > +++ b/net/ipv6/tcp_ipv6.c > > > @@ -325,7 +325,7 @@ static int tcp_v6_connect(struct sock *sk, struct sockaddr *uaddr, > > > inet->inet_dport = usin->sin6_port; > > > > > > tcp_set_state(sk, TCP_SYN_SENT); > > > - tcp_death_row = net->ipv4.tcp_death_row; > > > + tcp_death_row = &net->ipv4.tcp_death_row; > > > err = inet6_hash_connect(tcp_death_row, sk); > > > if (err) > > > goto late_failure;