Message ID | 4A2D4AA7.6020204@gmail.com |
---|---|
State | Superseded, archived |
Delegated to: | David Miller |
Headers | show |
Eric Dumazet wrote: > OK I suspect driver is buggy since 2.6.10 days :) I browsed the git history for a while but don't see since when it might be broken. But again, I don't know the code nor the hardware. > Could you try this patch ? That makes quite some sense, except of two comments - pure speculation/guesses really, since I don't know the hw. The patch does not re-program the card when we change MTU (where we merely set internal rx_buf_sz, but don't tell the card about this). Maybe we should call this method in rtl8169_set_rxbufsize() too? I don't know almost anything about things like vlans for example, but guess they use some additional headers. Does those need some space too? Maybe its better to allocate "a bit" more room in skb for that stuff? Recompiling the driver now... /mjt -- 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
Eric Dumazet wrote: [] > OK I suspect driver is buggy since 2.6.10 days :) > > Could you try this patch ? Tried it, and it appears to work. Tried various MTU combinations and packet sizes. Checked iperf too, with and without the patch and with different MTU too, to be sure the patch does not introduce any slowdowns - everything looks sane. In case the incoming packet is larger than the RX buffer size, `errors' and `frames' RX stats gets incremented. The only somewhat odd thing is that rx path accepts packets larger than MTU by 3 bytes. For example, if I set mtu to 2000, the largest packet I can send is 2003 bytes; with mtu=2002, largest actual packet size is 2005 bytes. This is complete frame - in terms of ping size (ping -s) it's 1975 and 1977 bytes. That to say, maybe we still have some corner case somewhere, for packets larger than mtu by 1, 2 or 3 bytes. Also I didn't try MTU < 1500. Other than that, Tested-By: Michael Tokarev <mjt@tls.msk.ru> And by the way, your email client uses quoted-printable encoding. I had to use trivial perl one-liner to convert your patches to plaintext. JFYI. Thanks! /mjt > diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c > index 8247a94..c5ab5a8 100644 > --- a/drivers/net/r8169.c > +++ b/drivers/net/r8169.c > @@ -2357,10 +2357,10 @@ static u16 rtl_rw_cpluscmd(void __iomem *ioaddr) > return cmd; > } > > -static void rtl_set_rx_max_size(void __iomem *ioaddr) > +static void rtl_set_rx_max_size(void __iomem *ioaddr, unsigned int rx_buf_sz) > { > /* Low hurts. Let's disable the filtering. */ > - RTL_W16(RxMaxSize, 16383); > + RTL_W16(RxMaxSize, rx_buf_sz); > } > > static void rtl8169_set_magic_reg(void __iomem *ioaddr, unsigned mac_version) > @@ -2407,7 +2407,7 @@ static void rtl_hw_start_8169(struct net_device *dev) > > RTL_W8(EarlyTxThres, EarlyTxThld); > > - rtl_set_rx_max_size(ioaddr); > + rtl_set_rx_max_size(ioaddr, tp->rx_buf_sz); > > if ((tp->mac_version == RTL_GIGA_MAC_VER_01) || > (tp->mac_version == RTL_GIGA_MAC_VER_02) || > @@ -2668,7 +2668,7 @@ static void rtl_hw_start_8168(struct net_device *dev) > > RTL_W8(EarlyTxThres, EarlyTxThld); > > - rtl_set_rx_max_size(ioaddr); > + rtl_set_rx_max_size(ioaddr, tp->rx_buf_sz); > > tp->cp_cmd |= RTL_R16(CPlusCmd) | PktCntrDisable | INTT_1; > > @@ -2846,7 +2846,7 @@ static void rtl_hw_start_8101(struct net_device *dev) > > RTL_W8(EarlyTxThres, EarlyTxThld); > > - rtl_set_rx_max_size(ioaddr); > + rtl_set_rx_max_size(ioaddr, tp->rx_buf_sz); > > tp->cp_cmd |= rtl_rw_cpluscmd(ioaddr) | PCIMulRW; > > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- 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
Michael Tokarev a écrit : > Eric Dumazet wrote: > [] >> OK I suspect driver is buggy since 2.6.10 days :) >> >> Could you try this patch ? > > Tried it, and it appears to work. Tried various MTU combinations > and packet sizes. Checked iperf too, with and without the patch > and with different MTU too, to be sure the patch does not introduce > any slowdowns - everything looks sane. In case the incoming packet > is larger than the RX buffer size, `errors' and `frames' RX stats > gets incremented. > > The only somewhat odd thing is that rx path accepts packets larger > than MTU by 3 bytes. For example, if I set mtu to 2000, the > largest packet I can send is 2003 bytes; with mtu=2002, largest > actual packet size is 2005 bytes. This is complete frame - in > terms of ping size (ping -s) it's 1975 and 1977 bytes. That to > say, maybe we still have some corner case somewhere, for packets > larger than mtu by 1, 2 or 3 bytes. > > Also I didn't try MTU < 1500. > > Other than that, > > Tested-By: Michael Tokarev <mjt@tls.msk.ru> Could you confirm this last patch was ok without former two patches ? > > And by the way, your email client uses quoted-printable encoding. > I had to use trivial perl one-liner to convert your patches to > plaintext. JFYI. Ah yes, this is when I reply to one of your mail, thank you for the hint. When submitting a new mail, my thunderbird agent uses a regular "Content-Transfer-Encoding: 7bit" BTW, this driver uses NAPI, but still calls dev_kfree_skb_irq() in rtl8169_tx_interrupt() You probably can get better performance calling dev_kfree_skb(tx_skb->skb); instead @@ -3372,7 +3372,7 @@ static void rtl8169_tx_interrupt(struct net_device *dev, rtl8169_unmap_tx_skb(tp->pci_dev, tx_skb, tp->TxDescArray + entry); if (status & LastFrag) { - dev_kfree_skb_irq(tx_skb->skb); + dev_kfree_skb(tx_skb->skb); tx_skb->skb = NULL; } dirty_tx++; -- 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
Eric Dumazet wrote: > Michael Tokarev a écrit : >> Eric Dumazet wrote: >> [] >>> OK I suspect driver is buggy since 2.6.10 days :) >>> >>> Could you try this patch ? >> Tried it, and it appears to work. Tried various MTU combinations >> and packet sizes. Checked iperf too, with and without the patch >> and with different MTU too, to be sure the patch does not introduce >> any slowdowns - everything looks sane. In case the incoming packet >> is larger than the RX buffer size, `errors' and `frames' RX stats >> gets incremented. >> >> The only somewhat odd thing is that rx path accepts packets larger >> than MTU by 3 bytes. For example, if I set mtu to 2000, the >> largest packet I can send is 2003 bytes; with mtu=2002, largest >> actual packet size is 2005 bytes. This is complete frame - in >> terms of ping size (ping -s) it's 1975 and 1977 bytes. That to >> say, maybe we still have some corner case somewhere, for packets >> larger than mtu by 1, 2 or 3 bytes. >> >> Also I didn't try MTU < 1500. >> >> Other than that, >> >> Tested-By: Michael Tokarev <mjt@tls.msk.ru> > > Could you confirm this last patch was ok without former two patches ? Yes, it's the last patch without former two which were for debugging as I understand them. Got fresh 2.6.29.4 source and applied your last patch to it, recompiled. All the testing above were done this way. >> And by the way, your email client uses quoted-printable encoding. >> I had to use trivial perl one-liner to convert your patches to >> plaintext. JFYI. > > Ah yes, this is when I reply to one of your mail, thank you for the hint. > > When submitting a new mail, my thunderbird agent uses a regular "Content-Transfer-Encoding: 7bit" Heh. I know why it's this way. Due to your "Michael Tokarev a écrit" in first line. Which gets added by Thunderbird, and which causes it to force quoted-printable instead of 7bit, because of this "é". Mine offers "пишет" instead of "écrit" ("wrote" in English) and the result is similar. > BTW, this driver uses NAPI, but still calls dev_kfree_skb_irq() in rtl8169_tx_interrupt() > > You probably can get better performance calling dev_kfree_skb(tx_skb->skb); instead > > @@ -3372,7 +3372,7 @@ static void rtl8169_tx_interrupt(struct net_device *dev, > rtl8169_unmap_tx_skb(tp->pci_dev, tx_skb, tp->TxDescArray + entry); > > if (status & LastFrag) { > - dev_kfree_skb_irq(tx_skb->skb); > + dev_kfree_skb(tx_skb->skb); > tx_skb->skb = NULL; > } > dirty_tx++; Well, the performance is quite good -- 935Mb/sec according to iperf for TCP. With UDP I got 1.05Gb/sec, but CPU usage is 100% during all test time (for TCP test the CPU is in use for less than 5%). I'll try the change tomorrow (it's 01:27 here now already). Thank you for the good work! /mjt -- 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
Michael Tokarev <mjt@tls.msk.ru> writes: > Well, the performance is quite good -- 935Mb/sec according to iperf > for TCP. With UDP I got 1.05Gb/sec, but CPU usage is 100% during > all test time (for TCP test the CPU is in use for less than 5%). BTW you may want to try pktgen for TX bandwidth tests (a kernel module). In "clone" mode a slow IXP425 (ARM XScale) 533 MHz + Intel's 82541xx E1000 chip are able to transmit 1500-byte packets at full gigabit speed.
diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c index 8247a94..c5ab5a8 100644 --- a/drivers/net/r8169.c +++ b/drivers/net/r8169.c @@ -2357,10 +2357,10 @@ static u16 rtl_rw_cpluscmd(void __iomem *ioaddr) return cmd; } -static void rtl_set_rx_max_size(void __iomem *ioaddr) +static void rtl_set_rx_max_size(void __iomem *ioaddr, unsigned int rx_buf_sz) { /* Low hurts. Let's disable the filtering. */ - RTL_W16(RxMaxSize, 16383); + RTL_W16(RxMaxSize, rx_buf_sz); } static void rtl8169_set_magic_reg(void __iomem *ioaddr, unsigned mac_version) @@ -2407,7 +2407,7 @@ static void rtl_hw_start_8169(struct net_device *dev) RTL_W8(EarlyTxThres, EarlyTxThld); - rtl_set_rx_max_size(ioaddr); + rtl_set_rx_max_size(ioaddr, tp->rx_buf_sz); if ((tp->mac_version == RTL_GIGA_MAC_VER_01) || (tp->mac_version == RTL_GIGA_MAC_VER_02) || @@ -2668,7 +2668,7 @@ static void rtl_hw_start_8168(struct net_device *dev) RTL_W8(EarlyTxThres, EarlyTxThld); - rtl_set_rx_max_size(ioaddr); + rtl_set_rx_max_size(ioaddr, tp->rx_buf_sz); tp->cp_cmd |= RTL_R16(CPlusCmd) | PktCntrDisable | INTT_1; @@ -2846,7 +2846,7 @@ static void rtl_hw_start_8101(struct net_device *dev) RTL_W8(EarlyTxThres, EarlyTxThld); - rtl_set_rx_max_size(ioaddr); + rtl_set_rx_max_size(ioaddr, tp->rx_buf_sz); tp->cp_cmd |= rtl_rw_cpluscmd(ioaddr) | PCIMulRW;