[PATCH] ixgbe: fix truesize calculation when merging active tail into lro skb

[PATCH] ixgbe: fix truesize calculation when merging active tail into lro skb

[Christian Brunner]

I'm seeing some page allocation failures with the ixgbe driver under heavy
load. While looking after it, I came accoss the truesize handling. I suspect,
that there is a small misstake in ixgbe_merge_active_tail(). (But I'm not
really sure).

Truesize allocation of the skb may be larger than skb->len, because
ixgbe is allocating PAGE_SIZE/2 for received fragments. Hence we 
should use the truesize of the tail when merging.

Signed-off-by: Christian Brunner <chb@muc.de>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index a42b0b2..c4d25af 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -1222,7 +1222,7 @@ static inline struct sk_buff *ixgbe_merge_active_tail(struct sk_buff *tail)
 
 	head->len += tail->len;
 	head->data_len += tail->len;
-	head->truesize += tail->len;
+	head->truesize += tail->truesize;
 
 	IXGBE_CB(tail)->head = NULL;
 
-- 
1.7.1

Re: [PATCH] ixgbe: fix truesize calculation when merging active tail into lro skb

[Eric Dumazet]

Le lundi 13 février 2012 à 14:52 +0100, Christian Brunner a écrit :
> I'm seeing some page allocation failures with the ixgbe driver under heavy
> load. While looking after it, I came accoss the truesize handling. I suspect,
> that there is a small misstake in ixgbe_merge_active_tail(). (But I'm not
> really sure).
> 
> Truesize allocation of the skb may be larger than skb->len, because
> ixgbe is allocating PAGE_SIZE/2 for received fragments. Hence we 
> should use the truesize of the tail when merging.
> 
> Signed-off-by: Christian Brunner <chb@muc.de>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index a42b0b2..c4d25af 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -1222,7 +1222,7 @@ static inline struct sk_buff *ixgbe_merge_active_tail(struct sk_buff *tail)
>  
>  	head->len += tail->len;
>  	head->data_len += tail->len;
> -	head->truesize += tail->len;
> +	head->truesize += tail->truesize;
>  
>  	IXGBE_CB(tail)->head = NULL;
>  

You forgot CC Intel guys, but they usually catch netdev traffic :)

Acked-by: Eric Dumazet <eric.dumazet@gmail.com>

Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Cc: Jesse Brandeburg <jesse.brandeburg@intel.com>

Re: [PATCH] ixgbe: fix truesize calculation when merging active tail into lro skb

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 1708 bytes --]

On Mon, 2012-02-13 at 15:12 +0100, Eric Dumazet wrote:
> Le lundi 13 février 2012 à 14:52 +0100, Christian Brunner a écrit :
> > I'm seeing some page allocation failures with the ixgbe driver under heavy
> > load. While looking after it, I came accoss the truesize handling. I suspect,
> > that there is a small misstake in ixgbe_merge_active_tail(). (But I'm not
> > really sure).
> > 
> > Truesize allocation of the skb may be larger than skb->len, because
> > ixgbe is allocating PAGE_SIZE/2 for received fragments. Hence we 
> > should use the truesize of the tail when merging.
> > 
> > Signed-off-by: Christian Brunner <chb@muc.de>
> > ---
> >  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> > index a42b0b2..c4d25af 100644
> > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> > @@ -1222,7 +1222,7 @@ static inline struct sk_buff *ixgbe_merge_active_tail(struct sk_buff *tail)
> >  
> >  	head->len += tail->len;
> >  	head->data_len += tail->len;
> > -	head->truesize += tail->len;
> > +	head->truesize += tail->truesize;
> >  
> >  	IXGBE_CB(tail)->head = NULL;
> >  
> 
> You forgot CC Intel guys, but they usually catch netdev traffic :)
> 
> Acked-by: Eric Dumazet <eric.dumazet@gmail.com>
> 
> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Cc: Jesse Brandeburg <jesse.brandeburg@intel.com>
> 
> 

Thanks Eric for bringing this to my attention.

Thanks Christian for the patch, I have added it to my queue.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] ixgbe: fix truesize calculation when merging active tail into lro skb

[Alexander Duyck]

