* Re: Heavy spin_lock contention in __udp4_lib_mcast_deliver increase
From: Eric Dumazet @ 2012-04-26 15:53 UTC (permalink / raw)
To: Shawn Bohrer; +Cc: netdev
In-Reply-To: <20120426151527.GA2479@BohrerMBP.rgmadvisors.com>
On Thu, 2012-04-26 at 10:15 -0500, Shawn Bohrer wrote:
> I've been doing some UDP multicast benchmarking and noticed that as we
> increase the number of sockets/multicast addresses the performance
> degrades. The test I'm running has multiple machines sending packets
> on multiple multicast addresses. A single receiving machine opens one
> socket per multicast address to receive all the packets. The
> receiving process is bound to a core that is not processing
> interrupts.
>
> Running this test with 300 multicast addresses and sockets and
> profiling the receiving machine with 'perf -a -g' I can see the
> following:
>
>
> # Events: 45K cycles
> #
> # Overhead
> # ........ .....................................
> #
> 52.56% [k] _raw_spin_lock
> |
> |--99.09%-- __udp4_lib_mcast_deliver
> 20.10% [k] __udp4_lib_mcast_deliver
> |
> --- __udp4_lib_rcv
>
> So if I understand this correctly 52.56% of the time is spent
> contending for the spin_lock in __udp4_lib_mcast_deliver. If I
> understand the code correctly it appears that for every packet
> received we walk the list of all UDP sockets while holding the
> spin_lock. Therefore I believe the thing that hurts so much in this
> case is that we have a lot of UDP sockets.
>
> Are there any ideas on how we can improve the performance in this
> case? Honestly I have two ideas though my understanding of the
> network stack is limited and it is unclear to me how to implement
> either of them.
>
> The first idea is to use RCU instead of acquiring the spin_lock. This
> is what the Unicast path does however looking back to 271b72c7 "udp:
> RCU handling for Unicast packets." Eric points out that the multicast
> path is difficult. It appears from that commit description that the
> problem is that since we have to find all sockets interested in
> receiving the packet instead of just one that restarting the scan of
> the hlist could lead us to deliver the packet twice to the same
> socket. That commit is rather old though I believe things may have
> changed. Looking at commit 1240d137 "ipv4: udp: Optimise multicast
> reception" I can see that Eric also has already done some work to
> reduce how long the spin_lock is held in __udp4_lib_mcast_deliver().
> That commit also says "It's also a base for a future RCU conversion of
> multicast recption". Is the idea that you could remove duplicate
> sockets within flush_stack()? Actually I don't think that would work
> since flush_stack() can be called multiple times if the stack gets
> full.
>
> The second idea would be to hash the sockets to reduce the number of
> sockets to walk for each packet. Once again it looks like the Unicast
> path already does this in commits 512615b6b "udp: secondary hash on
> (local port, local address)" and 5051ebd27 "ipv4: udp: optimize
> unicast RX path". Perhaps these hash lists could be used, however I
> don't think they can since they currently use RCU and thus it might
> depend on converting to RCU first.
Let me understand
You have 300 sockets bound to the same port, so a single message must be
copied 300 times and delivered to those sockets ?
^ permalink raw reply
* Re: [RFC v4] Add TCP encap_rcv hook (repost)
From: Jesse Gross @ 2012-04-26 16:13 UTC (permalink / raw)
To: Simon Horman
Cc: dev-yBygre7rU0TnMu66kgdUjQ, eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w,
netdev-u79uwXL29TY76Z2rM5mHXA, jhs-jkUAjuhPggJWk0Htik3J/w,
shemminger-ZtmgI6mnKB3QT0dZR+AlfA, David Miller
In-Reply-To: <20120426071321.GA25781-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
On Thu, Apr 26, 2012 at 12:13 AM, Simon Horman <horms@verge.net.au> wrote:
> On Wed, Apr 25, 2012 at 10:17:25AM -0700, Jesse Gross wrote:
>> On Wed, Apr 25, 2012 at 1:39 AM, Simon Horman <horms@verge.net.au> wrote:
>> >
>> > Hi Kyle,
>> >
>> > the component that is of most interest to me is enabling OVS to use in-tree
>> > tunnelling code - as it seems that makes most sense for an implementation
>> > of STT. I have taken a brief look over your vxlan work and it isn't clear
>> > to me if it is moving towards being an in-tree implementation. Moreover,
>> > I'm a rather unclear on what changes need to be made to OVS in order for
>> > in-tree tunneling to be used.
>> >
>> > My recollection is that OVS did make use of in-tree tunnelling code
>> > but this was removed in favour of the current implementation for various
>> > reasons (performance being one IIRC). I gather that revisiting in-tree
>> > tunnelling won't revisit the previous set of problems. But I'm unclear how.
>> >
>> > Jesse, is it possible for you to describe that in a little detail
>> > or point me to some information?
>>
>> This was what I had originally written a while back, although it's
>> more about OVS internally and less about how to connect to the in-tree
>> code:
>> http://openvswitch.org/pipermail/dev/2012-February/014779.html
>>
>> In order to flexibly implement support for current and future tunnel
>> protocols OVS needs to be able to get/set information about the outer
>> tunnel header when processing the inner packet. At the very least
>> this is src/dst IP addresses and the key/ID/VNI/etc. In the upstream
>> tunnel implementations those are implicitly encoded in the device that
>> sends or receives the packet. However, this has a two problems:
>> number of devices and ability to handle unknown values. We addressed
>> part of this problem by allowing the tunnel ID to be set and matched
>> through the OVS flow table and an action. In order to do this with
>> the in-tree tunneling code, we obviously need a way of passing this
>> information around since it would currently get lost as we pass
>> through the Linux device layer.
>>
>> The plan to deal with that is to add a function to the in-tree
>> tunneling code that allows a skb to be encapsulated with specific
>> parameters and conversely a hook to receive decapsulated packets along
>> with header info. This would make all of the kernel tunneling code
>> common, while still giving OVS userspace the ability to implement
>> essentially any type of tunneling policy. In many ways, this is very
>> similar to how vlans look in OVS today.
>>
>> While it would be possible to implement the hook to use the in-tree
>> tunnel code today without a lot of changes, we already know that we
>> want to move away from port-based model in the OVS kernel module
>> towards the flow model. As we push this upstream the userspace/kernel
>> API should be the correct one, so that's why these two things are tied
>> together.
>
>
> Thanks, that explanation along with Kyle's response helps a lot.
>
> It seems to me that something I could help out with is the implementation
> of the set_tunnel action which extents and replaces the tun_id action.
> It seems that is a requirement for the scheme you describe above.
>
> http://openvswitch.org/pipermail/dev/2012-April/016239.html
I agree that's probably the best place to start unless Kyle has some
specific plans otherwise.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
^ permalink raw reply
* Re: [RFC v4] Add TCP encap_rcv hook (repost)
From: Kyle Mestery (kmestery) @ 2012-04-26 16:16 UTC (permalink / raw)
To: Jesse Gross
Cc: <dev-yBygre7rU0TnMu66kgdUjQ@public.gmane.org>,
<eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>,
<netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
<jhs-jkUAjuhPggJWk0Htik3J/w@public.gmane.org>,
<shemminger-ZtmgI6mnKB3QT0dZR+AlfA@public.gmane.org>,
David Miller
In-Reply-To: <CAEP_g=8VQizt5iUc_yR+PynMYpZgD4ep+o379JK8k-KCKMYgmg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Apr 26, 2012, at 11:13 AM, Jesse Gross wrote:
> On Thu, Apr 26, 2012 at 12:13 AM, Simon Horman <horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org> wrote:
>> On Wed, Apr 25, 2012 at 10:17:25AM -0700, Jesse Gross wrote:
>>> On Wed, Apr 25, 2012 at 1:39 AM, Simon Horman <horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org> wrote:
>>>>
>>>> Hi Kyle,
>>>>
>>>> the component that is of most interest to me is enabling OVS to use in-tree
>>>> tunnelling code - as it seems that makes most sense for an implementation
>>>> of STT. I have taken a brief look over your vxlan work and it isn't clear
>>>> to me if it is moving towards being an in-tree implementation. Moreover,
>>>> I'm a rather unclear on what changes need to be made to OVS in order for
>>>> in-tree tunneling to be used.
>>>>
>>>> My recollection is that OVS did make use of in-tree tunnelling code
>>>> but this was removed in favour of the current implementation for various
>>>> reasons (performance being one IIRC). I gather that revisiting in-tree
>>>> tunnelling won't revisit the previous set of problems. But I'm unclear how.
>>>>
>>>> Jesse, is it possible for you to describe that in a little detail
>>>> or point me to some information?
>>>
>>> This was what I had originally written a while back, although it's
>>> more about OVS internally and less about how to connect to the in-tree
>>> code:
>>> http://openvswitch.org/pipermail/dev/2012-February/014779.html
>>>
>>> In order to flexibly implement support for current and future tunnel
>>> protocols OVS needs to be able to get/set information about the outer
>>> tunnel header when processing the inner packet. At the very least
>>> this is src/dst IP addresses and the key/ID/VNI/etc. In the upstream
>>> tunnel implementations those are implicitly encoded in the device that
>>> sends or receives the packet. However, this has a two problems:
>>> number of devices and ability to handle unknown values. We addressed
>>> part of this problem by allowing the tunnel ID to be set and matched
>>> through the OVS flow table and an action. In order to do this with
>>> the in-tree tunneling code, we obviously need a way of passing this
>>> information around since it would currently get lost as we pass
>>> through the Linux device layer.
>>>
>>> The plan to deal with that is to add a function to the in-tree
>>> tunneling code that allows a skb to be encapsulated with specific
>>> parameters and conversely a hook to receive decapsulated packets along
>>> with header info. This would make all of the kernel tunneling code
>>> common, while still giving OVS userspace the ability to implement
>>> essentially any type of tunneling policy. In many ways, this is very
>>> similar to how vlans look in OVS today.
>>>
>>> While it would be possible to implement the hook to use the in-tree
>>> tunnel code today without a lot of changes, we already know that we
>>> want to move away from port-based model in the OVS kernel module
>>> towards the flow model. As we push this upstream the userspace/kernel
>>> API should be the correct one, so that's why these two things are tied
>>> together.
>>
>>
>> Thanks, that explanation along with Kyle's response helps a lot.
>>
>> It seems to me that something I could help out with is the implementation
>> of the set_tunnel action which extents and replaces the tun_id action.
>> It seems that is a requirement for the scheme you describe above.
>>
>> http://openvswitch.org/pipermail/dev/2012-April/016239.html
>
> I agree that's probably the best place to start unless Kyle has some
> specific plans otherwise.
Simon and I chatted off-list, and this is indeed where we plan to start.
^ permalink raw reply
* Re: Heavy spin_lock contention in __udp4_lib_mcast_deliver increase
From: Eric Dumazet @ 2012-04-26 16:18 UTC (permalink / raw)
To: Shawn Bohrer; +Cc: netdev
In-Reply-To: <1335455595.2775.47.camel@edumazet-glaptop>
On Thu, 2012-04-26 at 17:53 +0200, Eric Dumazet wrote:
> Let me understand
>
> You have 300 sockets bound to the same port, so a single message must be
> copied 300 times and delivered to those sockets ?
>
>
Please try the following patch. It should allow up to 512 sockets (on
x86_64) to be stored in stack, and delivery performed out of the locked
section.
net/ipv4/udp.c | 16 ++++++++++++----
net/ipv6/udp.c | 15 +++++++++++----
2 files changed, 23 insertions(+), 8 deletions(-)
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 279fd08..beb9ea6 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1539,13 +1539,20 @@ static void flush_stack(struct sock **stack, unsigned int count,
static int __udp4_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
struct udphdr *uh,
__be32 saddr, __be32 daddr,
- struct udp_table *udptable)
+ struct udp_table *udptable,
+ int proto)
{
- struct sock *sk, *stack[256 / sizeof(struct sock *)];
+ struct sock *sk, **stack;
struct udp_hslot *hslot = udp_hashslot(udptable, net, ntohs(uh->dest));
int dif;
unsigned int i, count = 0;
+ stack = kmalloc(PAGE_SIZE, GFP_ATOMIC);
+ if (unlikely(!stack)) {
+ UDP_INC_STATS_BH(net, UDP_MIB_RCVBUFERRORS, proto == IPPROTO_UDPLITE);
+ kfree_skb(skb);
+ return 0;
+ }
spin_lock(&hslot->lock);
sk = sk_nulls_head(&hslot->head);
dif = skb->dev->ifindex;
@@ -1554,7 +1561,7 @@ static int __udp4_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
stack[count++] = sk;
sk = udp_v4_mcast_next(net, sk_nulls_next(sk), uh->dest,
daddr, uh->source, saddr, dif);
- if (unlikely(count == ARRAY_SIZE(stack))) {
+ if (unlikely(count == PAGE_SIZE/sizeof(*sk))) {
if (!sk)
break;
flush_stack(stack, count, skb, ~0);
@@ -1580,6 +1587,7 @@ static int __udp4_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
} else {
kfree_skb(skb);
}
+ kfree(stack);
return 0;
}
@@ -1661,7 +1669,7 @@ int __udp4_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
if (rt->rt_flags & (RTCF_BROADCAST|RTCF_MULTICAST))
return __udp4_lib_mcast_deliver(net, skb, uh,
- saddr, daddr, udptable);
+ saddr, daddr, udptable, proto);
sk = __udp4_lib_lookup_skb(skb, uh->source, uh->dest, udptable);
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index d39bbc9..fc79b87 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -640,14 +640,20 @@ drop:
*/
static int __udp6_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
const struct in6_addr *saddr, const struct in6_addr *daddr,
- struct udp_table *udptable)
+ struct udp_table *udptable, int proto)
{
- struct sock *sk, *stack[256 / sizeof(struct sock *)];
+ struct sock *sk, **stack;
const struct udphdr *uh = udp_hdr(skb);
struct udp_hslot *hslot = udp_hashslot(udptable, net, ntohs(uh->dest));
int dif;
unsigned int i, count = 0;
+ stack = kmalloc(PAGE_SIZE, GFP_ATOMIC);
+ if (unlikely(!stack)) {
+ UDP6_INC_STATS_BH(net, UDP_MIB_RCVBUFERRORS, proto == IPPROTO_UDPLITE);
+ kfree_skb(skb);
+ return 0;
+ }
spin_lock(&hslot->lock);
sk = sk_nulls_head(&hslot->head);
dif = inet6_iif(skb);
@@ -656,7 +662,7 @@ static int __udp6_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
stack[count++] = sk;
sk = udp_v6_mcast_next(net, sk_nulls_next(sk), uh->dest, daddr,
uh->source, saddr, dif);
- if (unlikely(count == ARRAY_SIZE(stack))) {
+ if (unlikely(count == PAGE_SIZE/sizeof(*sk))) {
if (!sk)
break;
flush_stack(stack, count, skb, ~0);
@@ -679,6 +685,7 @@ static int __udp6_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
} else {
kfree_skb(skb);
}
+ kfree(stack);
return 0;
}
@@ -763,7 +770,7 @@ int __udp6_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
*/
if (ipv6_addr_is_multicast(daddr))
return __udp6_lib_mcast_deliver(net, skb,
- saddr, daddr, udptable);
+ saddr, daddr, udptable, proto);
/* Unicast */
^ permalink raw reply related
* Re: Heavy spin_lock contention in __udp4_lib_mcast_deliver increase
From: Eric Dumazet @ 2012-04-26 16:21 UTC (permalink / raw)
To: Shawn Bohrer; +Cc: netdev
In-Reply-To: <1335457112.2775.50.camel@edumazet-glaptop>
On Thu, 2012-04-26 at 18:18 +0200, Eric Dumazet wrote:
> On Thu, 2012-04-26 at 17:53 +0200, Eric Dumazet wrote:
>
> > Let me understand
> >
> > You have 300 sockets bound to the same port, so a single message must be
> > copied 300 times and delivered to those sockets ?
> >
> >
>
> Please try the following patch. It should allow up to 512 sockets (on
> x86_64) to be stored in stack, and delivery performed out of the locked
> section.
>
> net/ipv4/udp.c | 16 ++++++++++++----
> net/ipv6/udp.c | 15 +++++++++++----
> 2 files changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index 279fd08..beb9ea6 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -1539,13 +1539,20 @@ static void flush_stack(struct sock **stack, unsigned int count,
> static int __udp4_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
> struct udphdr *uh,
> __be32 saddr, __be32 daddr,
> - struct udp_table *udptable)
> + struct udp_table *udptable,
> + int proto)
> {
> - struct sock *sk, *stack[256 / sizeof(struct sock *)];
> + struct sock *sk, **stack;
> struct udp_hslot *hslot = udp_hashslot(udptable, net, ntohs(uh->dest));
> int dif;
> unsigned int i, count = 0;
>
> + stack = kmalloc(PAGE_SIZE, GFP_ATOMIC);
> + if (unlikely(!stack)) {
> + UDP_INC_STATS_BH(net, UDP_MIB_RCVBUFERRORS, proto == IPPROTO_UDPLITE);
> + kfree_skb(skb);
> + return 0;
> + }
> spin_lock(&hslot->lock);
> sk = sk_nulls_head(&hslot->head);
> dif = skb->dev->ifindex;
> @@ -1554,7 +1561,7 @@ static int __udp4_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
> stack[count++] = sk;
> sk = udp_v4_mcast_next(net, sk_nulls_next(sk), uh->dest,
> daddr, uh->source, saddr, dif);
> - if (unlikely(count == ARRAY_SIZE(stack))) {
> + if (unlikely(count == PAGE_SIZE/sizeof(*sk))) {
Oops, should be PAGE_SIZE/sizeof(sk) of course
(same problem in ipv6/udp.c)
> if (!sk)
> break;
> flush_stack(stack, count, skb, ~0);
> @@ -1580,6 +1587,7 @@ static int __udp4_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
> } else {
> kfree_skb(skb);
> }
> + kfree(stack);
> return 0;
> }
>
^ permalink raw reply
* Re: Heavy spin_lock contention in __udp4_lib_mcast_deliver increase
From: Shawn Bohrer @ 2012-04-26 16:28 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev
In-Reply-To: <1335455595.2775.47.camel@edumazet-glaptop>
On Thu, Apr 26, 2012 at 05:53:15PM +0200, Eric Dumazet wrote:
> On Thu, 2012-04-26 at 10:15 -0500, Shawn Bohrer wrote:
> > I've been doing some UDP multicast benchmarking and noticed that as we
> > increase the number of sockets/multicast addresses the performance
> > degrades. The test I'm running has multiple machines sending packets
> > on multiple multicast addresses. A single receiving machine opens one
> > socket per multicast address to receive all the packets. The
> > receiving process is bound to a core that is not processing
> > interrupts.
> >
> > Running this test with 300 multicast addresses and sockets and
> > profiling the receiving machine with 'perf -a -g' I can see the
> > following:
> >
> >
> > # Events: 45K cycles
> > #
> > # Overhead
> > # ........ .....................................
> > #
> > 52.56% [k] _raw_spin_lock
> > |
> > |--99.09%-- __udp4_lib_mcast_deliver
> > 20.10% [k] __udp4_lib_mcast_deliver
> > |
> > --- __udp4_lib_rcv
> >
> > So if I understand this correctly 52.56% of the time is spent
> > contending for the spin_lock in __udp4_lib_mcast_deliver. If I
> > understand the code correctly it appears that for every packet
> > received we walk the list of all UDP sockets while holding the
> > spin_lock. Therefore I believe the thing that hurts so much in this
> > case is that we have a lot of UDP sockets.
> >
> > Are there any ideas on how we can improve the performance in this
> > case? Honestly I have two ideas though my understanding of the
> > network stack is limited and it is unclear to me how to implement
> > either of them.
> >
> > The first idea is to use RCU instead of acquiring the spin_lock. This
> > is what the Unicast path does however looking back to 271b72c7 "udp:
> > RCU handling for Unicast packets." Eric points out that the multicast
> > path is difficult. It appears from that commit description that the
> > problem is that since we have to find all sockets interested in
> > receiving the packet instead of just one that restarting the scan of
> > the hlist could lead us to deliver the packet twice to the same
> > socket. That commit is rather old though I believe things may have
> > changed. Looking at commit 1240d137 "ipv4: udp: Optimise multicast
> > reception" I can see that Eric also has already done some work to
> > reduce how long the spin_lock is held in __udp4_lib_mcast_deliver().
> > That commit also says "It's also a base for a future RCU conversion of
> > multicast recption". Is the idea that you could remove duplicate
> > sockets within flush_stack()? Actually I don't think that would work
> > since flush_stack() can be called multiple times if the stack gets
> > full.
> >
> > The second idea would be to hash the sockets to reduce the number of
> > sockets to walk for each packet. Once again it looks like the Unicast
> > path already does this in commits 512615b6b "udp: secondary hash on
> > (local port, local address)" and 5051ebd27 "ipv4: udp: optimize
> > unicast RX path". Perhaps these hash lists could be used, however I
> > don't think they can since they currently use RCU and thus it might
> > depend on converting to RCU first.
>
> Let me understand
>
> You have 300 sockets bound to the same port, so a single message must be
> copied 300 times and delivered to those sockets ?
No in this case it is 300 unique multicast addresses, and there is one
socket listening to each multicast address. So a single message is
only copied once to a single socket. The bottle neck appears to be
that even though a single message is only going to get copied to a
single socket we still have to walk the list of all 300 sockets while
holding the spin lock to figure that out. The incoming packet rate is
also roughly evenly distributed across all 300 multicast addresses so
even though we have multiple receive queues they are all contending
for the same spin lock.
--
Shawn
--
---------------------------------------------------------------
This email, along with any attachments, is confidential. If you
believe you received this message in error, please contact the
sender immediately and delete all copies of the message.
Thank you.
^ permalink raw reply
* Re: use-after-free in usbnet
From: Huajun Li @ 2012-04-26 16:30 UTC (permalink / raw)
To: Ming Lei
Cc: Oliver Neukum, Alan Stern, Dave Jones, netdev, linux-usb,
Fedora Kernel Team
In-Reply-To: <CACVXFVOFGA-Rn3X2HGGM-AYSygiysNnRo0RiRzmJhZ=swtZz_g@mail.gmail.com>
On Thu, Apr 26, 2012 at 1:02 PM, Ming Lei <tom.leiming@gmail.com> wrote:
> Hi Huajun,
>
> On Fri, Apr 20, 2012 at 9:37 PM, Huajun Li <huajun.li.lee@gmail.com> wrote:
>>
>>
>> Above patch has already been integrated to mainline. However, maybe
>> there still exists another potentail use-after-free issue, here is a
>> case:
>> After release the lock in unlink_urbs(), defer_bh() may move
>> current skb from rxq/txq to dev->done queue, even cause the skb be
>> released. Then in next loop cycle, it can't refer to expected skb, and
>> may Oops again.
>>
>> To easily reproduce it, in unlink_urbs(), you can delay a short time
>> after usb_put_urb(urb), then disconnect your device while transferring
>> data, and repeat it times you will find errors on your screen.
>>
>> Following is a draft patch to guarantee the queue consistent, and
>> refer to expected skb in each loop cycle:
>>
>> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
>> index b7b3f5b..6da0141 100644
>> --- a/drivers/net/usb/usbnet.c
>> +++ b/drivers/net/usb/usbnet.c
>> @@ -578,16 +578,28 @@ EXPORT_SYMBOL_GPL(usbnet_purge_paused_rxq);
>> static int unlink_urbs (struct usbnet *dev, struct sk_buff_head *q)
>> {
>> unsigned long flags;
>> - struct sk_buff *skb, *skbnext;
>> + struct sk_buff *skb;
>> int count = 0;
>>
>> spin_lock_irqsave (&q->lock, flags);
>> - skb_queue_walk_safe(q, skb, skbnext) {
>> + while (!skb_queue_empty(q)) {
>> struct skb_data *entry;
>> struct urb *urb;
>> int retval;
>>
>> - entry = (struct skb_data *) skb->cb;
>> + skb_queue_walk(q, skb) {
>> + entry = (struct skb_data *)skb->cb;
>> + if (entry->state == rx_done ||
>> + entry->state == tx_done ||
>> + entry->state == rx_cleanup)
>> + continue;
>> + else
>> + break;
>> + }
>> +
>> + if (skb == (struct sk_buff *)(q))
>> + break;
>> +
>> urb = entry->urb;
>>
>>
>
> After thinking about the issue further, the basic idea of your patch is good,
> but not safe(double unlink, also races on accessing entry->state), so I write
> a new one based on your patch.
>
> Are you OK this new one?
>
Yes, no more issue that I can see by now, thanks.
> Thanks,
> --
> Ming Lei
>
> From 89eb41968caa9523c90c1cf7b7e2259db00e04b8 Mon Sep 17 00:00:00 2001
> From: Ming Lei <tom.leiming@gmail.com>
> Date: Thu, 26 Apr 2012 11:33:46 +0800
> Subject: [PATCH] usbnet: fix skb traversing races during unlink
>
> Commit 4231d47e6fe69f061f96c98c30eaf9fb4c14b96d(net/usbnet: avoid
> recursive locking in usbnet_stop()) fixes the recursive locking
> problem by releasing the skb queue lock before unlink, but may
> cause skb traversing races:
> - after URB is unlinked and the queue lock is released,
> the refered skb and skb->next may be moved to done queue,
> even be released
> - in skb_queue_walk_safe, the next skb is still obtained
> by next pointer of the last skb
> - so maybe trigger oops or other problems
>
> This patch extends the usage of entry->state to describe 'start_unlink'
> state, so always holding the queue(rx/tx) lock to change the state if
> the referd skb is in rx or tx queue because we need to know if the
> refered urb has been started unlinking in unlink_urbs.
>
> Also the patch uses usb_block_urb introduced by Oliver to block
> URB resubmit in complete handler if the URB will be unlinked.
>
> The other part of this patch is based on Hugjun's patch:
s/Hugjun/Huajun
> always traverse from head of the tx/rx queue to get skb which is
> to be unlinked but not been started unlinking.
> ---
^ permalink raw reply
* Re: Heavy spin_lock contention in __udp4_lib_mcast_deliver increase
From: Eric Dumazet @ 2012-04-26 16:31 UTC (permalink / raw)
To: Shawn Bohrer; +Cc: netdev
In-Reply-To: <20120426162819.GD2479@BohrerMBP.rgmadvisors.com>
On Thu, 2012-04-26 at 11:28 -0500, Shawn Bohrer wrote:
> No in this case it is 300 unique multicast addresses, and there is one
> socket listening to each multicast address. So a single message is
> only copied once to a single socket. The bottle neck appears to be
> that even though a single message is only going to get copied to a
> single socket we still have to walk the list of all 300 sockets while
> holding the spin lock to figure that out. The incoming packet rate is
> also roughly evenly distributed across all 300 multicast addresses so
> even though we have multiple receive queues they are all contending
> for the same spin lock.
>
I repeat my question : Are these 300 sockets bound to the same UDP
port ?
If not, they should be spreaded in hash table.
You can make this hash table very big to reduce hash collisions
Boot parameter : uhash_entries=65536
^ permalink raw reply
* Re: [PATCH] b44: properly use pr_fmt()
From: Luis R. Rodriguez @ 2012-04-26 15:53 UTC (permalink / raw)
To: David Miller
Cc: zambrano, netdev, linux-wireless, linux-bluetooth,
Arend Van Spriel, Joe Perches
In-Reply-To: <CAB=NE6ViDGHPn=OsqFK1rJb-5J+1xzvf3h9+Rd-MgmWpn-T2fA@mail.gmail.com>
On Thu, Apr 26, 2012 at 6:02 AM, Luis R. Rodriguez
<mcgrof@do-not-panic.com> wrote:
> On Thu, Apr 26, 2012 at 5:23 AM, Luis R. Rodriguez
> <mcgrof@do-not-panic.com> wrote:
>> On Mon, Apr 23, 2012 at 9:47 PM, David Miller <davem@davemloft.net> wrote:
>>> From: "Luis R. Rodriguez" <mcgrof@do-not-panic.com>
>>> Date: Mon, 23 Apr 2012 19:46:43 -0700
>>>
>>>> From: "Luis R. Rodriguez" <mcgrof@do-not-panic.com>
>>>>
>>>> pr_fmt() is either defined or we redefine it. Typically
>>>> drivers define it prior to including printk.h but this
>>>> is done under the assumption that no other subsystem
>>>> it uses has already defined pr_fmt(). In such cases
>>>> pr_fmt() should be undefined and redefined.
>>>>
>>>> Doing this properly shaves down compilation time quite
>>>> considerably.
>>>>
>>>> Signed-off-by: Luis R. Rodriguez <mcgrof@do-not-panic.com>
>>>
>>> Every driver defines pr_fmt then includes the headers. I do not
>>> see any other place which performs the initial undef of pr_fmt
>>> like you want to start doing here.
>>
>> One example is, and that is where I got the hint:
>>
>> drivers/net/wireless/mwifiex/decl.h
>>
>>> If there is a reason we should start doing this, it would appear
>>> to be a universal reason, not one specific to this driver.
>>
>> Sure.
>>
>>> If that's the case, we should formally agree to this and then work
>>> on gradually converting all the drivers in reasonably sized chunks.
>>
>> Understood.
>>
>>> Either way I can't apply this patch as-is, sorry Luis.
>>
>> So the issue can occur if any parent header ends up defining this,
>> this could happen if a subsystem wants to define the pr_fmt in their
>> own header and a driver for example wants to override this. In theory
>> this does not occur but in practice this may vary and if we want to
>> ensure this does not happen perhaps the thing to do is to document
>> this as such.
>>
>> I just tested allmodconfig though and checked compilation for V=1
>> against drivers/net/wireless/ and found no issues against linux-next
>> though so in practice at least for wireless networking it seems we are
>> squeaky clean in its usage. I'll test across the kernel though next to
>> see if there are any real violators of the assumption. Unfortunately
>> I've hit an issue with compiling allmodconfig already with linux-next
>> that is already reported so my homework will be to find a shiny
>> linux-next to compile test against allmodconfig.
>>
>> An area that this does clearly happen but that likely does not merit
>> and upstream change for is in the backport work compat that defines
>> this for older kernels and later we have to also backport users of
>> pr_fmt such as pr_emerg_once. Ultimately drivers will want to actually
>> then override this and in that case the undef is necessary as
>> otherwise you get the redefinition warning which incurs in practice a
>> considerable CPU performance hit at compilation time.
>
> Additionally Arend has pointed pointed out that previously printk.h
> was removed as a direct include as well given that kernel.h includes
> it already:
>
> https://lkml.org/lkml/2011/5/25/331
>
> So this would be a second issue with this patch.
OK I found no pr_fmt warnings at all for make allmodconfig ; make V=1
; on v3.4-rc4. For this and others reason stated then yes, this patch
is simply bogus.
Luis
^ permalink raw reply
* Re: [net-next 1/4 (V3)] net: ethtool: add the EEE support
From: Ben Hutchings @ 2012-04-26 17:17 UTC (permalink / raw)
To: Giuseppe CAVALLARO; +Cc: netdev, rayagond, davem
In-Reply-To: <4F98FDCC.3040807@st.com>
On Thu, 2012-04-26 at 09:48 +0200, Giuseppe CAVALLARO wrote:
> Hello Ben
>
> On 4/19/2012 5:30 PM, Ben Hutchings wrote:
> [snip]
> >> I'm changing the code for getting/setting the EEE capability and trying
> >> to follow your suggestions.
> >>
> >> The "get" will show the following things; this is a bit different of the
> >> points "a" "b" and "c" we had discussed. Maybe, this could also be a
> >> more complete (*) .
> >> The ethtool (see output below as example) could report the phy
> >> (supported/advertised/lp_advertised) and mac eee capabilities separately.
> >
> > Sounds reasonable.
> >
> >> The "set" will be useful for some eth devices (like the stmmac) that can
> >> stop/enable internally the eee capability (at mac level).
> >
> > I don't know much about EEE, but shouldn't the driver take care of
> > configuring the MAC for this whenever the PHY is set to advertise EEE
> > capability?
>
> Yes indeed this can be done at driver level. So could I definitely
> remove it from ethtool? What do you suggest?
>
> In case of the stmmac I could add a specific driver option via sys to
> enable/disable the eee and set timer.
Generally, ethtool doesn't distinguish MAC and PHY settings because they
have to be configured consistently for the device to do anything useful.
If there is some good use for enabling EEE in the MAC and not the PHY,
or vice versa, then this should be exposed in the ethtool interface.
But if not then I don't believe it needs to be in either an ethtool or a
driver-specific interface.
> >> [snip]
> >>
> >> Current message level: 0x0000003f (63)
> >> drv probe link timer ifdown ifup
> >> Link detected: yes
> >> Energy-Efficient Ethernet: -------------------------
> >> MAC supports: yes |-> related to MAC side |
> >> phy supports modes: ... |-> from MMD 3.20 |
> >> phy advertising modes: ... |-> from MMD 7.60 |
> >> LP advertising modes: ... |-> from MMD 7.61 |
> >> --------------------------
> >> (*)
> >> PS. The "..." above means that we can actually dump: 100BASE-TX EEE etc
> >> for each advertising modes and also for phy support (reg 3.20).
> >
> > Yes, that's the sort of information I would expect to see (but try to be
> > consistent with the wording used for AN).:
>
> e.g. SUPPORTED_100baseT_EEE ... ADVERTISED_<...>
I meant the wording used in the ethtool output: 'supported',
'advertised', 'link partner advertised' rather than 'phy supports',
'phy advertising', 'LP advertising'.
> > The EEE advertising mask presumably should be changeable similarly to
> > the AN advertising mask ('ethtool -s <devname> eeeadv <mask>'). But I
> > don't know how the two advertising masks interact. Is one supposed to
> > be a subset of the other? Currently ethtool automatically changes the
> > AN advertising mask in response to a speed/duplex change; should it also
> > try to change the EEE advertising mask?
>
> I've just verified the IEEE (Table 45–150a—EEE advertisement register
> (Register 7.60) bit definitions) and sorry for my delay in reply but I
> was in trouble because looking at the registers for the phy (I am using)
> the reg 7.60 was in RO and I couldn't understand how to set the mask.
> I confirm that the Adv reg from the std is R/W and the mask as you
> suggest could be set according to the speed.
> The EEE should work on duplex mode only.
>
> I wonder so if if the final patch I should have no new option for the
> ethtool command and EEE info are directly passed from the kernel like
> speed and duplex when call get_settings.
Are you suggesting to define EEE mode flags in the existing supported,
advertising and lp_advertising masks?
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH net-next 3/5] be2net: Fix to apply duplex value as unknown when link is down.
From: Ben Hutchings @ 2012-04-26 17:24 UTC (permalink / raw)
To: Somnath Kotur; +Cc: netdev
In-Reply-To: <cf78444f-636f-44c5-beb7-5626b8a3df17@exht1.ad.emulex.com>
On Thu, 2012-04-26 at 19:58 +0530, Somnath Kotur wrote:
> From: Somnath Kotur <somnath.kotur@emulex.com>
>
>
> Signed-off-by: Somnath Kotur <somnath.kotur@emulex.com>
> ---
> drivers/net/ethernet/emulex/benet/be_ethtool.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/ethernet/emulex/benet/be_ethtool.c b/drivers/net/ethernet/emulex/benet/be_ethtool.c
> index 5296df5..bb2add7 100644
> --- a/drivers/net/ethernet/emulex/benet/be_ethtool.c
> +++ b/drivers/net/ethernet/emulex/benet/be_ethtool.c
> @@ -618,7 +618,7 @@ static int be_get_settings(struct net_device *netdev, struct ethtool_cmd *ecmd)
> ecmd->supported = adapter->phy.supported;
> }
>
> - ecmd->duplex = DUPLEX_FULL;
> + ecmd->duplex = netif_carrier_ok(netdev) ? DUPLEX_FULL : -1;
> ecmd->phy_address = adapter->port_num;
>
> return 0;
I don't see any problem with reporting full-duplex all the time if you
don't support any half-duplex link modes. But we don't yet have
consistency between drivers in speed/duplex reporting while the link is
down, so I won't insist that that is the right thing to do.
However you should use DUPLEX_UNKNOWN rather than -1.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH net-next 2/5] be2net: Fix to allow setting of debug levels in the firmware.
From: Ben Hutchings @ 2012-04-26 17:29 UTC (permalink / raw)
To: Somnath Kotur; +Cc: netdev
In-Reply-To: <464dcb28-2450-4e0b-a30f-d8c9104bac86@exht1.ad.emulex.com>
On Thu, 2012-04-26 at 19:57 +0530, Somnath Kotur wrote:
> From: Somnath Kotur <somnath.kotur@emulex.com>
[...]
> @@ -893,6 +959,7 @@ const struct ethtool_ops be_ethtool_ops = {
> .set_pauseparam = be_set_pauseparam,
> .get_strings = be_get_stat_strings,
> .set_phys_id = be_set_phys_id,
> + .set_msglevel = be_set_msglevel,
> .get_sset_count = be_get_sset_count,
> .get_ethtool_stats = be_get_ethtool_stats,
> .get_regs_len = be_get_reg_len,
This operation is intended for controlling logging by the driver, and
the flags are defined in <linux/netdevice.h>. (Not exported to userland
yet, but ethtool knows their names.)
If your firmware supports some kind of logging then it may be reasonable
to have this control both driver and firmware, but not *just* the
firmware.
You should also implement the get_msglevel operation at the same time.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* [PATCH] NFC: nci/data.c: quiet sparse noise about plain integer as NULL pointer
From: H Hartley Sweeten @ 2012-04-26 17:31 UTC (permalink / raw)
To: Linux Kernel
Cc: netdev, linux-wireless, lauro.venancio, aloisio.almeida, sameo,
davem
Pointers should be cleared with NULL, not 0.
Quiets a couple sparse warnings of the type:
warning: Using plain integer as NULL pointer
Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
Cc: Lauro Ramos Venancio <lauro.venancio@openbossa.org>
Cc: Aloisio Almeida Jr <aloisio.almeida@openbossa.org>
Cc: Samuel Ortiz <sameo@linux.intel.com>
Cc: "David S. Miller" <davem@davemloft.net>
---
diff --git a/net/nfc/nci/data.c b/net/nfc/nci/data.c
index a0bc326..76c48c5 100644
--- a/net/nfc/nci/data.c
+++ b/net/nfc/nci/data.c
@@ -49,7 +49,7 @@ void nci_data_exchange_complete(struct nci_dev *ndev, struct sk_buff *skb,
if (cb) {
ndev->data_exchange_cb = NULL;
- ndev->data_exchange_cb_context = 0;
+ ndev->data_exchange_cb_context = NULL;
/* forward skb to nfc core */
cb(cb_context, skb, err);
@@ -200,10 +200,10 @@ static void nci_add_rx_data_frag(struct nci_dev *ndev,
pr_err("error adding room for accumulated rx data\n");
kfree_skb(skb);
- skb = 0;
+ skb = NULL;
kfree_skb(ndev->rx_data_reassembly);
- ndev->rx_data_reassembly = 0;
+ ndev->rx_data_reassembly = NULL;
err = -ENOMEM;
goto exit;
@@ -216,7 +216,7 @@ static void nci_add_rx_data_frag(struct nci_dev *ndev,
/* third, free old reassembly */
kfree_skb(ndev->rx_data_reassembly);
- ndev->rx_data_reassembly = 0;
+ ndev->rx_data_reassembly = NULL;
}
if (pbf == NCI_PBF_CONT) {
^ permalink raw reply related
* [PATCH] NFC: nci/lib.c: include header for exported symbol prototype
From: H Hartley Sweeten @ 2012-04-26 17:56 UTC (permalink / raw)
To: Linux Kernel
Cc: netdev, linux-wireless, lauro.venancio, aloisio.almeida, sameo,
davem
Include the header to pickup the exported symbol prototype.
Quites the sparse warning:
warning: symbol 'nci_to_errno' was not declared. Should it be static?
Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
Cc: Lauro Ramos Venancio <lauro.venancio@openbossa.org>
Cc: Aloisio Almeida Jr <aloisio.almeida@openbossa.org>
CC: Samuel Ortiz <sameo@linux.intel.com>
CC: "David S. Miller" <davem@davemloft.net>
---
diff --git a/net/nfc/nci/lib.c b/net/nfc/nci/lib.c
index 6a63e5e..6b7fd26 100644
--- a/net/nfc/nci/lib.c
+++ b/net/nfc/nci/lib.c
@@ -31,6 +31,7 @@
#include <linux/errno.h>
#include <net/nfc/nci.h>
+#include <net/nfc/nci_core.h>
/* NCI status codes to Unix errno mapping */
int nci_to_errno(__u8 code)
^ permalink raw reply related
* [PATCH] NFC: nci/ntf.c: quiet sparse noise about plain integer as NULL pointer
From: H Hartley Sweeten @ 2012-04-26 17:59 UTC (permalink / raw)
To: Linux Kernel
Cc: netdev, linux-wireless, lauro.venancio, aloisio.almeida, sameo,
davem
Pointers should be cleared with NULL, not 0.
Quiets a couple sparse warnings of the type:
warning: Using plain integer as NULL pointer
Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
Cc: Lauro Ramos Venancio <lauro.venancio@openbossa.org>
Cc: Aloisio Almeida Jr <aloisio.almeida@openbossa.org>
Cc: Samuel Ortiz <sameo@linux.intel.com>
Cc: "David S. Miller" <davem@davemloft.net>
---
diff --git a/net/nfc/nci/ntf.c b/net/nfc/nci/ntf.c
index 99e1632..cb26461 100644
--- a/net/nfc/nci/ntf.c
+++ b/net/nfc/nci/ntf.c
@@ -497,7 +497,7 @@ static void nci_rf_deactivate_ntf_packet(struct nci_dev *ndev,
/* drop partial rx data packet */
if (ndev->rx_data_reassembly) {
kfree_skb(ndev->rx_data_reassembly);
- ndev->rx_data_reassembly = 0;
+ ndev->rx_data_reassembly = NULL;
}
/* complete the data exchange transaction, if exists */
^ permalink raw reply related
* [PATCH] NFC: hci/core.c: local variables should not be exposed globally
From: H Hartley Sweeten @ 2012-04-26 18:05 UTC (permalink / raw)
To: Linux Kernel
Cc: netdev, linux-wireless, lauro.venancio, aloisio.almeida, sameo,
davem
The variable 'hci_nfc_ops' is only referenced in this file and
should be marked static to prevent it from being exposed globally.
Quites the sparse warning:
warning: symbol 'hci_nfc_ops' was not declared. Should it be static?
Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
Cc: Lauro Ramos Venancio <lauro.venancio@openbossa.org>
Cc: Aloisio Almeida Jr <aloisio.almeida@openbossa.org>
Cc: Samuel Ortiz <sameo@linux.intel.com>
Cc: "David S. Miller" <davem@davemloft.net>
---
diff --git a/net/nfc/hci/core.c b/net/nfc/hci/core.c
index 86fd00d..0ce5ee4 100644
--- a/net/nfc/hci/core.c
+++ b/net/nfc/hci/core.c
@@ -605,7 +605,7 @@ static int hci_data_exchange(struct nfc_dev *nfc_dev, u32 target_idx,
return 0;
}
-struct nfc_ops hci_nfc_ops = {
+static struct nfc_ops hci_nfc_ops = {
.dev_up = hci_dev_up,
.dev_down = hci_dev_down,
.start_poll = hci_start_poll,
^ permalink raw reply related
* [PATCH] NFC: netlink.c: local variables should not be exposed globally
From: H Hartley Sweeten @ 2012-04-26 18:09 UTC (permalink / raw)
To: Linux Kernel
Cc: netdev, linux-wireless, lauro.venancio, aloisio.almeida, sameo,
davem
The variable ''nfc_genl_family' is only referenced in this file and
should be marked static to prevent it from being exposed globally.
Quites the sparse warning:
warning: symbol 'nfc_genl_family' was not declared. Should it be static?
Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
Cc: Lauro Ramos Venancio <lauro.venancio@openbossa.org>
Cc: Aloisio Almeida Jr <aloisio.almeida@openbossa.org>
Cc: Samuel Ortiz <sameo@linux.intel.com>
Cc: "David S. Miller" <davem@davemloft.net>
---
diff --git a/net/nfc/netlink.c b/net/nfc/netlink.c
index f1829f6..850485d 100644
--- a/net/nfc/netlink.c
+++ b/net/nfc/netlink.c
@@ -33,7 +33,7 @@ static struct genl_multicast_group nfc_genl_event_mcgrp = {
.name = NFC_GENL_MCAST_EVENT_NAME,
};
-struct genl_family nfc_genl_family = {
+static struct genl_family nfc_genl_family = {
.id = GENL_ID_GENERATE,
.hdrsize = 0,
.name = NFC_GENL_NAME,
^ permalink raw reply related
* [PATCH] IPVS: ip_vs_ftp.c: local functions should not be exposed globally
From: H Hartley Sweeten @ 2012-04-26 18:17 UTC (permalink / raw)
To: Linux Kernel; +Cc: netdev, davem
Functions not referenced outside of a source file should be marked
static to prevent it from being exposed globally.
This quiets the sparse warnings:
warning: symbol 'ip_vs_ftp_init' was not declared. Should it be static?
Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
Cc: "David S. Miller" <davem@davemloft.net>
---
diff --git a/net/netfilter/ipvs/ip_vs_ftp.c b/net/netfilter/ipvs/ip_vs_ftp.c
index debb8c7..091bec9 100644
--- a/net/netfilter/ipvs/ip_vs_ftp.c
+++ b/net/netfilter/ipvs/ip_vs_ftp.c
@@ -483,7 +483,7 @@ static struct pernet_operations ip_vs_ftp_ops = {
.exit = __ip_vs_ftp_exit,
};
-int __init ip_vs_ftp_init(void)
+static int __init ip_vs_ftp_init(void)
{
int rv;
^ permalink raw reply related
* [PATCH] IPVS: ip_vs_proto.c: local functions should not be exposed globally
From: H Hartley Sweeten @ 2012-04-26 18:26 UTC (permalink / raw)
To: Linux Kernel; +Cc: netdev, davem
Functions not referenced outside of a source file should be marked
static to prevent it from being exposed globally.
This quiets the sparse warnings:
warning: symbol '__ipvs_proto_data_get' was not declared. Should it be static?
Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
Cc: "David S. Miller" <davem@davemloft.net>
---
diff --git a/net/netfilter/ipvs/ip_vs_proto.c b/net/netfilter/ipvs/ip_vs_proto.c
index 6eda11d..d5d1d92 100644
--- a/net/netfilter/ipvs/ip_vs_proto.c
+++ b/net/netfilter/ipvs/ip_vs_proto.c
@@ -153,7 +153,7 @@ EXPORT_SYMBOL(ip_vs_proto_get);
/*
* get ip_vs_protocol object data by netns and proto
*/
-struct ip_vs_proto_data *
+static struct ip_vs_proto_data *
__ipvs_proto_data_get(struct netns_ipvs *ipvs, unsigned short proto)
{
struct ip_vs_proto_data *pd;
^ permalink raw reply related
* [PATCH] IPVS: ip_vs_sync.c: local functions should not be exposed globally
From: H Hartley Sweeten @ 2012-04-26 18:29 UTC (permalink / raw)
To: Linux Kernel; +Cc: netdev, davem
Functions not referenced outside of a source file should be marked
static to prevent it from being exposed globally.
This quiets the sparse warnings:
warning: symbol 'ip_vs_sync_conn_v0' was not declared. Should it be static?
Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
Cc: "David S. Miller" <davem@davemloft.net>
---
diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
index bf5e538..49a1fe8 100644
--- a/net/netfilter/ipvs/ip_vs_sync.c
+++ b/net/netfilter/ipvs/ip_vs_sync.c
@@ -446,7 +446,7 @@ ip_vs_sync_buff_create_v0(struct netns_ipvs *ipvs)
* Version 0 , could be switched in by sys_ctl.
* Add an ip_vs_conn information into the current sync_buff.
*/
-void ip_vs_sync_conn_v0(struct net *net, struct ip_vs_conn *cp)
+static void ip_vs_sync_conn_v0(struct net *net, struct ip_vs_conn *cp)
{
struct netns_ipvs *ipvs = net_ipvs(net);
struct ip_vs_sync_mesg_v0 *m;
^ permalink raw reply related
* Re: pull request: wireless-next 2012-04-25
From: John W. Linville @ 2012-04-26 18:41 UTC (permalink / raw)
To: David Miller
Cc: linux-wireless-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20120425.163926.2209040699696617525.davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
On Wed, Apr 25, 2012 at 04:39:26PM -0400, David Miller wrote:
> From: "John W. Linville" <linville-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
> Date: Wed, 25 Apr 2012 15:14:19 -0400
>
> > This is another batch of updates intended for 3.5...
> ...
> > Please let me know if there are problems!
>
> There are:
>
> From 792545c7bc5d6b922d3778dc602e557d64c83551 Mon Sep 17 00:00:00 2001
> From: "Luis R. Rodriguez" <mcgrof-3uybbJdB1yH774rrrx3eTA@public.gmane.org>
> Date: Mon, 23 Apr 2012 19:55:48 -0700
> Subject: [PATCH 88/88] libertas: include sched.h on firmware.c
>
> Do not assume we have our subsystem including this for us,
> at least for older kernels this is not true. Lets just be
> explicit about this requirement for the usage of wake_up().
>
> Signed-off-by: Luis R. Rodriguez <mcgrof-3uybbJdB1yH774rrrx3eTA@public.gmane.org>
> Signed-off-by: John W. Linville <linville-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
>
> This is bogus, wake_up() is defined in linux/wait.h, the whole
> point of the linux/sched.h split up is so that linux/sched.h
> includes could be removed and replaced with actual dependencies.
This is completely my bad. Anyway, Felix already added that include
in that file a few lines later in order to pick-up the definitions
for TASK_NORMAL and TASK_UNINTERRUPTIBLE. I'll just drop Luis's patch.
> Also, please don't accept any patches from Luis that add those
> #undef pr_fmt things to the atheros drivers.
>
> He tried to add it an ethernet driver, and I asked him to explain
> exactly why he's doing and that if it's appropriate then it's
> appropriate everywhere not just in a few specific drivers. He failed
> to respond to me, and therefore failed to explain the situation and
> address my concerned. And then I saw just today that he's submitting
> the same patch to wireless drivers. That's not acceptable.
>
> Thanks.
OK. I think the compat-wireless guys have found a different solution
anyway.
John
--
John W. Linville Someday the world will need a hero, and you
linville-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org might be all we have. Be ready.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH 0/2] drop_monitor: misc fixes for some recently reported bugs
From: Neil Horman @ 2012-04-26 18:47 UTC (permalink / raw)
To: netdev; +Cc: Neil Horman, Eric Dumazet, David Miller
Hey-
Eric was using dropwatch recently and reported a few bugs to me that he
had noted. This short series should fix them all up.
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: Eric Dumazet <eric.dumazet@gmail.com>
CC: David Miller <davem@davemloft.net>
^ permalink raw reply
* [PATCH 1/2] drop_monitor: fix sleeping in invalid context warning
From: Neil Horman @ 2012-04-26 18:47 UTC (permalink / raw)
To: netdev; +Cc: Neil Horman, David Miller
In-Reply-To: <1335466022-32661-1-git-send-email-nhorman@tuxdriver.com>
Eric Dumazet pointed out this warning in the drop_monitor protocol to me:
[ 38.352571] BUG: sleeping function called from invalid context at kernel/mutex.c:85
[ 38.352576] in_atomic(): 1, irqs_disabled(): 0, pid: 4415, name: dropwatch
[ 38.352580] Pid: 4415, comm: dropwatch Not tainted 3.4.0-rc2+ #71
[ 38.352582] Call Trace:
[ 38.352592] [<ffffffff8153aaf0>] ? trace_napi_poll_hit+0xd0/0xd0
[ 38.352599] [<ffffffff81063f2a>] __might_sleep+0xca/0xf0
[ 38.352606] [<ffffffff81655b16>] mutex_lock+0x26/0x50
[ 38.352610] [<ffffffff8153aaf0>] ? trace_napi_poll_hit+0xd0/0xd0
[ 38.352616] [<ffffffff810b72d9>] tracepoint_probe_register+0x29/0x90
[ 38.352621] [<ffffffff8153a585>] set_all_monitor_traces+0x105/0x170
[ 38.352625] [<ffffffff8153a8ca>] net_dm_cmd_trace+0x2a/0x40
[ 38.352630] [<ffffffff8154a81a>] genl_rcv_msg+0x21a/0x2b0
[ 38.352636] [<ffffffff810f8029>] ? zone_statistics+0x99/0xc0
[ 38.352640] [<ffffffff8154a600>] ? genl_rcv+0x30/0x30
[ 38.352645] [<ffffffff8154a059>] netlink_rcv_skb+0xa9/0xd0
[ 38.352649] [<ffffffff8154a5f0>] genl_rcv+0x20/0x30
[ 38.352653] [<ffffffff81549a7e>] netlink_unicast+0x1ae/0x1f0
[ 38.352658] [<ffffffff81549d76>] netlink_sendmsg+0x2b6/0x310
[ 38.352663] [<ffffffff8150824f>] sock_sendmsg+0x10f/0x130
[ 38.352668] [<ffffffff8150abe0>] ? move_addr_to_kernel+0x60/0xb0
[ 38.352673] [<ffffffff81515f04>] ? verify_iovec+0x64/0xe0
[ 38.352677] [<ffffffff81509c46>] __sys_sendmsg+0x386/0x390
[ 38.352682] [<ffffffff810ffaf9>] ? handle_mm_fault+0x139/0x210
[ 38.352687] [<ffffffff8165b5bc>] ? do_page_fault+0x1ec/0x4f0
[ 38.352693] [<ffffffff8106ba4d>] ? set_next_entity+0x9d/0xb0
[ 38.352699] [<ffffffff81310b49>] ? tty_ldisc_deref+0x9/0x10
[ 38.352703] [<ffffffff8106d363>] ? pick_next_task_fair+0x63/0x140
[ 38.352708] [<ffffffff8150b8d4>] sys_sendmsg+0x44/0x80
[ 38.352713] [<ffffffff8165f8e2>] system_call_fastpath+0x16/0x1b
It stems from holding a spinlock (trace_state_lock) while attempting to register
or unregister tracepoint hooks, making in_atomic() true in this context, leading
to the warning when the tracepoint calls might_sleep() while its taking a mutex.
Since we only use the trace_state_lock to prevent trace protocol state races, as
well as hardware stat list updates on an rcu write side, we can just convert the
spinlock to a mutex to avoid this problem.
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
CC: David Miller <davem@davemloft.net>
---
net/core/drop_monitor.c | 14 +++++++-------
1 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c
index 5c3c81a..04ce1dd 100644
--- a/net/core/drop_monitor.c
+++ b/net/core/drop_monitor.c
@@ -42,7 +42,7 @@ static void send_dm_alert(struct work_struct *unused);
* netlink alerts
*/
static int trace_state = TRACE_OFF;
-static DEFINE_SPINLOCK(trace_state_lock);
+static DEFINE_MUTEX(trace_state_lock);
struct per_cpu_dm_data {
struct work_struct dm_alert_work;
@@ -214,7 +214,7 @@ static int set_all_monitor_traces(int state)
struct dm_hw_stat_delta *new_stat = NULL;
struct dm_hw_stat_delta *temp;
- spin_lock(&trace_state_lock);
+ mutex_lock(&trace_state_lock);
if (state == trace_state) {
rc = -EAGAIN;
@@ -253,7 +253,7 @@ static int set_all_monitor_traces(int state)
rc = -EINPROGRESS;
out_unlock:
- spin_unlock(&trace_state_lock);
+ mutex_unlock(&trace_state_lock);
return rc;
}
@@ -296,12 +296,12 @@ static int dropmon_net_event(struct notifier_block *ev_block,
new_stat->dev = dev;
new_stat->last_rx = jiffies;
- spin_lock(&trace_state_lock);
+ mutex_lock(&trace_state_lock);
list_add_rcu(&new_stat->list, &hw_stats_list);
- spin_unlock(&trace_state_lock);
+ mutex_unlock(&trace_state_lock);
break;
case NETDEV_UNREGISTER:
- spin_lock(&trace_state_lock);
+ mutex_lock(&trace_state_lock);
list_for_each_entry_safe(new_stat, tmp, &hw_stats_list, list) {
if (new_stat->dev == dev) {
new_stat->dev = NULL;
@@ -312,7 +312,7 @@ static int dropmon_net_event(struct notifier_block *ev_block,
}
}
}
- spin_unlock(&trace_state_lock);
+ mutex_unlock(&trace_state_lock);
break;
}
out:
--
1.7.7.6
^ permalink raw reply related
* [PATCH 2/2] drop_monitor: Make updating data->skb smp safe
From: Neil Horman @ 2012-04-26 18:47 UTC (permalink / raw)
To: netdev; +Cc: Neil Horman, David Miller
In-Reply-To: <1335466022-32661-1-git-send-email-nhorman@tuxdriver.com>
Eric Dumazet pointed out to me that the drop_monitor protocol has some holes in
its smp protections. Specifically, its possible to replace data->skb while its
being written. This patch corrects that by making data->skb and rcu protected
variable. That will prevent it from being overwritten while a tracepoint is
modifying it.
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
CC: David Miller <davem@davemloft.net>
---
net/core/drop_monitor.c | 43 ++++++++++++++++++++++++++++++++-----------
1 files changed, 32 insertions(+), 11 deletions(-)
diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c
index 04ce1dd..852e36b 100644
--- a/net/core/drop_monitor.c
+++ b/net/core/drop_monitor.c
@@ -46,7 +46,7 @@ static DEFINE_MUTEX(trace_state_lock);
struct per_cpu_dm_data {
struct work_struct dm_alert_work;
- struct sk_buff *skb;
+ struct sk_buff __rcu *skb;
atomic_t dm_hit_count;
struct timer_list send_timer;
};
@@ -79,29 +79,41 @@ static void reset_per_cpu_data(struct per_cpu_dm_data *data)
size_t al;
struct net_dm_alert_msg *msg;
struct nlattr *nla;
+ struct sk_buff *skb;
al = sizeof(struct net_dm_alert_msg);
al += dm_hit_limit * sizeof(struct net_dm_drop_point);
al += sizeof(struct nlattr);
- data->skb = genlmsg_new(al, GFP_KERNEL);
- genlmsg_put(data->skb, 0, 0, &net_drop_monitor_family,
+ skb = genlmsg_new(al, GFP_KERNEL);
+ genlmsg_put(skb, 0, 0, &net_drop_monitor_family,
0, NET_DM_CMD_ALERT);
- nla = nla_reserve(data->skb, NLA_UNSPEC, sizeof(struct net_dm_alert_msg));
+ nla = nla_reserve(skb, NLA_UNSPEC, sizeof(struct net_dm_alert_msg));
msg = nla_data(nla);
memset(msg, 0, al);
+
+ /*
+ * Don't need to lock this, since we are guaranteed to only
+ * run this on a single cpu at a time
+ */
+ rcu_assign_pointer(data->skb, skb);
+
+ synchronize_rcu();
+
atomic_set(&data->dm_hit_count, dm_hit_limit);
}
static void send_dm_alert(struct work_struct *unused)
{
struct sk_buff *skb;
- struct per_cpu_dm_data *data = &__get_cpu_var(dm_cpu_data);
+ struct per_cpu_dm_data *data = &get_cpu_var(dm_cpu_data);
/*
* Grab the skb we're about to send
*/
- skb = data->skb;
+ rcu_read_lock();
+ skb = rcu_dereference(data->skb);
+ rcu_read_unlock();
/*
* Replace it with a new one
@@ -113,6 +125,7 @@ static void send_dm_alert(struct work_struct *unused)
*/
genlmsg_multicast(skb, 0, NET_DM_GRP_ALERT, GFP_KERNEL);
+ put_cpu_var(dm_cpu_data);
}
/*
@@ -123,9 +136,11 @@ static void send_dm_alert(struct work_struct *unused)
*/
static void sched_send_work(unsigned long unused)
{
- struct per_cpu_dm_data *data = &__get_cpu_var(dm_cpu_data);
+ struct per_cpu_dm_data *data = &get_cpu_var(dm_cpu_data);
+
+ schedule_work_on(smp_processor_id(), &data->dm_alert_work);
- schedule_work(&data->dm_alert_work);
+ put_cpu_var(dm_cpu_data);
}
static void trace_drop_common(struct sk_buff *skb, void *location)
@@ -134,9 +149,13 @@ static void trace_drop_common(struct sk_buff *skb, void *location)
struct nlmsghdr *nlh;
struct nlattr *nla;
int i;
- struct per_cpu_dm_data *data = &__get_cpu_var(dm_cpu_data);
+ struct sk_buff *dskb;
+ struct per_cpu_dm_data *data = &get_cpu_var(dm_cpu_data);
+ rcu_read_lock();
+ dskb = rcu_dereference(data->skb);
+
if (!atomic_add_unless(&data->dm_hit_count, -1, 0)) {
/*
* we're already at zero, discard this hit
@@ -144,7 +163,7 @@ static void trace_drop_common(struct sk_buff *skb, void *location)
goto out;
}
- nlh = (struct nlmsghdr *)data->skb->data;
+ nlh = (struct nlmsghdr *)dskb->data;
nla = genlmsg_data(nlmsg_data(nlh));
msg = nla_data(nla);
for (i = 0; i < msg->entries; i++) {
@@ -158,7 +177,7 @@ static void trace_drop_common(struct sk_buff *skb, void *location)
/*
* We need to create a new entry
*/
- __nla_reserve_nohdr(data->skb, sizeof(struct net_dm_drop_point));
+ __nla_reserve_nohdr(dskb, sizeof(struct net_dm_drop_point));
nla->nla_len += NLA_ALIGN(sizeof(struct net_dm_drop_point));
memcpy(msg->points[msg->entries].pc, &location, sizeof(void *));
msg->points[msg->entries].count = 1;
@@ -170,6 +189,8 @@ static void trace_drop_common(struct sk_buff *skb, void *location)
}
out:
+ rcu_read_unlock();
+ put_cpu_var(dm_cpu_data);
return;
}
--
1.7.7.6
^ permalink raw reply related
* Re: [PATCH 2/2] drop_monitor: Make updating data->skb smp safe
From: Eric Dumazet @ 2012-04-26 19:00 UTC (permalink / raw)
To: Neil Horman; +Cc: netdev, David Miller
In-Reply-To: <1335466022-32661-3-git-send-email-nhorman@tuxdriver.com>
On Thu, 2012-04-26 at 14:47 -0400, Neil Horman wrote:
> Eric Dumazet pointed out to me that the drop_monitor protocol has some holes in
> its smp protections. Specifically, its possible to replace data->skb while its
> being written. This patch corrects that by making data->skb and rcu protected
> variable. That will prevent it from being overwritten while a tracepoint is
> modifying it.
>
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
> CC: David Miller <davem@davemloft.net>
> ---
> net/core/drop_monitor.c | 43 ++++++++++++++++++++++++++++++++-----------
> 1 files changed, 32 insertions(+), 11 deletions(-)
>
Hi Neil
I believe more work is needed on this patch
> diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c
> index 04ce1dd..852e36b 100644
> --- a/net/core/drop_monitor.c
> +++ b/net/core/drop_monitor.c
> @@ -46,7 +46,7 @@ static DEFINE_MUTEX(trace_state_lock);
>
> struct per_cpu_dm_data {
> struct work_struct dm_alert_work;
> - struct sk_buff *skb;
> + struct sk_buff __rcu *skb;
> atomic_t dm_hit_count;
> struct timer_list send_timer;
> };
> @@ -79,29 +79,41 @@ static void reset_per_cpu_data(struct per_cpu_dm_data *data)
> size_t al;
> struct net_dm_alert_msg *msg;
> struct nlattr *nla;
> + struct sk_buff *skb;
>
> al = sizeof(struct net_dm_alert_msg);
> al += dm_hit_limit * sizeof(struct net_dm_drop_point);
> al += sizeof(struct nlattr);
>
> - data->skb = genlmsg_new(al, GFP_KERNEL);
> - genlmsg_put(data->skb, 0, 0, &net_drop_monitor_family,
> + skb = genlmsg_new(al, GFP_KERNEL);
skb can be NULL here...
> + genlmsg_put(skb, 0, 0, &net_drop_monitor_family,
> 0, NET_DM_CMD_ALERT);
> - nla = nla_reserve(data->skb, NLA_UNSPEC, sizeof(struct net_dm_alert_msg));
> + nla = nla_reserve(skb, NLA_UNSPEC, sizeof(struct net_dm_alert_msg));
> msg = nla_data(nla);
> memset(msg, 0, al);
> +
> + /*
> + * Don't need to lock this, since we are guaranteed to only
> + * run this on a single cpu at a time
> + */
> + rcu_assign_pointer(data->skb, skb);
> +
> + synchronize_rcu();
> +
> atomic_set(&data->dm_hit_count, dm_hit_limit);
> }
>
> static void send_dm_alert(struct work_struct *unused)
> {
> struct sk_buff *skb;
> - struct per_cpu_dm_data *data = &__get_cpu_var(dm_cpu_data);
> + struct per_cpu_dm_data *data = &get_cpu_var(dm_cpu_data);
>
> /*
> * Grab the skb we're about to send
> */
> - skb = data->skb;
> + rcu_read_lock();
> + skb = rcu_dereference(data->skb);
This protects nothing ...
> + rcu_read_unlock();
>
> /*
> * Replace it with a new one
> @@ -113,6 +125,7 @@ static void send_dm_alert(struct work_struct *unused)
> */
> genlmsg_multicast(skb, 0, NET_DM_GRP_ALERT, GFP_KERNEL);
>
> + put_cpu_var(dm_cpu_data);
> }
>
> /*
> @@ -123,9 +136,11 @@ static void send_dm_alert(struct work_struct *unused)
> */
> static void sched_send_work(unsigned long unused)
> {
> - struct per_cpu_dm_data *data = &__get_cpu_var(dm_cpu_data);
> + struct per_cpu_dm_data *data = &get_cpu_var(dm_cpu_data);
> +
> + schedule_work_on(smp_processor_id(), &data->dm_alert_work);
>
> - schedule_work(&data->dm_alert_work);
> + put_cpu_var(dm_cpu_data);
> }
>
> static void trace_drop_common(struct sk_buff *skb, void *location)
> @@ -134,9 +149,13 @@ static void trace_drop_common(struct sk_buff *skb, void *location)
> struct nlmsghdr *nlh;
> struct nlattr *nla;
> int i;
> - struct per_cpu_dm_data *data = &__get_cpu_var(dm_cpu_data);
> + struct sk_buff *dskb;
> + struct per_cpu_dm_data *data = &get_cpu_var(dm_cpu_data);
>
>
> + rcu_read_lock();
> + dskb = rcu_dereference(data->skb);
> +
dskb can be NULL here
> if (!atomic_add_unless(&data->dm_hit_count, -1, 0)) {
> /*
> * we're already at zero, discard this hit
> @@ -144,7 +163,7 @@ static void trace_drop_common(struct sk_buff *skb, void *location)
> goto out;
> }
>
> - nlh = (struct nlmsghdr *)data->skb->data;
> + nlh = (struct nlmsghdr *)dskb->data;
> nla = genlmsg_data(nlmsg_data(nlh));
> msg = nla_data(nla);
> for (i = 0; i < msg->entries; i++) {
> @@ -158,7 +177,7 @@ static void trace_drop_common(struct sk_buff *skb, void *location)
> /*
> * We need to create a new entry
> */
> - __nla_reserve_nohdr(data->skb, sizeof(struct net_dm_drop_point));
> + __nla_reserve_nohdr(dskb, sizeof(struct net_dm_drop_point));
> nla->nla_len += NLA_ALIGN(sizeof(struct net_dm_drop_point));
> memcpy(msg->points[msg->entries].pc, &location, sizeof(void *));
> msg->points[msg->entries].count = 1;
> @@ -170,6 +189,8 @@ static void trace_drop_common(struct sk_buff *skb, void *location)
> }
>
> out:
> + rcu_read_unlock();
> + put_cpu_var(dm_cpu_data);
> return;
> }
>
Thanks
^ permalink raw reply
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