* [PATCH] tcp: bind() use stronger condition for bind_conflict
From: Alexandru Copot @ 2012-03-27 21:11 UTC (permalink / raw)
To: davem, eric.dumazet
Cc: kuznet, jmorris, yoshfuji, kaber, netdev, linux-kernel,
Alexandru Copot, Daniel Baluta
We must try harder to get unique (addr, port) pairs when
doing port autoselection for sockets with SO_REUSEADDR
option set.
We achieve this by adding a relaxation parameter to
inet_csk_bind_conflict. When 'relax' parameter is off
we return a conflict whenever the current searched
pair (addr, port) is not unique.
This tries to address the problems reported in patch:
8d238b25b1ec22a73b1c2206f111df2faaff8285
Revert "tcp: bind() fix when many ports are bound"
Signed-off-by: Alexandru Copot <alex.mihai.c@gmail.com>
Signed-off-by: Daniel Baluta <dbaluta@ixiacom.com>
---
include/net/inet6_connection_sock.h | 2 +-
include/net/inet_connection_sock.h | 4 ++--
net/ipv4/inet_connection_sock.c | 17 +++++++++++++----
net/ipv6/inet6_connection_sock.c | 3 ++-
4 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/include/net/inet6_connection_sock.h b/include/net/inet6_connection_sock.h
index 3207e58..9d29ae2 100644
--- a/include/net/inet6_connection_sock.h
+++ b/include/net/inet6_connection_sock.h
@@ -23,7 +23,7 @@ struct sock;
struct sockaddr;
extern int inet6_csk_bind_conflict(const struct sock *sk,
- const struct inet_bind_bucket *tb);
+ const struct inet_bind_bucket *tb, int relax);
extern struct dst_entry* inet6_csk_route_req(struct sock *sk,
const struct request_sock *req);
diff --git a/include/net/inet_connection_sock.h b/include/net/inet_connection_sock.h
index dbf9aab..f05a032 100644
--- a/include/net/inet_connection_sock.h
+++ b/include/net/inet_connection_sock.h
@@ -60,7 +60,7 @@ struct inet_connection_sock_af_ops {
#endif
void (*addr2sockaddr)(struct sock *sk, struct sockaddr *);
int (*bind_conflict)(const struct sock *sk,
- const struct inet_bind_bucket *tb);
+ const struct inet_bind_bucket *tb, int relax);
};
/** inet_connection_sock - INET connection oriented sock
@@ -245,7 +245,7 @@ extern struct request_sock *inet_csk_search_req(const struct sock *sk,
const __be32 raddr,
const __be32 laddr);
extern int inet_csk_bind_conflict(const struct sock *sk,
- const struct inet_bind_bucket *tb);
+ const struct inet_bind_bucket *tb, int relax);
extern int inet_csk_get_port(struct sock *sk, unsigned short snum);
extern struct dst_entry* inet_csk_route_req(struct sock *sk,
diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
index 19d66ce..bf50e77 100644
--- a/net/ipv4/inet_connection_sock.c
+++ b/net/ipv4/inet_connection_sock.c
@@ -53,7 +53,7 @@ void inet_get_local_port_range(int *low, int *high)
EXPORT_SYMBOL(inet_get_local_port_range);
int inet_csk_bind_conflict(const struct sock *sk,
- const struct inet_bind_bucket *tb)
+ const struct inet_bind_bucket *tb, int relax)
{
struct sock *sk2;
struct hlist_node *node;
@@ -79,6 +79,13 @@ int inet_csk_bind_conflict(const struct sock *sk,
sk2_rcv_saddr == sk_rcv_saddr(sk))
break;
}
+ if (!relax && reuse && sk2->sk_reuse &&
+ sk2->sk_state != TCP_LISTEN) {
+ const __be32 sk2_rcv_saddr = sk_rcv_saddr(sk2);
+ if (!sk2_rcv_saddr || !sk_rcv_saddr(sk) ||
+ sk2_rcv_saddr == sk_rcv_saddr(sk))
+ break;
+ }
}
}
return node != NULL;
@@ -122,12 +129,13 @@ again:
(tb->num_owners < smallest_size || smallest_size == -1)) {
smallest_size = tb->num_owners;
smallest_rover = rover;
- if (atomic_read(&hashinfo->bsockets) > (high - low) + 1) {
+ if (atomic_read(&hashinfo->bsockets) > (high - low) + 1 &&
+ !inet_csk(sk)->icsk_af_ops->bind_conflict(sk, tb, 0)) {
snum = smallest_rover;
goto tb_found;
}
}
- if (!inet_csk(sk)->icsk_af_ops->bind_conflict(sk, tb)) {
+ if (!inet_csk(sk)->icsk_af_ops->bind_conflict(sk, tb, 0)) {
snum = rover;
goto tb_found;
}
@@ -178,12 +186,13 @@ tb_found:
goto success;
} else {
ret = 1;
- if (inet_csk(sk)->icsk_af_ops->bind_conflict(sk, tb)) {
+ if (inet_csk(sk)->icsk_af_ops->bind_conflict(sk, tb, 1)) {
if (sk->sk_reuse && sk->sk_state != TCP_LISTEN &&
smallest_size != -1 && --attempts >= 0) {
spin_unlock(&head->lock);
goto again;
}
+
goto fail_unlock;
}
}
diff --git a/net/ipv6/inet6_connection_sock.c b/net/ipv6/inet6_connection_sock.c
index 02dd203..dfc8bc3 100644
--- a/net/ipv6/inet6_connection_sock.c
+++ b/net/ipv6/inet6_connection_sock.c
@@ -28,7 +28,8 @@
#include <net/inet6_connection_sock.h>
int inet6_csk_bind_conflict(const struct sock *sk,
- const struct inet_bind_bucket *tb)
+ const struct inet_bind_bucket *tb,
+ int relax)
{
const struct sock *sk2;
const struct hlist_node *node;
--
1.7.9.4
^ permalink raw reply related
* RE: [PATCH net V4 2/2] igb: offer a PTP Hardware Clock instead of the timecompare method
From: Keller, Jacob E @ 2012-03-27 20:58 UTC (permalink / raw)
To: chetan loke
Cc: Richard Cochran, netdev@vger.kernel.org,
e1000-devel@lists.sourceforge.net, Kirsher, Jeffrey T,
Ronciak, John, john.stultz@linaro.org, tglx@linutronix.de
In-Reply-To: <CAAsGZS6=wCPKC_1HnXQNn=U=6h-akbWvLZ_9ztBjfva=DFmugQ@mail.gmail.com>
> -----Original Message-----
> From: chetan loke [mailto:loke.chetan@gmail.com]
> Sent: Tuesday, March 27, 2012 11:52 AM
> To: Keller, Jacob E
> Cc: Richard Cochran; netdev@vger.kernel.org; e1000-
> devel@lists.sourceforge.net; Kirsher, Jeffrey T; Ronciak, John;
> john.stultz@linaro.org; tglx@linutronix.de
> Subject: Re: [PATCH net V4 2/2] igb: offer a PTP Hardware Clock instead of the
> timecompare method
>
> On Tue, Mar 27, 2012 at 2:43 PM, chetan loke <loke.chetan@gmail.com> wrote:
> > On Tue, Mar 27, 2012 at 2:29 PM, Keller, Jacob E
> > <jacob.e.keller@intel.com> wrote:
> >
> >>>
> >>
> >> It isn't so much about performance gains as it is about preventing
> >> poorly written user apps from stalling the clean descriptor routines.
> >> I am working on a test case that should prove whether this is even an
> >> issue (at least with ixgbe). Once I have data on that it can be
> >> determined if the extra lock would alleviate it. The conceptual issue
> >> is that spamming get-time could cause the clean tx/rx irq routines to
> >> stall inside the interrupt for too long. (thereby not freeing up
> >> descriptors on the ring to
> >
> > What would be interesting to see is something like:
> >
> > 1) App1(getter/setter calls), executed '1000' ioctls per sec on eth0.
> > Other 'M' apps(execute just get calls), where M = 1 .. 10(?). And one
> > buggy app executed - 10K getter calls per sec.
> >
> > 2) eth0 receives PTP traffic and also around 80-90%(link rate)
> > regular/mixed traffic.
> >
> > And BTW, eth0 is the management IP.
> >
> > While the test is running, if we are still able to reach the host and
> > manage it via SSH then we can stop being paranoid.
> >
> >
>
> Guys please correct me if I'm wrong but if RPS/RFS is enabled then at a
> minimum we would need "N * 2" apps (where N == num_cpus). That way there will
> be contention w/ the completions and we will be able to see the real effect?
>
> Chetan
I think we could see contention regardless because the spinlock doesn't guarantee the ordering of who gets it next.
I am not sure. But I will try and set something like this up. However, I do think that many get-set calls is pretty high for even a 'highly' loaded system. Though the buggy app for sure is possible.
Here is what I am thinking as a test case. Linuxptp running normally with a higher sync rate than once per second, plus a 'buggy' app which will try to infinitely thread the gettime calls. I hope to have something like this working soon.
- Jake
^ permalink raw reply
* Re: [STRAW MAN PATCH] sch_teql doesn't load-balance ppp(oatm) slaves
From: David Woodhouse @ 2012-03-27 20:35 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, paulus, netdev
In-Reply-To: <1332878138.3547.41.camel@edumazet-glaptop>
[-- Attachment #1: Type: text/plain, Size: 1278 bytes --]
On Tue, 2012-03-27 at 21:55 +0200, Eric Dumazet wrote:
> I have no idea why you added all these barriers...
Um, some of them snuck in while I was working it out, and then I
couldn't prove to myself that they *weren't* needed, so I left them.
We definitely need a write barrier in pppoatm_pop() after the change to
sk->sk_sndbuf (which happens in the old_pop routine) and the increment
of pvcc->inflight. Those changes must hit *before* the test/change of
the BLOCKED bit.
But there's an implicit barrier in test_and_clear_bit() which should
achieve that, so the specific barrier you highlight may well be
superfluous. I could have sworn I had a reason for it at the time, but
can't justify it now.
On the pppoatm_may_send() side, the change to the BLOCKED bit needs a
corresponding read barrier after it, to ensure that its subsequent
checks of sk->sndbuf and pvcc->inflight are looking at the data which
were written before the BLOCKED bit is tested in pppoatm_pop().
But I suppose we can probably dispense with the barrier *before* setting
the BLOCKED bit in pppoatm_may_send(), and the barriers after increasing
pvcc->inflight.
If it looks sane other than that, I can knock up a new patch with a
S-O-B. Thanks for reviewing...
--
dwmw2
[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5818 bytes --]
^ permalink raw reply
* Re: [PATCH] net: fix a potential rcu_read_lock() imbalance in rt6_fill_node()
From: Greg KH @ 2012-03-27 20:25 UTC (permalink / raw)
To: Ben Greear
Cc: Eric Dumazet, David Miller, netdev, Paul E. McKenney, Dave Jones
In-Reply-To: <4F722045.80902@candelatech.com>
On Tue, Mar 27, 2012 at 01:17:09PM -0700, Ben Greear wrote:
> On 03/27/2012 12:53 PM, Eric Dumazet wrote:
> >Commit f2c31e32b378 (net: fix NULL dereferences in check_peer_redir() )
> >added a regression in rt6_fill_node(), leading to rcu_read_lock()
> >imbalance.
> >
> >Thats because NLA_PUT() can make a jump to nla_put_failure label.
> >
> >Fix this by using nla_put()
> >
> >Many thanks to Ben Greear for his help
> >
> >Reported-by: Ben Greear<greearb@candelatech.com>
> >Reported-by: Dave Jones<davej@redhat.com>
> >Signed-off-by: Eric Dumazet<eric.dumazet@gmail.com>
>
> This does indeed fix the problem for me (tested in the 3.0.25 kernel).
> So, please feel free to add:
>
> Tested-by: Ben Greear <greearb@candelatech.com>
Very nice job Eric, thanks for tracking this down.
greg k-h
^ permalink raw reply
* Re: [PATCH] net: fix a potential rcu_read_lock() imbalance in rt6_fill_node()
From: Ben Greear @ 2012-03-27 20:17 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev, gregkh, Paul E. McKenney, Dave Jones
In-Reply-To: <1332878032.3547.39.camel@edumazet-glaptop>
On 03/27/2012 12:53 PM, Eric Dumazet wrote:
> Commit f2c31e32b378 (net: fix NULL dereferences in check_peer_redir() )
> added a regression in rt6_fill_node(), leading to rcu_read_lock()
> imbalance.
>
> Thats because NLA_PUT() can make a jump to nla_put_failure label.
>
> Fix this by using nla_put()
>
> Many thanks to Ben Greear for his help
>
> Reported-by: Ben Greear<greearb@candelatech.com>
> Reported-by: Dave Jones<davej@redhat.com>
> Signed-off-by: Eric Dumazet<eric.dumazet@gmail.com>
This does indeed fix the problem for me (tested in the 3.0.25 kernel).
So, please feel free to add:
Tested-by: Ben Greear <greearb@candelatech.com>
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: [PATCH] net: fix a potential rcu_read_lock() imbalance in rt6_fill_node()
From: Ben Greear @ 2012-03-27 20:07 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev, gregkh, Paul E. McKenney, Dave Jones
In-Reply-To: <1332878032.3547.39.camel@edumazet-glaptop>
On 03/27/2012 12:53 PM, Eric Dumazet wrote:
> Commit f2c31e32b378 (net: fix NULL dereferences in check_peer_redir() )
> added a regression in rt6_fill_node(), leading to rcu_read_lock()
> imbalance.
>
> Thats because NLA_PUT() can make a jump to nla_put_failure label.
Ohhh, how truly awful! That NLA_PUT with hidden jump seems like
bugs just waiting to happen!
Maybe at least someone clever could make some static analysis tool to
catch that....
I'm back-porting and will test shortly.
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: [STRAW MAN PATCH] sch_teql doesn't load-balance ppp(oatm) slaves
From: Eric Dumazet @ 2012-03-27 19:55 UTC (permalink / raw)
To: David Woodhouse; +Cc: David Miller, paulus, netdev
In-Reply-To: <1332875447.2058.48.camel@shinybook.infradead.org>
On Tue, 2012-03-27 at 20:10 +0100, David Woodhouse wrote:
> On Sun, 2012-03-25 at 17:36 -0400, David Miller wrote:
> > Yes, the ATM devices deep transmit queue is quite undesirable.
>
> This should fix that, and while I'm at it should fix the gratuitous
> running of ppp_output_wakeup() from a tasklet on *every* packet, when
> it's almost never necessary. Some careful eyes over the locking issues
> on that would be much appreciated. I've documented how I *think* it
> works...
>
> I'm tempted to rip out the atm_may_send() bit; there's not a lot of
> point in checking against sk_sndbuf when we're limiting to two packets
> anyway, is there? There's always been a problem here if sk_sndbuf was
> set lower than the MTU of the interface; it would block for ever.
>
> I'm running this now on my ADSL router. I can watch it working, keeping
> precisely two packets in the queue at a time (one really in-flight and
> one ready for the ATM driver). My leftover debugging in sch_teql is
> triggering when the xmit returns NETDEV_TX_BUSY, and all seems to be
> well.
>
> --- net/atm/pppoatm.c~ 2012-03-27 19:59:54.379565896 +0100
> +++ net/atm/pppoatm.c 2012-03-27 20:03:02.676561017 +0100
> @@ -62,10 +62,13 @@ struct pppoatm_vcc {
> void (*old_pop)(struct atm_vcc *, struct sk_buff *);
> /* keep old push/pop for detaching */
> enum pppoatm_encaps encaps;
> + atomic_t inflight;
> + unsigned long blocked;
> int flags; /* SC_COMP_PROT - compress protocol */
> struct ppp_channel chan; /* interface to generic ppp layer */
> struct tasklet_struct wakeup_tasklet;
> };
> +#define BLOCKED 0
>
> /*
> * Header used for LLC Encapsulated PPP (4 bytes) followed by the LCP protocol
> @@ -102,16 +105,31 @@ static void pppoatm_wakeup_sender(unsign
> static void pppoatm_pop(struct atm_vcc *atmvcc, struct sk_buff *skb)
> {
> struct pppoatm_vcc *pvcc = atmvcc_to_pvcc(atmvcc);
> +
> pvcc->old_pop(atmvcc, skb);
> + smp_mb__before_atomic_dec();
I have no idea why you added all these barriers...
> + atomic_dec(&pvcc->inflight);
> +
> /*
^ permalink raw reply
* [PATCH] net: fix a potential rcu_read_lock() imbalance in rt6_fill_node()
From: Eric Dumazet @ 2012-03-27 19:53 UTC (permalink / raw)
To: Ben Greear; +Cc: David Miller, netdev, gregkh, Paul E. McKenney, Dave Jones
In-Reply-To: <1332877149.3547.24.camel@edumazet-glaptop>
Commit f2c31e32b378 (net: fix NULL dereferences in check_peer_redir() )
added a regression in rt6_fill_node(), leading to rcu_read_lock()
imbalance.
Thats because NLA_PUT() can make a jump to nla_put_failure label.
Fix this by using nla_put()
Many thanks to Ben Greear for his help
Reported-by: Ben Greear <greearb@candelatech.com>
Reported-by: Dave Jones <davej@redhat.com>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
net/ipv6/route.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 24c456e..496b627 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -2474,8 +2474,12 @@ static int rt6_fill_node(struct net *net,
rcu_read_lock();
n = dst_get_neighbour_noref(&rt->dst);
- if (n)
- NLA_PUT(skb, RTA_GATEWAY, 16, &n->primary_key);
+ if (n) {
+ if (nla_put(skb, RTA_GATEWAY, 16, &n->primary_key) < 0) {
+ rcu_read_unlock();
+ goto nla_put_failure;
+ }
+ }
rcu_read_unlock();
if (rt->dst.dev)
^ permalink raw reply related
* Re: RCU lock bug in 3.0.21 (bisected to: 682cb56a, fix NULL dereferences in check_peer_redir)
From: Eric Dumazet @ 2012-03-27 19:39 UTC (permalink / raw)
To: Ben Greear; +Cc: David Miller, netdev, gregkh, Paul E. McKenney
In-Reply-To: <4F71EF2A.8020507@candelatech.com>
On Tue, 2012-03-27 at 09:47 -0700, Ben Greear wrote:
> On 03/26/2012 04:39 PM, Eric Dumazet wrote:
> > On Mon, 2012-03-26 at 16:06 -0700, Ben Greear wrote:
> >> On 03/26/2012 02:53 PM, Ben Greear wrote:
> >>> On 03/26/2012 02:49 PM, David Miller wrote:
> >>>>
> >>>> Looks like all of those strange undiagnosable reported Dave Jones
> >>>> has been feeding us. Something in one part of the kernel leaves
> >>>> a lock held, and this shows up as a warning elsewhere.
> >>>
> >>> Every (initial) bug printout fingers ipv6 and the 'ip' tool on my system.
> >>
> >> I added a patch to convert rcu_read_lock/unlock to macros so
> >> that I could automatically grab the call site (_THIS_IP_)
> >> and pass it into the lockdep framework instead of the (useless)
> >> _THIS_IP_ in the old rcu_read_lock method which at best seems to
> >> only indicate which module the issue relates to...
> >
> > Hi Ben
> >
> > Is this problem also appears with current tree ?
> > (This could be a problem with the backport, as it was full of
> > dependencies)
> >
> > Also, if you use a patch to better track rcu_read_lock()/unlock(), you
> > could add new macros as well to track that a particular unlock() matches
> > one given lock(). (maybe returning the rcu_preempt_depth at
> > rcu_read_lock() time , but maybe a more absolute ref would be better)
> >
> > So we could have a warning if an unlock() doesnt match the lock()
> >
> > inet6_dump_fib () was already a suspect but we could not find why.
>
>
> Ok, I tried the patch below, and got the result farther down. Is this
> what you were thinking of? (The lockdep warning about rcu lock still
> held happened immediately after this..so it appears the depth mis-match
> does represent this problem...
>
>
> [greearb@fs3 linux-3.0.dev.y]$ git diff
> diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
> index 0f9b37a..ae3c7c9 100644
> --- a/net/ipv6/ip6_fib.c
> +++ b/net/ipv6/ip6_fib.c
> @@ -366,6 +366,7 @@ static int inet6_dump_fib(struct sk_buff *skb, struct netlink_callback *cb)
> struct hlist_node *node;
> struct hlist_head *head;
> int res = 0;
> + int depth = current->lockdep_depth;
>
> s_h = cb->args[0];
> s_e = cb->args[1];
> @@ -410,6 +411,8 @@ next:
> }
> out:
> rcu_read_unlock();
> + WARN(depth != current->lockdep_depth, "depth: %i lockdep-depth: %i\n",
> + depth, current->lockdep_depth);
> cb->args[1] = e;
> cb->args[0] = h;
>
>
>
> ------------[ cut here ]------------
> WARNING: at /home/greearb/git/linux-3.0.dev.y/net/ipv6/ip6_fib.c:415 inet6_dump_fib+0x25c/0x292 [ipv6]()
> Hardware name: To be filled by O.E.M.
> depth: 1 lockdep-depth: 2
> Modules linked in: 8021q garp stp llc fuse macvlan pktgen coretemp hwmon sunrpc ipv6 uinput arc4 ath9k snd_hda_codec_realtek mac80211 snd_hda_intel
> snd_hda_codec snd_hwdep snd_seq ath9k_common ath9k_hw snd_seq_device snd_pcm ath snd_timer e1000e cfg80211 snd mei(C) ppdev microcode i2c_i801 iTCO_wdt
> soundcore serio_raw pcspkr snd_page_alloc iTCO_vendor_support parport_pc parport i915 drm_kms_helper drm i2c_algo_bit i2c_core video [last unloaded: scsi_wait_scan]
> Pid: 6563, comm: ip Tainted: G C 3.0.25+ #16
> Call Trace:
> [<ffffffff81046866>] warn_slowpath_common+0x80/0x98
> [<ffffffff81046912>] warn_slowpath_fmt+0x41/0x43
> [<ffffffffa0251a3a>] inet6_dump_fib+0x25c/0x292 [ipv6]
> [<ffffffff813af450>] netlink_dump+0x5b/0x19b
> [<ffffffff81385da2>] ? consume_skb+0x28/0x2a
> [<ffffffff813af7bf>] netlink_recvmsg+0x1c7/0x2f8
> [<ffffffff8137c6cf>] __sock_recvmsg_nosec+0x65/0x6e
> [<ffffffff8137dde0>] __sock_recvmsg+0x49/0x54
> [<ffffffff8137e349>] sock_recvmsg+0xa6/0xbf
> [<ffffffff81072bf8>] ? lock_release_non_nested+0x9d/0x227
> [<ffffffff810ca002>] ? might_fault+0x4e/0x9e
> [<ffffffff810ca04b>] ? might_fault+0x97/0x9e
> [<ffffffff81387cae>] ? copy_from_user+0x2a/0x2c
> [<ffffffff810ca002>] ? might_fault+0x4e/0x9e
> [<ffffffff81388080>] ? verify_iovec+0x4f/0xa3
> [<ffffffff8137e0c4>] __sys_recvmsg+0x147/0x21e
> [<ffffffff81063868>] ? up_read+0x1e/0x36
> [<ffffffff810fc9fb>] ? fcheck_files+0xb7/0xee
> [<ffffffff810fcb30>] ? fget_light+0x3b/0xbc
> [<ffffffff8137e8a0>] sys_recvmsg+0x3d/0x5b
> [<ffffffff81450e92>] system_call_fastpath+0x16/0x1b
> ---[ end trace 5232c09c4fb31d15 ]---
>
>
>
I found the bug in rt6_fill_node()
will send a patch in a couple of minutes
^ permalink raw reply
* Re: [STRAW MAN PATCH] sch_teql doesn't load-balance ppp(oatm) slaves
From: David Woodhouse @ 2012-03-27 19:10 UTC (permalink / raw)
To: David Miller, paulus; +Cc: netdev
In-Reply-To: <20120325.173635.1909319488008466320.davem@davemloft.net>
[-- Attachment #1: Type: text/plain, Size: 6940 bytes --]
On Sun, 2012-03-25 at 17:36 -0400, David Miller wrote:
> Yes, the ATM devices deep transmit queue is quite undesirable.
This should fix that, and while I'm at it should fix the gratuitous
running of ppp_output_wakeup() from a tasklet on *every* packet, when
it's almost never necessary. Some careful eyes over the locking issues
on that would be much appreciated. I've documented how I *think* it
works...
I'm tempted to rip out the atm_may_send() bit; there's not a lot of
point in checking against sk_sndbuf when we're limiting to two packets
anyway, is there? There's always been a problem here if sk_sndbuf was
set lower than the MTU of the interface; it would block for ever.
I'm running this now on my ADSL router. I can watch it working, keeping
precisely two packets in the queue at a time (one really in-flight and
one ready for the ATM driver). My leftover debugging in sch_teql is
triggering when the xmit returns NETDEV_TX_BUSY, and all seems to be
well.
--- net/atm/pppoatm.c~ 2012-03-27 19:59:54.379565896 +0100
+++ net/atm/pppoatm.c 2012-03-27 20:03:02.676561017 +0100
@@ -62,10 +62,13 @@ struct pppoatm_vcc {
void (*old_pop)(struct atm_vcc *, struct sk_buff *);
/* keep old push/pop for detaching */
enum pppoatm_encaps encaps;
+ atomic_t inflight;
+ unsigned long blocked;
int flags; /* SC_COMP_PROT - compress protocol */
struct ppp_channel chan; /* interface to generic ppp layer */
struct tasklet_struct wakeup_tasklet;
};
+#define BLOCKED 0
/*
* Header used for LLC Encapsulated PPP (4 bytes) followed by the LCP protocol
@@ -102,16 +105,31 @@ static void pppoatm_wakeup_sender(unsign
static void pppoatm_pop(struct atm_vcc *atmvcc, struct sk_buff *skb)
{
struct pppoatm_vcc *pvcc = atmvcc_to_pvcc(atmvcc);
+
pvcc->old_pop(atmvcc, skb);
+ smp_mb__before_atomic_dec();
+ atomic_dec(&pvcc->inflight);
+
/*
- * We don't really always want to do this since it's
- * really inefficient - it would be much better if we could
- * test if we had actually throttled the generic layer.
- * Unfortunately then there would be a nasty SMP race where
- * we could clear that flag just as we refuse another packet.
- * For now we do the safe thing.
+ * We always used to run the wakeup tasklet unconditionally here, for
+ * fear of race conditions where we clear the BLOCKED flag just as we
+ * refuse another packet in pppoatm_send(). This was quite inefficient.
+ *
+ * In fact it's OK. The PPP core will only ever call pppoatm_send()
+ * while holding the channel->downl lock. And ppp_output_wakeup() as
+ * called by the tasklet will *also* grab that lock. So even if another
+ * CPU is in pppoatm_send() right now, the tasklet isn't going to race
+ * with it. The wakeup *will* happen after the other CPU is safely out
+ * of pppoatm_send() again.
+ *
+ * So if the CPU in pppoatm_send() has already set the BLOCKED bit and
+ * it about to return, that's fine. We trigger a wakeup which will
+ * happen later. And if the CPU in pppoatm_send() *hasn't* set the
+ * BLOCKED bit yet, that's fine too because of the double check in
+ * pppoatm_may_send() which is commented there.
*/
- tasklet_schedule(&pvcc->wakeup_tasklet);
+ if (test_and_clear_bit(BLOCKED, &pvcc->blocked))
+ tasklet_schedule(&pvcc->wakeup_tasklet);
}
/*
@@ -184,6 +202,54 @@ error:
ppp_input_error(&pvcc->chan, 0);
}
+static inline int pppoatm_may_send(struct pppoatm_vcc *pvcc, int size)
+{
+ /*
+ * We allow two packets in the queue. The one that's currently
+ * in flight, and *one* queued up ready for the ATM device to
+ * send immediately from its TX done IRQ. More than that is
+ * unnecessary, since the PPP core is designed to feed us packets
+ * with extremely low latency anyway.
+ *
+ * It's not clear that we need to bother with using atm_may_send()
+ * to check we don't exceed sk->sk_sndbuf. If userspace sets a
+ * value of sk_sndbuf which is lower than the MTU, we're going to
+ * block for ever. But the code always did that before we introduced
+ * the packet count limit, so...
+ */
+ if (atm_may_send(pvcc->atmvcc, size) &&
+ atomic_inc_not_zero_hint(&pvcc->inflight, -2)) {
+ smp_mb__after_atomic_inc();
+ return 1;
+ }
+
+ smp_mb__before_clear_bit();
+ set_bit(BLOCKED, &pvcc->blocked);
+ smp_mb__after_clear_bit();
+ /*
+ * We may have raced with pppoatm_pop(). If it ran for the
+ * last packet in the queue, *just* before we set the BLOCKED
+ * bit, then it might never run again and the channel could
+ * remain permanently blocked. Cope with that race by checking
+ * *again*. If it did run in that window, we'll have space on
+ * the queue now and can return success. It's harmless to leave
+ * the BLOCKED flag set, since it's only used as a trigger to
+ * run the wakeup tasklet.
+ * If pppoatm_pop() is running but hasn't got as far as making
+ * space on the queue yet, then it hasn't checked the BLOCKED
+ * flag yet either, so we're safe in that case too. It'll issue
+ * an "immediate" wakeup... where "immediate" actually involves
+ * taking the PPP channel's ->downl lock, which is held by the
+ * code path that calls pppoatm_send(), and is thus going to
+ * wait for us to finish.
+ */
+ if (atm_may_send(pvcc->atmvcc, size) &&
+ atomic_inc_not_zero(&pvcc->inflight)) {
+ smp_mb__after_atomic_inc();
+ return 1;
+ }
+ return 0;
+}
/*
* Called by the ppp_generic.c to send a packet - returns true if packet
* was accepted. If we return false, then it's our job to call
@@ -207,7 +273,7 @@ static int pppoatm_send(struct ppp_chann
struct sk_buff *n;
n = skb_realloc_headroom(skb, LLC_LEN);
if (n != NULL &&
- !atm_may_send(pvcc->atmvcc, n->truesize)) {
+ !pppoatm_may_send(pvcc, n->truesize)) {
kfree_skb(n);
goto nospace;
}
@@ -215,12 +281,12 @@ static int pppoatm_send(struct ppp_chann
skb = n;
if (skb == NULL)
return DROP_PACKET;
- } else if (!atm_may_send(pvcc->atmvcc, skb->truesize))
+ } else if (!pppoatm_may_send(pvcc, skb->truesize))
goto nospace;
memcpy(skb_push(skb, LLC_LEN), pppllc, LLC_LEN);
break;
case e_vc:
- if (!atm_may_send(pvcc->atmvcc, skb->truesize))
+ if (!pppoatm_may_send(pvcc, skb->truesize))
goto nospace;
break;
case e_autodetect:
@@ -285,6 +351,9 @@ static int pppoatm_assign_vcc(struct atm
if (pvcc == NULL)
return -ENOMEM;
pvcc->atmvcc = atmvcc;
+
+ /* Maximum is zero, so that we can use atomic_inc_not_zero() */
+ atomic_set(&pvcc->inflight, -2);
pvcc->old_push = atmvcc->push;
pvcc->old_pop = atmvcc->pop;
pvcc->encaps = (enum pppoatm_encaps) be.encaps;
--
David Woodhouse Open Source Technology Centre
David.Woodhouse@intel.com Intel Corporation
[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5818 bytes --]
^ permalink raw reply
* [PATCH v2] net/garp: avoid infinite loop if attribute already exists
From: David Ward @ 2012-03-27 19:01 UTC (permalink / raw)
To: netdev; +Cc: jorge, kaber, David Ward
In-Reply-To: <4F70942B.5000102@ll.mit.edu>
An infinite loop occurred if garp_attr_create was called with the values
of an existing attribute. This might happen if a previous leave request
for the attribute has not yet been followed by a PDU transmission (or,
if the application previously issued a join request for the attribute
and is now issuing another one, without having issued a leave request).
If garp_attr_create finds an existing attribute having the same values,
return the address to it. Its state will then get updated (i.e., if it
was in a leaving state, it will move into a non-leaving state and not
get deleted during the next PDU transmission).
To accomplish this fix, collapse garp_attr_insert into garp_attr_create
(which is its only caller).
Signed-off-by: David Ward <david.ward@ll.mit.edu>
---
net/802/garp.c | 22 ++++++++++------------
1 files changed, 10 insertions(+), 12 deletions(-)
diff --git a/net/802/garp.c b/net/802/garp.c
index 8e21b6d..a5c2248 100644
--- a/net/802/garp.c
+++ b/net/802/garp.c
@@ -167,7 +167,8 @@ static struct garp_attr *garp_attr_lookup(const struct garp_applicant *app,
return NULL;
}
-static void garp_attr_insert(struct garp_applicant *app, struct garp_attr *new)
+static struct garp_attr *garp_attr_create(struct garp_applicant *app,
+ const void *data, u8 len, u8 type)
{
struct rb_node *parent = NULL, **p = &app->gid.rb_node;
struct garp_attr *attr;
@@ -176,21 +177,16 @@ static void garp_attr_insert(struct garp_applicant *app, struct garp_attr *new)
while (*p) {
parent = *p;
attr = rb_entry(parent, struct garp_attr, node);
- d = garp_attr_cmp(attr, new->data, new->dlen, new->type);
+ d = garp_attr_cmp(attr, data, len, type);
if (d < 0)
p = &parent->rb_left;
else if (d > 0)
p = &parent->rb_right;
+ else {
+ /* The attribute already exists; re-use it. */
+ return attr;
+ }
}
- rb_link_node(&new->node, parent, p);
- rb_insert_color(&new->node, &app->gid);
-}
-
-static struct garp_attr *garp_attr_create(struct garp_applicant *app,
- const void *data, u8 len, u8 type)
-{
- struct garp_attr *attr;
-
attr = kmalloc(sizeof(*attr) + len, GFP_ATOMIC);
if (!attr)
return attr;
@@ -198,7 +194,9 @@ static struct garp_attr *garp_attr_create(struct garp_applicant *app,
attr->type = type;
attr->dlen = len;
memcpy(attr->data, data, len);
- garp_attr_insert(app, attr);
+
+ rb_link_node(&attr->node, parent, p);
+ rb_insert_color(&attr->node, &app->gid);
return attr;
}
--
1.7.4.1
^ permalink raw reply related
* Re: [PATCH net V4 2/2] igb: offer a PTP Hardware Clock instead of the timecompare method
From: chetan loke @ 2012-03-27 18:51 UTC (permalink / raw)
To: Keller, Jacob E
Cc: e1000-devel@lists.sourceforge.net, netdev@vger.kernel.org,
Ronciak, John, john.stultz@linaro.org, tglx@linutronix.de
In-Reply-To: <CAAsGZS7+L5Y7Mo8pipJf=T6x9vTUAcTS93t=BGsmCUvA3T31Zw@mail.gmail.com>
On Tue, Mar 27, 2012 at 2:43 PM, chetan loke <loke.chetan@gmail.com> wrote:
> On Tue, Mar 27, 2012 at 2:29 PM, Keller, Jacob E
> <jacob.e.keller@intel.com> wrote:
>
>>>
>>
>> It isn't so much about performance gains as it is about preventing poorly written user apps from stalling the clean descriptor routines. I am working on a test case that should prove whether this is even an issue (at least with ixgbe). Once I have data on that it can be determined if the extra lock would alleviate it. The conceptual issue is that spamming get-time could cause the clean tx/rx irq routines to stall inside the interrupt for too long. (thereby not freeing up descriptors on the ring to
>
> What would be interesting to see is something like:
>
> 1) App1(getter/setter calls), executed '1000' ioctls per sec on eth0.
> Other 'M' apps(execute just get calls), where M = 1 .. 10(?). And one
> buggy app executed - 10K getter calls per sec.
>
> 2) eth0 receives PTP traffic and also around 80-90%(link rate)
> regular/mixed traffic.
>
> And BTW, eth0 is the management IP.
>
> While the test is running, if we are still able to reach the host and
> manage it via SSH then we can stop being paranoid.
>
>
Guys please correct me if I'm wrong but if RPS/RFS is enabled then at
a minimum we would need "N * 2" apps (where N == num_cpus). That way
there will be contention w/ the completions and we will be able to see
the real effect?
Chetan
------------------------------------------------------------------------------
This SF email is sponsosred by:
Try Windows Azure free for 90 days Click Here
http://p.sf.net/sfu/sfd2d-msazure
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
^ permalink raw reply
* pull request: wireless 2012-03-27
From: John W. Linville @ 2012-03-27 18:32 UTC (permalink / raw)
To: davem; +Cc: linux-wireless, netdev, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 15755 bytes --]
commit 643c61e119459e9d750087b7b34be94491efebf9
Dave,
Here is a little round-up of fixes intended for 3.4...
First, we have a MAINTAINERS update -- not truly a "fix", but it might
help to keep unwanted traffic out of Bob's inbox. We also have a
removal of an outdated comment that was overlooked when the related
code was removed.
Beyond those, we have a fix for a memory leak in ath9k, a cfg80211
fix to prevent passing bad info over nl80211 to userspace, a tuning
fix for some rtlwifi family drivers, a couple of minor ath9k tuning
fixes, a mac80211 use-after-free fix, a couple of rt2x00 thinko fixes,
an iwlegacy fix to properly set BSSID after an association, and an
iwlegacy fix to avoid an oops by properly reinitializing a variable
after reset.
Please let me know if there are problems!
Thanks,
John
---
The following changes since commit 8a78335442cea429afb2b964318b6e257448ea00:
usbnet: consider device busy at each recieved packet (2012-03-22 23:04:27 -0400)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/linville/wireless.git master
Bob Copeland (1):
ath5k: drop self from MAINTAINERS
Eliad Peller (1):
mac80211: remove outdated comment
Eric Dumazet (1):
ath9k: fix a memory leak in ath_rx_tasklet()
John W. Linville (1):
cfg80211: allow CFG80211_SIGNAL_TYPE_UNSPEC in station_info
Larry Finger (1):
rtlwifi: rtl8192ce: rtl8192cu: rtl8192de: Fix low-gain setting when scanning
Rajkumar Manoharan (2):
ath9k: fix max noise floor threshold
ath9k: reduce listen time period
Stanislaw Gruszka (5):
iwlegacy: do not nulify il->vif on reset
iwlegacy: fix BSSID setting
rt2x00: rt2800usb: schedule txdone work on timeout
rt2x00: rt2800usb: fix status register reread logic
mac80211: fix possible tid_rx->reorder_timer use after free
MAINTAINERS | 1 -
drivers/net/wireless/ath/ath9k/calib.c | 5 +--
drivers/net/wireless/ath/ath9k/init.c | 2 +-
drivers/net/wireless/ath/ath9k/recv.c | 4 +-
drivers/net/wireless/iwlegacy/3945-mac.c | 2 -
drivers/net/wireless/iwlegacy/4965-mac.c | 2 -
drivers/net/wireless/iwlegacy/common.c | 31 +++++++++++--------
drivers/net/wireless/rt2x00/rt2800usb.c | 10 +++++-
drivers/net/wireless/rtlwifi/rtl8192c/phy_common.c | 2 +-
drivers/net/wireless/rtlwifi/rtl8192de/phy.c | 2 +-
include/net/cfg80211.h | 6 ++-
net/mac80211/agg-rx.c | 3 +-
net/mac80211/main.c | 3 --
net/wireless/nl80211.c | 29 ++++++++++++------
14 files changed, 58 insertions(+), 44 deletions(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index 92f9924..0ddc77fe 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1247,7 +1247,6 @@ ATHEROS ATH5K WIRELESS DRIVER
M: Jiri Slaby <jirislaby@gmail.com>
M: Nick Kossifidis <mickflemm@gmail.com>
M: "Luis R. Rodriguez" <mcgrof@qca.qualcomm.com>
-M: Bob Copeland <me@bobcopeland.com>
L: linux-wireless@vger.kernel.org
L: ath5k-devel@lists.ath5k.org
W: http://wireless.kernel.org/en/users/Drivers/ath5k
diff --git a/drivers/net/wireless/ath/ath9k/calib.c b/drivers/net/wireless/ath/ath9k/calib.c
index 2f4b48e..e5cceb0 100644
--- a/drivers/net/wireless/ath/ath9k/calib.c
+++ b/drivers/net/wireless/ath/ath9k/calib.c
@@ -20,7 +20,6 @@
/* Common calibration code */
-#define ATH9K_NF_TOO_HIGH -60
static int16_t ath9k_hw_get_nf_hist_mid(int16_t *nfCalBuffer)
{
@@ -346,10 +345,10 @@ static void ath9k_hw_nf_sanitize(struct ath_hw *ah, s16 *nf)
"NF calibrated [%s] [chain %d] is %d\n",
(i >= 3 ? "ext" : "ctl"), i % 3, nf[i]);
- if (nf[i] > ATH9K_NF_TOO_HIGH) {
+ if (nf[i] > limit->max) {
ath_dbg(common, CALIBRATE,
"NF[%d] (%d) > MAX (%d), correcting to MAX\n",
- i, nf[i], ATH9K_NF_TOO_HIGH);
+ i, nf[i], limit->max);
nf[i] = limit->max;
} else if (nf[i] < limit->min) {
ath_dbg(common, CALIBRATE,
diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c
index 60159f4..cb00645 100644
--- a/drivers/net/wireless/ath/ath9k/init.c
+++ b/drivers/net/wireless/ath/ath9k/init.c
@@ -680,7 +680,7 @@ void ath9k_set_hw_capab(struct ath_softc *sc, struct ieee80211_hw *hw)
hw->queues = 4;
hw->max_rates = 4;
hw->channel_change_time = 5000;
- hw->max_listen_interval = 10;
+ hw->max_listen_interval = 1;
hw->max_rate_tries = 10;
hw->sta_data_size = sizeof(struct ath_node);
hw->vif_data_size = sizeof(struct ath_vif);
diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c
index f4ae3ba..1c4583c 100644
--- a/drivers/net/wireless/ath/ath9k/recv.c
+++ b/drivers/net/wireless/ath/ath9k/recv.c
@@ -1913,13 +1913,13 @@ int ath_rx_tasklet(struct ath_softc *sc, int flush, bool hp)
if (sc->rx.frag) {
int space = skb->len - skb_tailroom(hdr_skb);
- sc->rx.frag = NULL;
-
if (pskb_expand_head(hdr_skb, 0, space, GFP_ATOMIC) < 0) {
dev_kfree_skb(skb);
goto requeue_drop_frag;
}
+ sc->rx.frag = NULL;
+
skb_copy_from_linear_data(skb, skb_put(hdr_skb, skb->len),
skb->len);
dev_kfree_skb_any(skb);
diff --git a/drivers/net/wireless/iwlegacy/3945-mac.c b/drivers/net/wireless/iwlegacy/3945-mac.c
index 0c12093..faec404 100644
--- a/drivers/net/wireless/iwlegacy/3945-mac.c
+++ b/drivers/net/wireless/iwlegacy/3945-mac.c
@@ -2673,8 +2673,6 @@ il3945_bg_restart(struct work_struct *data)
if (test_and_clear_bit(S_FW_ERROR, &il->status)) {
mutex_lock(&il->mutex);
- /* FIXME: vif can be dereferenced */
- il->vif = NULL;
il->is_open = 0;
mutex_unlock(&il->mutex);
il3945_down(il);
diff --git a/drivers/net/wireless/iwlegacy/4965-mac.c b/drivers/net/wireless/iwlegacy/4965-mac.c
index 7b54dbb..b88bb27 100644
--- a/drivers/net/wireless/iwlegacy/4965-mac.c
+++ b/drivers/net/wireless/iwlegacy/4965-mac.c
@@ -5651,8 +5651,6 @@ il4965_bg_restart(struct work_struct *data)
if (test_and_clear_bit(S_FW_ERROR, &il->status)) {
mutex_lock(&il->mutex);
- /* FIXME: do we dereference vif without mutex locked ? */
- il->vif = NULL;
il->is_open = 0;
__il4965_down(il);
diff --git a/drivers/net/wireless/iwlegacy/common.c b/drivers/net/wireless/iwlegacy/common.c
index e5ac047..eaf24945 100644
--- a/drivers/net/wireless/iwlegacy/common.c
+++ b/drivers/net/wireless/iwlegacy/common.c
@@ -4508,6 +4508,7 @@ il_mac_add_interface(struct ieee80211_hw *hw, struct ieee80211_vif *vif)
{
struct il_priv *il = hw->priv;
int err;
+ bool reset;
mutex_lock(&il->mutex);
D_MAC80211("enter: type %d, addr %pM\n", vif->type, vif->addr);
@@ -4518,7 +4519,12 @@ il_mac_add_interface(struct ieee80211_hw *hw, struct ieee80211_vif *vif)
goto out;
}
- if (il->vif) {
+ /*
+ * We do not support multiple virtual interfaces, but on hardware reset
+ * we have to add the same interface again.
+ */
+ reset = (il->vif == vif);
+ if (il->vif && !reset) {
err = -EOPNOTSUPP;
goto out;
}
@@ -4528,8 +4534,11 @@ il_mac_add_interface(struct ieee80211_hw *hw, struct ieee80211_vif *vif)
err = il_set_mode(il);
if (err) {
- il->vif = NULL;
- il->iw_mode = NL80211_IFTYPE_STATION;
+ IL_WARN("Fail to set mode %d\n", vif->type);
+ if (!reset) {
+ il->vif = NULL;
+ il->iw_mode = NL80211_IFTYPE_STATION;
+ }
}
out:
@@ -5279,9 +5288,9 @@ il_mac_bss_info_changed(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
D_MAC80211("BSSID %pM\n", bss_conf->bssid);
/*
- * If there is currently a HW scan going on in the
- * background then we need to cancel it else the RXON
- * below/in post_associate will fail.
+ * If there is currently a HW scan going on in the background,
+ * then we need to cancel it, otherwise sometimes we are not
+ * able to authenticate (FIXME: why ?)
*/
if (il_scan_cancel_timeout(il, 100)) {
D_MAC80211("leave - scan abort failed\n");
@@ -5290,14 +5299,10 @@ il_mac_bss_info_changed(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
}
/* mac80211 only sets assoc when in STATION mode */
- if (vif->type == NL80211_IFTYPE_ADHOC || bss_conf->assoc) {
- memcpy(il->staging.bssid_addr, bss_conf->bssid,
- ETH_ALEN);
+ memcpy(il->staging.bssid_addr, bss_conf->bssid, ETH_ALEN);
- /* currently needed in a few places */
- memcpy(il->bssid, bss_conf->bssid, ETH_ALEN);
- } else
- il->staging.filter_flags &= ~RXON_FILTER_ASSOC_MSK;
+ /* FIXME: currently needed in a few places */
+ memcpy(il->bssid, bss_conf->bssid, ETH_ALEN);
}
/*
diff --git a/drivers/net/wireless/rt2x00/rt2800usb.c b/drivers/net/wireless/rt2x00/rt2800usb.c
index cd490ab..001735f 100644
--- a/drivers/net/wireless/rt2x00/rt2800usb.c
+++ b/drivers/net/wireless/rt2x00/rt2800usb.c
@@ -163,7 +163,13 @@ static bool rt2800usb_tx_sta_fifo_read_completed(struct rt2x00_dev *rt2x00dev,
/* Reschedule urb to read TX status again instantly */
return true;
- } else if (rt2800usb_txstatus_pending(rt2x00dev)) {
+ }
+
+ /* Check if there is any entry that timedout waiting on TX status */
+ if (rt2800usb_txstatus_timeout(rt2x00dev))
+ queue_work(rt2x00dev->workqueue, &rt2x00dev->txdone_work);
+
+ if (rt2800usb_txstatus_pending(rt2x00dev)) {
/* Read register after 250 us */
hrtimer_start(&rt2x00dev->txstatus_timer, ktime_set(0, 250000),
HRTIMER_MODE_REL);
@@ -178,7 +184,7 @@ stop_reading:
* here again if status reading is needed.
*/
if (rt2800usb_txstatus_pending(rt2x00dev) &&
- test_and_set_bit(TX_STATUS_READING, &rt2x00dev->flags))
+ !test_and_set_bit(TX_STATUS_READING, &rt2x00dev->flags))
return true;
else
return false;
diff --git a/drivers/net/wireless/rtlwifi/rtl8192c/phy_common.c b/drivers/net/wireless/rtlwifi/rtl8192c/phy_common.c
index 1eec3a0..4c01624 100644
--- a/drivers/net/wireless/rtlwifi/rtl8192c/phy_common.c
+++ b/drivers/net/wireless/rtlwifi/rtl8192c/phy_common.c
@@ -1893,7 +1893,7 @@ void rtl92c_phy_set_io(struct ieee80211_hw *hw)
break;
case IO_CMD_PAUSE_DM_BY_SCAN:
rtlphy->initgain_backup.xaagccore1 = dm_digtable.cur_igvalue;
- dm_digtable.cur_igvalue = 0x17;
+ dm_digtable.cur_igvalue = 0x37;
rtl92c_dm_write_dig(hw);
break;
default:
diff --git a/drivers/net/wireless/rtlwifi/rtl8192de/phy.c b/drivers/net/wireless/rtlwifi/rtl8192de/phy.c
index 34591eeb..28fc5fb 100644
--- a/drivers/net/wireless/rtlwifi/rtl8192de/phy.c
+++ b/drivers/net/wireless/rtlwifi/rtl8192de/phy.c
@@ -3077,7 +3077,7 @@ static void rtl92d_phy_set_io(struct ieee80211_hw *hw)
break;
case IO_CMD_PAUSE_DM_BY_SCAN:
rtlphy->initgain_backup.xaagccore1 = de_digtable.cur_igvalue;
- de_digtable.cur_igvalue = 0x17;
+ de_digtable.cur_igvalue = 0x37;
rtl92d_dm_write_dig(hw);
break;
default:
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 69b7ad3..5ccac72 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -619,8 +619,10 @@ struct sta_bss_parameters {
* @llid: mesh local link id
* @plid: mesh peer link id
* @plink_state: mesh peer link state
- * @signal: signal strength of last received packet in dBm
- * @signal_avg: signal strength average in dBm
+ * @signal: the signal strength, type depends on the wiphy's signal_type
+ NOTE: For CFG80211_SIGNAL_TYPE_MBM, value is expressed in _dBm_.
+ * @signal_avg: avg signal strength, type depends on the wiphy's signal_type
+ NOTE: For CFG80211_SIGNAL_TYPE_MBM, value is expressed in _dBm_.
* @txrate: current unicast bitrate from this station
* @rxrate: current unicast bitrate to this station
* @rx_packets: packets received from this station
diff --git a/net/mac80211/agg-rx.c b/net/mac80211/agg-rx.c
index 1068f66..64d3ce5 100644
--- a/net/mac80211/agg-rx.c
+++ b/net/mac80211/agg-rx.c
@@ -49,6 +49,8 @@ static void ieee80211_free_tid_rx(struct rcu_head *h)
container_of(h, struct tid_ampdu_rx, rcu_head);
int i;
+ del_timer_sync(&tid_rx->reorder_timer);
+
for (i = 0; i < tid_rx->buf_size; i++)
dev_kfree_skb(tid_rx->reorder_buf[i]);
kfree(tid_rx->reorder_buf);
@@ -91,7 +93,6 @@ void ___ieee80211_stop_rx_ba_session(struct sta_info *sta, u16 tid,
tid, WLAN_BACK_RECIPIENT, reason);
del_timer_sync(&tid_rx->session_timer);
- del_timer_sync(&tid_rx->reorder_timer);
call_rcu(&tid_rx->rcu_head, ieee80211_free_tid_rx);
}
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index b581a24..1633648 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -102,9 +102,6 @@ int ieee80211_hw_config(struct ieee80211_local *local, u32 changed)
might_sleep();
- /* If this off-channel logic ever changes, ieee80211_on_oper_channel
- * may need to change as well.
- */
offchannel_flag = local->hw.conf.flags & IEEE80211_CONF_OFFCHANNEL;
if (local->scan_channel) {
chan = local->scan_channel;
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 4c1eb94..e49da27 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -2386,7 +2386,9 @@ nla_put_failure:
}
static int nl80211_send_station(struct sk_buff *msg, u32 pid, u32 seq,
- int flags, struct net_device *dev,
+ int flags,
+ struct cfg80211_registered_device *rdev,
+ struct net_device *dev,
const u8 *mac_addr, struct station_info *sinfo)
{
void *hdr;
@@ -2425,12 +2427,18 @@ static int nl80211_send_station(struct sk_buff *msg, u32 pid, u32 seq,
if (sinfo->filled & STATION_INFO_PLINK_STATE)
NLA_PUT_U8(msg, NL80211_STA_INFO_PLINK_STATE,
sinfo->plink_state);
- if (sinfo->filled & STATION_INFO_SIGNAL)
- NLA_PUT_U8(msg, NL80211_STA_INFO_SIGNAL,
- sinfo->signal);
- if (sinfo->filled & STATION_INFO_SIGNAL_AVG)
- NLA_PUT_U8(msg, NL80211_STA_INFO_SIGNAL_AVG,
- sinfo->signal_avg);
+ switch (rdev->wiphy.signal_type) {
+ case CFG80211_SIGNAL_TYPE_MBM:
+ if (sinfo->filled & STATION_INFO_SIGNAL)
+ NLA_PUT_U8(msg, NL80211_STA_INFO_SIGNAL,
+ sinfo->signal);
+ if (sinfo->filled & STATION_INFO_SIGNAL_AVG)
+ NLA_PUT_U8(msg, NL80211_STA_INFO_SIGNAL_AVG,
+ sinfo->signal_avg);
+ break;
+ default:
+ break;
+ }
if (sinfo->filled & STATION_INFO_TX_BITRATE) {
if (!nl80211_put_sta_rate(msg, &sinfo->txrate,
NL80211_STA_INFO_TX_BITRATE))
@@ -2523,7 +2531,7 @@ static int nl80211_dump_station(struct sk_buff *skb,
if (nl80211_send_station(skb,
NETLINK_CB(cb->skb).pid,
cb->nlh->nlmsg_seq, NLM_F_MULTI,
- netdev, mac_addr,
+ dev, netdev, mac_addr,
&sinfo) < 0)
goto out;
@@ -2568,7 +2576,7 @@ static int nl80211_get_station(struct sk_buff *skb, struct genl_info *info)
return -ENOMEM;
if (nl80211_send_station(msg, info->snd_pid, info->snd_seq, 0,
- dev, mac_addr, &sinfo) < 0) {
+ rdev, dev, mac_addr, &sinfo) < 0) {
nlmsg_free(msg);
return -ENOBUFS;
}
@@ -7596,7 +7604,8 @@ void nl80211_send_sta_event(struct cfg80211_registered_device *rdev,
if (!msg)
return;
- if (nl80211_send_station(msg, 0, 0, 0, dev, mac_addr, sinfo) < 0) {
+ if (nl80211_send_station(msg, 0, 0, 0,
+ rdev, dev, mac_addr, sinfo) < 0) {
nlmsg_free(msg);
return;
}
--
John W. Linville Someday the world will need a hero, and you
linville@tuxdriver.com might be all we have. Be ready.
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply related
* Re: [PATCH net V4 2/2] igb: offer a PTP Hardware Clock instead of the timecompare method
From: chetan loke @ 2012-03-27 18:43 UTC (permalink / raw)
To: Keller, Jacob E
Cc: e1000-devel@lists.sourceforge.net, netdev@vger.kernel.org,
Ronciak, John, john.stultz@linaro.org, tglx@linutronix.de
In-Reply-To: <02874ECE860811409154E81DA85FBB580DC68D@ORSMSX105.amr.corp.intel.com>
On Tue, Mar 27, 2012 at 2:29 PM, Keller, Jacob E
<jacob.e.keller@intel.com> wrote:
>>
>
> It isn't so much about performance gains as it is about preventing poorly written user apps from stalling the clean descriptor routines. I am working on a test case that should prove whether this is even an issue (at least with ixgbe). Once I have data on that it can be determined if the extra lock would alleviate it. The conceptual issue is that spamming get-time could cause the clean tx/rx irq routines to stall inside the interrupt for too long. (thereby not freeing up descriptors on the ring to
What would be interesting to see is something like:
1) App1(getter/setter calls), executed '1000' ioctls per sec on eth0.
Other 'M' apps(execute just get calls), where M = 1 .. 10(?). And one
buggy app executed - 10K getter calls per sec.
2) eth0 receives PTP traffic and also around 80-90%(link rate)
regular/mixed traffic.
And BTW, eth0 is the management IP.
While the test is running, if we are still able to reach the host and
manage it via SSH then we can stop being paranoid.
Chetan
------------------------------------------------------------------------------
This SF email is sponsosred by:
Try Windows Azure free for 90 days Click Here
http://p.sf.net/sfu/sfd2d-msazure
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
^ permalink raw reply
* Re: [PATCH net V4 2/2] igb: offer a PTP Hardware Clock instead of the timecompare method
From: chetan loke @ 2012-03-27 18:39 UTC (permalink / raw)
To: Richard Cochran
Cc: e1000-devel@lists.sourceforge.net, netdev@vger.kernel.org,
Ronciak, John, john.stultz@linaro.org, Keller, Jacob E,
tglx@linutronix.de
In-Reply-To: <20120327153345.GA13948@localhost.localdomain>
On Tue, Mar 27, 2012 at 11:33 AM, Richard Cochran
<richardcochran@gmail.com> wrote:
> On Mon, Mar 26, 2012 at 01:11:32PM -0400, chetan loke wrote:
>>
>> Why isn't ioctl-rate limiting acceptable?
>
> Let me turn the question around.
>
> What other kernel subsystem rate limits ioctls?
>
netdev needs to worry about reaching the host via ethernet. If you
lose SCSI LUNs because you are pounding those mailbox cmds to get that
temperature status from the HBA then who the heck cares? I mean we do
but we can reach the host via it's mgmt-IP and look at it. From what I
can see in SCSI for example, control commands are normally issued in
separate paths by the driver and don't choke the fast path. Those
completions are asynchronous like everything else and do NOT contend
with the Rx/Tx path.
May be I'm paranoid but let's wait for Intel(Jake) to run the ioctl test.
Chetan
------------------------------------------------------------------------------
This SF email is sponsosred by:
Try Windows Azure free for 90 days Click Here
http://p.sf.net/sfu/sfd2d-msazure
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
^ permalink raw reply
* Re: [PATCH net V4 2/2] igb: offer a PTP Hardware Clock instead of the timecompare method
From: Richard Cochran @ 2012-03-27 18:33 UTC (permalink / raw)
To: Keller, Jacob E
Cc: chetan loke, netdev@vger.kernel.org,
e1000-devel@lists.sourceforge.net, Kirsher, Jeffrey T,
Ronciak, John, john.stultz@linaro.org, tglx@linutronix.de
In-Reply-To: <02874ECE860811409154E81DA85FBB580DC1C2@ORSMSX105.amr.corp.intel.com>
On Mon, Mar 26, 2012 at 08:51:28PM +0000, Keller, Jacob E wrote:
>
> This is about a case where the user does something stupid. (runs the ioctls too
> fast). It appears that your answer is something like "The user did something
> stupid, we shouldn't care". My answer is, "we should do what we can to prevent
> this."
No matter how much you optimize your driver (or library or whatever),
there is always some user load that will kill your performance.
The locking is need for data integrity. The code path in the lock is
short. I doubt there is any serious performance issue here.
Richard
^ permalink raw reply
* RE: [PATCH net V4 2/2] igb: offer a PTP Hardware Clock instead of the timecompare method
From: Keller, Jacob E @ 2012-03-27 18:29 UTC (permalink / raw)
To: Richard Cochran
Cc: chetan loke, netdev@vger.kernel.org,
e1000-devel@lists.sourceforge.net, Kirsher, Jeffrey T,
Ronciak, John, john.stultz@linaro.org, tglx@linutronix.de
In-Reply-To: <20120327180501.GA3878@netboy.at.omicron.at>
> -----Original Message-----
> From: Richard Cochran [mailto:richardcochran@gmail.com]
> Sent: Tuesday, March 27, 2012 11:05 AM
> To: Keller, Jacob E
> Cc: chetan loke; netdev@vger.kernel.org; e1000-devel@lists.sourceforge.net;
> Kirsher, Jeffrey T; Ronciak, John; john.stultz@linaro.org; tglx@linutronix.de
> Subject: Re: [PATCH net V4 2/2] igb: offer a PTP Hardware Clock instead of the
> timecompare method
>
> I really doubt you will see any performance gain from such a change. It
> increases code complexity and size for some dubious, theoretical performance
> gain, for some really whacked use case.
>
> Richard
>
>
It isn't so much about performance gains as it is about preventing poorly written user apps from stalling the clean descriptor routines. I am working on a test case that should prove whether this is even an issue (at least with ixgbe). Once I have data on that it can be determined if the extra lock would alleviate it. The conceptual issue is that spamming get-time could cause the clean tx/rx irq routines to stall inside the interrupt for too long. (thereby not freeing up descriptors on the ring to allow more packets). While performance is an issue, I don't feel that it can be brushed off as "performance loss due to feature" unless I can show that it isn't very easy to trip, or doesn't cause a major issue. Once I know the data, it can be determined what the right approach is.
^ permalink raw reply
* Re: RCU lock bug in 3.0.21 (bisected to: 682cb56a, fix NULL dereferences in check_peer_redir)
From: Eric Dumazet @ 2012-03-27 18:06 UTC (permalink / raw)
To: Ben Greear; +Cc: David Miller, netdev, gregkh, Paul E. McKenney
In-Reply-To: <4F71EF2A.8020507@candelatech.com>
On Tue, 2012-03-27 at 09:47 -0700, Ben Greear wrote:
> On 03/26/2012 04:39 PM, Eric Dumazet wrote:
> > On Mon, 2012-03-26 at 16:06 -0700, Ben Greear wrote:
> >> On 03/26/2012 02:53 PM, Ben Greear wrote:
> >>> On 03/26/2012 02:49 PM, David Miller wrote:
> >>>>
> >>>> Looks like all of those strange undiagnosable reported Dave Jones
> >>>> has been feeding us. Something in one part of the kernel leaves
> >>>> a lock held, and this shows up as a warning elsewhere.
> >>>
> >>> Every (initial) bug printout fingers ipv6 and the 'ip' tool on my system.
> >>
> >> I added a patch to convert rcu_read_lock/unlock to macros so
> >> that I could automatically grab the call site (_THIS_IP_)
> >> and pass it into the lockdep framework instead of the (useless)
> >> _THIS_IP_ in the old rcu_read_lock method which at best seems to
> >> only indicate which module the issue relates to...
> >
> > Hi Ben
> >
> > Is this problem also appears with current tree ?
> > (This could be a problem with the backport, as it was full of
> > dependencies)
> >
> > Also, if you use a patch to better track rcu_read_lock()/unlock(), you
> > could add new macros as well to track that a particular unlock() matches
> > one given lock(). (maybe returning the rcu_preempt_depth at
> > rcu_read_lock() time , but maybe a more absolute ref would be better)
> >
> > So we could have a warning if an unlock() doesnt match the lock()
> >
> > inet6_dump_fib () was already a suspect but we could not find why.
>
>
> Ok, I tried the patch below, and got the result farther down. Is this
> what you were thinking of? (The lockdep warning about rcu lock still
> held happened immediately after this..so it appears the depth mis-match
> does represent this problem...
>
>
> [greearb@fs3 linux-3.0.dev.y]$ git diff
> diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
> index 0f9b37a..ae3c7c9 100644
> --- a/net/ipv6/ip6_fib.c
> +++ b/net/ipv6/ip6_fib.c
> @@ -366,6 +366,7 @@ static int inet6_dump_fib(struct sk_buff *skb, struct netlink_callback *cb)
> struct hlist_node *node;
> struct hlist_head *head;
> int res = 0;
> + int depth = current->lockdep_depth;
>
> s_h = cb->args[0];
> s_e = cb->args[1];
> @@ -410,6 +411,8 @@ next:
> }
> out:
> rcu_read_unlock();
> + WARN(depth != current->lockdep_depth, "depth: %i lockdep-depth: %i\n",
> + depth, current->lockdep_depth);
> cb->args[1] = e;
> cb->args[0] = h;
>
>
>
> ------------[ cut here ]------------
> WARNING: at /home/greearb/git/linux-3.0.dev.y/net/ipv6/ip6_fib.c:415 inet6_dump_fib+0x25c/0x292 [ipv6]()
> Hardware name: To be filled by O.E.M.
> depth: 1 lockdep-depth: 2
> Modules linked in: 8021q garp stp llc fuse macvlan pktgen coretemp hwmon sunrpc ipv6 uinput arc4 ath9k snd_hda_codec_realtek mac80211 snd_hda_intel
> snd_hda_codec snd_hwdep snd_seq ath9k_common ath9k_hw snd_seq_device snd_pcm ath snd_timer e1000e cfg80211 snd mei(C) ppdev microcode i2c_i801 iTCO_wdt
> soundcore serio_raw pcspkr snd_page_alloc iTCO_vendor_support parport_pc parport i915 drm_kms_helper drm i2c_algo_bit i2c_core video [last unloaded: scsi_wait_scan]
> Pid: 6563, comm: ip Tainted: G C 3.0.25+ #16
> Call Trace:
> [<ffffffff81046866>] warn_slowpath_common+0x80/0x98
> [<ffffffff81046912>] warn_slowpath_fmt+0x41/0x43
> [<ffffffffa0251a3a>] inet6_dump_fib+0x25c/0x292 [ipv6]
> [<ffffffff813af450>] netlink_dump+0x5b/0x19b
> [<ffffffff81385da2>] ? consume_skb+0x28/0x2a
> [<ffffffff813af7bf>] netlink_recvmsg+0x1c7/0x2f8
> [<ffffffff8137c6cf>] __sock_recvmsg_nosec+0x65/0x6e
> [<ffffffff8137dde0>] __sock_recvmsg+0x49/0x54
> [<ffffffff8137e349>] sock_recvmsg+0xa6/0xbf
> [<ffffffff81072bf8>] ? lock_release_non_nested+0x9d/0x227
> [<ffffffff810ca002>] ? might_fault+0x4e/0x9e
> [<ffffffff810ca04b>] ? might_fault+0x97/0x9e
> [<ffffffff81387cae>] ? copy_from_user+0x2a/0x2c
> [<ffffffff810ca002>] ? might_fault+0x4e/0x9e
> [<ffffffff81388080>] ? verify_iovec+0x4f/0xa3
> [<ffffffff8137e0c4>] __sys_recvmsg+0x147/0x21e
> [<ffffffff81063868>] ? up_read+0x1e/0x36
> [<ffffffff810fc9fb>] ? fcheck_files+0xb7/0xee
> [<ffffffff810fcb30>] ? fget_light+0x3b/0xbc
> [<ffffffff8137e8a0>] sys_recvmsg+0x3d/0x5b
> [<ffffffff81450e92>] system_call_fastpath+0x16/0x1b
> ---[ end trace 5232c09c4fb31d15 ]---
>
>
>
Now you could trace all rcu_read_lock()/rcu_read_unlock() done in this
context.
adding a current->rcu_trace_enabled flag, that you set/unset only in
inet6_dump_fib()
^ permalink raw reply
* Re: [PATCH net] bnx2x: fix vector traveling while looking for an empty entry
From: Maciej Żenczykowski @ 2012-03-27 18:06 UTC (permalink / raw)
To: Dmitry Kravkov; +Cc: davem, netdev, eilong
In-Reply-To: <1332832135-22054-1-git-send-email-dmitry@broadcom.com>
Ack.
On Tue, Mar 27, 2012 at 00:08, Dmitry Kravkov <dmitry@broadcom.com> wrote:
> Fixes the bug that may prevent from mac to be configured,
> while there is an empty slot for it.
>
> Reported-by: Maciej Żenczykowski <zenczykowski@gmail.com>
> Signed-off-by: Dmitry Kravkov <dmitry@broadcom.com>
> ---
> drivers/net/ethernet/broadcom/bnx2x/bnx2x_sp.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sp.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sp.c
> index 3f52fad..5135733 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sp.c
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sp.c
> @@ -3847,7 +3847,7 @@ static bool bnx2x_credit_pool_get_entry(
> continue;
>
> /* If we've got here we are going to find a free entry */
> - for (idx = vec * BNX2X_POOL_VEC_SIZE, i = 0;
> + for (idx = vec * BIT_VEC64_ELEM_SZ, i = 0;
> i < BIT_VEC64_ELEM_SZ; idx++, i++)
>
> if (BIT_VEC64_TEST_BIT(o->pool_mirror, idx)) {
> --
> 1.7.7.2
>
>
^ permalink raw reply
* Re: [PATCH net V4 2/2] igb: offer a PTP Hardware Clock instead of the timecompare method
From: Richard Cochran @ 2012-03-27 18:05 UTC (permalink / raw)
To: Keller, Jacob E
Cc: chetan loke, netdev@vger.kernel.org,
e1000-devel@lists.sourceforge.net, Kirsher, Jeffrey T,
Ronciak, John, john.stultz@linaro.org, tglx@linutronix.de
In-Reply-To: <02874ECE860811409154E81DA85FBB580DC1A0@ORSMSX105.amr.corp.intel.com>
Jacob,
Thanks for your detailed explanation...
On Mon, Mar 26, 2012 at 08:46:13PM +0000, Keller, Jacob E wrote:
>
> The issue with gettime is in the SYSTIM registers. Gettime could also
> use cyc2time, and allow the background worktask to be the only one updating
> the timecounter's last read value. (this only has to happen at least twice
> every wraparound value, so for the 10gig 82599 about once every 30 seconds).
> But the lock is also protecting against corruption of the SYSTIM register
> reads. (The lock does 2 different things. We could add a separate lock,
> but I am not convinced it is worth it).
Right, the spinlock in my patch protects both the SYSTIM registers and
the struct timecounter. For the SYSTIM pair, you must use a
spinlock. There is no way around it.
The struct timecounter fields .cycle_last and .nsec are used as follows
WRITE: timercoutner_read()
timercoutner_init()
READ: timercoutner_cyc2time()
Here you could conceivably use a reader/writer semaphore or a
seqlock. That would in turn enlarge the data structure and would
require nesting the locks.
I really doubt you will see any performance gain from such a
change. It increases code complexity and size for some dubious,
theoretical performance gain, for some really whacked use case.
Richard
^ permalink raw reply
* [RFC PATCH 2/2] net: ethtool: Add capability to retrieve plug-in module EEPROM
From: Stuart Hodgson @ 2012-03-27 17:51 UTC (permalink / raw)
To: netdev; +Cc: davem, linux-kernel, Ben Hutchings
Implementation in sfc driver to return the plugin module eeprom
Currently allows for SFP+ eeprom to be returned using the ethtool API.
This can be extended in future to handle different eeprom formats
and sizes.
Signed-off-by: Stuart Hodgson <smhodgson@solarflare.com>
---
drivers/net/ethernet/sfc/ethtool.c | 36 +++++++++++
drivers/net/ethernet/sfc/mcdi_phy.c | 105
+++++++++++++++++++++++++++++++++
drivers/net/ethernet/sfc/net_driver.h | 5 ++
3 files changed, 146 insertions(+), 0 deletions(-)
diff --git a/drivers/net/ethernet/sfc/ethtool.c
b/drivers/net/ethernet/sfc/ethtool.c
index f22f45f..e77895f 100644
--- a/drivers/net/ethernet/sfc/ethtool.c
+++ b/drivers/net/ethernet/sfc/ethtool.c
@@ -1108,6 +1108,40 @@ static int efx_ethtool_set_rxfh_indir(struct
net_device *net_dev,
return 0;
}
+static int efx_ethtool_get_module_eeprom(struct net_device *net_dev,
+ struct ethtool_eeprom *ee,
+ u8 *data)
+{
+ struct efx_nic *efx = netdev_priv(net_dev);
+ int ret;
+
+ if (!efx->phy_op ||
+ !efx->phy_op->get_module_eeprom)
+ return -EOPNOTSUPP;
+
+ mutex_lock(&efx->mac_lock);
+ ret = efx->phy_op->get_module_eeprom(efx, ee, data);
+ mutex_unlock(&efx->mac_lock);
+
+ return ret;
+}
+
+static int efx_ethtool_get_module_info(struct net_device *net_dev,
+ struct ethtool_modinfo *modinfo)
+{
+ struct efx_nic *efx = netdev_priv(net_dev);
+ int ret;
+
+ if (!efx->phy_op || !efx->phy_op->get_module_info)
+ return 0;
+
+ mutex_lock(&efx->mac_lock);
+ ret = efx->phy_op->get_module_info(efx, modinfo);
+ mutex_unlock(&efx->mac_lock);
+
+ return ret;
+}
+
const struct ethtool_ops efx_ethtool_ops = {
.get_settings = efx_ethtool_get_settings,
.set_settings = efx_ethtool_set_settings,
@@ -1137,4 +1171,6 @@ const struct ethtool_ops efx_ethtool_ops = {
.get_rxfh_indir_size = efx_ethtool_get_rxfh_indir_size,
.get_rxfh_indir = efx_ethtool_get_rxfh_indir,
.set_rxfh_indir = efx_ethtool_set_rxfh_indir,
+ .get_module_info = efx_ethtool_get_module_info,
+ .get_module_eeprom = efx_ethtool_get_module_eeprom,
};
diff --git a/drivers/net/ethernet/sfc/mcdi_phy.c
b/drivers/net/ethernet/sfc/mcdi_phy.c
index 7bcad89..be8e372 100644
--- a/drivers/net/ethernet/sfc/mcdi_phy.c
+++ b/drivers/net/ethernet/sfc/mcdi_phy.c
@@ -304,6 +304,17 @@ static u32 mcdi_to_ethtool_media(u32 media)
}
}
+static u32 mcdi_to_module_eeprom_len(u32 media)
+{
+ switch (media) {
+ case MC_CMD_MEDIA_SFP_PLUS:
+ return SFF_8079_LEN;
+ case MC_CMD_MEDIA_XFP:
+ default:
+ return 0;
+ }
+}
+
static int efx_mcdi_phy_probe(struct efx_nic *efx)
{
struct efx_mcdi_phy_data *phy_data;
@@ -739,6 +750,98 @@ static const char *efx_mcdi_phy_test_name(struct
efx_nic *efx,
return NULL;
}
+#define SFP_PAGE_SIZE 128
+#define NUM_PAGES 2
+#define OFF_TO_BUFF(x) (x + MC_CMD_GET_PHY_MEDIA_INFO_OUT_DATA_OFST)
+static int efx_mcdi_phy_get_module_eeprom(struct efx_nic *efx,
+ struct ethtool_eeprom *ee, u8 *data)
+{
+ u8 outbuf[MC_CMD_GET_PHY_MEDIA_INFO_OUT_LENMAX];
+ u8 inbuf[MC_CMD_GET_PHY_MEDIA_INFO_IN_LEN];
+ size_t outlen;
+ int rc;
+ int payload_len;
+ int copied = 0;
+ int space_remaining = ee->len;
+ int page;
+ int page_off;
+ int to_copy;
+ u8 *user_data = data;
+
+ if (!data || !ee)
+ return -EINVAL;
+
+ if (ee->offset > (SFP_PAGE_SIZE * NUM_PAGES)) {
+ rc = -EINVAL;
+ goto fail;
+ }
+
+ page_off = (ee->offset % SFP_PAGE_SIZE);
+ page = (ee->offset > SFP_PAGE_SIZE) ? 1 : 0;
+
+ while (space_remaining && (page < NUM_PAGES)) {
+
+ MCDI_SET_DWORD(inbuf, GET_PHY_MEDIA_INFO_IN_PAGE, page);
+
+ rc = efx_mcdi_rpc(efx, MC_CMD_GET_PHY_MEDIA_INFO,
+ inbuf, sizeof(inbuf),
+ outbuf, sizeof(outbuf),
+ &outlen);
+
+ if (rc)
+ goto fail;
+
+ /* Copy as much as we can into data */
+ if (outlen < MC_CMD_GET_PHY_MEDIA_INFO_OUT_LENMIN ||
+ outlen > MC_CMD_GET_PHY_MEDIA_INFO_OUT_LENMAX) {
+ rc = -EIO;
+ goto fail;
+ }
+
+ payload_len = MCDI_DWORD(outbuf,
+ GET_PHY_MEDIA_INFO_OUT_DATALEN);
+
+ to_copy = (space_remaining < payload_len) ?
+ space_remaining : payload_len;
+
+ to_copy -= page_off;
+
+ memcpy(user_data,
+ (outbuf + OFF_TO_BUFF(page_off)),
+ to_copy);
+
+ space_remaining -= to_copy;
+ user_data += to_copy;
+ copied += to_copy;
+ page_off = 0;
+ page++;
+ }
+
+ ee->len = copied;
+
+ return 0;
+fail:
+ return rc;
+}
+
+static int efx_mcdi_phy_get_module_info(struct efx_nic *efx,
+ struct ethtool_modinfo *modinfo)
+{
+ /* This will return a length of the eeprom
+ * type of the module that was detected during the probe,
+ * if not modules inserted then phy_data will be NULL */
+ struct efx_mcdi_phy_data *phy_cfg;
+
+ if (!efx || !efx->phy_data)
+ return -EOPNOTSUPP;
+
+ phy_cfg = efx->phy_data;
+ modinfo->eeprom_len = mcdi_to_module_eeprom_len(phy_cfg->media);
+ modinfo->type = SFF_8079;
+
+ return 0;
+}
+
const struct efx_phy_operations efx_mcdi_phy_ops = {
.probe = efx_mcdi_phy_probe,
.init = efx_port_dummy_op_int,
@@ -751,4 +854,6 @@ const struct efx_phy_operations efx_mcdi_phy_ops = {
.test_alive = efx_mcdi_phy_test_alive,
.run_tests = efx_mcdi_phy_run_tests,
.test_name = efx_mcdi_phy_test_name,
+ .get_module_eeprom = efx_mcdi_phy_get_module_eeprom,
+ .get_module_info = efx_mcdi_phy_get_module_info,
};
diff --git a/drivers/net/ethernet/sfc/net_driver.h
b/drivers/net/ethernet/sfc/net_driver.h
index 0b95505..245fc42 100644
--- a/drivers/net/ethernet/sfc/net_driver.h
+++ b/drivers/net/ethernet/sfc/net_driver.h
@@ -524,6 +524,11 @@ struct efx_phy_operations {
int (*test_alive) (struct efx_nic *efx);
const char *(*test_name) (struct efx_nic *efx, unsigned int index);
int (*run_tests) (struct efx_nic *efx, int *results, unsigned flags);
+ int (*get_module_eeprom) (struct efx_nic *efx,
+ struct ethtool_eeprom *ee,
+ u8 *data);
+ int (*get_module_info) (struct efx_nic *efx,
+ struct ethtool_modinfo *modinfo);
};
/**
--
1.7.7.6
^ permalink raw reply related
* [RFC PATCH 1/2] net: ethtool: Add capability to retrieve plug-in module EEPROM
From: Stuart Hodgson @ 2012-03-27 17:51 UTC (permalink / raw)
To: netdev
Cc: Ben Hutchings, bruce.w.allan, mirq-linux, decot, amit.salecha,
alexander.h.duyck, Ben Hutchings, davem, linux-kernel
Added extensions to the ethtool API to obtain plugin module eeprom data
This is useful for end users to be able to determine what the capabilities
of the in use module are.
The first provides a new struct ethtool_modinfo that will return the
type and size of plug-in module eeprom (such as SFP+) for parsing
by userland program.
The second provides the API to get the raw eeprom information
using the existing ethtool_eeprom structture to return the data
Signed-off-by: Stuart Hodgson <smhodgson@solarflare.com>
---
include/linux/ethtool.h | 20 ++++++++++++
net/core/ethtool.c | 79
+++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 99 insertions(+), 0 deletions(-)
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index da5b2de..50eaf35 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -117,6 +117,14 @@ struct ethtool_eeprom {
__u8 data[0];
};
+/* for passing plug-in module information */
+struct ethtool_modinfo {
+ __u32 cmd;
+ __u32 type;
+ __u32 eeprom_len;
+ __u32 reserved[8];
+};
+
/**
* struct ethtool_coalesce - coalescing parameters for IRQs and stats
updates
* @cmd: ETHTOOL_{G,S}COALESCE
@@ -936,6 +944,10 @@ struct ethtool_ops {
int (*get_dump_data)(struct net_device *,
struct ethtool_dump *, void *);
int (*set_dump)(struct net_device *, struct ethtool_dump *);
+ int (*get_module_info)(struct net_device *,
+ struct ethtool_modinfo *);
+ int (*get_module_eeprom)(struct net_device *,
+ struct ethtool_eeprom *, u8 *);
};
#endif /* __KERNEL__ */
@@ -1010,6 +1022,8 @@ struct ethtool_ops {
#define ETHTOOL_SET_DUMP 0x0000003e /* Set dump settings */
#define ETHTOOL_GET_DUMP_FLAG 0x0000003f /* Get dump settings */
#define ETHTOOL_GET_DUMP_DATA 0x00000040 /* Get dump data */
+#define ETHTOOL_GMODULEINFO 0x00000041 /* Get plug-in module
information */
+#define ETHTOOL_GMODULEEEPROM 0x00000042 /* Get plug-in module eeprom */
/* compatibility with older code */
#define SPARC_ETH_GSET ETHTOOL_GSET
@@ -1159,6 +1173,12 @@ struct ethtool_ops {
#define RX_CLS_LOC_FIRST 0xfffffffe
#define RX_CLS_LOC_LAST 0xfffffffd
+/* EEPROM Standards for plug in modules */
+#define SFF_8079 0x1
+#define SFF_8079_LEN 256
+#define SFF_8472 0x2
+#define SFF_8472_LEN 512
+
/* Reset flags */
/* The reset() operation must clear the flags for the components which
* were actually reset. On successful return, the flags indicate the
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 3f79db1..1e86bd9 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -1276,6 +1276,79 @@ out:
return ret;
}
+static int ethtool_get_module_info(struct net_device *dev,
+ void __user *useraddr)
+{
+ int ret;
+ struct ethtool_modinfo modinfo;
+ const struct ethtool_ops *ops = dev->ethtool_ops;
+
+ if (!ops->get_module_info)
+ return -EOPNOTSUPP;
+
+ if (copy_from_user(&modinfo, useraddr, sizeof(modinfo)))
+ return -EFAULT;
+
+ ret = ops->get_module_info(dev, &modinfo);
+ if (ret)
+ return ret;
+
+ if (copy_to_user(useraddr, &modinfo, sizeof(modinfo)))
+ return -EFAULT;
+
+ return 0;
+}
+
+static int ethtool_get_module_eeprom(struct net_device *dev,
+ void __user *useraddr)
+{
+ int ret;
+ struct ethtool_eeprom eeprom;
+ struct ethtool_modinfo modinfo;
+ const struct ethtool_ops *ops = dev->ethtool_ops;
+ void __user *userbuf = useraddr + sizeof(eeprom);
+ u8 *data;
+
+ if (!ops->get_module_info || !ops->get_module_eeprom)
+ return -EOPNOTSUPP;
+
+ if (copy_from_user(&eeprom, useraddr, sizeof(eeprom)))
+ return -EFAULT;
+
+ /* Check for wrap and zero */
+ if (eeprom.offset + eeprom.len <= eeprom.offset)
+ return -EINVAL;
+
+ /* Get the modinfo to get the length */
+ ret = ops->get_module_info(dev, &modinfo);
+ if (ret)
+ return ret;
+
+ if (eeprom.offset + eeprom.len > modinfo.eeprom_len)
+ return -EINVAL;
+
+ data = kmalloc(PAGE_SIZE, GFP_USER);
+ if (!data)
+ return -ENOMEM;
+
+ ret = ops->get_module_eeprom(dev, &eeprom, data);
+ if (ret)
+ goto out;
+
+
+ if (copy_to_user(userbuf, data, eeprom.len)) {
+ ret = -EFAULT;
+ goto out;
+ }
+
+ if (copy_to_user(useraddr, &eeprom, sizeof(eeprom)))
+ ret = -EFAULT;
+
+out:
+ kfree(data);
+ return ret;
+}
+
/* The main entry point in this file. Called from net/core/dev.c */
int dev_ethtool(struct net *net, struct ifreq *ifr)
@@ -1494,6 +1567,12 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
case ETHTOOL_GET_DUMP_DATA:
rc = ethtool_get_dump_data(dev, useraddr);
break;
+ case ETHTOOL_GMODULEINFO:
+ rc = ethtool_get_module_info(dev, useraddr);
+ break;
+ case ETHTOOL_GMODULEEEPROM:
+ rc = ethtool_get_module_eeprom(dev, useraddr);
+ break;
default:
rc = -EOPNOTSUPP;
}
--
1.7.7.6
^ permalink raw reply related
* Re: [RFC PATCH 0/1] NUMA aware scheduling per vhost thread patch
From: Shirley Ma @ 2012-03-27 17:43 UTC (permalink / raw)
To: Jason Wang; +Cc: Michael S. Tsirkin, netdev, kvm, tahm
In-Reply-To: <4F7191ED.80402@redhat.com>
On Tue, 2012-03-27 at 18:09 +0800, Jason Wang wrote:
> Hi:
>
> Thanks for the work and it looks very reasonable, some questions
> below.
>
> On 03/23/2012 07:48 AM, Shirley Ma wrote:
> > Sorry for being late to submit this patch. I have spent lots of time
> > trying to find the best approach. This effort is still going on...
> >
> > This patch is built against net-next tree.
> >
> > This is an experimental RFC patch. The purpose of this patch is to
> > address KVM networking scalability and NUMA scheduling issue.
>
> Need also test for non-NUMA machine, I see that you just choose the
> cpu
> that initiates the work for non-numa machine which seems sub optimal.
Good suggestions. I don't have any non-numa systems. But KK run some
tests on non-numa system. He could see around 20% performance gain for
single VMs local host to guest. I hope we can run a full test on
non-numa system.
On non-numa system, the same per vhost-cpu thread will be always picked
up consistently for a particular vq since all cores are on same cpu
socket. So there will be two per-cpu vhost threads handle TX/RX
simultaneously.
> > The existing implementation of vhost creats a vhost thread
> per-device
> > (virtio_net) based. RX and TX work of a VMs per-device is handled by
> > same vhost thread.
> >
> > One of the limitation of this implementation is with increasing the
> > number VMs or the number of virtio-net interfces, more vhost threads
> are
> > created, it will consume more kernel resources, and induce more
> threads
> > context switches/scheduling overhead. We noticed that the KVM
> network
> > performance doesn't scale with increasing number of VMs.
> >
> > The other limitation is to have single vhost thread to process both
> RX
> > and TX, the work will be blocked. So we create this per cpu vhost
> thread
> > implementation. The number of vhost cpu threads is limited to the
> number
> > of cpus on the host.
> >
> > To address these limitations, we are propsing a per-cpu vhost thread
> > model where the number of vhost threads are limited and equal to the
> > number of online cpus on the host.
>
> The number of vhost thread needs more consideration. Consider that we
> have a 1024 cores host with a card have 16 tx/rx queues, do we really
> need 1024 vhost threads?
In this case, we could add a module parameter to limit the number of
cores/sockets to be used.
> >
> > Based on our testing experience, the vcpus can be scheduled across
> cpu
> > sockets even when the number of vcpus is smaller than the number of
> > cores per cpu socket and there is no other activities besides KVM
> > networking workload. We found that if vhost thread is scheduled on
> the
> > same socket as the work is received, the performance will be better.
> >
> > So in this per cpu vhost thread implementation, a vhost thread is
> > selected dynamically based on where the TX/RX work is initiated. A
> vhost
> > thread on the same cpu socket is selected but not on the same cpu as
> the
> > vcpu/interrupt thread that initizated the TX/RX work.
> >
> > When we test this RFC patch, the other interesting thing we found is
> the
> > performance results also seem related to NIC flow steering. We are
> > spending time on evaluate different NICs flow director
> implementation
> > now. We will enhance this patch based on our findings later.
> >
> > We have tried different scheduling: per-device based, per vq based
> and
> > per work type (tx_kick, rx_kick, tx_net, rx_net) based vhost
> scheduling,
> > we found that so far the per vq based scheduling is good enough for
> now.
>
> Could you please explain more about those scheduling strategies? Does
> per-device based means let a dedicated vhost thread to handle all
> work
> from that vhost device? As you mentioned, maybe an improvement of the
> scheduling to take flow steering info (queue mapping, rxhash etc.) of
> skb in host into account.
Yes, per-device scheduling means one per-cpu vhost theads handle all
works from one particular vhost-device.
Yes, we think scheduling to take flow steering info would help
performance. I am studying this now.
> >
> > We also tried different algorithm to select which cpu vhost thread
> will
> > running on a specific cpu socket: avg_load balance, and randomly...
>
> May worth to account the out-of-oder packet during the test as for a
> single stream as different cpu/vhost/physical queue may be chose to
> do
> the packet transmission/reception?
Good point. I haven't gone through all data yet. netstat output might
tell us something.
We used Intel 10G NIC to run all test. For a single steam test, Intel
NIC receiving irq steers with same irq/queue which TX packets have been
sent. So when we mask vcpus from same VM on one socket, we shouldn't hit
packet out-of-order case. We might hit packet out of order when vcpus
run across sockets.
> >
> > > From our test results, we found that the scalability has been
> > significantly improved. And this patch is also helpful for small
> packets
> > performance.
> >
> > Hoever, we are seeing some regressions in a local guest to guest
> > scenario on a 8 cpu NUMA system.
> >
> > In one case, 24 VMs 256 bytes tcp_stream test shows it has improved
> from
> > 810Mb/s to 9.1Gb/s. :)
> > (We created two local VMs, and each VM has 2 vcpus. W/o this patch,
> the
> > number of threads is 4 vcpus + 2 vhosts = 6, w/i this patch is 4
> vcpus +
> > 8 vhosts = 12. It causes more context switches. When I change the
> > scheduling to use 2-4 vhost threads, the regressions are gone. I am
> > continue investigation on how to make small number of VMs, local
> guest
> > to gues performance better. Once I find the clue, I will share
> here.)
> >
> > The cpu hotplug support hasn't in place yet. I will post it later.
>
> Another question is why not just using workqueue? It has full support
> for cpu hotplug and allow more polices.
Yes, it's good to use workqueue. I just did everything on top of current
implementation so it's easy to compare/analyze the performance data.
I remembered the vhost implementation changed from workqueue to thread
for some reason. I couldn't recall the reason.
> >
> > Since we have per cpu vhost thread, each vhost thread will handle
> > multiple vqs, so we will be able to reduce/remove vq notification
> when
> > the work is heavy loaded in future.
>
> Does this issue still exist if event index is used? If vhost does not
> publish new used index, guest would not kick again.
Since the vhost model has been changed to handle multiple VMs' vqs work,
then it's not necessary to enable these VMs' vqs notification (published
new used idex) where these vqs' future work will be processed on the
same per-cpu vhost thread, as long as the per-cpu vhost thread is still
running.
> >
> > Here is my test results for remote host to guest test: tcp_rrs,
> udp_rrs,
> > tcp_stream with guest has 2 vpus, host has two cpu socket, each
> socket
> > has 4 cores.
> >
> > TCP_STREAM 256 512 1K 2K 4K 8K 16K
> > --------------------------------------------------------------------
> > Original
> >
> H->Guest 2501 4238 4744 5256 7203 6975 5799 Patch
> >
> H->Guest 1676 2290 3149 8026 8439 8283 8216
> >
> > Original
> >
> Guest->H 744 1773 5675 1397 8207 7296 8117
> > Patch
> > Guest->Host 1041 1386 5407 7057 8298 8127 8241
>
> Looks like there's some noise in the result, the throughput of
> "original
> guest -> Host 2K" looks too low. And some strange is that I see
> regressions of packet transmission of guest when testing this patch.
> (
> Guest to Local Host TCP_STREAM in a NUMA machine).
Yes, since I didn't mask the vcpus on the same socket, it might come
from packets out of order. I will rerun the test w/i masking vcpus on
the same socket to see any difference.
You can reference Tom's results. His test is more formal than mine.
> >
> > 60 instances TCP_RRs: Patch 150K trans/s vs. 91K trans/sec
> > 65% improved with taskset vcpus on the same socket
> > 60 instances UDP_RRs: Patch 172K trans/s vs. 103K trans/s
> > 67% improved with taskset vcpus on the same socket
> >
> > Tom has run 1VM to 24 VMs test for different work. He will post it
> here
> > soon.
> >
> > If the host scheduler ensures that the VM's vcpus are not scheduled
> to
> > another socket (i.e. cpu mask the vcpus on same socket) then the
> > performance will be better.
> >
> > Signed-off-by: Shirley Ma<xma@us.ibm.com>
> > Signed-off-by: Krishna Kumar<krkumar2@in.ibm.com>
> > Tested-by: Tom Lendacky<toml@us.ibm.com>
> > ---
> >
> > drivers/vhost/net.c | 26 ++-
> > drivers/vhost/vhost.c | 289
> > +++++++++++++++++++++++----------
> > drivers/vhost/vhost.h | 16 ++-
> > 3 files changed, 232 insertions(+), 103 deletions(-)
> >
> > Thanks
> > Shirley
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe netdev" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
^ permalink raw reply
* [PATCH net-next] virtio_net: do not rate limit counter increments
From: Rick Jones @ 2012-03-27 17:28 UTC (permalink / raw)
To: netdev; +Cc: rusty, mst, virtualization
From: Rick Jones <rick.jones2@hp.com>
While it is desirable to rate limit certain messages, it is not
desirable to rate limit the incrementing of counters associated
with those messages.
Signed-off-by: Rick Jones <rick.jones2@hp.com>
---
Compiled, and run briefly in a 1 vCPU guest under a netperf workload.
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 019da01..4de2760 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -625,12 +625,13 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
/* This can happen with OOM and indirect buffers. */
if (unlikely(capacity < 0)) {
- if (net_ratelimit()) {
- if (likely(capacity == -ENOMEM)) {
+ if (likely(capacity == -ENOMEM)) {
+ if (net_ratelimit()) {
dev_warn(&dev->dev,
"TX queue failure: out of memory\n");
} else {
- dev->stats.tx_fifo_errors++;
+ dev->stats.tx_fifo_errors++;
+ if (net_ratelimit())
dev_warn(&dev->dev,
"Unexpected TX queue failure: %d\n",
capacity);
^ permalink raw reply related
* Re: RCU lock bug in 3.0.21 (bisected to: 682cb56a, fix NULL dereferences in check_peer_redir)
From: Ben Greear @ 2012-03-27 16:47 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev, gregkh, Paul E. McKenney
In-Reply-To: <1332805148.3547.14.camel@edumazet-glaptop>
On 03/26/2012 04:39 PM, Eric Dumazet wrote:
> On Mon, 2012-03-26 at 16:06 -0700, Ben Greear wrote:
>> On 03/26/2012 02:53 PM, Ben Greear wrote:
>>> On 03/26/2012 02:49 PM, David Miller wrote:
>>>>
>>>> Looks like all of those strange undiagnosable reported Dave Jones
>>>> has been feeding us. Something in one part of the kernel leaves
>>>> a lock held, and this shows up as a warning elsewhere.
>>>
>>> Every (initial) bug printout fingers ipv6 and the 'ip' tool on my system.
>>
>> I added a patch to convert rcu_read_lock/unlock to macros so
>> that I could automatically grab the call site (_THIS_IP_)
>> and pass it into the lockdep framework instead of the (useless)
>> _THIS_IP_ in the old rcu_read_lock method which at best seems to
>> only indicate which module the issue relates to...
>
> Hi Ben
>
> Is this problem also appears with current tree ?
> (This could be a problem with the backport, as it was full of
> dependencies)
>
> Also, if you use a patch to better track rcu_read_lock()/unlock(), you
> could add new macros as well to track that a particular unlock() matches
> one given lock(). (maybe returning the rcu_preempt_depth at
> rcu_read_lock() time , but maybe a more absolute ref would be better)
>
> So we could have a warning if an unlock() doesnt match the lock()
>
> inet6_dump_fib () was already a suspect but we could not find why.
Ok, I tried the patch below, and got the result farther down. Is this
what you were thinking of? (The lockdep warning about rcu lock still
held happened immediately after this..so it appears the depth mis-match
does represent this problem...
[greearb@fs3 linux-3.0.dev.y]$ git diff
diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index 0f9b37a..ae3c7c9 100644
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -366,6 +366,7 @@ static int inet6_dump_fib(struct sk_buff *skb, struct netlink_callback *cb)
struct hlist_node *node;
struct hlist_head *head;
int res = 0;
+ int depth = current->lockdep_depth;
s_h = cb->args[0];
s_e = cb->args[1];
@@ -410,6 +411,8 @@ next:
}
out:
rcu_read_unlock();
+ WARN(depth != current->lockdep_depth, "depth: %i lockdep-depth: %i\n",
+ depth, current->lockdep_depth);
cb->args[1] = e;
cb->args[0] = h;
------------[ cut here ]------------
WARNING: at /home/greearb/git/linux-3.0.dev.y/net/ipv6/ip6_fib.c:415 inet6_dump_fib+0x25c/0x292 [ipv6]()
Hardware name: To be filled by O.E.M.
depth: 1 lockdep-depth: 2
Modules linked in: 8021q garp stp llc fuse macvlan pktgen coretemp hwmon sunrpc ipv6 uinput arc4 ath9k snd_hda_codec_realtek mac80211 snd_hda_intel
snd_hda_codec snd_hwdep snd_seq ath9k_common ath9k_hw snd_seq_device snd_pcm ath snd_timer e1000e cfg80211 snd mei(C) ppdev microcode i2c_i801 iTCO_wdt
soundcore serio_raw pcspkr snd_page_alloc iTCO_vendor_support parport_pc parport i915 drm_kms_helper drm i2c_algo_bit i2c_core video [last unloaded: scsi_wait_scan]
Pid: 6563, comm: ip Tainted: G C 3.0.25+ #16
Call Trace:
[<ffffffff81046866>] warn_slowpath_common+0x80/0x98
[<ffffffff81046912>] warn_slowpath_fmt+0x41/0x43
[<ffffffffa0251a3a>] inet6_dump_fib+0x25c/0x292 [ipv6]
[<ffffffff813af450>] netlink_dump+0x5b/0x19b
[<ffffffff81385da2>] ? consume_skb+0x28/0x2a
[<ffffffff813af7bf>] netlink_recvmsg+0x1c7/0x2f8
[<ffffffff8137c6cf>] __sock_recvmsg_nosec+0x65/0x6e
[<ffffffff8137dde0>] __sock_recvmsg+0x49/0x54
[<ffffffff8137e349>] sock_recvmsg+0xa6/0xbf
[<ffffffff81072bf8>] ? lock_release_non_nested+0x9d/0x227
[<ffffffff810ca002>] ? might_fault+0x4e/0x9e
[<ffffffff810ca04b>] ? might_fault+0x97/0x9e
[<ffffffff81387cae>] ? copy_from_user+0x2a/0x2c
[<ffffffff810ca002>] ? might_fault+0x4e/0x9e
[<ffffffff81388080>] ? verify_iovec+0x4f/0xa3
[<ffffffff8137e0c4>] __sys_recvmsg+0x147/0x21e
[<ffffffff81063868>] ? up_read+0x1e/0x36
[<ffffffff810fc9fb>] ? fcheck_files+0xb7/0xee
[<ffffffff810fcb30>] ? fget_light+0x3b/0xbc
[<ffffffff8137e8a0>] sys_recvmsg+0x3d/0x5b
[<ffffffff81450e92>] system_call_fastpath+0x16/0x1b
---[ end trace 5232c09c4fb31d15 ]---
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ 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