[PATCH next] r8169: add support for Byte Queue Limits

[PATCH next] r8169: add support for Byte Queue Limits

[Florian Westphal]

tested on RTL8168d/8111d.

Cc: Francois Romieu <romieu@fr.zoreil.com>
Cc: Hayes Wang <hayeswang@realtek.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 drivers/net/ethernet/realtek/r8169.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 1d81238..73c6ad1 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -4712,6 +4712,8 @@ static void rtl_hw_reset(struct rtl8169_private *tp)
 	RTL_W8(ChipCmd, CmdReset);
 
 	rtl_udelay_loop_wait_low(tp, &rtl_chipcmd_cond, 100, 100);
+
+	netdev_reset_queue(tp->dev);
 }
 
 static void rtl_request_uncached_firmware(struct rtl8169_private *tp)
@@ -6592,6 +6594,8 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
 
 	txd->opts2 = cpu_to_le32(opts[1]);
 
+	netdev_sent_queue(dev, skb->len);
+
 	skb_tx_timestamp(skb);
 
 	wmb();
@@ -6691,6 +6695,7 @@ static void rtl8169_pcierr_interrupt(struct net_device *dev)
 static void rtl_tx(struct net_device *dev, struct rtl8169_private *tp)
 {
 	unsigned int dirty_tx, tx_left;
+	unsigned bytes_compl = 0, pkts_compl = 0;
 
 	dirty_tx = tp->dirty_tx;
 	smp_rmb();
@@ -6709,10 +6714,8 @@ static void rtl_tx(struct net_device *dev, struct rtl8169_private *tp)
 		rtl8169_unmap_tx_skb(&tp->pci_dev->dev, tx_skb,
 				     tp->TxDescArray + entry);
 		if (status & LastFrag) {
-			u64_stats_update_begin(&tp->tx_stats.syncp);
-			tp->tx_stats.packets++;
-			tp->tx_stats.bytes += tx_skb->skb->len;
-			u64_stats_update_end(&tp->tx_stats.syncp);
+			pkts_compl++;
+			bytes_compl += tx_skb->skb->len;
 			dev_kfree_skb_any(tx_skb->skb);
 			tx_skb->skb = NULL;
 		}
@@ -6721,6 +6724,13 @@ static void rtl_tx(struct net_device *dev, struct rtl8169_private *tp)
 	}
 
 	if (tp->dirty_tx != dirty_tx) {
+		netdev_completed_queue(tp->dev, pkts_compl, bytes_compl);
+
+		u64_stats_update_begin(&tp->tx_stats.syncp);
+		tp->tx_stats.packets += pkts_compl;
+		tp->tx_stats.bytes += bytes_compl;
+		u64_stats_update_end(&tp->tx_stats.syncp);
+
 		tp->dirty_tx = dirty_tx;
 		/* Sync with rtl8169_start_xmit:
 		 * - publish dirty_tx ring index (write barrier)
-- 
1.8.1.5

Re: [PATCH next] r8169: add support for Byte Queue Limits

[Eric Dumazet]

On Mon, 2014-09-29 at 22:56 +0200, Florian Westphal wrote:
> tested on RTL8168d/8111d.
> 
> Cc: Francois Romieu <romieu@fr.zoreil.com>
> Cc: Hayes Wang <hayeswang@realtek.com>
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  drivers/net/ethernet/realtek/r8169.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)

Thanks Florian, this looks good to me.

Acked-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH next] r8169: add support for Byte Queue Limits

[Eric Dumazet]

On Mon, 2014-09-29 at 22:56 +0200, Florian Westphal wrote:
> tested on RTL8168d/8111d.

One small nit :

> @@ -6691,6 +6695,7 @@ static void rtl8169_pcierr_interrupt(struct net_device *dev)
>  static void rtl_tx(struct net_device *dev, struct rtl8169_private *tp)
>  {
>  	unsigned int dirty_tx, tx_left;
> +	unsigned bytes_compl = 0, pkts_compl = 0;

unsigned int bytes_compl = 0, pkts_compl = 0;

Re: [PATCH next] r8169: add support for Byte Queue Limits

[Tom Herbert]

Hi Florian,

Thanks for doing this! Can you add a little description in commit log
on test results, something that would indicate accounting is correct.

Thanks,
Tom


On Mon, Sep 29, 2014 at 1:56 PM, Florian Westphal <fw@strlen.de> wrote:
> tested on RTL8168d/8111d.
>
> Cc: Francois Romieu <romieu@fr.zoreil.com>
> Cc: Hayes Wang <hayeswang@realtek.com>
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  drivers/net/ethernet/realtek/r8169.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
> index 1d81238..73c6ad1 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
> @@ -4712,6 +4712,8 @@ static void rtl_hw_reset(struct rtl8169_private *tp)
>         RTL_W8(ChipCmd, CmdReset);
>
>         rtl_udelay_loop_wait_low(tp, &rtl_chipcmd_cond, 100, 100);
> +
> +       netdev_reset_queue(tp->dev);
>  }
>
>  static void rtl_request_uncached_firmware(struct rtl8169_private *tp)
> @@ -6592,6 +6594,8 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
>
>         txd->opts2 = cpu_to_le32(opts[1]);
>
> +       netdev_sent_queue(dev, skb->len);
> +
>         skb_tx_timestamp(skb);
>
>         wmb();
> @@ -6691,6 +6695,7 @@ static void rtl8169_pcierr_interrupt(struct net_device *dev)
>  static void rtl_tx(struct net_device *dev, struct rtl8169_private *tp)
>  {
>         unsigned int dirty_tx, tx_left;
> +       unsigned bytes_compl = 0, pkts_compl = 0;
>
>         dirty_tx = tp->dirty_tx;
>         smp_rmb();
> @@ -6709,10 +6714,8 @@ static void rtl_tx(struct net_device *dev, struct rtl8169_private *tp)
>                 rtl8169_unmap_tx_skb(&tp->pci_dev->dev, tx_skb,
>                                      tp->TxDescArray + entry);
>                 if (status & LastFrag) {
> -                       u64_stats_update_begin(&tp->tx_stats.syncp);
> -                       tp->tx_stats.packets++;
> -                       tp->tx_stats.bytes += tx_skb->skb->len;
> -                       u64_stats_update_end(&tp->tx_stats.syncp);
> +                       pkts_compl++;
> +                       bytes_compl += tx_skb->skb->len;
>                         dev_kfree_skb_any(tx_skb->skb);
>                         tx_skb->skb = NULL;
>                 }
> @@ -6721,6 +6724,13 @@ static void rtl_tx(struct net_device *dev, struct rtl8169_private *tp)
>         }
>
>         if (tp->dirty_tx != dirty_tx) {
> +               netdev_completed_queue(tp->dev, pkts_compl, bytes_compl);
> +
> +               u64_stats_update_begin(&tp->tx_stats.syncp);
> +               tp->tx_stats.packets += pkts_compl;
> +               tp->tx_stats.bytes += bytes_compl;
> +               u64_stats_update_end(&tp->tx_stats.syncp);
> +
>                 tp->dirty_tx = dirty_tx;
>                 /* Sync with rtl8169_start_xmit:
>                  * - publish dirty_tx ring index (write barrier)
> --
> 1.8.1.5
>
> --
> 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: [PATCH next] r8169: add support for Byte Queue Limits

[Florian Westphal]

Tom Herbert <therbert@google.com> wrote:
> Thanks for doing this! Can you add a little description in commit log
> on test results, something that would indicate accounting is correct.

Do you have any specific test in mind?

I ran 'super_netperf 40' for 20 minutes without stalls/hangups and
bql monitor seemed to look ok.

Anything else I should use/test with?

Thanks,
Florian

Re: [PATCH next] r8169: add support for Byte Queue Limits

[Tom Herbert]

On Mon, Sep 29, 2014 at 2:11 PM, Florian Westphal <fw@strlen.de> wrote:
> Tom Herbert <therbert@google.com> wrote:
>> Thanks for doing this! Can you add a little description in commit log
>> on test results, something that would indicate accounting is correct.
>
> Do you have any specific test in mind?
>
> I ran 'super_netperf 40' for 20 minutes without stalls/hangups and
> bql monitor seemed to look ok.
>
> Anything else I should use/test with?
>
Watch inflight and limit in the byte_queue_limits for the queue.
inflight must always go back to zero when link goes idle.

> Thanks,
> Florian

Re: [PATCH next] r8169: add support for Byte Queue Limits

[Florian Westphal]

Tom Herbert <therbert@google.com> wrote:
> Watch inflight and limit in the byte_queue_limits for the queue.
> inflight must always go back to zero when link goes idle.

Yes, inflight goes to 0 when link is idle.

Output of
while true; do
    for n in inflight limit; do
      echo -n $n\ ; cat $n;
      done; sleep 1;
done

during netperf run, 100mbit peer:

inflight 0
limit 3028
inflight 6056
limit 4542
[ no changes during test ]
limit 4542
inflight 3028
limit 6122
inflight 0
limit 6122
[ changed cable to 1gbit peer, restart netperf ]
inflight 37850
limit 36336
inflight 33308
limit 31794
inflight 33308
limit 31794
inflight 27252
limit 25738
[ no changes during test ]
inflight 27252
limit 25738
inflight 0
limit 28766
[ change cable to 100mbit peer, restart netperf ]
limit 28766
inflight 27370
limit 28766
inflight 4542
limit 5990
inflight 6056
limit 4542
inflight 6056
limit 4542
inflight 6056
limit 4542
inflight 6056
limit 4542
inflight 6056
limit 4542
inflight 6056
limit 4542
inflight 6056
limit 4542
inflight 6056
limit 4542
inflight 0

[ end of test ]

I think thats what its supposed to look like :-)

Re: [PATCH next] r8169: add support for Byte Queue Limits

[Tom Herbert]

On Tue, Sep 30, 2014 at 12:42 AM, Florian Westphal <fw@strlen.de> wrote:
> Tom Herbert <therbert@google.com> wrote:
>> Watch inflight and limit in the byte_queue_limits for the queue.
>> inflight must always go back to zero when link goes idle.
>
> Yes, inflight goes to 0 when link is idle.
>
> Output of
> while true; do
>     for n in inflight limit; do
>       echo -n $n\ ; cat $n;
>       done; sleep 1;
> done
>
> during netperf run, 100mbit peer:
>
> inflight 0
> limit 3028
> inflight 6056
> limit 4542
> [ no changes during test ]
> limit 4542
> inflight 3028
> limit 6122
> inflight 0
> limit 6122
> [ changed cable to 1gbit peer, restart netperf ]
> inflight 37850
> limit 36336
> inflight 33308
> limit 31794
> inflight 33308
> limit 31794
> inflight 27252
> limit 25738
> [ no changes during test ]
> inflight 27252
> limit 25738
> inflight 0
> limit 28766
> [ change cable to 100mbit peer, restart netperf ]
> limit 28766
> inflight 27370
> limit 28766
> inflight 4542
> limit 5990
> inflight 6056
> limit 4542
> inflight 6056
> limit 4542
> inflight 6056
> limit 4542
> inflight 6056
> limit 4542
> inflight 6056
> limit 4542
> inflight 6056
> limit 4542
> inflight 6056
> limit 4542
> inflight 6056
> limit 4542
> inflight 0
>
> [ end of test ]
>
> I think thats what its supposed to look like :-)

Looks good!

Acked-by: Tom Herbert <therbert@google.com>