Patchwork [net-next-2.6,2/2] e1000e: use GFP_KERNEL allocations at init time

login
register
mail settings
Submitter Eric Dumazet
Date July 11, 2011, 9:53 p.m.
Message ID <1310421182.2860.22.camel@edumazet-laptop>
Download mbox | patch
Permalink /patch/104292/
State Awaiting Upstream
Delegated to: David Miller
Headers show

Comments

Eric Dumazet - July 11, 2011, 9:53 p.m.
Note : This patch is untested, I dont have the hardware

Thanks

[PATCH net-next-2.6 2/2] e1000e: use GFP_KERNEL allocations at init time

In process and sleep allowed context, favor GFP_KERNEL allocations over
GFP_ATOMIC ones.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
CC: Ben Greear <greearb@candelatech.com>
CC: Bruce Allan <bruce.w.allan@intel.com>
---
 drivers/net/e1000e/e1000.h  |    2 +-
 drivers/net/e1000e/netdev.c |   33 +++++++++++++++++----------------
 2 files changed, 18 insertions(+), 17 deletions(-)



--
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
Jeff Kirsher - July 11, 2011, 11:51 p.m.
On Mon, Jul 11, 2011 at 14:53, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Note : This patch is untested, I dont have the hardware
>
> Thanks
>
> [PATCH net-next-2.6 2/2] e1000e: use GFP_KERNEL allocations at init time
>
> In process and sleep allowed context, favor GFP_KERNEL allocations over
> GFP_ATOMIC ones.
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> CC: Ben Greear <greearb@candelatech.com>
> CC: Bruce Allan <bruce.w.allan@intel.com>
> ---
>  drivers/net/e1000e/e1000.h  |    2 +-
>  drivers/net/e1000e/netdev.c |   33 +++++++++++++++++----------------
>  2 files changed, 18 insertions(+), 17 deletions(-)
>

Thanks Eric!  I have added the patch to my queue.
David Miller - July 12, 2011, 2:33 a.m.
From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Mon, 11 Jul 2011 16:51:23 -0700

> On Mon, Jul 11, 2011 at 14:53, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> Note : This patch is untested, I dont have the hardware
>>
>> Thanks
>>
>> [PATCH net-next-2.6 2/2] e1000e: use GFP_KERNEL allocations at init time
>>
>> In process and sleep allowed context, favor GFP_KERNEL allocations over
>> GFP_ATOMIC ones.
>>
>> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>> CC: Ben Greear <greearb@candelatech.com>
>> CC: Bruce Allan <bruce.w.allan@intel.com>
>> ---
>>  drivers/net/e1000e/e1000.h  |    2 +-
>>  drivers/net/e1000e/netdev.c |   33 +++++++++++++++++----------------
>>  2 files changed, 18 insertions(+), 17 deletions(-)
>>
> 
> Thanks Eric!  I have added the patch to my queue.

You can't until I put patch #1 into my tree, which adds the
new interfaces used by this patch.
--
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
Jeff Kirsher - July 12, 2011, 2:40 a.m.
On Mon, 2011-07-11 at 19:33 -0700, David Miller wrote:
> From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Date: Mon, 11 Jul 2011 16:51:23 -0700
> 
> > On Mon, Jul 11, 2011 at 14:53, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >> Note : This patch is untested, I dont have the hardware
> >>
> >> Thanks
> >>
> >> [PATCH net-next-2.6 2/2] e1000e: use GFP_KERNEL allocations at init time
> >>
> >> In process and sleep allowed context, favor GFP_KERNEL allocations over
> >> GFP_ATOMIC ones.
> >>
> >> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> >> CC: Ben Greear <greearb@candelatech.com>
> >> CC: Bruce Allan <bruce.w.allan@intel.com>
> >> ---
> >>  drivers/net/e1000e/e1000.h  |    2 +-
> >>  drivers/net/e1000e/netdev.c |   33 +++++++++++++++++----------------
> >>  2 files changed, 18 insertions(+), 17 deletions(-)
> >>
> > 
> > Thanks Eric!  I have added the patch to my queue.
> 
> You can't until I put patch #1 into my tree, which adds the
> new interfaces used by this patch.

I applied patch #1 to my queue as well (for testing purposes) since I
saw that patch #2 was dependent.  If it passes testing, I (or Bruce)
will just ACK patch #2, that way you can apply both patches at the same
time.
David Miller - July 12, 2011, 2:45 a.m.
From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Mon, 11 Jul 2011 19:40:31 -0700

> On Mon, 2011-07-11 at 19:33 -0700, David Miller wrote:
>> You can't until I put patch #1 into my tree, which adds the
>> new interfaces used by this patch.
> 
> I applied patch #1 to my queue as well (for testing purposes) since I
> saw that patch #2 was dependent.  If it passes testing, I (or Bruce)
> will just ACK patch #2, that way you can apply both patches at the same
> time.

