From: Arjan van de Ven <arjan@linux.intel.com> Subject: [PATCH] net: via-velocity.c fix sleep-with-spinlock bug during MTU change The via-velocity.c driver reinitializes (frees/allocates) several metadata structures during an MTU change. Unfortunately the allocations of the new versions of the metadata is done with GFP_KERNEL, even though this change of datastructures is (and needs to be) done while holding a spinlock (with irqs off). Clearly that isn't a good thing, and kerneloops.org has trapped a large deal of the resulting warnings. The fix is to use GFP_ATOMIC. While not elegant, avoiding the lock is going to be extremely complex. In addition, this is a "static", long lived allocation (after all, how often do you actually change your mtu) and not something that happens on an ongoing basis. Reported-by: www.kerneloops.org Signed-off-by: Arjan van de Ven <arjan@linux.intel.com> --- drivers/net/via-velocity.c | 12 +++++++++--- 1 files changed, 9 insertions(+), 3 deletions(-) diff --git a/drivers/net/via-velocity.c b/drivers/net/via-velocity.c index 6b8d882..4bf08fd 100644 --- a/drivers/net/via-velocity.c +++ b/drivers/net/via-velocity.c @@ -1,4 +1,4 @@ -/* +;/* * This code is derived from the VIA reference driver (copyright message * below) provided to Red Hat by VIA Networking Technologies, Inc. for * addition to the Linux kernel. @@ -1241,6 +1241,9 @@ static int velocity_rx_refill(struct velocity_info *vptr) * * Allocate and set up the receive buffers for each ring slot and * assign them to the network adapter. + * + * Note: This function gets called with irqs off/lock held + * from velocity_change_mtu() */ static int velocity_init_rd_ring(struct velocity_info *vptr) @@ -1251,7 +1254,7 @@ static int velocity_init_rd_ring(struct velocity_info *vptr) vptr->rx_buf_sz = (mtu <= ETH_DATA_LEN) ? PKT_BUF_SZ : mtu + 32; vptr->rd_info = kcalloc(vptr->options.numrx, - sizeof(struct velocity_rd_info), GFP_KERNEL); + sizeof(struct ...
On Sat, 31 May 2008 18:46:15 -0700 What happens if this allocation fails? I think the driver is dead? We've gone and freed the rd_ring and the td_ring and we _might_ have allocated a new rd_ring and not a new td_ring. And we've set vptr->rx_buf_sz, which may or may not be a problem. And we've gone and left the interface in a downed state. So hrm. It could all be a lot better. Just looking quickly at the code I _think_ we might be able to do all the needed allocations outside the lock and then swizzle them into place after taking the lock. ie, something as simple as: struct velocity_info *temp_vptr; ... velocity_init_rd_ring(temp_vptr); /* Can use GFP_KERNEL! */ spin_lock_irqsave(&vptr->lock, flags); velocity_free_td_ring(vptr); velocity_free_rd_ring(vptr); vptr->foo = temp_vptr->foo; vptr->bar = temp_vptr->bar; ... spin_unlock_irqrestore(&vptr->lock, flags); ? --
Andrew Morton <akpm@linux-foundation.org> : Almost. I'll wrap it in the next 24 hours. -- Ueimor --
Francois Romieu <romieu@fr.zoreil.com> : ... add 24 more as it starts to be ugly and I have not even considered velocity_init_rings. :o/ -- Ueimor --
Francois Romieu <romieu@fr.zoreil.com> : [...context available from http://lkml.org/lkml/2008/5/31/251 ...] Régis, I need your help. Can you give the patchkit below a try and check if the change of mtu works correctly ? http://userweb.kernel.org/~romieu/via-velocity/2.6.26-rc6/ or: git://git.kernel.org/pub/scm/linux/kernel/git/romieu/netdev-2.6.git velocity -- Ueimor --
With the patchkit, the driver doesn't work anymore. I think there is an error in TX, paquets don't arrive to other equipment. RX seem to work fine, I could see ingoming paquets. -- Régis --
With this modifications of your patchkit (the 0001), TX works again.
--- via-velocity.c 2008-06-17 00:43:03.000000000 +0200
+++ drivers/net/via-velocity.c 2008-06-17 00:43:56.000000000 +0200
@@ -2048,9 +2048,12 @@ static int velocity_xmit(struct sk_buff
int pktlen = skb->len;
__le16 len = cpu_to_le16(pktlen);
- if (skb_padto(skb, ETH_ZLEN))
- goto out;
-
+ if (pktlen < ETH_ZLEN)
+ {
+ if (skb_padto(skb, ETH_ZLEN))
+ goto out;
+ len = cpu_to_le16(ETH_ZLEN);
+ }
#ifdef VELOCITY_ZERO_COPY_SUPPORT
if (skb_shinfo(skb)->nr_frags > 6 && __skb_linearize(skb)) {
kfree_skb(skb);
Now, I'll try for mtu change
--
Régis
--
The mtu change doesn't work : Jun 16 23:22:34 apollo kernel: ------------[ cut here ]------------ Jun 16 23:22:34 apollo kernel: WARNING: at arch/x86/kernel/pci-dma.c:482 dma_free_coherent+0x3a/0x9c() Jun 16 23:22:34 apollo kernel: Pid: 1527, comm: ip Tainted: G W 2.6.26-rc6EPIA_SN_VB7001 #3 Jun 16 23:22:34 apollo kernel: [<c0115fe5>] warn_on_slowpath+0x3b/0x5f Jun 16 23:22:34 apollo kernel: [<c0133940>] get_page_from_freelist+0x24a/0x36f Jun 16 23:22:34 apollo kernel: [<c012d86b>] handle_IRQ_event+0x1a/0x3f Jun 16 23:22:34 apollo kernel: [<c0241193>] velocity_free_rd_ring+0xa5/0xb6 Jun 16 23:22:34 apollo kernel: [<c0144752>] kfree+0x6a/0x72 Jun 16 23:22:34 apollo kernel: [<c0241193>] velocity_free_rd_ring+0xa5/0xb6 Jun 16 23:22:34 apollo kernel: [<c0105816>] dma_free_coherent+0x3a/0x9c Jun 16 23:22:34 apollo kernel: [<c02411eb>] velocity_free_dma_rings+0x47/0x4d Jun 16 23:22:34 apollo kernel: [<c024225b>] velocity_change_mtu+0xf6/0x157 Jun 16 23:22:34 apollo kernel: [<c02856c4>] dev_set_mtu+0x2a/0x4f Jun 16 23:22:34 apollo kernel: [<c0286642>] dev_ioctl+0x4ab/0x530 Jun 16 23:22:34 apollo kernel: [<c012e4cb>] handle_fasteoi_irq+0x74/0x77 Jun 16 23:22:34 apollo kernel: [<c010479b>] do_IRQ+0x50/0x60 Jun 16 23:22:34 apollo kernel: [<c027d785>] sock_ioctl+0x0/0x177 Jun 16 23:22:34 apollo kernel: [<c01032eb>] common_interrupt+0x23/0x28 Jun 16 23:22:34 apollo kernel: [<c027d785>] sock_ioctl+0x0/0x177 Jun 16 23:22:34 apollo kernel: [<c0150b4a>] vfs_ioctl+0x16/0x48 Jun 16 23:22:34 apollo kernel: [<c0150d5a>] do_vfs_ioctl+0x1de/0x1f1 Jun 16 23:22:34 apollo kernel: [<c0150dae>] sys_ioctl+0x41/0x5b Jun 16 23:22:34 apollo kernel: [<c010291d>] sysenter_past_esp+0x6a/0x91 Jun 16 23:22:34 apollo kernel: ======================= Jun 16 23:22:34 apollo kernel: ---[ end trace 17457a054a24b83c ]--- -- Régis --
Séguier Régis <rseguier@e-teleport.net> : Does the patch below make a difference ? diff --git a/drivers/net/via-velocity.c b/drivers/net/via-velocity.c index 71a5133..fa303da 100644 --- a/drivers/net/via-velocity.c +++ b/drivers/net/via-velocity.c @@ -1274,7 +1274,7 @@ static void velocity_free_rd_ring(struct velocity_info *vptr) PCI_DMA_FROMDEVICE); rd_info->skb_dma = (dma_addr_t) NULL; - dev_kfree_skb(rd_info->skb); + dev_kfree_skb_any(rd_info->skb); rd_info->skb = NULL; } @@ -1336,7 +1336,7 @@ static void velocity_free_td_ring_entry(struct velocity_info *vptr, td_info->skb_dma[i] = (dma_addr_t) NULL; } } - dev_kfree_skb(td_info->skb); + dev_kfree_skb_any(td_info->skb); td_info->skb = NULL; } } -- Ueimor --
Idem. Jun 18 01:30:15 apollo kernel: ------------[ cut here ]------------ Jun 18 01:30:15 apollo kernel: WARNING: at arch/x86/kernel/pci-dma.c:482 dma_free_coherent+0x3a/0x9c() Jun 18 01:30:15 apollo kernel: Pid: 1401, comm: ip Not tainted 2.6.26-rc6EPIA_SN_VB7001 #4 Jun 18 01:30:15 apollo kernel: [<c0115fe5>] warn_on_slowpath+0x3b/0x5f Jun 18 01:30:15 apollo kernel: [<c0133b19>] __alloc_pages_internal+0xb4/0x349 Jun 18 01:30:15 apollo kernel: [<c0112bfd>] hrtick_start_fair+0x67/0xfd Jun 18 01:30:15 apollo kernel: [<c011414f>] check_preempt_wakeup+0x9d/0xbb Jun 18 01:30:15 apollo kernel: [<c0144752>] kfree+0x6a/0x72 Jun 18 01:30:15 apollo kernel: [<c0241193>] velocity_free_rd_ring+0xa5/0xb6 Jun 18 01:30:15 apollo kernel: [<c0105816>] dma_free_coherent+0x3a/0x9c Jun 18 01:30:15 apollo kernel: [<c02411eb>] velocity_free_dma_rings+0x47/0x4d Jun 18 01:30:15 apollo kernel: [<c024225b>] velocity_change_mtu+0xf6/0x157 Jun 18 01:30:15 apollo kernel: [<c02856c4>] dev_set_mtu+0x2a/0x4f Jun 18 01:30:15 apollo kernel: [<c0286642>] dev_ioctl+0x4ab/0x530 Jun 18 01:30:15 apollo kernel: [<c013964a>] __do_fault+0x256/0x28e Jun 18 01:30:15 apollo kernel: [<c027d8d7>] sock_ioctl+0x152/0x177 Jun 18 01:30:15 apollo kernel: [<c027d785>] sock_ioctl+0x0/0x177 Jun 18 01:30:15 apollo kernel: [<c0150b4a>] vfs_ioctl+0x16/0x48 Jun 18 01:30:15 apollo kernel: [<c0150d5a>] do_vfs_ioctl+0x1de/0x1f1 Jun 18 01:30:15 apollo kernel: [<c0150dae>] sys_ioctl+0x41/0x5b Jun 18 01:30:15 apollo kernel: [<c010291d>] sysenter_past_esp+0x6a/0x91 Jun 18 01:30:15 apollo kernel: ======================= Jun 18 01:30:15 apollo kernel: ---[ end trace abc2c54f7fac91dc ]--- -- Régis --
Séguier Régis <rseguier@e-teleport.net> : I have updated http://userweb.kernel.org/~romieu/via-velocity/2.6.26-rc6/ accordingly. Can you check whether it is fine or not ? -- Ueimor --
Patch 0001 doesn't work,
if I don't make a mistake skb->len is not modified in skb_pad{,to}when
the padding is applied. In this case, we need to attribute ETH_ZLEN to len.
--
Régis
--
Yes, that's what the driver did beforehand. The updated serie is at the usual place. Does it perform better ? -- Ueimor --
Ok for the mtu change, no error messages, I try multiples mtu (1400,2000,4500,9000). But if I generate a paquet with a size more than around 3825, the driver don't send it and no paquet is send after, work again after an interface down/up and until no paquet > 3825. -- Régis --
Séguier Régis <rseguier@e-teleport.net> : Does it qualify as a regression ? -- Ueimor --
I couldn't say yes now. To be honest, I never test mtu > 1500 with this driver before. This can be detected before with the difficulty of mtu change? I'll test the driver before patch with a bigger MTU. (i'll try to do this next week) -- Régis --
No, I test with the version of the driver before patches and I have the same error. I think error is in TX. (TX and RX equipments for the test have the same chip, I don't have other kind of chip available at the moment, that why I not sure) If theses informations help : When I transmit a paquet of ~ 4000 (for a mtu 4500), When I reboot and PCIE-AER is compiled, I got this message : Jun 23 18:28:31 apollo kernel: +------ PCI-Express Device Error ------+ Jun 23 18:28:31 apollo kernel: Error Severity^I^I: Uncorrected (Non-Fatal) Jun 23 18:28:31 apollo kernel: PCIE Bus Error type^I: Transaction Layer Jun 23 18:28:31 apollo kernel: Unsupported Request ^I: Multiple Jun 23 18:28:31 apollo kernel: Requester ID^I^I: 0300 Jun 23 18:28:31 apollo kernel: VendorID=1106h, DeviceID=3119h, Bus=03h, Device=00h, Function=00h Jun 23 18:28:31 apollo kernel: TLB Header: Jun 23 18:28:31 apollo kernel: 40000001 0018000c febffc7c 0000fdff Jun 23 18:28:31 apollo kernel: Broadcast error_detected message Jun 23 18:28:31 apollo kernel: Broadcast mmio_enabled message Jun 23 18:28:31 apollo kernel: Broadcast resume message When I reboot and "PCIE-AER and MSI" is compiled, I got this message : Jun 23 18:35:59 apollo kernel: irq 163, desc: c0495dc8, depth: 1, count: 0, unhandled: 0 Jun 23 18:35:59 apollo kernel: ->handle_irq(): c012d997, handle_bad_irq+0x0/0x199 Jun 23 18:35:59 apollo kernel: ->chip(): c0496b20, 0xc0496b20 Jun 23 18:35:59 apollo kernel: ->action(): 00000000 Jun 23 18:35:59 apollo kernel: IRQ_DISABLED set Jun 23 18:35:59 apollo kernel: unexpected IRQ trap at vector a3 -- Régis --
