rtl8168e-vl dropping tftp ack

rtl8168e-vl dropping tftp ack

[Stefan Bader]

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

There seems to be a subtle problem in handling the tftp part of
pxe booting a vm guest that is related to the specific hw /
driver.

I could observe this when using the r8169 driver from kernels
3.2, 3.5 and 3.8. It does not seem to happen when I replaced
the driver with r8168-8.035.00.tar.bz2 from the realtek site
(this I only did for kernel 3.2 for now).
It also seems to be worked-around by changing the tftp sequence
(see below).

The setup is as follows:

+-------------+
| TFTP server |
|      +------+
|      | eth0 +---+
+------+------+   |
                  |
+-------------+   |
| Xen host    |   |
|             |   |
| +-----------+   |
| | br0       |   |
| |    +------+   |   +-----------+
| |    | eth0 +---+   | Xen guest |
| |    +------+       +------+    |
| |    | vif0 +-------+ eth0 |    |
+-+----+------+       +------+----+

The Xen host has the Realtek NIC:

2:00.0 Ethernet controller [0200]: Realtek Semiconductor Co., Ltd.
       RTL8111/8168B PCI Express Gigabit Ethernet controller
       [10ec:8168] (rev 06)
       Subsystem: Micro-Star International Co., Ltd. Device [1462:7798]

This is configured to be the external port of a transparent bridge.
Normal networking is working without any apparent issues.
The problem arises when a xen guest tries to pxe boot. I had tcpdump
processes running on eth0 of the Xen host and on eth0 of the TFTP
server.

- DHCP of the Xen guest is successful (and can be pinged from then on)
- Transfer of pxelinux.0 via tftp works
- The guest then tries to load a config file named like the UUID of
  the virtual adapter. This does not exist and tftp returns "file not
  found"
  - Read Request, File: pxelinux.cfg/df80012c-a855-d44b-3fb2-4ef4932fdf83,
    Transfer type: octet, tsize\000=0\000, blksize\000=1408\000
  - Error Code, Code: File not found, Message: File not found
- Next the guest tries to transfer a config file which is named like
  the MAC address of the guest. This file exists.
  - Read Request, File: pxelinux.cfg/01-00-16-3e-cd-a2-82,
    Transfer type: octet, tsize\000=0\000, blksize\000=1408\000
- The tftp server returns an ACK together with the size of the file.
  - Option Acknowledgement, tsize\000=103\000, blksize\000=1408\000
- The guest would ACK with
  - Acknowledgement, Block: 0

However, that last package only shows up on the Xen host side tcpdump.
It never seems to make it out. The server retries its ack a few times
but without any reaction from the vm guest side.
Avoiding the "file not found" situation by creating a properly named
soft link on the tftp servers side seems to avoid this problem, too.
Also there are a few occations (but I cannot tell why those happen)
when the tftp sequence works even with the first file not found. But
in the majority of cases the transfer fails due to that ack of block 0
not reaching the tftp server.

I am not sure this is enough to find out what goes wrong but I can
add debugging code to the kernel driver or gather other info if
someone can hint me which things would be of interest.

Thanks,
-Stefan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]

Re: rtl8168e-vl dropping tftp ack

[Francois Romieu]

Stefan Bader <stefan.bader@canonical.com> :
[...]
> The Xen host has the Realtek NIC:
> 
> 2:00.0 Ethernet controller [0200]: Realtek Semiconductor Co., Ltd.
>        RTL8111/8168B PCI Express Gigabit Ethernet controller
>        [10ec:8168] (rev 06)
>        Subsystem: Micro-Star International Co., Ltd. Device [1462:7798]

Please include the XID line from the stock kernel.

[...]
> I am not sure this is enough to find out what goes wrong but I can
> add debugging code to the kernel driver or gather other info if
> someone can hint me which things would be of interest.

(tentative guess: small packet / checksumming problem)

