Misuse of LRO, how widespread

Misuse of LRO, how widespread

[Stephen Hemminger]

I inspected all drivers in net-next to see which drivers are using
LRO and which ones are broken. Most concerning is that Chelsio
and Solarflare drivers ignore ETH_FLAG_LRO.

The ones that are using LRO but allow disabling it:
  qlcnic, netxen, mv643, s2io, myi10ge, bnx2x, ixgbe, vmxnet3

One driver seems confused about LRO vs GRO:
  mlx4 - comments about LRO and depends on LRO but driver is using GRO

Drivers with not using ethtool interface to disable LRO:
  pasemi_mac, sfc, ehea, cxgb3, cxgb4

Re: Misuse of LRO, how widespread

[Ben Hutchings]

On Mon, 2010-12-06 at 12:18 -0800, Stephen Hemminger wrote:
> I inspected all drivers in net-next to see which drivers are using
> LRO and which ones are broken. Most concerning is that Chelsio
> and Solarflare drivers ignore ETH_FLAG_LRO.
> 
> The ones that are using LRO but allow disabling it:
>   qlcnic, netxen, mv643, s2io, myi10ge, bnx2x, ixgbe, vmxnet3
> 
> One driver seems confused about LRO vs GRO:
>   mlx4 - comments about LRO and depends on LRO but driver is using GRO

sfc is also in this category.  (And it's not confused, it was using
inet_lro before being converted to GRO.)

> Drivers with not using ethtool interface to disable LRO:
>   pasemi_mac, sfc, ehea, cxgb3, cxgb4

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: Misuse of LRO, how widespread

[Stephen Hemminger]

How about this?

Subject: sfc: convert references to LRO to GRO

This driver now uses Generic Receive Offload, not the older LRO.
Change references to LRO in names and comments.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


--- a/drivers/net/sfc/rx.c	2010-12-06 14:28:25.320929804 -0800
+++ b/drivers/net/sfc/rx.c	2010-12-06 14:29:54.424930087 -0800
@@ -37,7 +37,7 @@
  * This driver supports two methods for allocating and using RX buffers:
  * each RX buffer may be backed by an skb or by an order-n page.
  *
- * When LRO is in use then the second method has a lower overhead,
+ * When GRO is in use then the second method has a lower overhead,
  * since we don't have to allocate then free skbs on reassembled frames.
  *
  * Values:
@@ -50,25 +50,25 @@
  *
  *   - Since pushing and popping descriptors are separated by the rx_queue
  *     size, so the watermarks should be ~rxd_size.
- *   - The performance win by using page-based allocation for LRO is less
- *     than the performance hit of using page-based allocation of non-LRO,
+ *   - The performance win by using page-based allocation for GRO is less
+ *     than the performance hit of using page-based allocation of non-GRO,
  *     so the watermarks should reflect this.
  *
  * Per channel we maintain a single variable, updated by each channel:
  *
- *   rx_alloc_level += (lro_performed ? RX_ALLOC_FACTOR_LRO :
+ *   rx_alloc_level += (gro_performed ? RX_ALLOC_FACTOR_GRO :
  *                      RX_ALLOC_FACTOR_SKB)
  * Per NAPI poll interval, we constrain rx_alloc_level to 0..MAX (which
  * limits the hysteresis), and update the allocation strategy:
  *
- *   rx_alloc_method = (rx_alloc_level > RX_ALLOC_LEVEL_LRO ?
+ *   rx_alloc_method = (rx_alloc_level > RX_ALLOC_LEVEL_GRO ?
  *                      RX_ALLOC_METHOD_PAGE : RX_ALLOC_METHOD_SKB)
  */
 static int rx_alloc_method = RX_ALLOC_METHOD_AUTO;
 
-#define RX_ALLOC_LEVEL_LRO 0x2000
+#define RX_ALLOC_LEVEL_GRO 0x2000
 #define RX_ALLOC_LEVEL_MAX 0x3000
-#define RX_ALLOC_FACTOR_LRO 1
+#define RX_ALLOC_FACTOR_GRO 1
 #define RX_ALLOC_FACTOR_SKB (-2)
 
 /* This is the percentage fill level below which new RX descriptors
@@ -441,19 +441,19 @@ static void efx_rx_packet__check_len(str
 	efx_rx_queue_channel(rx_queue)->n_rx_overlength++;
 }
 
-/* Pass a received packet up through the generic LRO stack
+/* Pass a received packet up through the generic GRO stack
  *
  * Handles driverlink veto, and passes the fragment up via
- * the appropriate LRO method
+ * the appropriate GRO method
  */
-static void efx_rx_packet_lro(struct efx_channel *channel,
+static void efx_rx_packet_gro(struct efx_channel *channel,
 			      struct efx_rx_buffer *rx_buf,
 			      bool checksummed)
 {
 	struct napi_struct *napi = &channel->napi_str;
 	gro_result_t gro_result;
 
-	/* Pass the skb/page into the LRO engine */
+	/* Pass the skb/page into the GRO engine */
 	if (rx_buf->page) {
 		struct efx_nic *efx = channel->efx;
 		struct page *page = rx_buf->page;
@@ -499,7 +499,7 @@ static void efx_rx_packet_lro(struct efx
 	if (gro_result == GRO_NORMAL) {
 		channel->rx_alloc_level += RX_ALLOC_FACTOR_SKB;
 	} else if (gro_result != GRO_DROP) {
-		channel->rx_alloc_level += RX_ALLOC_FACTOR_LRO;
+		channel->rx_alloc_level += RX_ALLOC_FACTOR_GRO;
 		channel->irq_mod_score += 2;
 	}
 }
@@ -605,7 +605,7 @@ void __efx_rx_packet(struct efx_channel
 	}
 
 	if (likely(checksummed || rx_buf->page)) {
-		efx_rx_packet_lro(channel, rx_buf, checksummed);
+		efx_rx_packet_gro(channel, rx_buf, checksummed);
 		return;
 	}
 
@@ -628,7 +628,7 @@ void efx_rx_strategy(struct efx_channel
 {
 	enum efx_rx_alloc_method method = rx_alloc_method;
 
-	/* Only makes sense to use page based allocation if LRO is enabled */
+	/* Only makes sense to use page based allocation if GRO is enabled */
 	if (!(channel->efx->net_dev->features & NETIF_F_GRO)) {
 		method = RX_ALLOC_METHOD_SKB;
 	} else if (method == RX_ALLOC_METHOD_AUTO) {
@@ -639,7 +639,7 @@ void efx_rx_strategy(struct efx_channel
 			channel->rx_alloc_level = RX_ALLOC_LEVEL_MAX;
 
 		/* Decide on the allocation method */
-		method = ((channel->rx_alloc_level > RX_ALLOC_LEVEL_LRO) ?
+		method = ((channel->rx_alloc_level > RX_ALLOC_LEVEL_GRO) ?
 			  RX_ALLOC_METHOD_PAGE : RX_ALLOC_METHOD_SKB);
 	}
 

Re: Misuse of LRO, how widespread

[Ben Hutchings]

On Mon, 2010-12-06 at 14:33 -0800, Stephen Hemminger wrote:
> How about this?
> 
> Subject: sfc: convert references to LRO to GRO
> 
> This driver now uses Generic Receive Offload, not the older LRO.
> Change references to LRO in names and comments.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
Acked-by: Ben Hutchings <bhutchings@solarflare.com>

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: Misuse of LRO, how widespread

[David Miller]

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Mon, 06 Dec 2010 22:35:12 +0000

> On Mon, 2010-12-06 at 14:33 -0800, Stephen Hemminger wrote:
>> How about this?
>> 
>> Subject: sfc: convert references to LRO to GRO
>> 
>> This driver now uses Generic Receive Offload, not the older LRO.
>> Change references to LRO in names and comments.
>> 
>> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> Acked-by: Ben Hutchings <bhutchings@solarflare.com>

Applied to net-next-2.6, thanks!

Re: Misuse of LRO, how widespread

[Dimitris Michailidis]

Stephen Hemminger wrote:
> I inspected all drivers in net-next to see which drivers are using
> LRO and which ones are broken. Most concerning is that Chelsio
> and Solarflare drivers ignore ETH_FLAG_LRO.
> 
> The ones that are using LRO but allow disabling it:
>   qlcnic, netxen, mv643, s2io, myi10ge, bnx2x, ixgbe, vmxnet3
> 
> One driver seems confused about LRO vs GRO:
>   mlx4 - comments about LRO and depends on LRO but driver is using GRO
> 
> Drivers with not using ethtool interface to disable LRO:
>   pasemi_mac, sfc, ehea, cxgb3, cxgb4

cxgb4 uses GRO, not LRO.

Re: Misuse of LRO, how widespread

[Stephen Hemminger]

On Mon, 06 Dec 2010 13:15:42 -0800
Dimitris Michailidis <dm@chelsio.com> wrote:

> Stephen Hemminger wrote:
> > I inspected all drivers in net-next to see which drivers are using
> > LRO and which ones are broken. Most concerning is that Chelsio
> > and Solarflare drivers ignore ETH_FLAG_LRO.
> > 
> > The ones that are using LRO but allow disabling it:
> >   qlcnic, netxen, mv643, s2io, myi10ge, bnx2x, ixgbe, vmxnet3
> > 
> > One driver seems confused about LRO vs GRO:
> >   mlx4 - comments about LRO and depends on LRO but driver is using GRO
> > 
> > Drivers with not using ethtool interface to disable LRO:
> >   pasemi_mac, sfc, ehea, cxgb3, cxgb4
> 
> cxgb4 uses GRO, not LRO.

Ok. but cxgb3 still uses LRO (or it least calls it lro).

-- 

Re: Misuse of LRO, how widespread

[Dimitris Michailidis]

Stephen Hemminger wrote:
> On Mon, 06 Dec 2010 13:15:42 -0800
> Dimitris Michailidis <dm@chelsio.com> wrote:
> 
>> Stephen Hemminger wrote:
>>> I inspected all drivers in net-next to see which drivers are using
>>> LRO and which ones are broken. Most concerning is that Chelsio
>>> and Solarflare drivers ignore ETH_FLAG_LRO.
>>>
>>> The ones that are using LRO but allow disabling it:
>>>   qlcnic, netxen, mv643, s2io, myi10ge, bnx2x, ixgbe, vmxnet3
>>>
>>> One driver seems confused about LRO vs GRO:
>>>   mlx4 - comments about LRO and depends on LRO but driver is using GRO
>>>
>>> Drivers with not using ethtool interface to disable LRO:
>>>   pasemi_mac, sfc, ehea, cxgb3, cxgb4
>> cxgb4 uses GRO, not LRO.
> 
> Ok. but cxgb3 still uses LRO (or it least calls it lro).
> 

cxgb3 was the driver Herbert implemented GRO on I think, and he converted it 
to GRO.  It possibly has leftover LRO references as it was using LRO before.

Re: Misuse of LRO, how widespread

[Ben Hutchings]

On Mon, 2010-12-06 at 15:05 -0800, Dimitris Michailidis wrote:
> Stephen Hemminger wrote:
> > On Mon, 06 Dec 2010 13:15:42 -0800
> > Dimitris Michailidis <dm@chelsio.com> wrote:
> > 
> >> Stephen Hemminger wrote:
> >>> I inspected all drivers in net-next to see which drivers are using
> >>> LRO and which ones are broken. Most concerning is that Chelsio
> >>> and Solarflare drivers ignore ETH_FLAG_LRO.
> >>>
> >>> The ones that are using LRO but allow disabling it:
> >>>   qlcnic, netxen, mv643, s2io, myi10ge, bnx2x, ixgbe, vmxnet3
> >>>
> >>> One driver seems confused about LRO vs GRO:
> >>>   mlx4 - comments about LRO and depends on LRO but driver is using GRO
> >>>
> >>> Drivers with not using ethtool interface to disable LRO:
> >>>   pasemi_mac, sfc, ehea, cxgb3, cxgb4
> >> cxgb4 uses GRO, not LRO.
> > 
> > Ok. but cxgb3 still uses LRO (or it least calls it lro).
> > 
> 
> cxgb3 was the driver Herbert implemented GRO on I think, and he converted it 
> to GRO.  It possibly has leftover LRO references as it was using LRO before.

There's a fair amount of code setting LRO flags in various structures,
so either the driver still enables LRO in hardware/firmware or this is
dead code.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: Misuse of LRO, how widespread

[Dimitris Michailidis]

Ben Hutchings wrote:
> On Mon, 2010-12-06 at 15:05 -0800, Dimitris Michailidis wrote:
>> Stephen Hemminger wrote:
>>> On Mon, 06 Dec 2010 13:15:42 -0800
>>> Dimitris Michailidis <dm@chelsio.com> wrote:
>>>
>>>> Stephen Hemminger wrote:
>>>>> I inspected all drivers in net-next to see which drivers are using
>>>>> LRO and which ones are broken. Most concerning is that Chelsio
>>>>> and Solarflare drivers ignore ETH_FLAG_LRO.
>>>>>
>>>>> The ones that are using LRO but allow disabling it:
>>>>>   qlcnic, netxen, mv643, s2io, myi10ge, bnx2x, ixgbe, vmxnet3
>>>>>
>>>>> One driver seems confused about LRO vs GRO:
>>>>>   mlx4 - comments about LRO and depends on LRO but driver is using GRO
>>>>>
>>>>> Drivers with not using ethtool interface to disable LRO:
>>>>>   pasemi_mac, sfc, ehea, cxgb3, cxgb4
>>>> cxgb4 uses GRO, not LRO.
>>> Ok. but cxgb3 still uses LRO (or it least calls it lro).
>>>
>> cxgb3 was the driver Herbert implemented GRO on I think, and he converted it 
>> to GRO.  It possibly has leftover LRO references as it was using LRO before.
> 
> There's a fair amount of code setting LRO flags in various structures,
> so either the driver still enables LRO in hardware/firmware or this is
> dead code.

 From a quick look it appears to have a per queue lro flag.  HW/FW are not 
aware of LRO, whatever flags that driver has are for SW.  I'd say it's 
obsolete but probably not dead code.

> 
> Ben.
>