From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Dumazet Subject: Re: [PATCH] reduce the spinlock conflict during massive connect Date: Sun, 05 Nov 2017 22:58:27 -0800 Message-ID: <1509951507.2849.82.camel@edumazet-glaptop3.roam.corp.google.com> References: <9b38c346-0035-4c12-21e7-dbde9f961c8a@gmail.com> <1509946035.2849.79.camel@edumazet-glaptop3.roam.corp.google.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Cc: Linux Kernel Network Developers , "David S. Miller ,Alexey Kuznetsov " ",Hideaki YOSHIFUJI" To: Liu Yu Return-path: Received: from mail-it0-f66.google.com ([209.85.214.66]:55584 "EHLO mail-it0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751462AbdKFG6c (ORCPT ); Mon, 6 Nov 2017 01:58:32 -0500 Received: by mail-it0-f66.google.com with SMTP id l196so3923856itl.4 for ; Sun, 05 Nov 2017 22:58:32 -0800 (PST) In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On Mon, 2017-11-06 at 14:48 +0800, Liu Yu wrote: > On Mon, Nov 6, 2017 at 1:27 PM, Eric Dumazet wrote: > > On Mon, 2017-11-06 at 10:28 +0800, Liu Yu wrote: > >> From: Liu Yu > >> > >> When a mount of processes connect to the same port at the same address > >> simultaneously, they are likely getting the same bhash and therefore > >> conflict with each other. > >> > >> The more the cpu number, the worse in this case. > >> > >> Use spin_trylock instead for this scene, which seems doesn't matter > >> for common case. > >> > >> Signed-off-by: Liu Yu > >> --- > >> net/ipv4/inet_hashtables.c | 6 +++++- > >> 1 files changed, 5 insertions(+), 1 deletions(-) > >> > >> diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c > >> index e7d15fb..cc11ec7 100644 > >> --- a/net/ipv4/inet_hashtables.c > >> +++ b/net/ipv4/inet_hashtables.c > >> @@ -581,13 +581,17 @@ int __inet_hash_connect(struct inet_timewait_death_row *death_row, > >> other_parity_scan: > >> port = low + offset; > >> for (i = 0; i < remaining; i += 2, port += 2) { > >> + int ret; > >> + > >> if (unlikely(port >= high)) > >> port -= remaining; > >> if (inet_is_local_reserved_port(net, port)) > >> continue; > >> head = &hinfo->bhash[inet_bhashfn(net, port, > >> hinfo->bhash_size)]; > >> - spin_lock_bh(&head->lock); > >> + ret = spin_trylock(&head->lock); > >> + if (unlikely(!ret)) > >> + continue; > >> > >> /* Does not bother with rcv_saddr checks, because > >> * the established check is already unique enough. > > > > This is broken. > > > > I am pretty sure you have not really tested this patch properly. > > > > Chances are very high that a connect() will miss slots and wont succeed, > > when table is almost full. > > Thanks for your comments! > > Can you explain how connect() miss slots when table is almost full? Every time your spin_trylock() is failing, you will not examin one port. Now go back to the loop : for (i = 0; i < remaining; i += 2, port += 2) { Since we no longer look all the ports, we are going to fail to find a 4-tuple. Really a spin_trylock() is a not a good idea. Find something else if you really have contention at connect() > > > > > Performance is nice, but we actually need to allocate a 4-tuple in a > > more deterministic fashion. > > > > So, what's the 4th element would you suggest? What 4th element I am suggesting ?