A pcap capture file at the interface of the tftp server with Realtek's
driver (working) then with the kernel driver (failing) should help a
lot. No need to play the soft link trick but using a recent kernel will
be appreciated - fixes will be backported to stable kernels if needed.

-- 
Ueimor

Re: rtl8168e-vl dropping tftp ack

[Stefan Bader]

[-- Attachment #1.1: Type: text/plain, Size: 1660 bytes --]

On 17.04.2013 23:32, Francois Romieu wrote:
> Stefan Bader <stefan.bader@canonical.com> :
> [...]
>> The Xen host has the Realtek NIC:
>>
>> 2:00.0 Ethernet controller [0200]: Realtek Semiconductor Co., Ltd.
>>        RTL8111/8168B PCI Express Gigabit Ethernet controller
>>        [10ec:8168] (rev 06)
>>        Subsystem: Micro-Star International Co., Ltd. Device [1462:7798]
> 
> Please include the XID line from the stock kernel.

r8169 0000:02:00.0 eth0: RTL8168evl/8111evl at 0xffffc900000cc000,
 d4:3d:7e:37:f5:7b, XID 0c900800 IRQ 73
r8169 0000:02:00.0 eth0: jumbo features
 [frames: 9200 bytes, tx checksumming: ko]

r8168 Gigabit Ethernet driver 8.035.00-NAPI loaded
eth%d: RTL8168E-VL/8111E-VL at 0xffffc900000cc000, d4:3d:7e:37:f5:7b, IRQ 73
eth0: Identified chip type is 'RTL8168E-VL/8111E-VL'.

> 
> [...]
>> I am not sure this is enough to find out what goes wrong but I can
>> add debugging code to the kernel driver or gather other info if
>> someone can hint me which things would be of interest.
> 
> (tentative guess: small packet / checksumming problem)
> 
> A pcap capture file at the interface of the tftp server with Realtek's
> driver (working) then with the kernel driver (failing) should help a
> lot. No need to play the soft link trick but using a recent kernel will
> be appreciated - fixes will be backported to stable kernels if needed.
> 

This was taken with a 3.9-rc7 kernel running on the Xen host, listening on the
tftp server. First run is with the kernel driver and the second pass with
Realteks's driver (had to do minor adaption to handle the removed __devinit/exit
annotations).

[-- Attachment #1.2: guestpxe.pcap --]
[-- Type: application/vnd.tcpdump.pcap, Size: 89740 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]

Re: rtl8168e-vl dropping tftp ack

[Francois Romieu]

Stefan Bader <stefan.bader@canonical.com> :
[...]
> This was taken with a 3.9-rc7 kernel running on the Xen host, listening on the
> tftp server. First run is with the kernel driver and the second pass with

Please try the patch below with current kernel and capture traffic on
both the tftp server and r8169 interfaces.

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 4ecbe64..c1cd9f6 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -5747,7 +5747,7 @@ err_out:
 	return -EIO;
 }
 
