* Re: [PATCH][BNX2X] round three
From: David Miller @ 2007-10-11 21:40 UTC (permalink / raw)
To: eliezert; +Cc: netdev, jeff, mchan
In-Reply-To: <1192125219.29746.143.camel@eliezer>
From: "Eliezer Tamir" <eliezert@broadcom.com>
Date: Thu, 11 Oct 2007 19:53:39 +0200
> Will it be OK if we replace it with 4-5 headers so we can maintain them
> better manually. (I'm asking what is the upper bound to the number of
> header files you would consider reasonable for this driver.)
It's not the count, it's what's in them that's the problem.
^ permalink raw reply
* Re: iproute2: resend of patches from Debian.
From: David Miller @ 2007-10-11 21:48 UTC (permalink / raw)
To: andreas; +Cc: kaber, formorer, shemminger, netdev
In-Reply-To: <1192137732.4733.34.camel@localhost.localdomain>
From: Andreas Henriksson <andreas@fatal.se>
Date: Thu, 11 Oct 2007 23:22:12 +0200
> Copying Alexander Wirt here again, as I think this is his patch.
It is important that whoever is the package maintainer for an
upstream piece of code in any distribution:
1) Completely understands the patches he is applying.
and therefore:
2) When he submits upstream, he doesn't have to wake up
2,000 patch submitters from the dead just to figure out
why a change was made.
Neither seems to be occuring here.
^ permalink raw reply
* Re: [PATCH] IB/ipoib: Bound the net device to the ipoib_neigh structue
From: Jay Vosburgh @ 2007-10-11 22:01 UTC (permalink / raw)
To: Roland Dreier; +Cc: Moni Shoua, jeff, David Miller, ogerlitz, netdev, Moni Levy
In-Reply-To: <ada3awhcsnw.fsf@cisco.com>
Roland Dreier <rdreier@cisco.com> wrote:
[...]
>Yes, two napi_disable()s in a row without a matching napi_enable()
>will deadlock. I guess the question is why the ipoib interface is
>being stopped twice.
>
>If you just take the net-2.6.24 tree (without bonding patches), does
>bonding for ethernet interfaces work OK, or is there a similar problem
>with double napi_disable()? How about bonding of ethernet after this
>batch of bonding patches?
I just checked this on an x86 box. The bonding in stock net-2.6
pulled this morning or last night works ok (I did some basic tests,
including ifconfig down / up, with e100). This remains true with the
IPoIB bonding patches applied. I do not have hardware available to test
IPoIB.
I did get a whammy from tg3, but I think this is unrelated to
bonding (as it happens when tg3 comes up, before bonding is involved):
BUG: unable to handle kernel paging request at virtual address 00004214
printing eip:
e0828017
*pde = 00000000
Oops: 0002 [#1]
SMP
Modules linked in: thermal processor fan button loop e1000 sg evdev tg3 e100 rtb
CPU: 0
EIP: 0060:[<e0828017>] Not tainted VLI
EFLAGS: 00010206 (2.6.23-ipv6 #1)
EIP is at tg3_ape_write32+0x7/0x10 [tg3]
eax: de9304c0 ebx: dde8fe18 ecx: 00000000 edx: 00004214
esi: de9304c0 edi: 00000000 ebp: dde8fe28 esp: dde8fdd4
ds: 007b es: 007b fs: 00d8 gs: 0033 ss: 0068
Process ip (pid: 2817, ti=dde8e000 task=dff4e0b0 task.ti=dde8e000)
Stack: e082fb2e 00000000 dde8fdf4 c01ece3e dde8fdf8 000003fe 00000000 00005400
08000000 00001aa0 e083b340 08001aa0 00000060 e083ce00 08001b20 00000030
e083ce80 00000101 de9304c0 00000001 dde56800 dde8fe38 e0830178 dff69000
Call Trace:
[<c010536a>] show_trace_log_lvl+0x1a/0x30
[<c0105429>] show_stack_log_lvl+0xa9/0xd0
[<c0105639>] show_registers+0x1e9/0x2f0
[<c0105851>] die+0x111/0x260
[<c011c5dc>] do_page_fault+0x18c/0x6a0
[<c0319bea>] error_code+0x72/0x78
[<e0830178>] tg3_init_hw+0x38/0x50 [tg3]
[<e0838886>] tg3_open+0x276/0x5d0 [tg3]
[<c02aead8>] dev_open+0x38/0x80
[<c02ad5cd>] dev_change_flags+0x7d/0x1a0
[<c02f63d8>] devinet_ioctl+0x4c8/0x660
[<c02f698b>] inet_ioctl+0x6b/0x90
[<c02a0e5a>] sock_ioctl+0x5a/0x210
[<c017cd98>] do_ioctl+0x28/0x80
[<c017ce47>] vfs_ioctl+0x57/0x290
[<c017d0b9>] sys_ioctl+0x39/0x60
[<c01042a2>] sysenter_past_esp+0x5f/0x99
=======================
Code: <89> 0a c3 8d b6 00 00 00 00 55 8b 48 50 89 e5 5d 01 ca 8b 02 c3 8d
EIP: [<e0828017>] tg3_ape_write32+0x7/0x10 [tg3] SS:ESP 0068:dde8fdd4
Kernel panic - not syncing: Fatal exception in interrupt
I haven't investigated this further. I'm using a BCM5704 card;
if this isn't a known problem and anyone is curious, I can supply
additional info.
-J
---
-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com
^ permalink raw reply
* Re: iproute2: resend of patches from Debian.
From: Andreas Henriksson @ 2007-10-11 22:35 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20071011.144843.125887830.davem@davemloft.net>
On tor, 2007-10-11 at 14:48 -0700, David Miller wrote:
> From: Andreas Henriksson <andreas@fatal.se>
> Date: Thu, 11 Oct 2007 23:22:12 +0200
>
> > Copying Alexander Wirt here again, as I think this is his patch.
>
> It is important that whoever is the package maintainer for an
> upstream piece of code in any distribution:
>
> 1) Completely understands the patches he is applying.
>
> and therefore:
>
> 2) When he submits upstream, he doesn't have to wake up
> 2,000 patch submitters from the dead just to figure out
> why a change was made.
>
> Neither seems to be occuring here.
Alexander Wirt is both the Debian package maintainer and (probably) the
author of both of the two discussed patches (and only a single person as
far as I know). I don't consider it rude to notify him of the comments
made about the patches he's been carrying for a long time.
While doing a couple of fixes to iproute I thought it would be best to
not only submit those but all the patches that has been carried in
debian for review upstream and possibly inclusion. The extra review
seemed to have been a success so far and I will fix up the patches
myself if I have to.
If you want me to STFU please just say so and I'll give you all my
sincere appologies and go back to the status quo of no fixes shared with
others.
--
Regards,
Andreas Henriksson
^ permalink raw reply
* Re: iproute2: resend of patches from Debian.
From: David Miller @ 2007-10-11 22:38 UTC (permalink / raw)
To: andreas; +Cc: netdev
In-Reply-To: <1192142107.4733.53.camel@localhost.localdomain>
From: Andreas Henriksson <andreas@fatal.se>
Date: Fri, 12 Oct 2007 00:35:07 +0200
> If you want me to STFU please just say so and I'll give you all my
> sincere appologies and go back to the status quo of no fixes shared
> with others.
I applaud your efforts, it's not what the problem is.
I just wonder, therefore, when Alexander planned on submitting
this sizable backlog of local iproute2 patches.
^ permalink raw reply
* Non-linear SKBs
From: Kristian Evensen @ 2007-10-11 22:54 UTC (permalink / raw)
To: netdev; +Cc: linux-net
Hello,
I have developed a small patch for the TCP code in 2.6.19 and it works
flawlessly. A couple of days ago I decided to make it compatible with
2.6.22.5 and have stumbled upon a problem I cannot solve.
In 2.6.19 it seems that all packets (at least the ones my patch work
with) are linear, while they are non-linear in 2.6.22.5. I have searched
through the code (focusing on tcp_sendmsg) to try to figure out what
happens, but can't find any differences that would explain this. Does
anyone know what might be the cause and if there is an easy way to
return to linear skbs (unless that is totally stupid)? I would also like
the benefits offered by the collapsing when retransmitting (which
requires number of fragments to be 0).
Currently I us e the skb_linearize(skb) on all fragmentet packets, which
is not nice :) Both kernels are vanilla kernels with the patch applied,
and I used OpenSuse with 2.6.19 and Ubuntu with 2.6.22.5 (but I guess
that shouldn't matter in this case).
The reason this is a problem is that I copy data between SKBs, and in
2.6.19 I have used memcpy for this purpose. I have looked at the way the
kernel copies data into a non-linear skb in the else-part of the large
if-test in tcp_sendmsg and the skb_copy_bits-function, but I have to
admit that I think the first one is a bit advanced and I don't quite
understand how to use the last one. Does anyone have any easier to
understand examples or explanations on how to copy data from one
non-linear skb to another?
Thanks.
-Kristian
^ permalink raw reply
* bytes received from recvmsg doesn't match FIONREAD
From: Steven Dake @ 2007-10-11 23:08 UTC (permalink / raw)
To: netdev
I wanted to verify that the size of a multicast UDP message received
with recvmsg matches the size of the message the kernel thinks the
message is.
So I went about using the FIONREAD ioctl as follows:
res = ioctl (fd, FIONCREAD, &value);
assert (res != -1);
bytes_received = recvmsg (fd, &msg_recv, MSG_NOSIGNAL | MSG_DONTWAIT);
assert (bytes_received == value);
It appears the value and bytes_received do not match after many
thousands of runs with a UDP multicast protocol (www.openais.org).
(neither is -1)
Am I using this ioctl improperly? Shouldn't I expect that the full
datagram is returned by recvmsg no matter the state of the blocking or
nonblocking mode of the file descriptor?
pls copy me on responses - i am not onlist.
Regards
-steve
^ permalink raw reply
* Re: e100 problems in .23rc8 ?
From: Herbert Xu @ 2007-10-11 23:24 UTC (permalink / raw)
To: Kok, Auke; +Cc: Dave Jones, netdev, esandeen, dmack
In-Reply-To: <470E4AFA.60507@intel.com>
On Thu, Oct 11, 2007 at 09:10:34AM -0700, Kok, Auke wrote:
> >>
> >> commit 416b5d10afdc797c21c457ade3714e8f2f75edd9
> >> Author: Auke Kok <auke-jan.h.kok@intel.com>
> >> Date: Fri Jun 1 10:22:39 2007 -0700
> >>
> >> e1000: disable polling before registering netdevice
>
> this patch actually called napi_disable() in the probe routine which was wrong,
> but e100 does not do that. Nonetheless e100 doesn't call netif_carrier_off() and
> netif_stop_queue(), so to make e100 the same as e1000 we should probably do this,
> see below.
Back then we didn't have napi_disable at all. That patch calls
netif_poll_disable which has different semantics.
> Dave, can you see if this resolves the issue for you? If so then we might want to
> push this to -stable.
The problem is with netif_poll so this patch probably won't help.
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
* Question on TSO maximum segment sizes.
From: Waskiewicz Jr, Peter P @ 2007-10-11 23:27 UTC (permalink / raw)
To: netdev
I'm having an issue with TSO right vs. hardware that can't take the
maximum segment size sent from the stack. I've been told that the
maximum packet size that can be sent to the hardware today is 64k, but
my hardware can only take 32k in certain modes per queue due to hardware
limitations. I have two questions regarding this: 1) where is this
value set in the TCP code, and 2) Is this something that can be
configured on the fly? If the answer to 2 is no, I will try and put
something together to allow this to happen.
Thanks in advance,
PJ Waskiewicz
Intel Corporation
peter.p.waskiewicz.jr@intel.com
^ permalink raw reply
* Re: bytes received from recvmsg doesn't match FIONREAD
From: David Miller @ 2007-10-11 23:35 UTC (permalink / raw)
To: sdake; +Cc: netdev
In-Reply-To: <1192144096.4147.16.camel@balance>
From: Steven Dake <sdake@redhat.com>
Date: Thu, 11 Oct 2007 16:08:15 -0700
> I wanted to verify that the size of a multicast UDP message received
> with recvmsg matches the size of the message the kernel thinks the
> message is.
>
> So I went about using the FIONREAD ioctl as follows:
>
> res = ioctl (fd, FIONCREAD, &value);
^^^^^^^^^ (typo? should be FIONREAD not FION_C_READ)
> assert (res != -1);
> bytes_received = recvmsg (fd, &msg_recv, MSG_NOSIGNAL | MSG_DONTWAIT);
> assert (bytes_received == value);
I think you want to use SIOCINQ, FIONREAD is only valid on files.
I'm surprised you didn't get an error return from ioctl() as the
VFS code seems to enforce this.
^ permalink raw reply
* Re: bytes received from recvmsg doesn't match FIONREAD
From: David Stevens @ 2007-10-11 23:36 UTC (permalink / raw)
To: sdake; +Cc: netdev
In-Reply-To: <1192144096.4147.16.camel@balance>
Questions of this sort should normally be directed to linux-net mailing
list.
>From the code you quoted, I see at least one case where it
will fail -- when the allocated buffer you pass to recvmsg is smaller than
"value" (ie. the datagram is too big for the read buffer).
If that's not the problem, I suggest you reproduce it with a small test
case and post real code and results when it happens.
+-DLS
^ permalink raw reply
* Re: Question on TSO maximum segment sizes.
From: David Miller @ 2007-10-11 23:37 UTC (permalink / raw)
To: peter.p.waskiewicz.jr; +Cc: netdev
In-Reply-To: <D5C1322C3E673F459512FB59E0DDC32903C4407B@orsmsx414.amr.corp.intel.com>
From: "Waskiewicz Jr, Peter P" <peter.p.waskiewicz.jr@intel.com>
Date: Thu, 11 Oct 2007 16:27:14 -0700
> I'm having an issue with TSO right vs. hardware that can't take the
> maximum segment size sent from the stack. I've been told that the
> maximum packet size that can be sent to the hardware today is 64k, but
> my hardware can only take 32k in certain modes per queue due to hardware
> limitations. I have two questions regarding this: 1) where is this
> value set in the TCP code, and 2) Is this something that can be
> configured on the fly? If the answer to 2 is no, I will try and put
> something together to allow this to happen.
The TCP code just builds the maximum possible for the underlying
protocol, be it ipv4 or ipv6. It takes the underlying protocol
maximum packet length, subtracts the amount of header space it
knows will be used, and uses that.
You'll need to use GSO sw segmentation to split the TSO packets
which are too big for your HW to handle.
^ permalink raw reply
* Re: Regression in net-2.6.24?
From: David Miller @ 2007-10-11 23:48 UTC (permalink / raw)
To: takano; +Cc: netdev, ilpo.jarvinen, shemminger, mchan
In-Reply-To: <20071011.225146.94718703.takano@axe-inc.co.jp>
From: TAKANO Ryousei <takano@axe-inc.co.jp>
Date: Thu, 11 Oct 2007 22:51:46 +0900 (JST)
> Modules linked in: 8021q tcp_bic netconsole evdev joydev sg st sr_mod ohci_hcd i2c_amd756 i2c_amd8111 i2c_core ipv6 tg3 usbhid usbcore ff_memless dm_mod ext3 jbd sata_sil libata sd_mod scsi_mod
...
> RIP points at __list_del (net_rx_action -> list_move_tail -> __list_del).
There is a contract between the driver's ->poll() method and
net_rx_action() in that it is assumed that if the entire quota has
been used up, the driver will not perform a netif_rx_complete().
It seems that in a corner case the tg3 driver, which you appear to be
using, will not abide by this rule. That corner case is when the card
has exactly "budget" receive packets pending. In such a case
tg3_has_work() will be false, and we will RX complete when work_done
>= budget, which violates the above mentioned rule.
Can you see if the following test patch makes the crash go away?
Michael, I know you're not pleased with this patch and neither am
I. It might be better to just strictly RX complete when
(work < budget) and if tg3_has_work(), trigger a HW interrupt.
Alternatively we could loop in tg3_poll() until either budget
is exhausted or tg3_has_work() returns false. Actually, this sounds
like a cleaner scheme the more I think about it.
BNX2 likely has a similar issue.
diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index d2b30fb..8c27962 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -3599,7 +3599,7 @@ static int tg3_poll(struct napi_struct *napi, int budget)
sblk->status &= ~SD_STATUS_UPDATED;
/* if no more work, tell net stack and NIC we're done */
- if (!tg3_has_work(tp)) {
+ if ((work_done < budget) && !tg3_has_work(tp)) {
netif_rx_complete(netdev, napi);
tg3_restart_ints(tp);
}
^ permalink raw reply related
* Re: iproute2: resend of patches from Debian.
From: Stephen Hemminger @ 2007-10-11 23:54 UTC (permalink / raw)
To: David Miller; +Cc: andreas, kaber, formorer, netdev
In-Reply-To: <20071011.144843.125887830.davem@davemloft.net>
On Thu, 11 Oct 2007 14:48:43 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:
> From: Andreas Henriksson <andreas@fatal.se>
> Date: Thu, 11 Oct 2007 23:22:12 +0200
>
> > Copying Alexander Wirt here again, as I think this is his patch.
>
> It is important that whoever is the package maintainer for an
> upstream piece of code in any distribution:
>
> 1) Completely understands the patches he is applying.
>
> and therefore:
>
> 2) When he submits upstream, he doesn't have to wake up
> 2,000 patch submitters from the dead just to figure out
> why a change was made.
>
> Neither seems to be occuring here.
Also, I want the correct author name for legal and technical reasons.
--
Stephen Hemminger <shemminger@linux-foundation.org>
^ permalink raw reply
* Re: Question on TSO maximum segment sizes.
From: Rick Jones @ 2007-10-11 23:50 UTC (permalink / raw)
To: David Miller; +Cc: peter.p.waskiewicz.jr, netdev
In-Reply-To: <20071011.163737.36654032.davem@davemloft.net>
David Miller wrote:
> From: "Waskiewicz Jr, Peter P" <peter.p.waskiewicz.jr@intel.com>
> Date: Thu, 11 Oct 2007 16:27:14 -0700
>
>
>>I'm having an issue with TSO right vs. hardware that can't take the
>>maximum segment size sent from the stack. I've been told that the
>>maximum packet size that can be sent to the hardware today is 64k, but
>>my hardware can only take 32k in certain modes per queue due to hardware
>>limitations.
Bletch.
>> I have two questions regarding this: 1) where is this
>>value set in the TCP code, and 2) Is this something that can be
>>configured on the fly? If the answer to 2 is no, I will try and put
>>something together to allow this to happen.
>
>
> The TCP code just builds the maximum possible for the underlying
> protocol, be it ipv4 or ipv6. It takes the underlying protocol
> maximum packet length, subtracts the amount of header space it
> knows will be used, and uses that.
>
> You'll need to use GSO sw segmentation to split the TSO packets
> which are too big for your HW to handle.
For just messing about, might it be possible to tweak the socket buffer sizes
and tcp_tso_win_divisor to kludge things for a short while? Couldn't ship that
way certainly, but assuming Peter's going to get his broken hardware fixed it
might let him limp along until then.
rick jones
^ permalink raw reply
* Re: Regression in net-2.6.24?
From: Stephen Hemminger @ 2007-10-11 23:55 UTC (permalink / raw)
To: David Miller; +Cc: takano, netdev, ilpo.jarvinen, mchan
In-Reply-To: <20071011.164827.64988925.davem@davemloft.net>
On Thu, 11 Oct 2007 16:48:27 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:
> From: TAKANO Ryousei <takano@axe-inc.co.jp>
> Date: Thu, 11 Oct 2007 22:51:46 +0900 (JST)
>
> > Modules linked in: 8021q tcp_bic netconsole evdev joydev sg st sr_mod ohci_hcd i2c_amd756 i2c_amd8111 i2c_core ipv6 tg3 usbhid usbcore ff_memless dm_mod ext3 jbd sata_sil libata sd_mod scsi_mod
> ...
> > RIP points at __list_del (net_rx_action -> list_move_tail -> __list_del).
>
> There is a contract between the driver's ->poll() method and
> net_rx_action() in that it is assumed that if the entire quota has
> been used up, the driver will not perform a netif_rx_complete().
>
> It seems that in a corner case the tg3 driver, which you appear to be
> using, will not abide by this rule. That corner case is when the card
> has exactly "budget" receive packets pending. In such a case
> tg3_has_work() will be false, and we will RX complete when work_done
> >= budget, which violates the above mentioned rule.
>
> Can you see if the following test patch makes the crash go away?
>
> Michael, I know you're not pleased with this patch and neither am
> I. It might be better to just strictly RX complete when
> (work < budget) and if tg3_has_work(), trigger a HW interrupt.
>
> Alternatively we could loop in tg3_poll() until either budget
> is exhausted or tg3_has_work() returns false. Actually, this sounds
> like a cleaner scheme the more I think about it.
>
> BNX2 likely has a similar issue.
sky2 as well.
--
Stephen Hemminger <shemminger@linux-foundation.org>
^ permalink raw reply
* Re: Non-linear SKBs
From: David Miller @ 2007-10-12 0:00 UTC (permalink / raw)
To: kristrev; +Cc: netdev, linux-net
In-Reply-To: <470EA9AD.4020304@ifi.uio.no>
From: Kristian Evensen <kristrev@ifi.uio.no>
Date: Fri, 12 Oct 2007 00:54:37 +0200
> I have developed a small patch for the TCP code in 2.6.19 and it works
> flawlessly. A couple of days ago I decided to make it compatible with
> 2.6.22.5 and have stumbled upon a problem I cannot solve.
>
> In 2.6.19 it seems that all packets (at least the ones my patch work
> with) are linear, while they are non-linear in 2.6.22.5. I have searched
> through the code (focusing on tcp_sendmsg) to try to figure out what
> happens, but can't find any differences that would explain this. Does
> anyone know what might be the cause and if there is an easy way to
> return to linear skbs (unless that is totally stupid)? I would also like
> the benefits offered by the collapsing when retransmitting (which
> requires number of fragments to be 0).
If the underlying device can do scatter-gather and checksumming,
the TCP code builds outgoing packets by copying user date into
full system pages, and then attaching those pages into the SKB.
The protocol headers sit under the skb->data linear area, and
the user data mostly sits in the user pages under
skb_shinfo(skb)->frags[]
This increases the density of data packed into the memory allocated
compared to using skb->data for it. It also enormously simplifies
the logic necessary to support TSO.
^ permalink raw reply
* Re: Question on TSO maximum segment sizes.
From: David Miller @ 2007-10-12 0:02 UTC (permalink / raw)
To: rick.jones2; +Cc: peter.p.waskiewicz.jr, netdev
In-Reply-To: <470EB6D6.9020704@hp.com>
From: Rick Jones <rick.jones2@hp.com>
Date: Thu, 11 Oct 2007 16:50:46 -0700
> For just messing about, might it be possible to tweak the socket buffer sizes
> and tcp_tso_win_divisor to kludge things for a short while? Couldn't ship that
> way certainly, but assuming Peter's going to get his broken hardware fixed it
> might let him limp along until then.
TCP dynamically grows the socket buffer sizes unless the application
explicitly sets them via setsockopt() and the limits imposed in those
cases are controlled by tcp_{,r,w}mem[] sysctls. Decreasing those
will kill performance exactly for the cases this person cares about.
No, the only way to deal with this is to GSO segment incoming frames
inside of the driver when they exceed the HW limits.
^ permalink raw reply
* RE: Question on TSO maximum segment sizes.
From: Waskiewicz Jr, Peter P @ 2007-10-12 0:06 UTC (permalink / raw)
To: David Miller, rick.jones2; +Cc: netdev
In-Reply-To: <20071011.170244.32082446.davem@davemloft.net>
> From: Rick Jones <rick.jones2@hp.com>
> Date: Thu, 11 Oct 2007 16:50:46 -0700
>
> > For just messing about, might it be possible to tweak the socket
> > buffer sizes and tcp_tso_win_divisor to kludge things for a short
> > while? Couldn't ship that way certainly, but assuming
> Peter's going
> > to get his broken hardware fixed it might let him limp
> along until then.
>
> TCP dynamically grows the socket buffer sizes unless the
> application explicitly sets them via setsockopt() and the
> limits imposed in those cases are controlled by
> tcp_{,r,w}mem[] sysctls. Decreasing those will kill
> performance exactly for the cases this person cares about.
>
> No, the only way to deal with this is to GSO segment incoming
> frames inside of the driver when they exceed the HW limits.
Thanks Dave and Rick. I'll mess with this and make my driver happy
again.
Cheers,
-PJ Waskiewicz
^ permalink raw reply
* Re: Regression in net-2.6.24?
From: David Miller @ 2007-10-12 0:17 UTC (permalink / raw)
To: shemminger; +Cc: takano, netdev, ilpo.jarvinen, mchan
In-Reply-To: <20071011165548.6545b207@freepuppy.rosehill>
From: Stephen Hemminger <shemminger@linux-foundation.org>
Date: Thu, 11 Oct 2007 16:55:48 -0700
> On Thu, 11 Oct 2007 16:48:27 -0700 (PDT)
> David Miller <davem@davemloft.net> wrote:
>
> > Alternatively we could loop in tg3_poll() until either budget
> > is exhausted or tg3_has_work() returns false. Actually, this sounds
> > like a cleaner scheme the more I think about it.
> >
> > BNX2 likely has a similar issue.
>
> sky2 as well.
Thanks for the heads up Stephen.
Here is a patch that implements the looping idea in tg3, bnx2, and
sky2.
diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c
index bbfbdaf..b015d52 100644
--- a/drivers/net/bnx2.c
+++ b/drivers/net/bnx2.c
@@ -2633,15 +2633,11 @@ bnx2_has_work(struct bnx2 *bp)
return 0;
}
-static int
-bnx2_poll(struct napi_struct *napi, int budget)
+static int bnx2_poll_work(struct bnx2 *bp, int work_done, int budget)
{
- struct bnx2 *bp = container_of(napi, struct bnx2, napi);
- struct net_device *dev = bp->dev;
struct status_block *sblk = bp->status_blk;
u32 status_attn_bits = sblk->status_attn_bits;
u32 status_attn_bits_ack = sblk->status_attn_bits_ack;
- int work_done = 0;
if ((status_attn_bits & STATUS_ATTN_EVENTS) !=
(status_attn_bits_ack & STATUS_ATTN_EVENTS)) {
@@ -2660,27 +2656,43 @@ bnx2_poll(struct napi_struct *napi, int budget)
bnx2_tx_int(bp);
if (bp->status_blk->status_rx_quick_consumer_index0 != bp->hw_rx_cons)
- work_done = bnx2_rx_int(bp, budget);
+ work_done += bnx2_rx_int(bp, budget - work_done);
- bp->last_status_idx = bp->status_blk->status_idx;
- rmb();
+ return work_done;
+}
+
+static int bnx2_poll(struct napi_struct *napi, int budget)
+{
+ struct bnx2 *bp = container_of(napi, struct bnx2, napi);
+ int work_done = 0;
+
+ while (1) {
+ work_done += bnx2_poll_work(bp, work_done, budget);
- if (!bnx2_has_work(bp)) {
- netif_rx_complete(dev, napi);
- if (likely(bp->flags & USING_MSI_FLAG)) {
+ if (unlikely(work_done >= budget))
+ break;
+
+ if (likely(!bnx2_has_work(bp))) {
+ bp->last_status_idx = bp->status_blk->status_idx;
+ rmb();
+
+ netif_rx_complete(bp->dev, napi);
+ if (likely(bp->flags & USING_MSI_FLAG)) {
+ REG_WR(bp, BNX2_PCICFG_INT_ACK_CMD,
+ BNX2_PCICFG_INT_ACK_CMD_INDEX_VALID |
+ bp->last_status_idx);
+ return 0;
+ }
REG_WR(bp, BNX2_PCICFG_INT_ACK_CMD,
BNX2_PCICFG_INT_ACK_CMD_INDEX_VALID |
+ BNX2_PCICFG_INT_ACK_CMD_MASK_INT |
bp->last_status_idx);
- return 0;
- }
- REG_WR(bp, BNX2_PCICFG_INT_ACK_CMD,
- BNX2_PCICFG_INT_ACK_CMD_INDEX_VALID |
- BNX2_PCICFG_INT_ACK_CMD_MASK_INT |
- bp->last_status_idx);
- REG_WR(bp, BNX2_PCICFG_INT_ACK_CMD,
- BNX2_PCICFG_INT_ACK_CMD_INDEX_VALID |
- bp->last_status_idx);
+ REG_WR(bp, BNX2_PCICFG_INT_ACK_CMD,
+ BNX2_PCICFG_INT_ACK_CMD_INDEX_VALID |
+ bp->last_status_idx);
+ break;
+ }
}
return work_done;
diff --git a/drivers/net/sky2.c b/drivers/net/sky2.c
index fe0e756..25da238 100644
--- a/drivers/net/sky2.c
+++ b/drivers/net/sky2.c
@@ -2605,33 +2605,41 @@ static void sky2_err_intr(struct sky2_hw *hw, u32 status)
static int sky2_poll(struct napi_struct *napi, int work_limit)
{
struct sky2_hw *hw = container_of(napi, struct sky2_hw, napi);
- u32 status = sky2_read32(hw, B0_Y2_SP_EISR);
- int work_done;
+ int work_done = 0;
- if (unlikely(status & Y2_IS_ERROR))
- sky2_err_intr(hw, status);
+ while (1) {
+ u32 status = sky2_read32(hw, B0_Y2_SP_EISR);
- if (status & Y2_IS_IRQ_PHY1)
- sky2_phy_intr(hw, 0);
+ if (unlikely(status & Y2_IS_ERROR))
+ sky2_err_intr(hw, status);
- if (status & Y2_IS_IRQ_PHY2)
- sky2_phy_intr(hw, 1);
+ if (status & Y2_IS_IRQ_PHY1)
+ sky2_phy_intr(hw, 0);
- work_done = sky2_status_intr(hw, work_limit);
+ if (status & Y2_IS_IRQ_PHY2)
+ sky2_phy_intr(hw, 1);
- /* More work? */
- if (hw->st_idx == sky2_read16(hw, STAT_PUT_IDX)) {
- /* Bug/Errata workaround?
- * Need to kick the TX irq moderation timer.
- */
- if (sky2_read8(hw, STAT_TX_TIMER_CTRL) == TIM_START) {
- sky2_write8(hw, STAT_TX_TIMER_CTRL, TIM_STOP);
- sky2_write8(hw, STAT_TX_TIMER_CTRL, TIM_START);
- }
+ work_done += sky2_status_intr(hw, work_limit - work_done);
- napi_complete(napi);
- sky2_read32(hw, B0_Y2_SP_LISR);
+ if (unlikely(work_done >= work_limit))
+ break;
+
+ /* More work? */
+ if (likely(hw->st_idx == sky2_read16(hw, STAT_PUT_IDX))) {
+ /* Bug/Errata workaround?
+ * Need to kick the TX irq moderation timer.
+ */
+ if (sky2_read8(hw, STAT_TX_TIMER_CTRL) == TIM_START) {
+ sky2_write8(hw, STAT_TX_TIMER_CTRL, TIM_STOP);
+ sky2_write8(hw, STAT_TX_TIMER_CTRL, TIM_START);
+ }
+
+ napi_complete(napi);
+ sky2_read32(hw, B0_Y2_SP_LISR);
+ break;
+ }
}
+
return work_done;
}
diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index d2b30fb..ae08dda 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -3555,12 +3555,9 @@ next_pkt_nopost:
return received;
}
-static int tg3_poll(struct napi_struct *napi, int budget)
+static int tg3_poll_work(struct tg3 *tp, int work_done, int budget)
{
- struct tg3 *tp = container_of(napi, struct tg3, napi);
- struct net_device *netdev = tp->dev;
struct tg3_hw_status *sblk = tp->hw_status;
- int work_done = 0;
/* handle link change and other phy events */
if (!(tp->tg3_flags &
@@ -3578,11 +3575,8 @@ static int tg3_poll(struct napi_struct *napi, int budget)
/* run TX completion thread */
if (sblk->idx[0].tx_consumer != tp->tx_cons) {
tg3_tx(tp);
- if (unlikely(tp->tg3_flags & TG3_FLAG_TX_RECOVERY_PENDING)) {
- netif_rx_complete(netdev, napi);
- schedule_work(&tp->reset_task);
+ if (unlikely(tp->tg3_flags & TG3_FLAG_TX_RECOVERY_PENDING))
return 0;
- }
}
/* run RX thread, within the bounds set by NAPI.
@@ -3590,21 +3584,46 @@ static int tg3_poll(struct napi_struct *napi, int budget)
* code synchronizes with tg3->napi.poll()
*/
if (sblk->idx[0].rx_producer != tp->rx_rcb_ptr)
- work_done = tg3_rx(tp, budget);
+ work_done += tg3_rx(tp, budget - work_done);
- if (tp->tg3_flags & TG3_FLAG_TAGGED_STATUS) {
- tp->last_tag = sblk->status_tag;
- rmb();
- } else
- sblk->status &= ~SD_STATUS_UPDATED;
+ return work_done;
+}
- /* if no more work, tell net stack and NIC we're done */
- if (!tg3_has_work(tp)) {
- netif_rx_complete(netdev, napi);
- tg3_restart_ints(tp);
+static int tg3_poll(struct napi_struct *napi, int budget)
+{
+ struct tg3 *tp = container_of(napi, struct tg3, napi);
+ int work_done = 0;
+
+ while (1) {
+ work_done += tg3_poll_work(tp, work_done, budget);
+
+ if (unlikely(tp->tg3_flags & TG3_FLAG_TX_RECOVERY_PENDING))
+ goto tx_recovery;
+
+ if (unlikely(work_done >= budget))
+ break;
+
+ if (likely(!tg3_has_work(tp))) {
+ struct tg3_hw_status *sblk = tp->hw_status;
+
+ if (tp->tg3_flags & TG3_FLAG_TAGGED_STATUS) {
+ tp->last_tag = sblk->status_tag;
+ rmb();
+ } else
+ sblk->status &= ~SD_STATUS_UPDATED;
+
+ netif_rx_complete(tp->dev, napi);
+ tg3_restart_ints(tp);
+ break;
+ }
}
return work_done;
+
+tx_recovery:
+ netif_rx_complete(tp->dev, napi);
+ schedule_work(&tp->reset_task);
+ return 0;
}
static void tg3_irq_quiesce(struct tg3 *tp)
^ permalink raw reply related
* Re: Question on TSO maximum segment sizes.
From: Ben Greear @ 2007-10-12 0:17 UTC (permalink / raw)
To: Waskiewicz Jr, Peter P; +Cc: David Miller, rick.jones2, netdev
In-Reply-To: <D5C1322C3E673F459512FB59E0DDC32903C440EE@orsmsx414.amr.corp.intel.com>
Waskiewicz Jr, Peter P wrote:
>> From: Rick Jones <rick.jones2@hp.com>
>> Date: Thu, 11 Oct 2007 16:50:46 -0700
>>
>>> For just messing about, might it be possible to tweak the socket
>>> buffer sizes and tcp_tso_win_divisor to kludge things for a short
>>> while? Couldn't ship that way certainly, but assuming
>> Peter's going
>>> to get his broken hardware fixed it might let him limp
>> along until then.
>>
>> TCP dynamically grows the socket buffer sizes unless the
>> application explicitly sets them via setsockopt() and the
>> limits imposed in those cases are controlled by
>> tcp_{,r,w}mem[] sysctls. Decreasing those will kill
>> performance exactly for the cases this person cares about.
I just tried turning off my explicit SO_SNDBUF/SO_RCVBUG settings in my app,
and the connection ran very poorly through a link with even a small
bit of latency (~2-4ms I believe).
It ran near gige line speed through a cross-over cable.
I have the sysctl max values set very generous, though the min
and default are fairly small.
This was with kernel 2.6.20.
Was the auto-tuning put in after 2.6.20? If not, has this
been tested through a higher latency link? Or, am I confused
and you are talking about some other setsockopt?
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: Question on TSO maximum segment sizes.
From: David Miller @ 2007-10-12 0:22 UTC (permalink / raw)
To: greearb; +Cc: peter.p.waskiewicz.jr, rick.jones2, netdev
In-Reply-To: <470EBD2C.8020704@candelatech.com>
From: Ben Greear <greearb@candelatech.com>
Date: Thu, 11 Oct 2007 17:17:48 -0700
> Was the auto-tuning put in after 2.6.20? If not, has this
> been tested through a higher latency link? Or, am I confused
> and you are talking about some other setsockopt?
It's been there for quite some time.
^ permalink raw reply
* Re: [PATCH] rtnl: Simplify ASSERT_RTNL
From: David Miller @ 2007-10-12 0:30 UTC (permalink / raw)
To: ebiederm; +Cc: herbert, netdev, kaber
In-Reply-To: <m1tzoxehlo.fsf@ebiederm.dsl.xmission.com>
From: ebiederm@xmission.com (Eric W. Biederman)
Date: Thu, 11 Oct 2007 10:33:39 -0600
> Having ASSERT_RTNL warn if you were sleeping does not seem
> intuitive from the name.
>
> This instance of convoluted locking seems like a complete
> one off to me, and if it will warn about other constructs
> currently in the kernel it seems wrong.
RTNL is a semaphore, therefore it sleeps.
Therefore anything that requires RTNL is held can also assume that it
can do things like GFP_KERNEL allocations and other sleeping actions.
This is why any code path that runs with spinlocks held or interrupts
disabled, and hits an RTNL assertion, is buggy. It is the chain of
dependencies of what is allowed in such contexts.
ASSERT_RTNL();
page = alloc_page(GFP_KERNEL);
That sequence above is absolutely always legal.
The might_sleep() check is just letting us know the problem exists.
If spinlocks or interrupt disabling is needed to implement something
deeper in the call chain, that's fine, it just cannot call back into
the RTNL asserted domain with those spinlocks held or interrupts
disabled.
If the mac_vlan driver, or whichever one has the problem, does things
like this it must be fixed and putting or not putting a proper
might_sleep() check here doesn't change that.
^ permalink raw reply
* Re: [PATCH 1/7] [TCP]: Add bytes_acked (ABC) clearing to FRTO too
From: David Miller @ 2007-10-12 0:32 UTC (permalink / raw)
To: ilpo.jarvinen; +Cc: netdev, takano
In-Reply-To: <11921028673599-git-send-email-ilpo.jarvinen@helsinki.fi>
From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi>
Date: Thu, 11 Oct 2007 14:41:01 +0300
> I was reading tcp_enter_loss while looking for Cedric's bug and
> noticed bytes_acked adjustment is missing from FRTO side.
>
> Since bytes_acked will only be used in tcp_cong_avoid, I think
> it's safe to assume RTO would be spurious. During FRTO cwnd
> will be not controlled by tcp_cong_avoid and if FRTO calls for
> conventional recovery, cwnd is adjusted and the result of wrong
> assumption is cleared from bytes_acked. If RTO was in fact
> spurious, we did normal ABC already and can continue without
> any additional adjustments.
>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
Applied.
^ permalink raw reply
* Re: Regression in net-2.6.24?
From: Stephen Hemminger @ 2007-10-12 0:31 UTC (permalink / raw)
To: David Miller; +Cc: takano, netdev, ilpo.jarvinen, mchan
In-Reply-To: <20071011.171743.118962219.davem@davemloft.net>
On Thu, 11 Oct 2007 17:17:43 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:
> From: Stephen Hemminger <shemminger@linux-foundation.org>
> Date: Thu, 11 Oct 2007 16:55:48 -0700
>
> > On Thu, 11 Oct 2007 16:48:27 -0700 (PDT)
> > David Miller <davem@davemloft.net> wrote:
> >
> > > Alternatively we could loop in tg3_poll() until either budget
> > > is exhausted or tg3_has_work() returns false. Actually, this sounds
> > > like a cleaner scheme the more I think about it.
> > >
> > > BNX2 likely has a similar issue.
> >
> > sky2 as well.
>
> Thanks for the heads up Stephen.
>
> Here is a patch that implements the looping idea in tg3, bnx2, and
> sky2.
>
> diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c
> index bbfbdaf..b015d52 100644
> --- a/drivers/net/bnx2.c
> +++ b/drivers/net/bnx2.c
> @@ -2633,15 +2633,11 @@ bnx2_has_work(struct bnx2 *bp)
> return 0;
> }
>
> -static int
> -bnx2_poll(struct napi_struct *napi, int budget)
> +static int bnx2_poll_work(struct bnx2 *bp, int work_done, int budget)
> {
> - struct bnx2 *bp = container_of(napi, struct bnx2, napi);
> - struct net_device *dev = bp->dev;
> struct status_block *sblk = bp->status_blk;
> u32 status_attn_bits = sblk->status_attn_bits;
> u32 status_attn_bits_ack = sblk->status_attn_bits_ack;
> - int work_done = 0;
>
> if ((status_attn_bits & STATUS_ATTN_EVENTS) !=
> (status_attn_bits_ack & STATUS_ATTN_EVENTS)) {
> @@ -2660,27 +2656,43 @@ bnx2_poll(struct napi_struct *napi, int budget)
> bnx2_tx_int(bp);
>
> if (bp->status_blk->status_rx_quick_consumer_index0 != bp->hw_rx_cons)
> - work_done = bnx2_rx_int(bp, budget);
> + work_done += bnx2_rx_int(bp, budget - work_done);
>
> - bp->last_status_idx = bp->status_blk->status_idx;
> - rmb();
> + return work_done;
> +}
> +
> +static int bnx2_poll(struct napi_struct *napi, int budget)
> +{
> + struct bnx2 *bp = container_of(napi, struct bnx2, napi);
> + int work_done = 0;
> +
> + while (1) {
> + work_done += bnx2_poll_work(bp, work_done, budget);
>
> - if (!bnx2_has_work(bp)) {
> - netif_rx_complete(dev, napi);
> - if (likely(bp->flags & USING_MSI_FLAG)) {
> + if (unlikely(work_done >= budget))
> + break;
> +
> + if (likely(!bnx2_has_work(bp))) {
> + bp->last_status_idx = bp->status_blk->status_idx;
> + rmb();
> +
> + netif_rx_complete(bp->dev, napi);
> + if (likely(bp->flags & USING_MSI_FLAG)) {
> + REG_WR(bp, BNX2_PCICFG_INT_ACK_CMD,
> + BNX2_PCICFG_INT_ACK_CMD_INDEX_VALID |
> + bp->last_status_idx);
> + return 0;
> + }
> REG_WR(bp, BNX2_PCICFG_INT_ACK_CMD,
> BNX2_PCICFG_INT_ACK_CMD_INDEX_VALID |
> + BNX2_PCICFG_INT_ACK_CMD_MASK_INT |
> bp->last_status_idx);
> - return 0;
> - }
> - REG_WR(bp, BNX2_PCICFG_INT_ACK_CMD,
> - BNX2_PCICFG_INT_ACK_CMD_INDEX_VALID |
> - BNX2_PCICFG_INT_ACK_CMD_MASK_INT |
> - bp->last_status_idx);
>
> - REG_WR(bp, BNX2_PCICFG_INT_ACK_CMD,
> - BNX2_PCICFG_INT_ACK_CMD_INDEX_VALID |
> - bp->last_status_idx);
> + REG_WR(bp, BNX2_PCICFG_INT_ACK_CMD,
> + BNX2_PCICFG_INT_ACK_CMD_INDEX_VALID |
> + bp->last_status_idx);
> + break;
> + }
> }
>
> return work_done;
> diff --git a/drivers/net/sky2.c b/drivers/net/sky2.c
> index fe0e756..25da238 100644
> --- a/drivers/net/sky2.c
> +++ b/drivers/net/sky2.c
> @@ -2605,33 +2605,41 @@ static void sky2_err_intr(struct sky2_hw *hw, u32 status)
> static int sky2_poll(struct napi_struct *napi, int work_limit)
> {
> struct sky2_hw *hw = container_of(napi, struct sky2_hw, napi);
> - u32 status = sky2_read32(hw, B0_Y2_SP_EISR);
> - int work_done;
> + int work_done = 0;
>
> - if (unlikely(status & Y2_IS_ERROR))
> - sky2_err_intr(hw, status);
> + while (1) {
> + u32 status = sky2_read32(hw, B0_Y2_SP_EISR);
>
> - if (status & Y2_IS_IRQ_PHY1)
> - sky2_phy_intr(hw, 0);
> + if (unlikely(status & Y2_IS_ERROR))
> + sky2_err_intr(hw, status);
>
> - if (status & Y2_IS_IRQ_PHY2)
> - sky2_phy_intr(hw, 1);
> + if (status & Y2_IS_IRQ_PHY1)
> + sky2_phy_intr(hw, 0);
>
> - work_done = sky2_status_intr(hw, work_limit);
> + if (status & Y2_IS_IRQ_PHY2)
> + sky2_phy_intr(hw, 1);
>
> - /* More work? */
> - if (hw->st_idx == sky2_read16(hw, STAT_PUT_IDX)) {
> - /* Bug/Errata workaround?
> - * Need to kick the TX irq moderation timer.
> - */
> - if (sky2_read8(hw, STAT_TX_TIMER_CTRL) == TIM_START) {
> - sky2_write8(hw, STAT_TX_TIMER_CTRL, TIM_STOP);
> - sky2_write8(hw, STAT_TX_TIMER_CTRL, TIM_START);
> - }
> + work_done += sky2_status_intr(hw, work_limit - work_done);
>
> - napi_complete(napi);
> - sky2_read32(hw, B0_Y2_SP_LISR);
> + if (unlikely(work_done >= work_limit))
> + break;
> +
> + /* More work? */
> + if (likely(hw->st_idx == sky2_read16(hw, STAT_PUT_IDX))) {
> + /* Bug/Errata workaround?
> + * Need to kick the TX irq moderation timer.
> + */
> + if (sky2_read8(hw, STAT_TX_TIMER_CTRL) == TIM_START) {
> + sky2_write8(hw, STAT_TX_TIMER_CTRL, TIM_STOP);
> + sky2_write8(hw, STAT_TX_TIMER_CTRL, TIM_START);
> + }
> +
> + napi_complete(napi);
> + sky2_read32(hw, B0_Y2_SP_LISR);
> + break;
> + }
> }
> +
> return work_done;
> }
You don't need to re-read the status register and process the PHY irq's inside loop.
Try this:
--- a/drivers/net/sky2.c 2007-10-11 13:16:15.000000000 -0700
+++ b/drivers/net/sky2.c 2007-10-11 17:30:29.000000000 -0700
@@ -2606,7 +2606,7 @@ static int sky2_poll(struct napi_struct
{
struct sky2_hw *hw = container_of(napi, struct sky2_hw, napi);
u32 status = sky2_read32(hw, B0_Y2_SP_EISR);
- int work_done;
+ int work_done = 0;
if (unlikely(status & Y2_IS_ERROR))
sky2_err_intr(hw, status);
@@ -2617,10 +2617,16 @@ static int sky2_poll(struct napi_struct
if (status & Y2_IS_IRQ_PHY2)
sky2_phy_intr(hw, 1);
- work_done = sky2_status_intr(hw, work_limit);
+ for(;;) {
+ work_done += sky2_status_intr(hw, work_limit);
+
+ if (work_done >= work_limit)
+ break;
+
+ /* More work? */
+ if (hw->st_idx != sky2_read16(hw, STAT_PUT_IDX))
+ continue;
- /* More work? */
- if (hw->st_idx == sky2_read16(hw, STAT_PUT_IDX)) {
/* Bug/Errata workaround?
* Need to kick the TX irq moderation timer.
*/
@@ -2628,10 +2634,13 @@ static int sky2_poll(struct napi_struct
sky2_write8(hw, STAT_TX_TIMER_CTRL, TIM_STOP);
sky2_write8(hw, STAT_TX_TIMER_CTRL, TIM_START);
}
-
+
napi_complete(napi);
sky2_read32(hw, B0_Y2_SP_LISR);
+ break;
+
}
+
return work_done;
}
^ 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