From patchwork Mon Jan 13 15:25:36 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Christoph Paasch X-Patchwork-Id: 310020 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 3C9F12C0079 for ; Tue, 14 Jan 2014 02:26:04 +1100 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751962AbaAMPZv (ORCPT ); Mon, 13 Jan 2014 10:25:51 -0500 Received: from mail-we0-f179.google.com ([74.125.82.179]:62290 "EHLO mail-we0-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751655AbaAMPZp (ORCPT ); Mon, 13 Jan 2014 10:25:45 -0500 Received: by mail-we0-f179.google.com with SMTP id w62so1471939wes.24 for ; Mon, 13 Jan 2014 07:25:43 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:from:to:cc:subject:date:message-id; bh=Cf8OYHf6bCparm4MwugJzY26Uh8DHGTk5WO11lU7alI=; b=cG9QgZR4aDqEZ/C8rzIOV4JcWgLdu+KB8e5uqSUxUoDkPTBh2DdwGrQgkBCtvX4DJu xsYWiePI+3g71Z1QcUHHm6LwGzP2Wq1vHo1L/F0tlVgFueeNn4VeCBEkWzdeSMSrsIoK 1aDDTbYcNZSDD231Yxvg+B8Rc0TyGmEa3coEPW1fTARbh2TViC5PRQk9Nkhq4zqmRIXg m/1CTcC9xWnw9qkyC3o6fl0SUtTeg3r1dXz0zJEbDjOb2M2by18tbrw6C4M6HEtgFDGE SG8iYY8ociBTD1YfyLayUN2d1/1x6v3v96qQj4aig7VfkPgeDswW9uTpYs00qf52iQg5 /pWw== X-Received: by 10.194.61.84 with SMTP id n20mr4039226wjr.61.1389626743808; Mon, 13 Jan 2014 07:25:43 -0800 (PST) Received: from localhost (cpaasch-mac.dhcp.info.ucl.ac.be. [130.104.228.20]) by mx.google.com with ESMTPSA id bj3sm11763259wjb.14.2014.01.13.07.25.42 for (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Mon, 13 Jan 2014 07:25:43 -0800 (PST) From: Christoph Paasch To: netdev@vger.kernel.org Cc: David Miller , Eric Dumazet Subject: [RFC net] tcp: metrics: Avoid duplicate entries with the same destination-IP Date: Mon, 13 Jan 2014 16:25:36 +0100 Message-Id: <1389626736-3143-1-git-send-email-christoph.paasch@uclouvain.be> X-Mailer: git-send-email 1.8.3.2 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org Note: This patch is based on "net" and so the source-IP is not yet part of the tcp-metrics. Because the tcp-metrics is an RCU-list, it may be that two soft-interrupts are inside __tcp_get_metrics() for the same destination-IP at the same time. If this destination-IP is not yet part of the tcp-metrics, both soft-interrupts will end up in tcpm_new and create a new entry for this IP. So, we will have two tcp-metrics with the same destination-IP in the list. This patch now takes the tcp_metrics_lock before calling __tcp_get_metrics(). I post this as an RFC, because this patch will make the TCP-stack take the tcp_metrics_lock even if the metric is already in the cache. I tested it with apache-benchmark on a short file with ab -c 100 -n 100000 server/1KB IMO this should stress the code most, but there was no significant performance regression. Another solution might be to leave tcp_get_metrics() as it is, and in tcpm_new do another call to __tcp_get_metrics() while holding the spin-lock. We would then check __tcp_get_metrics twice for new entries but we won't hold the spin-lock needlessly anymore. So, it's a tradeoff between taking the tcp_metrics_lock more often vs. calling __tcp_get_metrics twice for new entries. Suggestions are very welcome. Fixes: 51c5d0c4b169b (tcp: Maintain dynamic metrics in local cache.) Signed-off-by: Christoph Paasch --- net/ipv4/tcp_metrics.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/net/ipv4/tcp_metrics.c b/net/ipv4/tcp_metrics.c index 06493736fbc8..7748a5d9f37a 100644 --- a/net/ipv4/tcp_metrics.c +++ b/net/ipv4/tcp_metrics.c @@ -138,7 +138,6 @@ static struct tcp_metrics_block *tcpm_new(struct dst_entry *dst, struct tcp_metrics_block *tm; struct net *net; - spin_lock_bh(&tcp_metrics_lock); net = dev_net(dst->dev); if (unlikely(reclaim)) { struct tcp_metrics_block *oldest; @@ -153,7 +152,7 @@ static struct tcp_metrics_block *tcpm_new(struct dst_entry *dst, } else { tm = kmalloc(sizeof(*tm), GFP_ATOMIC); if (!tm) - goto out_unlock; + return NULL; } tm->tcpm_addr = *addr; @@ -164,8 +163,6 @@ static struct tcp_metrics_block *tcpm_new(struct dst_entry *dst, rcu_assign_pointer(net->ipv4.tcp_metrics_hash[hash].chain, tm); } -out_unlock: - spin_unlock_bh(&tcp_metrics_lock); return tm; } @@ -303,6 +300,8 @@ static struct tcp_metrics_block *tcp_get_metrics(struct sock *sk, net = dev_net(dst->dev); hash = hash_32(hash, net->ipv4.tcp_metrics_hash_log); + spin_lock_bh(&tcp_metrics_lock); + tm = __tcp_get_metrics(&addr, net, hash); reclaim = false; if (tm == TCP_METRICS_RECLAIM_PTR) { @@ -314,6 +313,8 @@ static struct tcp_metrics_block *tcp_get_metrics(struct sock *sk, else tcpm_check_stamp(tm, dst); + spin_unlock_bh(&tcp_metrics_lock); + return tm; }