From: Michael Breuer on
On 1/27/2010 12:56 PM, Stephen Hemminger wrote:
> On Wed, 27 Jan 2010 11:57:35 -0500
> Michael Breuer<mbreuer(a)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(a)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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Michael Breuer on
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?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Michael Breuer on
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(a)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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Michael Breuer on
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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Jarek Poplawski on
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/