| Message ID | 20120213135248.GA23457@sir.fritz.box |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | David Miller |
| Headers | show |
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> -- 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
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.
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 -- 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
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 ? -- 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
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 -- 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
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;
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(-)