Ok.
--
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
Jeff Kirsher - July 12, 2011, 2:58 a.m.
On Mon, Jul 11, 2011 at 19:45, David Miller <davem@davemloft.net> wrote:
> From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Date: Mon, 11 Jul 2011 19:40:31 -0700
>
>> On Mon, 2011-07-11 at 19:33 -0700, David Miller wrote:
>>> You can't until I put patch #1 into my tree, which adds the
>>> new interfaces used by this patch.
>>
>> I applied patch #1 to my queue as well (for testing purposes) since I
>> saw that patch #2 was dependent.  If it passes testing, I (or Bruce)
>> will just ACK patch #2, that way you can apply both patches at the same
>> time.
>
> Ok.
> --

Hmm, this patch has 4 checkpatch.pl warnings so I guess I will be
sending you an updated version of the patch (if patch #1 is fine and
gets accepted) after we finish testing.
Eric Dumazet - July 12, 2011, 4:16 a.m.
Le lundi 11 juillet 2011 à 19:40 -0700, Jeff Kirsher a écrit :
> On Mon, 2011-07-11 at 19:33 -0700, David Miller wrote:
> > From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> > Date: Mon, 11 Jul 2011 16:51:23 -0700
> > 
> > > On Mon, Jul 11, 2011 at 14:53, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > >> Note : This patch is untested, I dont have the hardware
> > >>
> > >> Thanks
> > >>
> > >> [PATCH net-next-2.6 2/2] e1000e: use GFP_KERNEL allocations at init time
> > >>
> > >> In process and sleep allowed context, favor GFP_KERNEL allocations over
> > >> GFP_ATOMIC ones.
> > >>
> > >> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> > >> CC: Ben Greear <greearb@candelatech.com>
> > >> CC: Bruce Allan <bruce.w.allan@intel.com>
> > >> ---
> > >>  drivers/net/e1000e/e1000.h  |    2 +-
> > >>  drivers/net/e1000e/netdev.c |   33 +++++++++++++++++----------------
> > >>  2 files changed, 18 insertions(+), 17 deletions(-)
> > >>
> > > 
> > > Thanks Eric!  I have added the patch to my queue.
> > 
> > You can't until I put patch #1 into my tree, which adds the
> > new interfaces used by this patch.
> 
> I applied patch #1 to my queue as well (for testing purposes) since I
> saw that patch #2 was dependent.  If it passes testing, I (or Bruce)
> will just ACK patch #2, that way you can apply both patches at the same
> time.

I started this work and CC Ben Greear on this one because he sent a bug
report yesterday in case the interface is restarted.

(Crash in e1000e driver, 3.0-rc6+)

BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
IP: [<ffffffffa024ea92>] e1000_alloc_rx_buffers+0x58/0x14a [e1000e]
PGD 0
Oops: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
CPU 0
Modules linked in: macvlan pktgen iscsi_tcp libiscsi_tcp libiscsi scsi_transport_iscsi fuse ip6table_filter ip6_tables ebtable_nat ]

Pid: 2367, comm: kworker/0:2 Tainted: G        W   3.0.0-rc6+ #20 Supermicro X7DBU/X7DBU
RIP: 0010:[<ffffffffa024ea92>]  [<ffffffffa024ea92>] e1000_alloc_rx_buffers+0x58/0x14a [e1000e]
RSP: 0018:ffff8801143ddc70  EFLAGS: 00010206
RAX: ffff880128b08090 RBX: 0000000000000000 RCX: 0000000000000000
RDX: 0000000000000100 RSI: 00000000000000ff RDI: ffff880122d3c900
RBP: ffff8801143ddcc0 R08: ffff8801143ddb80 R09: ffff8801143ddbe0
R10: dead000000200200 R11: dead000000100100 R12: ffff880122d3c900
R13: 0000000000000000 R14: ffff880122818558 R15: 00000000000000ff
FS:  0000000000000000(0000) GS:ffff88012fc00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 0000000000000008 CR3: 0000000001a03000 CR4: 00000000000006f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process kworker/0:2 (pid: 2367, threadinfo ffff8801143dc000, task ffff8801259267e0)
Stack:
  ffff880122d3c900 ffff880122d3c000 000005f222818558 ffff880128b08090
  ffff8801143ddca0 ffff880122d3c900 ffff880122818558 0000000000001000
  0000000004008002 ffffffffa024d672 ffff8801143ddcf0 ffffffffa024a795
