* Re: [PATCH 4/4] bnx2x: merge common code when stopping queue
From: Eilon Greenstein @ 2010-03-10 16:02 UTC (permalink / raw)
To: Stanislaw Gruszka
Cc: netdev@vger.kernel.org, Vladislav Zolotarov, Michael Chan
In-Reply-To: <1268153703-4186-4-git-send-email-sgruszka@redhat.com>
On Tue, 2010-03-09 at 08:55 -0800, Stanislaw Gruszka wrote:
> Main reason for merge code is willingness to use memory barrier to avoid
> races with bnx2x_tx_int(). There is no real situation possible to
> trigger the races. But if we assume that bug of not stopped and full
> queue can happen, we should also stop queue then in the safe way.
>
> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> ---
> I have no strong fillings that patch should be applied, if Vladislav
> do not want the patch, I'll will not argue.
>
Stanislaw,
As you wrote, there is no real situation possible to trigger this race.
So basically, you are changing the code from “return” on error to “go
to” and in the “go to” branch you add some harmless but useless code in
the form of smp_mb and testing for a possible wake again. I find the
code to be less readable after this change and harder to maintain so I
prefer not to include this code change.
Thanks for all the changes and you participant in improving the bnx2x!
Eilon
> drivers/net/bnx2x_main.c | 37 ++++++++++++++++++++-----------------
> 1 files changed, 20 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/net/bnx2x_main.c b/drivers/net/bnx2x_main.c
> index 6c042a7..6751ca2 100644
> --- a/drivers/net/bnx2x_main.c
> +++ b/drivers/net/bnx2x_main.c
> @@ -11176,10 +11176,9 @@ static netdev_tx_t bnx2x_start_xmit(struct sk_buff *skb, struct net_device *dev)
> struct eth_tx_bd *tx_data_bd, *total_pkt_bd = NULL;
> struct eth_tx_parse_bd *pbd = NULL;
> u16 pkt_prod, bd_prod;
> - int nbd, fp_index;
> + int i, ret, nbd, fp_index;
> dma_addr_t mapping;
> u32 xmit_type = bnx2x_xmit_type(bp, skb);
> - int i;
> u8 hlen = 0;
> __le16 pkt_size = 0;
>
> @@ -11194,10 +11193,9 @@ static netdev_tx_t bnx2x_start_xmit(struct sk_buff *skb, struct net_device *dev)
> fp = &bp->fp[fp_index];
>
> if (unlikely(bnx2x_tx_avail(fp) < (skb_shinfo(skb)->nr_frags + 3))) {
> - fp->eth_q_stats.driver_xoff++;
> - netif_tx_stop_queue(txq);
> BNX2X_ERR("BUG! Tx ring full when queue awake!\n");
> - return NETDEV_TX_BUSY;
> + ret = NETDEV_TX_BUSY;
> + goto stop_queue;
> }
>
> DP(NETIF_MSG_TX_QUEUED, "SKB: summed %x protocol %x protocol(%x,%x)"
> @@ -11425,22 +11423,27 @@ static netdev_tx_t bnx2x_start_xmit(struct sk_buff *skb, struct net_device *dev)
> mmiowb();
>
> fp->tx_bd_prod += nbd;
> + fp->tx_pkt++;
>
> - if (unlikely(bnx2x_tx_avail(fp) < MAX_SKB_FRAGS + 3)) {
> - netif_tx_stop_queue(txq);
> + ret = NETDEV_TX_OK;
> + if (unlikely(bnx2x_tx_avail(fp) < MAX_SKB_FRAGS + 3))
> + goto stop_queue;
>
> - /* paired memory barrier is in bnx2x_tx_int(), we have to keep
> - * ordering of set_bit() in netif_tx_stop_queue() and read of
> - * fp->bd_tx_cons */
> - smp_mb();
> + return ret;
>
> - fp->eth_q_stats.driver_xoff++;
> - if (bnx2x_tx_avail(fp) >= MAX_SKB_FRAGS + 3)
> - netif_tx_wake_queue(txq);
> - }
> - fp->tx_pkt++;
> +stop_queue:
> + netif_tx_stop_queue(txq);
> +
> + /* paired memory barrier is in bnx2x_tx_int(), we have to keep
> + * ordering of set_bit() in netif_tx_stop_queue() and read of
> + * fp->bd_tx_cons */
> + smp_mb();
>
> - return NETDEV_TX_OK;
> + fp->eth_q_stats.driver_xoff++;
> + if (bnx2x_tx_avail(fp) >= MAX_SKB_FRAGS + 3)
> + netif_tx_wake_queue(txq);
> +
> + return ret;
> }
>
> /* called with rtnl_lock */
^ permalink raw reply
* Re: [PATCH 3/4] bnx2x: change smp_mb() comment to conform the true
From: Eilon Greenstein @ 2010-03-10 16:01 UTC (permalink / raw)
To: Stanislaw Gruszka
Cc: netdev@vger.kernel.org, Vladislav Zolotarov, Michael Chan
In-Reply-To: <1268153703-4186-3-git-send-email-sgruszka@redhat.com>
On Tue, 2010-03-09 at 08:55 -0800, Stanislaw Gruszka wrote:
> Access to fp->tx_bp_prod is protected by __netif_tx_lock,
> smp_mb() is not needed for that.
>
> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
^ permalink raw reply
* Re: [PATCH 2/4] bnx2x: remove not necessary compiler barrier
From: Eilon Greenstein @ 2010-03-10 16:00 UTC (permalink / raw)
To: Stanislaw Gruszka
Cc: netdev@vger.kernel.org, Vladislav Zolotarov, Michael Chan
In-Reply-To: <1268153703-4186-2-git-send-email-sgruszka@redhat.com>
On Tue, 2010-03-09 at 08:55 -0800, Stanislaw Gruszka wrote:
> Access to fp->tx_bd_prod is protected by __netif_tx_lock, so we do not
> need any barrier for that.
>
> Update of fp->tx_bd_cons in bnx2x_tx_int() is not protected by lock, but
> barrier() nor smb_mb() in bnx2x_tx_avail() not guarantee we will see
> values that is written on other cpu. Ordering issues between
> netif_tx_stop_queue(), netif_tx_queue_stopped(), fp->tx_bd_cons = bd_cons
> and bnx2x_tx_avail() are already handled by smp_mb() in bnx2x_tx_int()
> and bnx2x_start_xmit().
>
> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
^ permalink raw reply
* Re: [PATCH 1/4] bnx2x: use smp_mb() to keep ordering of read write operations
From: Eilon Greenstein @ 2010-03-10 15:59 UTC (permalink / raw)
To: Stanislaw Gruszka
Cc: netdev@vger.kernel.org, Vladislav Zolotarov, Michael Chan
In-Reply-To: <1268153703-4186-1-git-send-email-sgruszka@redhat.com>
On Tue, 2010-03-09 at 08:55 -0800, Stanislaw Gruszka wrote:
> Since we want to keep ordering of write to fp->bd_tx_cons and
> netif_tx_queue_stopped(txq), what is read of txq->state, we have to use
> general memory barrier.
>
> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
Singed-of-by: Eilon Greenstein <eilong@broadcom.com>
^ permalink raw reply
* bug: route metric signing
From: Curtis Doty @ 2010-03-10 15:41 UTC (permalink / raw)
To: Linux Network Development
The metric set is not the metric displayed. I'm presuming it's a cosmetic
error in iproute2?
# ip route add 192.0.2.42 via 127.0.0.1 metric 4294967295
# ip route ls 192.0.2.42
192.0.2.42 via 127.0.0.1 dev lo metric -1
And...
# ip route add 192.0.2.42 via 127.0.0.1 metric 2147483648
# ip route ls 192.0.2.42
192.0.2.42 via 127.0.0.1 dev lo metric -2147483648
But this one appears OK...
# ip route add 192.0.2.42 via 127.0.0.1 metric 2147483647
# ip route ls 192.0.2.42
192.0.2.42 via 127.0.0.1 dev lo metric 2147483647
Test platform is Fedora 12.
# rpm -q iproute
iproute-2.6.29-4.fc12.x86_64
# rpm -q kernel
kernel-2.6.32.9-70.fc12.x86_64
../C
^ permalink raw reply
* Re: [PATCH] can: bfin_can: switch to common Blackfin can header
From: David Miller @ 2010-03-10 15:35 UTC (permalink / raw)
To: vapier.adi-Re5JQEeQqe8AvxtiuMwx3w
Cc: urs.thuermann-l29pVbxQd1IUtdQbppsyvg,
netdev-u79uwXL29TY76Z2rM5mHXA,
socketcan-core-0fE9KPoRgkgATYTw5x5z8w,
oliver.hartkopp-l29pVbxQd1IUtdQbppsyvg,
uclinux-dist-devel-ZG0+EudsQA8dtHy/vicBwGD2FQJk+8+b,
wg-5Yr1BZd7O62+XT7JhA+gdA
In-Reply-To: <8bd0f97a1003100417x6e9830dav949fd6b0511d99d0-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
From: Mike Frysinger <vapier.adi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Date: Wed, 10 Mar 2010 07:17:09 -0500
> On Wed, Mar 10, 2010 at 02:41, Wolfgang Grandegger wrote:
>> Mike Frysinger wrote:
>>> The MMR bits are being moved to this header, so include it.
>>>
>>> Signed-off-by: Mike Frysinger <vapier-aBrp7R+bbdUdnm+yROfE0A@public.gmane.org>
>>
>> Acked-by: Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>
>>
>>> ---
>>> note: this can be merged for 2.6.34 or 2.6.35
>>
>> Is it required to make the driver working under 2.6.34?
>
> not for 2.6.34, but it will be for 2.6.35
I'll apply this to net-next-2.6 once I open it up, therefore.
^ permalink raw reply
* Re: [PATCH] net: remove rcu locking from fib_rules_event()
From: David Miller @ 2010-03-10 15:35 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev, paulmck
In-Reply-To: <1268201018.2872.139.camel@edumazet-laptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 10 Mar 2010 07:03:38 +0100
> We hold RTNL at this point and dont use RCU variants of list traversals,
> we dont need rcu_read_lock()/rcu_read_unlock()
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
I'll apply this, but to my net-next-2.6 tree once I open that
up.
^ permalink raw reply
* Re: [PATCH] net: Annotates neigh_invalidate()
From: David Miller @ 2010-03-10 15:35 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev
In-Reply-To: <1268199654.2872.134.camel@edumazet-laptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 10 Mar 2010 06:40:54 +0100
> Annotates neigh_invalidate() with __releases() and __acquires() for
> sparse sake.
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH] net: Fix dev_mc_add()
From: David Miller @ 2010-03-10 15:34 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev, jpirko
In-Reply-To: <1268197748.2872.129.camel@edumazet-laptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 10 Mar 2010 06:09:08 +0100
> Commit 6e17d45a (net: add addr len check to dev_mc_add)
> added a bug in dev_mc_add(), since it can now exit with a lock
> imbalance.
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> CC: Jiri Pirko <jpirko@redhat.com>
Applied.
^ permalink raw reply
* Re: When best to submit patches with small changes in multiple files?
From: David Miller @ 2010-03-10 15:34 UTC (permalink / raw)
To: elendil; +Cc: netdev
In-Reply-To: <201003100449.12952.elendil@planet.nl>
From: Frans Pop <elendil@planet.nl>
Date: Wed, 10 Mar 2010 04:49:12 +0100
> I have patches prepared to remove trailing spaces from kernel messages
> for ./net and ./drivers/net (including wireless).
>
> In total 15 patches with
> 81 files changed, 351 insertions(+), 351 deletions(-)
>
> When would be the best time to submit:
> - now, to be included in .34
> - now, for -next
> - any time
> - near beginning of merge window (rebased against netdev-next)
> - not at all ;-)
Later, for -next. Submit them say in a week or two.
^ permalink raw reply
* Re: [PATCH net-2.6] ipv6 ip6_tunnel: eliminate unused recursion field from ip6_tnl{}.
From: David Miller @ 2010-03-10 15:34 UTC (permalink / raw)
To: yoshfuji; +Cc: netdev
In-Reply-To: <201003100247.o2A2lqaX001898@94.43.138.210.xn.2iij.net>
From: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
Date: Wed, 10 Mar 2010 11:47:52 +0900
> Commit a43912ab19... ("tunnel: eliminate recursion field") eliminated
> use of recursion field from tunnel structures, but its definition
> still exists in ip6_tnl{}.
>
> Let's remove that unused field.
>
> Signed-off-by: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
Applied.
^ permalink raw reply
* Re: [PATCH] asix: fix setting mac address for AX88772
From: David Miller @ 2010-03-10 15:34 UTC (permalink / raw)
To: jussi.kivilinna; +Cc: netdev, dhollis
In-Reply-To: <20100309222438.10684.46211.stgit@fate.lan>
From: Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
Date: Wed, 10 Mar 2010 00:24:38 +0200
> Setting new MAC address only worked when device was set to promiscuous mode.
> Fix MAC address by writing new address to device using undocumented command
> AX_CMD_READ_NODE_ID+1. Patch is tested with AX88772 device.
>
> Signed-off-by: Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
Applied.
^ permalink raw reply
* [BUG 2.6.34-rc1] bogus "(null)" in tulip init messages
From: Mikael Pettersson @ 2010-03-10 15:33 UTC (permalink / raw)
To: netdev; +Cc: Grant Grundler, Kyle McMartin, linux-kernel
Booting 2.6.34-rc1 on a machine with a tulip nic I see
a number of kernel messages that include "(null)" where
previous kernels included the string "tulip0":
--- dmesg-2.6.33 2010-02-26 12:58:32.000000000 +0100
+++ dmesg-2.6.34-rc1 2010-03-10 16:17:06.000000000 +0100
...
Linux Tulip driver version 1.1.15 (Feb 27, 2007)
tulip 0000:00:0f.0: enabling device (0014 -> 0017)
-tulip0: EEPROM default media type Autosense.
-tulip0: Index #0 - Media 10baseT (#0) described by a 21142 Serial PHY (2) block.
-tulip0: Index #1 - Media 10baseT-FDX (#4) described by a 21142 Serial PHY (2) block.
-tulip0: Index #2 - Media 100baseTx (#3) described by a 21143 SYM PHY (4) block.
-tulip0: Index #3 - Media 100baseTx-FDX (#5) described by a 21143 SYM PHY (4) block.
-tulip0: Index #4 - Media 100baseTx (#3) described by a 21143 reset method (5) block.
-eth0: Digital DS21142/43 Tulip rev 48 at Port 0x800, 00:00:c5:50:f9:51, IRQ 25.
+ (null): EEPROM default media type Autosense
+ (null): Index #0 - Media 10baseT (#0) described by a 21142 Serial PHY (2) block
+ (null): Index #1 - Media 10baseT-FDX (#4) described by a 21142 Serial PHY (2) block
+ (null): Index #2 - Media 100baseTx (#3) described by a 21143 SYM PHY (4) block
+ (null): Index #3 - Media 100baseTx-FDX (#5) described by a 21143 SYM PHY (4) block
+ (null): Index #4 - Media 100baseTx (#3) described by a 21143 reset method (5) block
+net eth0: Digital DS21142/43 Tulip rev 48 at Port 0x800, 00:00:c5:50:f9:51, IRQ 25
Apart from this the tulip driver seems to work in 2.6.34-rc1.
/Mikael
^ permalink raw reply
* Re: [2.6.34-rc1] CPU stalls when closing TCP sockets.
From: David Miller @ 2010-03-10 15:33 UTC (permalink / raw)
To: eric.dumazet; +Cc: penguin-kernel, netdev, linux-kernel, paulmck, shemminger
In-Reply-To: <1268150156.3113.12.camel@edumazet-laptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 09 Mar 2010 16:55:56 +0100
> Le mardi 09 mars 2010 à 15:54 +0900, Tetsuo Handa a écrit :
>> Eric Dumazet wrote:
>> > Nothing comes to my mind, I'll try to reproduce this here.
>>
>> When stopped at (8), Ctrl-C doesn't work.
>>
>> > Is 2.6.33 OK ?
>>
>> Yes. 2.6.33 and earlier are OK.
>
> OK thanks !
>
> I believe commit d218d11133d888f9745802146a50255a4781d37a
> (tcp: Generalized TTL Security Mechanism) might be the bug origin.
>
> I am testing following patch, based on latest net-2.6 tree (including
> the LINUX_MIB_TCPMINTTLDROP bit)
>
> [PATCH] tcp: Fix tcp_v4_rcv()
>
> Commit d218d111 (tcp: Generalized TTL Security Mechanism) added a bug
> for TIMEWAIT sockets. We should not test min_ttl for TW sockets.
>
> Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Applied, thanks everyone.
^ permalink raw reply
* Re: [PATCH] tg3: Fix tg3_poll_controller() passing wrong pointer to tg3_interrupt()
From: David Miller @ 2010-03-10 15:33 UTC (permalink / raw)
To: louis.rilling; +Cc: mcarlson, mchan, netdev, stable
In-Reply-To: <1268151281-15012-1-git-send-email-louis.rilling@kerlabs.com>
From: Louis Rilling <louis.rilling@kerlabs.com>
Date: Tue, 9 Mar 2010 17:14:41 +0100
> [Resending without buggy address for stable. Sorry.]
>
> Commit 09943a1819a240ff4a72f924d0038818fcdd0a90
> Author: Matt Carlson <mcarlson@broadcom.com>
> Date: Fri Aug 28 14:01:57 2009 +0000
>
> tg3: Convert ISR parameter to tnapi
>
> forgot to update tg3_poll_controller(), leading to intermittent crashes with
> netpoll.
>
> Fix this.
>
> Signed-off-by: Louis Rilling <louis.rilling@kerlabs.com>
> Cc: stable@kernel.org
Applied, thanks.
^ permalink raw reply
* Re: [PATCH] net: add ColdFire support to the smc91x driver
From: David Miller @ 2010-03-10 15:33 UTC (permalink / raw)
To: gerg; +Cc: nico, gerg, netdev
In-Reply-To: <201003090703.o2973pbP013668@goober.internal.moreton.com.au>
From: Greg Ungerer <gerg@snapgear.com>
Date: Tue, 9 Mar 2010 17:03:51 +1000
> net: add ColdFire support to the smc91x driver
>
> Some embedded ColdFire based boards use the SMC 91x family of ethernet
> devices. (For example the Freescale M5249C3 and MoretonBay NETtel).
>
> Add IO access support to the SMC91x driver, and allow this driver to
> be configured for ColdFire platforms.
>
> Signed-off-by: Greg Ungerer <gerg@uclinux.org>
Applied.
^ permalink raw reply
* Re: [patch 0/5] qeth bug fixes for 2.6.34 next rc
From: David Miller @ 2010-03-10 15:32 UTC (permalink / raw)
To: frank.blaschka; +Cc: netdev, linux-s390
In-Reply-To: <20100309063652.706791000@de.ibm.com>
From: frank.blaschka@de.ibm.com
Date: Tue, 09 Mar 2010 07:36:52 +0100
> here are some qeth bug fixes for 2.6.34 next rc
All applied, thanks.
^ permalink raw reply
* [PATCH net-next-2.6] bridge: ensure to unlock in error path in br_multicast_query().
From: YOSHIFUJI Hideaki @ 2010-03-10 15:07 UTC (permalink / raw)
To: davem, netdev; +Cc: yoshfuji
Signed-off-by: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
---
net/bridge/br_multicast.c | 11 +++++++----
1 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 2559fb5..2326a34 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -823,6 +823,7 @@ static int br_multicast_query(struct net_bridge *br,
unsigned long max_delay;
unsigned long now = jiffies;
__be32 group;
+ int err = 0;
spin_lock(&br->multicast_lock);
if (!netif_running(br->dev) ||
@@ -841,12 +842,14 @@ static int br_multicast_query(struct net_bridge *br,
group = 0;
}
} else {
- if (!pskb_may_pull(skb, sizeof(struct igmpv3_query)))
- return -EINVAL;
+ if (!pskb_may_pull(skb, sizeof(struct igmpv3_query))) {
+ err = -EINVAL;
+ goto out;
+ }
ih3 = igmpv3_query_hdr(skb);
if (ih3->nsrcs)
- return 0;
+ goto out;
max_delay = ih3->code ? 1 :
IGMPV3_MRC(ih3->code) * (HZ / IGMP_TIMER_SCALE);
@@ -876,7 +879,7 @@ static int br_multicast_query(struct net_bridge *br,
out:
spin_unlock(&br->multicast_lock);
- return 0;
+ return err;
}
static void br_multicast_leave_group(struct net_bridge *br,
--
1.5.6.5
^ permalink raw reply related
* Re: bug report: netpoll: allow execution of multiple rx_hooks per interface
From: David Miller @ 2010-03-10 14:52 UTC (permalink / raw)
To: danborkmann; +Cc: error27, netdev
In-Reply-To: <4B977EDB.5010202@gmail.com>
From: Daniel Borkmann <danborkmann@googlemail.com>
Date: Wed, 10 Mar 2010 12:13:31 +0100
> Sorry for the late reply, I'm currently on holiday and have hardly
> access to the Internet. I'll fix the problem as soon as possible.
>
> You'll hear from me.
Thanks.
^ permalink raw reply
* Re: [PATCH 6/13] bridge: Add core IGMP snooping support
From: Herbert Xu @ 2010-03-10 14:07 UTC (permalink / raw)
To: Paul E. McKenney
Cc: Eric Dumazet, Arnd Bergmann, David S. Miller, netdev,
Stephen Hemminger
In-Reply-To: <20100310131317.GA6267@linux.vnet.ibm.com>
On Wed, Mar 10, 2010 at 05:13:18AM -0800, Paul E. McKenney wrote:
>
> If CONFIG_PROVE_RCU is set, rcu_dereference() checks for rcu_read_lock()
> and rcu_dereference_bh() checks for either rcu_read_lock_bh() or BH
> being disabled. Yes, this is a bit restrictive, but there are a few too
> many to check by hand these days.
Fair enough. We should get those fixed then. In fact I reckon
most of them should be using the BH variant so we might be able
to kill a few rcu_read_lock's which would be a real gain.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH 6/13] bridge: Add core IGMP snooping support
From: Paul E. McKenney @ 2010-03-10 13:57 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: Herbert Xu, David S. Miller, netdev, Stephen Hemminger
In-Reply-To: <201003101430.06736.arnd@arndb.de>
On Wed, Mar 10, 2010 at 02:30:06PM +0100, Arnd Bergmann wrote:
> On Wednesday 10 March 2010, Paul E. McKenney wrote:
> > > When rt_hash_table[h].chain gets the __rcu_bh annotation, we'd have to
> > > turn first rcu_dereference into rcu_dereference_bh in order to have a clean
> > > build with sparse. Would that change be
> > > a) correct from RCU perspective,
> > > b) desirable for code inspection, and
> > > c) lockdep-clean?
> >
> > I have a patch queued up that will make rcu_dereference_bh() handle this
> > correctly -- current -tip and mainline would complain. Please see below
> > for a sneak preview.
> >
> > Thoughts?
>
> Ok, so that would mean we can convert it all to rcu_dereference_bh().
> I guess an alternative to this would be to also change the rcu_read_lock()
> inside local_bh_disable() sections to rcu_read_lock_bh(), which is not
> necessary but also not harmful, right?
It does impose additional overhead, which the networking guys are eager
to avoid, given that network links speeds are continuing to increase. ;-)
So moving to rcu_dereference_bh() seems better to me.
(Please note that rcu_dereference_bh() imposes checking overhead only
in the presence of both CONFIG_PROVE_LOCKING and CONFIG_PROVE_RCU.)
Thanx, Paul
^ permalink raw reply
* Re: [PATCH 6/13] bridge: Add core IGMP snooping support
From: Arnd Bergmann @ 2010-03-10 13:39 UTC (permalink / raw)
To: Herbert Xu
Cc: Eric Dumazet, paulmck, David S. Miller, netdev, Stephen Hemminger
In-Reply-To: <20100310104907.GA23752@gondor.apana.org.au>
On Wednesday 10 March 2010, Herbert Xu wrote:
> >
> > Its really rcu_dereference_bh() that could/should be used:
> > I see no problem changing
> >
> >
> > local_bh_disable();
> > ...
> > rcu_read_lock();
> > rcu_dereference(rt_hash_table[h].chain);
> > rcu_read_unlock();
> > ...
> > local_bh_enable();
>
> Why don't we just ignore the bh part for rcu_dereference?
>
> After all it's call_rcu_bh and the other primitives that we really
> care about. For rcu_dereference bh should make no difference
> whatsoever.
To add some background on what I'm doing, I'm currently adding
new address space modifier __rcu, __rcu_bh, __rcu_sched and __srcu
to the sparse annotations along the same lines that our __iomem,
__user and __percpu annotations work [1].
In order to check all cases, I want to ensure that you can not
use any of those pointers outside of rcu_dereference* and
rcu_assign_pointer, as well as making sure that you cannot pass
a pointer without these annotations in there, so we can catch
code that uses rcu_dereference without rcu_assign_pointer or
call_rcu.
Consequently, rcu_dereference also checks that the pointer is actually
__rcu, and passing an __rcu_bh pointer in would be considered as
wrong as passing a regular pointer by sparse.
With the work that Paul has done on lockdep, rcu_dereference_bh
now also checks that bottom halves are really disabled, which is
a very useful thing to check if you want to prove that the
call_rcu is really serialized with the use of the data.
Arnd
[1] http://git.kernel.org/?p=linux/kernel/git/arnd/playground.git;a=shortlog;h=refs/heads/rcu-annotate
^ permalink raw reply
* Re: [PATCH 6/13] bridge: Add core IGMP snooping support
From: Arnd Bergmann @ 2010-03-10 13:27 UTC (permalink / raw)
To: Herbert Xu
Cc: Eric Dumazet, paulmck, David S. Miller, netdev, Stephen Hemminger
In-Reply-To: <20100310104907.GA23752@gondor.apana.org.au>
On Wednesday 10 March 2010, Herbert Xu wrote:
> >
> > Its really rcu_dereference_bh() that could/should be used:
> > I see no problem changing
> >
> >
> > local_bh_disable();
> > ...
> > rcu_read_lock();
> > rcu_dereference(rt_hash_table[h].chain);
> > rcu_read_unlock();
> > ...
> > local_bh_enable();
>
> Why don't we just ignore the bh part for rcu_dereference?
>
> After all it's call_rcu_bh and the other primitives that we really
> care about. For rcu_dereference bh should make no difference
> whatsoever.
To add some background on what I'm doing, I'm currently adding
new address space modifier __rcu, __rcu_bh, __rcu_sched and __srcu
to the sparse annotations along the same lines that our __iomem,
__user and __percpu annotations work [1].
In order to check all cases, I want to ensure that you can not
use any of those pointers outside of rcu_dereference* and
rcu_assign_pointer, as well as making sure that you cannot pass
a pointer without these annotations in there, so we can catch
code that uses rcu_dereference without rcu_assign_pointer or
call_rcu.
Consequently, rcu_dereference also checks that the pointer is actually
__rcu, and passing an __rcu_bh pointer in would be considered as
wrong as passing a regular pointer by sparse.
With the work that Paul has done on lockdep, rcu_dereference_bh
now also checks that bottom halves are really disabled, which is
a very useful thing to check if you want to prove that the
call_rcu is really serialized with the use of the data.
Arnd
[1] http://git.kernel.org/?p=linux/kernel/git/arnd/playground.git;a=shortlog;h=refs/heads/rcu-annotate
^ permalink raw reply
* Re: [PATCH 6/13] bridge: Add core IGMP snooping support
From: Arnd Bergmann @ 2010-03-10 13:30 UTC (permalink / raw)
To: paulmck; +Cc: Herbert Xu, David S. Miller, netdev, Stephen Hemminger
In-Reply-To: <20100310131946.GB6267@linux.vnet.ibm.com>
On Wednesday 10 March 2010, Paul E. McKenney wrote:
> > When rt_hash_table[h].chain gets the __rcu_bh annotation, we'd have to
> > turn first rcu_dereference into rcu_dereference_bh in order to have a clean
> > build with sparse. Would that change be
> > a) correct from RCU perspective,
> > b) desirable for code inspection, and
> > c) lockdep-clean?
>
> I have a patch queued up that will make rcu_dereference_bh() handle this
> correctly -- current -tip and mainline would complain. Please see below
> for a sneak preview.
>
> Thoughts?
Ok, so that would mean we can convert it all to rcu_dereference_bh().
I guess an alternative to this would be to also change the rcu_read_lock()
inside local_bh_disable() sections to rcu_read_lock_bh(), which is not
necessary but also not harmful, right?
Arnd
^ permalink raw reply
* Re: [PATCH 6/13] bridge: Add core IGMP snooping support
From: Paul E. McKenney @ 2010-03-10 13:19 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: Herbert Xu, David S. Miller, netdev, Stephen Hemminger
In-Reply-To: <201003101041.32518.arnd@arndb.de>
On Wed, Mar 10, 2010 at 10:41:32AM +0100, Arnd Bergmann wrote:
> On Wednesday 10 March 2010 03:14:10 Paul E. McKenney wrote:
> > On Tue, Mar 09, 2010 at 10:12:59PM +0100, Arnd Bergmann wrote:
> >
> > > I've just tried annotating net/ipv4/route.c like this and did not get
> > > very far, because the same pointers are used for rcu and rcu_bh.
> > > Could you check if this is a false positive or an actual finding?
> >
> > Hmmm... I am only seeing a call_rcu_bh() here, so unless I am missing
> > something, this is a real problem in TREE_PREEMPT_RCU kernels. The
> > call_rcu_bh() only interacts with the rcu_read_lock_bh() readers, not
> > the rcu_read_lock() readers.
> >
> > One approach is to run freed blocks through both types of grace periods,
> > I suppose.
>
> Well, if I introduce different __rcu and __rcu_bh address space annotations,
> sparse would still not like that, because then you can only pass the annotated
> pointers into either rcu_dereference or rcu_dereference_bh.
>
> What the code seems to be doing here is in some places
>
> local_bh_disable();
> ...
> rcu_read_lock();
> rcu_dereference(rt_hash_table[h].chain);
> rcu_read_unlock();
> ...
> local_bh_enable();
>
> and in others
>
> rcu_read_lock_bh();
> rcu_dereference_bh(rt_hash_table[h].chain);
> rcu_read_unlock_bh();
Hmmm... This is actually legal.
> When rt_hash_table[h].chain gets the __rcu_bh annotation, we'd have to
> turn first rcu_dereference into rcu_dereference_bh in order to have a clean
> build with sparse. Would that change be
> a) correct from RCU perspective,
> b) desirable for code inspection, and
> c) lockdep-clean?
I have a patch queued up that will make rcu_dereference_bh() handle this
correctly -- current -tip and mainline would complain. Please see below
for a sneak preview.
Thoughts?
Thanx, Paul
rcu: make rcu_read_lock_bh_held() allow for disabled BH
Disabling BH can stand in for rcu_read_lock_bh(), and this patch updates
rcu_read_lock_bh_held() to allow for this. In order to avoid
include-file hell, this function is moved out of line to kernel/rcupdate.c.
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
include/linux/rcupdate.h | 19 ++++---------------
kernel/rcupdate.c | 22 ++++++++++++++++++++++
2 files changed, 26 insertions(+), 15 deletions(-)
diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 75921b8..c393acc 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -119,22 +119,11 @@ static inline int rcu_read_lock_held(void)
return lock_is_held(&rcu_lock_map);
}
-/**
- * rcu_read_lock_bh_held - might we be in RCU-bh read-side critical section?
- *
- * If CONFIG_PROVE_LOCKING is selected and enabled, returns nonzero iff in
- * an RCU-bh read-side critical section. In absence of CONFIG_PROVE_LOCKING,
- * this assumes we are in an RCU-bh read-side critical section unless it can
- * prove otherwise.
- *
- * Check rcu_scheduler_active to prevent false positives during boot.
+/*
+ * rcu_read_lock_bh_held() is defined out of line to avoid #include-file
+ * hell.
*/
-static inline int rcu_read_lock_bh_held(void)
-{
- if (!debug_lockdep_rcu_enabled())
- return 1;
- return lock_is_held(&rcu_bh_lock_map);
-}
+extern int rcu_read_lock_bh_held(void);
/**
* rcu_read_lock_sched_held - might we be in RCU-sched read-side critical section?
diff --git a/kernel/rcupdate.c b/kernel/rcupdate.c
index f1125c1..913eccb 100644
--- a/kernel/rcupdate.c
+++ b/kernel/rcupdate.c
@@ -45,6 +45,7 @@
#include <linux/mutex.h>
#include <linux/module.h>
#include <linux/kernel_stat.h>
+#include <linux/hardirq.h>
#ifdef CONFIG_DEBUG_LOCK_ALLOC
static struct lock_class_key rcu_lock_key;
@@ -66,6 +67,27 @@ EXPORT_SYMBOL_GPL(rcu_sched_lock_map);
int rcu_scheduler_active __read_mostly;
EXPORT_SYMBOL_GPL(rcu_scheduler_active);
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+
+/**
+ * rcu_read_lock_bh_held - might we be in RCU-bh read-side critical section?
+ *
+ * Check for bottom half being disabled, which covers both the
+ * CONFIG_PROVE_RCU and not cases. Note that if someone uses
+ * rcu_read_lock_bh(), but then later enables BH, lockdep (if enabled)
+ * will show the situation.
+ *
+ * Check debug_lockdep_rcu_enabled() to prevent false positives during boot.
+ */
+int rcu_read_lock_bh_held(void)
+{
+ if (!debug_lockdep_rcu_enabled())
+ return 1;
+ return in_softirq();
+}
+
+#endif /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */
+
/*
* This function is invoked towards the end of the scheduler's initialization
* process. Before this is called, the idle task might contain
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox