* Re: [PATCH] dst_entry structure use,lastuse and refcnt abstraction
From: Christoph Lameter @ 2005-06-24 4:06 UTC (permalink / raw)
To: David S. Miller; +Cc: linux-kernel, linux-net, shai, akpm, netdev, herbert
In-Reply-To: <20050623.205447.66178303.davem@davemloft.net>
On Thu, 23 Jun 2005, David S. Miller wrote:
> Ok. You're going to have to come up with something better
> than a %3 AIM benchmark increase with 5000 threads to justify
> those invasive NUMA changes, and thus this infrastructure for
> it.
I am sure one can do better than that. The x445 is the smallest NUMA box
that I know of and the only one available in the context of that project.
But I am also not sure that this will reach proportions that will
outweigh the complexity added by these patches. What would be
the performance benefit that we would need to see to feel that is
is right to get such a change in?
> I'm picking those examples, because I am rather certain your patches
> will hurt performance in those cases. The data structure size
> expansion and extra memory allocations alone for the per-node dst
> stuff should be good about doing that.
Hmm. I like the idea of a separate routing cache per node. May actually
reach higher performance than splitting the counters. Lets think a bit
about that.
^ permalink raw reply
* Kconfig NET_EMATCH_TEXT
From: David S. Miller @ 2005-06-24 4:05 UTC (permalink / raw)
To: tgraf; +Cc: netdev
Shouldn't there be some dependency between NET_EMATCH_TEXT and
TEXTSEARCH so that the user gets the right thing to happen if he
enables the former?
For IPSEC we do this by making things like INET_ESP depend upon
CRYPTO, CRYPTO_HMAC, CRYPTO_MD5, etc. see net/ipv4/Kconfig
^ permalink raw reply
* Re: [PATCH] dst_entry structure use,lastuse and refcnt abstraction
From: David S. Miller @ 2005-06-24 3:54 UTC (permalink / raw)
To: christoph; +Cc: linux-kernel, linux-net, shai, akpm, netdev, herbert
In-Reply-To: <Pine.LNX.4.62.0506232047450.29103@graphe.net>
From: Christoph Lameter <christoph@lameter.com>
Date: Thu, 23 Jun 2005 20:49:17 -0700 (PDT)
> No I told you that we need to disassemble the atomic dec_and_test
> in order to be able to split the counters.
Ok. You're going to have to come up with something better
than a %3 AIM benchmark increase with 5000 threads to justify
those invasive NUMA changes, and thus this infrastructure for
it.
In order to convince me on this NUMA dst stuff, you need to put away
the benchmark microscope and instead use a "macroscrope" to analyze
the side effects of these changes on a higher level.
Really, what are the effects on other things? For example, what
does this do to routing performance? Does the packets per second
go down, if so by how much?
What happens if you bind network interfaces, and the processes that
use them, to cpus. Why would that be too intrusive to setup for the
administrator of such large NUMA machines?
What about loads that have low route locality, and thus touch many
different dst entries, not necessarily contending for a single one
amongst several nodes? Does performance go up or down for such a
case?
I'm picking those examples, because I am rather certain your patches
will hurt performance in those cases. The data structure size
expansion and extra memory allocations alone for the per-node dst
stuff should be good about doing that.
> > > Yes and it was recently changed. Typical use is linux-xxx@vger.kernel.org
> >
> > netdev@oss.sgi.com is what used to be the place for networking
> > stuff, it's not netdev@vger.kernel.org
>
> s/not/now/ right?
Right. :)
^ permalink raw reply
* Re: [PATCH] dst_entry structure use,lastuse and refcnt abstraction
From: Christoph Lameter @ 2005-06-24 3:49 UTC (permalink / raw)
To: David S. Miller; +Cc: linux-kernel, linux-net, shai, akpm, netdev, herbert
In-Reply-To: <20050623.204702.26274560.davem@davemloft.net>
On Thu, 23 Jun 2005, David S. Miller wrote:
> From: Christoph Lameter <christoph@lameter.com>
> Date: Thu, 23 Jun 2005 20:40:25 -0700 (PDT)
>
> > Nothing as far as I can tell. The main benefit may be reorganization of
> > the code.
>
> You told us way back in the original thread that the final atomic dec
> shows up very much on NUMA, and if it could be avoided (and changed
> into a read test), it would help a lot on NUMA.
No I told you that we need to disassemble the atomic dec_and_test
in order to be able to split the counters.
> > Yes and it was recently changed. Typical use is linux-xxx@vger.kernel.org
>
> netdev@oss.sgi.com is what used to be the place for networking
> stuff, it's not netdev@vger.kernel.org
s/not/now/ right?
^ permalink raw reply
* Re: [PATCH] dst_entry structure use,lastuse and refcnt abstraction
From: David S. Miller @ 2005-06-24 3:47 UTC (permalink / raw)
To: christoph; +Cc: linux-kernel, linux-net, shai, akpm, netdev, herbert
In-Reply-To: <Pine.LNX.4.62.0506232037430.28814@graphe.net>
From: Christoph Lameter <christoph@lameter.com>
Date: Thu, 23 Jun 2005 20:40:25 -0700 (PDT)
> Nothing as far as I can tell. The main benefit may be reorganization of
> the code.
You told us way back in the original thread that the final atomic dec
shows up very much on NUMA, and if it could be avoided (and changed
into a read test), it would help a lot on NUMA.
> > NETWORKING [GENERAL]
> > P: Networking Team
> > M: netdev@vger.kernel.org
> > L: netdev@vger.kernel.org
> > S: Maintained
> >
> > Thanks.
>
> Yes and it was recently changed. Typical use is linux-xxx@vger.kernel.org
netdev@oss.sgi.com is what used to be the place for networking
stuff, it's not netdev@vger.kernel.org
^ permalink raw reply
* Re: [PATCH] dst_entry structure use,lastuse and refcnt abstraction
From: Christoph Lameter @ 2005-06-24 3:40 UTC (permalink / raw)
To: David S. Miller; +Cc: linux-kernel, linux-net, shai, akpm, netdev, herbert
In-Reply-To: <20050623.203647.88475017.davem@davemloft.net>
On Thu, 23 Jun 2005, David S. Miller wrote:
> From: Christoph Lameter <christoph@lameter.com>
> Date: Thu, 23 Jun 2005 20:04:52 -0700 (PDT)
>
> > The patch also removes the atomic_dec_and_test in dst_destroy.
>
> That is the only part of this patch I'm willing to entertain
> at this time, as long as Herbert Xu ACKs it. How much of that
> quoted %3 gain does this change alone give you?
Nothing as far as I can tell. The main benefit may be reorganization of
the code.
> Also, please post networking patches to netdev@vger.kernel.org.
> Because you didn't, the majority of the networking maintainers
> did not see your dst patch submissions. It's mentioned in
> linux/MAINTAINERS for a reason:
>
> NETWORKING [GENERAL]
> P: Networking Team
> M: netdev@vger.kernel.org
> L: netdev@vger.kernel.org
> S: Maintained
>
> Thanks.
Yes and it was recently changed. Typical use is linux-xxx@vger.kernel.org
^ permalink raw reply
* Re: [PATCH] dst_entry structure use,lastuse and refcnt abstraction
From: David S. Miller @ 2005-06-24 3:36 UTC (permalink / raw)
To: christoph; +Cc: linux-kernel, linux-net, shai, akpm, netdev, herbert
In-Reply-To: <Pine.LNX.4.62.0506231953260.28244@graphe.net>
From: Christoph Lameter <christoph@lameter.com>
Date: Thu, 23 Jun 2005 20:04:52 -0700 (PDT)
> The patch also removes the atomic_dec_and_test in dst_destroy.
That is the only part of this patch I'm willing to entertain
at this time, as long as Herbert Xu ACKs it. How much of that
quoted %3 gain does this change alone give you?
Also, please post networking patches to netdev@vger.kernel.org.
Because you didn't, the majority of the networking maintainers
did not see your dst patch submissions. It's mentioned in
linux/MAINTAINERS for a reason:
NETWORKING [GENERAL]
P: Networking Team
M: netdev@vger.kernel.org
L: netdev@vger.kernel.org
S: Maintained
Thanks.
^ permalink raw reply
* Re: [git patch] urgent e1000 fix
From: Linus Torvalds @ 2005-06-23 23:33 UTC (permalink / raw)
To: Jeff Garzik; +Cc: David Lang, Andrew Morton, Linux Kernel, Netdev List
In-Reply-To: <42BB4172.5000904@pobox.com>
On Thu, 23 Jun 2005, Jeff Garzik wrote:
>
> patch(1) is a huge collection of heuristics like this, even without
> '-l', so I'm not surprised that it worked.
>
> Does git-apply support patches with "fuzz", out of curiosity?
No, git is strictly "zero fuzz". So was my "dotest" script even before
switching it to "git-apply", btw (ie I explicitly had a "--fuzz=0" there
when using GNU patch).
Now, I may have to reconsider the strictness if it causes lots of
problems, and it's not like I hate fuzz (or ignoring whitespace) with a
passion. I'd just rather try to be as strict as possible at least
initially.
Your special case of a corrupted patch where a single space turned into an
empty line is actually one case I considered allowing, just because the
fuzz there wouldn't be in the data itself, only in the first character
that just tells what kind of line it is (new, deleted or unmodified), and
just saying "empty means unmodified" doesn't really introduce any
possibility of ambiguity.
So let's see what happens. Tell me if you see more of this particular kind
of corruption, and I'll just make "git-apply" work around it. I don't
think _one_ patch is much of an indication in any direction, but two or
three might convince me to just relax the checks a bit.
To actually allow real fuzz or to allow real whitespace differences in the
patch data itself is a _much_ bigger issue than this trivial patch
corruption, and I'd prefer to avoid going there if at all possible.
Linus
^ permalink raw reply
* Re: [git patch] urgent e1000 fix
From: David Lang @ 2005-06-23 23:20 UTC (permalink / raw)
To: Jeff Garzik; +Cc: Andrew Morton, Linus Torvalds, Linux Kernel, Netdev List
In-Reply-To: <42BB2749.1020209@pobox.com>
This does not solve the problem that I reported last thursday where the
fourth port of a e1000 quad card doesn't work under SMP (I don't know if
it was supposed to, but since it was a locking fix I had hopes).
here's the symptom I am seeing
happy1-p:~# ifconfig eth11 192.168.255.1
SIOCSIFFLAGS: Invalid argument
happy1-p:~# netstat -nr
Kernel IP routing table
Destination Gateway Genmask Flags MSS Window irtt Iface
172.20.252.0 0.0.0.0 255.255.252.0 U 0 0 0 eth1
10.201.0.0 0.0.0.0 255.255.0.0 U 0 0 0 eth0
0.0.0.0 10.201.0.2 0.0.0.0 UG 0 0 0 eth0
happy1-p:~# ifconfig eth11
eth11 Link encap:Ethernet HWaddr 00:04:23:B4:BB:97
inet addr:192.168.255.1 Bcast:192.168.255.255 Mask:255.255.255.0
BROADCAST MULTICAST MTU:1500 Metric:1
RX packets:0 errors:0 dropped:0 overruns:0 frame:0
TX packets:0 errors:0 dropped:0 overruns:0 carrier:0
collisions:0 txqueuelen:1000
RX bytes:0 (0.0 b) TX bytes:0 (0.0 b)
Base address:0x60c0 Memory:fe460000-fe480000
userspace is debian woody
David Lang
On Thu, 23 Jun 2005, Jeff Garzik wrote:
> David Lang wrote:
>> hmm, I know I'm not that experianced with patch, but when I saved this to a
>> file and did patch -p1 <file the hunk was rejected, the reject file is
>> saying
>
> It's probably the whitespace thing that Linus's git-apply gadget was
> complaining about.
>
> I'm terribly surprising, though, since my patch(1) applied the diff just
> fine.
>
> <shrug>
>
> Jeff
>
>
>
--
There are two ways of constructing a software design. One way is to make it so simple that there are obviously no deficiencies. And the other way is to make it so complicated that there are no obvious deficiencies.
-- C.A.R. Hoare
^ permalink raw reply
* Re: [git patch] urgent e1000 fix
From: Jeff Garzik @ 2005-06-23 23:10 UTC (permalink / raw)
To: Linus Torvalds; +Cc: David Lang, Andrew Morton, Linux Kernel, Netdev List
In-Reply-To: <Pine.LNX.4.58.0506231506510.11175@ppc970.osdl.org>
Linus Torvalds wrote:
>
> On Thu, 23 Jun 2005, Jeff Garzik wrote:
>
>>It's probably the whitespace thing that Linus's git-apply gadget was
>>complaining about.
>>
>>I'm terribly surprising, though, since my patch(1) applied the diff just
>>fine.
>
>
> I could easily make git-apply accept empty lines as if they had a single
> space on it. What I find surprising is that "patch" allows that kind of
> whitespace corruption by default, and silently. Usually you have to give
> it the "-l" flag to make it ignore whitespace differences..
patch(1) is a huge collection of heuristics like this, even without
'-l', so I'm not surprised that it worked.
Does git-apply support patches with "fuzz", out of curiosity?
Jeff
^ permalink raw reply
* Re: [git patch] urgent e1000 fix
From: Linus Torvalds @ 2005-06-23 22:08 UTC (permalink / raw)
To: Jeff Garzik; +Cc: David Lang, Andrew Morton, Linux Kernel, Netdev List
In-Reply-To: <42BB2749.1020209@pobox.com>
On Thu, 23 Jun 2005, Jeff Garzik wrote:
>
> It's probably the whitespace thing that Linus's git-apply gadget was
> complaining about.
>
> I'm terribly surprising, though, since my patch(1) applied the diff just
> fine.
I could easily make git-apply accept empty lines as if they had a single
space on it. What I find surprising is that "patch" allows that kind of
whitespace corruption by default, and silently. Usually you have to give
it the "-l" flag to make it ignore whitespace differences..
Linus
^ permalink raw reply
* Re: [git patch] urgent e1000 fix
From: Jeff Garzik @ 2005-06-23 21:19 UTC (permalink / raw)
To: David Lang; +Cc: Andrew Morton, Linus Torvalds, Linux Kernel, Netdev List
In-Reply-To: <Pine.LNX.4.62.0506231402340.18154@qynat.qvtvafvgr.pbz>
David Lang wrote:
> hmm, I know I'm not that experianced with patch, but when I saved this
> to a file and did patch -p1 <file the hunk was rejected, the reject file
> is saying
It's probably the whitespace thing that Linus's git-apply gadget was
complaining about.
I'm terribly surprising, though, since my patch(1) applied the diff just
fine.
<shrug>
Jeff
^ permalink raw reply
* Re: [git patch] urgent e1000 fix
From: David Lang @ 2005-06-23 21:04 UTC (permalink / raw)
To: Jeff Garzik; +Cc: Andrew Morton, Linus Torvalds, Linux Kernel, Netdev List
In-Reply-To: <42BA7FB5.5020804@pobox.com>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 1528 bytes --]
hmm, I know I'm not that experianced with patch, but when I saved this to
a file and did patch -p1 <file the hunk was rejected, the reject file is
saying
***************
*** 2307,2312 ****
tso = e1000_tso(adapter, skb);
if (tso < 0) {
dev_kfree_skb_any(skb);
return NETDEV_TX_OK;
}
--- 2307,2313 ----
tso = e1000_tso(adapter, skb);
if (tso < 0) {
dev_kfree_skb_any(skb);
+ spin_unlock_irqrestore(&adapter->tx_lock, flags);
return NETDEV_TX_OK;
}
I manually put the line in and am compiling it now to test it, but is
someone could take a few seconds and teach me what I did wrong it would be
appriciated.
David Lang
On Thu, 23 Jun 2005, Jeff Garzik wrote:
> Date: Thu, 23 Jun 2005 05:24:05 -0400
> From: Jeff Garzik <jgarzik@pobox.com>
> To: Andrew Morton <akpm@osdl.org>, Linus Torvalds <torvalds@osdl.org>
> Cc: Linux Kernel <linux-kernel@vger.kernel.org>,
> Netdev List <netdev@vger.kernel.org>
> Subject: [git patch] urgent e1000 fix
>
> Please pull from 'misc-fixes' branch of
> rsync://rsync.kernel.org/pub/scm/linux/kernel/git/jgarzik/netdev-2.6.git
>
> to obtain the spinlock fix described in the attached text.
>
>
>
--
There are two ways of constructing a software design. One way is to make it so simple that there are obviously no deficiencies. And the other way is to make it so complicated that there are no obvious deficiencies.
-- C.A.R. Hoare
[-- Attachment #2: Type: TEXT/PLAIN, Size: 495 bytes --]
drivers/net/e1000/e1000_main.c | 1 +
1 files changed, 1 insertion(+)
Mitch Williams:
e1000: fix spinlock bug
diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
--- a/drivers/net/e1000/e1000_main.c
+++ b/drivers/net/e1000/e1000_main.c
@@ -2307,6 +2307,7 @@ e1000_xmit_frame(struct sk_buff *skb, st
tso = e1000_tso(adapter, skb);
if (tso < 0) {
dev_kfree_skb_any(skb);
+ spin_unlock_irqrestore(&adapter->tx_lock, flags);
return NETDEV_TX_OK;
}
^ permalink raw reply
* status of TSO in 2.6.12
From: Andre Tomt @ 2005-06-23 20:31 UTC (permalink / raw)
To: netdev
What is the status of TSO in 2.6.12? In 2.6.11 I used to disable it by
default on production kernels as not beeing ready for prime time, still
a wise thing to do?
The TSO discussions kind-of overflowed for me eventually :-)
--
Cheers,
André Tomt
^ permalink raw reply
* Re: [patch] devinet: cleanup if statements
From: Prasanna Meda @ 2005-06-23 18:13 UTC (permalink / raw)
To: Herbert Xu; +Cc: David S. Miller, jgarzik, akpm, netdev
In-Reply-To: <E1DlQ1w-0003yI-00@gondolin.me.apana.org.au>
Herbert Xu wrote:
>
>
> With your interpretation above his patch is correct as well. If
> we want to use the label when it is different from the device name,
> then it is equivalent to always use the label since the only time
> we'd use the device name is when it's equal to the label :)
Sounds correct to me. Actually I did not think about second
interpretation in first.
Thanks,
Prasanna.
^ permalink raw reply
* Re: RFC: NAPI packet weighting patch
From: David Mosberger @ 2005-06-23 17:36 UTC (permalink / raw)
To: hadi
Cc: David S. Miller, Lennert Buytenhek, davidm, netdev, dada1, ak,
leonid.grossman, becker, rick.jones2, davem
In-Reply-To: <1119528852.11975.65.camel@localhost.localdomain>
>>>>> On Thu, 23 Jun 2005 08:14:11 -0400, jamal <hadi@cyberus.ca> said:
Jamal> For the fans of the e1000 (or even the tg3 deprived people),
Jamal> heres a patch which originated from David Mosberger that i
Jamal> played around (about 9 months back) - it will need some hand
Jamal> patching for the latest driver. Similar approach: prefetch
Jamal> skb->data,twiddle twiddle not little star, touch header.
Jamal> I found the aggressive mode effective on a xeon but i belive
Jamal> David is using this on x86_64. So Lennert, I lied to you
Jamal> saying it was never effective on x86. You just have to do the
Jamal> right juju such as factoring in the memory load-latency and
Jamal> how much cache you have on your specific CPU. CCing davidm
Jamal> (in addition To: davem of course ;->) so he may provide more
Jamal> insight on his tests.
I didn't remember what experiments I did, but I found the original
mail, with all the data. The experiments were done on ia64 (naturally
;-).
Enjoy,
--david
---
From: David Mosberger <davidm@linux.hpl.hp.com>
To: hadi@cyberus.ca
Cc: Alexey <kuznet@ms2.inr.ac.ru>, "David S. Miller" <davem@davemloft.net>,
Robert Olsson <Robert.Olsson@data.slu.se>,
Lennert Buytenhek <buytenh@wantstofly.org>, davidm@hpl.hp.com,
eranian@linux.hpl.hp.com, grundler@parisc-linux.org
Subject: Re: prefetch
Date: Thu, 30 Sep 2004 06:51:29 -0700
Reply-To: davidm@hpl.hp.com
X-URL: http://www.hpl.hp.com/personal/David_Mosberger/
>>>>> On 27 Sep 2004 11:08:00 -0400, jamal <hadi@cyberus.ca> said:
Jamal> one of the top abusers of cpu cycles in the netcode is
Jamal> eth_type_trans() on x86 type hardware. This is where the
Jamal> first time the skb->data is touched (hence a cache miss).
Jamal> Clearly a good place to prefecth is in eth_type_trans itself
Jamal> maybe right at the top you could prefetch skb->data or after
Jamal> skb_pull() you could prefetch skb->mac.ethernet.
Jamal> oprofile shows me the cycles being abused
Jamal> (GLOBAL_POWER_EVENTS on xeon box) went down when i do either;
Jamal> i cut down more cycles on doing skb->mac.ethernet that
Jamal> skb->data - but thats a different topic.
Jamal> My test is purely forwarding: packets come in through eth0,
Jamal> get exercised by routing code and come out eth1. So the
Jamal> important parameters for my test case are primarly throughput
Jamal> and secondary is latency. Adding the prefetch above while
Jamal> showing lower CPU cycles, results in decreeased throughput
Jamal> numbers and higher latency numbers. What gives?
Jamal> I am CCing the HP folks since they have some interesting
Jamal> tools i heard David talk about at SUCON.
I don't have a good setup to measure packet forwarding performance.
However, prefetching skb->data certainly does reduce CPU utilization
on ia64 as the measurements below show.
I tried three versions:
- original 2.6.9-rc3 (ORIGINAL)
- 2.6.9-rc3 with a prefetch in e1000_clean_rx_irq (OBVIOUS)
- 2.6.9-rc3 which prefetches the _next_ rx buffer (AGGRESSIVE)
All 3 cases use an e1000 board with NAPI enabled.
netperf results for 3 runs of ORIGINAL and AGGRESSIVE:
ORIGINAL:
$ netperf -l30 -c -C -H 192.168.10.15 -- -m1 -D
TCP STREAM TEST to 192.168.10.15 : nodelay
Recv Send Send Utilization Service Demand
Socket Socket Message Elapsed Send Recv Send Recv
Size Size Size Time Throughput local remote local remote
bytes bytes bytes secs. 10^6bits/s % S % S us/KB us/KB
87380 16384 1 30.00 1.59 99.93 10.94 5155.593 2257.461
87380 16384 1 30.00 1.62 99.87 11.19 5045.549 2260.294
87380 16384 1 30.00 1.62 99.89 11.29 5045.269 2281.327
AGGRESSIVE:
$ netperf -l30 -c -C -H 192.168.10.15 -- -m1 -D
TCP STREAM TEST to 192.168.10.15 : nodelay
Recv Send Send Utilization Service Demand
Socket Socket Message Elapsed Send Recv Send Recv
Size Size Size Time Throughput local remote local remote
bytes bytes bytes secs. 10^6bits/s % S % S us/KB us/KB
87380 16384 1 30.00 1.62 99.98 10.51 5062.204 2128.695
87380 16384 1 30.00 1.62 99.99 10.51 5064.528 2128.940
87380 16384 1 30.00 1.62 99.98 10.67 5053.365 2156.333
As you can see, not much of a throughput difference (I'd not expect
that, given the test...), but service demand on the receiver is down
significantly. This is also confirmed with the following three
profiles (collected with q-syscollect):
ORIGINAL:
% time self cumul calls self/call tot/call name
53.73 32.05 32.05 471k 68.1u 68.1u default_idle
4.59 2.74 34.79 12.0M 228n 259n eth_type_trans
OBVIOUS:
% time self cumul calls self/call tot/call name
55.72 33.25 33.25 469k 70.8u 70.8u default_idle
4.49 2.68 35.93 12.0M 222n 278n tcp_v4_rcv
2.84 1.70 37.63 473k 3.59u 32.6u e1000_clean
2.81 1.68 39.30 12.2M 137n 525n tcp_rcv_established
2.71 1.62 40.92 12.1M 134n 711n netif_receive_skb
2.39 1.43 42.34 12.0M 119n 148n eth_type_trans
AGGRESSIVE:
% time self cumul calls self/call tot/call name
57.51 34.34 34.34 395k 86.9u 86.9u default_idle
4.40 2.62 36.96 12.3M 214n 265n tcp_v4_rcv
3.12 1.86 38.82 455k 4.09u 31.3u e1000_clean
3.09 1.84 40.66 12.0M 154n 584n tcp_rcv_established
2.89 1.72 42.39 12.0M 144n 723n netif_receive_skb
1.94 1.16 43.55 918k 1.26u 1.26u _spin_unlock_irq
1.90 1.13 44.68 12.3M 92.4n 115n ip_route_input
1.87 1.11 45.79 12.6M 88.4n 89.6n kfree
1.87 1.11 46.91 12.1M 91.8n 572n ip_rcv
1.68 1.00 47.91 12.1M 82.4n 351n ip_local_deliver
1.21 0.72 48.63 12.6M 57.7n 58.9n __kmalloc
1.01 0.60 49.23 12.3M 48.8n 53.7n sba_unmap_single
1.00 0.59 49.83 12.0M 49.4n 81.0n eth_type_trans
Comparing ORIGINAL and AGGRESSIVE, we see that the latter spends an
additional 2.29 seconds in the idle-loop (default_idle), which
corresponds closely to the 2.19 seconds savings we're seeing in
eth_type_trans(), so the saving the prefetch achieves is real and not
offset by extra costs in other places.
The above also shows that the OBVIOUS prefetch is unable to cover the
entire load-latency. Thus, I suspect it would really be best to use
the AGGRESSIVE prefetching policy. If we were to do this, then the
code at label next_desc could be simplified, since we already
precomputed the next value of i/rx_desc as part of the prefetch.
It would be interesting to know how (modern) x86 CPUs behave. If
somebody wants to try this, I attached a patch below (setting
AGGRESSIVE to 1 gives you the AGGRESSIVE version, seting it to 0 gives
you the OBVIOUS version).
Cheers,
--david
===== drivers/net/e1000/e1000_main.c 1.134 vs edited =====
--- 1.134/drivers/net/e1000/e1000_main.c 2004-09-12 16:52:48 -07:00
+++ edited/drivers/net/e1000/e1000_main.c 2004-09-30 06:05:11 -07:00
@@ -2278,12 +2278,30 @@
uint8_t last_byte;
unsigned int i;
boolean_t cleaned = FALSE;
+#define AGGRESSIVE 1
i = rx_ring->next_to_clean;
+#if AGGRESSIVE
+ prefetch(rx_ring->buffer_info[i].skb->data);
+#endif
rx_desc = E1000_RX_DESC(*rx_ring, i);
while(rx_desc->status & E1000_RXD_STAT_DD) {
buffer_info = &rx_ring->buffer_info[i];
+# if AGGRESSIVE
+ {
+ struct e1000_rx_desc *next_rx;
+ unsigned int j = i + 1;
+
+ if (j == rx_ring->count)
+ j = 0;
+ next_rx = E1000_RX_DESC(*rx_ring, j);
+ if (next_rx->status & E1000_RXD_STAT_DD)
+ prefetch(rx_ring->buffer_info[j].skb->data);
+ }
+# else
+ prefetch(buffer_info->skb->data);
+# endif
#ifdef CONFIG_E1000_NAPI
if(*work_done >= work_to_do)
break;
^ permalink raw reply
* [RFC/PATCH] "safer ipv4 reassembly" (fwd)
From: Arthur Kepner @ 2005-06-23 16:33 UTC (permalink / raw)
To: netdev; +Cc: Rick Jones, Herbert Xu
What with the recent migration to vger.kernel.org, I'm
forwarding this to oss.sgi.com, just in case any interested
parties missed it.
---------- Forwarded message ----------
Date: Wed, 22 Jun 2005 16:00:55 -0700 (PDT)
From: Arthur Kepner <akepner@sgi.com>
To: netdev@vger.kernel.org
Subject: [RFC/PATCH] "safer ipv4 reassembly"
A little more than a month ago I sent a RFC/PATCH for
something I called "strict ipv4 reassembly". This was
an attempt to make it much less likely that IP fragments
from different IP datagrams were reassembled together
when the IP id wraps. That patch was considered
unacceptable because it required fragments to arrive in
order or they'd be dropped.
One idea that resulted from that thread was to keep a
count of IP datagrams for a (src,dst,proto) and use that
as a kind of sequence number to check that a fragment is
valid. (I believe that Rick Jones and Herbert Xu each
independently came up with this idea, or something very
close to it.)
Following is a patch which implements that idea.
A new sysctl "sysctl_ip_reassembly_count" is used to control
how much reordering of IP fragments we'll tolerate. If it's
zero, the patch is a no-op. If sysctl_ip_reassembly_count is
non-zero, it defines a "window size" for IP fragments. When
a new fragment queue is made, the "bottom" of the window is
defined by the number if IP packets which have been received
for the associated (src,dst,proto), and each time a fragment
is added to the queue, the bottom of the window is advanced.
But before adding a fragment to the queue, a check is made
that the number of IP fragments in the queue falls within the
window. If not, the queue is dropped.
Comments?
include/linux/sysctl.h | 1
include/net/ip.h | 1
net/ipv4/ip_fragment.c | 206 +++++++++++++++++++++++++++++++++++++++++++++
net/ipv4/ip_input.c | 24 ++++-
net/ipv4/sysctl_net_ipv4.c | 11 ++
5 files changed, 240 insertions(+), 3 deletions(-)
Signed-off-by: Arthur Kepner <akepner@sgi.com>
diff -rup linux.orig/include/linux/sysctl.h linux.new/include/linux/sysctl.h
--- linux.orig/include/linux/sysctl.h 2005-06-14 11:35:18.611069887 -0700
+++ linux.new/include/linux/sysctl.h 2005-06-22 14:04:17.384853993 -0700
@@ -347,6 +347,7 @@ enum
NET_TCP_MODERATE_RCVBUF=106,
NET_TCP_TSO_WIN_DIVISOR=107,
NET_TCP_BIC_BETA=108,
+ NET_IPV4_REASM_COUNT=109,
};
enum {
diff -rup linux.orig/include/net/ip.h linux.new/include/net/ip.h
--- linux.orig/include/net/ip.h 2005-06-14 11:52:09.878700520 -0700
+++ linux.new/include/net/ip.h 2005-06-22 14:04:33.508057469 -0700
@@ -300,6 +300,7 @@ enum ip_defrag_users
};
struct sk_buff *ip_defrag(struct sk_buff *skb, u32 user);
+void ip_count(u32 saddr, u32 daddr, u8 protocol);
extern int ip_frag_nqueues;
extern atomic_t ip_frag_mem;
diff -rup linux.orig/net/ipv4/ip_fragment.c linux.new/net/ipv4/ip_fragment.c
--- linux.orig/net/ipv4/ip_fragment.c 2005-06-13 16:49:55.290992303 -0700
+++ linux.new/net/ipv4/ip_fragment.c 2005-06-22 14:17:54.136940893 -0700
@@ -56,6 +56,8 @@
int sysctl_ipfrag_high_thresh = 256*1024;
int sysctl_ipfrag_low_thresh = 192*1024;
+extern int sysctl_ip_reassembly_count;
+
/* Important NOTE! Fragment queue must be destroyed before MSL expires.
* RFC791 is wrong proposing to prolongate timer each fragment arrival by TTL.
*/
@@ -69,6 +71,25 @@ struct ipfrag_skb_cb
#define FRAG_CB(skb) ((struct ipfrag_skb_cb*)((skb)->cb))
+/* struct ipc contains a count of the number of IP datagrams
+ * received for a (saddr, daddr, protocol) tuple - but one of
+ * these structures exists for a given (saddr, daddr, protocol)
+ * if and only if there is a queue of IP fragments associated
+ * with that 3-tuple and sysctl_ip_reassembly_count is non-zero.
+ */
+struct ipc {
+ struct hlist_node node;
+ u32 saddr;
+ u32 daddr;
+ u8 protocol;
+ atomic_t refcnt; /* how many ipqs hold refs to us */
+ atomic_t seq; /* how many ip datagrams for this
+ * (saddr,daddr,protocol) since we
+ * were created */
+ struct timer_list timer;
+ struct rcu_head rcu;
+};
+
/* Describe an entry in the "incomplete datagrams" queue. */
struct ipq {
struct ipq *next; /* linked list pointers */
@@ -92,6 +113,14 @@ struct ipq {
struct ipq **pprev;
int iif;
struct timeval stamp;
+ struct ipc *ipc;
+ atomic_t seq;
+ /* ipq->seq defines the "bottom" of the window of sequence numbers
+ * that are valid for this fragment - the "top" of the window is
+ * (ipq->seq + sysctl_ip_reassembly_count). ipq->seq is initialized
+ * to the value in the associated ipc when the fragment queue is
+ * created, and incremented each time a fragment is added to the
+ * queue */
};
/* Hash table. */
@@ -105,6 +134,12 @@ static u32 ipfrag_hash_rnd;
static LIST_HEAD(ipq_lru_list);
int ip_frag_nqueues = 0;
+#define IPC_HASHSZ IPQ_HASHSZ
+static struct {
+ struct hlist_head head;
+ spinlock_t lock;
+} ipc_hash[IPC_HASHSZ];
+
static __inline__ void __ipq_unlink(struct ipq *qp)
{
if(qp->next)
@@ -121,6 +156,11 @@ static __inline__ void ipq_unlink(struct
write_unlock(&ipfrag_lock);
}
+static unsigned int ipchashfn(u32 saddr, u32 daddr, u8 prot)
+{
+ return jhash_3words(prot, saddr, daddr, 0) & (IPC_HASHSZ - 1);
+}
+
static unsigned int ipqhashfn(u16 id, u32 saddr, u32 daddr, u8 prot)
{
return jhash_3words((u32)id << 16 | prot, saddr, daddr,
@@ -231,8 +271,16 @@ static __inline__ void ipq_put(struct ip
*/
static void ipq_kill(struct ipq *ipq)
{
+ struct ipc *cp = ipq->ipc;
+
if (del_timer(&ipq->timer))
atomic_dec(&ipq->refcnt);
+ if (cp) {
+ atomic_dec(&cp->refcnt);
+ /* no particular reason to use sysctl_ipfrag_time
+ * for this timer */
+ mod_timer(&cp->timer, jiffies + sysctl_ipfrag_time);
+ }
if (!(ipq->last_in & COMPLETE)) {
ipq_unlink(ipq);
@@ -348,10 +396,109 @@ static struct ipq *ip_frag_intern(unsign
return qp;
}
+static inline void __ipc_destroy(struct rcu_head *head)
+{
+ kfree(container_of(head, struct ipc, rcu));
+}
+
+static void ipc_destroy(unsigned long arg)
+{
+ struct ipc *cp = (struct ipc *) arg;
+ unsigned int hash = ipchashfn(cp->saddr, cp->daddr, cp->protocol);
+
+ spin_lock(&ipc_hash[hash].lock);
+ BUG_ON((atomic_read(&cp->refcnt)) < 0);
+ if (atomic_read(&cp->refcnt) == 0) {
+ hlist_del_rcu(&cp->node);
+ call_rcu(&cp->rcu, __ipc_destroy);
+ }
+ spin_unlock(&ipc_hash[hash].lock);
+}
+
+/*
+ * must hold spinlock for the appropriate hash list head when
+ * __ipc_create is called
+ */
+
+static inline struct ipc *__ipc_create(struct iphdr *iph,
+ const unsigned int hash)
+{
+ struct ipc *cp = kmalloc(sizeof(struct ipc), GFP_ATOMIC);
+ /* XXX should we account size to ip_frag_mem ??? */
+ if (cp) {
+ cp->saddr = iph->saddr;
+ cp->daddr = iph->daddr;
+ cp->protocol = iph->protocol;
+ atomic_set(&cp->seq, 0);
+ atomic_set(&cp->refcnt, 1);
+ INIT_HLIST_NODE(&cp->node);
+ hlist_add_head_rcu(&cp->node, &ipc_hash[hash].head);
+ init_timer(&cp->timer);
+ cp->timer.data = (unsigned long) cp;
+ cp->timer.function = ipc_destroy;
+ } else {
+ NETDEBUG(if (net_ratelimit())
+ printk(KERN_ERR "__ipc_create: no memory left !\n"));
+ }
+ return cp;
+}
+
+/*
+ * must be "rcu safe" when __ipc_find is called - either use
+ * rcu_read_lock (if you intend only to read the returned struct)
+ * or grab the spinlock for the appropriate hash list head (if
+ * you might modify the returned struct)
+ */
+static inline struct ipc *__ipc_find(u32 saddr, u32 daddr, u8 protocol,
+ const unsigned int hash)
+{
+ struct hlist_node *p;
+
+ hlist_for_each_rcu(p, &ipc_hash[hash].head) {
+ struct ipc * cp = (struct ipc *)p;
+ if(cp->saddr == saddr &&
+ cp->daddr == daddr &&
+ cp->protocol == protocol) {
+ return cp;
+ }
+ }
+ return NULL;
+}
+
+static struct ipc *ipc_find(struct iphdr *iph)
+{
+ struct ipc *cp;
+ unsigned int hash = ipchashfn(iph->saddr, iph->daddr, iph->protocol);
+
+ rcu_read_lock();
+ if((cp = __ipc_find(iph->saddr, iph->daddr,
+ iph->protocol, hash)) != NULL) {
+ atomic_inc(&cp->refcnt);
+ rcu_read_unlock();
+ return cp;
+ }
+ rcu_read_unlock();
+ spin_lock(&ipc_hash[hash].lock);
+ if((cp = __ipc_find(iph->saddr, iph->daddr,
+ iph->protocol, hash)) != NULL) {
+ atomic_inc(&cp->refcnt);
+ spin_unlock(&ipc_hash[hash].lock);
+ return cp;
+ }
+ cp = __ipc_create(iph, hash);
+ spin_unlock(&ipc_hash[hash].lock);
+ return cp;
+}
+
+
/* Add an entry to the 'ipq' queue for a newly received IP datagram. */
static struct ipq *ip_frag_create(unsigned hash, struct iphdr *iph, u32 user)
{
struct ipq *qp;
+ struct ipc *cp = NULL;
+
+ if (sysctl_ip_reassembly_count && (cp = ipc_find(iph)) == NULL)
+ return NULL;
if ((qp = frag_alloc_queue()) == NULL)
goto out_nomem;
@@ -366,6 +513,10 @@ static struct ipq *ip_frag_create(unsign
qp->meat = 0;
qp->fragments = NULL;
qp->iif = 0;
+ qp->ipc = cp;
+ if (sysctl_ip_reassembly_count && cp) {
+ atomic_set(&qp->seq, atomic_read(&cp->seq));
+ }
/* Initialize a timer for this entry. */
init_timer(&qp->timer);
@@ -381,6 +532,51 @@ out_nomem:
return NULL;
}
+void ip_count(u32 saddr, u32 daddr, u8 protocol)
+{
+ struct ipc *cp = NULL;
+ unsigned int hash = ipchashfn(saddr, daddr, protocol);
+
+ rcu_read_lock();
+ if((cp = __ipc_find(saddr, daddr, protocol, hash)) != NULL) {
+ atomic_inc(&cp->seq);
+ }
+ rcu_read_unlock();
+}
+
+static inline int in_window(int bottom, int size, int seq) {
+ return (((seq - bottom) >= 0) && ((seq - (bottom + size)) < 0));
+}
+
+static int __ip_reassembly_count_check(const struct iphdr *iph, struct ipq *qp)
+{
+ struct ipc *cp = qp->ipc;
+ int cseq, qseq;
+
+ /* qp->ipc may be NULL if sysctl_ip_reassembly_count was off
+ * at the time the fragment queue was created */
+ if (cp == NULL)
+ return 0;
+
+ cseq = atomic_read(&cp->seq);
+ qseq = atomic_inc_return(&qp->seq);
+
+ if (!in_window(qseq, sysctl_ip_reassembly_count, cseq)) {
+ atomic_inc(&qp->refcnt);
+ read_unlock(&ipfrag_lock);
+ spin_lock(&qp->lock);
+ if (!(qp->last_in&COMPLETE))
+ ipq_kill(qp);
+ spin_unlock(&qp->lock);
+ ipq_put(qp, NULL);
+ IP_INC_STATS_BH(IPSTATS_MIB_REASMFAILS);
+ read_lock(&ipfrag_lock);
+ return 1;
+ }
+ return 0;
+}
+
+
/* Find the correct entry in the "incomplete datagrams" queue for
* this IP datagram, and create new one, if nothing is found.
*/
@@ -400,6 +596,10 @@ static inline struct ipq *ip_find(struct
qp->daddr == daddr &&
qp->protocol == protocol &&
qp->user == user) {
+ if (sysctl_ip_reassembly_count &&
+ __ip_reassembly_count_check(iph, qp)) {
+ break;
+ }
atomic_inc(&qp->refcnt);
read_unlock(&ipfrag_lock);
return qp;
@@ -679,9 +879,15 @@ struct sk_buff *ip_defrag(struct sk_buff
void ipfrag_init(void)
{
+ int i;
ipfrag_hash_rnd = (u32) ((num_physpages ^ (num_physpages>>7)) ^
(jiffies ^ (jiffies >> 6)));
+ for (i = 0; i < IPC_HASHSZ; i++ ) {
+ INIT_HLIST_HEAD(&ipc_hash[i].head);
+ spin_lock_init(&ipc_hash[i].lock);
+ }
+
init_timer(&ipfrag_secret_timer);
ipfrag_secret_timer.function = ipfrag_secret_rebuild;
ipfrag_secret_timer.expires = jiffies + sysctl_ipfrag_secret_interval;
diff -rup linux.orig/net/ipv4/ip_input.c linux.new/net/ipv4/ip_input.c
--- linux.orig/net/ipv4/ip_input.c 2005-06-13 16:23:41.824620856 -0700
+++ linux.new/net/ipv4/ip_input.c 2005-06-22 14:02:35.705155734 -0700
@@ -146,6 +146,14 @@
#include <linux/mroute.h>
#include <linux/netlink.h>
+/*
+ * A non-zero value for sysctl_ip_reassembly_count defines the
+ * size of the window of ip fragments that are considered valid.
+ * This is useful for preventing reassembly of fragments from
+ * different IP datagrams when the 16-bit IP id wraps.
+ * A value of zero means the window is unlimited.
+ */
+int sysctl_ip_reassembly_count = 0;
/*
* SNMP management statistics
*/
@@ -286,13 +294,17 @@ static inline int ip_rcv_finish(struct s
{
struct net_device *dev = skb->dev;
struct iphdr *iph = skb->nh.iph;
+ __u32 saddr = iph->saddr;
+ __u32 daddr = iph->daddr;
+ __u8 proto = iph->protocol;
+ int ret;
/*
* Initialise the virtual path cache for the packet. It describes
* how the packet travels inside Linux networking.
*/
if (skb->dst == NULL) {
- if (ip_route_input(skb, iph->daddr, iph->saddr, iph->tos, dev))
+ if (ip_route_input(skb, daddr, saddr, iph->tos, dev))
goto drop;
}
@@ -334,7 +346,7 @@ static inline int ip_rcv_finish(struct s
if (!IN_DEV_SOURCE_ROUTE(in_dev)) {
if (IN_DEV_LOG_MARTIANS(in_dev) && net_ratelimit())
printk(KERN_INFO "source route option %u.%u.%u.%u -> %u.%u.%u.%u\n",
- NIPQUAD(iph->saddr), NIPQUAD(iph->daddr));
+ NIPQUAD(saddr), NIPQUAD(iph->daddr));
in_dev_put(in_dev);
goto drop;
}
@@ -345,7 +357,13 @@ static inline int ip_rcv_finish(struct s
}
}
- return dst_input(skb);
+ ret = dst_input(skb);
+
+ if (sysctl_ip_reassembly_count) {
+ ip_count(saddr, daddr, proto);
+ }
+
+ return ret;
inhdr_error:
IP_INC_STATS_BH(IPSTATS_MIB_INHDRERRORS);
diff -rup linux.orig/net/ipv4/sysctl_net_ipv4.c linux.new/net/ipv4/sysctl_net_ipv4.c
--- linux.orig/net/ipv4/sysctl_net_ipv4.c 2005-06-14 11:36:29.923218508 -0700
+++ linux.new/net/ipv4/sysctl_net_ipv4.c 2005-06-22 14:03:50.869948048 -0700
@@ -29,6 +29,7 @@ extern int sysctl_ipfrag_low_thresh;
extern int sysctl_ipfrag_high_thresh;
extern int sysctl_ipfrag_time;
extern int sysctl_ipfrag_secret_interval;
+extern int sysctl_ip_reassembly_count;
/* From ip_output.c */
extern int sysctl_ip_dynaddr;
@@ -49,6 +50,7 @@ extern int inet_peer_gc_mintime;
extern int inet_peer_gc_maxtime;
#ifdef CONFIG_SYSCTL
+static int zero;
static int tcp_retr1_max = 255;
static int ip_local_port_range_min[] = { 1, 1 };
static int ip_local_port_range_max[] = { 65535, 65535 };
@@ -595,6 +597,15 @@ ctl_table ipv4_table[] = {
.strategy = &sysctl_jiffies
},
{
+ .ctl_name = NET_IPV4_REASM_COUNT,
+ .procname = "ip_reassembly_count",
+ .data = &sysctl_ip_reassembly_count,
+ .maxlen = sizeof(int),
+ .mode = 0644,
+ .proc_handler = &proc_dointvec_minmax,
+ .extra1 = &zero
+ },
+ {
.ctl_name = NET_TCP_NO_METRICS_SAVE,
.procname = "tcp_no_metrics_save",
.data = &sysctl_tcp_nometrics_save,
--
Arthur
^ permalink raw reply
* Re: RFC: NAPI packet weighting patch
From: jamal @ 2005-06-23 12:14 UTC (permalink / raw)
To: David S. Miller
Cc: Lennert Buytenhek, davidm, netdev, dada1, ak, leonid.grossman,
becker, rick.jones2, davem
In-Reply-To: <20050622.152325.15263910.davem@davemloft.net>
On Wed, 2005-22-06 at 15:23 -0700, David S. Miller wrote:
> From: Eric Dumazet <dada1@cosmosbay.com>
> Date: Wed, 22 Jun 2005 23:38:21 +0200
>
> > Then maybe we could also play with prefetchw() in the case the
> > incoming frame is small enough to be copied to a new skb.
>
> That's a good idea too. In fact, this would deal with platforms
> that use non-temporal stores in their memcpy() implementation.
For the fans of the e1000 (or even the tg3 deprived people), heres a
patch which originated from David Mosberger that i played around (about
9 months back) - it will need some hand patching for the latest driver.
Similar approach: prefetch skb->data,twiddle twiddle not little star,
touch header.
I found the aggressive mode effective on a xeon but i belive David is
using this on x86_64. So Lennert, I lied to you saying it was never
effective on x86. You just have to do the right juju such as factoring
in the memory load-latency and how much cache you have on your specific
CPU.
CCing davidm (in addition To: davem of course ;->) so he may provide
more insight on his tests.
Interesting of course is if you miss the "twiddle here" (as i saw in my
experiments) and do the obvious (such as defining AGGRESSIVE to 0), you
infact end up paying a penalty in performance.
cheers,
jamal
===== drivers/net/e1000/e1000_main.c 1.134 vs edited =====
--- 1.134/drivers/net/e1000/e1000_main.c 2004-09-12 16:52:48
-07:00
+++ edited/drivers/net/e1000/e1000_main.c 2004-09-30 06:05:11
-07:00
@@ -2278,12 +2278,30 @@
uint8_t last_byte;
unsigned int i;
boolean_t cleaned = FALSE;
+#define AGGRESSIVE 1
i = rx_ring->next_to_clean;
+#if AGGRESSIVE
+ prefetch(rx_ring->buffer_info[i].skb->data);
+#endif
rx_desc = E1000_RX_DESC(*rx_ring, i);
while(rx_desc->status & E1000_RXD_STAT_DD) {
buffer_info = &rx_ring->buffer_info[i];
+# if AGGRESSIVE
+ {
+ struct e1000_rx_desc *next_rx;
+ unsigned int j = i + 1;
+
+ if (j == rx_ring->count)
+ j = 0;
+ next_rx = E1000_RX_DESC(*rx_ring, j);
+ if (next_rx->status & E1000_RXD_STAT_DD)
+ prefetch(rx_ring->buffer_info[j].skb->data);
+ }
+# else
+ prefetch(buffer_info->skb->data);
+# endif
#ifdef CONFIG_E1000_NAPI
if(*work_done >= work_to_do)
break;
^ permalink raw reply
* Re: [patch] devinet: cleanup if statements
From: Herbert Xu @ 2005-06-23 11:38 UTC (permalink / raw)
To: David S. Miller; +Cc: pmeda, jgarzik, akpm, netdev
In-Reply-To: <20050621.134822.21926602.davem@davemloft.net>
David S. Miller <davem@davemloft.net> wrote:
>
> The second hunk of your patch seems to defeat the intention
> of that code. I believe the idea is that if the label and
> the device name differ, use the label.
Actually I think Prasanna is right. The if conditional is testing
whether ifa->ifa_label is NULL. As ifa->ifa_label is an array and
it's not the first element in the structure, it can't possibly be
NULL.
With your interpretation above his patch is correct as well. If
we want to use the label when it is different from the device name,
then it is equivalent to always use the label since the only time
we'd use the device name is when it's equal to the label :)
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
* [git patch] urgent e1000 fix
From: Jeff Garzik @ 2005-06-23 9:24 UTC (permalink / raw)
To: Andrew Morton, Linus Torvalds; +Cc: Linux Kernel, Netdev List
[-- Attachment #1: Type: text/plain, Size: 174 bytes --]
Please pull from 'misc-fixes' branch of
rsync://rsync.kernel.org/pub/scm/linux/kernel/git/jgarzik/netdev-2.6.git
to obtain the spinlock fix described in the attached text.
[-- Attachment #2: netdev-2.6.txt --]
[-- Type: text/plain, Size: 495 bytes --]
drivers/net/e1000/e1000_main.c | 1 +
1 files changed, 1 insertion(+)
Mitch Williams:
e1000: fix spinlock bug
diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
--- a/drivers/net/e1000/e1000_main.c
+++ b/drivers/net/e1000/e1000_main.c
@@ -2307,6 +2307,7 @@ e1000_xmit_frame(struct sk_buff *skb, st
tso = e1000_tso(adapter, skb);
if (tso < 0) {
dev_kfree_skb_any(skb);
+ spin_unlock_irqrestore(&adapter->tx_lock, flags);
return NETDEV_TX_OK;
}
^ permalink raw reply
* Re: RFC: NAPI packet weighting patch
From: P @ 2005-06-23 8:56 UTC (permalink / raw)
To: hadi
Cc: David S. Miller, gandalf, shemminger, mitch.a.williams,
john.ronciak, mchan, buytenh, jdmason, netdev, Robert.Olsson,
ganesh.venkatesan, jesse.brandeburg
In-Reply-To: <1119469066.6918.168.camel@localhost.localdomain>
jamal wrote:
> On Wed, 2005-22-06 at 09:42 +0100, P@draigBrady.com wrote:
>
>
>>Yes the copy is essentially free here as the data is already cached.
>>
>>As a data point, I went the whole hog and used buffer recycling
>>in my essentially packet sniffing application. I.E. there are no
>>allocs per packet at all, and this make a HUGE difference. On a
>>2x3.4GHz 2xe1000 system I can receive 620Kpps per port sustained
>>into my userspace app which does a LOT of processing per packet.
>>Without the buffer recycling is was around 250Kpps.
>>Note I don't reuse an skb until the packet is copied into a
>>PACKET_MMAP buffer.
>
>
> Was this machine SMP?
Yes. 2 x 3.4GHz P4s
1 logical CPU per port (irq affinity)
1 thread (NB on same logical CPU as irq (sched_affinity))
to do user space per packet processing.
> NAPI involved?
Yep.
> I take it nothing interfering in
> the middle with the headers?
It uses the standard path to PACKET_MMAP buffer
e1000_clean_rx_irq -> netif_receive_skb -> tpacket_rcv
Pádraig.
^ permalink raw reply
* Re: [patch 0/3] netpoll: support multiple netpoll clients per net_device
From: David S. Miller @ 2005-06-23 5:06 UTC (permalink / raw)
To: jmoyer; +Cc: mpm, netdev, linux-kernel, akpm
In-Reply-To: <17082.4037.875432.648439@segfault.boston.redhat.com>
From: Jeff Moyer <jmoyer@redhat.com>
Date: Wed, 22 Jun 2005 21:26:29 -0400
> This patch series restores the ability to register multiple netpoll clients
> against the same network interface. To this end, I created a new structure:
...
> I have tested this by registering two netpoll clients, and verifying that
> they both function properly. The clients were netconsole, and a quick
> module I hacked together to send console messages to syslog. I issued
> sysrq-h, sysrq-m, and sysrq-t's both by echo'ing to /proc/sysrq-trigger and
> by hitting the key combination on the keyboard. This verifies that the
> modules work both inside and out of interrupt context.
This all looks great. I've applied all 3 patches.
Thanks for taking care of this Jeff.
^ permalink raw reply
* [patch 3/3] netpoll: allow multiple netpoll_clients to register against one interface
From: Jeff Moyer @ 2005-06-23 1:30 UTC (permalink / raw)
To: mpm, netdev, linux-kernel, akpm
In-Reply-To: <17082.4037.875432.648439@segfault.boston.redhat.com>
Hi,
This patch provides support for registering multiple netpoll clients to the
same network device. Only one of these clients may register an rx_hook,
however. In practice, this restriction has not been problematic. It is
worth mentioning, though, that the current design can be easily extended to
allow for the registration of multiple rx_hooks.
The basic idea of the patch is that the rx_np pointer in the netpoll_info
structure points to the struct netpoll that has rx_hook filled in. Aside
from this one case, there is no need for a pointer from the struct
net_device to an individual struct netpoll.
A lock is introduced to protect the setting and clearing of the np_rx
pointer. The pointer will only be cleared upon netpoll client module
removal, and the lock should be uncontested.
-Jeff
Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
---
--- linux-2.6.12/net/core/netpoll.c.orig 2005-06-22 20:13:21.823259533 -0400
+++ linux-2.6.12/net/core/netpoll.c 2005-06-22 20:13:33.304353028 -0400
@@ -349,11 +349,15 @@ static void arp_reply(struct sk_buff *sk
unsigned char *arp_ptr;
int size, type = ARPOP_REPLY, ptype = ETH_P_ARP;
u32 sip, tip;
+ unsigned long flags;
struct sk_buff *send_skb;
struct netpoll *np = NULL;
- if (npinfo)
- np = npinfo->np;
+ spin_lock_irqsave(&npinfo->rx_lock, flags);
+ if (npinfo->rx_np && npinfo->rx_np->dev == skb->dev)
+ np = npinfo->rx_np;
+ spin_unlock_irqrestore(&npinfo->rx_lock, flags);
+
if (!np)
return;
@@ -436,9 +440,9 @@ int __netpoll_rx(struct sk_buff *skb)
int proto, len, ulen;
struct iphdr *iph;
struct udphdr *uh;
- struct netpoll *np = skb->dev->npinfo->np;
+ struct netpoll *np = skb->dev->npinfo->rx_np;
- if (!np->rx_hook)
+ if (!np)
goto out;
if (skb->dev->type != ARPHRD_ETHER)
goto out;
@@ -619,6 +623,7 @@ int netpoll_setup(struct netpoll *np)
struct net_device *ndev = NULL;
struct in_device *in_dev;
struct netpoll_info *npinfo;
+ unsigned long flags;
if (np->dev_name)
ndev = dev_get_by_name(np->dev_name);
@@ -634,9 +639,10 @@ int netpoll_setup(struct netpoll *np)
if (!npinfo)
goto release;
- npinfo->np = NULL;
+ npinfo->rx_np = NULL;
npinfo->poll_lock = SPIN_LOCK_UNLOCKED;
npinfo->poll_owner = -1;
+ npinfo->rx_lock = SPIN_LOCK_UNLOCKED;
} else
npinfo = ndev->npinfo;
@@ -706,9 +712,13 @@ int netpoll_setup(struct netpoll *np)
np->name, HIPQUAD(np->local_ip));
}
- if(np->rx_hook)
- npinfo->rx_flags = NETPOLL_RX_ENABLED;
- npinfo->np = np;
+ if (np->rx_hook) {
+ spin_lock_irqsave(&npinfo->rx_lock, flags);
+ npinfo->rx_flags |= NETPOLL_RX_ENABLED;
+ npinfo->rx_np = np;
+ spin_unlock_irqrestore(&npinfo->rx_lock, flags);
+ }
+ /* last thing to do is link it to the net device structure */
ndev->npinfo = npinfo;
return 0;
@@ -723,11 +733,20 @@ int netpoll_setup(struct netpoll *np)
void netpoll_cleanup(struct netpoll *np)
{
+ struct netpoll_info *npinfo;
+ unsigned long flags;
+
if (np->dev) {
- if (np->dev->npinfo)
- np->dev->npinfo->np = NULL;
+ npinfo = np->dev->npinfo;
+ if (npinfo && npinfo->rx_np == np) {
+ spin_lock_irqsave(&npinfo->rx_lock, flags);
+ npinfo->rx_np = NULL;
+ npinfo->rx_flags &= ~NETPOLL_RX_ENABLED;
+ spin_unlock_irqrestore(&npinfo->rx_lock, flags);
+ }
dev_put(np->dev);
}
+
np->dev = NULL;
}
--- linux-2.6.12/include/linux/netpoll.h.orig 2005-06-22 20:11:59.119992646 -0400
+++ linux-2.6.12/include/linux/netpoll.h 2005-06-22 20:13:33.306352696 -0400
@@ -27,7 +27,8 @@ struct netpoll_info {
spinlock_t poll_lock;
int poll_owner;
int rx_flags;
- struct netpoll *np;
+ spinlock_t rx_lock;
+ struct netpoll *rx_np; /* netpoll that registered an rx_hook */
};
void netpoll_poll(struct netpoll *np);
@@ -44,11 +45,19 @@ void netpoll_queue(struct sk_buff *skb);
static inline int netpoll_rx(struct sk_buff *skb)
{
struct netpoll_info *npinfo = skb->dev->npinfo;
+ unsigned long flags;
+ int ret = 0;
- if (!npinfo || !npinfo->rx_flags)
+ if (!npinfo || (!npinfo->rx_np && !npinfo->rx_flags))
return 0;
- return npinfo->np && __netpoll_rx(skb);
+ spin_lock_irqsave(&npinfo->rx_lock, flags);
+ /* check rx_flags again with the lock held */
+ if (npinfo->rx_flags && __netpoll_rx(skb))
+ ret = 1;
+ spin_unlock_irqrestore(&npinfo->rx_lock, flags);
+
+ return ret;
}
static inline void netpoll_poll_lock(struct net_device *dev)
^ permalink raw reply
* [patch 2/3] netpoll: Introduce a netpoll_info struct
From: Jeff Moyer @ 2005-06-23 1:30 UTC (permalink / raw)
To: mpm, netdev, linux-kernel, akpm
In-Reply-To: <17082.4037.875432.648439@segfault.boston.redhat.com>
Hi,
This patch introduces a netpoll_info structure, which the struct net_device
will now point to instead of pointing to a struct netpoll. The reason for
this is two-fold: 1) fields such as the rx_flags, poll_owner, and poll_lock
should be maintained per net_device, not per netpoll; and 2) this is a first
step in providing support for multiple netpoll clients to register against the
same net_device.
The struct netpoll is now pointed to by the netpoll_info structure. As
such, the previous behaviour of the code is preserved.
-Jeff
Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
---
--- linux-2.6.12/net/core/netpoll.c.orig 2005-06-22 20:11:06.673701253 -0400
+++ linux-2.6.12/net/core/netpoll.c 2005-06-22 20:11:45.323283581 -0400
@@ -130,19 +130,20 @@ static int checksum_udp(struct sk_buff *
*/
static void poll_napi(struct netpoll *np)
{
+ struct netpoll_info *npinfo = np->dev->npinfo;
int budget = 16;
if (test_bit(__LINK_STATE_RX_SCHED, &np->dev->state) &&
- np->poll_owner != smp_processor_id() &&
- spin_trylock(&np->poll_lock)) {
- np->rx_flags |= NETPOLL_RX_DROP;
+ npinfo->poll_owner != smp_processor_id() &&
+ spin_trylock(&npinfo->poll_lock)) {
+ npinfo->rx_flags |= NETPOLL_RX_DROP;
atomic_inc(&trapped);
np->dev->poll(np->dev, &budget);
atomic_dec(&trapped);
- np->rx_flags &= ~NETPOLL_RX_DROP;
- spin_unlock(&np->poll_lock);
+ npinfo->rx_flags &= ~NETPOLL_RX_DROP;
+ spin_unlock(&npinfo->poll_lock);
}
}
@@ -245,6 +246,7 @@ repeat:
static void netpoll_send_skb(struct netpoll *np, struct sk_buff *skb)
{
int status;
+ struct netpoll_info *npinfo;
repeat:
if(!np || !np->dev || !netif_running(np->dev)) {
@@ -253,8 +255,9 @@ repeat:
}
/* avoid recursion */
- if(np->poll_owner == smp_processor_id() ||
- np->dev->xmit_lock_owner == smp_processor_id()) {
+ npinfo = np->dev->npinfo;
+ if (npinfo->poll_owner == smp_processor_id() ||
+ np->dev->xmit_lock_owner == smp_processor_id()) {
if (np->drop)
np->drop(skb);
else
@@ -341,14 +344,18 @@ void netpoll_send_udp(struct netpoll *np
static void arp_reply(struct sk_buff *skb)
{
+ struct netpoll_info *npinfo = skb->dev->npinfo;
struct arphdr *arp;
unsigned char *arp_ptr;
int size, type = ARPOP_REPLY, ptype = ETH_P_ARP;
u32 sip, tip;
struct sk_buff *send_skb;
- struct netpoll *np = skb->dev->np;
+ struct netpoll *np = NULL;
- if (!np) return;
+ if (npinfo)
+ np = npinfo->np;
+ if (!np)
+ return;
/* No arp on this interface */
if (skb->dev->flags & IFF_NOARP)
@@ -429,7 +436,7 @@ int __netpoll_rx(struct sk_buff *skb)
int proto, len, ulen;
struct iphdr *iph;
struct udphdr *uh;
- struct netpoll *np = skb->dev->np;
+ struct netpoll *np = skb->dev->npinfo->np;
if (!np->rx_hook)
goto out;
@@ -611,9 +618,7 @@ int netpoll_setup(struct netpoll *np)
{
struct net_device *ndev = NULL;
struct in_device *in_dev;
-
- np->poll_lock = SPIN_LOCK_UNLOCKED;
- np->poll_owner = -1;
+ struct netpoll_info *npinfo;
if (np->dev_name)
ndev = dev_get_by_name(np->dev_name);
@@ -624,7 +629,16 @@ int netpoll_setup(struct netpoll *np)
}
np->dev = ndev;
- ndev->np = np;
+ if (!ndev->npinfo) {
+ npinfo = kmalloc(sizeof(*npinfo), GFP_KERNEL);
+ if (!npinfo)
+ goto release;
+
+ npinfo->np = NULL;
+ npinfo->poll_lock = SPIN_LOCK_UNLOCKED;
+ npinfo->poll_owner = -1;
+ } else
+ npinfo = ndev->npinfo;
if (!ndev->poll_controller) {
printk(KERN_ERR "%s: %s doesn't support polling, aborting.\n",
@@ -693,12 +707,15 @@ int netpoll_setup(struct netpoll *np)
}
if(np->rx_hook)
- np->rx_flags = NETPOLL_RX_ENABLED;
+ npinfo->rx_flags = NETPOLL_RX_ENABLED;
+ npinfo->np = np;
+ ndev->npinfo = npinfo;
return 0;
release:
- ndev->np = NULL;
+ if (!ndev->npinfo)
+ kfree(npinfo);
np->dev = NULL;
dev_put(ndev);
return -1;
@@ -706,9 +723,11 @@ int netpoll_setup(struct netpoll *np)
void netpoll_cleanup(struct netpoll *np)
{
- if (np->dev)
- np->dev->np = NULL;
- dev_put(np->dev);
+ if (np->dev) {
+ if (np->dev->npinfo)
+ np->dev->npinfo->np = NULL;
+ dev_put(np->dev);
+ }
np->dev = NULL;
}
--- linux-2.6.12/include/linux/netpoll.h.orig 2005-06-22 20:11:15.326264524 -0400
+++ linux-2.6.12/include/linux/netpoll.h 2005-06-22 20:11:59.119992646 -0400
@@ -16,14 +16,18 @@ struct netpoll;
struct netpoll {
struct net_device *dev;
char dev_name[16], *name;
- int rx_flags;
void (*rx_hook)(struct netpoll *, int, char *, int);
void (*drop)(struct sk_buff *skb);
u32 local_ip, remote_ip;
u16 local_port, remote_port;
unsigned char local_mac[6], remote_mac[6];
+};
+
+struct netpoll_info {
spinlock_t poll_lock;
int poll_owner;
+ int rx_flags;
+ struct netpoll *np;
};
void netpoll_poll(struct netpoll *np);
@@ -39,22 +43,27 @@ void netpoll_queue(struct sk_buff *skb);
#ifdef CONFIG_NETPOLL
static inline int netpoll_rx(struct sk_buff *skb)
{
- return skb->dev->np && skb->dev->np->rx_flags && __netpoll_rx(skb);
+ struct netpoll_info *npinfo = skb->dev->npinfo;
+
+ if (!npinfo || !npinfo->rx_flags)
+ return 0;
+
+ return npinfo->np && __netpoll_rx(skb);
}
static inline void netpoll_poll_lock(struct net_device *dev)
{
- if (dev->np) {
- spin_lock(&dev->np->poll_lock);
- dev->np->poll_owner = smp_processor_id();
+ if (dev->npinfo) {
+ spin_lock(&dev->npinfo->poll_lock);
+ dev->npinfo->poll_owner = smp_processor_id();
}
}
static inline void netpoll_poll_unlock(struct net_device *dev)
{
- if (dev->np) {
- dev->np->poll_owner = -1;
- spin_unlock(&dev->np->poll_lock);
+ if (dev->npinfo) {
+ dev->npinfo->poll_owner = -1;
+ spin_unlock(&dev->npinfo->poll_lock);
}
}
--- linux-2.6.12/include/linux/netdevice.h.orig 2005-06-22 20:11:24.277778150 -0400
+++ linux-2.6.12/include/linux/netdevice.h 2005-06-22 20:11:45.325283249 -0400
@@ -41,7 +41,7 @@
struct divert_blk;
struct vlan_group;
struct ethtool_ops;
-struct netpoll;
+struct netpoll_info;
/* source back-compat hooks */
#define SET_ETHTOOL_OPS(netdev,ops) \
( (netdev)->ethtool_ops = (ops) )
@@ -468,7 +468,7 @@ struct net_device
unsigned char *haddr);
int (*neigh_setup)(struct net_device *dev, struct neigh_parms *);
#ifdef CONFIG_NETPOLL
- struct netpoll *np;
+ struct netpoll_info *npinfo;
#endif
#ifdef CONFIG_NET_POLL_CONTROLLER
void (*poll_controller)(struct net_device *dev);
^ permalink raw reply
* [patch 1/3] netpoll: set poll_owner to -1 before unlocking in netpoll_poll_unlock
From: Jeff Moyer @ 2005-06-23 1:27 UTC (permalink / raw)
To: mpm, netdev, linux-kernel, akpm
In-Reply-To: <17082.4037.875432.648439@segfault.boston.redhat.com>
Hi,
This trivial patch moves the assignment of poll_owner to -1 inside of the
lock. This fixes a potential SMP race in the code.
-Jeff
Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
---
--- linux-2.6.12/include/linux/netpoll.h.orig 2005-06-22 18:47:12.917261688 -0400
+++ linux-2.6.12/include/linux/netpoll.h 2005-06-22 18:47:15.799783018 -0400
@@ -53,8 +53,8 @@ static inline void netpoll_poll_lock(str
static inline void netpoll_poll_unlock(struct net_device *dev)
{
if (dev->np) {
- spin_unlock(&dev->np->poll_lock);
dev->np->poll_owner = -1;
+ spin_unlock(&dev->np->poll_lock);
}
}
^ 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