On 02/13/2012 01:43 PM, Jeff Kirsher wrote:
> On Mon, 2012-02-13 at 15:12 +0100, Eric Dumazet wrote:
>> Le lundi 13 février 2012 à 14:52 +0100, Christian Brunner a écrit :
>>> I'm seeing some page allocation failures with the ixgbe driver under heavy
>>> load. While looking after it, I came accoss the truesize handling. I suspect,
>>> that there is a small misstake in ixgbe_merge_active_tail(). (But I'm not
>>> really sure).
>>>
>>> Truesize allocation of the skb may be larger than skb->len, because
>>> ixgbe is allocating PAGE_SIZE/2 for received fragments. Hence we 
>>> should use the truesize of the tail when merging.
>>>
>>> Signed-off-by: Christian Brunner <chb@muc.de>
>>> ---
>>>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |    2 +-
>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> index a42b0b2..c4d25af 100644
>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> @@ -1222,7 +1222,7 @@ static inline struct sk_buff *ixgbe_merge_active_tail(struct sk_buff *tail)
>>>  
>>>  	head->len += tail->len;
>>>  	head->data_len += tail->len;
>>> -	head->truesize += tail->len;
>>> +	head->truesize += tail->truesize;
>>>  
>>>  	IXGBE_CB(tail)->head = NULL;
>>>  
>> You forgot CC Intel guys, but they usually catch netdev traffic :)
>>
>> Acked-by: Eric Dumazet <eric.dumazet@gmail.com>
>>
>> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>> Cc: Jesse Brandeburg <jesse.brandeburg@intel.com>
>>
>>
> Thanks Eric for bringing this to my attention.
>
> Thanks Christian for the patch, I have added it to my queue.

The code itself is correct, but the comment isn't.  This code path is
applied only to the case where we are not using pages.  The default Rx
buffer size is actually about 3K when RSC is in use, which means
truesize is about 4.25K per buffer.

Thanks,

Alex

Re: [PATCH] ixgbe: fix truesize calculation when merging active tail into lro skb

[Eric Dumazet]

Le mardi 14 février 2012 à 09:21 -0800, Alexander Duyck a écrit :

> The code itself is correct, but the comment isn't.  This code path is
> applied only to the case where we are not using pages.  The default Rx
> buffer size is actually about 3K when RSC is in use, which means
> truesize is about 4.25K per buffer.
> 

Hmm... any reason its not 2.25K per buffer ? (assuming MTU=1500)

Do you really need this code in ixgbe_set_rx_buffer_len() ?

                /*
                 * Make best use of allocation by using all but 1K of a
                 * power of 2 allocation that will be used for skb->head.
                 */
                else if (max_frame <= IXGBE_RXBUFFER_3K)
                        rx_buf_len = IXGBE_RXBUFFER_3K;
                else if (max_frame <= IXGBE_RXBUFFER_7K)
                        rx_buf_len = IXGBE_RXBUFFER_7K;
                else if (max_frame <= IXGBE_RXBUFFER_15K)
                        rx_buf_len = IXGBE_RXBUFFER_15K;
                else
                        rx_buf_len = IXGBE_MAX_RXBUFFER;

Why not using :
		rx_buf_len = max_frame;

and let kmalloc() do its best ?

Re: [PATCH] ixgbe: fix truesize calculation when merging active tail into lro skb

[Alexander Duyck]

On 02/14/2012 09:39 AM, Eric Dumazet wrote:
> Le mardi 14 février 2012 à 09:21 -0800, Alexander Duyck a écrit :
>
>> The code itself is correct, but the comment isn't.  This code path is
>> applied only to the case where we are not using pages.  The default Rx
>> buffer size is actually about 3K when RSC is in use, which means
>> truesize is about 4.25K per buffer.
>>
> Hmm... any reason its not 2.25K per buffer ? (assuming MTU=1500)
>
> Do you really need this code in ixgbe_set_rx_buffer_len() ?
>
>                 /*
>                  * Make best use of allocation by using all but 1K of a
>                  * power of 2 allocation that will be used for skb->head.
>                  */
>                 else if (max_frame <= IXGBE_RXBUFFER_3K)
>                         rx_buf_len = IXGBE_RXBUFFER_3K;
>                 else if (max_frame <= IXGBE_RXBUFFER_7K)
>                         rx_buf_len = IXGBE_RXBUFFER_7K;
>                 else if (max_frame <= IXGBE_RXBUFFER_15K)
>                         rx_buf_len = IXGBE_RXBUFFER_15K;
>                 else
>                         rx_buf_len = IXGBE_MAX_RXBUFFER;
>
> Why not using :
> 		rx_buf_len = max_frame;
>
> and let kmalloc() do its best ?

The reason for all of this is receive side coalescing.  RSC causes us to
do full buffer size DMAs even if the max frame size is less than the Rx
buffer length.  If RSC is disabled via the NETIF_F_LRO flag then the
default will drop to a 1522 buffer allocation size, and kmalloc can do a
2K allocation.

If I am not mistaken, kmalloc only allocates power of 2 sized blocks for
anything over 256 bytes.  I made the above code change a little while
back when I realized that when RSC was enabled we were setting up a 2K
buffer, which after adding padding and skb_shared_info was 2.375K
resulting in a 4K allocation.  After see that I decided it was better
for us to set the buffer size to 3K which reduced RSC descriptor
processing overhead for the standard case by 50%, and made use of 1K of
the wasted space.

I already have patches in the works that will do away with all of this
code pretty soon anyway, and replace it all with something similar to
our page based packet split path.  It will also end up doing away with
the current RSC code since page based receives end up not needing to be
queued as we are just adding pages to frags.

Thanks,

Alex