[PATCH netdev-2.6 7/10] ixgb: Replace kmalloc with vmalloc to allocate driver local data structures

[PATCH netdev-2.6 7/10] ixgb: Replace kmalloc with vmalloc to allocate driver local data structures

[Ganesh Venkatesan]

Signed-off-by: ganesh venkatesan <ganesh.venkatesan@intel.com>

diff -up netdev-2.6/drivers/net/ixgb/ixgb.h netdev-2.6/drivers/net/ixgb.new/ixgb.h
--- netdev-2.6/drivers/net/ixgb/ixgb.h	2004-10-15 13:15:38.000000000 -0700
+++ netdev-2.6/drivers/net/ixgb.new/ixgb.h	2004-10-15 13:15:51.000000000 -0700
@@ -46,6 +46,7 @@
 #include <linux/delay.h>
 #include <linux/timer.h>
 #include <linux/slab.h>
+#include <linux/vmalloc.h>
 #include <linux/interrupt.h>
 #include <linux/string.h>
 #include <linux/pagemap.h>
diff -up netdev-2.6/drivers/net/ixgb/ixgb_main.c netdev-2.6/drivers/net/ixgb.new/ixgb_main.c
--- netdev-2.6/drivers/net/ixgb/ixgb_main.c	2004-10-15 13:15:38.000000000 -0700
+++ netdev-2.6/drivers/net/ixgb.new/ixgb_main.c	2004-10-15 13:15:55.000000000 -0700
@@ -608,8 +608,8 @@ static int ixgb_close(struct net_device 
 	int size;
 
 	size = sizeof(struct ixgb_buffer) * txdr->count;
-	txdr->buffer_info = kmalloc(size, GFP_KERNEL);
 	if (!txdr->buffer_info) {
+	txdr->buffer_info = vmalloc(size);
 		return -ENOMEM;
 	}
 	memset(txdr->buffer_info, 0, size);
@@ -621,7 +620,7 @@ static int ixgb_setup_tx_resources(struc
 
 	txdr->desc = pci_alloc_consistent(pdev, txdr->size, &txdr->dma);
 	if (!txdr->desc) {
-		kfree(txdr->buffer_info);
+		vfree(txdr->buffer_info);
 		return -ENOMEM;
 	}
 	memset(txdr->desc, 0, txdr->size);
@@ -697,8 +692,8 @@ static void ixgb_configure_tx(struct ixg
 	int size;
 
 	size = sizeof(struct ixgb_buffer) * rxdr->count;
-	rxdr->buffer_info = kmalloc(size, GFP_KERNEL);
 	if (!rxdr->buffer_info) {
+	rxdr->buffer_info = vmalloc(size);
 		return -ENOMEM;
 	}
 	memset(rxdr->buffer_info, 0, size);
@@ -711,7 +713,7 @@ static int ixgb_setup_rx_resources(struc
 	rxdr->desc = pci_alloc_consistent(pdev, rxdr->size, &rxdr->dma);
 
 	if (!rxdr->desc) {
-		kfree(rxdr->buffer_info);
+		vfree(rxdr->buffer_info);
 		return -ENOMEM;
 	}
 	memset(rxdr->desc, 0, rxdr->size);
@@ -865,7 +867,7 @@ static void ixgb_configure_rx(struct ixg
 
 	ixgb_clean_tx_ring(adapter);
 
-	kfree(adapter->tx_ring.buffer_info);
+	vfree(adapter->tx_ring.buffer_info);
 	adapter->tx_ring.buffer_info = NULL;
 
 	pci_free_consistent(pdev, adapter->tx_ring.size,
@@ -931,7 +935,7 @@ static void ixgb_clean_tx_ring(struct ix
 
 	ixgb_clean_rx_ring(adapter);
 
-	kfree(rx_ring->buffer_info);
+	vfree(rx_ring->buffer_info);
 	rx_ring->buffer_info = NULL;
 
 	pci_free_consistent(pdev, rx_ring->size, rx_ring->desc, rx_ring->dma);

Re: [PATCH netdev-2.6 7/10] ixgb: Replace kmalloc with vmalloc to allocate driver local data structures

[Christoph Hellwig]

this uses up vmalloc space and thus is a regression.

RE: [PATCH netdev-2.6 7/10] ixgb: Replace kmalloc with vmalloc to allocate driver local data structures

[Venkatesan, Ganesh]

This is a trade-off between space allocated via kmalloc and vmalloc. My
understanding is that kmalloc space is more limited than vmalloc. Is
this incorrect?

With the original implementation that used kmalloc for all allocations
in the driver, I have noticed allocation failures when the ring sizes
were set to 4096. Switching the allocation of this space from kmalloc to
vmalloc guarantees successful allocation.

What am I missing?

Ganesh.

-----Original Message-----
From: Christoph Hellwig [mailto:hch@infradead.org] 
Sent: Friday, October 29, 2004 5:15 AM
To: Venkatesan, Ganesh
Cc: jgarzik@pobox.com; netdev
Subject: Re: [PATCH netdev-2.6 7/10] ixgb: Replace kmalloc with vmalloc
to allocate driver local data structures

this uses up vmalloc space and thus is a regression.

Re: [PATCH netdev-2.6 7/10] ixgb: Replace kmalloc with vmalloc to allocate driver local data structures

[Christoph Hellwig]

On Fri, Oct 29, 2004 at 05:50:45AM -0700, Venkatesan, Ganesh wrote:
> This is a trade-off between space allocated via kmalloc and vmalloc. My
> understanding is that kmalloc space is more limited than vmalloc. Is
> this incorrect?

Yes. kmalloc space is only limited by the amount of free memory you have
in your system, vmalloc has very low absolute limits (down to 64MB in
some configurations)..

> With the original implementation that used kmalloc for all allocations
> in the driver, I have noticed allocation failures when the ring sizes
> were set to 4096.

4096 what?

Re: [PATCH netdev-2.6 7/10] ixgb: Replace kmalloc with vmalloc to allocate driver local data structures

[Jon Mason]

On Friday 29 October 2004 08:08 am, Christoph Hellwig wrote:
> On Fri, Oct 29, 2004 at 05:50:45AM -0700, Venkatesan, Ganesh wrote:
> > This is a trade-off between space allocated via kmalloc and vmalloc. My
> > understanding is that kmalloc space is more limited than vmalloc. Is
> > this incorrect?
>
> Yes. kmalloc space is only limited by the amount of free memory you have
> in your system, vmalloc has very low absolute limits (down to 64MB in
> some configurations)..
>
> > With the original implementation that used kmalloc for all allocations
> > in the driver, I have noticed allocation failures when the ring sizes
> > were set to 4096.
>
> 4096 what?

When the Tx or Rx descriptor queues are set greater than 3000 descriptors on 
64bit systems, the amount of memory the driver tries to allocate for this 
queue is too large for kmalloc. See ixgb_setup_tx_resources() of ixgb_main.c 
(line 611).  The cause of the problem is that ixgb_buffer grows large on 
64bit systems because of unsigned long fields in the struct.

There seems to be 3 ways around this problem.  
1) change the unsigned long fields in ixgb_buffer to unsigned ints (this 
decreases the size of the struct enough to use kmalloc)
2) Modify the driver to move/remove one of the unsigned long fields (most 
likely time_stamp)
3) replace kmalloc with vmalloc

e1000 made a similar change recently, which was accepted after some 
discussion.

-- 
Jon Mason
jdmason@us.ibm.com

RE: [PATCH netdev-2.6 7/10] ixgb: Replace kmalloc with vmalloc to allocate driver local data structures

[Venkatesan, Ganesh]

>There seems to be 3 ways around this problem.
>1) change the unsigned long fields in ixgb_buffer to unsigned ints
(this
>decreases the size of the struct enough to use kmalloc)

PATCH netdev-2.6 6/10 does this.

Re: [PATCH netdev-2.6 7/10] ixgb: Replace kmalloc with vmalloc to allocate driver local data structures

[Jon Mason]

On Friday 29 October 2004 10:35 am, Venkatesan, Ganesh wrote:
> >There seems to be 3 ways around this problem.
> >1) change the unsigned long fields in ixgb_buffer to unsigned ints
>
> (this
>
> >decreases the size of the struct enough to use kmalloc)
>
> PATCH netdev-2.6 6/10 does this.

Problem with patch 6/10.

--- netdev-2.6/drivers/net/ixgb/ixgb.h  2004-10-15 13:15:38.000000000 -0700
+++ netdev-2.6/drivers/net/ixgb.new/ixgb.h      2004-10-15 13:15:51.000000000 
-0700
@@ -105,9 +105,9 @@ struct ixgb_adapter;
 struct ixgb_buffer {
        struct sk_buff *skb;
        uint64_t dma;
-       unsigned long length;
+       uint16_t length;
        unsigned long time_stamp;
-       unsigned int next_to_watch;
+       uint16_t next_to_watch;
 };

Since the 2 16bit fields are not contigious, they will take up 32bits each.  
This ballons the struct to still be too large.  By moving thse 2 fields next 
to one another, this fixes the problem and makes the struct small enough to 
use kmalloc for up to 4096 descriptors.  

The patch below removes the need for patch 7/10 (and I have verified this).

--- ixgb.h.orig 2004-10-29 11:36:06.757879120 -0700
+++ ixgb.h      2004-10-29 11:35:51.065001784 -0700
@@ -123,8 +123,8 @@ struct ixgb_adapter;
 struct ixgb_buffer {
        struct sk_buff *skb;
        uint64_t dma;
-       uint16_t length;
        unsigned long time_stamp;
+       uint16_t length;
        uint16_t next_to_watch;
 };


-- 
Jon Mason
jdmason@us.ibm.com

Re: [PATCH netdev-2.6 7/10] ixgb: Replace kmalloc with vmalloc to allocate driver local data structures

[Jeff Garzik]

Christoph Hellwig wrote:
> On Fri, Oct 29, 2004 at 05:50:45AM -0700, Venkatesan, Ganesh wrote:
> 
>>This is a trade-off between space allocated via kmalloc and vmalloc. My
>>understanding is that kmalloc space is more limited than vmalloc. Is
>>this incorrect?
> 
> 
> Yes. kmalloc space is only limited by the amount of free memory you have
> in your system, vmalloc has very low absolute limits (down to 64MB in
> some configurations)..

However, kmalloc() has much lower per-call limits than vmalloc().

I'm interested to see a VM person weigh in on this...

Drivers are certainly allowed to use vmalloc.

	Jeff

Re: [PATCH netdev-2.6 7/10] ixgb: Replace kmalloc with vmalloc to allocate driver local data structures

[William Lee Irwin III]

On Fri, Oct 29, 2004 at 05:50:45AM -0700, Venkatesan, Ganesh wrote:
>>> This is a trade-off between space allocated via kmalloc and vmalloc. My
>>> understanding is that kmalloc space is more limited than vmalloc. Is
>>> this incorrect?

Christoph Hellwig wrote:
>> Yes. kmalloc space is only limited by the amount of free memory you have
>> in your system, vmalloc has very low absolute limits (down to 64MB in
>> some configurations)..

On Fri, Oct 29, 2004 at 01:02:47PM -0400, Jeff Garzik wrote:
> However, kmalloc() has much lower per-call limits than vmalloc().
> I'm interested to see a VM person weigh in on this...
> Drivers are certainly allowed to use vmalloc.

vmalloc is relatively highly penalized. For instance, global TLB
flushes associated with vfree() and so on. Aggregate vmallocspace
limitations are a big concern. I probably wouldn't use vmallocspace
for much besides infrequent allocations, e.g. things allocated during
driver initialization. One per driver instance won't really hurt much
because vmallocspace limits prevent large numbers of drivers from being
simultaneously loaded as it stands now anyway.


-- wli

Re: [PATCH netdev-2.6 7/10] ixgb: Replace kmalloc with vmalloc to allocate driver local data structures

[Jeff Garzik]

William Lee Irwin III wrote:
> vmalloc is relatively highly penalized. For instance, global TLB
> flushes associated with vfree() and so on. Aggregate vmallocspace
> limitations are a big concern. I probably wouldn't use vmallocspace
> for much besides infrequent allocations, e.g. things allocated during
> driver initialization. One per driver instance won't really hurt much
> because vmallocspace limits prevent large numbers of drivers from being
> simultaneously loaded as it stands now anyway.


That's precisely what we're contemplating -- a once per driver 
allocation that remains static for the entire driver runtime (descriptors)

	Jeff

Re: [PATCH netdev-2.6 7/10] ixgb: Replace kmalloc with vmalloc to allocate driver local data structures

[William Lee Irwin III]

William Lee Irwin III wrote:
>> vmalloc is relatively highly penalized. For instance, global TLB
>> flushes associated with vfree() and so on. Aggregate vmallocspace
>> limitations are a big concern. I probably wouldn't use vmallocspace
>> for much besides infrequent allocations, e.g. things allocated during
>> driver initialization. One per driver instance won't really hurt much
>> because vmallocspace limits prevent large numbers of drivers from being
>> simultaneously loaded as it stands now anyway.

On Fri, Oct 29, 2004 at 02:26:37PM -0400, Jeff Garzik wrote:
> That's precisely what we're contemplating -- a once per driver 
> allocation that remains static for the entire driver runtime (descriptors)

An infrastructural change that does this would probably need to be
considered more carefully. I think you can probably just do it.


-- wli