-static inline void rtl8169_tso_csum(struct rtl8169_private *tp,
+static inline bool rtl8169_tso_csum(struct rtl8169_private *tp,
 				    struct sk_buff *skb, u32 *opts)
 {
 	const struct rtl_tx_desc_info *info = tx_desc_info + tp->txd_version;
@@ -5760,6 +5760,15 @@ static inline void rtl8169_tso_csum(struct rtl8169_private *tp,
 	} else if (skb->ip_summed == CHECKSUM_PARTIAL) {
 		const struct iphdr *ip = ip_hdr(skb);
 
+		if (unlikely(skb->len < ETH_ZLEN &&
+		    (tp->mac_version == RTL_GIGA_MAC_VER_34))) {
+			if (skb_padto(skb, ETH_ZLEN))
+				return false;
+			skb_checksum_help(skb);
+			skb_put(skb, ETH_ZLEN - skb->len);
+			return true;
+		}
+
 		if (ip->protocol == IPPROTO_TCP)
 			opts[offset] |= info->checksum.tcp;
 		else if (ip->protocol == IPPROTO_UDP)
@@ -5767,6 +5776,7 @@ static inline void rtl8169_tso_csum(struct rtl8169_private *tp,
 		else
 			WARN_ON_ONCE(1);
 	}
+	return true;
 }
 
 static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
@@ -5790,25 +5800,26 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
 	if (unlikely(le32_to_cpu(txd->opts1) & DescOwn))
 		goto err_stop_0;
 
+	opts[1] = cpu_to_le32(rtl8169_tx_vlan_tag(skb));
+	opts[0] = DescOwn;
+
+	if (!rtl8169_tso_csum(tp, skb, opts))
+		goto err_update_stats_0;
+
 	len = skb_headlen(skb);
 	mapping = dma_map_single(d, skb->data, len, DMA_TO_DEVICE);
 	if (unlikely(dma_mapping_error(d, mapping))) {
 		if (net_ratelimit())
 			netif_err(tp, drv, dev, "Failed to map TX DMA!\n");
-		goto err_dma_0;
+		goto err_free_skb_1;
 	}
 
 	tp->tx_skb[entry].len = len;
 	txd->addr = cpu_to_le64(mapping);
 
-	opts[1] = cpu_to_le32(rtl8169_tx_vlan_tag(skb));
-	opts[0] = DescOwn;
-
-	rtl8169_tso_csum(tp, skb, opts);
-
 	frags = rtl8169_xmit_frags(tp, skb, opts);
 	if (frags < 0)
-		goto err_dma_1;
+		goto err_unmap_2;
 	else if (frags)
 		opts[0] |= FirstFrag;
 	else {
@@ -5854,10 +5865,11 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
 
 	return NETDEV_TX_OK;
 
-err_dma_1:
+err_unmap_2:
 	rtl8169_unmap_tx_skb(d, tp->tx_skb + entry, txd);
-err_dma_0:
+err_free_skb_1:
 	dev_kfree_skb(skb);
+err_update_stats_0:
 	dev->stats.tx_dropped++;
 	return NETDEV_TX_OK;
 

RE: rtl8168e-vl dropping tftp ack

[hayeswang]

Francois Romieu [mailto:romieu@fr.zoreil.com] 
> Sent: Friday, April 19, 2013 5:55 AM
> To: Stefan Bader
> Cc: netdev@vger.kernel.org; nic_swsd
> Subject: Re: rtl8168e-vl dropping tftp ack
> 
[...]
> +		if (unlikely(skb->len < ETH_ZLEN &&
> +		    (tp->mac_version == RTL_GIGA_MAC_VER_34))) {
> +			if (skb_padto(skb, ETH_ZLEN))
> +				return false;
> +			skb_checksum_help(skb);
> +			skb_put(skb, ETH_ZLEN - skb->len);
> +			return true;
> +		}
> +

I don't think the hw checksum is enabled, because there are many successful
small packets before the failure. However, I also want to know if any skb->len <
60 occurs, so I think the checking should be included by the other case.
 
Best Regards,
Hayes

Re: rtl8168e-vl dropping tftp ack

[Francois Romieu]

hayeswang <hayeswang@realtek.com> :
[...]
> I don't think the hw checksum is enabled, because there are many successful
> small packets before the failure.

Tentative rationale: the first missing packet (112 at 88.147981) is a 60
bytes one whose tail does not contain zeroes only. Previous 60 bytes
packets would exhibit no change of content if they were they generously
cut or paded with zeroes.

> However, I also want to know if any skb->len < 60 occurs so I think the
> checking should be included by the other case.

I don't get it: arp aside, the normal trace in the capture file exhibits no
sub-60 bytes packet. Could you reformulate ?

-- 
Ueimor

RE: rtl8168e-vl dropping tftp ack

[hayeswang]

Francois Romieu [mailto:romieu@fr.zoreil.com] 
> Sent: Friday, April 19, 2013 2:14 PM
> To: Hayeswang
> Cc: 'Stefan Bader'; netdev@vger.kernel.org; nic_swsd
> Subject: Re: rtl8168e-vl dropping tftp ack
[...]
> Tentative rationale: the first missing packet (112 at 
> 88.147981) is a 60
> bytes one whose tail does not contain zeroes only. Previous 60 bytes
> packets would exhibit no change of content if they were they 
> generously cut or paded with zeroes.

It still has problem no matter the paded value is zeros or not, because the
length of the IP header would be modified by hw when using hw checksum.

[...]
> 
> I don't get it: arp aside, the normal trace in the capture 
> file exhibits no
> sub-60 bytes packet. Could you reformulate ?
> 

In brief, when the packet < 60, that is skb->len < 60, the hw should pad the
packet to 60 bytes automatically. However, in my memory, the rtl8168e-vl
wouldn't do this, and the packet wouldn't be sent. Therefore, the patch would be
similar with the followings.

--- r8169.c.org 2013-04-19 22:35:40.785759473 +0800
+++ r8169.c     2013-04-19 22:38:24.227189535 +0800
@@ -5760,12 +5760,29 @@ static inline void rtl8169_tso_csum(stru
        } else if (skb->ip_summed == CHECKSUM_PARTIAL) {
                const struct iphdr *ip = ip_hdr(skb);

+               if (unlikely(skb->len < ETH_ZLEN &&
+                   (tp->mac_version == RTL_GIGA_MAC_VER_34))) {
+                       if (skb_padto(skb, ETH_ZLEN))
+                               return false;
+                       skb_checksum_help(skb);
+                       skb_put(skb, ETH_ZLEN - skb->len);
+                       return true;
+               }
+
                if (ip->protocol == IPPROTO_TCP)
                        opts[offset] |= info->checksum.tcp;
                else if (ip->protocol == IPPROTO_UDP)
                        opts[offset] |= info->checksum.udp;
                else
                        WARN_ON_ONCE(1);
+       } else {
+               if (unlikely(skb->len < ETH_ZLEN &&
+                   (tp->mac_version == RTL_GIGA_MAC_VER_34))) {
+                       if (skb_padto(skb, ETH_ZLEN))
+                               return false;
+                       skb_put(skb, ETH_ZLEN - skb->len);
+                       return true;
+               }
        }
 }

Re: rtl8168e-vl dropping tftp ack

[Stefan Bader]

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

On 19.04.2013 09:49, hayeswang wrote:

>> I don't get it: arp aside, the normal trace in the capture 
>> file exhibits no
>> sub-60 bytes packet. Could you reformulate ?
>>
> 
> In brief, when the packet < 60, that is skb->len < 60, the hw should pad the
> packet to 60 bytes automatically. However, in my memory, the rtl8168e-vl
> wouldn't do this, and the packet wouldn't be sent. Therefore, the patch would be
> similar with the followings.
> 
> --- r8169.c.org 2013-04-19 22:35:40.785759473 +0800
> +++ r8169.c     2013-04-19 22:38:24.227189535 +0800
> @@ -5760,12 +5760,29 @@ static inline void rtl8169_tso_csum(stru
>         } else if (skb->ip_summed == CHECKSUM_PARTIAL) {
>                 const struct iphdr *ip = ip_hdr(skb);
> 
> +               if (unlikely(skb->len < ETH_ZLEN &&
> +                   (tp->mac_version == RTL_GIGA_MAC_VER_34))) {
> +                       if (skb_padto(skb, ETH_ZLEN))
> +                               return false;
> +                       skb_checksum_help(skb);
> +                       skb_put(skb, ETH_ZLEN - skb->len);
> +                       return true;
> +               }
> +
>                 if (ip->protocol == IPPROTO_TCP)
>                         opts[offset] |= info->checksum.tcp;
>                 else if (ip->protocol == IPPROTO_UDP)
>                         opts[offset] |= info->checksum.udp;
>                 else
>                         WARN_ON_ONCE(1);
> +       } else {
> +               if (unlikely(skb->len < ETH_ZLEN &&
> +                   (tp->mac_version == RTL_GIGA_MAC_VER_34))) {
> +                       if (skb_padto(skb, ETH_ZLEN))
> +                               return false;
> +                       skb_put(skb, ETH_ZLEN - skb->len);
> +                       return true;

Hardware is now back and I ran a couple of tests. First, the crash I ran into
previously was because I accidentally slipped in a skp_checksum_help into the
else case that Hayes proposed. Sorry.

So with the right version and both hunks in rtl8169_tso_csum, I see the problem
go away. I do observe the else case being hit on other occasions (not only while
doing pxe boots) from time to time. So that may happen more often than it causes
visible problems. I never saw the checksum case being hit.

Naively assuming that the checksum path never gets executed (which may be wrong)
I condensed the changes to:

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek
index 4ecbe64..1519a2f 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -5790,12 +5790,23 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *sk
        if (unlikely(le32_to_cpu(txd->opts1) & DescOwn))
                goto err_stop_0;

+       /*
+        * 8168E-VL hardware does not automatically pad to minimum
+        * length.
+        */
+       if (unlikely(skb->len < ETH_ZLEN &&
+           (tp->mac_version == RTL_GIGA_MAC_VER_34))) {
+               if (skb_padto(skb, ETH_ZLEN))
+                       goto err_update_stats_0;
+               skb_put(skb, ETH_ZLEN - skb->len);
+       }
+
        len = skb_headlen(skb);
        mapping = dma_map_single(d, skb->data, len, DMA_TO_DEVICE);
        if (unlikely(dma_mapping_error(d, mapping))) {
                if (net_ratelimit())
                        netif_err(tp, drv, dev, "Failed to map TX DMA!\n");
-               goto err_dma_0;
+               goto err_free_skb_1;
        }

        tp->tx_skb[entry].len = len;
@@ -5808,7 +5819,7 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,

        frags = rtl8169_xmit_frags(tp, skb, opts);
        if (frags < 0)
-               goto err_dma_1;
+               goto err_unmap_dma_1;
        else if (frags)
                opts[0] |= FirstFrag;
        else {
@@ -5854,10 +5865,11 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *sk

        return NETDEV_TX_OK;

-err_dma_1:
+err_unmap_dma_1:
        rtl8169_unmap_tx_skb(d, tp->tx_skb + entry, txd);
-err_dma_0:
+err_free_skb_1:
        dev_kfree_skb(skb);
+err_update_stats_0:
        dev->stats.tx_dropped++;
        return NETDEV_TX_OK;


This variant also seems to work (and does not drop the tftp packet). Whichever
path looks more suitable to you, you could add my tested-by to both.
We should also flag whichever change results from this as stable material as I
could see this happen even in v3.2 (just did not try further back).

Thanks,
Stefan

> +               }
>         }
>  }
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]

Re: rtl8168e-vl dropping tftp ack

[Francois Romieu]

Stefan Bader <stefan.bader@canonical.com> :
[...]
> This variant also seems to work (and does not drop the tftp packet). Whichever
> path looks more suitable to you, you could add my tested-by to both.

It's a bit late today. I'll submit it tomorrow.

> We should also flag whichever change results from this as stable material as I
> could see this happen even in v3.2 (just did not try further back).

Davem usually takes care of the last stable. I'll forward it to every
8168evl-supporting stable kernel once it gets included in mainline.

Thanks.

-- 
Ueimor

Re: rtl8168e-vl dropping tftp ack

[Stefan Bader]

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

On 18.04.2013 23:55, Francois Romieu wrote:
> Stefan Bader <stefan.bader@canonical.com> :
> [...]
>> This was taken with a 3.9-rc7 kernel running on the Xen host, listening on the
>> tftp server. First run is with the kernel driver and the second pass with
> 
> Please try the patch below with current kernel and capture traffic on
> both the tftp server and r8169 interfaces.
> 
> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
> index 4ecbe64..c1cd9f6 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
> @@ -5747,7 +5747,7 @@ err_out:
>  	return -EIO;
>  }
>  
> -static inline void rtl8169_tso_csum(struct rtl8169_private *tp,
> +static inline bool rtl8169_tso_csum(struct rtl8169_private *tp,
>  				    struct sk_buff *skb, u32 *opts)
>  {
>  	const struct rtl_tx_desc_info *info = tx_desc_info + tp->txd_version;
> @@ -5760,6 +5760,15 @@ static inline void rtl8169_tso_csum(struct rtl8169_private *tp,
>  	} else if (skb->ip_summed == CHECKSUM_PARTIAL) {
>  		const struct iphdr *ip = ip_hdr(skb);
>  
> +		if (unlikely(skb->len < ETH_ZLEN &&
> +		    (tp->mac_version == RTL_GIGA_MAC_VER_34))) {
> +			if (skb_padto(skb, ETH_ZLEN))
> +				return false;
> +			skb_checksum_help(skb);
> +			skb_put(skb, ETH_ZLEN - skb->len);
> +			return true;
> +		}
> +
>  		if (ip->protocol == IPPROTO_TCP)
>  			opts[offset] |= info->checksum.tcp;
>  		else if (ip->protocol == IPPROTO_UDP)
> @@ -5767,6 +5776,7 @@ static inline void rtl8169_tso_csum(struct rtl8169_private *tp,
>  		else
>  			WARN_ON_ONCE(1);
>  	}
> +	return true;
>  }
>  
>  static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
> @@ -5790,25 +5800,26 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
>  	if (unlikely(le32_to_cpu(txd->opts1) & DescOwn))
>  		goto err_stop_0;
>  
> +	opts[1] = cpu_to_le32(rtl8169_tx_vlan_tag(skb));
> +	opts[0] = DescOwn;
> +
> +	if (!rtl8169_tso_csum(tp, skb, opts))
> +		goto err_update_stats_0;
> +
>  	len = skb_headlen(skb);
>  	mapping = dma_map_single(d, skb->data, len, DMA_TO_DEVICE);
>  	if (unlikely(dma_mapping_error(d, mapping))) {
>  		if (net_ratelimit())
>  			netif_err(tp, drv, dev, "Failed to map TX DMA!\n");
> -		goto err_dma_0;
> +		goto err_free_skb_1;
>  	}
>  
>  	tp->tx_skb[entry].len = len;
>  	txd->addr = cpu_to_le64(mapping);
>  
> -	opts[1] = cpu_to_le32(rtl8169_tx_vlan_tag(skb));
> -	opts[0] = DescOwn;
> -
> -	rtl8169_tso_csum(tp, skb, opts);
> -
>  	frags = rtl8169_xmit_frags(tp, skb, opts);
>  	if (frags < 0)
> -		goto err_dma_1;
> +		goto err_unmap_2;
>  	else if (frags)
>  		opts[0] |= FirstFrag;
>  	else {
> @@ -5854,10 +5865,11 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
>  
>  	return NETDEV_TX_OK;
>  
> -err_dma_1:
> +err_unmap_2:
>  	rtl8169_unmap_tx_skb(d, tp->tx_skb + entry, txd);
> -err_dma_0:
> +err_free_skb_1:
>  	dev_kfree_skb(skb);
> +err_update_stats_0:
>  	dev->stats.tx_dropped++;
>  	return NETDEV_TX_OK;
>  
> 

Might be a while till I will be able to run any tests since msi was so generous
to provide an uefi bios that can brick itself from their efi shell. *sigh*
I had tried the combination of your change and Hayes' but that would cause
panics in skb_checksum_help (not sure which of the two) on boot.

-Stefan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]