Call Trace:
  [<ffffffffa024d672>] ? e1000e_reinit_locked+0x5f/0x5f [e1000e]
  [<ffffffffa024a795>] e1000_configure+0x507/0x510 [e1000e]
  [<ffffffffa024a7af>] e1000e_up+0x11/0xc9 [e1000e]
  [<ffffffffa024d665>] e1000e_reinit_locked+0x52/0x5f [e1000e]
  [<ffffffffa024dd0e>] e1000_reset_task+0x69c/0x6ab [e1000e]
  [<ffffffff8106128e>] ? process_one_work+0x17b/0x41d
  [<ffffffff81041b91>] ? get_parent_ip+0x11/0x41
  [<ffffffffa024d672>] ? e1000e_reinit_locked+0x5f/0x5f [e1000e]
  [<ffffffff81061343>] process_one_work+0x230/0x41d
  [<ffffffff8106128e>] ? process_one_work+0x17b/0x41d
  [<ffffffff8106379f>] worker_thread+0x133/0x217
  [<ffffffff8106366c>] ? manage_workers+0x191/0x191
  [<ffffffff81066f9c>] kthread+0x7d/0x85
  [<ffffffff81485ee4>] kernel_thread_helper+0x4/0x10
  [<ffffffff8147f0d8>] ? retint_restore_args+0x13/0x13
  [<ffffffff81066f1f>] ? __init_kthread_worker+0x56/0x56
  [<ffffffff81485ee0>] ? gs_change+0x13/0x13
Code: 00 00 89 45 c4 41 0f b7 5e 18 48 8b 87 28 05 00 00 41 89 dd 48 05 90 00 00 00 4d 6b ed 28 4d 03 6e 20 48 89 45 c8 e9 d5 00 00
  8b 45 08 48 85 c0 74 14 48 89 c7 31 f6 48 89 45 b0 e8 29 85
RIP  [<ffffffffa024ea92>] e1000_alloc_rx_buffers+0x58/0x14a [e1000e]
  RSP <ffff8801143ddc70>
CR2: 0000000000000008
---[ end trace cddc6b4ca68ac6e9 ]---


I feel that this driver assumes the refill is done without any
OutOfMemory conditions, I've not yet found how to fix this bad
assumption, only make the refill use GFP_KERNEL to avoid OOM



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

Patch

diff --git a/drivers/net/e1000e/e1000.h b/drivers/net/e1000e/e1000.h
index c1e7f94..2de6fc8 100644
--- a/drivers/net/e1000e/e1000.h
+++ b/drivers/net/e1000e/e1000.h
@@ -341,7 +341,7 @@  struct e1000_adapter {
 			  int *work_done, int work_to_do)
 						____cacheline_aligned_in_smp;
 	void (*alloc_rx_buf) (struct e1000_adapter *adapter,
-			      int cleaned_count);
+			      int cleaned_count, gfp_t gfp);
 	struct e1000_ring *rx_ring;
 
 	u32 rx_int_delay;
diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c
index ed7a93d..365a324 100644
--- a/drivers/net/e1000e/netdev.c
+++ b/drivers/net/e1000e/netdev.c
@@ -523,7 +523,7 @@  static void e1000_rx_checksum(struct e1000_adapter *adapter, u32 status_err,
  * @adapter: address of board private structure
  **/
 static void e1000_alloc_rx_buffers(struct e1000_adapter *adapter,
-				   int cleaned_count)
+				   int cleaned_count, gfp_t gfp)
 {
 	struct net_device *netdev = adapter->netdev;
 	struct pci_dev *pdev = adapter->pdev;
@@ -544,7 +544,7 @@  static void e1000_alloc_rx_buffers(struct e1000_adapter *adapter,
 			goto map_skb;
 		}
 
-		skb = netdev_alloc_skb_ip_align(netdev, bufsz);
+		skb = __netdev_alloc_skb_ip_align(netdev, bufsz, gfp);
 		if (!skb) {
 			/* Better luck next round */
 			adapter->alloc_rx_buff_failed++;
@@ -589,7 +589,7 @@  map_skb:
  * @adapter: address of board private structure
  **/
 static void e1000_alloc_rx_buffers_ps(struct e1000_adapter *adapter,
-				      int cleaned_count)
+				      int cleaned_count, gfp_t gfp)
 {
 	struct net_device *netdev = adapter->netdev;
 	struct pci_dev *pdev = adapter->pdev;
@@ -615,7 +615,7 @@  static void e1000_alloc_rx_buffers_ps(struct e1000_adapter *adapter,
 				continue;
 			}
 			if (!ps_page->page) {
-				ps_page->page = alloc_page(GFP_ATOMIC);
+				ps_page->page = alloc_page(gfp);
 				if (!ps_page->page) {
 					adapter->alloc_rx_buff_failed++;
 					goto no_buffers;
@@ -641,8 +641,9 @@  static void e1000_alloc_rx_buffers_ps(struct e1000_adapter *adapter,
 			    cpu_to_le64(ps_page->dma);
 		}
 
-		skb = netdev_alloc_skb_ip_align(netdev,
-						adapter->rx_ps_bsize0);
+		skb = __netdev_alloc_skb_ip_align(netdev,
+						  adapter->rx_ps_bsize0,
+						  gfp);
 
 		if (!skb) {
 			adapter->alloc_rx_buff_failed++;
@@ -692,7 +693,7 @@  no_buffers:
  **/
 
 static void e1000_alloc_jumbo_rx_buffers(struct e1000_adapter *adapter,
-                                         int cleaned_count)
+					 int cleaned_count, gfp_t gfp)
 {
 	struct net_device *netdev = adapter->netdev;
 	struct pci_dev *pdev = adapter->pdev;
@@ -713,7 +714,7 @@  static void e1000_alloc_jumbo_rx_buffers(struct e1000_adapter *adapter,
 			goto check_page;
 		}
 
-		skb = netdev_alloc_skb_ip_align(netdev, bufsz);
+		skb = __netdev_alloc_skb_ip_align(netdev, bufsz, gfp);
 		if (unlikely(!skb)) {
 			/* Better luck next round */
 			adapter->alloc_rx_buff_failed++;
@@ -724,7 +725,7 @@  static void e1000_alloc_jumbo_rx_buffers(struct e1000_adapter *adapter,
 check_page:
 		/* allocate a new page if necessary */
 		if (!buffer_info->page) {
-			buffer_info->page = alloc_page(GFP_ATOMIC);
+			buffer_info->page = alloc_page(gfp);
 			if (unlikely(!buffer_info->page)) {
 				adapter->alloc_rx_buff_failed++;
 				break;
@@ -888,7 +889,7 @@  next_desc:
 
 		/* return some buffers to hardware, one at a time is too slow */
 		if (cleaned_count >= E1000_RX_BUFFER_WRITE) {
-			adapter->alloc_rx_buf(adapter, cleaned_count);
+			adapter->alloc_rx_buf(adapter, cleaned_count, GFP_ATOMIC);
 			cleaned_count = 0;
 		}
 
@@ -900,7 +901,7 @@  next_desc:
 
 	cleaned_count = e1000_desc_unused(rx_ring);
 	if (cleaned_count)
-		adapter->alloc_rx_buf(adapter, cleaned_count);
+		adapter->alloc_rx_buf(adapter, cleaned_count, GFP_ATOMIC);
 
 	adapter->total_rx_bytes += total_rx_bytes;
 	adapter->total_rx_packets += total_rx_packets;
@@ -1230,7 +1231,7 @@  next_desc:
 
 		/* return some buffers to hardware, one at a time is too slow */
 		if (cleaned_count >= E1000_RX_BUFFER_WRITE) {
-			adapter->alloc_rx_buf(adapter, cleaned_count);
+			adapter->alloc_rx_buf(adapter, cleaned_count, GFP_ATOMIC);
 			cleaned_count = 0;
 		}
 
@@ -1244,7 +1245,7 @@  next_desc:
 
 	cleaned_count = e1000_desc_unused(rx_ring);
 	if (cleaned_count)
-		adapter->alloc_rx_buf(adapter, cleaned_count);
+		adapter->alloc_rx_buf(adapter, cleaned_count, GFP_ATOMIC);
 
 	adapter->total_rx_bytes += total_rx_bytes;
 	adapter->total_rx_packets += total_rx_packets;
@@ -1411,7 +1412,7 @@  next_desc:
 
 		/* return some buffers to hardware, one at a time is too slow */
 		if (unlikely(cleaned_count >= E1000_RX_BUFFER_WRITE)) {
-			adapter->alloc_rx_buf(adapter, cleaned_count);
+			adapter->alloc_rx_buf(adapter, cleaned_count, GFP_ATOMIC);
 			cleaned_count = 0;
 		}
 
@@ -1423,7 +1424,7 @@  next_desc:
 
 	cleaned_count = e1000_desc_unused(rx_ring);
 	if (cleaned_count)
-		adapter->alloc_rx_buf(adapter, cleaned_count);
+		adapter->alloc_rx_buf(adapter, cleaned_count, GFP_ATOMIC);
 
 	adapter->total_rx_bytes += total_rx_bytes;
 	adapter->total_rx_packets += total_rx_packets;
@@ -3105,7 +3106,7 @@  static void e1000_configure(struct e1000_adapter *adapter)
 	e1000_configure_tx(adapter);
 	e1000_setup_rctl(adapter);
 	e1000_configure_rx(adapter);
-	adapter->alloc_rx_buf(adapter, e1000_desc_unused(adapter->rx_ring));
+	adapter->alloc_rx_buf(adapter, e1000_desc_unused(adapter->rx_ring), GFP_KERNEL);
 }
 
 /**