* Re: [PATCH 5/5] net: Make AF_UNIX per network namespace safe.
From: Eric W. Biederman @ 2007-09-29 17:03 UTC (permalink / raw)
To: Patrick McHardy; +Cc: David Miller, netdev, Linux Containers
In-Reply-To: <46FE73AC.40807@trash.net>
Patrick McHardy <kaber@trash.net> writes:
> Eric W. Biederman wrote:
>> Because of the global nature of garbage collection, and because of the
>> cost of per namespace hash tables unix_socket_table has been kept
>> global. With a filter added on lookups so we don't see sockets from
>> the wrong namespace.
>>
>> Currently I don't fold the namesapce into the hash so multiple
>> namespaces using the same socket name will be guaranteed a hash
>> collision.
>
>
> That doesn't sound like a good thing :) Is there a reason for
> not avoiding the collisions?
Two reasons. Minimizing the size of the changes to make review
easier, and I don't know if hash collisions are likely in practice
or if they matter. I don't believe we can't physically collide and
have the same inode because we make a node in the filesystem. The
abstract domain is local to linux and so people don't use it as much.
All of which boils down to. I don't see it matter a heck of a lot
especially initially. So I did the traditional unix thing and started
with a simple and stupid implementation. But it didn't quite feel
right to me either so I documented it.
Whipping up a patch to take the namespace into account in mkname
doesn't look to hard though.
Eric
^ permalink raw reply
* Re: [PATCH] rtnl: Simplify ASSERT_RTNL
From: Eric W. Biederman @ 2007-09-29 17:18 UTC (permalink / raw)
To: Herbert Xu; +Cc: davem, netdev
In-Reply-To: <E1IbTz0-0000FK-00@gondolin.me.apana.org.au>
Herbert Xu <herbert@gondor.apana.org.au> writes:
> Eric W. Biederman <ebiederm@xmission.com> wrote:
>>
>> Currently we have the call path:
>> macvlan_open -> dev_unicast_add -> __dev_set_rx_mode ->
>> __dev_set_promiscuity -> ASSERT_RTNL -> mutex_trylock
>>
>> When mutex debugging is on taking a mutex complains if we are not
>> allowed to sleep. At that point we have called netif_tx_lock_bh
>> so we are clearly not allowed to sleep. Arguably this is not a
>> problem for mutex_trylock.
>
> Actually holding the TX lock here is a bug. We're going to
> call down into the hardware with __dev_set_promiscuity, which
> may sleep (think USB NICs), so we definitely shouldn't be holding
> any spin locks.
Regardless of the correctness of where we have ASSERT_RTNL.
I think not actually taking the mutex on the assertion failure path
(just so we can release it), is still a good deal regardless.
For this particular call site clearly we need to look at what
is happening a little more. The obvious thing would be to add
an explicit might_sleep if we are calling code that can sleep.
Eric
^ permalink raw reply
* Re: SFQ: backport some features from ESFQ (try 4)
From: Corey Hickey @ 2007-09-29 17:36 UTC (permalink / raw)
To: Patrick McHardy; +Cc: netdev
In-Reply-To: <46FE7171.70501@trash.net>
Patrick McHardy wrote:
> Corey Hickey wrote:
>> Patchset try 2 addresses the review by Michael Buesch.
>> Patchset try 3 addresses the review by Patrick McHardy.
>> Patchset try 4 has a few cosmetic improvements.
>>
>> Nobody reviewed my last set of patches, and I wasn't pushy about asking.
>> Since it's been a while, I ported the kernel and userspace patchsets to
>> current git of net-2.6 and iproute2, respectively.
>
> Thanks for posting these patches, I'll have a closer look tommorrow.
Many thanks for reviewing.
-Corey
^ permalink raw reply
* Re: [PATCH 2/5] net: Make rtnetlink infrastructure network namespace aware
From: Patrick McHardy @ 2007-09-29 17:48 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: David Miller, netdev, Linux Containers
In-Reply-To: <m1tzpd8lfh.fsf@ebiederm.dsl.xmission.com>
Eric W. Biederman wrote:
> Patrick McHardy <kaber@trash.net> writes:
>
>
>>I'm wondering why this receive queue processing on unlock is still
>>necessary today, we don't do trylock in rtnetlink_rcv anymore, so
>>all senders will simply wait until the lock is released and then
>>process the queue.
>
>
> Good question, I should probably look. I was lazy and didn't go back
> and audit why we were doing this. I just coded a routine that I was
> certain would work. It does appear that we are processing the queue
> with sk_data_read when we add a message, so this may be completely
> unnecessary. I will go back and look. If we can remove this bit
> things should be simpler.
Maybe I can save you some time: we used to do down_trylock()
for the rtnl mutex, so senders would simply return if someone
else was already processing the queue *or* the rtnl was locked
for some other reason. In the first case the process already
processing the queue would also process the new messages, but
if it the rtnl was locked for some other reason (for example
during module registration) the message would sit in the
queue until the next rtnetlink sendmsg call, which is why
rtnl_unlock does queue processing. Commit 6756ae4b changed
the down_trylock to mutex_lock, so senders will now simply wait
until the mutex is released and then call netlink_run_queue
themselves. This means its not needed anymore.
^ permalink raw reply
* Re: [PATCH 5/5] net: Make AF_UNIX per network namespace safe.
From: Patrick McHardy @ 2007-09-29 17:50 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: David Miller, netdev, Linux Containers
In-Reply-To: <m1abr58kvq.fsf@ebiederm.dsl.xmission.com>
Eric W. Biederman wrote:
> Patrick McHardy <kaber@trash.net> writes:
>
>>>Currently I don't fold the namesapce into the hash so multiple
>>>namespaces using the same socket name will be guaranteed a hash
>>>collision.
>>
>>
>>That doesn't sound like a good thing :) Is there a reason for
>>not avoiding the collisions?
>
>
> Two reasons. Minimizing the size of the changes to make review
> easier, and I don't know if hash collisions are likely in practice
> or if they matter. I don't believe we can't physically collide and
> have the same inode because we make a node in the filesystem. The
> abstract domain is local to linux and so people don't use it as much.
>
> All of which boils down to. I don't see it matter a heck of a lot
> especially initially. So I did the traditional unix thing and started
> with a simple and stupid implementation. But it didn't quite feel
> right to me either so I documented it.
>
> Whipping up a patch to take the namespace into account in mkname
> doesn't look to hard though.
It doesn't look like it would increase patch size significantly
(about 4 more changed lines), but it could of course be done in
a follow-up patch.
^ permalink raw reply
* Re: [PATCH] rtnl: Simplify ASSERT_RTNL
From: Patrick McHardy @ 2007-09-29 17:51 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: Herbert Xu, davem, netdev
In-Reply-To: <m13awx8k6d.fsf@ebiederm.dsl.xmission.com>
Eric W. Biederman wrote:
> Regardless of the correctness of where we have ASSERT_RTNL.
> I think not actually taking the mutex on the assertion failure path
> (just so we can release it), is still a good deal regardless.
Agreed. I actually have an identical patch somewhere that
I wanted to submit soon.
^ permalink raw reply
* [RFC][IPv6] Export userland ND options through netlink (RDNSS support)
From: Pierre Ynard @ 2007-09-29 17:47 UTC (permalink / raw)
To: netdev
As discussed before, this patch provides userland with a way to access
relevant options in Router Advertisements, after they are processed and
validated by the kernel. Extra options are processed in a generic way;
this patch only exports RDNSS options described in RFC5006, but support
to control which options are exported could be easily added.
Options are exported using a new message type of rtnetlink. I am totally
new to netlink, so help would be greatly appreciated to design a right
interface. For now each message only contains a single ND option, in its
raw format. Should they contains several options when possible?
Also, depending on the option, userland might need context information
contained in the rest of the RA packet. For example, RFC5006 states:
"Note: An RDNSS address MUST be used only as long as both the RA
router lifetime and the RDNSS option lifetime have not expired."
which implies that a userland daemon processing RDNSS options needs a
way to associate the option to the router that sent it, and fetch its
lifetime. This kind of information could be included in a header in the
rtnetlink message (in this version of the patch there is none).
Regards,
diff --git a/include/linux/rtnetlink.h b/include/linux/rtnetlink.h
index dff3192..f69d415 100644
--- a/include/linux/rtnetlink.h
+++ b/include/linux/rtnetlink.h
@@ -97,6 +97,9 @@ enum {
RTM_SETNEIGHTBL,
#define RTM_SETNEIGHTBL RTM_SETNEIGHTBL
+ RTM_NEWNDUSEROPT = 68,
+#define RTM_NEWNDUSEROPT RTM_NEWNDUSEROPT
+
__RTM_MAX,
#define RTM_MAX (((__RTM_MAX + 3) & ~3) - 1)
};
@@ -542,6 +545,8 @@ enum rtnetlink_groups {
#define RTNLGRP_IPV6_PREFIX RTNLGRP_IPV6_PREFIX
RTNLGRP_IPV6_RULE,
#define RTNLGRP_IPV6_RULE RTNLGRP_IPV6_RULE
+ RTNLGRP_ND_USEROPT,
+#define RTNLGRP_ND_USEROPT RTNLGRP_ND_USEROPT
__RTNLGRP_MAX
};
#define RTNLGRP_MAX (__RTNLGRP_MAX - 1)
diff --git a/include/net/ndisc.h b/include/net/ndisc.h
index 475b10c..6684f7e 100644
--- a/include/net/ndisc.h
+++ b/include/net/ndisc.h
@@ -24,6 +24,7 @@ enum {
ND_OPT_MTU = 5, /* RFC2461 */
__ND_OPT_ARRAY_MAX,
ND_OPT_ROUTE_INFO = 24, /* RFC4191 */
+ ND_OPT_RDNSS = 25, /* RFC5006 */
__ND_OPT_MAX
};
diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
index 74c4d8d..646cd4a 100644
--- a/net/ipv6/ndisc.c
+++ b/net/ipv6/ndisc.c
@@ -15,9 +15,10 @@
/*
* Changes:
*
+ * Pierre Ynard : export userland ND options
+ * through netlink (RDNSS support)
* Lars Fenneberg : fixed MTU setting on receipt
* of an RA.
- *
* Janos Farkas : kmalloc failure checks
* Alexey Kuznetsov : state machine reworked
* and moved to net/core.
@@ -78,6 +79,9 @@
#include <net/addrconf.h>
#include <net/icmp.h>
+#include <net/netlink.h>
+#include <linux/rtnetlink.h>
+
#include <net/flow.h>
#include <net/ip6_checksum.h>
#include <linux/proc_fs.h>
@@ -161,6 +165,8 @@ struct ndisc_options {
struct nd_opt_hdr *nd_opts_ri;
struct nd_opt_hdr *nd_opts_ri_end;
#endif
+ struct nd_opt_hdr *nd_useropts;
+ struct nd_opt_hdr *nd_useropts_end;
};
#define nd_opts_src_lladdr nd_opt_array[ND_OPT_SOURCE_LL_ADDR]
@@ -225,6 +231,22 @@ static struct nd_opt_hdr *ndisc_next_option(struct nd_opt_hdr *cur,
return (cur <= end && cur->nd_opt_type == type ? cur : NULL);
}
+static inline int ndisc_is_useropt(struct nd_opt_hdr *opt)
+{
+ return (opt->nd_opt_type == ND_OPT_RDNSS);
+}
+
+static struct nd_opt_hdr *ndisc_next_useropt(struct nd_opt_hdr *cur,
+ struct nd_opt_hdr *end)
+{
+ if (!cur || !end || cur >= end)
+ return NULL;
+ do {
+ cur = ((void *)cur) + (cur->nd_opt_len << 3);
+ } while(cur < end && !ndisc_is_useropt(cur));
+ return (cur <= end && ndisc_is_useropt(cur) ? cur : NULL);
+}
+
static struct ndisc_options *ndisc_parse_options(u8 *opt, int opt_len,
struct ndisc_options *ndopts)
{
@@ -267,14 +289,21 @@ static struct ndisc_options *ndisc_parse_options(u8 *opt, int opt_len,
break;
#endif
default:
- /*
- * Unknown options must be silently ignored,
- * to accommodate future extension to the protocol.
- */
- ND_PRINTK2(KERN_NOTICE
- "%s(): ignored unsupported option; type=%d, len=%d\n",
- __FUNCTION__,
- nd_opt->nd_opt_type, nd_opt->nd_opt_len);
+ if (ndisc_is_useropt(nd_opt)) {
+ ndopts->nd_useropts_end = nd_opt;
+ if (!ndopts->nd_useropts)
+ ndopts->nd_useropts = nd_opt;
+ } else {
+ /*
+ * Unknown options must be silently ignored,
+ * to accommodate future extension to the
+ * protocol.
+ */
+ ND_PRINTK2(KERN_NOTICE
+ "%s(): ignored unsupported option; type=%d, len=%d\n",
+ __FUNCTION__,
+ nd_opt->nd_opt_type, nd_opt->nd_opt_len);
+ }
}
opt_len -= l;
nd_opt = ((void *)nd_opt) + l;
@@ -984,6 +1013,33 @@ out:
in6_dev_put(idev);
}
+static void ndisc_useropt_notify(struct nd_opt_hdr *opt)
+{
+ struct sk_buff *skb;
+ struct nlmsghdr *nlh;
+ int err = -ENOBUFS;
+
+ skb = nlmsg_new(opt->nd_opt_len << 3, GFP_ATOMIC);
+ if (skb == NULL)
+ goto errout;
+
+ nlh = nlmsg_put(skb, 0, 0, RTM_NEWNDUSEROPT, opt->nd_opt_len << 3, 0);
+ if (nlh == NULL) {
+ nlmsg_free(skb);
+ goto errout;
+ }
+
+ memcpy(nlmsg_data(nlh), opt, opt->nd_opt_len << 3);
+ nlmsg_end(skb, nlh);
+
+ err = rtnl_notify(skb, 0, RTNLGRP_ND_USEROPT, NULL, GFP_ATOMIC);
+
+ if (err < 0) {
+errout:
+ rtnl_set_sk_err(RTNLGRP_ND_USEROPT, err);
+ }
+}
+
static void ndisc_router_discovery(struct sk_buff *skb)
{
struct ra_msg *ra_msg = (struct ra_msg *)skb_transport_header(skb);
@@ -1216,6 +1272,15 @@ skip_defrtr:
}
}
+ if (ndopts.nd_useropts) {
+ struct nd_opt_hdr *opt;
+ for (opt = ndopts.nd_useropts;
+ opt;
+ opt = ndisc_next_useropt(opt, ndopts.nd_useropts_end)) {
+ ndisc_useropt_notify(opt);
+ }
+ }
+
if (ndopts.nd_opts_tgt_lladdr || ndopts.nd_opts_rh) {
ND_PRINTK2(KERN_WARNING
"ICMPv6 RA: invalid RA options");
--
Pierre Ynard
For hire in Quebec City, Canada, during Fall 2007
"Une âme dans un corps, c'est comme un dessin sur une feuille de papier."
^ permalink raw reply related
* Re: 2.6.23-rc8-mm2 - tcp_fastretrans_alert() WARNING
From: Ilpo Järvinen @ 2007-09-29 20:49 UTC (permalink / raw)
To: Cedric Le Goater; +Cc: Andrew Morton, LKML, Netdev, David Miller
In-Reply-To: <46FE6751.3050205@free.fr>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 2047 bytes --]
On Sat, 29 Sep 2007, Cedric Le Goater wrote:
> Ilpo Järvinen wrote:
> > On Fri, 28 Sep 2007, Ilpo Järvinen wrote:
> >> On Fri, 28 Sep 2007, Cedric Le Goater wrote:
> >>
> >>> I just found that warning in my logs. It seems that it's been
> >>> happening since rc7-mm1 at least.
> >>>
> >>> WARNING: at /home/legoater/linux/2.6.23-rc8-mm2/net/ipv4/tcp_input.c:2314 tcp_fastretrans_alert()
> >>>
> >>> Call Trace:
> >>> <IRQ> [<ffffffff8040fdc3>] tcp_ack+0xcd6/0x1894
> >>> ...snip...
> >> ...Thanks for the report, I'll have look what could still break
> >> fackets_out...
> >
> > I think this one is now clear to me, tcp_fragment/collapse adjusts
> > fackets_out (incorrectly) also for reno flow when there were some dupACKs
> > that made sacked_out != 0. Could you please try if patch below proves all
> > them to be of non-SACK origin... In case that's true, it's rather
> > harmless, I'll send a fix on Monday or so (this would anyway be needed)...
> > If you find out that them occur with SACK enabled flow, that would be
> > more interesting and requires more digging...
>
> I'm trying now to reproduce this WARNING.
>
> It seems that the n/w behaves differently during the week ends. Probably
> taking a break.
Thanks.
Of course there are other means too to determine if TCP flows do negotiate
SACK enabled or not. Depending on your test case (which is fully unknown
to me) they may or may not be usable... At least the value of tcp_sack
sysctl on both systems or tcpdump catching SYN packets should give that
detail. ...If you know to which hosts TCP could be connected (and active)
to, while the WARNING triggers, it's really easy to test what is being
negotiated as it's unlikely to change at short notice and any TCP flow to
that host will get us the same information though the WARNING would not be
triggered with it at this time. Obviously if at least one of the remotes
is not known or the set ends up being mixture of reno and SACK flows, then
we'll just have to wait and see which fish we get...
--
i.
^ permalink raw reply
* Re: [PATCH 2/5] net: Make rtnetlink infrastructure network namespace aware
From: Eric W. Biederman @ 2007-09-29 21:00 UTC (permalink / raw)
To: Patrick McHardy; +Cc: David Miller, netdev, Linux Containers
In-Reply-To: <46FE8FD3.2030308@trash.net>
Patrick McHardy <kaber@trash.net> writes:
> Maybe I can save you some time: we used to do down_trylock()
> for the rtnl mutex, so senders would simply return if someone
> else was already processing the queue *or* the rtnl was locked
> for some other reason. In the first case the process already
> processing the queue would also process the new messages, but
> if it the rtnl was locked for some other reason (for example
> during module registration) the message would sit in the
> queue until the next rtnetlink sendmsg call, which is why
> rtnl_unlock does queue processing. Commit 6756ae4b changed
> the down_trylock to mutex_lock, so senders will now simply wait
> until the mutex is released and then call netlink_run_queue
> themselves. This means its not needed anymore.
Sounds reasonable.
I started looking through the code paths and I currently cannot
see anything that would leave a message on a kernel rtnl socket.
However I did a quick test adding a WARN_ON if there were any messages
found in the queue during rtnl_unlock and I found this code path
getting invoked from linkwatch_event. So there is clearly something I
don't understand, and it sounds at odds just a bit from your
description.
If we can remove the extra queue processing that would be great,
as it looks like a nice way to simplify the locking and the special
cases in the code.
Eric
^ permalink raw reply
* Re: [PKT_SCHED]: Add stateless NAT
From: Herbert Xu @ 2007-09-29 23:36 UTC (permalink / raw)
To: Patrick McHardy; +Cc: davem, hadi, netdev, kuznet
In-Reply-To: <46FE6D73.4000002@trash.net>
On Sat, Sep 29, 2007 at 05:21:23PM +0200, Patrick McHardy wrote:
>
> Unfortunately gcc doesn't perform tail call optimization when the
> address of a parameter or local variable is passed to an extern
> function before the tail call, which NF_HOOK does for the skb.
You mean the struct sk_buff **pskb bit? I've always wondered
why netfilter had to modify the skb. Could we perhaps make it
so that it doesn't need to do so?
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: [PKT_SCHED]: Add stateless NAT
From: David Miller @ 2007-09-30 0:13 UTC (permalink / raw)
To: kaber; +Cc: herbert, hadi, netdev, kuznet
In-Reply-To: <46FE6D73.4000002@trash.net>
From: Patrick McHardy <kaber@trash.net>
Date: Sat, 29 Sep 2007 17:21:23 +0200
> Unfortunately gcc doesn't perform tail call optimization when the
> address of a parameter or local variable is passed to an extern
> function before the tail call, which NF_HOOK does for the skb.
To which Herbert later asked why we need the **pskb thing.
The reason is that if we have to COW or resize the SKB it
could result in a realloc of the sk_buff struct which is
why we need the **pskb thing.
But strangely I can't find any skbuff.h interfaces that
require that any longer :-))) So this might be some artifact
that we can in fact kill off.
^ permalink raw reply
* Re: [PATCH] rtnl: Simplify ASSERT_RTNL
From: Herbert Xu @ 2007-09-30 0:24 UTC (permalink / raw)
To: Patrick McHardy; +Cc: netdev, oliver, Eric W. Biederman, linux-usb-devel, davem
In-Reply-To: <46FE7019.4030705@trash.net>
On Sat, Sep 29, 2007 at 05:32:41PM +0200, Patrick McHardy wrote:
>
> For unicast addresses its not strictly necessary since they may
> only be changed under the RTNL anyway. The reason why it takes
> the tx_lock is for consistency with multicast address handling,
> which can't rely on the RTNL since IPv6 changes them from
> BH context. The idea was that the ->set_rx_mode function should
> handle both secondary unicast and multicast addresses for
> simplicity.
OK, the documentation (which predates USB NIC drivers) disagrees
with me and dev->set_multicast will always operate with the xmit
lock.
This isn't very nice for USB drivers since they have to leave
the change request hanging after dev->set_multicast returns.
I suppose the one thing that guarantees this will work is the
S in USB :)
However, at least one USB driver is buggy (kaweth) in not
allowing another set_multicast until the previous one is
done, which can lead to silent losses of set_multicast calls
since it's a void.
In any case, coming back to the original question, the RTNL
assertion is simply wrong in this case because if we're being
called from IPv6 then the RTNL won't even be held.
So I think we need to
1) Move the assert into dev_set_promiscuity.
2) Take the TX lock in dev_set_promiscuity.
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
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
^ permalink raw reply
* Re: [PKT_SCHED]: Add stateless NAT
From: Herbert Xu @ 2007-09-30 0:26 UTC (permalink / raw)
To: David Miller; +Cc: kaber, hadi, netdev, kuznet
In-Reply-To: <20070929.171339.59681016.davem@davemloft.net>
On Sat, Sep 29, 2007 at 05:13:39PM -0700, David Miller wrote:
>
> The reason is that if we have to COW or resize the SKB it
> could result in a realloc of the sk_buff struct which is
> why we need the **pskb thing.
>
> But strangely I can't find any skbuff.h interfaces that
> require that any longer :-))) So this might be some artifact
> that we can in fact kill off.
Indeed. The only other case I can think of is defragmentation.
But even there we should be able to squeeze it into the original
skb :)
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH] rtnl: Simplify ASSERT_RTNL
From: Herbert Xu @ 2007-09-30 0:28 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: davem, netdev
In-Reply-To: <m13awx8k6d.fsf@ebiederm.dsl.xmission.com>
On Sat, Sep 29, 2007 at 11:18:18AM -0600, Eric W. Biederman wrote:
>
> Regardless of the correctness of where we have ASSERT_RTNL.
> I think not actually taking the mutex on the assertion failure path
> (just so we can release it), is still a good deal regardless.
Provided that you add a might_sleep call in there so that if
somebody does this under locks it'll complain then I agree.
Checking RTNL under spin locks is almost certainly the sign
of a bug.
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: [PKT_SCHED]: Add stateless NAT
From: Herbert Xu @ 2007-09-30 0:43 UTC (permalink / raw)
To: David Miller; +Cc: kaber, hadi, netdev, kuznet
In-Reply-To: <20070930002601.GB7502@gondor.apana.org.au>
On Sun, Sep 30, 2007 at 08:26:01AM +0800, Herbert Xu wrote:
>
> Indeed. The only other case I can think of is defragmentation.
> But even there we should be able to squeeze it into the original
> skb :)
OK it won't be pretty but it's definitely doable. We simply
swap the contents of that skb with the head skb that would've
otherwise been returned.
If this is the only place in netfilter where we need to modify
*pskb then I think it's worthwhile.
So the question is are there any other places that we haven't
covered which also needs to change *pskb?
Thanks,
--
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: 2.6.23-rc8 network problem. Mem leak? ip1000a?
From: linux @ 2007-09-30 7:59 UTC (permalink / raw)
To: akpm, linux; +Cc: linux-kernel, netdev
In-Reply-To: <20070928022051.a6f27b1e.akpm@linux-foundation.org>
> ntpd. Sounds like pps leaking to me.
That's what I'd think, except that pps does no allocation in the normal
running state, so there's nothing to leak. The interrupt path just
records the time in some preallocated, static buffers and wakes up
blocked readers. The read path copies the latest data out of those
static buffers. There's allocation when the PPS device is created,
and more when it's opened.
>> Can anyone offer some diagnosis advice?
> CONFIG_DEBUG_SLAB_LEAK?
Ah, thanks you; I've been using SLUB which doesn't support this option.
Here's what I've extracted. I've only presented the top few
slab_allocators and a small subset of the oom-killer messages, but I
have full copies if desired. Unfortunately, I've discovered that the
machine doesn't live in this unhappy state forever. Indeed, I'm not
sure if killing ntpd "fixes" anything; my previous observations
may have been optimistic ignorance.
(For my own personal reference looking for more oom-kill, I nuked ntpd
at 06:46:56. And the oom-kills are continuing, with the latest at
07:43:52.)
Anyway, I have a bunch of information from the slab_allocators file, but
I'm not quire sure how to make sense of it.
With a machine in the unhappy state and firing the OOM killer, the top
20 slab_allocators are:
$ sort -rnk2 /proc/slab_allocators | head -20
skbuff_head_cache: 1712746 __alloc_skb+0x31/0x121
size-512: 1706572 tcp_send_ack+0x23/0x102
skbuff_fclone_cache: 149113 __alloc_skb+0x31/0x121
size-2048: 148500 tcp_sendmsg+0x1b5/0xae1
sysfs_dir_cache: 5289 sysfs_new_dirent+0x4b/0xec
size-512: 2613 sock_alloc_send_skb+0x93/0x1dd
Acpi-Operand: 2014 acpi_ut_allocate_object_desc_dbg+0x34/0x6e
size-32: 1995 sysfs_new_dirent+0x29/0xec
vm_area_struct: 1679 mmap_region+0x18f/0x421
size-512: 1618 tcp_xmit_probe_skb+0x1f/0xcd
size-512: 1571 arp_create+0x4e/0x1cd
vm_area_struct: 1544 copy_process+0x9f1/0x1108
anon_vma: 1448 anon_vma_prepare+0x29/0x74
filp: 1201 get_empty_filp+0x44/0xcd
UDP: 1173 sk_alloc+0x25/0xaf
size-128: 1048 r1bio_pool_alloc+0x23/0x3b
size-128: 1024 nfsd_cache_init+0x2d/0xcf
Acpi-Namespace: 973 acpi_ns_create_node+0x2c/0x45
vm_area_struct: 717 split_vma+0x33/0xe5
dentry: 594 d_alloc+0x24/0x177
I'm not sure quite what "normal" numbers are, but I do wonder why there
are 1.7 million TCP acks buffered in the system. Shouldn't they be
transmitted and deallocated pretty quickly?
This machine receives more data than it sends, so I'd expect acks to
outnumber "real" packets. Could the ip1000a driver's transmit path be
leaking skbs somehow? that would also explain the "flailing" of the
oom-killer; it can't associate the allocations with a process.
Here's /proc/meminfo:
MemTotal: 1035756 kB
MemFree: 43508 kB
Buffers: 72920 kB
Cached: 224056 kB
SwapCached: 344916 kB
Active: 664976 kB
Inactive: 267656 kB
SwapTotal: 4950368 kB
SwapFree: 3729384 kB
Dirty: 6460 kB
Writeback: 0 kB
AnonPages: 491708 kB
Mapped: 79232 kB
Slab: 41324 kB
SReclaimable: 25008 kB
SUnreclaim: 16316 kB
PageTables: 8132 kB
NFS_Unstable: 0 kB
Bounce: 0 kB
CommitLimit: 5468244 kB
Committed_AS: 1946008 kB
VmallocTotal: 253900 kB
VmallocUsed: 2672 kB
VmallocChunk: 251228 kB
I have a lot of oom-killer messages, that I have saved but am not
posting for size reasons, but here are some example backtraces.
They're not very helpful to me; do they enlighten anyone else?
02:50:20: apcupsd invoked oom-killer: gfp_mask=0xd0, order=1, oomkilladj=0
02:50:22:
02:50:22: Call Trace:
02:50:22: [<ffffffff80246053>] out_of_memory+0x71/0x1ba
02:50:22: [<ffffffff8024755d>] __alloc_pages+0x255/0x2d7
02:50:22: [<ffffffff8025cbd6>] cache_alloc_refill+0x2f4/0x60a
02:50:22: [<ffffffff8040602c>] hiddev_ioctl+0x579/0x919
02:50:22: [<ffffffff8025d0fc>] kmem_cache_alloc+0x57/0x95
02:50:22: [<ffffffff8040602c>] hiddev_ioctl+0x579/0x919
02:50:22: [<ffffffff80262511>] cp_new_stat+0xe5/0xfd
02:50:22: [<ffffffff804058ff>] hiddev_read+0x199/0x1f6
02:50:22: [<ffffffff80222fa0>] default_wake_function+0x0/0xe
02:50:22: [<ffffffff80269bb5>] do_ioctl+0x45/0x50
02:50:22: [<ffffffff80269db9>] vfs_ioctl+0x1f9/0x20b
02:50:22: [<ffffffff80269e07>] sys_ioctl+0x3c/0x5d
02:50:22: [<ffffffff8020b43e>] system_call+0x7e/0x83
02:52:18: postgres invoked oom-killer: gfp_mask=0xd0, order=1, oomkilladj=0
02:52:18:
02:52:18: Call Trace:
02:52:18: [<ffffffff80246053>] out_of_memory+0x71/0x1ba
02:52:18: [<ffffffff8024755d>] __alloc_pages+0x255/0x2d7
02:52:18: [<ffffffff8025be8a>] poison_obj+0x26/0x2f
02:52:18: [<ffffffff8024761f>] __get_free_pages+0x40/0x79
02:52:18: [<ffffffff80224d66>] copy_process+0xb0/0x1108
02:52:18: [<ffffffff80233388>] alloc_pid+0x1f/0x27d
02:52:18: [<ffffffff80225ed6>] do_fork+0xb1/0x1a7
02:52:18: [<ffffffff802f0627>] copy_user_generic_string+0x17/0x40
02:52:18: [<ffffffff8020b43e>] system_call+0x7e/0x83
02:52:18: [<ffffffff8020b757>] ptregscall_common+0x67/0xb0
02:52:18: kthreadd invoked oom-killer: gfp_mask=0xd0, order=1, oomkilladj=0
02:52:18:
02:52:18: Call Trace:
02:52:18: [<ffffffff80246053>] out_of_memory+0x71/0x1ba
02:52:18: [<ffffffff8024755d>] __alloc_pages+0x255/0x2d7
02:52:18: [<ffffffff8024761f>] __get_free_pages+0x40/0x79
02:52:18: [<ffffffff80224d66>] copy_process+0xb0/0x1108
02:52:18: [<ffffffff80233388>] alloc_pid+0x1f/0x27d
02:52:18: [<ffffffff80225ed6>] do_fork+0xb1/0x1a7
02:52:18: [<ffffffff80222bb8>] update_curr+0xe6/0x10b
02:52:18: [<ffffffff8022334d>] dequeue_entity+0x73/0x97
02:52:18: [<ffffffff8020bd21>] kernel_thread+0x81/0xde
02:52:18: [<ffffffff802e9c81>] cfq_may_queue+0x0/0xd2
02:52:18: [<ffffffff802355db>] kthread+0x0/0x75
02:52:18: [<ffffffff8020bd7e>] child_rip+0x0/0x12
02:52:18: [<ffffffff802354ad>] kthreadd+0xb4/0xf5
02:52:18: [<ffffffff8020bd88>] child_rip+0xa/0x12
02:52:18: [<ffffffff802353f9>] kthreadd+0x0/0xf5
02:52:18: [<ffffffff8020bd7e>] child_rip+0x0/0x12
02:54:53: apache2 invoked oom-killer: gfp_mask=0xd0, order=1, oomkilladj=0
02:54:53:
02:54:53: Call Trace:
02:54:53: [<ffffffff80246053>] out_of_memory+0x71/0x1ba
02:54:53: [<ffffffff8024755d>] __alloc_pages+0x255/0x2d7
02:54:53: [<ffffffff8025be8a>] poison_obj+0x26/0x2f
02:54:53: [<ffffffff8024761f>] __get_free_pages+0x40/0x79
02:54:53: [<ffffffff80224d66>] copy_process+0xb0/0x1108
02:54:53: [<ffffffff80233388>] alloc_pid+0x1f/0x27d
02:54:53: [<ffffffff80225ed6>] do_fork+0xb1/0x1a7
02:54:53: [<ffffffff8020b43e>] system_call+0x7e/0x83
02:54:53: [<ffffffff8020b757>] ptregscall_common+0x67/0xb0
02:55:45: ssh invoked oom-killer: gfp_mask=0x4d0, order=2, oomkilladj=0
02:55:45:
02:55:45: Call Trace:
02:55:45: [<ffffffff80246053>] out_of_memory+0x71/0x1ba
02:55:45: [<ffffffff8024755d>] __alloc_pages+0x255/0x2d7
02:55:45: [<ffffffff8025cbd6>] cache_alloc_refill+0x2f4/0x60a
02:55:45: [<ffffffff8025be8a>] poison_obj+0x26/0x2f
02:55:45: [<ffffffff8040e0df>] __alloc_skb+0x31/0x121
02:55:45: [<ffffffff8040e0df>] __alloc_skb+0x31/0x121
02:55:45: [<ffffffff8040ab8b>] sock_alloc_send_skb+0x93/0x1dd
02:55:45: [<ffffffff8025d067>] __kmalloc_track_caller+0x9d/0xdb
02:55:45: [<ffffffff8040e109>] __alloc_skb+0x5b/0x121
02:55:45: [<ffffffff8040ab8b>] sock_alloc_send_skb+0x93/0x1dd
02:55:45: [<ffffffff802f0627>] copy_user_generic_string+0x17/0x40
02:55:45: [<ffffffff8048155c>] unix_stream_sendmsg+0x151/0x2ea
02:55:45: [<ffffffff80408349>] sock_aio_write+0xe5/0xf0
02:55:45: [<ffffffff802437d3>] find_get_page+0xe/0x36
02:55:45: [<ffffffff8025f9b4>] do_sync_write+0xd1/0x118
02:55:45: [<ffffffff802357f5>] autoremove_wake_function+0x0/0x2e
02:55:45: [<ffffffff80222bb8>] update_curr+0xe6/0x10b
02:55:45: [<ffffffff80260118>] vfs_write+0xc0/0x136
02:55:45: [<ffffffff802605c2>] sys_write+0x45/0x6e
02:55:45: [<ffffffff8020b43e>] system_call+0x7e/0x83
03:01:34: smbclient invoked oom-killer: gfp_mask=0x4d0, order=2, oomkilladj=0
03:01:34:
03:01:34: Call Trace:
03:01:34: [<ffffffff80246053>] out_of_memory+0x71/0x1ba
03:01:34: [<ffffffff8024755d>] __alloc_pages+0x255/0x2d7
03:01:34: [<ffffffff8025cbd6>] cache_alloc_refill+0x2f4/0x60a
03:01:34: [<ffffffff8025be8a>] poison_obj+0x26/0x2f
03:01:34: [<ffffffff8040e0df>] __alloc_skb+0x31/0x121
03:01:34: [<ffffffff8040e0df>] __alloc_skb+0x31/0x121
03:01:34: [<ffffffff8040ab8b>] sock_alloc_send_skb+0x93/0x1dd
03:01:34: [<ffffffff8025d067>] __kmalloc_track_caller+0x9d/0xdb
03:01:34: [<ffffffff8040e109>] __alloc_skb+0x5b/0x121
03:01:34: [<ffffffff8040ab8b>] sock_alloc_send_skb+0x93/0x1dd
03:01:34: [<ffffffff8040a8ab>] release_sock+0xe/0x7f
03:01:34: [<ffffffff8048155c>] unix_stream_sendmsg+0x151/0x2ea
03:01:34: [<ffffffff80408349>] sock_aio_write+0xe5/0xf0
03:01:34: [<ffffffff8025f9b4>] do_sync_write+0xd1/0x118
03:01:34: [<ffffffff802357f5>] autoremove_wake_function+0x0/0x2e
03:01:34: [<ffffffff80222bb8>] update_curr+0xe6/0x10b
03:01:34: [<ffffffff80260118>] vfs_write+0xc0/0x136
03:01:34: [<ffffffff802605c2>] sys_write+0x45/0x6e
03:01:34: [<ffffffff8020b43e>] system_call+0x7e/0x83
05:48:04: scp invoked oom-killer: gfp_mask=0x4d0, order=1, oomkilladj=0
05:48:04:
05:48:04: Call Trace:
05:48:04: [<ffffffff80246053>] out_of_memory+0x71/0x1ba
05:48:04: [<ffffffff8024755d>] __alloc_pages+0x255/0x2d7
05:48:04: [<ffffffff8025cbd6>] cache_alloc_refill+0x2f4/0x60a
05:48:04: [<ffffffff8025be8a>] poison_obj+0x26/0x2f
05:48:04: [<ffffffff8040e0df>] __alloc_skb+0x31/0x121
05:48:04: [<ffffffff8040e0df>] __alloc_skb+0x31/0x121
05:48:04: [<ffffffff8040ab8b>] sock_alloc_send_skb+0x93/0x1dd
05:48:04: [<ffffffff8025d067>] __kmalloc_track_caller+0x9d/0xdb
05:48:04: [<ffffffff8040e109>] __alloc_skb+0x5b/0x121
05:48:04: [<ffffffff8040ab8b>] sock_alloc_send_skb+0x93/0x1dd
05:48:04: [<ffffffff8048155c>] unix_stream_sendmsg+0x151/0x2ea
05:48:04: [<ffffffff80243645>] file_read_actor+0x0/0x118
05:48:04: [<ffffffff80408349>] sock_aio_write+0xe5/0xf0
05:48:04: [<ffffffff8025f9b4>] do_sync_write+0xd1/0x118
05:48:04: [<ffffffff802357f5>] autoremove_wake_function+0x0/0x2e
05:48:04: [<ffffffff80260118>] vfs_write+0xc0/0x136
05:48:04: [<ffffffff802605c2>] sys_write+0x45/0x6e
05:48:04: [<ffffffff8020b43e>] system_call+0x7e/0x83
And here's the latest one, in full:
05:48:11: smbclient invoked oom-killer: gfp_mask=0x4d0, order=2, oomkilladj=0
05:48:12:
05:48:12: Call Trace:
05:48:12: [<ffffffff80246053>] out_of_memory+0x71/0x1ba
05:48:12: [<ffffffff8024755d>] __alloc_pages+0x255/0x2d7
05:48:12: [<ffffffff8025cbd6>] cache_alloc_refill+0x2f4/0x60a
05:48:12: [<ffffffff8025be8a>] poison_obj+0x26/0x2f
05:48:12: [<ffffffff8040e0df>] __alloc_skb+0x31/0x121
05:48:12: [<ffffffff8040e0df>] __alloc_skb+0x31/0x121
05:48:12: [<ffffffff8040ab8b>] sock_alloc_send_skb+0x93/0x1dd
05:48:12: [<ffffffff8025d067>] __kmalloc_track_caller+0x9d/0xdb
05:48:12: [<ffffffff8040e109>] __alloc_skb+0x5b/0x121
05:48:12: [<ffffffff8040ab8b>] sock_alloc_send_skb+0x93/0x1dd
05:48:12: [<ffffffff802f0627>] copy_user_generic_string+0x17/0x40
05:48:12: [<ffffffff8048155c>] unix_stream_sendmsg+0x151/0x2ea
05:48:12: [<ffffffff80408349>] sock_aio_write+0xe5/0xf0
05:48:12: [<ffffffff8025f9b4>] do_sync_write+0xd1/0x118
05:48:12: [<ffffffff802357f5>] autoremove_wake_function+0x0/0x2e
05:48:12: [<ffffffff80222bb8>] update_curr+0xe6/0x10b
05:48:12: [<ffffffff80260118>] vfs_write+0xc0/0x136
05:48:12: [<ffffffff802605c2>] sys_write+0x45/0x6e
05:48:12: [<ffffffff8020b43e>] system_call+0x7e/0x83
05:48:12:
05:48:12: Mem-info:
05:48:12: DMA per-cpu:
05:48:12: CPU 0: Hot: hi: 0, btch: 1 usd: 0 Cold: hi: 0, btch: 1 usd: 0
05:48:12: DMA32 per-cpu:
05:48:12: CPU 0: Hot: hi: 186, btch: 31 usd: 10 Cold: hi: 62, btch: 15 usd: 58
05:48:12: Active:67 inactive:1197 dirty:0 writeback:779 unstable:0
05:48:12: free:39163 slab:464538 mapped:518 pagetables:1800 bounce:0
05:48:12: DMA free:8040kB min:28kB low:32kB high:40kB active:0kB inactive:0kB present:11132kB pages_scanned:0 all_unreclaimable? yes
05:48:12: lowmem_reserve[]: 0 2003 2003 2003
05:48:12: DMA32 free:148612kB min:5712kB low:7140kB high:8568kB active:268kB inactive:4788kB present:2051184kB pages_scanned:7841 all_unreclaimable? yes
05:48:12: lowmem_reserve[]: 0 0 0 0
05:48:12: DMA: 56*4kB 1*8kB 0*16kB 0*32kB 0*64kB 1*128kB 0*256kB 1*512kB 1*1024kB 1*2048kB 1*4096kB = 8040kB
05:48:12: DMA32: 36459*4kB 55*8kB 2*16kB 0*32kB 0*64kB 0*128kB 1*256kB 0*512kB 0*1024kB 1*2048kB 0*4096kB = 148612kB
05:48:12: Swap cache: add 6853288, delete 6852498, find 1146430/2641691, race 0+0
05:48:12: Free swap = 5213316kB
05:48:12: Total swap = 5855208kB
05:48:12: Free swap: 5213316kB
05:48:12: 524000 pages of RAM
05:48:12: 11264 reserved pages
05:48:12: 241 pages shared
05:48:12: 790 pages swap cached
05:48:12: Out of memory: kill process 9156 (apache2) score 83536 or a child
05:48:12: Killed process 9156 (apache2)
Oh, FWIW, a later snapshot of /proc/alab_allocators:
skbuff_head_cache: 1746940 __alloc_skb+0x31/0x121
size-512: 1740532 tcp_send_ack+0x23/0x102
skbuff_fclone_cache: 152230 __alloc_skb+0x31/0x121
size-2048: 151603 tcp_sendmsg+0x1b5/0xae1
sysfs_dir_cache: 5279 sysfs_new_dirent+0x4b/0xec
size-512: 2837 sock_alloc_send_skb+0x93/0x1dd
Acpi-Operand: 2014 acpi_ut_allocate_object_desc_dbg+0x34/0x6e
size-32: 1989 sysfs_new_dirent+0x29/0xec
size-512: 1678 arp_create+0x4e/0x1cd
size-512: 1619 tcp_xmit_probe_skb+0x1f/0xcd
UDP: 1217 sk_alloc+0x25/0xaf
size-128: 1024 r1bio_pool_alloc+0x23/0x3b
size-128: 1024 nfsd_cache_init+0x2d/0xcf
Acpi-Namespace: 973 acpi_ns_create_node+0x2c/0x45
vm_area_struct: 804 copy_process+0x9f1/0x1108
dentry: 488 d_alloc+0x24/0x177
size-2048: 480 tcp_fragment+0xdf/0x4aa
anon_vma: 463 anon_vma_prepare+0x29/0x74
filp: 442 get_empty_filp+0x44/0xcd
ip_dst_cache: 421 dst_alloc+0x29/0x76
I'm backing out the ip1000a driver and seeing what happens.
^ permalink raw reply
* Re: 2.6.23-rc8 network problem. Mem leak? ip1000a?
From: Andrew Morton @ 2007-09-30 9:23 UTC (permalink / raw)
To: linux; +Cc: linux-kernel, netdev, Francois Romieu
In-Reply-To: <20070930075956.11935.qmail@science.horizon.com>
On 30 Sep 2007 03:59:56 -0400 linux@horizon.com wrote:
> > ntpd. Sounds like pps leaking to me.
>
> That's what I'd think, except that pps does no allocation in the normal
> running state, so there's nothing to leak. The interrupt path just
> records the time in some preallocated, static buffers and wakes up
> blocked readers. The read path copies the latest data out of those
> static buffers. There's allocation when the PPS device is created,
> and more when it's opened.
OK. Did you try to reproduce it without the pps patch applied?
> >> Can anyone offer some diagnosis advice?
>
> > CONFIG_DEBUG_SLAB_LEAK?
>
> Ah, thanks you; I've been using SLUB which doesn't support this option.
> Here's what I've extracted. I've only presented the top few
> slab_allocators and a small subset of the oom-killer messages, but I
> have full copies if desired. Unfortunately, I've discovered that the
> machine doesn't live in this unhappy state forever. Indeed, I'm not
> sure if killing ntpd "fixes" anything; my previous observations
> may have been optimistic ignorance.
>
> (For my own personal reference looking for more oom-kill, I nuked ntpd
> at 06:46:56. And the oom-kills are continuing, with the latest at
> 07:43:52.)
>
> Anyway, I have a bunch of information from the slab_allocators file, but
> I'm not quire sure how to make sense of it.
>
>
> With a machine in the unhappy state and firing the OOM killer, the top
> 20 slab_allocators are:
> $ sort -rnk2 /proc/slab_allocators | head -20
> skbuff_head_cache: 1712746 __alloc_skb+0x31/0x121
> size-512: 1706572 tcp_send_ack+0x23/0x102
> skbuff_fclone_cache: 149113 __alloc_skb+0x31/0x121
> size-2048: 148500 tcp_sendmsg+0x1b5/0xae1
> sysfs_dir_cache: 5289 sysfs_new_dirent+0x4b/0xec
> size-512: 2613 sock_alloc_send_skb+0x93/0x1dd
> Acpi-Operand: 2014 acpi_ut_allocate_object_desc_dbg+0x34/0x6e
> size-32: 1995 sysfs_new_dirent+0x29/0xec
> vm_area_struct: 1679 mmap_region+0x18f/0x421
> size-512: 1618 tcp_xmit_probe_skb+0x1f/0xcd
> size-512: 1571 arp_create+0x4e/0x1cd
> vm_area_struct: 1544 copy_process+0x9f1/0x1108
> anon_vma: 1448 anon_vma_prepare+0x29/0x74
> filp: 1201 get_empty_filp+0x44/0xcd
> UDP: 1173 sk_alloc+0x25/0xaf
> size-128: 1048 r1bio_pool_alloc+0x23/0x3b
> size-128: 1024 nfsd_cache_init+0x2d/0xcf
> Acpi-Namespace: 973 acpi_ns_create_node+0x2c/0x45
> vm_area_struct: 717 split_vma+0x33/0xe5
> dentry: 594 d_alloc+0x24/0x177
>
> I'm not sure quite what "normal" numbers are, but I do wonder why there
> are 1.7 million TCP acks buffered in the system. Shouldn't they be
> transmitted and deallocated pretty quickly?
Yeah, that's an skbuff leak.
> This machine receives more data than it sends, so I'd expect acks to
> outnumber "real" packets. Could the ip1000a driver's transmit path be
> leaking skbs somehow?
Absolutely. Normally a driver's transmit completion interrupt handler will
run dev_kfree_skb_irq() against the skbs which have been fully sent.
However it'd be darned odd if the driver was leaking only tcp acks.
I can find no occurrence of "dev_kfree_skb" in drivers/net/ipg.c, which is
suspicious.
Where did you get your ipg.c from, btw? davem's tree? rc8-mm1? rc8-mm2??
> that would also explain the "flailing" of the
> oom-killer; it can't associate the allocations with a process.
>
> Here's /proc/meminfo:
> MemTotal: 1035756 kB
> MemFree: 43508 kB
> Buffers: 72920 kB
> Cached: 224056 kB
> SwapCached: 344916 kB
> Active: 664976 kB
> Inactive: 267656 kB
> SwapTotal: 4950368 kB
> SwapFree: 3729384 kB
> Dirty: 6460 kB
> Writeback: 0 kB
> AnonPages: 491708 kB
> Mapped: 79232 kB
> Slab: 41324 kB
> SReclaimable: 25008 kB
> SUnreclaim: 16316 kB
> PageTables: 8132 kB
> NFS_Unstable: 0 kB
> Bounce: 0 kB
> CommitLimit: 5468244 kB
> Committed_AS: 1946008 kB
> VmallocTotal: 253900 kB
> VmallocUsed: 2672 kB
> VmallocChunk: 251228 kB
I assume that meminfo was not captured when the system was ooming? There
isn't much slab there.
^ permalink raw reply
* Re: 2.6.23-rc8 network problem. Mem leak? ip1000a?
From: linux @ 2007-09-30 11:40 UTC (permalink / raw)
To: akpm, linux; +Cc: jesse, linux-kernel, netdev, romieu, s.l-h
In-Reply-To: <20070930022347.37514be3.akpm@linux-foundation.org>
> OK. Did you try to reproduce it without the pps patch applied?
No. But I've yanked the ip1000a driver (using old crufy vendor-supplied
out-of-kernel module) and the problems are GONE.
>> This machine receives more data than it sends, so I'd expect acks to
>> outnumber "real" packets. Could the ip1000a driver's transmit path be
>> leaking skbs somehow?
> Absolutely. Normally a driver's transmit completion interrupt handler will
> run dev_kfree_skb_irq() against the skbs which have been fully sent.
>
> However it'd be darned odd if the driver was leaking only tcp acks.
It's leaking lots of things... you can see ARP packets in there and
all sorts of stuff. But the big traffic hog is BackupPC doing inbound
rsyncs all night long, which generates a lot of acks. Those are the
packets it sends, so those are the packets that get leaked.
> I can find no occurrence of "dev_kfree_skb" in drivers/net/ipg.c, which is
> suspicious.
Look for "IPG_DEV_KFREE_SKB", which is a wrapper macro. (Or just add
"-i" to your grep.) It should probably be deleted (it just expands to
dev_kfree_skb), but was presumably useful to someone during development.
> Where did you get your ipg.c from, btw? davem's tree? rc8-mm1? rc8-mm2??
As I wrote originally, I got it from
http://marc.info/?l=linux-netdev&m=118980588419882
which was a reuqest for mainline submission.
If there are other patches floating around, I'm happy to try them.
Now that I know what to look for, it's easy to spot the leak before OOM.
> I assume that meminfo was not captured when the system was ooming? There
> isn't much slab there.
Oops, sorry. I captured slabinfo but not meminfo.
Thank you very much! Sorry to jump the gun and post a lot before I had
all the data, but if it WAS a problem in -rc8, I wanted to mention it
before -final.
Now, the rush is to get the ip1000a driver fixed before the merge
window opens. I've added all the ip1000a developers to the Cc: list in
an attempt to speed that up.
^ permalink raw reply
* Re: [Devel] Re: [PATCH 2/5] net: Make rtnetlink infrastructure network namespace aware
From: Denis V. Lunev @ 2007-09-30 13:13 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: Patrick McHardy, Linux Containers, netdev, David Miller
In-Reply-To: <m1myv56vb3.fsf@ebiederm.dsl.xmission.com>
Hmm, so it looks like we do not need this queue processing at all...
Regards,
Den
Eric W. Biederman wrote:
> Patrick McHardy <kaber@trash.net> writes:
>
>> Maybe I can save you some time: we used to do down_trylock()
>> for the rtnl mutex, so senders would simply return if someone
>> else was already processing the queue *or* the rtnl was locked
>> for some other reason. In the first case the process already
>> processing the queue would also process the new messages, but
>> if it the rtnl was locked for some other reason (for example
>> during module registration) the message would sit in the
>> queue until the next rtnetlink sendmsg call, which is why
>> rtnl_unlock does queue processing. Commit 6756ae4b changed
>> the down_trylock to mutex_lock, so senders will now simply wait
>> until the mutex is released and then call netlink_run_queue
>> themselves. This means its not needed anymore.
>
> Sounds reasonable.
>
> I started looking through the code paths and I currently cannot
> see anything that would leave a message on a kernel rtnl socket.
>
> However I did a quick test adding a WARN_ON if there were any messages
> found in the queue during rtnl_unlock and I found this code path
> getting invoked from linkwatch_event. So there is clearly something I
> don't understand, and it sounds at odds just a bit from your
> description.
>
> If we can remove the extra queue processing that would be great,
> as it looks like a nice way to simplify the locking and the special
> cases in the code.
>
> Eric
^ permalink raw reply
* Re: [PKT_SCHED]: Add stateless NAT
From: Patrick McHardy @ 2007-09-30 15:38 UTC (permalink / raw)
To: Herbert Xu; +Cc: David Miller, hadi, netdev, kuznet
In-Reply-To: <20070930004341.GA7769@gondor.apana.org.au>
Herbert Xu wrote:
> On Sun, Sep 30, 2007 at 08:26:01AM +0800, Herbert Xu wrote:
>
>>Indeed. The only other case I can think of is defragmentation.
>>But even there we should be able to squeeze it into the original
>>skb :)
>
>
> OK it won't be pretty but it's definitely doable. We simply
> swap the contents of that skb with the head skb that would've
> otherwise been returned.
>
> If this is the only place in netfilter where we need to modify
> *pskb then I think it's worthwhile.
>
> So the question is are there any other places that we haven't
> covered which also needs to change *pskb?
>From a quick look it seems at least IPv4 and IPv6 don't need to
change the skb anywhere else. It seems to be necessary for
bridging though to unshare the skb. OTOH br_netfilter.c already
unshares the skb for IPv4 and IPv6, so we could simply do this
unconditionally before calling the hooks.
^ permalink raw reply
* Re: [PATCH 2/5] net: Make rtnetlink infrastructure network namespace aware
From: Patrick McHardy @ 2007-09-30 15:39 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: David Miller, netdev, Linux Containers
In-Reply-To: <m1myv56vb3.fsf@ebiederm.dsl.xmission.com>
Eric W. Biederman wrote:
> Patrick McHardy <kaber@trash.net> writes:
>
>
>>Maybe I can save you some time: we used to do down_trylock()
>>for the rtnl mutex, so senders would simply return if someone
>>else was already processing the queue *or* the rtnl was locked
>>for some other reason. In the first case the process already
>>processing the queue would also process the new messages, but
>>if it the rtnl was locked for some other reason (for example
>>during module registration) the message would sit in the
>>queue until the next rtnetlink sendmsg call, which is why
>>rtnl_unlock does queue processing. Commit 6756ae4b changed
>>the down_trylock to mutex_lock, so senders will now simply wait
>>until the mutex is released and then call netlink_run_queue
>>themselves. This means its not needed anymore.
>
>
> Sounds reasonable.
>
> I started looking through the code paths and I currently cannot
> see anything that would leave a message on a kernel rtnl socket.
>
> However I did a quick test adding a WARN_ON if there were any messages
> found in the queue during rtnl_unlock and I found this code path
> getting invoked from linkwatch_event. So there is clearly something I
> don't understand, and it sounds at odds just a bit from your
> description.
That sounds like a bug. Did you place the WARN_ON before or after
the mutex_unlock()?
^ permalink raw reply
* Re: [PATCH] rtnl: Simplify ASSERT_RTNL
From: Patrick McHardy @ 2007-09-30 15:47 UTC (permalink / raw)
To: Herbert Xu; +Cc: Eric W. Biederman, davem, netdev, oliver, linux-usb-devel
In-Reply-To: <20070930002421.GA7502@gondor.apana.org.au>
Herbert Xu wrote:
> On Sat, Sep 29, 2007 at 05:32:41PM +0200, Patrick McHardy wrote:
>
>>For unicast addresses its not strictly necessary since they may
>>only be changed under the RTNL anyway. The reason why it takes
>>the tx_lock is for consistency with multicast address handling,
>>which can't rely on the RTNL since IPv6 changes them from
>>BH context. The idea was that the ->set_rx_mode function should
>>handle both secondary unicast and multicast addresses for
>>simplicity.
>
>
> In any case, coming back to the original question, the RTNL
> assertion is simply wrong in this case because if we're being
> called from IPv6 then the RTNL won't even be held.
>
> So I think we need to
>
> 1) Move the assert into dev_set_promiscuity.
> 2) Take the TX lock in dev_set_promiscuity.
In the IPv6 case we're only changing the multicast list,
so we're never calling into __dev_set_promiscuity.
I actually even added a comment about this :)
/* Unicast addresses changes may only happen under the rtnl,
* therefore calling __dev_set_promiscuity here is safe.
*/
I would prefer to keep the ASSERT_RTNL in __dev_set_promiscuity
since it also covers the __dev_set_rx_mode path. How about
adding an ASSERT_RTNL_ATOMIC without the might_sleep or simply
open coding it?
^ permalink raw reply
* ehea work queues
From: Anton Blanchard @ 2007-09-30 16:20 UTC (permalink / raw)
To: themann, netdev
Hi,
I booted 2.6.23-rc8 and noticed that ehea loves its workqueues:
# ps aux|grep ehea
root 3266 0.0 0.0 0 0 ? S< 11:02 0:00 [ehea_driver_wq/]
root 3268 0.0 0.0 0 0 ? S< 11:02 0:00 [ehea_driver_wq/]
root 3269 0.0 0.0 0 0 ? S< 11:02 0:00 [ehea_driver_wq/]
root 3270 0.0 0.0 0 0 ? S< 11:02 0:00 [ehea_driver_wq/]
root 3271 0.0 0.0 0 0 ? S< 11:02 0:00 [ehea_driver_wq/]
root 3272 0.0 0.0 0 0 ? S< 11:02 0:00 [ehea_driver_wq/]
root 3273 0.0 0.0 0 0 ? S< 11:02 0:00 [ehea_driver_wq/]
root 3274 0.0 0.0 0 0 ? S< 11:02 0:00 [ehea_driver_wq/]
root 3275 0.0 0.0 0 0 ? S< 11:02 0:00 [ehea_wq/0]
root 3276 0.0 0.0 0 0 ? S< 11:02 0:00 [ehea_wq/1]
root 3278 0.0 0.0 0 0 ? S< 11:02 0:00 [ehea_wq/2]
root 3279 0.0 0.0 0 0 ? S< 11:02 0:00 [ehea_wq/3]
root 3280 0.0 0.0 0 0 ? S< 11:02 0:00 [ehea_wq/4]
root 3281 0.0 0.0 0 0 ? S< 11:02 0:00 [ehea_wq/5]
root 3282 0.0 0.0 0 0 ? S< 11:02 0:00 [ehea_wq/6]
root 3283 0.0 0.0 0 0 ? S< 11:02 0:00 [ehea_wq/7]
(notice also that the ehea_driver_wq/XXX exceeds TASK_COMM_LEN).
Since they are both infrequent events and not performance critical
(memory hotplug and driver reset), can we just use schedule_work?
Anton
^ permalink raw reply
* [PATCH][E1000E] some cleanups
From: jamal @ 2007-09-30 17:41 UTC (permalink / raw)
To: Kok, Auke; +Cc: netdev
[-- Attachment #1: Type: text/plain, Size: 843 bytes --]
Auke,
heres part of something i promised.
I couldnt do any packet testing on because 82571EB is disabled in the
driver. I uncommented the code out in the table, but the best i could
get was the module loading, some probing and some sysfs renaming
failures (probably a debianism); the machine access is intermittent, so
thats as far as i could go. In any case, you probably have a good reason
for disabling that chip. So, heres the patch, the burden of testing now
falls on you ;->
Once you have 82571EB on and kicking, my next steps are to kill LLTX
then add batching on top.
BTW, since this driver is just for PCIE, would you take a similar patch
for non-PCIE e1000?
comment:
There used to be an "mmiowb()" call right after the dma wake which is
gone now; is this unneeded with pcie? I have restored it, look for the
"XXX".
cheers,
jamal
[-- Attachment #2: e1000e-p1 --]
[-- Type: text/plain, Size: 10445 bytes --]
[E1000E] some cleanups
This patch makes the xmit path code a lot more readable and reusable.
preps for removing LLTX.
Signed-off-by: Jamal Hadi Salim <hadi@cyberus.ca>
---
commit ad63c288ce980907f68d94d5faac08625c0b1782
tree 296a6da371b98c6488c544a8910941ea6d8c18a8
parent 7f5d0afdff875b2c4957031f8934741aefe257cc
author Jamal Hadi Salim <hadi@cyberus.ca> Sun, 30 Sep 2007 13:26:00 -0400
committer Jamal Hadi Salim <hadi@cyberus.ca> Sun, 30 Sep 2007 13:26:00 -0400
drivers/net/e1000e/netdev.c | 187 +++++++++++++++++++++++++++----------------
1 files changed, 119 insertions(+), 68 deletions(-)
diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c
index 4a21d7d..5043504 100644
--- a/drivers/net/e1000e/netdev.c
+++ b/drivers/net/e1000e/netdev.c
@@ -3076,6 +3076,18 @@ link_up:
#define E1000_TX_FLAGS_VLAN_MASK 0xffff0000
#define E1000_TX_FLAGS_VLAN_SHIFT 16
+struct e1000_tx_cbdata {
+ int count;
+ unsigned int max_per_txd;
+ unsigned int nr_frags;
+ unsigned int mss;
+ unsigned int tx_flags;
+};
+
+#define E1000_SKB_CB(__skb) ((struct e1000_tx_cbdata *)&((__skb)->cb[8]))
+#define NETDEV_TX_DROPPED -5
+#define TXD_USE_COUNT(S, X) (((S) >> (X)) + 1)
+
static int e1000_tso(struct e1000_adapter *adapter,
struct sk_buff *skb)
{
@@ -3194,8 +3206,7 @@ static bool e1000_tx_csum(struct e1000_adapter *adapter, struct sk_buff *skb)
static int e1000_tx_map(struct e1000_adapter *adapter,
struct sk_buff *skb, unsigned int first,
- unsigned int max_per_txd, unsigned int nr_frags,
- unsigned int mss)
+ struct e1000_tx_cbdata *cb)
{
struct e1000_ring *tx_ring = adapter->tx_ring;
struct e1000_buffer *buffer_info;
@@ -3207,11 +3218,11 @@ static int e1000_tx_map(struct e1000_adapter *adapter,
while (len) {
buffer_info = &tx_ring->buffer_info[i];
- size = min(len, max_per_txd);
+ size = min(len, cb->max_per_txd);
/* Workaround for premature desc write-backs
* in TSO mode. Append 4-byte sentinel desc */
- if (mss && !nr_frags && size == len && size > 8)
+ if (cb->mss && !cb->nr_frags && size == len && size > 8)
size -= 4;
buffer_info->length = size;
@@ -3237,7 +3248,7 @@ static int e1000_tx_map(struct e1000_adapter *adapter,
i = 0;
}
- for (f = 0; f < nr_frags; f++) {
+ for (f = 0; f < cb->nr_frags; f++) {
struct skb_frag_struct *frag;
frag = &skb_shinfo(skb)->frags[f];
@@ -3246,10 +3257,11 @@ static int e1000_tx_map(struct e1000_adapter *adapter,
while (len) {
buffer_info = &tx_ring->buffer_info[i];
- size = min(len, max_per_txd);
+ size = min(len, cb->max_per_txd);
/* Workaround for premature desc write-backs
* in TSO mode. Append 4-byte sentinel desc */
- if (mss && f == (nr_frags-1) && size == len && size > 8)
+ if (cb->mss && f == (cb->nr_frags-1) &&
+ size == len && size > 8)
size -= 4;
buffer_info->length = size;
@@ -3334,18 +3346,7 @@ static void e1000_tx_queue(struct e1000_adapter *adapter,
}
tx_desc->lower.data |= cpu_to_le32(adapter->txd_cmd);
-
- /* Force memory writes to complete before letting h/w
- * know there are new descriptors to fetch. (Only
- * applicable for weak-ordered memory model archs,
- * such as IA-64). */
- wmb();
-
tx_ring->next_to_use = i;
- writel(i, adapter->hw.hw_addr + tx_ring->tail);
- /* we need this if more than one processor can write to our tail
- * at a time, it synchronizes IO on IA64/Altix systems */
- mmiowb();
}
#define MINIMUM_DHCP_PACKET_SIZE 282
@@ -3417,45 +3418,54 @@ static int e1000_maybe_stop_tx(struct net_device *netdev, int size)
return __e1000_maybe_stop_tx(netdev, size);
}
-#define TXD_USE_COUNT(S, X) (((S) >> (X)) + 1 )
-static int e1000_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
+static void
+e1000_complete_tx(struct net_device *netdev)
{
struct e1000_adapter *adapter = netdev_priv(netdev);
struct e1000_ring *tx_ring = adapter->tx_ring;
- unsigned int first;
- unsigned int max_per_txd = E1000_MAX_PER_TXD;
+ /* Force memory writes to complete before letting h/w
+ * know there are new descriptors to fetch. (Only
+ * applicable for weak-ordered memory model archs,
+ * such as IA-64). */
+ wmb();
+ writel(tx_ring->next_to_use, adapter->hw.hw_addr + tx_ring->tail);
+ /* XXX: we need this if more than one processor can write to
+ * our tail at a time, it syncronizes IO on IA64/Altix systems */
+ mmiowb();
+ netdev->trans_start = jiffies;
+}
+
+static int e1000_prep_frame(struct sk_buff *skb, struct net_device *netdev)
+{
+ struct e1000_adapter *adapter = netdev_priv(netdev);
unsigned int max_txd_pwr = E1000_MAX_TXD_PWR;
- unsigned int tx_flags = 0;
unsigned int len = skb->len;
- unsigned long irq_flags;
- unsigned int nr_frags = 0;
- unsigned int mss = 0;
- int count = 0;
- int tso;
+ struct e1000_tx_cbdata *cb = E1000_SKB_CB(skb);
unsigned int f;
- len -= skb->data_len;
- if (test_bit(__E1000_DOWN, &adapter->state)) {
- dev_kfree_skb_any(skb);
- return NETDEV_TX_OK;
- }
+ cb->nr_frags = 0;
+ cb->mss = 0;
+ cb->count = 0;
+ cb->max_per_txd = E1000_MAX_PER_TXD;
+
+ len -= skb->data_len;
if (skb->len <= 0) {
dev_kfree_skb_any(skb);
- return NETDEV_TX_OK;
+ return NETDEV_TX_DROPPED;
}
- mss = skb_shinfo(skb)->gso_size;
+ cb->mss = skb_shinfo(skb)->gso_size;
/* The controller does a simple calculation to
* make sure there is enough room in the FIFO before
* initiating the DMA for each buffer. The calc is:
* 4 = ceil(buffer len/mss). To make sure we don't
* overrun the FIFO, adjust the max buffer len if mss
* drops. */
- if (mss) {
+ if (cb->mss) {
u8 hdr_len;
- max_per_txd = min(mss << 2, max_per_txd);
- max_txd_pwr = fls(max_per_txd) - 1;
+ cb->max_per_txd = min(cb->mss << 2, cb->max_per_txd);
+ max_txd_pwr = fls(cb->max_per_txd) - 1;
/* TSO Workaround for 82571/2/3 Controllers -- if skb->data
* points to just header, pull a few bytes of payload from
@@ -3469,80 +3479,121 @@ static int e1000_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
ndev_err(netdev,
"__pskb_pull_tail failed.\n");
dev_kfree_skb_any(skb);
- return NETDEV_TX_OK;
+ return NETDEV_TX_DROPPED;
}
len = skb->len - skb->data_len;
}
}
/* reserve a descriptor for the offload context */
- if ((mss) || (skb->ip_summed == CHECKSUM_PARTIAL))
- count++;
- count++;
+ if ((cb->mss) || (skb->ip_summed == CHECKSUM_PARTIAL))
+ cb->count++;
+ cb->count++;
- count += TXD_USE_COUNT(len, max_txd_pwr);
+ cb->count += TXD_USE_COUNT(len, max_txd_pwr);
- nr_frags = skb_shinfo(skb)->nr_frags;
- for (f = 0; f < nr_frags; f++)
- count += TXD_USE_COUNT(skb_shinfo(skb)->frags[f].size,
+ cb->nr_frags = skb_shinfo(skb)->nr_frags;
+ for (f = 0; f < cb->nr_frags; f++)
+ cb->count += TXD_USE_COUNT(skb_shinfo(skb)->frags[f].size,
max_txd_pwr);
if (adapter->hw.mac.tx_pkt_filtering)
e1000_transfer_dhcp_info(adapter, skb);
- if (!spin_trylock_irqsave(&adapter->tx_queue_lock, irq_flags))
- /* Collision - tell upper layer to requeue */
- return NETDEV_TX_LOCKED;
+ if (adapter->vlgrp && vlan_tx_tag_present(skb)) {
+ cb->tx_flags |= E1000_TX_FLAGS_VLAN;
+ cb->tx_flags |= (vlan_tx_tag_get(skb) <<
+ E1000_TX_FLAGS_VLAN_SHIFT);
+ }
+
+ return NETDEV_TX_OK;
+}
+
+static int e1000_queue_frame(struct sk_buff *skb, struct net_device *netdev)
+{
+ struct e1000_adapter *adapter = netdev_priv(netdev);
+ struct e1000_ring *tx_ring = adapter->tx_ring;
+ unsigned int first;
+ int tso;
+ struct e1000_tx_cbdata *cb = E1000_SKB_CB(skb);
+
/* need: count + 2 desc gap to keep tail from touching
* head, otherwise try next time */
- if (e1000_maybe_stop_tx(netdev, count + 2)) {
- spin_unlock_irqrestore(&adapter->tx_queue_lock, irq_flags);
+ if (e1000_maybe_stop_tx(netdev, cb->count + 2))
return NETDEV_TX_BUSY;
- }
-
- if (adapter->vlgrp && vlan_tx_tag_present(skb)) {
- tx_flags |= E1000_TX_FLAGS_VLAN;
- tx_flags |= (vlan_tx_tag_get(skb) << E1000_TX_FLAGS_VLAN_SHIFT);
- }
first = tx_ring->next_to_use;
tso = e1000_tso(adapter, skb);
if (tso < 0) {
dev_kfree_skb_any(skb);
- spin_unlock_irqrestore(&adapter->tx_queue_lock, irq_flags);
- return NETDEV_TX_OK;
+ return NETDEV_TX_DROPPED;
}
if (tso)
- tx_flags |= E1000_TX_FLAGS_TSO;
+ cb->tx_flags |= E1000_TX_FLAGS_TSO;
else if (e1000_tx_csum(adapter, skb))
- tx_flags |= E1000_TX_FLAGS_CSUM;
+ cb->tx_flags |= E1000_TX_FLAGS_CSUM;
/* Old method was to assume IPv4 packet by default if TSO was enabled.
* 82571 hardware supports TSO capabilities for IPv6 as well...
* no longer assume, we must. */
if (skb->protocol == htons(ETH_P_IP))
- tx_flags |= E1000_TX_FLAGS_IPV4;
+ cb->tx_flags |= E1000_TX_FLAGS_IPV4;
- count = e1000_tx_map(adapter, skb, first, max_per_txd, nr_frags, mss);
- if (count < 0) {
+ cb->count = e1000_tx_map(adapter, skb, first, cb);
+ if (cb->count < 0) {
/* handle pci_map_single() error in e1000_tx_map */
dev_kfree_skb_any(skb);
- spin_unlock_irqrestore(&adapter->tx_queue_lock, irq_flags);
+ return NETDEV_TX_DROPPED;
+ }
+
+ e1000_tx_queue(adapter, cb->tx_flags, cb->count);
+
+ return NETDEV_TX_OK;
+}
+
+static int e1000_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
+{
+ int ret = NETDEV_TX_OK;
+ struct e1000_adapter *adapter = netdev_priv(netdev);
+ unsigned long irq_flags;
+ struct e1000_tx_cbdata *cb = E1000_SKB_CB(skb);
+
+ ret = e1000_prep_frame(skb, netdev);
+ if (unlikely(ret != NETDEV_TX_OK))
+ return NETDEV_TX_OK;
+
+ if (test_bit(__E1000_DOWN, &adapter->state)) {
+ dev_kfree_skb_any(skb);
return NETDEV_TX_OK;
}
- e1000_tx_queue(adapter, tx_flags, count);
+ if (!spin_trylock_irqsave(&adapter->tx_queue_lock, irq_flags))
+ /* Collision - tell upper layer to requeue */
+ return NETDEV_TX_LOCKED;
- netdev->trans_start = jiffies;
+ /* need: count + 2 desc gap to keep tail from touching
+ * head, otherwise try next time */
+ if (e1000_maybe_stop_tx(netdev, cb->count + 2)) {
+ spin_unlock_irqrestore(&adapter->tx_queue_lock, irq_flags);
+ return NETDEV_TX_BUSY;
+ }
+
+ ret = e1000_queue_frame(skb, netdev);
+
+ if (ret == NETDEV_TX_OK)
+ e1000_complete_tx(netdev);
+
+ if (ret == NETDEV_TX_DROPPED)
+ ret = NETDEV_TX_OK;
/* Make sure there is space in the ring for the next send. */
e1000_maybe_stop_tx(netdev, MAX_SKB_FRAGS + 2);
spin_unlock_irqrestore(&adapter->tx_queue_lock, irq_flags);
- return NETDEV_TX_OK;
+ return ret;
}
/**
^ permalink raw reply related
* [PATCH][TG3]Some cleanups
From: jamal @ 2007-09-30 18:11 UTC (permalink / raw)
To: Michael Chan, Matt Carlson; +Cc: netdev
[-- Attachment #1: Type: text/plain, Size: 187 bytes --]
Here are some non-batching related changes that i have in my batching
tree. Like the e1000e, they make the xmit code more readable.
I wouldnt mind if you take them over.
cheers,
jamal
[-- Attachment #2: tg3-p1 --]
[-- Type: text/plain, Size: 13376 bytes --]
[TG3] Some cleanups
These cleanups make the xmit path code better functionally organized.
Matt Carlson contributed the moving of the VLAN formatting into
XXXX_prep_frame() portion.
Signed-off-by: Jamal Hadi Salim <hadi@cyberus.ca>
---
commit 260dbcc4b0195897c539c5ff79d95afdddeb3378
tree b2047b0e474abb9f05dd40c22af7f0a86369957d
parent ad63c288ce980907f68d94d5faac08625c0b1782
author Jamal Hadi Salim <hadi@cyberus.ca> Sun, 30 Sep 2007 14:01:46 -0400
committer Jamal Hadi Salim <hadi@cyberus.ca> Sun, 30 Sep 2007 14:01:46 -0400
drivers/net/tg3.c | 278 ++++++++++++++++++++++++++++++++---------------------
1 files changed, 169 insertions(+), 109 deletions(-)
diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index d4ac6e9..5a864bd 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -3910,47 +3910,69 @@ static void tg3_set_txd(struct tg3 *tp, int entry,
txd->vlan_tag = vlan_tag << TXD_VLAN_TAG_SHIFT;
}
-/* hard_start_xmit for devices that don't have any bugs and
- * support TG3_FLG2_HW_TSO_2 only.
- */
-static int tg3_start_xmit(struct sk_buff *skb, struct net_device *dev)
+struct tg3_tx_cbdata {
+ u32 base_flags;
+ unsigned int mss;
+};
+#define TG3_SKB_CB(__skb) ((struct tg3_tx_cbdata *)&((__skb)->cb[0]))
+#define NETDEV_TX_DROPPED -5
+
+static int tg3_prep_bug_frame(struct sk_buff *skb, struct net_device *dev)
{
+ struct tg3_tx_cbdata *cb = TG3_SKB_CB(skb);
+#if TG3_VLAN_TAG_USED
struct tg3 *tp = netdev_priv(dev);
- dma_addr_t mapping;
- u32 len, entry, base_flags, mss;
+ u32 vlantag = 0;
- len = skb_headlen(skb);
+ if (tp->vlgrp != NULL && vlan_tx_tag_present(skb))
+ vlantag = (TXD_FLAG_VLAN | (vlan_tx_tag_get(skb) << 16));
- /* We are running in BH disabled context with netif_tx_lock
- * and TX reclaim runs via tp->napi.poll inside of a software
- * interrupt. Furthermore, IRQ processing runs lockless so we have
- * no IRQ context deadlocks to worry about either. Rejoice!
- */
- if (unlikely(tg3_tx_avail(tp) <= (skb_shinfo(skb)->nr_frags + 1))) {
- if (!netif_queue_stopped(dev)) {
- netif_stop_queue(dev);
+ cb->base_flags = vlantag;
+#endif
- /* This is a hard error, log it. */
- printk(KERN_ERR PFX "%s: BUG! Tx Ring full when "
- "queue awake!\n", dev->name);
+ cb->mss = skb_shinfo(skb)->gso_size;
+ if (cb->mss != 0) {
+ if (skb_header_cloned(skb) &&
+ pskb_expand_head(skb, 0, 0, GFP_ATOMIC)) {
+ dev_kfree_skb(skb);
+ return NETDEV_TX_DROPPED;
}
- return NETDEV_TX_BUSY;
+
+ cb->base_flags |= (TXD_FLAG_CPU_PRE_DMA |
+ TXD_FLAG_CPU_POST_DMA);
}
- entry = tp->tx_prod;
- base_flags = 0;
- mss = 0;
- if ((mss = skb_shinfo(skb)->gso_size) != 0) {
+ if (skb->ip_summed == CHECKSUM_PARTIAL)
+ cb->base_flags |= TXD_FLAG_TCPUDP_CSUM;
+
+ return NETDEV_TX_OK;
+}
+
+static int tg3_prep_frame(struct sk_buff *skb, struct net_device *dev)
+{
+ struct tg3_tx_cbdata *cb = TG3_SKB_CB(skb);
+#if TG3_VLAN_TAG_USED
+ struct tg3 *tp = netdev_priv(dev);
+ u32 vlantag = 0;
+
+ if (tp->vlgrp != NULL && vlan_tx_tag_present(skb))
+ vlantag = (TXD_FLAG_VLAN | (vlan_tx_tag_get(skb) << 16));
+
+ cb->base_flags = vlantag;
+#endif
+
+ cb->mss = skb_shinfo(skb)->gso_size;
+ if (cb->mss != 0) {
int tcp_opt_len, ip_tcp_len;
if (skb_header_cloned(skb) &&
pskb_expand_head(skb, 0, 0, GFP_ATOMIC)) {
dev_kfree_skb(skb);
- goto out_unlock;
+ return NETDEV_TX_DROPPED;
}
if (skb_shinfo(skb)->gso_type & SKB_GSO_TCPV6)
- mss |= (skb_headlen(skb) - ETH_HLEN) << 9;
+ cb->mss |= (skb_headlen(skb) - ETH_HLEN) << 9;
else {
struct iphdr *iph = ip_hdr(skb);
@@ -3958,32 +3980,58 @@ static int tg3_start_xmit(struct sk_buff *skb, struct net_device *dev)
ip_tcp_len = ip_hdrlen(skb) + sizeof(struct tcphdr);
iph->check = 0;
- iph->tot_len = htons(mss + ip_tcp_len + tcp_opt_len);
- mss |= (ip_tcp_len + tcp_opt_len) << 9;
+ iph->tot_len = htons(cb->mss + ip_tcp_len
+ + tcp_opt_len);
+ cb->mss |= (ip_tcp_len + tcp_opt_len) << 9;
}
- base_flags |= (TXD_FLAG_CPU_PRE_DMA |
+ cb->base_flags |= (TXD_FLAG_CPU_PRE_DMA |
TXD_FLAG_CPU_POST_DMA);
tcp_hdr(skb)->check = 0;
}
else if (skb->ip_summed == CHECKSUM_PARTIAL)
- base_flags |= TXD_FLAG_TCPUDP_CSUM;
-#if TG3_VLAN_TAG_USED
- if (tp->vlgrp != NULL && vlan_tx_tag_present(skb))
- base_flags |= (TXD_FLAG_VLAN |
- (vlan_tx_tag_get(skb) << 16));
-#endif
+ cb->base_flags |= TXD_FLAG_TCPUDP_CSUM;
+
+ return NETDEV_TX_OK;
+}
+
+void tg3_kick_DMA(struct net_device *dev)
+{
+ struct tg3 *tp = netdev_priv(dev);
+ u32 entry = tp->tx_prod;
+
+ /* Packets are ready, update Tx producer idx local and on card. */
+ tw32_tx_mbox((MAILBOX_SNDHOST_PROD_IDX_0 + TG3_64BIT_REG_LOW), entry);
+
+ if (unlikely(tg3_tx_avail(tp) <= (MAX_SKB_FRAGS + 1))) {
+ netif_stop_queue(dev);
+ if (tg3_tx_avail(tp) >= TG3_TX_WAKEUP_THRESH(tp))
+ netif_wake_queue(dev);
+ }
+
+ mmiowb();
+ dev->trans_start = jiffies;
+}
+static int tg3_enqueue(struct sk_buff *skb, struct net_device *dev)
+{
+ struct tg3 *tp = netdev_priv(dev);
+ dma_addr_t mapping;
+ u32 len, entry;
+ struct tg3_tx_cbdata *cb = TG3_SKB_CB(skb);
+
+ entry = tp->tx_prod;
+ len = skb_headlen(skb);
/* Queue skb data, a.k.a. the main skb fragment. */
mapping = pci_map_single(tp->pdev, skb->data, len, PCI_DMA_TODEVICE);
tp->tx_buffers[entry].skb = skb;
pci_unmap_addr_set(&tp->tx_buffers[entry], mapping, mapping);
- tg3_set_txd(tp, entry, mapping, len, base_flags,
- (skb_shinfo(skb)->nr_frags == 0) | (mss << 1));
+ tg3_set_txd(tp, entry, mapping, len, cb->base_flags,
+ (skb_shinfo(skb)->nr_frags == 0) | (cb->mss << 1));
entry = NEXT_TX(entry);
@@ -4005,28 +4053,51 @@ static int tg3_start_xmit(struct sk_buff *skb, struct net_device *dev)
pci_unmap_addr_set(&tp->tx_buffers[entry], mapping, mapping);
tg3_set_txd(tp, entry, mapping, len,
- base_flags, (i == last) | (mss << 1));
+ cb->base_flags,
+ (i == last) | (cb->mss << 1));
entry = NEXT_TX(entry);
}
}
- /* Packets are ready, update Tx producer idx local and on card. */
- tw32_tx_mbox((MAILBOX_SNDHOST_PROD_IDX_0 + TG3_64BIT_REG_LOW), entry);
-
tp->tx_prod = entry;
- if (unlikely(tg3_tx_avail(tp) <= (MAX_SKB_FRAGS + 1))) {
- netif_stop_queue(dev);
- if (tg3_tx_avail(tp) > TG3_TX_WAKEUP_THRESH(tp))
- netif_wake_queue(tp->dev);
- }
+ return NETDEV_TX_OK;
+}
+
+/* hard_start_xmit for devices that don't have any bugs and
+ * support TG3_FLG2_HW_TSO_2 only.
+ */
+static int tg3_start_xmit(struct sk_buff *skb, struct net_device *dev)
+{
+ struct tg3 *tp = netdev_priv(dev);
+ int ret = tg3_prep_frame(skb, dev);
+ /* XXX: original code did mmiowb(); on failure,
+ * I dont think thats necessary
+ */
+ if (unlikely(ret != NETDEV_TX_OK))
+ return NETDEV_TX_OK;
-out_unlock:
- mmiowb();
+ /* We are running in BH disabled context with netif_tx_lock
+ * and TX reclaim runs via tp->poll inside of a software
+ * interrupt. Furthermore, IRQ processing runs lockless so we have
+ * no IRQ context deadlocks to worry about either. Rejoice!
+ */
+ if (unlikely(tg3_tx_avail(tp) <= (skb_shinfo(skb)->nr_frags + 1))) {
+ if (!netif_queue_stopped(dev)) {
+ netif_stop_queue(dev);
- dev->trans_start = jiffies;
+ /* This is a hard error, log it. */
+ printk(KERN_ERR PFX "%s: BUG! Tx Ring full when "
+ "queue awake!\n", dev->name);
+ }
+ return NETDEV_TX_BUSY;
+ }
- return NETDEV_TX_OK;
+ ret = tg3_enqueue(skb, dev);
+ if (ret == NETDEV_TX_OK)
+ tg3_kick_DMA(dev);
+
+ return ret;
}
static int tg3_start_xmit_dma_bug(struct sk_buff *, struct net_device *);
@@ -4067,46 +4138,19 @@ tg3_tso_bug_end:
/* hard_start_xmit for devices that have the 4G bug and/or 40-bit bug and
* support TG3_FLG2_HW_TSO_1 or firmware TSO only.
*/
-static int tg3_start_xmit_dma_bug(struct sk_buff *skb, struct net_device *dev)
+static int tg3_enqueue_buggy(struct sk_buff *skb, struct net_device *dev)
{
struct tg3 *tp = netdev_priv(dev);
dma_addr_t mapping;
- u32 len, entry, base_flags, mss;
+ u32 len, entry;
int would_hit_hwbug;
+ struct tg3_tx_cbdata *cb = TG3_SKB_CB(skb);
- len = skb_headlen(skb);
- /* We are running in BH disabled context with netif_tx_lock
- * and TX reclaim runs via tp->napi.poll inside of a software
- * interrupt. Furthermore, IRQ processing runs lockless so we have
- * no IRQ context deadlocks to worry about either. Rejoice!
- */
- if (unlikely(tg3_tx_avail(tp) <= (skb_shinfo(skb)->nr_frags + 1))) {
- if (!netif_queue_stopped(dev)) {
- netif_stop_queue(dev);
-
- /* This is a hard error, log it. */
- printk(KERN_ERR PFX "%s: BUG! Tx Ring full when "
- "queue awake!\n", dev->name);
- }
- return NETDEV_TX_BUSY;
- }
-
- entry = tp->tx_prod;
- base_flags = 0;
- if (skb->ip_summed == CHECKSUM_PARTIAL)
- base_flags |= TXD_FLAG_TCPUDP_CSUM;
- mss = 0;
- if ((mss = skb_shinfo(skb)->gso_size) != 0) {
+ if (cb->mss != 0) {
struct iphdr *iph;
int tcp_opt_len, ip_tcp_len, hdr_len;
- if (skb_header_cloned(skb) &&
- pskb_expand_head(skb, 0, 0, GFP_ATOMIC)) {
- dev_kfree_skb(skb);
- goto out_unlock;
- }
-
tcp_opt_len = tcp_optlen(skb);
ip_tcp_len = ip_hdrlen(skb) + sizeof(struct tcphdr);
@@ -4115,15 +4159,13 @@ static int tg3_start_xmit_dma_bug(struct sk_buff *skb, struct net_device *dev)
(tp->tg3_flags2 & TG3_FLG2_TSO_BUG))
return (tg3_tso_bug(tp, skb));
- base_flags |= (TXD_FLAG_CPU_PRE_DMA |
- TXD_FLAG_CPU_POST_DMA);
iph = ip_hdr(skb);
iph->check = 0;
- iph->tot_len = htons(mss + hdr_len);
+ iph->tot_len = htons(cb->mss + hdr_len);
if (tp->tg3_flags2 & TG3_FLG2_HW_TSO) {
tcp_hdr(skb)->check = 0;
- base_flags &= ~TXD_FLAG_TCPUDP_CSUM;
+ cb->base_flags &= ~TXD_FLAG_TCPUDP_CSUM;
} else
tcp_hdr(skb)->check = ~csum_tcpudp_magic(iph->saddr,
iph->daddr, 0,
@@ -4136,22 +4178,19 @@ static int tg3_start_xmit_dma_bug(struct sk_buff *skb, struct net_device *dev)
int tsflags;
tsflags = (iph->ihl - 5) + (tcp_opt_len >> 2);
- mss |= (tsflags << 11);
+ cb->mss |= (tsflags << 11);
}
} else {
if (tcp_opt_len || iph->ihl > 5) {
int tsflags;
tsflags = (iph->ihl - 5) + (tcp_opt_len >> 2);
- base_flags |= tsflags << 12;
+ cb->base_flags |= tsflags << 12;
}
}
}
-#if TG3_VLAN_TAG_USED
- if (tp->vlgrp != NULL && vlan_tx_tag_present(skb))
- base_flags |= (TXD_FLAG_VLAN |
- (vlan_tx_tag_get(skb) << 16));
-#endif
+ len = skb_headlen(skb);
+ entry = tp->tx_prod;
/* Queue skb data, a.k.a. the main skb fragment. */
mapping = pci_map_single(tp->pdev, skb->data, len, PCI_DMA_TODEVICE);
@@ -4164,8 +4203,8 @@ static int tg3_start_xmit_dma_bug(struct sk_buff *skb, struct net_device *dev)
if (tg3_4g_overflow_test(mapping, len))
would_hit_hwbug = 1;
- tg3_set_txd(tp, entry, mapping, len, base_flags,
- (skb_shinfo(skb)->nr_frags == 0) | (mss << 1));
+ tg3_set_txd(tp, entry, mapping, len, cb->base_flags,
+ (skb_shinfo(skb)->nr_frags == 0) | (cb->mss << 1));
entry = NEXT_TX(entry);
@@ -4194,10 +4233,11 @@ static int tg3_start_xmit_dma_bug(struct sk_buff *skb, struct net_device *dev)
if (tp->tg3_flags2 & TG3_FLG2_HW_TSO)
tg3_set_txd(tp, entry, mapping, len,
- base_flags, (i == last)|(mss << 1));
+ cb->base_flags,
+ (i == last)|(cb->mss << 1));
else
tg3_set_txd(tp, entry, mapping, len,
- base_flags, (i == last));
+ cb->base_flags, (i == last));
entry = NEXT_TX(entry);
}
@@ -4214,28 +4254,48 @@ static int tg3_start_xmit_dma_bug(struct sk_buff *skb, struct net_device *dev)
* failure, silently drop this packet.
*/
if (tigon3_dma_hwbug_workaround(tp, skb, last_plus_one,
- &start, base_flags, mss))
- goto out_unlock;
+ &start, cb->base_flags,
+ cb->mss)) {
+ mmiowb();
+ return NETDEV_TX_OK;
+ }
entry = start;
}
- /* Packets are ready, update Tx producer idx local and on card. */
- tw32_tx_mbox((MAILBOX_SNDHOST_PROD_IDX_0 + TG3_64BIT_REG_LOW), entry);
-
tp->tx_prod = entry;
- if (unlikely(tg3_tx_avail(tp) <= (MAX_SKB_FRAGS + 1))) {
- netif_stop_queue(dev);
- if (tg3_tx_avail(tp) > TG3_TX_WAKEUP_THRESH(tp))
- netif_wake_queue(tp->dev);
- }
+ return NETDEV_TX_OK;
+}
+
+static int tg3_start_xmit_dma_bug(struct sk_buff *skb, struct net_device *dev)
+{
+ struct tg3 *tp = netdev_priv(dev);
+ int ret = tg3_prep_bug_frame(skb, dev);
-out_unlock:
- mmiowb();
+ if (unlikely(ret != NETDEV_TX_OK))
+ return NETDEV_TX_OK;
- dev->trans_start = jiffies;
+ /* We are running in BH disabled context with netif_tx_lock
+ * and TX reclaim runs via tp->poll inside of a software
+ * interrupt. Furthermore, IRQ processing runs lockless so we have
+ * no IRQ context deadlocks to worry about either. Rejoice!
+ */
+ if (unlikely(tg3_tx_avail(tp) <= (skb_shinfo(skb)->nr_frags + 1))) {
+ if (!netif_queue_stopped(dev)) {
+ netif_stop_queue(dev);
- return NETDEV_TX_OK;
+ /* This is a hard error, log it. */
+ printk(KERN_ERR PFX "%s: BUG! Tx Ring full when "
+ "queue awake!\n", dev->name);
+ }
+ return NETDEV_TX_BUSY;
+ }
+
+ ret = tg3_enqueue_buggy(skb, dev);
+ if (ret == NETDEV_TX_OK)
+ tg3_kick_DMA(dev);
+
+ return ret;
}
static inline void tg3_set_mtu(struct net_device *dev, struct tg3 *tp,
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox