[net-next PATCH 0/5] net: Add helper for padding short Ethernet frames

[net-next PATCH 0/5] net: Add helper for padding short Ethernet frames

[Alexander Duyck]

This patch series adds a helper to pad short Ethernet frames.  The general
idea is to clean up a number of code paths that were all writing their own
versions of the same or similar function.

An added advantage is that this will help to discourage introducing new
bugs as in at least one case I found the skb->len had been updated, but the
tail pointer update was overlooked.

---

Alexander Duyck (5):
      etherdevice: Add function for handling padding frame to ETH_ZLEN
      ethernet/intel: Use eth_skb_pad helper
      niu: Use eth_skb_pad helper
      myri10ge: use eth_skb_pad helper
      r8169: Use eth_skb_pad function


 drivers/net/ethernet/intel/e1000/e1000_main.c     |    8 ++------
 drivers/net/ethernet/intel/fm10k/fm10k_main.c     |   11 +++--------
 drivers/net/ethernet/intel/igb/igb_main.c         |   11 +++--------
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c     |   11 +++--------
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |   11 +++--------
 drivers/net/ethernet/myricom/myri10ge/myri10ge.c  |   15 +++++----------
 drivers/net/ethernet/realtek/r8169.c              |   12 ++----------
 drivers/net/ethernet/sun/niu.c                    |    9 ++-------
 include/linux/etherdevice.h                       |   21 +++++++++++++++++++++
 9 files changed, 44 insertions(+), 65 deletions(-)

--

[net-next PATCH 1/5] etherdevice: Add function for handling padding frame to ETH_ZLEN

[Alexander Duyck]

This patch adds a simple function for padding a frame up to the minimum
size for for Ethernet.  The motivation behind it is that there are a number
of implementations throughout the network device drivers that are all doing
the same thing, but each a little bit differently and as a result several
implementations contain bugs such as updating the length without updating
the tail offset and other similar issues.

Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
---
 include/linux/etherdevice.h |   21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
index 733980f..7e436f3 100644
--- a/include/linux/etherdevice.h
+++ b/include/linux/etherdevice.h
@@ -392,4 +392,25 @@ static inline unsigned long compare_ether_header(const void *a, const void *b)
 #endif
 }
 
+/**
+ * eth_skb_pad - Pad buffer to mininum number of octets for Ethernet frame
+ * @skb: Buffer to pad
+ *
+ * An Ethernet frame should have a minimum size of 60 bytes.  This function
+ * takes short frames and pads them with zeros up to the 60 byte limit.
+ */
+static inline int eth_skb_pad(struct sk_buff *skb)
+{
+	unsigned int size = skb->len;
+
+	if (unlikely(size < ETH_ZLEN)) {
+		size = ETH_ZLEN - size;
+		if (skb_pad(skb, size))
+			return -ENOMEM;
+		__skb_put(skb, size);
+	}
+
+	return 0;
+}
+
 #endif	/* _LINUX_ETHERDEVICE_H */

[net-next PATCH 2/5] ethernet/intel: Use eth_skb_pad helper

[Alexander Duyck]

Update the Intel Ethernet drivers to use eth_skb_pad() instead of doing
their own implementations of the function.

Also this cleans up two other spots where skb_pad was called but the length
and tail pointers were being manipulated directly instead of just having
the padding length added via __skb_put.

Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
---
 drivers/net/ethernet/intel/e1000/e1000_main.c     |    8 ++------
 drivers/net/ethernet/intel/fm10k/fm10k_main.c     |   11 +++--------
 drivers/net/ethernet/intel/igb/igb_main.c         |   11 +++--------
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c     |   11 +++--------
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |   11 +++--------
 5 files changed, 14 insertions(+), 38 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
index 24f3986..862d198 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_main.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
@@ -3136,12 +3136,8 @@ static netdev_tx_t e1000_xmit_frame(struct sk_buff *skb,
 	 * packets may get corrupted during padding by HW.
 	 * To WA this issue, pad all small packets manually.
 	 */
-	if (skb->len < ETH_ZLEN) {
-		if (skb_pad(skb, ETH_ZLEN - skb->len))
-			return NETDEV_TX_OK;
-		skb->len = ETH_ZLEN;
-		skb_set_tail_pointer(skb, ETH_ZLEN);
-	}
+	if (eth_skb_pad(skb))
+		return NETDEV_TX_OK;
 
 	mss = skb_shinfo(skb)->gso_size;
 	/* The controller does a simple calculation to
diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_main.c b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
index 73457ed..91516ae 100644
--- a/drivers/net/ethernet/intel/fm10k/fm10k_main.c
+++ b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
@@ -578,14 +578,9 @@ static bool fm10k_cleanup_headers(struct fm10k_ring *rx_ring,
 	if (skb_is_nonlinear(skb))
 		fm10k_pull_tail(rx_ring, rx_desc, skb);
 
-	/* if skb_pad returns an error the skb was freed */
-	if (unlikely(skb->len < 60)) {
-		int pad_len = 60 - skb->len;
-
-		if (skb_pad(skb, pad_len))
-			return true;
-		__skb_put(skb, pad_len);
-	}
+	/* if eth_skb_pad returns an error the skb was freed */
+	if (eth_skb_pad(skb))
+		return true;
 
 	return false;
 }
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index b0e12e7..3cb54d0 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -6843,14 +6843,9 @@ static bool igb_cleanup_headers(struct igb_ring *rx_ring,
 	if (skb_is_nonlinear(skb))
 		igb_pull_tail(rx_ring, rx_desc, skb);
 
-	/* if skb_pad returns an error the skb was freed */
-	if (unlikely(skb->len < 60)) {
-		int pad_len = 60 - skb->len;
-
-		if (skb_pad(skb, pad_len))
-			return true;
-		__skb_put(skb, pad_len);
-	}
+	/* if eth_skb_pad returns an error the skb was freed */
+	if (eth_skb_pad(skb))
+		return true;
 
 	return false;
 }
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 932f779..7d0991a 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -1766,14 +1766,9 @@ static bool ixgbe_cleanup_headers(struct ixgbe_ring *rx_ring,
 		return false;
 
 #endif
-	/* if skb_pad returns an error the skb was freed */
-	if (unlikely(skb->len < 60)) {
-		int pad_len = 60 - skb->len;
-
-		if (skb_pad(skb, pad_len))
-			return true;
-		__skb_put(skb, pad_len);
-	}
+	/* if eth_skb_pad returns an error the skb was freed */
+	if (eth_skb_pad(skb))
+		return true;
 
 	return false;
 }
diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
index 755f71f..465d6a8 100644
--- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
+++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
@@ -612,14 +612,9 @@ static bool ixgbevf_cleanup_headers(struct ixgbevf_ring *rx_ring,
 	if (skb_is_nonlinear(skb))
 		ixgbevf_pull_tail(rx_ring, skb);
 
-	/* if skb_pad returns an error the skb was freed */
-	if (unlikely(skb->len < 60)) {
-		int pad_len = 60 - skb->len;
-
-		if (skb_pad(skb, pad_len))
-			return true;
-		__skb_put(skb, pad_len);
-	}
+	/* if eth_skb_pad returns an error the skb was freed */
+	if (eth_skb_pad(skb))
+		return true;
 
 	return false;
 }

[net-next PATCH 3/5] niu: Use eth_skb_pad helper

[Alexander Duyck]

Replace the standard layout for padding an ethernet frame with the
eth_skb_pad call.

Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
---
 drivers/net/ethernet/sun/niu.c |    9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/sun/niu.c b/drivers/net/ethernet/sun/niu.c
index 904fd1a..4aaa324 100644
--- a/drivers/net/ethernet/sun/niu.c
+++ b/drivers/net/ethernet/sun/niu.c
@@ -6651,13 +6651,8 @@ static netdev_tx_t niu_start_xmit(struct sk_buff *skb,
 		return NETDEV_TX_BUSY;
 	}
 
-	if (skb->len < ETH_ZLEN) {
-		unsigned int pad_bytes = ETH_ZLEN - skb->len;
-
-		if (skb_pad(skb, pad_bytes))
-			goto out;
-		skb_put(skb, pad_bytes);
-	}
+	if (eth_skb_pad(skb))
+		goto out;
 
 	len = sizeof(struct tx_pkt_hdr) + 15;
 	if (skb_headroom(skb) < len) {

[net-next PATCH 4/5] myri10ge: use eth_skb_pad helper

[Alexander Duyck]

Update myri10ge to use eth_skb_pad helper.  This also corrects a minor
issue as the driver was updating length without updating the tail pointer.

Cc: Hyong-Youb Kim <hykim@myri.com>
Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
---
 drivers/net/ethernet/myricom/myri10ge/myri10ge.c |   15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/myricom/myri10ge/myri10ge.c b/drivers/net/ethernet/myricom/myri10ge/myri10ge.c
index 9e7e3f1..af09905 100644
--- a/drivers/net/ethernet/myricom/myri10ge/myri10ge.c
+++ b/drivers/net/ethernet/myricom/myri10ge/myri10ge.c
@@ -2913,16 +2913,11 @@ again:
 		flags |= MXGEFW_FLAGS_SMALL;
 
 		/* pad frames to at least ETH_ZLEN bytes */
-		if (unlikely(skb->len < ETH_ZLEN)) {
-			if (skb_padto(skb, ETH_ZLEN)) {
-				/* The packet is gone, so we must
-				 * return 0 */
-				ss->stats.tx_dropped += 1;
-				return NETDEV_TX_OK;
-			}
-			/* adjust the len to account for the zero pad
-			 * so that the nic can know how long it is */
-			skb->len = ETH_ZLEN;
+		if (eth_skb_pad(skb)) {
+			/* The packet is gone, so we must
+			 * return 0 */
+			ss->stats.tx_dropped += 1;
+			return NETDEV_TX_OK;
 		}
 	}
 

[net-next PATCH 5/5] r8169: Use eth_skb_pad function

[Alexander Duyck]

Replace rtl_skb_pad with eth_skb_pad since they do the same thing.

Cc: Realtek linux nic maintainers <nic_swsd@realtek.com>
Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
---
 drivers/net/ethernet/realtek/r8169.c |   12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index cf154f7..7ff4646 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -6832,14 +6832,6 @@ err_out:
 	return -EIO;
 }
 
-static bool rtl_skb_pad(struct sk_buff *skb)
-{
-	if (skb_padto(skb, ETH_ZLEN))
-		return false;
-	skb_put(skb, ETH_ZLEN - skb->len);
-	return true;
-}
-
 static bool rtl_test_hw_pad_bug(struct rtl8169_private *tp, struct sk_buff *skb)
 {
 	return skb->len < ETH_ZLEN && tp->mac_version == RTL_GIGA_MAC_VER_34;
@@ -6980,7 +6972,7 @@ static bool rtl8169_tso_csum_v2(struct rtl8169_private *tp,
 		u8 ip_protocol;
 
 		if (unlikely(rtl_test_hw_pad_bug(tp, skb)))
-			return skb_checksum_help(skb) == 0 && rtl_skb_pad(skb);
+			return skb_checksum_help(skb) == 0 && eth_skb_pad(skb);
 
 		if (transport_offset > TCPHO_MAX) {
 			netif_warn(tp, tx_err, tp->dev,
@@ -7015,7 +7007,7 @@ static bool rtl8169_tso_csum_v2(struct rtl8169_private *tp,
 		opts[1] |= transport_offset << TCPHO_SHIFT;
 	} else {
 		if (unlikely(rtl_test_hw_pad_bug(tp, skb)))
-			return rtl_skb_pad(skb);
+			return eth_skb_pad(skb);
 	}
 
 	return true;

Re: [net-next PATCH 2/5] ethernet/intel: Use eth_skb_pad helper

[Eric Dumazet]

On Tue, 2014-11-25 at 14:44 -0800, Alexander Duyck wrote:
> Update the Intel Ethernet drivers to use eth_skb_pad() instead of doing
> their own implementations of the function.
> 
> Also this cleans up two other spots where skb_pad was called but the length
> and tail pointers were being manipulated directly instead of just having
> the padding length added via __skb_put.
> 
> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
> ---
>  drivers/net/ethernet/intel/e1000/e1000_main.c     |    8 ++------
>  drivers/net/ethernet/intel/fm10k/fm10k_main.c     |   11 +++--------
>  drivers/net/ethernet/intel/igb/igb_main.c         |   11 +++--------
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c     |   11 +++--------
>  drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |   11 +++--------
>  5 files changed, 14 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
> index 24f3986..862d198 100644
> --- a/drivers/net/ethernet/intel/e1000/e1000_main.c
> +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
> @@ -3136,12 +3136,8 @@ static netdev_tx_t e1000_xmit_frame(struct sk_buff *skb,
>  	 * packets may get corrupted during padding by HW.
>  	 * To WA this issue, pad all small packets manually.
>  	 */
> -	if (skb->len < ETH_ZLEN) {
> -		if (skb_pad(skb, ETH_ZLEN - skb->len))
> -			return NETDEV_TX_OK;
> -		skb->len = ETH_ZLEN;
> -		skb_set_tail_pointer(skb, ETH_ZLEN);
> -	}
> +	if (eth_skb_pad(skb))
> +		return NETDEV_TX_OK;
>  

Its a bit sad almost no driver increments some drop counter.

This probably could be generically done in eth_skb_pad()

atomic_long_inc(&skb->dev->tx_dropped)

Re: [net-next PATCH 5/5] r8169: Use eth_skb_pad function

[Francois Romieu]

Alexander Duyck <alexander.h.duyck@redhat.com> :
> Replace rtl_skb_pad with eth_skb_pad since they do the same thing.

rtl_skb_pad returns true on success.

eth_skb_pad returns 0 on success.

-- 
Ueimor

Re: [net-next PATCH 5/5] r8169: Use eth_skb_pad function

[Alexander Duyck]

On 11/25/2014 04:02 PM, Francois Romieu wrote:
> Alexander Duyck <alexander.h.duyck@redhat.com> :
>> Replace rtl_skb_pad with eth_skb_pad since they do the same thing.
> rtl_skb_pad returns true on success.
>
> eth_skb_pad returns 0 on success.
>

Okay, that is an easy enough fix.  I will just test for !eth_skb_pad in
the v2 version.

- Alex

Re: [net-next PATCH 2/5] ethernet/intel: Use eth_skb_pad helper

[Alexander Duyck]

On 11/25/2014 03:14 PM, Eric Dumazet wrote:
> On Tue, 2014-11-25 at 14:44 -0800, Alexander Duyck wrote:
>> Update the Intel Ethernet drivers to use eth_skb_pad() instead of doing
>> their own implementations of the function.
>>
>> Also this cleans up two other spots where skb_pad was called but the length
>> and tail pointers were being manipulated directly instead of just having
>> the padding length added via __skb_put.
>>
>> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
>> ---
>>  drivers/net/ethernet/intel/e1000/e1000_main.c     |    8 ++------
>>  drivers/net/ethernet/intel/fm10k/fm10k_main.c     |   11 +++--------
>>  drivers/net/ethernet/intel/igb/igb_main.c         |   11 +++--------
>>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c     |   11 +++--------
>>  drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |   11 +++--------
>>  5 files changed, 14 insertions(+), 38 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
>> index 24f3986..862d198 100644
>> --- a/drivers/net/ethernet/intel/e1000/e1000_main.c
>> +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
>> @@ -3136,12 +3136,8 @@ static netdev_tx_t e1000_xmit_frame(struct sk_buff *skb,
>>  	 * packets may get corrupted during padding by HW.
>>  	 * To WA this issue, pad all small packets manually.
>>  	 */
>> -	if (skb->len < ETH_ZLEN) {
>> -		if (skb_pad(skb, ETH_ZLEN - skb->len))
>> -			return NETDEV_TX_OK;
>> -		skb->len = ETH_ZLEN;
>> -		skb_set_tail_pointer(skb, ETH_ZLEN);
>> -	}
>> +	if (eth_skb_pad(skb))
>> +		return NETDEV_TX_OK;
>>  
> Its a bit sad almost no driver increments some drop counter.
>
> This probably could be generically done in eth_skb_pad()
>
> atomic_long_inc(&skb->dev->tx_dropped)

The only problem is eth_skb_pad is called in the Rx path of some drivers
as well.

I wonder if we couldn't make this some sort of netdevice attribute to
indicate what the smallest frame we can handle is and then just pad the
frame to that as a part of __dev_xmit_skb.  Then we could do that
outside of the locks and take care of it before we even hit the qdisc layer.

- Alex

Re: [net-next PATCH 1/5] etherdevice: Add function for handling padding frame to ETH_ZLEN

[Florian Fainelli]

On 25/11/14 14:44, Alexander Duyck wrote:
> This patch adds a simple function for padding a frame up to the minimum
> size for for Ethernet.  The motivation behind it is that there are a number
> of implementations throughout the network device drivers that are all doing
> the same thing, but each a little bit differently and as a result several
> implementations contain bugs such as updating the length without updating
> the tail offset and other similar issues.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
> ---
>  include/linux/etherdevice.h |   21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
> index 733980f..7e436f3 100644
> --- a/include/linux/etherdevice.h
> +++ b/include/linux/etherdevice.h
> @@ -392,4 +392,25 @@ static inline unsigned long compare_ether_header(const void *a, const void *b)
>  #endif
>  }
>  
> +/**
> + * eth_skb_pad - Pad buffer to mininum number of octets for Ethernet frame
> + * @skb: Buffer to pad
> + *
> + * An Ethernet frame should have a minimum size of 60 bytes.  This function
> + * takes short frames and pads them with zeros up to the 60 byte limit.

minimum size without FCS

> + */
> +static inline int eth_skb_pad(struct sk_buff *skb)
> +{
> +	unsigned int size = skb->len;
> +
> +	if (unlikely(size < ETH_ZLEN)) {
> +		size = ETH_ZLEN - size;
> +		if (skb_pad(skb, size))
> +			return -ENOMEM;
> +		__skb_put(skb, size);
> +	}

most drivers call skb_padto(skb, ETH_ZLEN), besides the extra
__skb_put() here, this is not different.

> +
> +	return 0;
> +}
> +
>  #endif	/* _LINUX_ETHERDEVICE_H */
> 
> --
> 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
> 

Re: [net-next PATCH 2/5] ethernet/intel: Use eth_skb_pad helper

[Florian Fainelli]

On 25/11/14 16:44, Alexander Duyck wrote:
> On 11/25/2014 03:14 PM, Eric Dumazet wrote:
>> On Tue, 2014-11-25 at 14:44 -0800, Alexander Duyck wrote:
>>> Update the Intel Ethernet drivers to use eth_skb_pad() instead of doing
>>> their own implementations of the function.
>>>
>>> Also this cleans up two other spots where skb_pad was called but the length
>>> and tail pointers were being manipulated directly instead of just having
>>> the padding length added via __skb_put.
>>>
>>> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>>> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
>>> ---
>>>  drivers/net/ethernet/intel/e1000/e1000_main.c     |    8 ++------
>>>  drivers/net/ethernet/intel/fm10k/fm10k_main.c     |   11 +++--------
>>>  drivers/net/ethernet/intel/igb/igb_main.c         |   11 +++--------
>>>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c     |   11 +++--------
>>>  drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |   11 +++--------
>>>  5 files changed, 14 insertions(+), 38 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
>>> index 24f3986..862d198 100644
>>> --- a/drivers/net/ethernet/intel/e1000/e1000_main.c
>>> +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
>>> @@ -3136,12 +3136,8 @@ static netdev_tx_t e1000_xmit_frame(struct sk_buff *skb,
>>>  	 * packets may get corrupted during padding by HW.
>>>  	 * To WA this issue, pad all small packets manually.
>>>  	 */
>>> -	if (skb->len < ETH_ZLEN) {
>>> -		if (skb_pad(skb, ETH_ZLEN - skb->len))
>>> -			return NETDEV_TX_OK;
>>> -		skb->len = ETH_ZLEN;
>>> -		skb_set_tail_pointer(skb, ETH_ZLEN);
>>> -	}
>>> +	if (eth_skb_pad(skb))
>>> +		return NETDEV_TX_OK;
>>>  
>> Its a bit sad almost no driver increments some drop counter.
>>
>> This probably could be generically done in eth_skb_pad()
>>
>> atomic_long_inc(&skb->dev->tx_dropped)
> 
> The only problem is eth_skb_pad is called in the Rx path of some drivers
> as well.
> 
> I wonder if we couldn't make this some sort of netdevice attribute to
> indicate what the smallest frame we can handle is and then just pad the
> frame to that as a part of __dev_xmit_skb.  Then we could do that
> outside of the locks and take care of it before we even hit the qdisc layer.

One potential problem could be that the padding size varies at runtime
based on e.g: netdev features, connection to an Ethernet switch etc...
we could probably just advertise whatever maximum padding we need once
and for all and just assume that any skb we get called with in a
driver's xmit() has the required padding, that is probably fine too.
--
Florian

Re: [net-next PATCH 2/5] ethernet/intel: Use eth_skb_pad helper

[Eric Dumazet]

On Tue, 2014-11-25 at 16:44 -0800, Alexander Duyck wrote:


> 
> I wonder if we couldn't make this some sort of netdevice attribute to
> indicate what the smallest frame we can handle is and then just pad the
> frame to that as a part of __dev_xmit_skb.  Then we could do that
> outside of the locks and take care of it before we even hit the qdisc layer.

Well, many NIC do not have such restriction.

Do we have some kind of counters of skb->head reallocations caused by
this padding ?

I believe I finally have an idea why we had various + 15 in skb
allocations in TCP stack !

We probably should reinstate them, as ACK packets can be 54 bytes long.

Re: [net-next PATCH 1/5] etherdevice: Add function for handling padding frame to ETH_ZLEN

[Alexander Duyck]

On 11/25/2014 04:56 PM, Florian Fainelli wrote:
> On 25/11/14 14:44, Alexander Duyck wrote:
>> This patch adds a simple function for padding a frame up to the minimum
>> size for for Ethernet.  The motivation behind it is that there are a number
>> of implementations throughout the network device drivers that are all doing
>> the same thing, but each a little bit differently and as a result several
>> implementations contain bugs such as updating the length without updating
>> the tail offset and other similar issues.
>>
>> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
>> ---
>>  include/linux/etherdevice.h |   21 +++++++++++++++++++++
>>  1 file changed, 21 insertions(+)
>>
>> diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
>> index 733980f..7e436f3 100644
>> --- a/include/linux/etherdevice.h
>> +++ b/include/linux/etherdevice.h
>> @@ -392,4 +392,25 @@ static inline unsigned long compare_ether_header(const void *a, const void *b)
>>  #endif
>>  }
>>  
>> +/**
>> + * eth_skb_pad - Pad buffer to mininum number of octets for Ethernet frame
>> + * @skb: Buffer to pad
>> + *
>> + * An Ethernet frame should have a minimum size of 60 bytes.  This function
>> + * takes short frames and pads them with zeros up to the 60 byte limit.
> 
> minimum size without FCS
> 
>> + */
>> +static inline int eth_skb_pad(struct sk_buff *skb)
>> +{
>> +	unsigned int size = skb->len;
>> +
>> +	if (unlikely(size < ETH_ZLEN)) {
>> +		size = ETH_ZLEN - size;
>> +		if (skb_pad(skb, size))
>> +			return -ENOMEM;
>> +		__skb_put(skb, size);
>> +	}
> 
> most drivers call skb_padto(skb, ETH_ZLEN), besides the extra
> __skb_put() here, this is not different.

Yes, but the __skb_put is the difference.  Calling skb_padto() only
zeros out the bytes following the tail, but it doesn't add the extra
bytes to the frame nor does it update the tail.  As such if we only call
skb_padto() before handing a short frame off to the network stack the
network stack will still have issues with it being too short.

The difference between the two is that skb_padto() is really meant for
transmit, whereas eth_skb_pad is really meant for receive.

- Alex

Re: [net-next PATCH 2/5] ethernet/intel: Use eth_skb_pad helper

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 25 Nov 2014 17:43:05 -0800

> I believe I finally have an idea why we had various + 15 in skb
> allocations in TCP stack !

It was so that you could do one level of tunneling with "for
free".  Or that is my recollection.

Those + 15 existed way before any of these padto() calls even
existed.

Re: [net-next PATCH 2/5] ethernet/intel: Use eth_skb_pad helper

[Eric Dumazet]

On Tue, 2014-11-25 at 22:19 -0500, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Tue, 25 Nov 2014 17:43:05 -0800
> 
> > I believe I finally have an idea why we had various + 15 in skb
> > allocations in TCP stack !
> 
> It was so that you could do one level of tunneling with "for
> free".  Or that is my recollection.
> 
> Those + 15 existed way before any of these padto() calls even
> existed.


Well, tunneling is added in front of the packet. Thats why we use
MAX_TCP_HEADER.

The +15 is in fact because TCP stack wanted to make sure the eventual
padding (needing tailroom, not headroom) was possible...

Note that ack packets never used the +15, but other packets did.

Re: [net-next PATCH 2/5] ethernet/intel: Use eth_skb_pad helper

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 25 Nov 2014 20:01:13 -0800

[ I am still intrigued, CC:'ing Alexey ]

> On Tue, 2014-11-25 at 22:19 -0500, David Miller wrote:
>> From: Eric Dumazet <eric.dumazet@gmail.com>
>> Date: Tue, 25 Nov 2014 17:43:05 -0800
>> 
>> > I believe I finally have an idea why we had various + 15 in skb
>> > allocations in TCP stack !
>> 
>> It was so that you could do one level of tunneling with "for
>> free".  Or that is my recollection.
>> 
>> Those + 15 existed way before any of these padto() calls even
>> existed.
> 
> Well, tunneling is added in front of the packet. Thats why we use
> MAX_TCP_HEADER.
> 
> The +15 is in fact because TCP stack wanted to make sure the eventual
> padding (needing tailroom, not headroom) was possible...
> 
> Note that ack packets never used the +15, but other packets did.

Alexey, do you remember exact reason for that +15 everywhere in TCP
packet allocation sizing?

I thought it was for headroom, but as Eric shows that's illogical,
it can only be for tailroom considerations.