* [PATCH net-next-2.6 3/5] udpv6: Fix gso_size setting in ip6_ufo_append_data
From: Sridhar Samudrala @ 2009-07-09 18:09 UTC (permalink / raw)
To: David Miller, Herbert Xu, netdev
[PATCH net-next-2.6 3/5] udpv6: Fix gso_size setting in ip6_ufo_append_data
- fix gso_size setting for ipv6 fragment to be a multiple of 8 bytes.
Signed-off-by: Sridhar Samudrala <sri@us.ibm.com>
---
net/ipv6/ip6_output.c | 8 +++++---
1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 7c76e3d..1c6f0fc 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -1087,9 +1087,11 @@ static inline int ip6_ufo_append_data(struct sock *sk,
if (!err) {
struct frag_hdr fhdr;
- /* specify the length of each IP datagram fragment*/
- skb_shinfo(skb)->gso_size = mtu - fragheaderlen -
- sizeof(struct frag_hdr);
+ /* Specify the length of each IPv6 datagram fragment.
+ * It has to be a multiple of 8.
+ */
+ skb_shinfo(skb)->gso_size = (mtu - fragheaderlen -
+ sizeof(struct frag_hdr)) & ~7;
skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
ipv6_select_ident(skb, &fhdr);
skb_shinfo(skb)->ip6_frag_id = fhdr.identification;
^ permalink raw reply related
* [PATCH net-next-2.6 2/5] udpv6: Fix HW checksum support for outgoing UFO packets
From: Sridhar Samudrala @ 2009-07-09 18:09 UTC (permalink / raw)
To: David Miller, Herbert Xu, netdev
[PATCH net-next-2.6 2/6] udpv6: Fix HW checksum support for outgoing UFO packets
- add HW checksum support for outgoing large UDP/IPv6 packets destined for
a UFO enabled device.
Signed-off-by: Sridhar Samudrala <sri@us.ibm.com>
---
net/ipv6/udp.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 47 insertions(+), 1 deletions(-)
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 33b59bd..f31b1b9 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -638,6 +638,47 @@ static void udp_v6_flush_pending_frames(struct sock *sk)
}
}
+/**
+ * udp6_hwcsum_outgoing - handle outgoing HW checksumming
+ * @sk: socket we are sending on
+ * @skb: sk_buff containing the filled-in UDP header
+ * (checksum field must be zeroed out)
+ */
+static void udp6_hwcsum_outgoing(struct sock *sk, struct sk_buff *skb,
+ const struct in6_addr *saddr,
+ const struct in6_addr *daddr, int len)
+{
+ unsigned int offset;
+ struct udphdr *uh = udp_hdr(skb);
+ __wsum csum = 0;
+
+ if (skb_queue_len(&sk->sk_write_queue) == 1) {
+ /* Only one fragment on the socket. */
+ skb->csum_start = skb_transport_header(skb) - skb->head;
+ skb->csum_offset = offsetof(struct udphdr, check);
+ uh->check = ~csum_ipv6_magic(saddr, daddr, len, IPPROTO_UDP, 0);
+ } else {
+ /*
+ * HW-checksum won't work as there are two or more
+ * fragments on the socket so that all csums of sk_buffs
+ * should be together
+ */
+ offset = skb_transport_offset(skb);
+ skb->csum = skb_checksum(skb, offset, skb->len - offset, 0);
+
+ skb->ip_summed = CHECKSUM_NONE;
+
+ skb_queue_walk(&sk->sk_write_queue, skb) {
+ csum = csum_add(csum, skb->csum);
+ }
+
+ uh->check = csum_ipv6_magic(saddr, daddr, len, IPPROTO_UDP,
+ csum);
+ if (uh->check == 0)
+ uh->check = CSUM_MANGLED_0;
+ }
+}
+
/*
* Sending
*/
@@ -668,7 +709,11 @@ static int udp_v6_push_pending_frames(struct sock *sk)
if (is_udplite)
csum = udplite_csum_outgoing(sk, skb);
- else
+ else if (skb->ip_summed == CHECKSUM_PARTIAL) { /* UDP hardware csum */
+ udp6_hwcsum_outgoing(sk, skb, &fl->fl6_src, &fl->fl6_dst,
+ up->len);
+ goto send;
+ } else
csum = udp_csum_outgoing(sk, skb);
/* add protocol-dependent pseudo-header */
@@ -677,6 +722,7 @@ static int udp_v6_push_pending_frames(struct sock *sk)
if (uh->check == 0)
uh->check = CSUM_MANGLED_0;
+send:
err = ip6_push_pending_frames(sk);
out:
up->len = 0;
^ permalink raw reply related
* [PATCH net-next-2.6 1/5] udpv4: Handle large incoming UDP/IPv4 packets and support software UFO.
From: Sridhar Samudrala @ 2009-07-09 18:09 UTC (permalink / raw)
To: David Miller, Herbert Xu, netdev
[PATCH net-next-2.6 1/5] udpv4: Handle large incoming UDP/IPv4 packets and support software UFO.
- validate and forward GSO UDP/IPv4 packets from untrusted sources.
- do software UFO if the outgoing device doesn't support UFO.
Signed-off-by: Sridhar Samudrala <sri@us.ibm.com>
---
include/net/udp.h | 3 +++
net/ipv4/af_inet.c | 12 +++++++++-
net/ipv4/udp.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 75 insertions(+), 1 deletions(-)
diff --git a/include/net/udp.h b/include/net/udp.h
--- a/include/net/udp.h
+++ b/include/net/udp.h
@@ -207,4 +207,7 @@ extern void udp4_proc_exit(void);
#endif
extern void udp_init(void);
+
+extern int udp4_ufo_send_check(struct sk_buff *skb);
+extern struct sk_buff *udp4_ufo_fragment(struct sk_buff *skb, int features);
#endif /* _UDP_H */
diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 566ea6c..197d024 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -1187,6 +1187,7 @@ static struct sk_buff *inet_gso_segment(struct sk_buff *skb, int features)
int proto;
int ihl;
int id;
+ unsigned int offset = 0;
if (!(features & NETIF_F_V4_CSUM))
features &= ~NETIF_F_SG;
@@ -1229,7 +1230,14 @@ static struct sk_buff *inet_gso_segment(struct sk_buff *skb, int features)
skb = segs;
do {
iph = ip_hdr(skb);
- iph->id = htons(id++);
+ if (proto == IPPROTO_UDP) {
+ iph->id = htons(id);
+ iph->frag_off = htons(offset >> 3);
+ if (skb->next != NULL)
+ iph->frag_off |= htons(IP_MF);
+ offset += (skb->len - skb->mac_len - iph->ihl * 4);
+ } else
+ iph->id = htons(id++);
iph->tot_len = htons(skb->len - skb->mac_len);
iph->check = 0;
iph->check = ip_fast_csum(skb_network_header(skb), iph->ihl);
@@ -1425,6 +1433,8 @@ static struct net_protocol tcp_protocol = {
static struct net_protocol udp_protocol = {
.handler = udp_rcv,
.err_handler = udp_err,
+ .gso_send_check = udp4_ufo_send_check,
+ .gso_segment = udp4_ufo_fragment,
.no_policy = 1,
.netns_ok = 1,
};
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 80e3812..7bc2d08 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1816,6 +1816,67 @@ void __init udp_init(void)
sysctl_udp_wmem_min = SK_MEM_QUANTUM;
}
+int udp4_ufo_send_check(struct sk_buff *skb)
+{
+ const struct iphdr *iph;
+ struct udphdr *uh;
+
+ if (!pskb_may_pull(skb, sizeof(*uh)))
+ return -EINVAL;
+
+ iph = ip_hdr(skb);
+ uh = udp_hdr(skb);
+
+ uh->check = ~csum_tcpudp_magic(iph->saddr, iph->daddr, skb->len,
+ IPPROTO_UDP, 0);
+ skb->csum_start = skb_transport_header(skb) - skb->head;
+ skb->csum_offset = offsetof(struct udphdr, check);
+ skb->ip_summed = CHECKSUM_PARTIAL;
+ return 0;
+}
+
+struct sk_buff *udp4_ufo_fragment(struct sk_buff *skb, int features)
+{
+ struct sk_buff *segs = ERR_PTR(-EINVAL);
+ unsigned int mss;
+ int offset;
+ __wsum csum;
+
+ mss = skb_shinfo(skb)->gso_size;
+ if (unlikely(skb->len <= mss))
+ goto out;
+
+ if (skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) {
+ /* Packet is from an untrusted source, reset gso_segs. */
+ int type = skb_shinfo(skb)->gso_type;
+
+ if (unlikely(type & ~(SKB_GSO_UDP | SKB_GSO_DODGY) ||
+ !(type & (SKB_GSO_UDP))))
+ goto out;
+
+ skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(skb->len, mss);
+
+ segs = NULL;
+ goto out;
+ }
+
+ /* Do software UFO. Complete and fill in the UDP checksum as HW cannot
+ * do checksum of UDP packets sent as multiple IP fragments.
+ */
+ offset = skb->csum_start - skb_headroom(skb);
+ csum = skb_checksum(skb, offset, skb->len- offset, 0);
+ offset += skb->csum_offset;
+ *(__sum16 *)(skb->data + offset) = csum_fold(csum);
+ skb->ip_summed = CHECKSUM_NONE;
+
+ /* Fragment the skb. IP headers of the fragments are updated in
+ * inet_gso_segment()
+ */
+ segs = skb_segment(skb, features);
+out:
+ return segs;
+}
+
EXPORT_SYMBOL(udp_disconnect);
EXPORT_SYMBOL(udp_ioctl);
EXPORT_SYMBOL(udp_prot);
^ permalink raw reply related
* [PATCH net-next-2.6 0/5] Add support for software UDP fragmentation offload(UFO)
From: Sridhar Samudrala @ 2009-07-09 18:09 UTC (permalink / raw)
To: David Miller, Herbert Xu, netdev
The following set of patches fixes support for UDP fragmentation offload
in the core networking stack to send/receive large UDP packets over UFO
enabled devices and fallback to software UFO when the outgoing device
doesn't support UFO.
These patches enable setting UFO on virtio-net and tap devices and allow
large UDP packets to be sent between guest to host and inter guest under
KVM.
[PATCH net-next-2.6 1/5] udpv4: Handle large incoming UDP/IPv4 packets and support software UFO.
[PATCH net-next-2.6 2/5] udpv6: Fix HW checksum support for outgoing UFO packets
[PATCH net-next-2.6 3/5] udpv6: Fix gso_size setting in ip6_ufo_append_data
[PATCH net-next-2.6 4/5] udpv6: Remove unused skb argument of ipv6_select_ident()
[PATCH net-next-2.6 5/5] udpv6: Handle large incoming UDP/IPv6 packets and support software UFO
Thanks
Sridhar
^ permalink raw reply
* Re: Soft-Lockup/Race in networking in 2.6.31-rc1+195 ( possibly?caused by netem)
From: Thomas Gleixner @ 2009-07-09 17:44 UTC (permalink / raw)
To: Andres Freund
Cc: Jarek Poplawski, Joao Correia, Arun R Bharadwaj,
Stephen Hemminger, netdev, LKML, Patrick McHardy, Peter Zijlstra
In-Reply-To: <200907091846.37926.andres@anarazel.de>
Andres,
On Thu, 9 Jul 2009, Andres Freund wrote:
> On Thursday 09 July 2009 18:01:56 Thomas Gleixner wrote:
> > Andres,
> >
> > On Thu, 9 Jul 2009, Andres Freund wrote:
> > > On Thursday 09 July 2009 16:28:05 Thomas Gleixner wrote:
> > > > Please test them separate from each other. The one I sent in this
> > > > thread was just for narrowing down the issue, but I'm now quite sure
> > > > that they really hit the issue which is addressed by the hrtimer
> > > > patch.
> > >
> > > No crash yet. 15min running (seconds to a minute before).
> > >
> > > Will let it run for some hours to be sure.
> >
> > Which one of the patches are you testing ?
> Your "real" one, i.e. de907e8432b08f2d5966c36e0747e97c0e596810
>
> 1h30m now...
Looks like I hit the nail on the head. Queueing it up for Linus.
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH] netpoll: Fix carrier detection for drivers that are using phylib
From: Linus Torvalds @ 2009-07-09 17:29 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Matt Mackall, Anton Vorontsov, Andrew Morton, oleg, mingo,
linux-kernel, netdev
In-Reply-To: <1247149862.12784.6.camel@twins>
On Thu, 9 Jul 2009, Peter Zijlstra wrote:
>
> So something like (utterly untested and such)
This looks like a good patch. Please make it so - who knows what other
uses of cond_resched() we have in module init routines that might have
deadlocks without it. The netpoll case got fixed, but please just do this.
I'd like to do my system_state movement too (the thing is, when you load
drivers as modules you _will_ have "system_state == SYSTEM_RUNNING", so
any initcall that depends on it being "early boot" is already broken), but
there's no way that patch is appropriate for post-rc2. This one, however,
looks appropriate (modulo getting some testing, of course)
Linus
^ permalink raw reply
* Multthreading with netfilter?
From: [AvataR] @ 2009-07-09 17:15 UTC (permalink / raw)
To: netdev
I need to do difficult processing of IP packets. For speed up, I want
to process parts of packets in different kernel threads.
I start n threads (n = CPU count -1) in module initialization part,
and make them waiting (I use semaphores and self-made queue). At
nf_hook I
add tasks to queues and up() threads. As I in atomic context in
nf_iterate, I do in main thread computations, and then wait for
threads done with while() loop without scheduling. The problem is:
other threads sometimes don't wake up after up(). Sometimes, when I
press Enter key (SIC!) threads wake up, sometimes not.
Is there any right solvation for this problem?
^ permalink raw reply
* Re: [PATCH] net: sk_prot_alloc() should not blindly overwrite memory
From: Paul E. McKenney @ 2009-07-09 17:13 UTC (permalink / raw)
To: Eric Dumazet
Cc: David Miller, emil.s.tantilov, emils.tantilov, netdev,
jesse.brandeburg, jeffrey.t.kirsher, jolsa, Patrick McHardy
In-Reply-To: <4A5581C5.5070409@gmail.com>
On Thu, Jul 09, 2009 at 07:36:05AM +0200, Eric Dumazet wrote:
> Eric Dumazet a écrit :
> > David Miller a écrit :
> >> From: Eric Dumazet <eric.dumazet@gmail.com>
> >> Date: Wed, 08 Jul 2009 00:33:29 +0200
> >>
> >>> [PATCH] net: sk_prot_alloc() should not blindly overwrite memory
> >>>
> >>> Some sockets use SLAB_DESTROY_BY_RCU, and our RCU code rely that some
> >>> fields should not be blindly overwritten, even with null.
> >>>
> >>> These fields are sk->sk_refcnt and sk->sk_nulls_node.next
> >>>
> >>> Current sk_prot_alloc() implementation doesnt respect this hypothesis,
> >>> calling kmem_cache_alloc() with __GFP_ZERO and setting sk_refcnt to 1
> >>> instead of atomically increment it.
> >>>
> >>> Reported-by: Emil S Tantilov <emils.tantilov@gmail.com>
> >>> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> >> I've applied this but will wait for some more testing before
> >> I push it out for real to kernel.org
> >
> > Thanks David
> >
> > I forgot to CC Paul and Patrick, so I'll ask them to look at this patch.
> >
> > Patrick, a similar fix is needed in conntrack as well, we currently
> > uses "ct = kmem_cache_zalloc(nf_conntrack_cachep, gfp);" and thus
> > overwrite struct hlist_nulls_node hnnode; contained
> > in "struct nf_conntrack_tuple_hash", while lockless readers still
> > potentialy need them. Setting hnnode.next to NULL is dangerous
> > since last bit is not set (not a nulls value), a reader could
> > try to dereference this NULL pointer and trap.
> >
> >
> > Here is the patch again so that Paul & Patrick can comment on it.
> >
> > I am not sure about the refcnt thing (blindly setting it to 0 again
> > should be OK in fact, since no reader should/can to the
> > atomic_inc_if_not_zero on it), but the nulls.next thing is problematic.
>
> Here is an updated and much simpler patch, taking care of sk_node.next being not set to 0
>
> This patch applies to >= 2.6.29 kernels
Does this one also need the rearrangement of struct elements in the
earlier patch? (And apologies about being slow to get to that one.)
Thanx, Paul
> [PATCH] net: sk_prot_alloc() should not blindly overwrite memory
>
> Some sockets use SLAB_DESTROY_BY_RCU, and our RCU code correctness
> depends on sk->sk_nulls_node.next being always valid. A NULL
> value is not allowed as it might fault a lockless reader.
>
> Current sk_prot_alloc() implementation doesnt respect this hypothesis,
> calling kmem_cache_alloc() with __GFP_ZERO. Just call memset() around
> the forbidden field.
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---
> diff --git a/net/core/sock.c b/net/core/sock.c
> index b0ba569..7b87ec0 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -939,8 +939,23 @@ static struct sock *sk_prot_alloc(struct proto *prot, gfp_t priority,
> struct kmem_cache *slab;
>
> slab = prot->slab;
> - if (slab != NULL)
> - sk = kmem_cache_alloc(slab, priority);
> + if (slab != NULL) {
> + sk = kmem_cache_alloc(slab, priority & ~__GFP_ZERO);
> + if (!sk)
> + return sk;
> + if (priority & __GFP_ZERO) {
> + /*
> + * caches using SLAB_DESTROY_BY_RCU should let
> + * sk_node.next un-modified. Special care is taken
> + * when initializing object to zero.
> + */
> + if (offsetof(struct sock, sk_node.next) != 0)
> + memset(sk, 0, offsetof(struct sock, sk_node.next));
> + memset(&sk->sk_node.pprev, 0,
> + prot->obj_size - offsetof(struct sock,
> + sk_node.pprev));
> + }
> + }
> else
> sk = kmalloc(prot->obj_size, priority);
>
^ permalink raw reply
* Re: Soft-Lockup/Race in networking in 2.6.31-rc1+195 ( possibly?caused by netem)
From: Andres Freund @ 2009-07-09 16:46 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Jarek Poplawski, Joao Correia, Arun R Bharadwaj,
Stephen Hemminger, netdev, LKML, Patrick McHardy, Peter Zijlstra
In-Reply-To: <alpine.LFD.2.00.0907091801130.24704@localhost.localdomain>
On Thursday 09 July 2009 18:01:56 Thomas Gleixner wrote:
> Andres,
>
> On Thu, 9 Jul 2009, Andres Freund wrote:
> > On Thursday 09 July 2009 16:28:05 Thomas Gleixner wrote:
> > > Please test them separate from each other. The one I sent in this
> > > thread was just for narrowing down the issue, but I'm now quite sure
> > > that they really hit the issue which is addressed by the hrtimer
> > > patch.
> >
> > No crash yet. 15min running (seconds to a minute before).
> >
> > Will let it run for some hours to be sure.
>
> Which one of the patches are you testing ?
Your "real" one, i.e. de907e8432b08f2d5966c36e0747e97c0e596810
1h30m now...
Andres
^ permalink raw reply
* Re: Soft-Lockup/Race in networking in 2.6.31-rc1+195 ( possibly?caused by netem)
From: Thomas Gleixner @ 2009-07-09 16:01 UTC (permalink / raw)
To: Andres Freund
Cc: Jarek Poplawski, Joao Correia, Arun R Bharadwaj,
Stephen Hemminger, netdev, LKML, Patrick McHardy, Peter Zijlstra
In-Reply-To: <200907091728.23367.andres@anarazel.de>
Andres,
On Thu, 9 Jul 2009, Andres Freund wrote:
> On Thursday 09 July 2009 16:28:05 Thomas Gleixner wrote:
> > Please test them separate from each other. The one I sent in this
> > thread was just for narrowing down the issue, but I'm now quite sure
> > that they really hit the issue which is addressed by the hrtimer
> > patch.
> No crash yet. 15min running (seconds to a minute before).
>
> Will let it run for some hours to be sure.
Which one of the patches are you testing ?
Thanks,
tglx
^ permalink raw reply
* Re: use after free bug in socket code
From: Herbert Xu @ 2009-07-09 15:45 UTC (permalink / raw)
To: Lothar Waßmann; +Cc: davem, netdev, urs.thuermann, oliver.hartkopp
In-Reply-To: <19028.16049.907160.45293@ipc1.ka-ro>
Lothar Waßmann <LW@karo-electronics.de> wrote:
>
> So, could you point me to the place where the reference count of the
> socket object is being incremented when a struct sock is associated
> with it?
It's implicit. Anyway, you should remodel your release function
on a working protocol.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: Soft-Lockup/Race in networking in 2.6.31-rc1+195 ( possibly?caused by netem)
From: Andres Freund @ 2009-07-09 15:28 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Jarek Poplawski, Joao Correia, Arun R Bharadwaj,
Stephen Hemminger, netdev, LKML, Patrick McHardy, Peter Zijlstra
In-Reply-To: <alpine.LFD.2.00.0907091625540.24704@localhost.localdomain>
Hi,
On Thursday 09 July 2009 16:28:05 Thomas Gleixner wrote:
> On Thu, 9 Jul 2009, Jarek Poplawski wrote:
> > On Thu, Jul 09, 2009 at 04:15:28PM +0200, Thomas Gleixner wrote:
> > > On Thu, 9 Jul 2009, Jarek Poplawski wrote:
> > > > On Thu, Jul 09, 2009 at 02:03:50PM +0200, Thomas Gleixner wrote:
> > > > > On Thu, 9 Jul 2009, Jarek Poplawski wrote:
> > > > > > > I have the feeling that the code relies on some implicit cpu
> > > > > > > boundness, which is not longer guaranteed with the timer
> > > > > > > migration changes, but that's a question for the network
> > > > > > > experts.
> > > > > >
> > > > > > As a matter of fact, I've just looked at this __netif_schedule(),
> > > > > > which really is cpu bound, so you might be 100% right.
> > > > >
> > > > > So the watchdog is the one which causes the trouble. The patch
> > > > > below should fix this.
> > > >
> > > > I hope so. On the other hand it seems it should work with this
> > > > migration yet, so it probably needs additional debugging.
> > >
> > > Right. I just provided the patch to narrow down the problem, but
> > > please test the fix of the hrtimer migration code which I sent out a
> > > bit earlier: http://lkml.org/lkml/2009/7/9/150
> > >
> > > It fixes a possible endless loop in the timer code which is related to
> > > the migration changes. Looking at the backtraces of the spinlock
> > > lockup I think that is what you hit.
> >
> > Actually, Andres and Joao hit this, and I hope they'll try these two
> > patches.
>
> Please test them separate from each other. The one I sent in this
> thread was just for narrowing down the issue, but I'm now quite sure
> that they really hit the issue which is addressed by the hrtimer
> patch.
No crash yet. 15min running (seconds to a minute before).
Will let it run for some hours to be sure.
Nice!
Andres
^ permalink raw reply
* patch acceptance question
From: Don Fry @ 2009-07-09 15:21 UTC (permalink / raw)
To: David Miller, netdev
I was surprised today to see a change to pcnet32.c in 2.6.31-rc2-git4
which maybe I missed yesterday on netdev. Most of the changes are okay,
but one in particular will result in some ugly printing.
What is the path that (probably janitorial) changes take that get
included in mainline? Had I seen it before it went in I would have
commented on it then.
Old:
pcnet32: PCnet/FAST+ 79C972 at 0x1020, 00:0c:46:53:8d:0c
tx_start_pt(0x0c00):~220 bytes, BCR18(9861):BurstWrEn BurstRdEn
NoUFlow
SRAMSIZE=0x1700, SRAM_BND=0x0800, assigned IRQ 21.
New:
pcnet32: PCnet/FAST+ 79C972 at 0x1020, 00:0c:46:53:8d:0c<6> tx_start_pt
(0x0c00):~220 bytes, BCR18(9861):BurstWrEn BurstRdEn NoUFlow
SRAMSIZE=0x1700, SRAM_BND=0x0800, assigned IRQ 21.
Thanks,
Don Fry
^ permalink raw reply
* Re: [PATCH] netpoll: Fix carrier detection for drivers that are using phylib
From: Matt Mackall @ 2009-07-09 15:06 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Linus Torvalds, Anton Vorontsov, Andrew Morton, oleg, mingo,
linux-kernel, netdev
In-Reply-To: <1247151079.12784.10.camel@twins>
On Thu, 2009-07-09 at 16:51 +0200, Peter Zijlstra wrote:
> > Does anything actually use scheduler_running yet? Perhaps my tree is
> > old.
>
> # git grep scheduler_running
> kernel/sched.c:static __read_mostly int scheduler_running;
> kernel/sched.c: scheduler_running = 1;
> kernel/sched_rt.c: if (unlikely(!scheduler_running))
> kernel/sched_rt.c: if (unlikely(!scheduler_running))
>
> If memory serves there used to be more, but I think that migrated into
> kernel/sched_clock.c, which has sched_clock_running.
The static in the first line makes that a bit of a head-scratcher? Oh, I
see: it's living in sin. Going to avert my eyes for now.
--
http://selenic.com : development and support for Mercurial and Linux
^ permalink raw reply
* Re: [PATCH] netpoll: Fix carrier detection for drivers that are using phylib
From: Peter Zijlstra @ 2009-07-09 14:51 UTC (permalink / raw)
To: Matt Mackall
Cc: Linus Torvalds, Anton Vorontsov, Andrew Morton, oleg, mingo,
linux-kernel, netdev
In-Reply-To: <1247150580.21295.937.camel@calx>
On Thu, 2009-07-09 at 09:43 -0500, Matt Mackall wrote:
> Yeah, that's what I had in mind. Probably throw in a define:
>
> /* for disabling scheduling in early boot */
> #define PREEMPT_EARLY (1 + PREEMPT_ACTIVE)
>
> and slap a comment on the sub_preempt_count().
Right, and visit all the other arch init code ;-)
I'll wait to see if Ingo has anything to say about it and then complete
this thing.
> Does anything actually use scheduler_running yet? Perhaps my tree is
> old.
# git grep scheduler_running
kernel/sched.c:static __read_mostly int scheduler_running;
kernel/sched.c: scheduler_running = 1;
kernel/sched_rt.c: if (unlikely(!scheduler_running))
kernel/sched_rt.c: if (unlikely(!scheduler_running))
If memory serves there used to be more, but I think that migrated into
kernel/sched_clock.c, which has sched_clock_running.
> Also, might_sleep's use of system_state probably bears revisiting.
Yeah, all that code is from long before we had scheduler_running (which
was introduced around CFS/.23).
^ permalink raw reply
* Re: [PATCH] netpoll: Fix carrier detection for drivers that are using phylib
From: Matt Mackall @ 2009-07-09 14:43 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Linus Torvalds, Anton Vorontsov, Andrew Morton, oleg, mingo,
linux-kernel, netdev
In-Reply-To: <1247149862.12784.6.camel@twins>
On Thu, 2009-07-09 at 16:31 +0200, Peter Zijlstra wrote:
> On Thu, 2009-07-09 at 09:18 -0500, Matt Mackall wrote:
> >
> > Sorry if I was unclear. I'm suggesting setting the count so the existing
> > PREEMPT_ACTIVE test here fires:
> >
> > int __sched _cond_resched(void)
> > {
> > if (need_resched() && !(preempt_count() & PREEMPT_ACTIVE) &&
> > system_state == SYSTEM_RUNNING) {
> > __cond_resched();
> > return 1;
> > }
> > return 0;
> > }
>
> Right, /me read preempt and thought a simple preempt inc but didn't read
> the code. Shame on me.
>
> So something like (utterly untested and such)
Yeah, that's what I had in mind. Probably throw in a define:
/* for disabling scheduling in early boot */
#define PREEMPT_EARLY (1 + PREEMPT_ACTIVE)
and slap a comment on the sub_preempt_count().
Does anything actually use scheduler_running yet? Perhaps my tree is
old.
Also, might_sleep's use of system_state probably bears revisiting.
--
http://selenic.com : development and support for Mercurial and Linux
^ permalink raw reply
* Re: [PATCH] net: ip_push_pending_frames() fix
From: Eric Dumazet @ 2009-07-09 14:38 UTC (permalink / raw)
To: Tantilov, Emil S
Cc: David Miller, emils.tantilov@gmail.com, netdev@vger.kernel.org,
Brandeburg, Jesse, Kirsher, Jeffrey T, jolsa@redhat.com
In-Reply-To: <EA929A9653AAE14F841771FB1DE5A1365F8AD4E64E@rrsmsx501.amr.corp.intel.com>
Tantilov, Emil S a écrit :
> Eric Dumazet wrote:
>> Tantilov, Emil S a écrit :
>>> Eric Dumazet wrote:
>>>> David Miller a écrit :
>>>>> From: "Tantilov, Emil S" <emil.s.tantilov@intel.com>
>>>>> Date: Wed, 8 Jul 2009 11:02:22 -0600
>>>>>
>>>>>> Still seeing traces during the test even with this patch applied:
>>>>>>
>>>>>> [ 1089.430093] ------------[ cut here ]------------
>>>>>> [ 1089.435667] WARNING: at include/net/sock.h:423
>>>>>> udp_lib_unhash+0x73/0xa0() [ 1089.435670] Hardware name: S5520HC
>>>>> Ok I'll back this out for now, needs more investigation
>>>>> obviously.
>>>> Hmm... I never said it was supposed to fix Emil problem, just that
>>>> I discovered one potential problem by code inspection.
>>>>
>>>> I could not find yet sk_refcnt mismatch.
>>>> As we do less atomic ops per packet than before, some old bug could
>>>> surface now...
>>>>
>>>> Emil, is it easy to reproduce this problem, considering I have a
>>>> similar platform than yours (dual quad core machine, E5450 cpus @
>>>> 3GHz) ?
>>> Eric,
>>>
>>> It should be easy to reproduce. At least I have been able to
>>> consistently
>>> reproduce it on several different systems with different drivers
>>> (e1000, e1000e, igb).
>>>
>>> The test I'm running is a mix of IPV4/6 TCP/UDP traffic with netperf
>>> (also mixing different types TCP/UDP_STREAM, TCP_MAERTS, TCP_UDP_RR
>>> etc). How much this matters I don't know - it's possible that just
>>> UDP traffic would do it. I also think it may have something to do
>>> with IPv6 because of the trace, but I am not sure.
>>>
>>> If you need more information let me know.
>>>
>> OK thanks, this was helpful, corking or not corking, that is the
>> question :)
>>
>> I think ip6_push_pending_frames() & ip_push_pending_frames
>> have a problem after recent commit
>> 2b85a34e911bf483c27cfdd124aeb1605145dc80 (net: No more expensive
>> sock_hold()/sock_put() on each tx)
>>
>> [PATCH] net: ip_push_pending_frames() fix
>>
>> After commit 2b85a34e911bf483c27cfdd124aeb1605145dc80
>> (net: No more expensive sock_hold()/sock_put() on each tx)
>> we do not take any more references on sk->sk_refcnt on outgoing
>> packets.
>>
>> I forgot to delete two __sock_put() from ip_push_pending_frames()
>> and ip6_push_pending_frames().
>>
>> Reported-by: Emil S Tantilov <emils.tantilov@gmail.com>
>> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>> ---
>> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
>> index 2470262..7d08210 100644
>> --- a/net/ipv4/ip_output.c
>> +++ b/net/ipv4/ip_output.c
>> @@ -1243,7 +1243,6 @@ int ip_push_pending_frames(struct sock *sk)
>> skb->len += tmp_skb->len;
>> skb->data_len += tmp_skb->len;
>> skb->truesize += tmp_skb->truesize;
>> - __sock_put(tmp_skb->sk);
>> tmp_skb->destructor = NULL;
>> tmp_skb->sk = NULL;
>> }
>> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
>> index 7c76e3d..87f8419 100644
>> --- a/net/ipv6/ip6_output.c
>> +++ b/net/ipv6/ip6_output.c
>> @@ -1484,7 +1484,6 @@ int ip6_push_pending_frames(struct sock *sk)
>> skb->len += tmp_skb->len;
>> skb->data_len += tmp_skb->len;
>> skb->truesize += tmp_skb->truesize;
>> - __sock_put(tmp_skb->sk);
>> tmp_skb->destructor = NULL;
>> tmp_skb->sk = NULL;
>> }
>
> Thanks Eric,
>
> With this patch the test ran all night without issues.
>
Thanks a lot Emil for testing and your feedback.
David, could you please add another tag ?
Tested-by: Emil S Tantilov <emils.tantilov@gmail.com>
Thanks
^ permalink raw reply
* RE: [PATCH] net: ip_push_pending_frames() fix
From: Tantilov, Emil S @ 2009-07-09 14:32 UTC (permalink / raw)
To: Eric Dumazet
Cc: David Miller, emils.tantilov@gmail.com, netdev@vger.kernel.org,
Brandeburg, Jesse, Kirsher, Jeffrey T, jolsa@redhat.com
In-Reply-To: <4A5537DA.1060200@gmail.com>
Eric Dumazet wrote:
> Tantilov, Emil S a écrit :
>> Eric Dumazet wrote:
>>> David Miller a écrit :
>>>> From: "Tantilov, Emil S" <emil.s.tantilov@intel.com>
>>>> Date: Wed, 8 Jul 2009 11:02:22 -0600
>>>>
>>>>> Still seeing traces during the test even with this patch applied:
>>>>>
>>>>> [ 1089.430093] ------------[ cut here ]------------
>>>>> [ 1089.435667] WARNING: at include/net/sock.h:423
>>>>> udp_lib_unhash+0x73/0xa0() [ 1089.435670] Hardware name: S5520HC
>>>> Ok I'll back this out for now, needs more investigation
>>>> obviously.
>>> Hmm... I never said it was supposed to fix Emil problem, just that
>>> I discovered one potential problem by code inspection.
>>>
>>> I could not find yet sk_refcnt mismatch.
>>> As we do less atomic ops per packet than before, some old bug could
>>> surface now...
>>>
>>> Emil, is it easy to reproduce this problem, considering I have a
>>> similar platform than yours (dual quad core machine, E5450 cpus @
>>> 3GHz) ?
>>
>> Eric,
>>
>> It should be easy to reproduce. At least I have been able to
>> consistently
>> reproduce it on several different systems with different drivers
>> (e1000, e1000e, igb).
>>
>> The test I'm running is a mix of IPV4/6 TCP/UDP traffic with netperf
>> (also mixing different types TCP/UDP_STREAM, TCP_MAERTS, TCP_UDP_RR
>> etc). How much this matters I don't know - it's possible that just
>> UDP traffic would do it. I also think it may have something to do
>> with IPv6 because of the trace, but I am not sure.
>>
>> If you need more information let me know.
>>
>
> OK thanks, this was helpful, corking or not corking, that is the
> question :)
>
> I think ip6_push_pending_frames() & ip_push_pending_frames
> have a problem after recent commit
> 2b85a34e911bf483c27cfdd124aeb1605145dc80 (net: No more expensive
> sock_hold()/sock_put() on each tx)
>
> [PATCH] net: ip_push_pending_frames() fix
>
> After commit 2b85a34e911bf483c27cfdd124aeb1605145dc80
> (net: No more expensive sock_hold()/sock_put() on each tx)
> we do not take any more references on sk->sk_refcnt on outgoing
> packets.
>
> I forgot to delete two __sock_put() from ip_push_pending_frames()
> and ip6_push_pending_frames().
>
> Reported-by: Emil S Tantilov <emils.tantilov@gmail.com>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---
> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
> index 2470262..7d08210 100644
> --- a/net/ipv4/ip_output.c
> +++ b/net/ipv4/ip_output.c
> @@ -1243,7 +1243,6 @@ int ip_push_pending_frames(struct sock *sk)
> skb->len += tmp_skb->len;
> skb->data_len += tmp_skb->len;
> skb->truesize += tmp_skb->truesize;
> - __sock_put(tmp_skb->sk);
> tmp_skb->destructor = NULL;
> tmp_skb->sk = NULL;
> }
> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> index 7c76e3d..87f8419 100644
> --- a/net/ipv6/ip6_output.c
> +++ b/net/ipv6/ip6_output.c
> @@ -1484,7 +1484,6 @@ int ip6_push_pending_frames(struct sock *sk)
> skb->len += tmp_skb->len;
> skb->data_len += tmp_skb->len;
> skb->truesize += tmp_skb->truesize;
> - __sock_put(tmp_skb->sk);
> tmp_skb->destructor = NULL;
> tmp_skb->sk = NULL;
> }
Thanks Eric,
With this patch the test ran all night without issues.
Emil
^ permalink raw reply
* Re: [PATCH] netpoll: Fix carrier detection for drivers that are using phylib
From: Peter Zijlstra @ 2009-07-09 14:31 UTC (permalink / raw)
To: Matt Mackall
Cc: Linus Torvalds, Anton Vorontsov, Andrew Morton, oleg, mingo,
linux-kernel, netdev
In-Reply-To: <1247149093.21295.915.camel@calx>
On Thu, 2009-07-09 at 09:18 -0500, Matt Mackall wrote:
>
> Sorry if I was unclear. I'm suggesting setting the count so the existing
> PREEMPT_ACTIVE test here fires:
>
> int __sched _cond_resched(void)
> {
> if (need_resched() && !(preempt_count() & PREEMPT_ACTIVE) &&
> system_state == SYSTEM_RUNNING) {
> __cond_resched();
> return 1;
> }
> return 0;
> }
Right, /me read preempt and thought a simple preempt inc but didn't read
the code. Shame on me.
So something like (utterly untested and such)
---
arch/x86/include/asm/thread_info.h | 2 +-
kernel/sched.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
index b078352..7b5dbce 100644
--- a/arch/x86/include/asm/thread_info.h
+++ b/arch/x86/include/asm/thread_info.h
@@ -49,7 +49,7 @@ struct thread_info {
.exec_domain = &default_exec_domain, \
.flags = 0, \
.cpu = 0, \
- .preempt_count = 1, \
+ .preempt_count = 1 + PREEMPT_ACTIVE, \
.addr_limit = KERNEL_DS, \
.restart_block = { \
.fn = do_no_restart_syscall, \
diff --git a/kernel/sched.c b/kernel/sched.c
index fd3ac58..8e81162 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -6599,8 +6599,7 @@ static void __cond_resched(void)
int __sched _cond_resched(void)
{
- if (need_resched() && !(preempt_count() & PREEMPT_ACTIVE) &&
- system_state == SYSTEM_RUNNING) {
+ if (need_resched() && !(preempt_count() & PREEMPT_ACTIVE)) {
__cond_resched();
return 1;
}
@@ -9422,6 +9421,7 @@ void __init sched_init(void)
perf_counter_init();
scheduler_running = 1;
+ sub_preempt_count(PREEMPT_ACTIVE);
}
#ifdef CONFIG_DEBUG_SPINLOCK_SLEEP
^ permalink raw reply related
* Re: Soft-Lockup/Race in networking in 2.6.31-rc1+195 ( possibly?caused by netem)
From: Thomas Gleixner @ 2009-07-09 14:28 UTC (permalink / raw)
To: Jarek Poplawski
Cc: Andres Freund, Joao Correia, Arun R Bharadwaj, Stephen Hemminger,
netdev, LKML, Patrick McHardy, Peter Zijlstra
In-Reply-To: <20090709142414.GC3651@ami.dom.local>
On Thu, 9 Jul 2009, Jarek Poplawski wrote:
> On Thu, Jul 09, 2009 at 04:15:28PM +0200, Thomas Gleixner wrote:
> > On Thu, 9 Jul 2009, Jarek Poplawski wrote:
> > > On Thu, Jul 09, 2009 at 02:03:50PM +0200, Thomas Gleixner wrote:
> > > > On Thu, 9 Jul 2009, Jarek Poplawski wrote:
> > > > > >
> > > > > > I have the feeling that the code relies on some implicit cpu
> > > > > > boundness, which is not longer guaranteed with the timer migration
> > > > > > changes, but that's a question for the network experts.
> > > > >
> > > > > As a matter of fact, I've just looked at this __netif_schedule(),
> > > > > which really is cpu bound, so you might be 100% right.
> > > >
> > > > So the watchdog is the one which causes the trouble. The patch below
> > > > should fix this.
> > >
> > > I hope so. On the other hand it seems it should work with this
> > > migration yet, so it probably needs additional debugging.
> >
> > Right. I just provided the patch to narrow down the problem, but
> > please test the fix of the hrtimer migration code which I sent out a
> > bit earlier: http://lkml.org/lkml/2009/7/9/150
> >
> > It fixes a possible endless loop in the timer code which is related to
> > the migration changes. Looking at the backtraces of the spinlock
> > lockup I think that is what you hit.
>
> Actually, Andres and Joao hit this, and I hope they'll try these two
> patches.
Please test them separate from each other. The one I sent in this
thread was just for narrowing down the issue, but I'm now quite sure
that they really hit the issue which is addressed by the hrtimer
patch.
Thanks,
tglx
^ permalink raw reply
* Re: Soft-Lockup/Race in networking in 2.6.31-rc1+195 ( possibly?caused by netem)
From: Joao Correia @ 2009-07-09 14:25 UTC (permalink / raw)
To: Jarek Poplawski
Cc: Andres Freund, Arun R Bharadwaj, Stephen Hemminger, netdev, LKML,
Patrick McHardy, Peter Zijlstra, Thomas Gleixner
In-Reply-To: <20090709142414.GC3651@ami.dom.local>
On Thu, Jul 9, 2009 at 3:24 PM, Jarek Poplawski<jarkao2@gmail.com> wrote:
> On Thu, Jul 09, 2009 at 04:15:28PM +0200, Thomas Gleixner wrote:
>> On Thu, 9 Jul 2009, Jarek Poplawski wrote:
>> > On Thu, Jul 09, 2009 at 02:03:50PM +0200, Thomas Gleixner wrote:
>> > > On Thu, 9 Jul 2009, Jarek Poplawski wrote:
>> > > > >
>> > > > > I have the feeling that the code relies on some implicit cpu
>> > > > > boundness, which is not longer guaranteed with the timer migration
>> > > > > changes, but that's a question for the network experts.
>> > > >
>> > > > As a matter of fact, I've just looked at this __netif_schedule(),
>> > > > which really is cpu bound, so you might be 100% right.
>> > >
>> > > So the watchdog is the one which causes the trouble. The patch below
>> > > should fix this.
>> >
>> > I hope so. On the other hand it seems it should work with this
>> > migration yet, so it probably needs additional debugging.
>>
>> Right. I just provided the patch to narrow down the problem, but
>> please test the fix of the hrtimer migration code which I sent out a
>> bit earlier: http://lkml.org/lkml/2009/7/9/150
>>
>> It fixes a possible endless loop in the timer code which is related to
>> the migration changes. Looking at the backtraces of the spinlock
>> lockup I think that is what you hit.
>
> Actually, Andres and Joao hit this, and I hope they'll try these two
> patches.
>
> Thanks,
> Jarek P.
>
I can only try later on today. Will post back as soon as i do it.
Joao Correia
^ permalink raw reply
* Re: Soft-Lockup/Race in networking in 2.6.31-rc1+195 ( possibly?caused by netem)
From: Jarek Poplawski @ 2009-07-09 14:24 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Andres Freund, Joao Correia, Arun R Bharadwaj, Stephen Hemminger,
netdev, LKML, Patrick McHardy, Peter Zijlstra
In-Reply-To: <alpine.LFD.2.00.0907091601120.24704@localhost.localdomain>
On Thu, Jul 09, 2009 at 04:15:28PM +0200, Thomas Gleixner wrote:
> On Thu, 9 Jul 2009, Jarek Poplawski wrote:
> > On Thu, Jul 09, 2009 at 02:03:50PM +0200, Thomas Gleixner wrote:
> > > On Thu, 9 Jul 2009, Jarek Poplawski wrote:
> > > > >
> > > > > I have the feeling that the code relies on some implicit cpu
> > > > > boundness, which is not longer guaranteed with the timer migration
> > > > > changes, but that's a question for the network experts.
> > > >
> > > > As a matter of fact, I've just looked at this __netif_schedule(),
> > > > which really is cpu bound, so you might be 100% right.
> > >
> > > So the watchdog is the one which causes the trouble. The patch below
> > > should fix this.
> >
> > I hope so. On the other hand it seems it should work with this
> > migration yet, so it probably needs additional debugging.
>
> Right. I just provided the patch to narrow down the problem, but
> please test the fix of the hrtimer migration code which I sent out a
> bit earlier: http://lkml.org/lkml/2009/7/9/150
>
> It fixes a possible endless loop in the timer code which is related to
> the migration changes. Looking at the backtraces of the spinlock
> lockup I think that is what you hit.
Actually, Andres and Joao hit this, and I hope they'll try these two
patches.
Thanks,
Jarek P.
^ permalink raw reply
* Re: [PATCH] netpoll: Fix carrier detection for drivers that are using phylib
From: Matt Mackall @ 2009-07-09 14:18 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Linus Torvalds, Anton Vorontsov, Andrew Morton, oleg, mingo,
linux-kernel, netdev
In-Reply-To: <1247147206.7439.2.camel@twins>
On Thu, 2009-07-09 at 15:46 +0200, Peter Zijlstra wrote:
> On Thu, 2009-07-09 at 08:26 -0500, Matt Mackall wrote:
> > On Wed, 2009-07-08 at 17:01 -0700, Linus Torvalds wrote:
> > >
> > > On Thu, 9 Jul 2009, Anton Vorontsov wrote:
> > > >
> > > > The netpoll code is using msleep() just a few lines below cond_resched(),
> > > > so we won't make things worse. ;-)
> > >
> > > Yeah. That function is definitely sleeping. It does things like
> > > kmalloc(GFP_KERNEL), rtnl_lock() and synchronize_rcu() etc too, so an
> > > added msleep() is the least of our problems.
> > >
> > > Afaik, it's called from a bog-standard "module_init()", which happens late
> > > enough that everything works.
> > >
> > > In fact, I wonder if we should set SYSTEM_RUNNING much earlier - _before_
> > > doing the whole "do_initcalls()".
> >
> > Well there are two ways of consistently defining SYSTEM_RUNNING:
> >
> > a) define it with reference to the well-understood notion of booting vs
> > running and don't switch it until handing off to init
>
> This makes the most sense IMHO.
>
> > b) define it with reference to its usage by an arbitrary user like
> > cond_resched()
> >
> > In the latter case, we obviously need to move it to the earliest point
> > that scheduling is possible. But there are a number of things like
> >
> > http://lxr.linux.no/linux+v2.6.30/kernel/printk.c#L228
> >
> > that assume the definition is actually (a). We're currently within a
> > couple lines of a strict definition of (a) already, so I actually think
> > cond_resched() is just wrong (and we already know it broke a
> > previously-working user). It should perhaps be using another private
> > flag that gets set as soon as scheduling is up and running.
>
> Right as mentioned before in this thread, we grew scheduler_running a
> while back which could be used for this.
>
> > But I'd actually go further and say that it's unfortunate to be checking
> > extra flags in such an important inline, especially since the check is
> > false for all but the first couple seconds of run time. Seems like we
> > could avoid adding an extra check by artificially elevating the preempt
> > count in early boot (or at compile time) then dropping it when
> > scheduling becomes available.
>
> Calling cond_resched() and co when !preemptable is an error so this
> wouldn't actually work.
Sorry if I was unclear. I'm suggesting setting the count so the existing
PREEMPT_ACTIVE test here fires:
int __sched _cond_resched(void)
{
if (need_resched() && !(preempt_count() & PREEMPT_ACTIVE) &&
system_state == SYSTEM_RUNNING) {
__cond_resched();
return 1;
}
return 0;
}
That should be kosher.
--
http://selenic.com : development and support for Mercurial and Linux
^ permalink raw reply
* Re: Soft-Lockup/Race in networking in 2.6.31-rc1+195 ( possibly?caused by netem)
From: Thomas Gleixner @ 2009-07-09 14:15 UTC (permalink / raw)
To: Jarek Poplawski
Cc: Andres Freund, Joao Correia, Arun R Bharadwaj, Stephen Hemminger,
netdev, LKML, Patrick McHardy, Peter Zijlstra
In-Reply-To: <20090709132256.GB3651@ami.dom.local>
On Thu, 9 Jul 2009, Jarek Poplawski wrote:
> On Thu, Jul 09, 2009 at 02:03:50PM +0200, Thomas Gleixner wrote:
> > On Thu, 9 Jul 2009, Jarek Poplawski wrote:
> > > >
> > > > I have the feeling that the code relies on some implicit cpu
> > > > boundness, which is not longer guaranteed with the timer migration
> > > > changes, but that's a question for the network experts.
> > >
> > > As a matter of fact, I've just looked at this __netif_schedule(),
> > > which really is cpu bound, so you might be 100% right.
> >
> > So the watchdog is the one which causes the trouble. The patch below
> > should fix this.
>
> I hope so. On the other hand it seems it should work with this
> migration yet, so it probably needs additional debugging.
Right. I just provided the patch to narrow down the problem, but
please test the fix of the hrtimer migration code which I sent out a
bit earlier: http://lkml.org/lkml/2009/7/9/150
It fixes a possible endless loop in the timer code which is related to
the migration changes. Looking at the backtraces of the spinlock
lockup I think that is what you hit.
spin_lock(root_lock);
qdisc_run(q);
__qdisc_run(q);
dequeue_skb(q);
q->dequeue(q);
qdisc_watchdog_schedule();
hrtimer_start();
switch_hrtimer_base(); <- loops forever
Now the other CPU is stuck in dev_xmit() spin_lock(root_lock)
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH] netpoll: Fix carrier detection for drivers that are using phylib
From: Peter Zijlstra @ 2009-07-09 13:46 UTC (permalink / raw)
To: Matt Mackall
Cc: Linus Torvalds, Anton Vorontsov, Andrew Morton, oleg, mingo,
linux-kernel, netdev
In-Reply-To: <1247145977.21295.899.camel@calx>
On Thu, 2009-07-09 at 08:26 -0500, Matt Mackall wrote:
> On Wed, 2009-07-08 at 17:01 -0700, Linus Torvalds wrote:
> >
> > On Thu, 9 Jul 2009, Anton Vorontsov wrote:
> > >
> > > The netpoll code is using msleep() just a few lines below cond_resched(),
> > > so we won't make things worse. ;-)
> >
> > Yeah. That function is definitely sleeping. It does things like
> > kmalloc(GFP_KERNEL), rtnl_lock() and synchronize_rcu() etc too, so an
> > added msleep() is the least of our problems.
> >
> > Afaik, it's called from a bog-standard "module_init()", which happens late
> > enough that everything works.
> >
> > In fact, I wonder if we should set SYSTEM_RUNNING much earlier - _before_
> > doing the whole "do_initcalls()".
>
> Well there are two ways of consistently defining SYSTEM_RUNNING:
>
> a) define it with reference to the well-understood notion of booting vs
> running and don't switch it until handing off to init
This makes the most sense IMHO.
> b) define it with reference to its usage by an arbitrary user like
> cond_resched()
>
> In the latter case, we obviously need to move it to the earliest point
> that scheduling is possible. But there are a number of things like
>
> http://lxr.linux.no/linux+v2.6.30/kernel/printk.c#L228
>
> that assume the definition is actually (a). We're currently within a
> couple lines of a strict definition of (a) already, so I actually think
> cond_resched() is just wrong (and we already know it broke a
> previously-working user). It should perhaps be using another private
> flag that gets set as soon as scheduling is up and running.
Right as mentioned before in this thread, we grew scheduler_running a
while back which could be used for this.
> But I'd actually go further and say that it's unfortunate to be checking
> extra flags in such an important inline, especially since the check is
> false for all but the first couple seconds of run time. Seems like we
> could avoid adding an extra check by artificially elevating the preempt
> count in early boot (or at compile time) then dropping it when
> scheduling becomes available.
Calling cond_resched() and co when !preemptable is an error so this
wouldn't actually work.
^ 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