* Re: fixing sk_stream_rfree()
From: David S. Miller @ 2006-04-15 9:03 UTC (permalink / raw)
To: netdev; +Cc: herbert
In-Reply-To: <20060415.012327.68977713.davem@davemloft.net>
From: "David S. Miller" <davem@davemloft.net>
Date: Sat, 15 Apr 2006 01:23:27 -0700 (PDT)
> I came up with one more possible approach, it goes something like
> this:
>
> 1) Get rid of the skb->destructor callback for TCP receive
> data.
>
> 2) Adjust sk_rmem_alloc and sk_forward_alloc explicitly when we add
> packets to sk_receive_queue/out_of_order_queue, and when we unlink
> them from sk_receive_queue and __kfree_skb() them up.
It turns out DCCP doesn't use the sk_forward_alloc memory
scheduling at least not for receive.
So, as food for thought, below is a comile-tested-only
implementation of the above idea. The basic transformations
are:
1) sk_eat_skb --> sk_stream_eat_skb which does the sk_forward_alloc
et al. accounting by hand which would have been done by the
skb->destructor sk_stream_rfree()
2) __skb_queue_purge --> sk_stream_eat_queue
3) sk_stream_set_owner_r --> sk_stream_charge_r which does
the accounting for a new receive skb but does not hook up
the destructor
4) unlink and free SKB not received to userspace yet -->
sk_stream_eat_skb()
... and then I noticed that ip_rcv() unshares the SKB, so
skb->users should always be 1 and therefore it is impossible
for sk_stream_rfree() to be called from a non socket locked
context.
And all of this was a wild goose chase, oh well :-)
So the BUG_ON(!sk_forward_alloc) problem is elsewhere. :-/
diff --git a/arch/x86_64/kernel/functionlist b/arch/x86_64/kernel/functionlist
index 2bcebdc..da3379a 100644
--- a/arch/x86_64/kernel/functionlist
+++ b/arch/x86_64/kernel/functionlist
@@ -751,7 +751,6 @@
*(.text.strcmp)
*(.text.steal_locks)
*(.text.sock_create)
-*(.text.sk_stream_rfree)
*(.text.sk_stream_mem_schedule)
*(.text.skip_atoi)
*(.text.sk_alloc)
diff --git a/include/net/sock.h b/include/net/sock.h
index af2b054..54d5a2f 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -442,14 +442,56 @@ static inline int sk_stream_memory_free(
return sk->sk_wmem_queued < sk->sk_sndbuf;
}
-extern void sk_stream_rfree(struct sk_buff *skb);
-
-static inline void sk_stream_set_owner_r(struct sk_buff *skb, struct sock *sk)
+/* Charge SKB as receive queue memory to SK. The socket must be locked
+ * when we get here.
+ */
+static inline void sk_stream_charge_r(struct sk_buff *skb, struct sock *sk)
{
- skb->sk = sk;
- skb->destructor = sk_stream_rfree;
atomic_add(skb->truesize, &sk->sk_rmem_alloc);
sk->sk_forward_alloc -= skb->truesize;
+}
+
+/* Release SKB as receive queue memory from SK. The socket must be
+ * locked when we get here.
+ */
+static inline void sk_stream_release_r(struct sk_buff *skb, struct sock *sk)
+{
+ atomic_sub(skb->truesize, &sk->sk_rmem_alloc);
+ sk->sk_forward_alloc += skb->truesize;
+}
+
+/**
+ * sk_stream_eat_skb - Release a skb if it is no longer needed
+ * @sk: socket to eat this skb from
+ * @skb: socket buffer to eat
+ * @queue: skb queue to unlink from
+ *
+ * This routine must be called with interrupts disabled or with the socket
+ * locked so that the sk_buff queue operation is ok.
+ */
+static inline void sk_stream_eat_skb(struct sk_buff *skb, struct sock *sk, struct sk_buff_head *queue)
+{
+ __skb_unlink(skb, queue);
+ sk_stream_release_r(skb, sk);
+ __kfree_skb(skb);
+}
+
+/*
+ * sk_stream_eat_queue - Release an entire queue of skbs, when no longer needed
+ * @sk: socket to eat these skbs from
+ * @queue: queue of skbs to eat
+ *
+ * This routine must be called with interrupts disabled or with the socket
+ * locked so that the sk_buff queue operation is ok.
+ */
+static inline void sk_stream_eat_queue(struct sock *sk, struct sk_buff_head *queue)
+{
+ struct sk_buff *skb;
+
+ while ((skb = __skb_dequeue(queue)) != NULL) {
+ sk_stream_release_r(skb, sk);
+ __kfree_skb(skb);
+ }
}
static inline void sk_stream_free_skb(struct sock *sk, struct sk_buff *skb)
diff --git a/net/core/stream.c b/net/core/stream.c
index 35e2525..a034f2d 100644
--- a/net/core/stream.c
+++ b/net/core/stream.c
@@ -172,16 +172,6 @@ do_interrupted:
EXPORT_SYMBOL(sk_stream_wait_memory);
-void sk_stream_rfree(struct sk_buff *skb)
-{
- struct sock *sk = skb->sk;
-
- atomic_sub(skb->truesize, &sk->sk_rmem_alloc);
- sk->sk_forward_alloc += skb->truesize;
-}
-
-EXPORT_SYMBOL(sk_stream_rfree);
-
int sk_stream_error(struct sock *sk, int flags, int err)
{
if (err == -EPIPE)
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 87f68e7..c45a9c2 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -1072,11 +1072,11 @@ int tcp_read_sock(struct sock *sk, read_
break;
}
if (skb->h.th->fin) {
- sk_eat_skb(sk, skb);
+ sk_stream_eat_skb(skb, sk, &sk->sk_receive_queue);
++seq;
break;
}
- sk_eat_skb(sk, skb);
+ sk_stream_eat_skb(skb, sk, &sk->sk_receive_queue);
if (!desc->count)
break;
}
@@ -1355,15 +1355,17 @@ skip_copy:
if (skb->h.th->fin)
goto found_fin_ok;
- if (!(flags & MSG_PEEK))
- sk_eat_skb(sk, skb);
+ if (!(flags & MSG_PEEK)) {
+ sk_stream_eat_skb(skb, sk, &sk->sk_receive_queue);
+ }
continue;
found_fin_ok:
/* Process the FIN. */
++*seq;
- if (!(flags & MSG_PEEK))
- sk_eat_skb(sk, skb);
+ if (!(flags & MSG_PEEK)) {
+ sk_stream_eat_skb(skb, sk, &sk->sk_receive_queue);
+ }
break;
} while (len > 0);
@@ -1489,6 +1491,7 @@ void tcp_close(struct sock *sk, long tim
u32 len = TCP_SKB_CB(skb)->end_seq - TCP_SKB_CB(skb)->seq -
skb->h.th->fin;
data_was_unread += len;
+ sk_stream_release_r(skb, sk);
__kfree_skb(skb);
}
@@ -1650,9 +1653,9 @@ int tcp_disconnect(struct sock *sk, int
sk->sk_err = ECONNRESET;
tcp_clear_xmit_timers(sk);
- __skb_queue_purge(&sk->sk_receive_queue);
+ sk_stream_eat_queue(sk, &sk->sk_receive_queue);
sk_stream_writequeue_purge(sk);
- __skb_queue_purge(&tp->out_of_order_queue);
+ sk_stream_eat_queue(sk, &tp->out_of_order_queue);
inet->dport = 0;
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 9f0cca4..5a30648 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -2875,7 +2875,7 @@ static void tcp_fin(struct sk_buff *skb,
/* It _is_ possible, that we have something out-of-order _after_ FIN.
* Probably, we should reset in this case. For now drop them.
*/
- __skb_queue_purge(&tp->out_of_order_queue);
+ sk_stream_eat_queue(sk, &tp->out_of_order_queue);
if (tp->rx_opt.sack_ok)
tcp_sack_reset(&tp->rx_opt);
sk_stream_mem_reclaim(sk);
@@ -3093,8 +3093,7 @@ static void tcp_ofo_queue(struct sock *s
if (!after(TCP_SKB_CB(skb)->end_seq, tp->rcv_nxt)) {
SOCK_DEBUG(sk, "ofo packet was already received \n");
- __skb_unlink(skb, &tp->out_of_order_queue);
- __kfree_skb(skb);
+ sk_stream_eat_skb(skb, sk, &tp->out_of_order_queue);
continue;
}
SOCK_DEBUG(sk, "ofo requeuing : rcv_next %X seq %X - %X\n",
@@ -3166,7 +3165,7 @@ queue_and_out:
!sk_stream_rmem_schedule(sk, skb))
goto drop;
}
- sk_stream_set_owner_r(skb, sk);
+ sk_stream_charge_r(skb, sk);
__skb_queue_tail(&sk->sk_receive_queue, skb);
}
tp->rcv_nxt = TCP_SKB_CB(skb)->end_seq;
@@ -3248,7 +3247,7 @@ drop:
SOCK_DEBUG(sk, "out of order segment: rcv_next %X seq %X - %X\n",
tp->rcv_nxt, TCP_SKB_CB(skb)->seq, TCP_SKB_CB(skb)->end_seq);
- sk_stream_set_owner_r(skb, sk);
+ sk_stream_charge_r(skb, sk);
if (!skb_peek(&tp->out_of_order_queue)) {
/* Initial out of order segment, build 1 SACK. */
@@ -3290,6 +3289,7 @@ drop:
before(seq, TCP_SKB_CB(skb1)->end_seq)) {
if (!after(end_seq, TCP_SKB_CB(skb1)->end_seq)) {
/* All the bits are present. Drop. */
+ sk_stream_release_r(skb, sk);
__kfree_skb(skb);
tcp_dsack_set(tp, seq, end_seq);
goto add_sack;
@@ -3311,9 +3311,8 @@ drop:
tcp_dsack_extend(tp, TCP_SKB_CB(skb1)->seq, end_seq);
break;
}
- __skb_unlink(skb1, &tp->out_of_order_queue);
tcp_dsack_extend(tp, TCP_SKB_CB(skb1)->seq, TCP_SKB_CB(skb1)->end_seq);
- __kfree_skb(skb1);
+ sk_stream_eat_skb(skb1, sk, &tp->out_of_order_queue);
}
add_sack:
@@ -3340,8 +3339,7 @@ tcp_collapse(struct sock *sk, struct sk_
/* No new bits? It is possible on ofo queue. */
if (!before(start, TCP_SKB_CB(skb)->end_seq)) {
struct sk_buff *next = skb->next;
- __skb_unlink(skb, list);
- __kfree_skb(skb);
+ sk_stream_eat_skb(skb, sk, list);
NET_INC_STATS_BH(LINUX_MIB_TCPRCVCOLLAPSED);
skb = next;
continue;
@@ -3387,7 +3385,7 @@ tcp_collapse(struct sock *sk, struct sk_
memcpy(nskb->cb, skb->cb, sizeof(skb->cb));
TCP_SKB_CB(nskb)->seq = TCP_SKB_CB(nskb)->end_seq = start;
__skb_insert(nskb, skb->prev, skb, list);
- sk_stream_set_owner_r(nskb, sk);
+ sk_stream_charge_r(nskb, sk);
/* Copy data, releasing collapsed skbs. */
while (copy > 0) {
@@ -3405,8 +3403,7 @@ tcp_collapse(struct sock *sk, struct sk_
}
if (!before(start, TCP_SKB_CB(skb)->end_seq)) {
struct sk_buff *next = skb->next;
- __skb_unlink(skb, list);
- __kfree_skb(skb);
+ sk_stream_eat_skb(skb, sk, list);
NET_INC_STATS_BH(LINUX_MIB_TCPRCVCOLLAPSED);
skb = next;
if (skb == tail || skb->h.th->syn || skb->h.th->fin)
@@ -3494,7 +3491,7 @@ static int tcp_prune_queue(struct sock *
/* First, purge the out_of_order queue. */
if (!skb_queue_empty(&tp->out_of_order_queue)) {
NET_INC_STATS_BH(LINUX_MIB_OFOPRUNED);
- __skb_queue_purge(&tp->out_of_order_queue);
+ sk_stream_eat_queue(sk, &tp->out_of_order_queue);
/* Reset SACK state. A conforming SACK implementation will
* do the same at a timeout based retransmit. When a connection
@@ -3703,10 +3700,8 @@ static void tcp_check_urg(struct sock *
tp->copied_seq != tp->rcv_nxt) {
struct sk_buff *skb = skb_peek(&sk->sk_receive_queue);
tp->copied_seq++;
- if (skb && !before(tp->copied_seq, TCP_SKB_CB(skb)->end_seq)) {
- __skb_unlink(skb, &sk->sk_receive_queue);
- __kfree_skb(skb);
- }
+ if (skb && !before(tp->copied_seq, TCP_SKB_CB(skb)->end_seq))
+ sk_stream_eat_skb(skb, sk, &sk->sk_receive_queue);
}
tp->urg_data = TCP_URG_NOTYET;
@@ -3950,7 +3945,7 @@ int tcp_rcv_established(struct sock *sk,
/* Bulk data transfer: receiver */
__skb_pull(skb,tcp_header_len);
__skb_queue_tail(&sk->sk_receive_queue, skb);
- sk_stream_set_owner_r(skb, sk);
+ sk_stream_charge_r(skb, sk);
tp->rcv_nxt = TCP_SKB_CB(skb)->end_seq;
}
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 672950e..e518d97 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1294,7 +1294,7 @@ int tcp_v4_destroy_sock(struct sock *sk)
sk_stream_writequeue_purge(sk);
/* Cleans up our, hopefully empty, out_of_order_queue. */
- __skb_queue_purge(&tp->out_of_order_queue);
+ sk_stream_eat_queue(sk, &tp->out_of_order_queue);
/* Clean prequeue, it must be empty really */
__skb_queue_purge(&tp->ucopy.prequeue);
^ permalink raw reply related
* Re: fixing sk_stream_rfree()
From: David S. Miller @ 2006-04-15 8:23 UTC (permalink / raw)
To: netdev; +Cc: herbert
In-Reply-To: <20060414.205927.13626300.davem@davemloft.net>
From: "David S. Miller" <davem@davemloft.net>
Date: Fri, 14 Apr 2006 20:59:27 -0700 (PDT)
> Any ideas? Come to think of it, __sk_stream_mem_reclaim() looks
> really racey even as-is.
I came up with one more possible approach, it goes something like
this:
1) Get rid of the skb->destructor callback for TCP receive
data.
2) Adjust sk_rmem_alloc and sk_forward_alloc explicitly when we add
packets to sk_receive_queue/out_of_order_queue, and when we unlink
them from sk_receive_queue and __kfree_skb() them up.
I think this would work and get rid of the unlocked accesses to
sk_forward_alloc. And it would do so without adding new overheads
(the other two ideas did, by adding locks or turning sk_forward_alloc
into an atomic_t).
The implementation needs a bit of thinking in order to not mess up all
the nice abstractions Arnaldo has created for stream sockets and the
interfaces we share between TCP and DCCP.
But actually DCCP needs this fix too, since it also uses
sk_forward_alloc, so building the solution into the shared interfaces
is desirable.
Too bad this will take a little bit of time to implement, I really
want to be able to cook up a much simpler test patch that the people
who can reproduce the BUG() can try out.
^ permalink raw reply
* Re: [patch] ipv4: initialize arp_tbl rw lock
From: David S. Miller @ 2006-04-15 7:34 UTC (permalink / raw)
To: heiko.carstens
Cc: shemminger, jgarzik, akpm, netdev, linux-kernel, fpavlic, davem
In-Reply-To: <20060415072745.GA17011@osiris.boeblingen.de.ibm.com>
From: Heiko Carstens <heiko.carstens@de.ibm.com>
Date: Sat, 15 Apr 2006 09:27:45 +0200
> Tried to figure out what is causing the delays I experienced when I replaced
> module_init() in af_inet.c with fs_initcall(). After all it turned out that
> synchronize_net() which is basicically nothing else than synchronize_rcu()
> sometimes takes several seconds to complete?! No idea why that is...
>
> callchain: inet_init() -> inet_register_protosw() -> synchronize_net()
The problem can't be rcu_init(), that gets done very early
in init/main.c
Maybe it's some timer or something else specific to s390?
It could also be that there's perhaps nothing to context
switch to, thus the RCU takes forever to "happen".
^ permalink raw reply
* Re: [patch] ipv4: initialize arp_tbl rw lock
From: Heiko Carstens @ 2006-04-15 7:27 UTC (permalink / raw)
To: David S. Miller
Cc: shemminger, jgarzik, akpm, netdev, linux-kernel, fpavlic, davem
In-Reply-To: <20060408.031404.111884281.davem@davemloft.net>
> > Ok, so the problem seems to be that inet_init gets called after qeth_init.
> > Looking at the top level Makefile this seems to be true for all network
> > drivers in drivers/net/ and drivers/s390/net/ since we have
> >
> > vmlinux-main := $(core-y) $(libs-y) $(drivers-y) $(net-y)
> >
> > The patch below works for me... I guess there must be a better way to make
> > sure that any networking driver's initcall is made before inet_init?
> >
> > Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
>
> We could make inet_init() a subsystem init but I vaguely recall
> that we were doing that at one point and it broke things for
> some reason.
>
> Perhaps fs_initcall() would work better. Or if that causes
> problems we could create a net_initcall() that sits between
> fs_initcall() and device_initcall().
>
> Or any other ideas?
Tried to figure out what is causing the delays I experienced when I replaced
module_init() in af_inet.c with fs_initcall(). After all it turned out that
synchronize_net() which is basicically nothing else than synchronize_rcu()
sometimes takes several seconds to complete?! No idea why that is...
callchain: inet_init() -> inet_register_protosw() -> synchronize_net()
^ permalink raw reply
* fixing sk_stream_rfree()
From: David S. Miller @ 2006-04-15 3:59 UTC (permalink / raw)
To: netdev; +Cc: herbert
Herbert, as you may have noticed we found some missing
locking in sk_stream_rfree(). It could explain the
"!sk_forward_alloc" BUG() we thought was caused by e1000's
TSO implementation and the Intel folks have provided enough
datapoints to prove that it is indeed not an e1000 specific
problem.
sk_stream_rfree() modifies sk->sk_forward_alloc without holding any
locks from the __kfree_skb() callback.
Now, _most_ of the time it is OK because we're being invoked with the
socket lock held as ACKs come back to liberate the transmit queue or
during socket shutdown which also holds the socket locked. (I did
audit this, but I may have missed something, please double check this
assertion)
However, if skb->users is incremented or similar, that entity could
end up being the context in which the skb is freed up and that will
not necessarily have the socket locked correctly when the
skb->destructor callback invokes sk_stream_rfree(). Consider tcpdump
or similar.
But we're stuck if we try to cure this:
1) We can't take bh_lock_sock(). Well, we could if we disabled
BH explicitly first but even then if the socket is locked
by the user we can't tell if that's the current cpu and there
is no easy way to handle deferring this work.
We really can't consider using a workqueue or something like
that to handle this, that's way too heavy handed.
2) We can't turn sk_forward_alloc easily into an atomic_t. The
masking operation in __sk_stream_mem_reclaim() does not translate
readily into an atomic_t op:
sk->sk_forward_alloc &= SK_STREAM_MEM_QUANTUM - 1;
That line has always driven me nuts, but I know that it is there
to handle partial page allocations existing when that function
is called.
I guess for #2 we could change those two lines into:
int n = atomic_read(&sk->sk_forward_alloc) /
SK_STREAM_MEM_QUANTUM;
atomic_sub(n, sk->sk_prot->memory_allocated);
sk->sk_forward_alloc -= n * SK_STREAM_MEM_QUANTUM;
in order to make it "atomic_t op" translatable.
But even if we did this with sk->sk_forward_alloc as an atomic_t
it would have the same kinds of races we're trying to cure here,
namely that we test the value and then act upon it while an unlocked
asynchronous context can modify the value in another thread of
execution in between the test and the action.
Any ideas? Come to think of it, __sk_stream_mem_reclaim() looks
really racey even as-is.
^ permalink raw reply
* Re: [e1000 debug] KERNEL: assertion (!sk_forward_alloc) failed...
From: David S. Miller @ 2006-04-14 23:53 UTC (permalink / raw)
To: jesse.brandeburg
Cc: bb, herbert, kernel, nipsy, jrlundgren, cat, djani22,
yoseph.basri, mykleb, olel, michal, chris, netdev,
jesse.brandeburg, E1000-devel, ak, jgarzik
In-Reply-To: <Pine.WNT.4.63.0604141553210.2116@jbrandeb-desk.amr.corp.intel.com>
From: Jesse Brandeburg <jesse.brandeburg@intel.com>
Date: Fri, 14 Apr 2006 15:55:10 -0700 (Pacific Daylight Time)
> I'm trying to isolate more of a reproduction case, I'll be sure to
> post if I can find anything with more detail.
I think I see the bug.
If tbench with large numbers of clients is part of what helps
reproduce it, the key might be hitting the memory limits in tcp_mem[]
and friends, or something to do with concurrent access to
sk->sk_forward_alloc.
I bet there is some race in there.
A lot of the action is in net/core/stream.c We modify
sk->sk_forward_alloc non-atomically but that should be ok
since we ought to be holding all of the correct locks when
we hit these accesses. But it is the first thing to audit.
Let's look at sk_stream_rfree() as that is invoked from SKB
freeing callbacks and is the most likely suspect for these
kinds of problems.
It is hooked up to the skb->destructor by sk_stream_set_owner_r() and
then invoked via __kfree_skb().
Nothing here takes any locks, and as stated above we modify
sk->sk_forward_alloc non-atomically, and this is therefore the bug.
Shit.
I'll think of how to fix this in the least invasive manner. I also
want to search the changelog history to see if this race was always
present or if it was "introduced".
Making sk->sk_forward_alloc an atomic_t would be incredibly expensive
so I'll try to find a way to avoid that. We may be able to just do
a bh_lock_sock()/bh_unlock_sock() around the body of sk_stream_rfree()
to fix this.
^ permalink raw reply
* Re: [PATCH 05/10] e1000: Update truesize with the length of the packet for packet split
From: Jesse Brandeburg @ 2006-04-14 23:52 UTC (permalink / raw)
To: David S. Miller
Cc: jesse.brandeburg, jgarzik, auke-jan.h.kok, netdev, john.ronciak,
Jeffrey.t.kirsher, auke
In-Reply-To: <20060414.164512.17280154.davem@davemloft.net>
On Fri, 14 Apr 2006, David S. Miller wrote:
> > they are not accounted for in the skb yet. the way e1000 works is it
> > pre-allocates all its 128 byte receive buffers using skb_alloc, and then
> > chains in any pages that are used for receive packets.
>
> Ok so you allocate the base skb->data memory using alloc_skb(). Then
> you add on pages and increment skb->truesize to account for the pages.
> And you do all of this for receive packets before they are passed up
> to the stack.
>
> Right?
yes, exactly.
> If so, then that should be OK.
>
> Jeff, I seem to have misunderstood what e1000 was doing, the patches
> changing skb->truesize should be fine.
Thanks
^ permalink raw reply
* Re: [PATCH 05/10] e1000: Update truesize with the length of the packet for packet split
From: David S. Miller @ 2006-04-14 23:45 UTC (permalink / raw)
To: jesse.brandeburg
Cc: jgarzik, auke-jan.h.kok, netdev, john.ronciak, Jeffrey.t.kirsher,
auke
In-Reply-To: <Pine.WNT.4.63.0604141555200.2116@jbrandeb-desk.amr.corp.intel.com>
From: Jesse Brandeburg <jesse.brandeburg@intel.com>
Date: Fri, 14 Apr 2006 16:04:21 -0700 (Pacific Daylight Time)
> On Fri, 14 Apr 2006, David S. Miller wrote:
>
> > From: Jesse Brandeburg <jesse.brandeburg@intel.com>
> > Date: Fri, 14 Apr 2006 15:43:02 -0700 (Pacific Daylight Time)
> >
> > > Please help me understand how you think it should work when we have a
> > > device that wants to receive a packet using header in the skb->data, and
> > > application data in the ->frags[]?
> >
> > If you're removing the pages from ->frags[] and you already accounted
> > for them in skb->truesize, then yes I guess it could be correct to
> > subtract it back out.
>
> they are not accounted for in the skb yet. the way e1000 works is it
> pre-allocates all its 128 byte receive buffers using skb_alloc, and then
> chains in any pages that are used for receive packets.
Ok so you allocate the base skb->data memory using alloc_skb(). Then
you add on pages and increment skb->truesize to account for the pages.
And you do all of this for receive packets before they are passed up
to the stack.
Right?
If so, then that should be OK.
Jeff, I seem to have misunderstood what e1000 was doing, the patches
changing skb->truesize should be fine.
^ permalink raw reply
* Re: [PATCH 05/10] e1000: Update truesize with the length of the packet for packet split
From: Jesse Brandeburg @ 2006-04-14 23:04 UTC (permalink / raw)
To: David S. Miller
Cc: jesse.brandeburg, jgarzik, auke-jan.h.kok, netdev, john.ronciak,
Jeffrey.t.kirsher, auke
In-Reply-To: <20060414.155129.121789313.davem@davemloft.net>
On Fri, 14 Apr 2006, David S. Miller wrote:
> From: Jesse Brandeburg <jesse.brandeburg@intel.com>
> Date: Fri, 14 Apr 2006 15:43:02 -0700 (Pacific Daylight Time)
>
> > Please help me understand how you think it should work when we have a
> > device that wants to receive a packet using header in the skb->data, and
> > application data in the ->frags[]?
>
> If you're removing the pages from ->frags[] and you already accounted
> for them in skb->truesize, then yes I guess it could be correct to
> subtract it back out.
they are not accounted for in the skb yet. the way e1000 works is it
pre-allocates all its 128 byte receive buffers using skb_alloc, and then
chains in any pages that are used for receive packets.
> Looking at S2IO it's doing something very wrong with it's modification
> of skb->truesize. It's not changing the amount of memory allocated
> to an SKB in any way yet it's bumping skb->truesize for some reason.
> That will definitely lead to performance problems.
I can only speak to what e1000 is trying to do. we're technically
increasing the size of the skb after we've allocated it, so
does nr_frags and frags[] need truesize to be correct?
> That's why generally when we see an skb->truesize modification, it's
> just assumed to be a bug, it's generally not necessary but may be
> so in your e1000 situation here.
cool, I would really prefer that we extend the kernel api's to "do the
right thing" for receives using frags[]
> Another problem to look out for is that once a socket has a reference
> to an SKB you must never ever modify skb->truesize because doing so
> will break socket memory accounting. There is exactly one spot in
> the TCP stack where we are able to do this by adjusting the socket
> memory accounting variables carefully in a locked context at the same
> time, but otherwise it's not safe to modify skb->truesize when there
> is an attached socket.
Absolutely! we're indicating the packet to the stack using netif_rx* (and
we allocated it), so no socket has the reference until after we change
truesize.
^ permalink raw reply
* Re: [PATCH] IrDA: smsc-ircc2, smcinit support for ALi ISA bridges
From: David S. Miller @ 2006-04-14 23:03 UTC (permalink / raw)
To: samuel.ortiz; +Cc: netdev, jt, irda-users
In-Reply-To: <20060414101617.GB32724@irie>
From: Samuel Ortiz <samuel.ortiz@nokia.com>
Date: Fri, 14 Apr 2006 13:16:18 +0300
> Hi David,
>
> This patch enables support for ALi ISA bridges when we run the smcinit code.
> It is needed to properly configure some Toshiba laptops.
>
> Patch from Linus Walleij <triad@df.lth.se>
> Signed-off-by: Samuel Ortiz <samuel.ortiz@nokia.com>
Also applied, thanks.
^ permalink raw reply
* Re: [PATCH] IrDA: irda-usb, unregister netdev when patch upload fails
From: David S. Miller @ 2006-04-14 23:02 UTC (permalink / raw)
To: samuel.ortiz; +Cc: irda-users, jt, netdev
In-Reply-To: <20060414101607.GA32724@irie>
From: Samuel Ortiz <samuel.ortiz@nokia.com>
Date: Fri, 14 Apr 2006 13:16:07 +0300
> Hi David,
>
> In the STIR421x case, when the firmware upload fails, we need to unregister_netdev. Otherwise we hit a BUG on free_netdev(), if sysfs is enabled.
>
> Signed-off-by: Samuel Ortiz <samuel.ortiz@nokia.com>
Applied, but please become familiar with the carriage return key on
your keyboard and format your changelog descirptions to ~80 columns.
Thanks.
^ permalink raw reply
* Re: [PATCH 1/4] clip: run through Lindent
From: David S. Miller @ 2006-04-14 22:59 UTC (permalink / raw)
To: shemminger; +Cc: chas, linux-atm-general, netdev, stable
In-Reply-To: <20060413152224.2d46df99@localhost.localdomain>
Ignore my previous email, I tried to apply it to the wrong
tree :-)
^ permalink raw reply
* Re: [PATCH 1/4] clip: run through Lindent
From: David S. Miller @ 2006-04-14 22:58 UTC (permalink / raw)
To: shemminger; +Cc: chas, linux-atm-general, netdev, stable
In-Reply-To: <20060413152224.2d46df99@localhost.localdomain>
From: Stephen Hemminger <shemminger@osdl.org>
Date: Thu, 13 Apr 2006 15:22:24 -0700
> Run CLIP driver through Lindent script to fix formatting.
>
> Signed-off-by: Stephen Hemminger <shemminger@osdl.org>
These no longer apply after putting in the two CLIP
bug fixes.
Please respin, thanks.
^ permalink raw reply
* Re: [e1000 debug] KERNEL: assertion (!sk_forward_alloc) failed...
From: Jesse Brandeburg @ 2006-04-14 22:55 UTC (permalink / raw)
To: David S. Miller
Cc: jesse.brandeburg, bb, herbert, kernel, nipsy, jrlundgren, cat,
djani22, yoseph.basri, mykleb, olel, michal, chris, netdev,
jesse.brandeburg, E1000-devel, ak, jgarzik
In-Reply-To: <20060414.155202.82987818.davem@davemloft.net>
On Fri, 14 Apr 2006, David S. Miller wrote:
> From: Jesse Brandeburg <jesse.brandeburg@intel.com>
> Date: Fri, 14 Apr 2006 15:46:31 -0700 (Pacific Daylight Time)
>
> > sure, thats fine, but we just reproduced it in two seperate systems
> > without the e1000 driver loaded, using the instructions as mentioned in a
> > previous email. We used a 5704 with TSO enabled.
>
> That I didn't notice, thanks for the datapoint.
you're welcome, its "hot off the presses" (we just reproduced it 5 minutes
ago)
I'm trying to isolate more of a reproduction case, I'll be sure to post if
I can find anything with more detail.
^ permalink raw reply
* Re: [PATCH] atm: clip timer race
From: David S. Miller @ 2006-04-14 22:56 UTC (permalink / raw)
To: shemminger; +Cc: herbert, chas, linux-atm-general, netdev
In-Reply-To: <20060412154214.1dd4117d@localhost.localdomain>
From: Stephen Hemminger <shemminger@osdl.org>
Date: Wed, 12 Apr 2006 15:42:14 -0700
> By inspection, the clip idle timer code is racy on SMP.
> Here is a safe version of timer management.
> Untested, I don't have ATM hardware.
>
> Signed-off-by: Stephen Hemminger <shemminger@osdl.org>
Applied, thanks Stephen.
^ permalink raw reply
* Re: [e1000 debug] KERNEL: assertion (!sk_forward_alloc) failed...
From: David S. Miller @ 2006-04-14 22:52 UTC (permalink / raw)
To: jesse.brandeburg
Cc: bb, herbert, kernel, nipsy, jrlundgren, cat, djani22,
yoseph.basri, mykleb, olel, michal, chris, netdev,
jesse.brandeburg, E1000-devel, ak, jgarzik
In-Reply-To: <Pine.WNT.4.63.0604141543170.2116@jbrandeb-desk.amr.corp.intel.com>
From: Jesse Brandeburg <jesse.brandeburg@intel.com>
Date: Fri, 14 Apr 2006 15:46:31 -0700 (Pacific Daylight Time)
> sure, thats fine, but we just reproduced it in two seperate systems
> without the e1000 driver loaded, using the instructions as mentioned in a
> previous email. We used a 5704 with TSO enabled.
That I didn't notice, thanks for the datapoint.
^ permalink raw reply
* Re: [PATCH 05/10] e1000: Update truesize with the length of the packet for packet split
From: David S. Miller @ 2006-04-14 22:51 UTC (permalink / raw)
To: jesse.brandeburg
Cc: jgarzik, auke-jan.h.kok, netdev, john.ronciak, Jeffrey.t.kirsher,
auke
In-Reply-To: <Pine.WNT.4.63.0604141533250.2116@jbrandeb-desk.amr.corp.intel.com>
From: Jesse Brandeburg <jesse.brandeburg@intel.com>
Date: Fri, 14 Apr 2006 15:43:02 -0700 (Pacific Daylight Time)
> Please help me understand how you think it should work when we have a
> device that wants to receive a packet using header in the skb->data, and
> application data in the ->frags[]?
If you're removing the pages from ->frags[] and you already accounted
for them in skb->truesize, then yes I guess it could be correct to
subtract it back out.
Looking at S2IO it's doing something very wrong with it's modification
of skb->truesize. It's not changing the amount of memory allocated
to an SKB in any way yet it's bumping skb->truesize for some reason.
That will definitely lead to performance problems.
That's why generally when we see an skb->truesize modification, it's
just assumed to be a bug, it's generally not necessary but may be
so in your e1000 situation here.
Another problem to look out for is that once a socket has a reference
to an SKB you must never ever modify skb->truesize because doing so
will break socket memory accounting. There is exactly one spot in
the TCP stack where we are able to do this by adjusting the socket
memory accounting variables carefully in a locked context at the same
time, but otherwise it's not safe to modify skb->truesize when there
is an attached socket.
^ permalink raw reply
* Re: [e1000 debug] KERNEL: assertion (!sk_forward_alloc) failed...
From: Jesse Brandeburg @ 2006-04-14 22:46 UTC (permalink / raw)
To: David S. Miller
Cc: jesse.brandeburg, bb, herbert, kernel, nipsy, jrlundgren, cat,
djani22, yoseph.basri, mykleb, olel, michal, chris, netdev,
jesse.brandeburg, E1000-devel, ak, jgarzik
In-Reply-To: <20060414.154240.30125561.davem@davemloft.net>
On Fri, 14 Apr 2006, David S. Miller wrote:
> From: Jesse Brandeburg <jesse.brandeburg@intel.com>
> Date: Fri, 14 Apr 2006 15:32:55 -0700 (Pacific Daylight Time)
>
> > well there was one of them here, but the tg3 bit may actually be due to
> > the 2.6.14 problems.
> >
> > http://bugzilla.kernel.org/show_bug.cgi?id=6279
>
> There are 2 e1000 gigabit devices in that person's system, and
> not one tg3 device.
sure, thats fine, but we just reproduced it in two seperate systems
without the e1000 driver loaded, using the instructions as mentioned in a
previous email. We used a 5704 with TSO enabled.
I'm willing to try any debug patches you might come up with.
Jesse
^ permalink raw reply
* Re: [PATCH 05/10] e1000: Update truesize with the length of the packet for packet split
From: Jesse Brandeburg @ 2006-04-14 22:43 UTC (permalink / raw)
To: David S. Miller
Cc: jesse.brandeburg, jgarzik, auke-jan.h.kok, netdev, john.ronciak,
Jeffrey.t.kirsher, auke
In-Reply-To: <20060414.153219.62111494.davem@davemloft.net>
On Fri, 14 Apr 2006, David S. Miller wrote:
> From: Jesse Brandeburg <jesse.brandeburg@intel.com>
> Date: Fri, 14 Apr 2006 15:13:31 -0700 (Pacific Daylight Time)
>
> > in the old driver we indicate a packet with truesize = 128, which has 1000
> > bytes of data in it. normally (for everyone using only an skb) skb->len,
> > and skb->truesize are updated by skb_put(); In this case we don't have a
> > kernel support function for adding length to skb->len *and* to
> > skb->truesize when chaining on something to ->frags[]
>
> Where does skb_put() update truesize?
my mistake, it does not. but truesize in the "normal" case is already set
to the full length of the allocated skb (which would hold the whole
frame). In the packet split case the received skb looks dramatically
different, and since we're one of the only drivers using this mechanism.
I could easily be incorrect here, but my interpretation based on the
stack's use of ->truesize in do_tcp_sendpages is what I based our code
change on.
Please help me understand how you think it should work when we have a
device that wants to receive a packet using header in the skb->data, and
application data in the ->frags[]?
Jesse
^ permalink raw reply
* Re: [e1000 debug] KERNEL: assertion (!sk_forward_alloc) failed...
From: David S. Miller @ 2006-04-14 22:42 UTC (permalink / raw)
To: jesse.brandeburg
Cc: bb, herbert, kernel, nipsy, jrlundgren, cat, djani22,
yoseph.basri, mykleb, olel, michal, chris, netdev,
jesse.brandeburg, E1000-devel, ak, jgarzik
In-Reply-To: <Pine.WNT.4.63.0604141527420.2116@jbrandeb-desk.amr.corp.intel.com>
From: Jesse Brandeburg <jesse.brandeburg@intel.com>
Date: Fri, 14 Apr 2006 15:32:55 -0700 (Pacific Daylight Time)
> well there was one of them here, but the tg3 bit may actually be due to
> the 2.6.14 problems.
>
> http://bugzilla.kernel.org/show_bug.cgi?id=6279
There are 2 e1000 gigabit devices in that person's system, and
not one tg3 device.
^ permalink raw reply
* Re: [e1000 debug] KERNEL: assertion (!sk_forward_alloc) failed...
From: Jesse Brandeburg @ 2006-04-14 22:32 UTC (permalink / raw)
To: David S. Miller
Cc: jesse.brandeburg, bb, herbert, kernel, nipsy, jrlundgren, cat,
djani22, yoseph.basri, mykleb, olel, michal, chris, netdev,
jesse.brandeburg, E1000-devel, ak, jgarzik
In-Reply-To: <20060414.140225.113553109.davem@davemloft.net>
On Fri, 14 Apr 2006, David S. Miller wrote:
> From: Jesse Brandeburg <jesse.brandeburg@intel.com>
> Date: Fri, 14 Apr 2006 13:28:10 -0700
>
> > We also have some new data from the last couple of days. First, I think
> > that this problem is likely not just E1000's fault. We have multiple
> > reports both in bugzilla.kernel.org and from a distro that show this
> > problem has occurred on (at least) tg3 driven adapters as well as e1000.
>
> That's interesting since tg3 does not enable TSO by default for any
> chip until very recent versions of the tg3 driver. And even with
> those recent tg3 drivers which will enable TSO by default, it is only
> done for a very specific selection of chip revisions.
>
> Where are these reports that precisely implicate tg3?
well there was one of them here, but the tg3 bit may actually be due to
the 2.6.14 problems.
http://bugzilla.kernel.org/show_bug.cgi?id=6279
the other one is buried at a distro's bugzilla, so I can't post it.
> Note that there were legitimate TSO retransmit bugs in the 2.6.14
> timeframe that triggered those messages and did get fixed. And yes
> some of those reports were with tg3.
I've removed the call to pskb_expand_head in e1000 (yes i know its not
quite right, but it shouldn't be fatal either) and the bug still occurs.
so its something else. I'm not giving up on this, I just want to post the
state of the investigation.
Jesse
^ permalink raw reply
* Re: [PATCH 05/10] e1000: Update truesize with the length of the packet for packet split
From: David S. Miller @ 2006-04-14 22:32 UTC (permalink / raw)
To: jesse.brandeburg
Cc: jgarzik, auke-jan.h.kok, netdev, john.ronciak, Jeffrey.t.kirsher,
auke
In-Reply-To: <Pine.WNT.4.63.0604141448520.2116@jbrandeb-desk.amr.corp.intel.com>
From: Jesse Brandeburg <jesse.brandeburg@intel.com>
Date: Fri, 14 Apr 2006 15:13:31 -0700 (Pacific Daylight Time)
> in the old driver we indicate a packet with truesize = 128, which has 1000
> bytes of data in it. normally (for everyone using only an skb) skb->len,
> and skb->truesize are updated by skb_put(); In this case we don't have a
> kernel support function for adding length to skb->len *and* to
> skb->truesize when chaining on something to ->frags[]
Where does skb_put() update truesize?
^ permalink raw reply
* Re: [PATCH 05/10] e1000: Update truesize with the length of the packet for packet split
From: Jesse Brandeburg @ 2006-04-14 22:13 UTC (permalink / raw)
To: Jeff Garzik
Cc: Kok, Auke, netdev, Miller, David, Ronciak, John,
Brandeburg, Jesse, Kirsher, Jeff, Kok, Auke
In-Reply-To: <4440112D.9040509@pobox.com>
On Fri, 14 Apr 2006, Jeff Garzik wrote:
> Kok, Auke wrote:
> > diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
> > index 2cc9955..7ba1b16 100644
> > --- a/drivers/net/e1000/e1000_main.c
> > +++ b/drivers/net/e1000/e1000_main.c
> > @@ -3770,6 +3770,7 @@ e1000_clean_rx_irq_ps(struct e1000_adapt
> > ps_page->ps_page[j] = NULL;
> > skb->len += length;
> > skb->data_len += length;
> > + skb->truesize += length;
>
> NAK, apparently this is adding a very serious bug.
I'm surprised to hear you say that, here's why. Elaboration would be
helpful.
our packet split receive code uses two buffers for any packet the hardware
was able to split. The skb when we're receiving say a 1000 byte IPv4
packet looks like this:
skb -
|- len = total packet length (1000)
|- data_len = length in skb_shinfo(skb)->frags[] (1000 - headerlen)
|- truesize = size of originally allocated skb (128 + NET_IP_ALIGN)
in this case, the stack is using truesize to figure out how much to charge
the receive socket for our receive packet.
in the old driver we indicate a packet with truesize = 128, which has 1000
bytes of data in it. normally (for everyone using only an skb) skb->len,
and skb->truesize are updated by skb_put(); In this case we don't have a
kernel support function for adding length to skb->len *and* to
skb->truesize when chaining on something to ->frags[]
see the code in do_tcp_sendpages for an example of what the stack does in
setting up a packet for transmit using frags[]
it updates skb->len and skb->truesize, right after calling
skb_fill_page_desc, just like we do.
interesingly, that is right where sk_forward_alloc is modified (but thats
another thread)
Jesse
^ permalink raw reply
* Re: [PATCH] atm: clip causes unregister hang
From: David S. Miller @ 2006-04-14 22:07 UTC (permalink / raw)
To: shemminger; +Cc: herbert, chas, linux-atm-general, netdev, stable
In-Reply-To: <20060412145254.4dd21be6@localhost.localdomain>
From: Stephen Hemminger <shemminger@osdl.org>
Date: Wed, 12 Apr 2006 14:52:54 -0700
> If Classical IP over ATM module is loaded, its neighbor table gets
> populated when permanent neighbor entries are created; but these entries
> are not flushed when the device is removed. Since the entry never gets
> flushed the unregister of the network device never completes.
>
> This version of the patch also adds locking around the reference to
> the atm arp daemon to avoid races with events and daemon state changes.
> (Note: barrier() was never really safe)
>
> Bug-reference: http://bugzilla.kernel.org/show_bug.cgi?id=6295
> Signed-off-by: Stephen Hemminger <shemminger@osdl.org>
Applied, thanks Stephen.
^ permalink raw reply
* Re: [XFRM Doc]: aevent description
From: David S. Miller @ 2006-04-14 22:05 UTC (permalink / raw)
To: hadi; +Cc: netdev, hidden, herbert
In-Reply-To: <1144933208.5262.9.camel@jzny2>
From: jamal <hadi@cyberus.ca>
Date: Thu, 13 Apr 2006 09:00:08 -0400
> There is dependency on the previous patch i sent since the issue that
> patch fixes is assumed in this text description. It would be a good
> idea to apply at the same time as the other.
Applied, after fixing 28 lines containing trailing whitespace :-)
^ 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