From patchwork Fri Apr 27 18:28:52 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Benjamin Poirier X-Patchwork-Id: 155568 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 949EDB6FD7 for ; Sat, 28 Apr 2012 04:30:04 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1761145Ab2D0S3p (ORCPT ); Fri, 27 Apr 2012 14:29:45 -0400 Received: from mail-iy0-f174.google.com ([209.85.210.174]:36744 "EHLO mail-iy0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1761022Ab2D0S3n (ORCPT ); Fri, 27 Apr 2012 14:29:43 -0400 Received: by iadi9 with SMTP id i9so1330645iad.19 for ; Fri, 27 Apr 2012 11:29:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:from:to:cc:subject:date:message-id:x-mailer; bh=eBGgbar34ihKf6exm6FpJ4oLzc8UhzM84ajKl3svII0=; b=uxI99orBq5Gk9aLdRISjkTsgE3ruH4XpLOpbM53UYtvGCyRmJH634/DHx4XGf2Qx8K aLaPjCg1SL7e+ZwtfEMGjWPWY6Wwk2yz118gMf8YfRMrBNqJZG/PBEEr0+YFBH/P+EYN MMJdndBxR89trOL6Mh6YgIzPJmR0S0eVTDjFXko80IGHaNdRyvrXd1cQPCo0/Wc97u7D RwQQoXi2Y2k2PfSUUktXISFakI3fzPn1qi1KItQquBZ/nwqF44r1j2b0/RqUGQwdJ05w ORI452NWRHufW8X6A7YQ51gFzgqytQerEogUnFrLtP1ZJV2Mg8h8ih8TpZutA4/pKxtz Y6kg== Received: by 10.42.63.80 with SMTP id b16mr10984179ici.29.1335551382577; Fri, 27 Apr 2012 11:29:42 -0700 (PDT) Received: from localhost.localdomain (modemcable118.38-22-96.mc.videotron.ca. [96.22.38.118]) by mx.google.com with ESMTPS id vp3sm6255247igb.3.2012.04.27.11.29.39 (version=TLSv1/SSLv3 cipher=OTHER); Fri, 27 Apr 2012 11:29:41 -0700 (PDT) From: Benjamin Poirier To: netdev@vger.kernel.org Cc: Pablo Neira Ayuso , Patrick McHardy , "David S. Miller" , Andrew Morton , Eric Dumazet , Mike Frysinger , Arun Sharma , netfilter-devel@vger.kernel.org, netfilter@vger.kernel.org, coreteam@netfilter.org, linux-kernel@vger.kernel.org, "Paul E. McKenney" Subject: [PATCH RFC 1/2] netfilter: conntrack: remove RCU usage in conntrack notifier Date: Fri, 27 Apr 2012 14:28:52 -0400 Message-Id: <1335551333-6103-1-git-send-email-bpoirier@suse.de> X-Mailer: git-send-email 1.7.7 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org I think that the rcu usage in this code is pointless. It should either be removed or, if it was intended to protect against something, it ought to make that clear. 1) The code does not make use of the deferred deletion/wait for completion rcu api (ie. synchronize_rcu(), call_rcu()). 2) It does not benefit from the barriers implied by the rcu primitives used. The code deals with callback pointers. There's no need to order writes to the function code (!) before writes to the function pointers here. --- include/net/netfilter/nf_conntrack_ecache.h | 20 +++------ include/net/netns/conntrack.h | 4 +- net/netfilter/nf_conntrack_ecache.c | 58 +++++++------------------- 3 files changed, 25 insertions(+), 57 deletions(-) diff --git a/include/net/netfilter/nf_conntrack_ecache.h b/include/net/netfilter/nf_conntrack_ecache.h index a88fb69..ac5b8d2 100644 --- a/include/net/netfilter/nf_conntrack_ecache.h +++ b/include/net/netfilter/nf_conntrack_ecache.h @@ -99,14 +99,13 @@ nf_conntrack_eventmask_report(unsigned int eventmask, struct nf_ct_event_notifier *notify; struct nf_conntrack_ecache *e; - rcu_read_lock(); - notify = rcu_dereference(net->ct.nf_conntrack_event_cb); + notify = net->ct.nf_conntrack_event_cb; if (notify == NULL) - goto out_unlock; + return ret; e = nf_ct_ecache_find(ct); if (e == NULL) - goto out_unlock; + return ret; if (nf_ct_is_confirmed(ct) && !nf_ct_is_dying(ct)) { struct nf_ct_event item = { @@ -118,7 +117,7 @@ nf_conntrack_eventmask_report(unsigned int eventmask, unsigned long missed = e->pid ? 0 : e->missed; if (!((eventmask | missed) & e->ctmask)) - goto out_unlock; + return ret; ret = notify->fcn(eventmask | missed, &item); if (unlikely(ret < 0 || missed)) { @@ -137,8 +136,6 @@ nf_conntrack_eventmask_report(unsigned int eventmask, spin_unlock_bh(&ct->lock); } } -out_unlock: - rcu_read_unlock(); return ret; } @@ -178,14 +175,13 @@ nf_ct_expect_event_report(enum ip_conntrack_expect_events event, struct nf_exp_event_notifier *notify; struct nf_conntrack_ecache *e; - rcu_read_lock(); - notify = rcu_dereference(net->ct.nf_expect_event_cb); + notify = net->ct.nf_expect_event_cb; if (notify == NULL) - goto out_unlock; + return; e = nf_ct_ecache_find(exp->master); if (e == NULL) - goto out_unlock; + return; if (e->expmask & (1 << event)) { struct nf_exp_event item = { @@ -195,8 +191,6 @@ nf_ct_expect_event_report(enum ip_conntrack_expect_events event, }; notify->fcn(1 << event, &item); } -out_unlock: - rcu_read_unlock(); } static inline void diff --git a/include/net/netns/conntrack.h b/include/net/netns/conntrack.h index 7a911ec..c96fd8c 100644 --- a/include/net/netns/conntrack.h +++ b/include/net/netns/conntrack.h @@ -18,8 +18,8 @@ struct netns_ct { struct hlist_nulls_head unconfirmed; struct hlist_nulls_head dying; struct ip_conntrack_stat __percpu *stat; - struct nf_ct_event_notifier __rcu *nf_conntrack_event_cb; - struct nf_exp_event_notifier __rcu *nf_expect_event_cb; + struct nf_ct_event_notifier *nf_conntrack_event_cb; + struct nf_exp_event_notifier *nf_expect_event_cb; int sysctl_events; unsigned int sysctl_events_retry_timeout; int sysctl_acct; diff --git a/net/netfilter/nf_conntrack_ecache.c b/net/netfilter/nf_conntrack_ecache.c index b924f3a..0134009 100644 --- a/net/netfilter/nf_conntrack_ecache.c +++ b/net/netfilter/nf_conntrack_ecache.c @@ -38,19 +38,18 @@ void nf_ct_deliver_cached_events(struct nf_conn *ct) struct nf_ct_event item; int ret; - rcu_read_lock(); - notify = rcu_dereference(net->ct.nf_conntrack_event_cb); + notify = net->ct.nf_conntrack_event_cb; if (notify == NULL) - goto out_unlock; + return; e = nf_ct_ecache_find(ct); if (e == NULL) - goto out_unlock; + return; events = xchg(&e->cache, 0); if (!nf_ct_is_confirmed(ct) || nf_ct_is_dying(ct) || !events) - goto out_unlock; + return; /* We make a copy of the missed event cache without taking * the lock, thus we may send missed events twice. However, @@ -58,7 +57,7 @@ void nf_ct_deliver_cached_events(struct nf_conn *ct) missed = e->missed; if (!((events | missed) & e->ctmask)) - goto out_unlock; + return; item.ct = ct; item.pid = 0; @@ -67,7 +66,7 @@ void nf_ct_deliver_cached_events(struct nf_conn *ct) ret = notify->fcn(events | missed, &item); if (likely(ret >= 0 && !missed)) - goto out_unlock; + return; spin_lock_bh(&ct->lock); if (ret < 0) @@ -75,9 +74,6 @@ void nf_ct_deliver_cached_events(struct nf_conn *ct) else e->missed &= ~missed; spin_unlock_bh(&ct->lock); - -out_unlock: - rcu_read_unlock(); } EXPORT_SYMBOL_GPL(nf_ct_deliver_cached_events); @@ -85,21 +81,14 @@ int nf_conntrack_register_notifier(struct net *net, struct nf_ct_event_notifier *new) { int ret = 0; - struct nf_ct_event_notifier *notify; mutex_lock(&nf_ct_ecache_mutex); - notify = rcu_dereference_protected(net->ct.nf_conntrack_event_cb, - lockdep_is_held(&nf_ct_ecache_mutex)); - if (notify != NULL) { + if (net->ct.nf_conntrack_event_cb != NULL) ret = -EBUSY; - goto out_unlock; - } - rcu_assign_pointer(net->ct.nf_conntrack_event_cb, new); + else + net->ct.nf_conntrack_event_cb = new; mutex_unlock(&nf_ct_ecache_mutex); - return ret; -out_unlock: - mutex_unlock(&nf_ct_ecache_mutex); return ret; } EXPORT_SYMBOL_GPL(nf_conntrack_register_notifier); @@ -107,13 +96,9 @@ EXPORT_SYMBOL_GPL(nf_conntrack_register_notifier); void nf_conntrack_unregister_notifier(struct net *net, struct nf_ct_event_notifier *new) { - struct nf_ct_event_notifier *notify; - mutex_lock(&nf_ct_ecache_mutex); - notify = rcu_dereference_protected(net->ct.nf_conntrack_event_cb, - lockdep_is_held(&nf_ct_ecache_mutex)); - BUG_ON(notify != new); - RCU_INIT_POINTER(net->ct.nf_conntrack_event_cb, NULL); + BUG_ON(net->ct.nf_conntrack_event_cb != new); + net->ct.nf_conntrack_event_cb = NULL; mutex_unlock(&nf_ct_ecache_mutex); } EXPORT_SYMBOL_GPL(nf_conntrack_unregister_notifier); @@ -122,21 +107,14 @@ int nf_ct_expect_register_notifier(struct net *net, struct nf_exp_event_notifier *new) { int ret = 0; - struct nf_exp_event_notifier *notify; mutex_lock(&nf_ct_ecache_mutex); - notify = rcu_dereference_protected(net->ct.nf_expect_event_cb, - lockdep_is_held(&nf_ct_ecache_mutex)); - if (notify != NULL) { + if (net->ct.nf_expect_event_cb != NULL) ret = -EBUSY; - goto out_unlock; - } - rcu_assign_pointer(net->ct.nf_expect_event_cb, new); + else + net->ct.nf_expect_event_cb = new; mutex_unlock(&nf_ct_ecache_mutex); - return ret; -out_unlock: - mutex_unlock(&nf_ct_ecache_mutex); return ret; } EXPORT_SYMBOL_GPL(nf_ct_expect_register_notifier); @@ -144,13 +122,9 @@ EXPORT_SYMBOL_GPL(nf_ct_expect_register_notifier); void nf_ct_expect_unregister_notifier(struct net *net, struct nf_exp_event_notifier *new) { - struct nf_exp_event_notifier *notify; - mutex_lock(&nf_ct_ecache_mutex); - notify = rcu_dereference_protected(net->ct.nf_expect_event_cb, - lockdep_is_held(&nf_ct_ecache_mutex)); - BUG_ON(notify != new); - RCU_INIT_POINTER(net->ct.nf_expect_event_cb, NULL); + BUG_ON(net->ct.nf_expect_event_cb != new); + net->ct.nf_expect_event_cb = NULL; mutex_unlock(&nf_ct_ecache_mutex); } EXPORT_SYMBOL_GPL(nf_ct_expect_unregister_notifier);