* [PATCH 2/3] sky2: Allocate initial skbs in sky2_alloc_buffers
From: Mike McCormack @ 2010-01-27 15:05 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
Allocating everything in one place means there's a single point
of failure in sky2_up, and sky2_rx_start can no longer fail.
This also fixes a memory leak in the case that sky2_rx_start
fails in the middle of allocating skbs, since any allocated
skbs will not be free'd in sky2_up's failure path.
Signed-off-by: Mike McCormack <mikem@ring3k.org>
---
drivers/net/sky2.c | 42 +++++++++++++++++++++++-------------------
1 files changed, 23 insertions(+), 19 deletions(-)
diff --git a/drivers/net/sky2.c b/drivers/net/sky2.c
index 2061eb8..a967912 100644
--- a/drivers/net/sky2.c
+++ b/drivers/net/sky2.c
@@ -1358,7 +1358,7 @@ static inline void sky2_rx_update(struct sky2_port *sky2, unsigned rxq)
}
/*
- * Allocate and setup receiver buffer pool.
+ * Setup receiver buffer pool.
* Normal case this ends up creating one list element for skb
* in the receive ring. Worst case if using large MTU and each
* allocation falls on a different 64 bit region, that results
@@ -1392,22 +1392,10 @@ static int sky2_rx_start(struct sky2_port *sky2)
if (!(hw->flags & SKY2_HW_NEW_LE))
rx_set_checksum(sky2);
- sky2->rx_data_size = sky2_get_rx_data_size(sky2);
- /* Fill Rx ring */
+ /* submit Rx ring */
for (i = 0; i < sky2->rx_pending; i++) {
re = sky2->rx_ring + i;
-
- re->skb = sky2_rx_alloc(sky2);
- if (!re->skb)
- goto nomem;
-
- if (sky2_rx_map_skb(hw->pdev, re, sky2->rx_data_size)) {
- dev_kfree_skb(re->skb);
- re->skb = NULL;
- goto nomem;
- }
-
sky2_rx_submit(sky2, re);
}
@@ -1453,14 +1441,12 @@ static int sky2_rx_start(struct sky2_port *sky2)
return 0;
-nomem:
- sky2_rx_clean(sky2);
- return -ENOMEM;
}
static int sky2_alloc_buffers(struct sky2_port *sky2)
{
struct sky2_hw *hw = sky2->hw;
+ unsigned i;
/* must be power of 2 */
sky2->tx_le = pci_alloc_consistent(hw->pdev,
@@ -1486,6 +1472,24 @@ static int sky2_alloc_buffers(struct sky2_port *sky2)
if (!sky2->rx_ring)
goto nomem;
+ sky2->rx_data_size = sky2_get_rx_data_size(sky2);
+
+ /* Fill Rx ring */
+ for (i = 0; i < sky2->rx_pending; i++) {
+ struct rx_ring_info *re = sky2->rx_ring + i;
+
+ re->skb = sky2_rx_alloc(sky2);
+ if (!re->skb)
+ goto nomem;
+
+ if (sky2_rx_map_skb(hw->pdev, re, sky2->rx_data_size)) {
+ dev_kfree_skb(re->skb);
+ re->skb = NULL;
+ goto nomem;
+ }
+ }
+
+
return 0;
nomem:
return -ENOMEM;
@@ -1495,6 +1499,8 @@ static void sky2_free_buffers(struct sky2_port *sky2)
{
struct sky2_hw *hw = sky2->hw;
+ sky2_rx_clean(sky2);
+
if (sky2->rx_le) {
pci_free_consistent(hw->pdev, RX_LE_BYTES,
sky2->rx_le, sky2->rx_le_map);
@@ -1953,8 +1959,6 @@ static int sky2_down(struct net_device *dev)
/* Free any pending frames stuck in HW queue */
sky2_tx_complete(sky2, sky2->tx_prod);
- sky2_rx_clean(sky2);
-
sky2_free_buffers(sky2);
return 0;
--
1.5.6.5
^ permalink raw reply related
* [PATCH 3/3] sky2: Remove failure cases for sky2_rx_start
From: Mike McCormack @ 2010-01-27 15:05 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
sky2_rx_start() can no longer fail, so remove redundant code pathes.
Signed-off-by: Mike McCormack <mikem@ring3k.org>
---
drivers/net/sky2.c | 24 ++++++------------------
1 files changed, 6 insertions(+), 18 deletions(-)
diff --git a/drivers/net/sky2.c b/drivers/net/sky2.c
index a967912..6ea660f 100644
--- a/drivers/net/sky2.c
+++ b/drivers/net/sky2.c
@@ -1366,7 +1366,7 @@ static inline void sky2_rx_update(struct sky2_port *sky2, unsigned rxq)
* One element is used for checksum enable/disable, and one
* extra to avoid wrap.
*/
-static int sky2_rx_start(struct sky2_port *sky2)
+static void sky2_rx_start(struct sky2_port *sky2)
{
struct sky2_hw *hw = sky2->hw;
struct rx_ring_info *re;
@@ -1392,7 +1392,6 @@ static int sky2_rx_start(struct sky2_port *sky2)
if (!(hw->flags & SKY2_HW_NEW_LE))
rx_set_checksum(sky2);
-
/* submit Rx ring */
for (i = 0; i < sky2->rx_pending; i++) {
re = sky2->rx_ring + i;
@@ -1437,10 +1436,6 @@ static int sky2_rx_start(struct sky2_port *sky2)
sky2_write32(hw, Q_ADDR(txqaddr[sky2->port], Q_TEST),
TBMU_TEST_HOME_ADD_FIX_EN | TBMU_TEST_ROUTING_ADD_FIX_EN);
}
-
-
-
- return 0;
}
static int sky2_alloc_buffers(struct sky2_port *sky2)
@@ -1590,9 +1585,7 @@ static int sky2_up(struct net_device *dev)
sky2_set_vlan_mode(hw, port, sky2->vlgrp != NULL);
#endif
- err = sky2_rx_start(sky2);
- if (err)
- goto err_out;
+ sky2_rx_start(sky2);
/* Enable interrupts from phy/mac for port */
imask = sky2_read32(hw, B0_IMSK);
@@ -2200,7 +2193,6 @@ static int sky2_change_mtu(struct net_device *dev, int new_mtu)
struct sky2_port *sky2 = netdev_priv(dev);
struct sky2_hw *hw = sky2->hw;
unsigned port = sky2->port;
- int err;
u16 ctl, mode;
u32 imask;
@@ -2246,21 +2238,17 @@ static int sky2_change_mtu(struct net_device *dev, int new_mtu)
sky2_write8(hw, RB_ADDR(rxqaddr[port], RB_CTRL), RB_ENA_OP_MD);
- err = sky2_rx_start(sky2);
+ sky2_rx_start(sky2);
sky2_write32(hw, B0_IMSK, imask);
sky2_read32(hw, B0_Y2_SP_LISR);
napi_enable(&hw->napi);
- if (err)
- dev_close(dev);
- else {
- gma_write16(hw, port, GM_GP_CTRL, ctl);
+ gma_write16(hw, port, GM_GP_CTRL, ctl);
- netif_wake_queue(dev);
- }
+ netif_wake_queue(dev);
- return err;
+ return 0;
}
/* For small just reuse existing skb for next receive */
--
1.5.6.5
^ permalink raw reply related
* Re: [PATCH] tcp: fix ICMP-RTO war
From: Alexey Kuznetsov @ 2010-01-27 14:14 UTC (permalink / raw)
To: Ilpo J?rvinen; +Cc: David Miller, damian, denys, Netdev
In-Reply-To: <alpine.DEB.2.00.1001260957540.17103@melkinpaasi.cs.helsinki.fi>
Hello!
> I went through some history, it seems that this comment about the lower
> bound originates from Alexey [1]:
Yes, I even remember why it was done.
Actual comment starts with "If rtt variance happened to be less 50msec...".
It was observed that sometimes due to high burstiness of ACKs
(found originally when linux commuinitected to solaris tcp stack,
which issued huge burst of acks each 50msec. I think he data can be found
in netdev archives), rtt deviation tends to collapse to 0 during such burst,
rto was calculated as plain rtt and obviously we falled to retransmit.
So, the algorithm was repaired. First, to avoid glitches in the future
mdev was clamped to 50ms (rttvar, contribution to rto, is 200msec).
That's what the comment is about.
Second, Linux used to sample rtt each ack, not each rtt (which is technically
main source of the problem). Instead of decreasing sampling rate
I used different sampling rates for growing and dropping rtt:
it grows with rate of arriving acks and drops each rtt i.e. rto grows
quickly, but drops slowly.
Alexey
^ permalink raw reply
* Re: 0% cpu usasge after fresh boot or net restart but 10% CPU if kernel flush route cache
From: Eric Dumazet @ 2010-01-27 15:26 UTC (permalink / raw)
To: cold cold; +Cc: netdev
In-Reply-To: <41ac0f9e1001260858o7d2a6a6dgb37ecfba5c325932@mail.gmail.com>
Le mardi 26 janvier 2010 à 18:58 +0200, cold cold a écrit :
> HI,
>
>
> i have expiriance some CPU usage spikes up to 10% on each four cpus
> after the first kernel route cache flush.
> After mashine start first 20 min CPU is 0%si 300Mbits/s full duplex
> and arount 100k pps forwarded traffic, without any firewall, just
> plain routing.
>
> route -n |wc -l
> 34
>
> ip route show cache | wc -l
> 2140842
>
> cat /proc/sys/net/ipv4/route/secret_interval
> 600
> cat /proc/sys/net/ipv4/route/max_size
> 33554432
>
> after kernel flush i got 10% on all CPUs for 5-6 mins. It's not from
> rebuilding route cashe becouse after
> fresh boot or network restrat there is no CPU usage until kernel flush
> route cache.
> I try to play with rhash_entries= 300000 to 2000000 same result.
If you have one million dst entries to flush, it takes some time.
You could try to not increase the rhash_entries
(or keep it low, say 131072)
but tune /proc/sys/net/ipv4/route settings.
Try to reduce gc_elasticity from 8 to 2
Try to reduce gc_interval from 60 to 1
Important thing to consider is to irq affinities (so that one cpu
handles network interrupts, to minimize cache ping poings )
^ permalink raw reply
* Re: Hang: 2.6.32.4 sky2/DMAR (was [PATCH] sky2: Fix WARNING: at lib/dma-debug.c:902 check_sync)
From: Michael Breuer @ 2010-01-27 15:34 UTC (permalink / raw)
To: Jarek Poplawski
Cc: David Miller, Stephen Hemminger, akpm, flyboy, linux-kernel,
netdev, Michael Chan, Don Fry, Francois Romieu, Matt Carlson
In-Reply-To: <20100123232133.GA3487@del.dom.local>
On 01/23/2010 06:21 PM, Jarek Poplawski wrote:
> On Fri, Jan 22, 2010 at 06:50:21PM -0500, Michael Breuer wrote:
>
>> When the packets were dropped, there was a different sequence in the
>> log - DISCOVER/OFFER repeated. The "normal" is that the sequence
>> appeared correct and complete - DISCOVER/OFFER/REQUEST/ACK - or
>> INFORM/ACK (vs. INFORM repeatedly sans ACK) as the case may be.
>>
> Anyway, I'd be intersted if the switch matters here.
>
> Plus one more test: could you try to load sky2 with the parameter:
> "copybreak=1" (the rest as in any recent test, which gave you dmar
> errors; any switch).
>
> Thanks,
> Jarek P.
>
Ok - now up 80+ hours with copybreak=1. I'm going to redo w/o copybreak
to confirm that I haven't inadvertently fixed something. However, given
that it might be copybreak-related, I looked at sky2.c again and I'm
wondering about the copybreak max size in sky2_rx_start:
size = roundup(sky2->netdev->mtu + ETH_HLEN + VLAN_HLEN, 8);
/* Stopping point for hardware truncation */
thresh = (size - 8) / sizeof(u32);
sky2->rx_nfrags = size >> PAGE_SHIFT;
BUG_ON(sky2->rx_nfrags > ARRAY_SIZE(re->frag_addr));
/* Compute residue after pages */
size -= sky2->rx_nfrags << PAGE_SHIFT;
/* Optimize to handle small packets and headers */
if (size < copybreak)
size = copybreak;
if (size < ETH_HLEN)
size = ETH_HLEN;
Why would increasing size to copybreak be valid here?
Guessing a bit as I'm not sure about rx_nfrags, but if I read this
correctly, if size is ever less than copybreak it's because there isn't
enough space left for anything larger. If so, wouldn't increasing size
potentially corrupt something? I'd further guess that the resulting
condition manifests sooner (or at least with a more visible effect) when
using DMAR.
In any event, why "copybreak" as the minimum buffer size? I'd suggest
that if it isn't possible to allocate at least MTU + overhead that
sky2_rx_start ought to be delayed until there is room.
^ permalink raw reply
* [PATCH 2.6.33-rc5] drivers/net: ks8851_mll ethernet network driver
From: Choi, David @ 2010-01-27 16:03 UTC (permalink / raw)
To: davem; +Cc: netdev
Hello David Miller,
I fix a bug in ks8851_mll driver, which has existed since 2.6.32-rc6.
>From : David J. Choi <david.choi@micrel.com>
Fix a bug that the data pointers in the interrupt handler are set wrong, which is related with the 5th parameter of request_irq().
Signed-off-by : David J. Choi <david.choi@micrel.com>
---
--- linux-2.6.33-rc5/drivers/net/ks8851_mll.c.orig 2010-01-26 17:36:51.000000000 -0800
+++ linux-2.6.33-rc5/drivers/net/ks8851_mll.c 2010-01-26 17:34:34.000000000 -0800
@@ -854,8 +854,8 @@ static void ks_update_link_status(struct
static irqreturn_t ks_irq(int irq, void *pw)
{
- struct ks_net *ks = pw;
- struct net_device *netdev = ks->netdev;
+ struct net_device *netdev = pw;
+ struct ks_net *ks = netdev_priv(netdev);
u16 status;
/*this should be the first in IRQ handler */
---
^ permalink raw reply
* Re: Network QoS support in applications
From: Olaf van der Spek @ 2010-01-27 16:18 UTC (permalink / raw)
To: Kalle Valo; +Cc: netdev, linux-wireless
In-Reply-To: <87k4v5nuej.fsf@purkki.valot.fi>
On Tue, Jan 26, 2010 at 9:27 AM, Kalle Valo <kalle.valo@iki.fi> wrote:
> I would like to clear up all this by and I'm willing to write a
> document for application developers about network QoS. But I need help
> to understand what's the proper way to mark different QoS
> prioritities.
Maybe it shouldn't be done by applications, unless the streams of an
app use different priorities.
It might be useful to look at configuration of networked apps in a
broader sense, including stuff like to what IP addresses and ports an
app should bind. Maybe even to what Unix sockets or other transports.
Being able to configure this in one central place is way easier then
having to dive into the conf file of each individual application.
In this central place the QoS stuff could be configured too.
As this doesn't require application support, it's probably easier to
implement. It also provides more flexibility and maybe even more
security.
Olaf
^ permalink raw reply
* Re: [PATCH 2.6.33-rc5] drivers/net: ks8851_mll ethernet network driver
From: Ben Hutchings @ 2010-01-27 16:28 UTC (permalink / raw)
To: davem; +Cc: Choi, David, netdev
In-Reply-To: <C43529A246480145B0A6D0234BDB0F0D02129C@MELANITE.micrel.com>
On Wed, 2010-01-27 at 08:03 -0800, Choi, David wrote:
> Hello David Miller,
>
> I fix a bug in ks8851_mll driver, which has existed since 2.6.32-rc6.
>
> From : David J. Choi <david.choi@micrel.com>
>
> Fix a bug that the data pointers in the interrupt handler are set wrong, which is related with the 5th parameter of request_irq().
>
> Signed-off-by : David J. Choi <david.choi@micrel.com>
This should go to stable as well since the driver will be useless
without this.
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: Hang: 2.6.32.4 sky2/DMAR (was [PATCH] sky2: Fix WARNING: at lib/dma-debug.c:902 check_sync)
From: Stephen Hemminger @ 2010-01-27 16:50 UTC (permalink / raw)
To: Michael Breuer
Cc: Jarek Poplawski, David Miller, akpm, flyboy, linux-kernel, netdev,
Michael Chan, Don Fry, Francois Romieu, Matt Carlson
In-Reply-To: <4B605D1B.60402@majjas.com>
On Wed, 27 Jan 2010 10:34:51 -0500
Michael Breuer <mbreuer@majjas.com> wrote:
> On 01/23/2010 06:21 PM, Jarek Poplawski wrote:
> > On Fri, Jan 22, 2010 at 06:50:21PM -0500, Michael Breuer wrote:
> >
> >> When the packets were dropped, there was a different sequence in the
> >> log - DISCOVER/OFFER repeated. The "normal" is that the sequence
> >> appeared correct and complete - DISCOVER/OFFER/REQUEST/ACK - or
> >> INFORM/ACK (vs. INFORM repeatedly sans ACK) as the case may be.
> >>
> > Anyway, I'd be intersted if the switch matters here.
> >
> > Plus one more test: could you try to load sky2 with the parameter:
> > "copybreak=1" (the rest as in any recent test, which gave you dmar
> > errors; any switch).
> >
> > Thanks,
> > Jarek P.
> >
> Ok - now up 80+ hours with copybreak=1. I'm going to redo w/o copybreak
> to confirm that I haven't inadvertently fixed something. However, given
> that it might be copybreak-related, I looked at sky2.c again and I'm
> wondering about the copybreak max size in sky2_rx_start:
>
> size = roundup(sky2->netdev->mtu + ETH_HLEN + VLAN_HLEN, 8);
>
> /* Stopping point for hardware truncation */
> thresh = (size - 8) / sizeof(u32);
>
> sky2->rx_nfrags = size >> PAGE_SHIFT;
> BUG_ON(sky2->rx_nfrags > ARRAY_SIZE(re->frag_addr));
>
> /* Compute residue after pages */
> size -= sky2->rx_nfrags << PAGE_SHIFT;
>
> /* Optimize to handle small packets and headers */
> if (size < copybreak)
> size = copybreak;
> if (size < ETH_HLEN)
> size = ETH_HLEN;
>
>
> Why would increasing size to copybreak be valid here?
>
> Guessing a bit as I'm not sure about rx_nfrags, but if I read this
> correctly, if size is ever less than copybreak it's because there isn't
> enough space left for anything larger. If so, wouldn't increasing size
> potentially corrupt something? I'd further guess that the resulting
> condition manifests sooner (or at least with a more visible effect) when
> using DMAR.
>
> In any event, why "copybreak" as the minimum buffer size? I'd suggest
> that if it isn't possible to allocate at least MTU + overhead that
> sky2_rx_start ought to be delayed until there is room.
This code is where driver decides how much data will be received in skb
data area and the remaining data spills over into skb frags.
Copybreak is the threshold so that packets less than size are copied
to a new skb. The code doing the copying there assumes the data is
totally contained in the skb (not in frags). The size increase there
is to make sure that assumption is always true. I suppose you
could do something perverse like setting copybreak really huge
and confuse driver, but that is a user error.
^ permalink raw reply
* Re: Hang: 2.6.32.4 sky2/DMAR (was [PATCH] sky2: Fix WARNING: at lib/dma-debug.c:902 check_sync)
From: Michael Breuer @ 2010-01-27 16:57 UTC (permalink / raw)
To: Stephen Hemminger
Cc: Jarek Poplawski, David Miller, akpm, flyboy, linux-kernel, netdev,
Michael Chan, Don Fry, Francois Romieu, Matt Carlson
In-Reply-To: <20100127085049.5b5048e9@nehalam>
On 1/27/2010 11:50 AM, Stephen Hemminger wrote:
> On Wed, 27 Jan 2010 10:34:51 -0500
> Michael Breuer<mbreuer@majjas.com> wrote:
>
>
>> On 01/23/2010 06:21 PM, Jarek Poplawski wrote:
>>
>>> On Fri, Jan 22, 2010 at 06:50:21PM -0500, Michael Breuer wrote:
>>>
>>>
>>>> When the packets were dropped, there was a different sequence in the
>>>> log - DISCOVER/OFFER repeated. The "normal" is that the sequence
>>>> appeared correct and complete - DISCOVER/OFFER/REQUEST/ACK - or
>>>> INFORM/ACK (vs. INFORM repeatedly sans ACK) as the case may be.
>>>>
>>>>
>>> Anyway, I'd be intersted if the switch matters here.
>>>
>>> Plus one more test: could you try to load sky2 with the parameter:
>>> "copybreak=1" (the rest as in any recent test, which gave you dmar
>>> errors; any switch).
>>>
>>> Thanks,
>>> Jarek P.
>>>
>>>
>> Ok - now up 80+ hours with copybreak=1. I'm going to redo w/o copybreak
>> to confirm that I haven't inadvertently fixed something. However, given
>> that it might be copybreak-related, I looked at sky2.c again and I'm
>> wondering about the copybreak max size in sky2_rx_start:
>>
>> size = roundup(sky2->netdev->mtu + ETH_HLEN + VLAN_HLEN, 8);
>>
>> /* Stopping point for hardware truncation */
>> thresh = (size - 8) / sizeof(u32);
>>
>> sky2->rx_nfrags = size>> PAGE_SHIFT;
>> BUG_ON(sky2->rx_nfrags> ARRAY_SIZE(re->frag_addr));
>>
>> /* Compute residue after pages */
>> size -= sky2->rx_nfrags<< PAGE_SHIFT;
>>
>> /* Optimize to handle small packets and headers */
>> if (size< copybreak)
>> size = copybreak;
>> if (size< ETH_HLEN)
>> size = ETH_HLEN;
>>
>>
>> Why would increasing size to copybreak be valid here?
>>
>> Guessing a bit as I'm not sure about rx_nfrags, but if I read this
>> correctly, if size is ever less than copybreak it's because there isn't
>> enough space left for anything larger. If so, wouldn't increasing size
>> potentially corrupt something? I'd further guess that the resulting
>> condition manifests sooner (or at least with a more visible effect) when
>> using DMAR.
>>
>> In any event, why "copybreak" as the minimum buffer size? I'd suggest
>> that if it isn't possible to allocate at least MTU + overhead that
>> sky2_rx_start ought to be delayed until there is room.
>>
> This code is where driver decides how much data will be received in skb
> data area and the remaining data spills over into skb frags.
> Copybreak is the threshold so that packets less than size are copied
> to a new skb. The code doing the copying there assumes the data is
> totally contained in the skb (not in frags). The size increase there
> is to make sure that assumption is always true. I suppose you
> could do something perverse like setting copybreak really huge
> and confuse driver, but that is a user error.
>
>
Ok - but I'm wondering under what circumstances size would be <
copybreak in the first place after computing the residue. If size ends
up being unreasonably small, is simply increasing the number to whatever
copybreak is correct? Assuming my testing is correct, then the crash
I've been experiencing when using dmar (only) seems related to the value
of copybreak. I don't think the other use (skb reuse) is the issue (but
hey, I could have missed something). The crash occurs when copybreak is
the default of 128, didn't happen when I set copybreak to 1.
^ permalink raw reply
* Re: [PATCH 2/3] sky2: Allocate initial skbs in sky2_alloc_buffers
From: Stephen Hemminger @ 2010-01-27 17:00 UTC (permalink / raw)
To: Mike McCormack; +Cc: netdev
In-Reply-To: <4B605648.7020705@ring3k.org>
On Thu, 28 Jan 2010 00:05:44 +0900
Mike McCormack <mikem@ring3k.org> wrote:
> Allocating everything in one place means there's a single point
> of failure in sky2_up, and sky2_rx_start can no longer fail.
>
> This also fixes a memory leak in the case that sky2_rx_start
> fails in the middle of allocating skbs, since any allocated
> skbs will not be free'd in sky2_up's failure path.
I may incorporate the cleanup, but there is no leak.
if rx_start fails, it already called rx_clean.
^ permalink raw reply
* Re: [PATCH 1/2] syncookies: print synflood warning if syn queue is full
From: Olaf van der Spek @ 2010-01-27 17:01 UTC (permalink / raw)
To: Florian Westphal; +Cc: netdev
In-Reply-To: <20091208.130959.138311725.davem@davemloft.net>
On Tue, Dec 8, 2009 at 10:09 PM, David Miller <davem@davemloft.net> wrote:
> From: Olaf van der Spek <olafvdspek@gmail.com>
> Date: Tue, 8 Dec 2009 15:47:59 +0100
>
>> On Fri, Oct 16, 2009 at 8:49 PM, Florian Westphal <fw@strlen.de> wrote:
>>> Always print a warning if the syn queue is full, just like
>>> the tcp/ipv6 code does.
>>>
>>> The "want_cookie" define is no longer needed -- gcc
>>> removes the relevant branches in the CONFIG_SYN_COOKIES=n case.
>>>
>>> Signed-off-by: Florian Westphal <fw@strlen.de>
>>
>> Any comments?
>
> You patch isn't even in patchwork any more, so for one thing
> it's definitely not in my queue any more.
Florian?
^ permalink raw reply
* [PATCH] cxgb3/iw_cxgb3: doorbell overflow avoidance and recovery.
From: Steve Wise @ 2010-01-27 17:03 UTC (permalink / raw)
To: rdreier-FYB4Gu1CFyUAvxtiuMwx3w
Cc: jeff-o2qLIJkoznsdnm+yROfE0A, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA, divy-ut6Up61K2wZBDgjK7y7TUQ
T3 hardware doorbell fifo overflows can cause application stalls due to
lost doorbell ring events. This has been seen when running large NP IMB
alltoall MPI jobs. The T3 hardware supports an xon/xoff type control
mechanism to help avoid overflowing the hw db fifo.
This patch utilizes these interrupts to disable rdma qp db rings when
we near an overflow condition, and then turn them back on (and ring all
the active qp dbs) when when the db fifo emptys out. In addition if an
db ring is dropped by the hardware, the code will now recover.
Design:
cxgb3:
- enable these DB interrupts
- in the intr handler, schedule work tasks to call the ULPs event handlers
with the new events.
- ring all the qset txqs when an overflow is detected.
iw_cxgb3:
- disable db ringing on all active qps when we get the DB_FULL event
- enable db ringing on all active qps and ring all active dbs when we get
the DB_EMPTY event
- On DB_DROP event:
- disable db rings in the event handler
- delay-schedule a work task which rings and enables the dbs on
all active qps.
- in post_send and post_recv logic, don't ring the db if its disabled.
Signed-off-by: Steve Wise <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>
---
drivers/infiniband/hw/cxgb3/cxio_wr.h | 17 +++++++
drivers/infiniband/hw/cxgb3/iwch.c | 79 +++++++++++++++++++++++++++++++--
drivers/infiniband/hw/cxgb3/iwch.h | 2 +
drivers/infiniband/hw/cxgb3/iwch_qp.c | 9 +++-
drivers/net/cxgb3/adapter.h | 5 ++
drivers/net/cxgb3/cxgb3_main.c | 57 +++++++++++++++++++++++-
drivers/net/cxgb3/cxgb3_offload.h | 5 ++
drivers/net/cxgb3/regs.h | 16 +++++++
drivers/net/cxgb3/sge.c | 15 +++++-
drivers/net/cxgb3/t3_hw.c | 5 ++
10 files changed, 196 insertions(+), 14 deletions(-)
diff --git a/drivers/infiniband/hw/cxgb3/cxio_wr.h b/drivers/infiniband/hw/cxgb3/cxio_wr.h
index a197a5b..15073b2 100644
--- a/drivers/infiniband/hw/cxgb3/cxio_wr.h
+++ b/drivers/infiniband/hw/cxgb3/cxio_wr.h
@@ -730,7 +730,22 @@ struct t3_cq {
static inline void cxio_set_wq_in_error(struct t3_wq *wq)
{
- wq->queue->wq_in_err.err = 1;
+ wq->queue->wq_in_err.err |= 1;
+}
+
+static inline void cxio_disable_wq_db(struct t3_wq *wq)
+{
+ wq->queue->wq_in_err.err |= 2;
+}
+
+static inline void cxio_enable_wq_db(struct t3_wq *wq)
+{
+ wq->queue->wq_in_err.err &= ~2;
+}
+
+static inline int cxio_wq_db_enabled(struct t3_wq *wq)
+{
+ return !(wq->queue->wq_in_err.err & 2);
}
static inline struct t3_cqe *cxio_next_hw_cqe(struct t3_cq *cq)
diff --git a/drivers/infiniband/hw/cxgb3/iwch.c b/drivers/infiniband/hw/cxgb3/iwch.c
index b0ea010..6a3214a 100644
--- a/drivers/infiniband/hw/cxgb3/iwch.c
+++ b/drivers/infiniband/hw/cxgb3/iwch.c
@@ -65,6 +65,46 @@ struct cxgb3_client t3c_client = {
static LIST_HEAD(dev_list);
static DEFINE_MUTEX(dev_mutex);
+static int disable_qp_db(int id, void *p, void *data)
+{
+ struct iwch_qp *qhp = p;
+
+ cxio_disable_wq_db(&qhp->wq);
+ return 0;
+}
+
+static int enable_qp_db(int id, void *p, void *data)
+{
+ struct iwch_qp *qhp = p;
+
+ if (data)
+ ring_doorbell(qhp->rhp->rdev.ctrl_qp.doorbell, qhp->wq.qpid);
+ cxio_enable_wq_db(&qhp->wq);
+ return 0;
+}
+
+static void disable_dbs(struct iwch_dev *rnicp)
+{
+ spin_lock_irq(&rnicp->lock);
+ idr_for_each(&rnicp->qpidr, disable_qp_db, NULL);
+ spin_unlock_irq(&rnicp->lock);
+}
+
+static void enable_dbs(struct iwch_dev *rnicp, int ring_db)
+{
+ spin_lock_irq(&rnicp->lock);
+ idr_for_each(&rnicp->qpidr, enable_qp_db,
+ (void *)(unsigned long)ring_db);
+ spin_unlock_irq(&rnicp->lock);
+}
+
+static void iwch_db_drop_task(struct work_struct *work)
+{
+ struct iwch_dev *rnicp = container_of(work, struct iwch_dev,
+ db_drop_task.work);
+ enable_dbs(rnicp, 1);
+}
+
static void rnic_init(struct iwch_dev *rnicp)
{
PDBG("%s iwch_dev %p\n", __func__, rnicp);
@@ -72,6 +112,7 @@ static void rnic_init(struct iwch_dev *rnicp)
idr_init(&rnicp->qpidr);
idr_init(&rnicp->mmidr);
spin_lock_init(&rnicp->lock);
+ INIT_DELAYED_WORK(&rnicp->db_drop_task, iwch_db_drop_task);
rnicp->attr.max_qps = T3_MAX_NUM_QP - 32;
rnicp->attr.max_wrs = T3_MAX_QP_DEPTH;
@@ -147,6 +188,7 @@ static void close_rnic_dev(struct t3cdev *tdev)
mutex_lock(&dev_mutex);
list_for_each_entry_safe(dev, tmp, &dev_list, entry) {
if (dev->rdev.t3cdev_p == tdev) {
+ cancel_delayed_work_sync(&dev->db_drop_task);
list_del(&dev->entry);
iwch_unregister_device(dev);
cxio_rdev_close(&dev->rdev);
@@ -165,7 +207,8 @@ static void iwch_event_handler(struct t3cdev *tdev, u32 evt, u32 port_id)
struct cxio_rdev *rdev = tdev->ulp;
struct iwch_dev *rnicp;
struct ib_event event;
- u32 portnum = port_id + 1;
+ u32 portnum = port_id + 1;
+ int dispatch = 0;
if (!rdev)
return;
@@ -174,21 +217,49 @@ static void iwch_event_handler(struct t3cdev *tdev, u32 evt, u32 port_id)
case OFFLOAD_STATUS_DOWN: {
rdev->flags = CXIO_ERROR_FATAL;
event.event = IB_EVENT_DEVICE_FATAL;
+ dispatch = 1;
break;
}
case OFFLOAD_PORT_DOWN: {
event.event = IB_EVENT_PORT_ERR;
+ dispatch = 1;
break;
}
case OFFLOAD_PORT_UP: {
event.event = IB_EVENT_PORT_ACTIVE;
+ dispatch = 1;
+ break;
+ }
+ case OFFLOAD_DB_FULL: {
+ disable_dbs(rnicp);
+ break;
+ }
+ case OFFLOAD_DB_EMPTY: {
+ enable_dbs(rnicp, 1);
+ break;
+ }
+ case OFFLOAD_DB_DROP: {
+ unsigned long delay = 1000;
+ unsigned short r;
+
+ disable_dbs(rnicp);
+ get_random_bytes(&r, 2);
+ delay += r & 1023;
+
+ /*
+ * delay is between 1000-2023 usecs.
+ */
+ schedule_delayed_work(&rnicp->db_drop_task,
+ usecs_to_jiffies(delay));
break;
}
}
- event.device = &rnicp->ibdev;
- event.element.port_num = portnum;
- ib_dispatch_event(&event);
+ if (dispatch) {
+ event.device = &rnicp->ibdev;
+ event.element.port_num = portnum;
+ ib_dispatch_event(&event);
+ }
return;
}
diff --git a/drivers/infiniband/hw/cxgb3/iwch.h b/drivers/infiniband/hw/cxgb3/iwch.h
index 8473550..a1c4457 100644
--- a/drivers/infiniband/hw/cxgb3/iwch.h
+++ b/drivers/infiniband/hw/cxgb3/iwch.h
@@ -36,6 +36,7 @@
#include <linux/list.h>
#include <linux/spinlock.h>
#include <linux/idr.h>
+#include <linux/workqueue.h>
#include <rdma/ib_verbs.h>
@@ -110,6 +111,7 @@ struct iwch_dev {
struct idr mmidr;
spinlock_t lock;
struct list_head entry;
+ struct delayed_work db_drop_task;
};
static inline struct iwch_dev *to_iwch_dev(struct ib_device *ibdev)
diff --git a/drivers/infiniband/hw/cxgb3/iwch_qp.c b/drivers/infiniband/hw/cxgb3/iwch_qp.c
index 3eb8cec..b4d893d 100644
--- a/drivers/infiniband/hw/cxgb3/iwch_qp.c
+++ b/drivers/infiniband/hw/cxgb3/iwch_qp.c
@@ -452,7 +452,8 @@ int iwch_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
++(qhp->wq.sq_wptr);
}
spin_unlock_irqrestore(&qhp->lock, flag);
- ring_doorbell(qhp->wq.doorbell, qhp->wq.qpid);
+ if (cxio_wq_db_enabled(&qhp->wq))
+ ring_doorbell(qhp->wq.doorbell, qhp->wq.qpid);
out:
if (err)
@@ -514,7 +515,8 @@ int iwch_post_receive(struct ib_qp *ibqp, struct ib_recv_wr *wr,
num_wrs--;
}
spin_unlock_irqrestore(&qhp->lock, flag);
- ring_doorbell(qhp->wq.doorbell, qhp->wq.qpid);
+ if (cxio_wq_db_enabled(&qhp->wq))
+ ring_doorbell(qhp->wq.doorbell, qhp->wq.qpid);
out:
if (err)
@@ -597,7 +599,8 @@ int iwch_bind_mw(struct ib_qp *qp,
++(qhp->wq.sq_wptr);
spin_unlock_irqrestore(&qhp->lock, flag);
- ring_doorbell(qhp->wq.doorbell, qhp->wq.qpid);
+ if (cxio_wq_db_enabled(&qhp->wq))
+ ring_doorbell(qhp->wq.doorbell, qhp->wq.qpid);
return err;
}
diff --git a/drivers/net/cxgb3/adapter.h b/drivers/net/cxgb3/adapter.h
index 3e8618b..11d148c 100644
--- a/drivers/net/cxgb3/adapter.h
+++ b/drivers/net/cxgb3/adapter.h
@@ -263,6 +263,10 @@ struct adapter {
struct work_struct ext_intr_handler_task;
struct work_struct fatal_error_handler_task;
struct work_struct link_fault_handler_task;
+
+ struct work_struct db_full_task;
+ struct work_struct db_empty_task;
+ struct work_struct db_drop_task;
struct dentry *debugfs_root;
@@ -335,6 +339,7 @@ int t3_sge_alloc_qset(struct adapter *adapter, unsigned int id, int nports,
int t3_get_desc(const struct sge_qset *qs, unsigned int qnum, unsigned int idx,
unsigned char *data);
irqreturn_t t3_sge_intr_msix(int irq, void *cookie);
+extern struct workqueue_struct *cxgb3_wq;
int t3_get_edc_fw(struct cphy *phy, int edc_idx, int size);
diff --git a/drivers/net/cxgb3/cxgb3_main.c b/drivers/net/cxgb3/cxgb3_main.c
index 2aaa747..f47dc76 100644
--- a/drivers/net/cxgb3/cxgb3_main.c
+++ b/drivers/net/cxgb3/cxgb3_main.c
@@ -45,6 +45,7 @@
#include <linux/firmware.h>
#include <linux/log2.h>
#include <linux/stringify.h>
+#include <linux/sched.h>
#include <asm/uaccess.h>
#include "common.h"
@@ -140,7 +141,7 @@ MODULE_PARM_DESC(ofld_disable, "whether to enable offload at init time or not");
* will block keventd as it needs the rtnl lock, and we'll deadlock waiting
* for our work to complete. Get our own work queue to solve this.
*/
-static struct workqueue_struct *cxgb3_wq;
+struct workqueue_struct *cxgb3_wq;
/**
* link_report - show link status and link speed/duplex
@@ -590,6 +591,19 @@ static void setup_rss(struct adapter *adap)
V_RRCPLCPUSIZE(6) | F_HASHTOEPLITZ, cpus, rspq_map);
}
+static void ring_dbs(struct adapter *adap)
+{
+ int i, j;
+
+ for (i = 0; i < SGE_QSETS; i++) {
+ struct sge_qset *qs = &adap->sge.qs[i];
+
+ if (qs->adap)
+ for (j=0; j < SGE_TXQ_PER_SET; j++)
+ t3_write_reg(adap, A_SG_KDOORBELL, F_SELEGRCNTX | V_EGRCNTX(qs->txq[j].cntxt_id));
+ }
+}
+
static void init_napi(struct adapter *adap)
{
int i;
@@ -2778,6 +2792,42 @@ static void t3_adap_check_task(struct work_struct *work)
spin_unlock_irq(&adapter->work_lock);
}
+static void db_full_task(struct work_struct *work)
+{
+ struct adapter *adapter = container_of(work, struct adapter,
+ db_full_task);
+
+ cxgb3_event_notify(&adapter->tdev, OFFLOAD_DB_FULL, 0);
+}
+
+static void db_empty_task(struct work_struct *work)
+{
+ struct adapter *adapter = container_of(work, struct adapter,
+ db_empty_task);
+
+ cxgb3_event_notify(&adapter->tdev, OFFLOAD_DB_EMPTY, 0);
+}
+
+static void db_drop_task(struct work_struct *work)
+{
+ struct adapter *adapter = container_of(work, struct adapter,
+ db_drop_task);
+ unsigned long delay = 1000;
+ unsigned short r;
+
+ cxgb3_event_notify(&adapter->tdev, OFFLOAD_DB_DROP, 0);
+
+ /*
+ * Sleep a while before ringing the driver qset dbs.
+ * The delay is between 1000-2023 usecs.
+ */
+ get_random_bytes(&r, 2);
+ delay += r & 1023;
+ set_current_state(TASK_UNINTERRUPTIBLE);
+ schedule_timeout(usecs_to_jiffies(delay));
+ ring_dbs(adapter);
+}
+
/*
* Processes external (PHY) interrupts in process context.
*/
@@ -3246,6 +3296,11 @@ static int __devinit init_one(struct pci_dev *pdev,
INIT_LIST_HEAD(&adapter->adapter_list);
INIT_WORK(&adapter->ext_intr_handler_task, ext_intr_task);
INIT_WORK(&adapter->fatal_error_handler_task, fatal_error_task);
+
+ INIT_WORK(&adapter->db_full_task, db_full_task);
+ INIT_WORK(&adapter->db_empty_task, db_empty_task);
+ INIT_WORK(&adapter->db_drop_task, db_drop_task);
+
INIT_DELAYED_WORK(&adapter->adap_check_task, t3_adap_check_task);
for (i = 0; i < ai->nports0 + ai->nports1; ++i) {
diff --git a/drivers/net/cxgb3/cxgb3_offload.h b/drivers/net/cxgb3/cxgb3_offload.h
index 670aa62..929c298 100644
--- a/drivers/net/cxgb3/cxgb3_offload.h
+++ b/drivers/net/cxgb3/cxgb3_offload.h
@@ -73,7 +73,10 @@ enum {
OFFLOAD_STATUS_UP,
OFFLOAD_STATUS_DOWN,
OFFLOAD_PORT_DOWN,
- OFFLOAD_PORT_UP
+ OFFLOAD_PORT_UP,
+ OFFLOAD_DB_FULL,
+ OFFLOAD_DB_EMPTY,
+ OFFLOAD_DB_DROP
};
struct cxgb3_client {
diff --git a/drivers/net/cxgb3/regs.h b/drivers/net/cxgb3/regs.h
index 1b5327b..cb42353 100644
--- a/drivers/net/cxgb3/regs.h
+++ b/drivers/net/cxgb3/regs.h
@@ -254,6 +254,22 @@
#define V_LOPIODRBDROPERR(x) ((x) << S_LOPIODRBDROPERR)
#define F_LOPIODRBDROPERR V_LOPIODRBDROPERR(1U)
+#define S_HIPRIORITYDBFULL 7
+#define V_HIPRIORITYDBFULL(x) ((x) << S_HIPRIORITYDBFULL)
+#define F_HIPRIORITYDBFULL V_HIPRIORITYDBFULL(1U)
+
+#define S_HIPRIORITYDBEMPTY 6
+#define V_HIPRIORITYDBEMPTY(x) ((x) << S_HIPRIORITYDBEMPTY)
+#define F_HIPRIORITYDBEMPTY V_HIPRIORITYDBEMPTY(1U)
+
+#define S_LOPRIORITYDBFULL 5
+#define V_LOPRIORITYDBFULL(x) ((x) << S_LOPRIORITYDBFULL)
+#define F_LOPRIORITYDBFULL V_LOPRIORITYDBFULL(1U)
+
+#define S_LOPRIORITYDBEMPTY 4
+#define V_LOPRIORITYDBEMPTY(x) ((x) << S_LOPRIORITYDBEMPTY)
+#define F_LOPRIORITYDBEMPTY V_LOPRIORITYDBEMPTY(1U)
+
#define S_RSPQDISABLED 3
#define V_RSPQDISABLED(x) ((x) << S_RSPQDISABLED)
#define F_RSPQDISABLED V_RSPQDISABLED(1U)
diff --git a/drivers/net/cxgb3/sge.c b/drivers/net/cxgb3/sge.c
index bdbd147..08978ad 100644
--- a/drivers/net/cxgb3/sge.c
+++ b/drivers/net/cxgb3/sge.c
@@ -42,6 +42,7 @@
#include "sge_defs.h"
#include "t3_cpl.h"
#include "firmware_exports.h"
+#include "cxgb3_offload.h"
#define USE_GTS 0
@@ -2828,9 +2829,17 @@ void t3_sge_err_intr_handler(struct adapter *adapter)
"(0x%x)\n", (v >> S_RSPQ0DISABLED) & 0xff);
}
- if (status & (F_HIPIODRBDROPERR | F_LOPIODRBDROPERR))
- CH_ALERT(adapter, "SGE dropped %s priority doorbell\n",
- status & F_HIPIODRBDROPERR ? "high" : "lo");
+ if (status & (F_HIPIODRBDROPERR | F_LOPIODRBDROPERR)) {
+ queue_work(cxgb3_wq, &adapter->db_drop_task);
+ }
+
+ if (status & (F_HIPRIORITYDBFULL | F_LOPRIORITYDBFULL)) {
+ queue_work(cxgb3_wq, &adapter->db_full_task);
+ }
+
+ if (status & (F_HIPRIORITYDBEMPTY | F_LOPRIORITYDBEMPTY)) {
+ queue_work(cxgb3_wq, &adapter->db_empty_task);
+ }
t3_write_reg(adapter, A_SG_INT_CAUSE, status);
if (status & SGE_FATALERR)
diff --git a/drivers/net/cxgb3/t3_hw.c b/drivers/net/cxgb3/t3_hw.c
index 032cfe0..c38fc71 100644
--- a/drivers/net/cxgb3/t3_hw.c
+++ b/drivers/net/cxgb3/t3_hw.c
@@ -1432,7 +1432,10 @@ static int t3_handle_intr_status(struct adapter *adapter, unsigned int reg,
F_IRPARITYERROR | V_ITPARITYERROR(M_ITPARITYERROR) | \
V_FLPARITYERROR(M_FLPARITYERROR) | F_LODRBPARITYERROR | \
F_HIDRBPARITYERROR | F_LORCQPARITYERROR | \
- F_HIRCQPARITYERROR)
+ F_HIRCQPARITYERROR | F_LOPRIORITYDBFULL | \
+ F_HIPRIORITYDBFULL | F_LOPRIORITYDBEMPTY | \
+ F_HIPRIORITYDBEMPTY | F_HIPIODRBDROPERR | \
+ F_LOPIODRBDROPERR)
#define MC5_INTR_MASK (F_PARITYERR | F_ACTRGNFULL | F_UNKNOWNCMD | \
F_REQQPARERR | F_DISPQPARERR | F_DELACTEMPTY | \
F_NFASRCHFAIL)
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [PATCH 3/3] sky2: Remove failure cases for sky2_rx_start
From: Stephen Hemminger @ 2010-01-27 17:10 UTC (permalink / raw)
To: Mike McCormack; +Cc: netdev
In-Reply-To: <4B605652.6020400@ring3k.org>
On Thu, 28 Jan 2010 00:05:54 +0900
Mike McCormack <mikem@ring3k.org> wrote:
> sky2_rx_start() can no longer fail, so remove redundant code pathes.
>
> Signed-off-by: Mike McCormack <mikem@ring3k.org>
This can't work right if MTU is increased. The buffers need to be reallocated
in change_mtu(); and yes it could fail when getting the new buffers.
--
^ permalink raw reply
* Re: [PATCH net-next-2.6] packet: Add GSO/checksum offload support to af_packet sockets
From: Sridhar Samudrala @ 2010-01-27 17:42 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: David Miller, Rusty Russell, Herbert Xu, netdev
In-Reply-To: <20100127114202.GA6696@redhat.com>
On Wed, 2010-01-27 at 13:42 +0200, Michael S. Tsirkin wrote:
> On Tue, Jan 26, 2010 at 12:30:19PM -0800, Sridhar Samudrala wrote:
> > This patch adds GSO/checksum offload to af_packet sockets using
> > virtio_net_hdr. Based on Rusty's patch to add this support to tun.
> > It allows GSO/checksum offload to be enabled when using raw socket
> > backend with virtio_net.
> > Adds PACKET_VNET_HDR socket option to prepend virtio_net_hdr in the
> > receive path and process/skip virtio_net_hdr in the send path. This
> > option is only allowed with SOCK_RAW sockets attached to ethernet
> > type devices.
> >
> > Signed-off-by: Sridhar Samudrala <sri@us.ibm.com>
>
> So the main issue with this implemenation is that it silently fails for
> non-ethernet protocols. It would be better to detect unsupported
> protocols and return an error to user.
Yes. I could return EINVAL or EOPNOTSUPP when trying to send/receive a
packet with virtio_net_hdr on non-ethernet devices.
> This is same issue that was
> pointed out by DaveM with my earlier attempt to solve a different
> (related) problem:
> http://lkml.org/lkml/2010/1/5/474
> For an incomplete prototype attempting to solve the issue in a generic way:
> http://lkml.org/lkml/2010/1/6/56
>
> A couple of additional comments below.
>
> > diff --git a/include/linux/if_packet.h b/include/linux/if_packet.h
> > index 4021d47..aa57a5f 100644
> > --- a/include/linux/if_packet.h
> > +++ b/include/linux/if_packet.h
> > @@ -46,6 +46,7 @@ struct sockaddr_ll {
> > #define PACKET_RESERVE 12
> > #define PACKET_TX_RING 13
> > #define PACKET_LOSS 14
> > +#define PACKET_VNET_HDR 15
> >
> > struct tpacket_stats {
> > unsigned int tp_packets;
> > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> > index 53633c5..36d5360 100644
> > --- a/net/packet/af_packet.c
> > +++ b/net/packet/af_packet.c
> > @@ -80,6 +80,8 @@
> > #include <linux/init.h>
> > #include <linux/mutex.h>
> > #include <linux/if_vlan.h>
> > +#include <linux/virtio_net.h>
> > +#include <linux/if_arp.h>
> >
> > #ifdef CONFIG_INET
> > #include <net/inet_common.h>
> > @@ -193,7 +195,8 @@ struct packet_sock {
> > struct mutex pg_vec_lock;
> > unsigned int running:1, /* prot_hook is attached*/
> > auxdata:1,
> > - origdev:1;
> > + origdev:1,
> > + vnet_hdr:1;
> > int ifindex; /* bound device */
> > __be16 num;
> > struct packet_mclist *mclist;
> > @@ -1056,6 +1059,30 @@ out:
> > }
> > #endif
> >
> > +static inline struct sk_buff *packet_alloc_skb(struct sock *sk, size_t prepad,
> > + size_t reserve, size_t len,
> > + size_t linear, int noblock,
> > + int *err)
> > +{
> > + struct sk_buff *skb;
> > +
> > + /* Under a page? Don't bother with paged skb. */
> > + if (prepad + len < PAGE_SIZE || !linear)
> > + linear = len;
> > +
> > + skb = sock_alloc_send_pskb(sk, prepad + linear, len - linear, noblock,
> > + err);
> > + if (!skb)
> > + return NULL;
> > +
> > + skb_reserve(skb, reserve);
> > + skb_put(skb, linear);
> > + skb->data_len = len - linear;
> > + skb->len += len - linear;
> > +
> > + return skb;
> > +}
> > +
> > static int packet_snd(struct socket *sock,
> > struct msghdr *msg, size_t len)
> > {
> > @@ -1066,14 +1093,15 @@ static int packet_snd(struct socket *sock,
> > __be16 proto;
> > unsigned char *addr;
> > int ifindex, err, reserve = 0;
> > + struct virtio_net_hdr vnethdr = { 0 };
> > + int offset = 0;
> > + struct packet_sock *po = pkt_sk(sk);
> >
> > /*
> > * Get and verify the address.
> > */
> >
> > if (saddr == NULL) {
> > - struct packet_sock *po = pkt_sk(sk);
> > -
> > ifindex = po->ifindex;
> > proto = po->num;
> > addr = NULL;
> > @@ -1100,25 +1128,52 @@ static int packet_snd(struct socket *sock,
> > if (!(dev->flags & IFF_UP))
> > goto out_unlock;
> >
> > - err = -EMSGSIZE;
> > - if (len > dev->mtu+reserve)
> > - goto out_unlock;
> > + if (po->vnet_hdr) {
> > + err = -EINVAL;
> > + if (dev->type != ARPHRD_ETHER)
> > + goto out_unlock;
> > +
> > + if (len < sizeof(vnethdr))
> > + goto out_unlock;
> >
> > - skb = sock_alloc_send_skb(sk, len + LL_ALLOCATED_SPACE(dev),
> > - msg->msg_flags & MSG_DONTWAIT, &err);
> > + len -= sizeof(vnethdr);
> > +
> > + err = -EFAULT;
> > + if (memcpy_fromiovec((void *)&vnethdr, msg->msg_iov,
> > + sizeof(vnethdr)))
> > + goto out_unlock;
> > +
> > + if ((vnethdr.flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) &&
> > + (vnethdr.csum_start + vnethdr.csum_offset + 2 >
> > + vnethdr.hdr_len))
> > + vnethdr.hdr_len = vnethdr.csum_start +
> > + vnethdr.csum_offset + 2;
> > +
> > + err = -EINVAL;
> > + if (vnethdr.hdr_len > len)
> > + goto out_unlock;
> > + } else {
> > + err = -EMSGSIZE;
> > + if (len > dev->mtu+reserve)
> > + goto out_unlock;
>
> IMO we should always perform the length check if GSO is off.
OK. I will fix this.
>
> > + }
> > +
> > + err = -ENOBUFS;
> > + skb = packet_alloc_skb(sk, LL_ALLOCATED_SPACE(dev),
> > + LL_RESERVED_SPACE(dev), len, vnethdr.hdr_len,
> > + msg->msg_flags & MSG_DONTWAIT, &err);
> > if (skb == NULL)
> > goto out_unlock;
> >
> > - skb_reserve(skb, LL_RESERVED_SPACE(dev));
> > - skb_reset_network_header(skb);
> > + skb_set_network_header(skb, reserve);
>
> I think the above is wrong for vlans?
I also thought we need to address vlans here, but even tun doesn't
handle this in the send routine. I submitted a patch that fixed
skb_gso_segment() to handle vlan packets. This will address both
tun and packet sockets.
http://thread.gmane.org/gmane.linux.network/150198
With this patch, i tested vlans with both ipv4 and ipv6.
>
> >
> > err = -EINVAL;
> > if (sock->type == SOCK_DGRAM &&
> > - dev_hard_header(skb, dev, ntohs(proto), addr, NULL, len) < 0)
> > + (offset = dev_hard_header(skb, dev, ntohs(proto), addr, NULL, len)) < 0)
> > goto out_free;
> >
> > /* Returns -EFAULT on error */
> > - err = memcpy_fromiovec(skb_put(skb, len), msg->msg_iov, len);
> > + err = skb_copy_datagram_from_iovec(skb, offset, msg->msg_iov, 0, len);
> > if (err)
> > goto out_free;
> >
> > @@ -1127,6 +1182,51 @@ static int packet_snd(struct socket *sock,
> > skb->priority = sk->sk_priority;
> > skb->mark = sk->sk_mark;
> >
> > + if (po->vnet_hdr) {
> > + skb_reset_mac_header(skb);
> > + skb->protocol = eth_hdr(skb)->h_proto;
> > +
>
> Is this also broken for vlans?
Same as above.
>
> > + if (vnethdr.flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) {
> > + if (!skb_partial_csum_set(skb, vnethdr.csum_start,
> > + vnethdr.csum_offset)) {
> > + err = -EINVAL;
> > + goto out_free;
> > + }
> > + }
> > +
> > + if (vnethdr.gso_type != VIRTIO_NET_HDR_GSO_NONE) {
> > + switch (vnethdr.gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
> > + case VIRTIO_NET_HDR_GSO_TCPV4:
> > + skb_shinfo(skb)->gso_type = SKB_GSO_TCPV4;
> > + break;
> > + case VIRTIO_NET_HDR_GSO_TCPV6:
> > + skb_shinfo(skb)->gso_type = SKB_GSO_TCPV6;
> > + break;
> > + case VIRTIO_NET_HDR_GSO_UDP:
> > + skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
> > + break;
> > + default:
> > + err = -EINVAL;
> > + goto out_free;
> > + }
> > +
> > + if (vnethdr.gso_type & VIRTIO_NET_HDR_GSO_ECN)
> > + skb_shinfo(skb)->gso_type |= SKB_GSO_TCP_ECN;
> > +
> > + skb_shinfo(skb)->gso_size = vnethdr.gso_size;
> > + if (skb_shinfo(skb)->gso_size == 0) {
> > + err = -EINVAL;
> > + goto out_free;
> > + }
> > +
> > + /* Header must be checked, and gso_segs computed. */
> > + skb_shinfo(skb)->gso_type |= SKB_GSO_DODGY;
> > + skb_shinfo(skb)->gso_segs = 0;
> > + }
> > +
> > + len += sizeof(vnethdr);
> > + }
> > +
> > /*
> > * Now send it
> > */
> > @@ -1420,6 +1520,7 @@ static int packet_recvmsg(struct kiocb *iocb, struct socket *sock,
> > struct sk_buff *skb;
> > int copied, err;
> > struct sockaddr_ll *sll;
> > + int vnet_hdr_len = 0;
> >
> > err = -EINVAL;
> > if (flags & ~(MSG_PEEK|MSG_DONTWAIT|MSG_TRUNC|MSG_CMSG_COMPAT))
> > @@ -1451,6 +1552,44 @@ static int packet_recvmsg(struct kiocb *iocb, struct socket *sock,
> > if (skb == NULL)
> > goto out;
> >
> > + if (pkt_sk(sk)->vnet_hdr) {
> > + struct virtio_net_hdr vnethdr = { 0 };
> > +
> > + vnet_hdr_len = sizeof(vnethdr);
> > + if ((len -= vnet_hdr_len) < 0)
> > + return -EINVAL;
> > +
> > + if (skb_is_gso(skb)) {
> > + struct skb_shared_info *sinfo = skb_shinfo(skb);
> > +
> > + /* This is a hint as to how much should be linear. */
> > + vnethdr.hdr_len = skb_headlen(skb);
> > + vnethdr.gso_size = sinfo->gso_size;
> > + if (sinfo->gso_type & SKB_GSO_TCPV4)
> > + vnethdr.gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
> > + else if (sinfo->gso_type & SKB_GSO_TCPV6)
> > + vnethdr.gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
> > + else if (sinfo->gso_type & SKB_GSO_UDP)
> > + vnethdr.gso_type = VIRTIO_NET_HDR_GSO_UDP;
> > + else
> > + BUG();
>
> Is there any chance this can get SKB_GSO_FCOE by binding to
> an appropriate interface? Maybe we don't want to BUG().
I could return -EINVAL in that case.
>
> > + if (sinfo->gso_type & SKB_GSO_TCP_ECN)
> > + vnethdr.gso_type |= VIRTIO_NET_HDR_GSO_ECN;
> > + } else
> > + vnethdr.gso_type = VIRTIO_NET_HDR_GSO_NONE;
> > +
> > + if (skb->ip_summed == CHECKSUM_PARTIAL) {
> > + vnethdr.flags = VIRTIO_NET_HDR_F_NEEDS_CSUM;
> > + vnethdr.csum_start = skb->csum_start - skb_headroom(skb);
> > + vnethdr.csum_offset = skb->csum_offset;
> > + } /* else everything is zero */
> > +
> > + if (unlikely(memcpy_toiovec(msg->msg_iov, (void *)&vnethdr,
> > + sizeof(vnethdr)))) {
> > + return -EFAULT;
> > + }
> > + }
> > +
> > /*
> > * If the address length field is there to be filled in, we fill
> > * it in now.
> > @@ -1502,7 +1641,7 @@ static int packet_recvmsg(struct kiocb *iocb, struct socket *sock,
> > * Free or return the buffer as appropriate. Again this
> > * hides all the races and re-entrancy issues from us.
> > */
> > - err = (flags&MSG_TRUNC) ? skb->len : copied;
> > + err = vnet_hdr_len + ((flags&MSG_TRUNC) ? skb->len : copied);
> >
> > out_free:
> > skb_free_datagram(sk, skb);
> > @@ -1826,6 +1965,22 @@ packet_setsockopt(struct socket *sock, int level, int optname, char __user *optv
> > po->origdev = !!val;
> > return 0;
> > }
> > + case PACKET_VNET_HDR:
> > + {
> > + int val;
> > +
> > + if (sock->type != SOCK_RAW)
> > + return -EINVAL;
> > + if (po->rx_ring.pg_vec || po->tx_ring.pg_vec)
> > + return -EBUSY;
>
> Another way to get a broken ring + vnet hdr configuration
> would be to enable vnet hdr first and mmap second.
> I think we need to guard against this as well, by checking vnet_hdr
> when tx/rx ring is enabled.
OK. i will add a check when setting PACKET_RX_RING/TX_RING socket
options.
> > + if (optlen < sizeof(val))
> > + return -EINVAL;
> > + if (copy_from_user(&val, optval, sizeof(val)))
> > + return -EFAULT;
> > +
> > + po->vnet_hdr = !!val;
> > + return 0;
> > + }
> > default:
> > return -ENOPROTOOPT;
> > }
> > @@ -1876,6 +2031,13 @@ static int packet_getsockopt(struct socket *sock, int level, int optname,
> >
> > data = &val;
> > break;
> > + case PACKET_VNET_HDR:
> > + if (len > sizeof(int))
> > + len = sizeof(int);
> > + val = po->vnet_hdr;
> > +
> > + data = &val;
> > + break;
> > #ifdef CONFIG_PACKET_MMAP
> > case PACKET_VERSION:
> > if (len > sizeof(int))
> >
>
^ permalink raw reply
* Re: Hang: 2.6.32.4 sky2/DMAR (was [PATCH] sky2: Fix WARNING: at lib/dma-debug.c:902 check_sync)
From: Stephen Hemminger @ 2010-01-27 17:45 UTC (permalink / raw)
To: Michael Breuer
Cc: Jarek Poplawski, David Miller, akpm, flyboy, linux-kernel, netdev,
Michael Chan, Don Fry, Francois Romieu, Matt Carlson
In-Reply-To: <4B60707F.1000608@majjas.com>
On Wed, 27 Jan 2010 11:57:35 -0500
Michael Breuer <mbreuer@majjas.com> wrote:
> On 1/27/2010 11:50 AM, Stephen Hemminger wrote:
> > On Wed, 27 Jan 2010 10:34:51 -0500
> > Michael Breuer<mbreuer@majjas.com> wrote:
> >
> >
> >> On 01/23/2010 06:21 PM, Jarek Poplawski wrote:
> >>
> >>> On Fri, Jan 22, 2010 at 06:50:21PM -0500, Michael Breuer wrote:
> >>>
> >>>
> >>>> When the packets were dropped, there was a different sequence in the
> >>>> log - DISCOVER/OFFER repeated. The "normal" is that the sequence
> >>>> appeared correct and complete - DISCOVER/OFFER/REQUEST/ACK - or
> >>>> INFORM/ACK (vs. INFORM repeatedly sans ACK) as the case may be.
> >>>>
> >>>>
> >>> Anyway, I'd be intersted if the switch matters here.
> >>>
> >>> Plus one more test: could you try to load sky2 with the parameter:
> >>> "copybreak=1" (the rest as in any recent test, which gave you dmar
> >>> errors; any switch).
> >>>
> >>> Thanks,
> >>> Jarek P.
> >>>
> >>>
> >> Ok - now up 80+ hours with copybreak=1. I'm going to redo w/o copybreak
> >> to confirm that I haven't inadvertently fixed something. However, given
> >> that it might be copybreak-related, I looked at sky2.c again and I'm
> >> wondering about the copybreak max size in sky2_rx_start:
> >>
> >> size = roundup(sky2->netdev->mtu + ETH_HLEN + VLAN_HLEN, 8);
> >>
> >> /* Stopping point for hardware truncation */
> >> thresh = (size - 8) / sizeof(u32);
> >>
> >> sky2->rx_nfrags = size>> PAGE_SHIFT;
> >> BUG_ON(sky2->rx_nfrags> ARRAY_SIZE(re->frag_addr));
> >>
> >> /* Compute residue after pages */
> >> size -= sky2->rx_nfrags<< PAGE_SHIFT;
> >>
> >> /* Optimize to handle small packets and headers */
> >> if (size< copybreak)
> >> size = copybreak;
> >> if (size< ETH_HLEN)
> >> size = ETH_HLEN;
> >>
> >>
> >> Why would increasing size to copybreak be valid here?
> >>
> >> Guessing a bit as I'm not sure about rx_nfrags, but if I read this
> >> correctly, if size is ever less than copybreak it's because there isn't
> >> enough space left for anything larger. If so, wouldn't increasing size
> >> potentially corrupt something? I'd further guess that the resulting
> >> condition manifests sooner (or at least with a more visible effect) when
> >> using DMAR.
> >>
> >> In any event, why "copybreak" as the minimum buffer size? I'd suggest
> >> that if it isn't possible to allocate at least MTU + overhead that
> >> sky2_rx_start ought to be delayed until there is room.
> >>
> > This code is where driver decides how much data will be received in skb
> > data area and the remaining data spills over into skb frags.
> > Copybreak is the threshold so that packets less than size are copied
> > to a new skb. The code doing the copying there assumes the data is
> > totally contained in the skb (not in frags). The size increase there
> > is to make sure that assumption is always true. I suppose you
> > could do something perverse like setting copybreak really huge
> > and confuse driver, but that is a user error.
> >
> >
> Ok - but I'm wondering under what circumstances size would be <
> copybreak in the first place after computing the residue. If size ends
> up being unreasonably small, is simply increasing the number to whatever
> copybreak is correct? Assuming my testing is correct, then the crash
> I've been experiencing when using dmar (only) seems related to the value
> of copybreak. I don't think the other use (skb reuse) is the issue (but
> hey, I could have missed something). The crash occurs when copybreak is
> the default of 128, didn't happen when I set copybreak to 1.
>
Setting it to 1 causes driver to never go through the dma_sync_single/memcpy
path. Perhaps the code for DMAR doesn't do dma_sync_single_for_cpu
properly, or the value passed to sync_single_for_cpu doesn't account for
all the overhead of padding and/or ether header.
--
^ permalink raw reply
* Re: Hang: 2.6.32.4 sky2/DMAR (was [PATCH] sky2: Fix WARNING: at lib/dma-debug.c:902 check_sync)
From: Stephen Hemminger @ 2010-01-27 17:56 UTC (permalink / raw)
To: Michael Breuer
Cc: Jarek Poplawski, David Miller, akpm, flyboy, linux-kernel, netdev,
Michael Chan, Don Fry, Francois Romieu, Matt Carlson
In-Reply-To: <4B60707F.1000608@majjas.com>
On Wed, 27 Jan 2010 11:57:35 -0500
Michael Breuer <mbreuer@majjas.com> wrote:
> On 1/27/2010 11:50 AM, Stephen Hemminger wrote:
> > On Wed, 27 Jan 2010 10:34:51 -0500
> > Michael Breuer<mbreuer@majjas.com> wrote:
> >
> >
> >> On 01/23/2010 06:21 PM, Jarek Poplawski wrote:
> >>
> >>> On Fri, Jan 22, 2010 at 06:50:21PM -0500, Michael Breuer wrote:
> >>>
> >>>
> >>>> When the packets were dropped, there was a different sequence in the
> >>>> log - DISCOVER/OFFER repeated. The "normal" is that the sequence
> >>>> appeared correct and complete - DISCOVER/OFFER/REQUEST/ACK - or
> >>>> INFORM/ACK (vs. INFORM repeatedly sans ACK) as the case may be.
> >>>>
> >>>>
> >>> Anyway, I'd be intersted if the switch matters here.
> >>>
> >>> Plus one more test: could you try to load sky2 with the parameter:
> >>> "copybreak=1" (the rest as in any recent test, which gave you dmar
> >>> errors; any switch).
> >>>
> >>> Thanks,
> >>> Jarek P.
> >>>
> >>>
> >> Ok - now up 80+ hours with copybreak=1. I'm going to redo w/o copybreak
> >> to confirm that I haven't inadvertently fixed something. However, given
> >> that it might be copybreak-related, I looked at sky2.c again and I'm
> >> wondering about the copybreak max size in sky2_rx_start:
> >>
> >> size = roundup(sky2->netdev->mtu + ETH_HLEN + VLAN_HLEN, 8);
> >>
> >> /* Stopping point for hardware truncation */
> >> thresh = (size - 8) / sizeof(u32);
> >>
> >> sky2->rx_nfrags = size>> PAGE_SHIFT;
> >> BUG_ON(sky2->rx_nfrags> ARRAY_SIZE(re->frag_addr));
> >>
> >> /* Compute residue after pages */
> >> size -= sky2->rx_nfrags<< PAGE_SHIFT;
> >>
> >> /* Optimize to handle small packets and headers */
> >> if (size< copybreak)
> >> size = copybreak;
> >> if (size< ETH_HLEN)
> >> size = ETH_HLEN;
> >>
> >>
> >> Why would increasing size to copybreak be valid here?
> >>
> >> Guessing a bit as I'm not sure about rx_nfrags, but if I read this
> >> correctly, if size is ever less than copybreak it's because there isn't
> >> enough space left for anything larger. If so, wouldn't increasing size
> >> potentially corrupt something? I'd further guess that the resulting
> >> condition manifests sooner (or at least with a more visible effect) when
> >> using DMAR.
> >>
> >> In any event, why "copybreak" as the minimum buffer size? I'd suggest
> >> that if it isn't possible to allocate at least MTU + overhead that
> >> sky2_rx_start ought to be delayed until there is room.
> >>
> > This code is where driver decides how much data will be received in skb
> > data area and the remaining data spills over into skb frags.
> > Copybreak is the threshold so that packets less than size are copied
> > to a new skb. The code doing the copying there assumes the data is
> > totally contained in the skb (not in frags). The size increase there
> > is to make sure that assumption is always true. I suppose you
> > could do something perverse like setting copybreak really huge
> > and confuse driver, but that is a user error.
> >
> >
> Ok - but I'm wondering under what circumstances size would be <
> copybreak in the first place after computing the residue. If size ends
> up being unreasonably small, is simply increasing the number to whatever
> copybreak is correct? Assuming my testing is correct, then the crash
> I've been experiencing when using dmar (only) seems related to the value
> of copybreak. I don't think the other use (skb reuse) is the issue (but
> hey, I could have missed something). The crash occurs when copybreak is
> the default of 128, didn't happen when I set copybreak to 1.
Does this change it? If so the dma code is (not sky2) is buggy and not
rounding up properly.
--- a/drivers/net/sky2.c 2010-01-27 09:46:10.940005248 -0800
+++ b/drivers/net/sky2.c 2010-01-27 09:53:47.141267850 -0800
@@ -2257,13 +2257,16 @@ static struct sk_buff *receive_copy(stru
skb = netdev_alloc_skb_ip_align(sky2->netdev, length);
if (likely(skb)) {
+ unsigned dma_align = dma_get_cache_alignment();
+ unsigned dma_size = ALIGN(length+1, dma_align);
+
pci_dma_sync_single_for_cpu(sky2->hw->pdev, re->data_addr,
- length, PCI_DMA_FROMDEVICE);
+ dma_size, PCI_DMA_FROMDEVICE);
skb_copy_from_linear_data(re->skb, skb->data, length);
skb->ip_summed = re->skb->ip_summed;
skb->csum = re->skb->csum;
pci_dma_sync_single_for_device(sky2->hw->pdev, re->data_addr,
- length, PCI_DMA_FROMDEVICE);
+ dma_size, PCI_DMA_FROMDEVICE);
re->skb->ip_summed = CHECKSUM_NONE;
skb_put(skb, length);
}
^ permalink raw reply
* Re: Hang: 2.6.32.4 sky2/DMAR (was [PATCH] sky2: Fix WARNING: at lib/dma-debug.c:902 check_sync)
From: Michael Breuer @ 2010-01-27 17:57 UTC (permalink / raw)
To: Stephen Hemminger
Cc: Jarek Poplawski, David Miller, akpm, flyboy, linux-kernel, netdev,
Michael Chan, Don Fry, Francois Romieu, Matt Carlson
In-Reply-To: <20100127094531.53c85aa7@nehalam>
On 1/27/2010 12:45 PM, Stephen Hemminger wrote:
> On Wed, 27 Jan 2010 11:57:35 -0500
> Michael Breuer<mbreuer@majjas.com> wrote:
>
>
>> On 1/27/2010 11:50 AM, Stephen Hemminger wrote:
>>
>>> On Wed, 27 Jan 2010 10:34:51 -0500
>>> Michael Breuer<mbreuer@majjas.com> wrote:
>>>
>>>
>>>
>>>> On 01/23/2010 06:21 PM, Jarek Poplawski wrote:
>>>>
>>>>
>>>>> On Fri, Jan 22, 2010 at 06:50:21PM -0500, Michael Breuer wrote:
>>>>>
>>>>>
>>>>>
>>>>>> When the packets were dropped, there was a different sequence in the
>>>>>> log - DISCOVER/OFFER repeated. The "normal" is that the sequence
>>>>>> appeared correct and complete - DISCOVER/OFFER/REQUEST/ACK - or
>>>>>> INFORM/ACK (vs. INFORM repeatedly sans ACK) as the case may be.
>>>>>>
>>>>>>
>>>>>>
>>>>> Anyway, I'd be intersted if the switch matters here.
>>>>>
>>>>> Plus one more test: could you try to load sky2 with the parameter:
>>>>> "copybreak=1" (the rest as in any recent test, which gave you dmar
>>>>> errors; any switch).
>>>>>
>>>>> Thanks,
>>>>> Jarek P.
>>>>>
>>>>>
>>>>>
>>>> Ok - now up 80+ hours with copybreak=1. I'm going to redo w/o copybreak
>>>> to confirm that I haven't inadvertently fixed something. However, given
>>>> that it might be copybreak-related, I looked at sky2.c again and I'm
>>>> wondering about the copybreak max size in sky2_rx_start:
>>>>
>>>> size = roundup(sky2->netdev->mtu + ETH_HLEN + VLAN_HLEN, 8);
>>>>
>>>> /* Stopping point for hardware truncation */
>>>> thresh = (size - 8) / sizeof(u32);
>>>>
>>>> sky2->rx_nfrags = size>> PAGE_SHIFT;
>>>> BUG_ON(sky2->rx_nfrags> ARRAY_SIZE(re->frag_addr));
>>>>
>>>> /* Compute residue after pages */
>>>> size -= sky2->rx_nfrags<< PAGE_SHIFT;
>>>>
>>>> /* Optimize to handle small packets and headers */
>>>> if (size< copybreak)
>>>> size = copybreak;
>>>> if (size< ETH_HLEN)
>>>> size = ETH_HLEN;
>>>>
>>>>
>>>> Why would increasing size to copybreak be valid here?
>>>>
>>>> Guessing a bit as I'm not sure about rx_nfrags, but if I read this
>>>> correctly, if size is ever less than copybreak it's because there isn't
>>>> enough space left for anything larger. If so, wouldn't increasing size
>>>> potentially corrupt something? I'd further guess that the resulting
>>>> condition manifests sooner (or at least with a more visible effect) when
>>>> using DMAR.
>>>>
>>>> In any event, why "copybreak" as the minimum buffer size? I'd suggest
>>>> that if it isn't possible to allocate at least MTU + overhead that
>>>> sky2_rx_start ought to be delayed until there is room.
>>>>
>>>>
>>> This code is where driver decides how much data will be received in skb
>>> data area and the remaining data spills over into skb frags.
>>> Copybreak is the threshold so that packets less than size are copied
>>> to a new skb. The code doing the copying there assumes the data is
>>> totally contained in the skb (not in frags). The size increase there
>>> is to make sure that assumption is always true. I suppose you
>>> could do something perverse like setting copybreak really huge
>>> and confuse driver, but that is a user error.
>>>
>>>
>>>
>> Ok - but I'm wondering under what circumstances size would be<
>> copybreak in the first place after computing the residue. If size ends
>> up being unreasonably small, is simply increasing the number to whatever
>> copybreak is correct? Assuming my testing is correct, then the crash
>> I've been experiencing when using dmar (only) seems related to the value
>> of copybreak. I don't think the other use (skb reuse) is the issue (but
>> hey, I could have missed something). The crash occurs when copybreak is
>> the default of 128, didn't happen when I set copybreak to 1.
>>
>>
> Setting it to 1 causes driver to never go through the dma_sync_single/memcpy
> path. Perhaps the code for DMAR doesn't do dma_sync_single_for_cpu
> properly, or the value passed to sync_single_for_cpu doesn't account for
> all the overhead of padding and/or ether header.
>
>
Ah - ok... will poke around there... if you have any suggestions,
diagnostics, whatever, let me know. Also, just an FYI - before rebooting
with copybreak back to defaults, I tried mtu=9000 again. That hung the
server immediately - no diagnostic output - system froze until watchdog
rebooted. Don't know right now if the copybreak had anything to do with
this, but when I've tried in the past I've had errors on sky2, but never
crashed the system like this. Only two things different were copybreak
and the length of time the system had been up. I'll try later with
copybreak default and copybreak=1 to see if that affects mtu behavior.
^ permalink raw reply
* Re: Hang: 2.6.32.4 sky2/DMAR (was [PATCH] sky2: Fix WARNING: at lib/dma-debug.c:902 check_sync)
From: Michael Breuer @ 2010-01-27 17:58 UTC (permalink / raw)
To: Stephen Hemminger
Cc: Jarek Poplawski, David Miller, akpm, flyboy, linux-kernel, netdev,
Michael Chan, Don Fry, Francois Romieu, Matt Carlson
In-Reply-To: <20100127095614.14313677@nehalam>
On 1/27/2010 12:56 PM, Stephen Hemminger wrote:
> On Wed, 27 Jan 2010 11:57:35 -0500
> Michael Breuer<mbreuer@majjas.com> wrote:
>
>
>> On 1/27/2010 11:50 AM, Stephen Hemminger wrote:
>>
>>> On Wed, 27 Jan 2010 10:34:51 -0500
>>> Michael Breuer<mbreuer@majjas.com> wrote:
>>>
>>>
>>>
>>>> On 01/23/2010 06:21 PM, Jarek Poplawski wrote:
>>>>
>>>>
>>>>> On Fri, Jan 22, 2010 at 06:50:21PM -0500, Michael Breuer wrote:
>>>>>
>>>>>
>>>>>
>>>>>> When the packets were dropped, there was a different sequence in the
>>>>>> log - DISCOVER/OFFER repeated. The "normal" is that the sequence
>>>>>> appeared correct and complete - DISCOVER/OFFER/REQUEST/ACK - or
>>>>>> INFORM/ACK (vs. INFORM repeatedly sans ACK) as the case may be.
>>>>>>
>>>>>>
>>>>>>
>>>>> Anyway, I'd be intersted if the switch matters here.
>>>>>
>>>>> Plus one more test: could you try to load sky2 with the parameter:
>>>>> "copybreak=1" (the rest as in any recent test, which gave you dmar
>>>>> errors; any switch).
>>>>>
>>>>> Thanks,
>>>>> Jarek P.
>>>>>
>>>>>
>>>>>
>>>> Ok - now up 80+ hours with copybreak=1. I'm going to redo w/o copybreak
>>>> to confirm that I haven't inadvertently fixed something. However, given
>>>> that it might be copybreak-related, I looked at sky2.c again and I'm
>>>> wondering about the copybreak max size in sky2_rx_start:
>>>>
>>>> size = roundup(sky2->netdev->mtu + ETH_HLEN + VLAN_HLEN, 8);
>>>>
>>>> /* Stopping point for hardware truncation */
>>>> thresh = (size - 8) / sizeof(u32);
>>>>
>>>> sky2->rx_nfrags = size>> PAGE_SHIFT;
>>>> BUG_ON(sky2->rx_nfrags> ARRAY_SIZE(re->frag_addr));
>>>>
>>>> /* Compute residue after pages */
>>>> size -= sky2->rx_nfrags<< PAGE_SHIFT;
>>>>
>>>> /* Optimize to handle small packets and headers */
>>>> if (size< copybreak)
>>>> size = copybreak;
>>>> if (size< ETH_HLEN)
>>>> size = ETH_HLEN;
>>>>
>>>>
>>>> Why would increasing size to copybreak be valid here?
>>>>
>>>> Guessing a bit as I'm not sure about rx_nfrags, but if I read this
>>>> correctly, if size is ever less than copybreak it's because there isn't
>>>> enough space left for anything larger. If so, wouldn't increasing size
>>>> potentially corrupt something? I'd further guess that the resulting
>>>> condition manifests sooner (or at least with a more visible effect) when
>>>> using DMAR.
>>>>
>>>> In any event, why "copybreak" as the minimum buffer size? I'd suggest
>>>> that if it isn't possible to allocate at least MTU + overhead that
>>>> sky2_rx_start ought to be delayed until there is room.
>>>>
>>>>
>>> This code is where driver decides how much data will be received in skb
>>> data area and the remaining data spills over into skb frags.
>>> Copybreak is the threshold so that packets less than size are copied
>>> to a new skb. The code doing the copying there assumes the data is
>>> totally contained in the skb (not in frags). The size increase there
>>> is to make sure that assumption is always true. I suppose you
>>> could do something perverse like setting copybreak really huge
>>> and confuse driver, but that is a user error.
>>>
>>>
>>>
>> Ok - but I'm wondering under what circumstances size would be<
>> copybreak in the first place after computing the residue. If size ends
>> up being unreasonably small, is simply increasing the number to whatever
>> copybreak is correct? Assuming my testing is correct, then the crash
>> I've been experiencing when using dmar (only) seems related to the value
>> of copybreak. I don't think the other use (skb reuse) is the issue (but
>> hey, I could have missed something). The crash occurs when copybreak is
>> the default of 128, didn't happen when I set copybreak to 1.
>>
> Does this change it? If so the dma code is (not sky2) is buggy and not
> rounding up properly.
>
> --- a/drivers/net/sky2.c 2010-01-27 09:46:10.940005248 -0800
> +++ b/drivers/net/sky2.c 2010-01-27 09:53:47.141267850 -0800
> @@ -2257,13 +2257,16 @@ static struct sk_buff *receive_copy(stru
>
> skb = netdev_alloc_skb_ip_align(sky2->netdev, length);
> if (likely(skb)) {
> + unsigned dma_align = dma_get_cache_alignment();
> + unsigned dma_size = ALIGN(length+1, dma_align);
> +
> pci_dma_sync_single_for_cpu(sky2->hw->pdev, re->data_addr,
> - length, PCI_DMA_FROMDEVICE);
> + dma_size, PCI_DMA_FROMDEVICE);
> skb_copy_from_linear_data(re->skb, skb->data, length);
> skb->ip_summed = re->skb->ip_summed;
> skb->csum = re->skb->csum;
> pci_dma_sync_single_for_device(sky2->hw->pdev, re->data_addr,
> - length, PCI_DMA_FROMDEVICE);
> + dma_size, PCI_DMA_FROMDEVICE);
> re->skb->ip_summed = CHECKSUM_NONE;
> skb_put(skb, length);
> }
>
Ok - will queue this - want to reconfirm that the system still crashes
w/o this (or copybreak). That should take a few days.
^ permalink raw reply
* Re: [PATCH net-next-2.6] packet: Add GSO/checksum offload support to af_packet sockets
From: Michael S. Tsirkin @ 2010-01-27 18:02 UTC (permalink / raw)
To: Sridhar Samudrala; +Cc: David Miller, Rusty Russell, Herbert Xu, netdev
In-Reply-To: <1264614157.20320.16.camel@w-sridhar.beaverton.ibm.com>
On Wed, Jan 27, 2010 at 09:42:37AM -0800, Sridhar Samudrala wrote:
> On Wed, 2010-01-27 at 13:42 +0200, Michael S. Tsirkin wrote:
> > On Tue, Jan 26, 2010 at 12:30:19PM -0800, Sridhar Samudrala wrote:
> > > This patch adds GSO/checksum offload to af_packet sockets using
> > > virtio_net_hdr. Based on Rusty's patch to add this support to tun.
> > > It allows GSO/checksum offload to be enabled when using raw socket
> > > backend with virtio_net.
> > > Adds PACKET_VNET_HDR socket option to prepend virtio_net_hdr in the
> > > receive path and process/skip virtio_net_hdr in the send path. This
> > > option is only allowed with SOCK_RAW sockets attached to ethernet
> > > type devices.
> > >
> > > Signed-off-by: Sridhar Samudrala <sri@us.ibm.com>
> >
> > So the main issue with this implemenation is that it silently fails for
> > non-ethernet protocols. It would be better to detect unsupported
> > protocols and return an error to user.
>
> Yes. I could return EINVAL or EOPNOTSUPP when trying to send/receive a
> packet with virtio_net_hdr on non-ethernet devices.
non-ethernet *protocols* are at issue here.
> > This is same issue that was
> > pointed out by DaveM with my earlier attempt to solve a different
> > (related) problem:
> > http://lkml.org/lkml/2010/1/5/474
> > For an incomplete prototype attempting to solve the issue in a generic way:
> > http://lkml.org/lkml/2010/1/6/56
> >
> > A couple of additional comments below.
> >
> > > diff --git a/include/linux/if_packet.h b/include/linux/if_packet.h
> > > index 4021d47..aa57a5f 100644
> > > --- a/include/linux/if_packet.h
> > > +++ b/include/linux/if_packet.h
> > > @@ -46,6 +46,7 @@ struct sockaddr_ll {
> > > #define PACKET_RESERVE 12
> > > #define PACKET_TX_RING 13
> > > #define PACKET_LOSS 14
> > > +#define PACKET_VNET_HDR 15
> > >
> > > struct tpacket_stats {
> > > unsigned int tp_packets;
> > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> > > index 53633c5..36d5360 100644
> > > --- a/net/packet/af_packet.c
> > > +++ b/net/packet/af_packet.c
> > > @@ -80,6 +80,8 @@
> > > #include <linux/init.h>
> > > #include <linux/mutex.h>
> > > #include <linux/if_vlan.h>
> > > +#include <linux/virtio_net.h>
> > > +#include <linux/if_arp.h>
> > >
> > > #ifdef CONFIG_INET
> > > #include <net/inet_common.h>
> > > @@ -193,7 +195,8 @@ struct packet_sock {
> > > struct mutex pg_vec_lock;
> > > unsigned int running:1, /* prot_hook is attached*/
> > > auxdata:1,
> > > - origdev:1;
> > > + origdev:1,
> > > + vnet_hdr:1;
> > > int ifindex; /* bound device */
> > > __be16 num;
> > > struct packet_mclist *mclist;
> > > @@ -1056,6 +1059,30 @@ out:
> > > }
> > > #endif
> > >
> > > +static inline struct sk_buff *packet_alloc_skb(struct sock *sk, size_t prepad,
> > > + size_t reserve, size_t len,
> > > + size_t linear, int noblock,
> > > + int *err)
> > > +{
> > > + struct sk_buff *skb;
> > > +
> > > + /* Under a page? Don't bother with paged skb. */
> > > + if (prepad + len < PAGE_SIZE || !linear)
> > > + linear = len;
> > > +
> > > + skb = sock_alloc_send_pskb(sk, prepad + linear, len - linear, noblock,
> > > + err);
> > > + if (!skb)
> > > + return NULL;
> > > +
> > > + skb_reserve(skb, reserve);
> > > + skb_put(skb, linear);
> > > + skb->data_len = len - linear;
> > > + skb->len += len - linear;
> > > +
> > > + return skb;
> > > +}
> > > +
> > > static int packet_snd(struct socket *sock,
> > > struct msghdr *msg, size_t len)
> > > {
> > > @@ -1066,14 +1093,15 @@ static int packet_snd(struct socket *sock,
> > > __be16 proto;
> > > unsigned char *addr;
> > > int ifindex, err, reserve = 0;
> > > + struct virtio_net_hdr vnethdr = { 0 };
> > > + int offset = 0;
> > > + struct packet_sock *po = pkt_sk(sk);
> > >
> > > /*
> > > * Get and verify the address.
> > > */
> > >
> > > if (saddr == NULL) {
> > > - struct packet_sock *po = pkt_sk(sk);
> > > -
> > > ifindex = po->ifindex;
> > > proto = po->num;
> > > addr = NULL;
> > > @@ -1100,25 +1128,52 @@ static int packet_snd(struct socket *sock,
> > > if (!(dev->flags & IFF_UP))
> > > goto out_unlock;
> > >
> > > - err = -EMSGSIZE;
> > > - if (len > dev->mtu+reserve)
> > > - goto out_unlock;
> > > + if (po->vnet_hdr) {
> > > + err = -EINVAL;
> > > + if (dev->type != ARPHRD_ETHER)
> > > + goto out_unlock;
> > > +
> > > + if (len < sizeof(vnethdr))
> > > + goto out_unlock;
> > >
> > > - skb = sock_alloc_send_skb(sk, len + LL_ALLOCATED_SPACE(dev),
> > > - msg->msg_flags & MSG_DONTWAIT, &err);
> > > + len -= sizeof(vnethdr);
> > > +
> > > + err = -EFAULT;
> > > + if (memcpy_fromiovec((void *)&vnethdr, msg->msg_iov,
> > > + sizeof(vnethdr)))
> > > + goto out_unlock;
> > > +
> > > + if ((vnethdr.flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) &&
> > > + (vnethdr.csum_start + vnethdr.csum_offset + 2 >
> > > + vnethdr.hdr_len))
> > > + vnethdr.hdr_len = vnethdr.csum_start +
> > > + vnethdr.csum_offset + 2;
> > > +
> > > + err = -EINVAL;
> > > + if (vnethdr.hdr_len > len)
> > > + goto out_unlock;
> > > + } else {
> > > + err = -EMSGSIZE;
> > > + if (len > dev->mtu+reserve)
> > > + goto out_unlock;
> >
> > IMO we should always perform the length check if GSO is off.
> OK. I will fix this.
> >
> > > + }
> > > +
> > > + err = -ENOBUFS;
> > > + skb = packet_alloc_skb(sk, LL_ALLOCATED_SPACE(dev),
> > > + LL_RESERVED_SPACE(dev), len, vnethdr.hdr_len,
> > > + msg->msg_flags & MSG_DONTWAIT, &err);
> > > if (skb == NULL)
> > > goto out_unlock;
> > >
> > > - skb_reserve(skb, LL_RESERVED_SPACE(dev));
> > > - skb_reset_network_header(skb);
> > > + skb_set_network_header(skb, reserve);
> >
> > I think the above is wrong for vlans?
>
> I also thought we need to address vlans here, but even tun doesn't
> handle this in the send routine. I submitted a patch that fixed
> skb_gso_segment() to handle vlan packets. This will address both
> tun and packet sockets.
> http://thread.gmane.org/gmane.linux.network/150198
>
> With this patch, i tested vlans with both ipv4 and ipv6.
Well, there are more protocols than just vlans :)
BTW, if there are dependencies between patches, you probably
want to put them in a patchset so they can be judged together.
> >
> > >
> > > err = -EINVAL;
> > > if (sock->type == SOCK_DGRAM &&
> > > - dev_hard_header(skb, dev, ntohs(proto), addr, NULL, len) < 0)
> > > + (offset = dev_hard_header(skb, dev, ntohs(proto), addr, NULL, len)) < 0)
> > > goto out_free;
> > >
> > > /* Returns -EFAULT on error */
> > > - err = memcpy_fromiovec(skb_put(skb, len), msg->msg_iov, len);
> > > + err = skb_copy_datagram_from_iovec(skb, offset, msg->msg_iov, 0, len);
> > > if (err)
> > > goto out_free;
> > >
> > > @@ -1127,6 +1182,51 @@ static int packet_snd(struct socket *sock,
> > > skb->priority = sk->sk_priority;
> > > skb->mark = sk->sk_mark;
> > >
> > > + if (po->vnet_hdr) {
> > > + skb_reset_mac_header(skb);
> > > + skb->protocol = eth_hdr(skb)->h_proto;
> > > +
> >
> > Is this also broken for vlans?
>
> Same as above.
> >
> > > + if (vnethdr.flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) {
> > > + if (!skb_partial_csum_set(skb, vnethdr.csum_start,
> > > + vnethdr.csum_offset)) {
> > > + err = -EINVAL;
> > > + goto out_free;
> > > + }
> > > + }
> > > +
> > > + if (vnethdr.gso_type != VIRTIO_NET_HDR_GSO_NONE) {
> > > + switch (vnethdr.gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
> > > + case VIRTIO_NET_HDR_GSO_TCPV4:
> > > + skb_shinfo(skb)->gso_type = SKB_GSO_TCPV4;
> > > + break;
> > > + case VIRTIO_NET_HDR_GSO_TCPV6:
> > > + skb_shinfo(skb)->gso_type = SKB_GSO_TCPV6;
> > > + break;
> > > + case VIRTIO_NET_HDR_GSO_UDP:
> > > + skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
> > > + break;
> > > + default:
> > > + err = -EINVAL;
> > > + goto out_free;
> > > + }
> > > +
> > > + if (vnethdr.gso_type & VIRTIO_NET_HDR_GSO_ECN)
> > > + skb_shinfo(skb)->gso_type |= SKB_GSO_TCP_ECN;
> > > +
> > > + skb_shinfo(skb)->gso_size = vnethdr.gso_size;
> > > + if (skb_shinfo(skb)->gso_size == 0) {
> > > + err = -EINVAL;
> > > + goto out_free;
> > > + }
> > > +
> > > + /* Header must be checked, and gso_segs computed. */
> > > + skb_shinfo(skb)->gso_type |= SKB_GSO_DODGY;
> > > + skb_shinfo(skb)->gso_segs = 0;
> > > + }
> > > +
> > > + len += sizeof(vnethdr);
> > > + }
> > > +
> > > /*
> > > * Now send it
> > > */
> > > @@ -1420,6 +1520,7 @@ static int packet_recvmsg(struct kiocb *iocb, struct socket *sock,
> > > struct sk_buff *skb;
> > > int copied, err;
> > > struct sockaddr_ll *sll;
> > > + int vnet_hdr_len = 0;
> > >
> > > err = -EINVAL;
> > > if (flags & ~(MSG_PEEK|MSG_DONTWAIT|MSG_TRUNC|MSG_CMSG_COMPAT))
> > > @@ -1451,6 +1552,44 @@ static int packet_recvmsg(struct kiocb *iocb, struct socket *sock,
> > > if (skb == NULL)
> > > goto out;
> > >
> > > + if (pkt_sk(sk)->vnet_hdr) {
> > > + struct virtio_net_hdr vnethdr = { 0 };
> > > +
> > > + vnet_hdr_len = sizeof(vnethdr);
> > > + if ((len -= vnet_hdr_len) < 0)
> > > + return -EINVAL;
> > > +
> > > + if (skb_is_gso(skb)) {
> > > + struct skb_shared_info *sinfo = skb_shinfo(skb);
> > > +
> > > + /* This is a hint as to how much should be linear. */
> > > + vnethdr.hdr_len = skb_headlen(skb);
> > > + vnethdr.gso_size = sinfo->gso_size;
> > > + if (sinfo->gso_type & SKB_GSO_TCPV4)
> > > + vnethdr.gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
> > > + else if (sinfo->gso_type & SKB_GSO_TCPV6)
> > > + vnethdr.gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
> > > + else if (sinfo->gso_type & SKB_GSO_UDP)
> > > + vnethdr.gso_type = VIRTIO_NET_HDR_GSO_UDP;
> > > + else
> > > + BUG();
> >
> > Is there any chance this can get SKB_GSO_FCOE by binding to
> > an appropriate interface? Maybe we don't want to BUG().
> I could return -EINVAL in that case.
>
> >
> > > + if (sinfo->gso_type & SKB_GSO_TCP_ECN)
> > > + vnethdr.gso_type |= VIRTIO_NET_HDR_GSO_ECN;
> > > + } else
> > > + vnethdr.gso_type = VIRTIO_NET_HDR_GSO_NONE;
> > > +
> > > + if (skb->ip_summed == CHECKSUM_PARTIAL) {
> > > + vnethdr.flags = VIRTIO_NET_HDR_F_NEEDS_CSUM;
> > > + vnethdr.csum_start = skb->csum_start - skb_headroom(skb);
> > > + vnethdr.csum_offset = skb->csum_offset;
> > > + } /* else everything is zero */
> > > +
> > > + if (unlikely(memcpy_toiovec(msg->msg_iov, (void *)&vnethdr,
> > > + sizeof(vnethdr)))) {
> > > + return -EFAULT;
> > > + }
> > > + }
> > > +
> > > /*
> > > * If the address length field is there to be filled in, we fill
> > > * it in now.
> > > @@ -1502,7 +1641,7 @@ static int packet_recvmsg(struct kiocb *iocb, struct socket *sock,
> > > * Free or return the buffer as appropriate. Again this
> > > * hides all the races and re-entrancy issues from us.
> > > */
> > > - err = (flags&MSG_TRUNC) ? skb->len : copied;
> > > + err = vnet_hdr_len + ((flags&MSG_TRUNC) ? skb->len : copied);
> > >
> > > out_free:
> > > skb_free_datagram(sk, skb);
> > > @@ -1826,6 +1965,22 @@ packet_setsockopt(struct socket *sock, int level, int optname, char __user *optv
> > > po->origdev = !!val;
> > > return 0;
> > > }
> > > + case PACKET_VNET_HDR:
> > > + {
> > > + int val;
> > > +
> > > + if (sock->type != SOCK_RAW)
> > > + return -EINVAL;
> > > + if (po->rx_ring.pg_vec || po->tx_ring.pg_vec)
> > > + return -EBUSY;
> >
> > Another way to get a broken ring + vnet hdr configuration
> > would be to enable vnet hdr first and mmap second.
> > I think we need to guard against this as well, by checking vnet_hdr
> > when tx/rx ring is enabled.
>
> OK. i will add a check when setting PACKET_RX_RING/TX_RING socket
> options.
> > > + if (optlen < sizeof(val))
> > > + return -EINVAL;
> > > + if (copy_from_user(&val, optval, sizeof(val)))
> > > + return -EFAULT;
> > > +
> > > + po->vnet_hdr = !!val;
> > > + return 0;
> > > + }
> > > default:
> > > return -ENOPROTOOPT;
> > > }
> > > @@ -1876,6 +2031,13 @@ static int packet_getsockopt(struct socket *sock, int level, int optname,
> > >
> > > data = &val;
> > > break;
> > > + case PACKET_VNET_HDR:
> > > + if (len > sizeof(int))
> > > + len = sizeof(int);
> > > + val = po->vnet_hdr;
> > > +
> > > + data = &val;
> > > + break;
> > > #ifdef CONFIG_PACKET_MMAP
> > > case PACKET_VERSION:
> > > if (len > sizeof(int))
> > >
> >
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: Hang: 2.6.32.4 sky2/DMAR (was [PATCH] sky2: Fix WARNING: at lib/dma-debug.c:902 check_sync)
From: Michael Breuer @ 2010-01-27 18:08 UTC (permalink / raw)
To: Stephen Hemminger
Cc: Jarek Poplawski, David Miller, akpm, flyboy, linux-kernel, netdev,
Michael Chan, Don Fry, Francois Romieu, Matt Carlson
In-Reply-To: <20100127095614.14313677@nehalam>
On 01/27/2010 12:56 PM, Stephen Hemminger wrote:
> --- a/drivers/net/sky2.c 2010-01-27 09:46:10.940005248 -0800
> +++ b/drivers/net/sky2.c 2010-01-27 09:53:47.141267850 -0800
> @@ -2257,13 +2257,16 @@ static struct sk_buff *receive_copy(stru
>
> skb = netdev_alloc_skb_ip_align(sky2->netdev, length);
> if (likely(skb)) {
> + unsigned dma_align = dma_get_cache_alignment();
> + unsigned dma_size = ALIGN(length+1, dma_align);
> +
> pci_dma_sync_single_for_cpu(sky2->hw->pdev, re->data_addr,
> - length, PCI_DMA_FROMDEVICE);
> + dma_size, PCI_DMA_FROMDEVICE);
> skb_copy_from_linear_data(re->skb, skb->data, length);
> skb->ip_summed = re->skb->ip_summed;
> skb->csum = re->skb->csum;
> pci_dma_sync_single_for_device(sky2->hw->pdev, re->data_addr,
> - length, PCI_DMA_FROMDEVICE);
> + dma_size, PCI_DMA_FROMDEVICE);
> re->skb->ip_summed = CHECKSUM_NONE;
> skb_put(skb, length);
> }
>
This doesn't apply - I'm missing some intermediate patch.
I've got (both in 2.6.32.4 and 2.6.33-rc5: pci_unmap_len(re, data_size)
vs., "length." I assume that I can just replace the pci_unmap_len with
dma_size... but perhaps the intermediate change may have affected this
as well?
^ permalink raw reply
* Re: Hang: 2.6.32.4 sky2/DMAR (was [PATCH] sky2: Fix WARNING: at lib/dma-debug.c:902 check_sync)
From: Michael Breuer @ 2010-01-27 18:33 UTC (permalink / raw)
To: Stephen Hemminger
Cc: Jarek Poplawski, David Miller, akpm, flyboy, linux-kernel, netdev,
Michael Chan, Don Fry, Francois Romieu, Matt Carlson
In-Reply-To: <4B607E70.4060402@majjas.com>
On 01/27/2010 12:57 PM, Michael Breuer wrote:
> On 1/27/2010 12:45 PM, Stephen Hemminger wrote:
>> On Wed, 27 Jan 2010 11:57:35 -0500
>> Michael Breuer<mbreuer@majjas.com> wrote:
>>
>>
> Ah - ok... will poke around there... if you have any suggestions,
> diagnostics, whatever, let me know. Also, just an FYI - before
> rebooting with copybreak back to defaults, I tried mtu=9000 again.
> That hung the server immediately - no diagnostic output - system froze
> until watchdog rebooted. Don't know right now if the copybreak had
> anything to do with this, but when I've tried in the past I've had
> errors on sky2, but never crashed the system like this. Only two
> things different were copybreak and the length of time the system had
> been up. I'll try later with copybreak default and copybreak=1 to see
> if that affects mtu behavior.
>
FYI - just redid this a few times. Looks like it's how long the system
was up, not copybreak wrt crash on resetting MTU.
That said, while the system seems OK after resetting the MTU, I do get a
WARNING from netdev watchdog - same warning regardless of copybreak.
Setting the mtu back to 1500 generates rx errors after which things
work. Going back to 9000 again does not generate new errors.
Jan 27 13:21:54 mail kernel: ------------[ cut here ]------------
Jan 27 13:21:54 mail kernel: WARNING: at net/sched/sch_generic.c:261
dev_watchdog+0xf3/0x164()
Jan 27 13:21:54 mail kernel: Hardware name: System Product Name
Jan 27 13:21:54 mail kernel: NETDEV WATCHDOG: eth0 (sky2): transmit
queue 0 timed out
Jan 27 13:21:54 mail kernel: Modules linked in: microcode(+)
ip6table_mangle ip6table_filter ip6_tables ipt_MASQUERADE iptable_nat
nf_nat iptable_mangle iptable_raw bridge stp appletalk psnap llc nfsd
lockd nfs_acl auth_rpcgss exportfs hwmon_vid coretemp sunrpc
acpi_cpufreq sit tunnel4 ipt_LOG nf_conntrack_netbios_ns
nf_conntrack_ftp nf_conntrack_ipv6 xt_multiport xt_DSCP xt_dscp xt_MARK
ipv6 dm_multipath kvm_intel kvm snd_hda_codec_analog snd_ens1371
gameport snd_rawmidi snd_ac97_codec snd_hda_intel snd_hda_codec
snd_hwdep ac97_bus snd_seq gspca_spca505 gspca_main videodev
snd_seq_device asus_atk0110 v4l1_compat snd_pcm hwmon
v4l2_compat_ioctl32 pcspkr i2c_i801 firewire_ohci firewire_core
crc_itu_t snd_timer snd soundcore wmi snd_page_alloc sky2 iTCO_wdt
iTCO_vendor_support fbcon tileblit font bitblit softcursor raid456
async_raid6_recov async_pq raid6_pq async_xor xor async_memcpy async_tx
raid1 ata_generic pata_acpi pata_marvell nouveau ttm drm_kms_helper drm
agpgart fb i2c_algo_bit cfbcopyarea i2c_core cfb
Jan 27 13:21:54 mail kernel: imgblt cfbfillrect [last unloaded: ip6_tables]
Jan 27 13:21:54 mail kernel: Pid: 0, comm: swapper Tainted: G W
2.6.32.4MMAPDMARAF3SKY2PSKBMAYPULL-00912-g914160d-dirty #6
Jan 27 13:21:54 mail kernel: Call Trace:
Jan 27 13:21:54 mail kernel: <IRQ> [<ffffffff810536ee>]
warn_slowpath_common+0x7c/0x94
Jan 27 13:21:54 mail kernel: [<ffffffff8105375d>]
warn_slowpath_fmt+0x41/0x43
Jan 27 13:21:54 mail kernel: [<ffffffff813e3b6b>] ? netif_tx_lock+0x44/0x6c
Jan 27 13:21:54 mail kernel: [<ffffffff813e3cd3>] dev_watchdog+0xf3/0x164
Jan 27 13:21:54 mail kernel: [<ffffffff8106e990>] ? __queue_work+0x3a/0x42
Jan 27 13:21:54 mail kernel: [<ffffffff8106323f>]
run_timer_softirq+0x1c8/0x270
Jan 27 13:21:54 mail kernel: [<ffffffff8105af0f>] __do_softirq+0xf8/0x1cd
Jan 27 13:21:54 mail kernel: [<ffffffff8107f0ab>] ?
tick_program_event+0x2a/0x2c
Jan 27 13:21:54 mail kernel: [<ffffffff81012e1c>] call_softirq+0x1c/0x30
Jan 27 13:21:54 mail kernel: [<ffffffff810143a3>] do_softirq+0x4b/0xa6
Jan 27 13:21:54 mail kernel: [<ffffffff8105aaef>] irq_exit+0x4a/0x8c
Jan 27 13:21:54 mail kernel: [<ffffffff81470612>]
smp_apic_timer_interrupt+0x86/0x94
Jan 27 13:21:54 mail kernel: [<ffffffff810127e3>]
apic_timer_interrupt+0x13/0x20
Jan 27 13:21:54 mail kernel: <EOI> [<ffffffff812c729a>] ?
acpi_idle_enter_bm+0x256/0x28a
Jan 27 13:21:54 mail kernel: [<ffffffff812c7293>] ?
acpi_idle_enter_bm+0x24f/0x28a
Jan 27 13:21:54 mail kernel: [<ffffffff813a6c3c>] ?
cpuidle_idle_call+0x9e/0xfa
Jan 27 13:21:54 mail kernel: [<ffffffff81010c90>] ? cpu_idle+0xb4/0xf6
Jan 27 13:21:54 mail kernel: [<ffffffff81465ba5>] ?
start_secondary+0x201/0x242
Jan 27 13:21:54 mail kernel: ---[ end trace 57f7151f6a5def07 ]---
Jan 27 13:21:54 mail kernel: sky2 eth0: tx timeout
Jan 27 13:21:54 mail kernel: sky2 eth0: transmit ring 51 .. 10 report=51
done=51
Jan 27 13:21:54 mail kernel: sky2 eth0: disabling interface
Jan 27 13:21:54 mail kernel: sky2 eth0: enabling interface
^ permalink raw reply
* Re: Hang: 2.6.32.4 sky2/DMAR (was [PATCH] sky2: Fix WARNING: at lib/dma-debug.c:902 check_sync)
From: Michael Breuer @ 2010-01-27 18:45 UTC (permalink / raw)
To: Stephen Hemminger
Cc: Jarek Poplawski, David Miller, akpm, flyboy, linux-kernel, netdev,
Michael Chan, Don Fry, Francois Romieu, Matt Carlson
In-Reply-To: <4B608128.7090607@majjas.com>
On 1/27/2010 1:08 PM, Michael Breuer wrote:
> On 01/27/2010 12:56 PM, Stephen Hemminger wrote:
>> --- a/drivers/net/sky2.c 2010-01-27 09:46:10.940005248 -0800
>> +++ b/drivers/net/sky2.c 2010-01-27 09:53:47.141267850 -0800
>> @@ -2257,13 +2257,16 @@ static struct sk_buff *receive_copy(stru
>>
>> skb = netdev_alloc_skb_ip_align(sky2->netdev, length);
>> if (likely(skb)) {
>> + unsigned dma_align = dma_get_cache_alignment();
>> + unsigned dma_size = ALIGN(length+1, dma_align);
>> +
>> pci_dma_sync_single_for_cpu(sky2->hw->pdev, re->data_addr,
>> - length, PCI_DMA_FROMDEVICE);
>> + dma_size, PCI_DMA_FROMDEVICE);
>> skb_copy_from_linear_data(re->skb, skb->data, length);
>> skb->ip_summed = re->skb->ip_summed;
>> skb->csum = re->skb->csum;
>> pci_dma_sync_single_for_device(sky2->hw->pdev, re->data_addr,
>> - length, PCI_DMA_FROMDEVICE);
>> + dma_size, PCI_DMA_FROMDEVICE);
>> re->skb->ip_summed = CHECKSUM_NONE;
>> skb_put(skb, length);
>> }
> This doesn't apply - I'm missing some intermediate patch.
>
> I've got (both in 2.6.32.4 and 2.6.33-rc5: pci_unmap_len(re,
> data_size) vs., "length." I assume that I can just replace the
> pci_unmap_len with dma_size... but perhaps the intermediate change may
> have affected this as well?
>
Never mind - that was from one of the earlier patches I had been trying
out. will try the above patch after reestablishing that the system still
crashes without copybreak=1.
^ permalink raw reply
* Re: Hang: 2.6.32.4 sky2/DMAR (was [PATCH] sky2: Fix WARNING: at lib/dma-debug.c:902 check_sync)
From: Jarek Poplawski @ 2010-01-27 19:23 UTC (permalink / raw)
To: Michael Breuer
Cc: Stephen Hemminger, David Miller, akpm, flyboy, linux-kernel,
netdev, Michael Chan, Don Fry, Francois Romieu, Matt Carlson
In-Reply-To: <4B6089C7.4010803@majjas.com>
On Wed, Jan 27, 2010 at 01:45:27PM -0500, Michael Breuer wrote:
> On 1/27/2010 1:08 PM, Michael Breuer wrote:
> >On 01/27/2010 12:56 PM, Stephen Hemminger wrote:
> >>--- a/drivers/net/sky2.c 2010-01-27 09:46:10.940005248 -0800
> >>+++ b/drivers/net/sky2.c 2010-01-27 09:53:47.141267850 -0800
> >>@@ -2257,13 +2257,16 @@ static struct sk_buff *receive_copy(stru
> >>
> >> skb = netdev_alloc_skb_ip_align(sky2->netdev, length);
> >> if (likely(skb)) {
> >>+ unsigned dma_align = dma_get_cache_alignment();
> >>+ unsigned dma_size = ALIGN(length+1, dma_align);
> >>+
> >> pci_dma_sync_single_for_cpu(sky2->hw->pdev, re->data_addr,
> >>- length, PCI_DMA_FROMDEVICE);
> >>+ dma_size, PCI_DMA_FROMDEVICE);
> >> skb_copy_from_linear_data(re->skb, skb->data, length);
> >> skb->ip_summed = re->skb->ip_summed;
> >> skb->csum = re->skb->csum;
> >> pci_dma_sync_single_for_device(sky2->hw->pdev, re->data_addr,
> >>- length, PCI_DMA_FROMDEVICE);
> >>+ dma_size, PCI_DMA_FROMDEVICE);
> >> re->skb->ip_summed = CHECKSUM_NONE;
> >> skb_put(skb, length);
> >> }
> >This doesn't apply - I'm missing some intermediate patch.
> >
> >I've got (both in 2.6.32.4 and 2.6.33-rc5: pci_unmap_len(re,
> >data_size) vs., "length." I assume that I can just replace the
> >pci_unmap_len with dma_size... but perhaps the intermediate change
> >may have affected this as well?
> >
> Never mind - that was from one of the earlier patches I had been
> trying out. will try the above patch after reestablishing that the
> system still crashes without copybreak=1.
>
Stephen, I'm not sure this patch can show much after the patch with
"legal" dma_size == re->data_addr didn't help. It looks like David
was right: dma_sync can't affect dmar, because it doesn't use it at
all.
Then I'd rather suggest to test if using copybreak more often, e.g.
with copybreak=1000 or even more can trigger these errors faster.
Jarek P.
^ permalink raw reply
* Re: [PATCH] cxgb3/iw_cxgb3: doorbell overflow avoidance and recovery.
From: Roland Dreier @ 2010-01-27 19:30 UTC (permalink / raw)
To: Steve Wise
Cc: jeff-o2qLIJkoznsdnm+yROfE0A, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA, divy-ut6Up61K2wZBDgjK7y7TUQ
In-Reply-To: <20100127170333.19854.40039.stgit-T4OLL4TyM9aNDNWfRnPdfg@public.gmane.org>
Looks reasonable at first reading. I can take this through my tree,
assuming it's OK with you, Divy?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ 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