Re: [PATCH] net: via-velocity.c fix sleep-with-spinlock bug during MTU change

Previous thread: howto use ioremap_wc? by Brice Goglin on Saturday, May 31, 2008 - 2:02 am. (6 messages)

Next thread: PATCH] net: b44.c fix sleeping-with-spinlock-helt during resume by Arjan van de Ven on Saturday, May 31, 2008 - 8:11 pm. (4 messages)
From: Arjan van de Ven
Date: Saturday, May 31, 2008 - 6:46 pm

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 ...
From: Andrew Morton
Date: Tuesday, June 3, 2008 - 1:40 pm

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);

?

--

From: Francois Romieu
Date: Tuesday, June 3, 2008 - 2:51 pm

Andrew Morton <akpm@linux-foundation.org> :

Almost.

I'll wrap it in the next 24 hours.

-- 
Ueimor
--

From: Francois Romieu
Date: Wednesday, June 4, 2008 - 2:59 pm

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
--

From: Francois Romieu
Date: Saturday, June 14, 2008 - 2:23 pm

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
--

From: Séguier Régis
Date: Monday, June 16, 2008 - 10:21 am

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
--

From: Séguier Régis
Date: Monday, June 16, 2008 - 2:17 pm

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
--

From: Séguier Régis
Date: Monday, June 16, 2008 - 2:25 pm

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
--

From: Francois Romieu
Date: Tuesday, June 17, 2008 - 2:45 pm

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
--

From: Séguier Régis
Date: Tuesday, June 17, 2008 - 4:37 pm

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
--

From: Séguier Régis
Date: Wednesday, June 18, 2008 - 9:56 am

[Empty message]
From: Francois Romieu
Date: Wednesday, June 18, 2008 - 1:10 pm

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
--

From: Séguier Régis
Date: Wednesday, June 18, 2008 - 3:09 pm

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
--

From: Francois Romieu
Date: Thursday, June 19, 2008 - 1:43 pm

Yes, that's what the driver did beforehand.

The updated serie is at the usual place. Does it perform better ?

-- 
Ueimor
--

From: Séguier Régis
Date: Friday, June 20, 2008 - 3:24 am

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
--

From: Francois Romieu
Date: Friday, June 20, 2008 - 3:39 am

Séguier Régis <rseguier@e-teleport.net> :

Does it qualify as a regression ?

-- 
Ueimor
--

From: Séguier Régis
Date: Friday, June 20, 2008 - 9:50 am

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
--

From: Séguier Régis
Date: Monday, June 23, 2008 - 10:04 am

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
--

Previous thread: howto use ioremap_wc? by Brice Goglin on Saturday, May 31, 2008 - 2:02 am. (6 messages)

Next thread: PATCH] net: b44.c fix sleeping-with-spinlock-helt during resume by Arjan van de Ven on Saturday, May 31, 2008 - 8:11 pm. (4 messages)