From: "Paul E. McKenney" <paulmck@linux•vnet.ibm.com>
To: Herbert Xu <herbert@gondor•apana.org.au>
Cc: "Michael S. Tsirkin" <mst@redhat•com>,
Qianfeng Zhang <frzhang@redhat•com>,
"David S. Miller" <davem@davemloft•net>,
netdev@vger•kernel.org, WANG Cong <amwang@redhat•com>,
Stephen Hemminger <shemminger@vyatta•com>,
Matt Mackall <mpm@selenic•com>
Subject: Re: [PATCH 3/8] netpoll: Fix RCU usage
Date: Fri, 11 Jun 2010 16:10:21 -0700 [thread overview]
Message-ID: <20100611231021.GK2394@linux.vnet.ibm.com> (raw)
In-Reply-To: <E1OMtjg-0006Ob-Sb@gondolin.me.apana.org.au>
On Fri, Jun 11, 2010 at 12:12:44PM +1000, Herbert Xu wrote:
> netpoll: Fix RCU usage
>
> The use of RCU in netpoll is incorrect in a number of places:
>
> 1) The initial setting is lacking a write barrier.
> 2) The synchronize_rcu is in the wrong place.
> 3) Read barriers are missing.
> 4) Some places are even missing rcu_read_lock.
> 5) npinfo is zeroed after freeing.
>
> This patch fixes those issues. As most users are in BH context,
> this also converts the RCU usage to the BH variant.
Looks good for me from an RCU viewpoint, but as usual I confess much
ignorance of the surrounding code.
Acked-by: Paul E. McKenney <paulmck@linux•vnet.ibm.com>
> Signed-off-by: Herbert Xu <herbert@gondor•apana.org.au>
> ---
>
> include/linux/netpoll.h | 13 ++++++++-----
> net/core/netpoll.c | 20 ++++++++++++--------
> 2 files changed, 20 insertions(+), 13 deletions(-)
>
> diff --git a/include/linux/netpoll.h b/include/linux/netpoll.h
> index e9e2312..95c9f7e 100644
> --- a/include/linux/netpoll.h
> +++ b/include/linux/netpoll.h
> @@ -57,12 +57,15 @@ void netpoll_send_skb(struct netpoll *np, struct sk_buff *skb);
> #ifdef CONFIG_NETPOLL
> static inline bool netpoll_rx(struct sk_buff *skb)
> {
> - struct netpoll_info *npinfo = skb->dev->npinfo;
> + struct netpoll_info *npinfo;
> unsigned long flags;
> bool ret = false;
>
> + rcu_read_lock_bh();
> + npinfo = rcu_dereference(skb->dev->npinfo);
> +
> if (!npinfo || (list_empty(&npinfo->rx_np) && !npinfo->rx_flags))
> - return false;
> + goto out;
>
> spin_lock_irqsave(&npinfo->rx_lock, flags);
> /* check rx_flags again with the lock held */
> @@ -70,12 +73,14 @@ static inline bool netpoll_rx(struct sk_buff *skb)
> ret = true;
> spin_unlock_irqrestore(&npinfo->rx_lock, flags);
>
> +out:
> + rcu_read_unlock_bh();
> return ret;
> }
>
> static inline int netpoll_rx_on(struct sk_buff *skb)
> {
> - struct netpoll_info *npinfo = skb->dev->npinfo;
> + struct netpoll_info *npinfo = rcu_dereference(skb->dev->npinfo);
>
> return npinfo && (!list_empty(&npinfo->rx_np) || npinfo->rx_flags);
> }
> @@ -91,7 +96,6 @@ static inline void *netpoll_poll_lock(struct napi_struct *napi)
> {
> struct net_device *dev = napi->dev;
>
> - rcu_read_lock(); /* deal with race on ->npinfo */
> if (dev && dev->npinfo) {
> spin_lock(&napi->poll_lock);
> napi->poll_owner = smp_processor_id();
> @@ -108,7 +112,6 @@ static inline void netpoll_poll_unlock(void *have)
> napi->poll_owner = -1;
> spin_unlock(&napi->poll_lock);
> }
> - rcu_read_unlock();
> }
>
> #else
> diff --git a/net/core/netpoll.c b/net/core/netpoll.c
> index 748c930..4b7623d 100644
> --- a/net/core/netpoll.c
> +++ b/net/core/netpoll.c
> @@ -292,6 +292,7 @@ void netpoll_send_skb(struct netpoll *np, struct sk_buff *skb)
> unsigned long tries;
> struct net_device *dev = np->dev;
> const struct net_device_ops *ops = dev->netdev_ops;
> + /* It is up to the caller to keep npinfo alive. */
> struct netpoll_info *npinfo = np->dev->npinfo;
>
> if (!npinfo || !netif_running(dev) || !netif_device_present(dev)) {
> @@ -841,10 +842,7 @@ int netpoll_setup(struct netpoll *np)
> refill_skbs();
>
> /* last thing to do is link it to the net device structure */
> - ndev->npinfo = npinfo;
> -
> - /* avoid racing with NAPI reading npinfo */
> - synchronize_rcu();
> + rcu_assign_pointer(ndev->npinfo, npinfo);
>
> return 0;
>
> @@ -888,6 +886,16 @@ void netpoll_cleanup(struct netpoll *np)
>
> if (atomic_dec_and_test(&npinfo->refcnt)) {
> const struct net_device_ops *ops;
> +
> + ops = np->dev->netdev_ops;
> + if (ops->ndo_netpoll_cleanup)
> + ops->ndo_netpoll_cleanup(np->dev);
> +
> + rcu_assign_pointer(np->dev->npinfo, NULL);
> +
> + /* avoid racing with NAPI reading npinfo */
> + synchronize_rcu_bh();
> +
> skb_queue_purge(&npinfo->arp_tx);
> skb_queue_purge(&npinfo->txq);
> cancel_rearming_delayed_work(&npinfo->tx_work);
> @@ -895,10 +903,6 @@ void netpoll_cleanup(struct netpoll *np)
> /* clean after last, unfinished work */
> __skb_queue_purge(&npinfo->txq);
> kfree(npinfo);
> - ops = np->dev->netdev_ops;
> - if (ops->ndo_netpoll_cleanup)
> - ops->ndo_netpoll_cleanup(np->dev);
> - np->dev->npinfo = NULL;
> }
> }
>
next prev parent reply other threads:[~2010-06-11 23:10 UTC|newest]
Thread overview: 63+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-06-10 12:40 [0/8] netpoll/bridge fixes Herbert Xu
2010-06-10 12:42 ` [PATCH 1/7] netpoll: Set npinfo to NULL even with ndo_netpoll_cleanup Herbert Xu
2010-06-10 12:42 ` [PATCH 2/7] bridge: Remove redundant npinfo NULL setting Herbert Xu
2010-06-10 12:42 ` [PATCH 3/7] netpoll: Fix RCU usage Herbert Xu
2010-06-10 12:42 ` [PATCH 4/7] netpoll: Add locking for netpoll_setup/cleanup Herbert Xu
2010-06-10 12:42 ` [PATCH 5/7] netpoll: Add ndo_netpoll_setup Herbert Xu
2010-06-10 12:42 ` [PATCH 6/7] netpoll: Allow netpoll_setup/cleanup recursion Herbert Xu
2010-06-10 12:42 ` [PATCH 7/7] bridge: Fix netpoll support Herbert Xu
2010-06-10 14:49 ` [0/8] netpoll/bridge fixes Stephen Hemminger
2010-06-10 21:56 ` Herbert Xu
2010-06-10 21:59 ` Stephen Hemminger
2010-06-10 22:48 ` Herbert Xu
2010-06-11 2:11 ` Herbert Xu
2010-06-11 2:12 ` [PATCH 1/8] netpoll: Set npinfo to NULL even with ndo_netpoll_cleanup Herbert Xu
2010-06-11 2:12 ` [PATCH 2/8] bridge: Remove redundant npinfo NULL setting Herbert Xu
2010-06-11 2:12 ` [PATCH 3/8] netpoll: Fix RCU usage Herbert Xu
2010-06-11 23:10 ` Paul E. McKenney [this message]
2010-06-11 2:12 ` [PATCH 4/8] netpoll: Add locking for netpoll_setup/cleanup Herbert Xu
2010-06-11 2:12 ` [PATCH 5/8] netpoll: Add ndo_netpoll_setup Herbert Xu
2010-06-11 2:12 ` [PATCH 6/8] netpoll: Allow netpoll_setup/cleanup recursion Herbert Xu
2010-06-25 1:21 ` Andrew Morton
2010-06-25 3:01 ` Herbert Xu
2010-06-25 3:30 ` David Miller
2010-06-25 3:50 ` Andrew Morton
2010-06-25 4:27 ` David Miller
2010-06-25 4:42 ` Andrew Morton
2010-06-25 4:52 ` David Miller
2010-06-25 8:08 ` Peter Zijlstra
2010-06-25 8:42 ` Andrew Morton
2010-06-25 9:45 ` Peter Zijlstra
2010-06-25 8:46 ` Ingo Molnar
2010-06-25 10:08 ` Nick Piggin
2010-06-11 2:12 ` [PATCH 7/8] netpoll: Add netpoll_tx_running Herbert Xu
2010-06-11 2:12 ` [PATCH 8/8] bridge: Fix netpoll support Herbert Xu
2010-06-11 3:08 ` fired a bug report on bugzilla.redhat.com Qianfeng Zhang
2010-06-15 10:28 ` [PATCH 8/8] bridge: Fix netpoll support Cong Wang
2010-06-17 10:38 ` Herbert Xu
2010-06-17 10:57 ` Cong Wang
2010-06-17 10:55 ` Herbert Xu
2010-06-18 3:06 ` Cong Wang
2010-06-11 20:03 ` [0/8] netpoll/bridge fixes Matt Mackall
2010-06-15 10:17 ` Cong Wang
2010-06-15 18:39 ` David Miller
2010-06-16 2:58 ` Eric Dumazet
2010-06-16 3:03 ` Eric Dumazet
2010-06-16 3:33 ` Herbert Xu
2010-06-16 4:47 ` David Miller
2010-06-16 23:02 ` Paul E. McKenney
2010-06-17 10:18 ` Michael S. Tsirkin
2010-06-17 21:26 ` Paul E. McKenney
2010-06-16 6:16 ` Eric Dumazet
2010-06-16 5:08 ` Paul E. McKenney
2010-06-16 6:21 ` Eric Dumazet
2010-06-16 16:01 ` Paul E. McKenney
2010-07-19 10:19 ` Michael S. Tsirkin
2010-07-19 10:53 ` Herbert Xu
2010-07-19 11:54 ` Herbert Xu
2010-07-19 16:05 ` David Miller
2010-07-19 16:52 ` Eric Dumazet
2010-07-19 20:35 ` David Miller
2010-07-20 5:26 ` Herbert Xu
2010-07-20 6:28 ` David Miller
2010-06-29 12:53 ` Yanko Kaneti
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20100611231021.GK2394@linux.vnet.ibm.com \
--to=paulmck@linux$(echo .)vnet.ibm.com \
--cc=amwang@redhat$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=frzhang@redhat$(echo .)com \
--cc=herbert@gondor$(echo .)apana.org.au \
--cc=mpm@selenic$(echo .)com \
--cc=mst@redhat$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=shemminger@vyatta$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox