[PATCH net-next 0/3] Gianfar byte queue limits

[PATCH net-next 0/3] Gianfar byte queue limits

[Paul Gortmaker]

The BQL support here is unchanged from what I posted earlier as an
RFC[1] -- with the exception of the fact that I'm now happier with
the runtime testing vs. the simple "hey it boots" that I'd done
for the RFC.  Plus I added a couple trivial cleanup patches.

For testing, I made a couple spiders homeless by reviving an ancient
10baseT hub.  I connected an sbc8349 into that, and connected the
yellowing hub into a GigE 16port, which was also connected to the
recipient x86 box.

Gianfar saw the interface as follows:

fsl-gianfar e0024000.ethernet: eth0: mac: 00:a0:1e:a0:26:5a
fsl-gianfar e0024000.ethernet: eth0: Running with NAPI enabled
fsl-gianfar e0024000.ethernet: eth0: RX BD ring size for Q[0]: 256
fsl-gianfar e0024000.ethernet: eth0: TX BD ring size for Q[0]: 256
PHY: mdio@e0024520:19 - Link is Up - 10/Half

With the sbc8349 being diskless, I simply used an scp of /proc/kcore
to the connected x86 box as a rudimentary Tx heavy workload.

BQL data was collected by changing into the dir:

  /sys/devices/e0000000.soc8349/e0024000.ethernet/net/eth0/queues/tx-0/byte_queue_limits

and running the following:

  for i in * ; do echo -n $i": " ; cat $i ; done

Running with the defaults, data like below was typical:

hold_time: 1000
inflight: 4542
limit: 3456
limit_max: 1879048192
limit_min: 0

hold_time: 1000
inflight: 4542
limit: 3378
limit_max: 1879048192
limit_min: 0

i.e. 2 or 3 MTU sized packets in flight and the limit value lying
somewhere between those two values.

The interesting thing is that the interactive speed reported by scp
seemed somewhat erratic, ranging from ~450 to ~700kB/s. (This was
the only traffic on the old junk - perhaps expected oscillations such
as those seen in isolated ARED tests?)  Average speed for 100M was:

104857600 bytes (105 MB) copied, 172.616 s, 607 kB/s

Anyway, back to BQL testing; setting the values as follows:

hold_time: 1000
inflight: 1514
limit: 1400
limit_max: 1400
limit_min: 1000

had the effect of serializing the interface to a single packet, and
the crusty old hub seemed much happier with this arrangement, keeping
a constant speed and achieving the following on a 100MB Tx block:

104857600 bytes (105 MB) copied, 112.52 s, 932 kB/s

It might be interesting to know more about why the defaults suffer
the slowdown, but the hub could possibly be ancient spec violating
trash.  Definitely something that nobody would ever use for anything
today. (aside from contrived tests like this)

But it did give me an example of where I could see the effects of
changing the BQL settings, and I'm reasonably confident they are
working as expected.

Paul.
---

[1] http://lists.openwall.net/netdev/2012/01/06/64

Paul Gortmaker (3):
  gianfar: Add support for byte queue limits.
  gianfar: constify giant block of status descriptor strings
  gianfar: delete orphaned version strings and dead macros

 drivers/net/ethernet/freescale/gianfar.c         |   22 ++++++++++++++++------
 drivers/net/ethernet/freescale/gianfar.h         |    3 ---
 drivers/net/ethernet/freescale/gianfar_ethtool.c |    2 +-
 3 files changed, 17 insertions(+), 10 deletions(-)

-- 
1.7.9.1

[PATCH net-next 1/3] gianfar: Add support for byte queue limits.

[Paul Gortmaker]

Add support for byte queue limits (BQL), based on the similar
modifications made to intel/igb/igb_main.c from Eric Dumazet
in commit bdbc063129e811264cd6c311d8c2d9b95de01231
	"igb: Add support for byte queue limits."

A local variable for tx_queue->qindex was introduced in
gfar_clean_tx_ring, since it is now used often enough to warrant it,
and it cleans up the readability somewhat as well.

Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/net/ethernet/freescale/gianfar.c |   19 ++++++++++++++++---
 1 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index adb0ae4..a4c934b 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -1755,9 +1755,12 @@ static void free_skb_resources(struct gfar_private *priv)
 
 	/* Go through all the buffer descriptors and free their data buffers */
 	for (i = 0; i < priv->num_tx_queues; i++) {
+		struct netdev_queue *txq;
 		tx_queue = priv->tx_queue[i];
+		txq = netdev_get_tx_queue(tx_queue->dev, tx_queue->qindex);
 		if(tx_queue->tx_skbuff)
 			free_skb_tx_queue(tx_queue);
+		netdev_tx_reset_queue(txq);
 	}
 
 	for (i = 0; i < priv->num_rx_queues; i++) {
@@ -2217,6 +2220,8 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev)
 		lstatus |= BD_LFLAG(TXBD_CRC | TXBD_READY) | skb_headlen(skb);
 	}
 
+	netdev_tx_sent_queue(txq, skb->len);
+
 	/*
 	 * We can work in parallel with gfar_clean_tx_ring(), except
 	 * when modifying num_txbdfree. Note that we didn't grab the lock
@@ -2460,6 +2465,7 @@ static void gfar_align_skb(struct sk_buff *skb)
 static int gfar_clean_tx_ring(struct gfar_priv_tx_q *tx_queue)
 {
 	struct net_device *dev = tx_queue->dev;
+	struct netdev_queue *txq;
 	struct gfar_private *priv = netdev_priv(dev);
 	struct gfar_priv_rx_q *rx_queue = NULL;
 	struct txbd8 *bdp, *next = NULL;
@@ -2471,10 +2477,13 @@ static int gfar_clean_tx_ring(struct gfar_priv_tx_q *tx_queue)
 	int frags = 0, nr_txbds = 0;
 	int i;
 	int howmany = 0;
+	int tqi = tx_queue->qindex;
+	unsigned int bytes_sent = 0;
 	u32 lstatus;
 	size_t buflen;
 
-	rx_queue = priv->rx_queue[tx_queue->qindex];
+	rx_queue = priv->rx_queue[tqi];
+	txq = netdev_get_tx_queue(dev, tqi);
 	bdp = tx_queue->dirty_tx;
 	skb_dirtytx = tx_queue->skb_dirtytx;
 
@@ -2533,6 +2542,8 @@ static int gfar_clean_tx_ring(struct gfar_priv_tx_q *tx_queue)
 			bdp = next_txbd(bdp, base, tx_ring_size);
 		}
 
+		bytes_sent += skb->len;
+
 		/*
 		 * If there's room in the queue (limit it to rx_buffer_size)
 		 * we add this skb back into the pool, if it's the right size
@@ -2557,13 +2568,15 @@ static int gfar_clean_tx_ring(struct gfar_priv_tx_q *tx_queue)
 	}
 
 	/* If we freed a buffer, we can restart transmission, if necessary */
-	if (__netif_subqueue_stopped(dev, tx_queue->qindex) && tx_queue->num_txbdfree)
-		netif_wake_subqueue(dev, tx_queue->qindex);
+	if (__netif_subqueue_stopped(dev, tqi) && tx_queue->num_txbdfree)
+		netif_wake_subqueue(dev, tqi);
 
 	/* Update dirty indicators */
 	tx_queue->skb_dirtytx = skb_dirtytx;
 	tx_queue->dirty_tx = bdp;
 
+	netdev_tx_completed_queue(txq, howmany, bytes_sent);
+
 	return howmany;
 }
 
-- 
1.7.9.1

Re: [PATCH net-next 1/3] gianfar: Add support for byte queue limits.

[Eric Dumazet]

Le dimanche 18 mars 2012 à 12:56 -0400, Paul Gortmaker a écrit :

...

>  		 * we add this skb back into the pool, if it's the right size
> @@ -2557,13 +2568,15 @@ static int gfar_clean_tx_ring(struct gfar_priv_tx_q *tx_queue)
>  	}
>  
>  	/* If we freed a buffer, we can restart transmission, if necessary */
> -	if (__netif_subqueue_stopped(dev, tx_queue->qindex) && tx_queue->num_txbdfree)
> -		netif_wake_subqueue(dev, tx_queue->qindex);
> +	if (__netif_subqueue_stopped(dev, tqi) && tx_queue->num_txbdfree)
> +		netif_wake_subqueue(dev, tqi);
>  

You can use netif_tx_queue_stopped(txq) here instead of
__netif_subqueue_stopped(dev, tqi) 

>  	/* Update dirty indicators */
>  	tx_queue->skb_dirtytx = skb_dirtytx;
>  	tx_queue->dirty_tx = bdp;
>  
> +	netdev_tx_completed_queue(txq, howmany, bytes_sent);
> +
>  	return howmany;
>  }
>  

Re: [PATCH net-next 1/3] gianfar: Add support for byte queue limits.

[Paul Gortmaker]

On Sun, Mar 18, 2012 at 4:20 PM, Eric Dumazet <eric.dumazet@gmail.com> wrot=
e:
> Le dimanche 18 mars 2012 =E0 12:56 -0400, Paul Gortmaker a =E9crit :
>
> ...
>
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* we add this skb back into the pool, if =
it's the right size
>> @@ -2557,13 +2568,15 @@ static int gfar_clean_tx_ring(struct gfar_priv_t=
x_q *tx_queue)
>> =A0 =A0 =A0 }
>>
>> =A0 =A0 =A0 /* If we freed a buffer, we can restart transmission, if nec=
essary */
>> - =A0 =A0 if (__netif_subqueue_stopped(dev, tx_queue->qindex) && tx_queu=
e->num_txbdfree)
>> - =A0 =A0 =A0 =A0 =A0 =A0 netif_wake_subqueue(dev, tx_queue->qindex);
>> + =A0 =A0 if (__netif_subqueue_stopped(dev, tqi) && tx_queue->num_txbdfr=
ee)
>> + =A0 =A0 =A0 =A0 =A0 =A0 netif_wake_subqueue(dev, tqi);
>>
>
> You can use netif_tx_queue_stopped(txq) here instead of
> __netif_subqueue_stopped(dev, tqi)

Yes, and it looks better too.  I will do it as a patch #4 since I think
there is some small value in leaving the above patch chunk alone,
since it makes it clear that it was just the introduction of a local
variable and the code was otherwise unchanged here.

Will resend shortly....

Thanks,
Paul.
---

>
>> =A0 =A0 =A0 /* Update dirty indicators */
>> =A0 =A0 =A0 tx_queue->skb_dirtytx =3D skb_dirtytx;
>> =A0 =A0 =A0 tx_queue->dirty_tx =3D bdp;
>>
>> + =A0 =A0 netdev_tx_completed_queue(txq, howmany, bytes_sent);
>> +
>> =A0 =A0 =A0 return howmany;
>> =A0}
>>
>
>
> --
> 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 =A0http://vger.kernel.org/majordomo-info.html

[PATCH net-next 2/3] gianfar: constify giant block of status descriptor strings

[Paul Gortmaker]

Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/net/ethernet/freescale/gianfar_ethtool.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar_ethtool.c b/drivers/net/ethernet/freescale/gianfar_ethtool.c
index 5a78d55..8d74efd 100644
--- a/drivers/net/ethernet/freescale/gianfar_ethtool.c
+++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c
@@ -58,7 +58,7 @@ static void gfar_gringparam(struct net_device *dev, struct ethtool_ringparam *rv
 static int gfar_sringparam(struct net_device *dev, struct ethtool_ringparam *rvals);
 static void gfar_gdrvinfo(struct net_device *dev, struct ethtool_drvinfo *drvinfo);
 
-static char stat_gstrings[][ETH_GSTRING_LEN] = {
+static const char stat_gstrings[][ETH_GSTRING_LEN] = {
 	"rx-dropped-by-kernel",
 	"rx-large-frame-errors",
 	"rx-short-frame-errors",
-- 
1.7.9.1

[PATCH net-next 3/3] gianfar: delete orphaned version strings and dead macros

[Paul Gortmaker]

There were two version strings, and neither one was being used.
Also in the same proximity were some unused #define that were
left over from the past.  Delete them all.

Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/net/ethernet/freescale/gianfar.c |    3 ---
 drivers/net/ethernet/freescale/gianfar.h |    3 ---
 2 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index a4c934b..6e66cc3 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -104,10 +104,7 @@
 #include "fsl_pq_mdio.h"
 
 #define TX_TIMEOUT      (1*HZ)
-#undef BRIEF_GFAR_ERRORS
-#undef VERBOSE_GFAR_ERRORS
 
-const char gfar_driver_name[] = "Gianfar Ethernet";
 const char gfar_driver_version[] = "1.3";
 
 static int gfar_enet_open(struct net_device *dev);
diff --git a/drivers/net/ethernet/freescale/gianfar.h b/drivers/net/ethernet/freescale/gianfar.h
index 4fe0f34..fc2488a 100644
--- a/drivers/net/ethernet/freescale/gianfar.h
+++ b/drivers/net/ethernet/freescale/gianfar.h
@@ -78,11 +78,8 @@ struct ethtool_rx_list {
 #define INCREMENTAL_BUFFER_SIZE 512
 
 #define PHY_INIT_TIMEOUT 100000
-#define GFAR_PHY_CHANGE_TIME 2
 
-#define DEVICE_NAME "%s: Gianfar Ethernet Controller Version 1.2, "
 #define DRV_NAME "gfar-enet"
-extern const char gfar_driver_name[];
 extern const char gfar_driver_version[];
 
 /* MAXIMUM NUMBER OF QUEUES SUPPORTED */
-- 
1.7.9.1

Re: [PATCH net-next 0/3] Gianfar byte queue limits

[Eric Dumazet]

Le dimanche 18 mars 2012 à 12:56 -0400, Paul Gortmaker a écrit :
> The BQL support here is unchanged from what I posted earlier as an
> RFC[1] -- with the exception of the fact that I'm now happier with
> the runtime testing vs. the simple "hey it boots" that I'd done
> for the RFC.  Plus I added a couple trivial cleanup patches.
> 
> For testing, I made a couple spiders homeless by reviving an ancient
> 10baseT hub.  I connected an sbc8349 into that, and connected the
> yellowing hub into a GigE 16port, which was also connected to the
> recipient x86 box.
> 
> Gianfar saw the interface as follows:
> 
> fsl-gianfar e0024000.ethernet: eth0: mac: 00:a0:1e:a0:26:5a
> fsl-gianfar e0024000.ethernet: eth0: Running with NAPI enabled
> fsl-gianfar e0024000.ethernet: eth0: RX BD ring size for Q[0]: 256
> fsl-gianfar e0024000.ethernet: eth0: TX BD ring size for Q[0]: 256
> PHY: mdio@e0024520:19 - Link is Up - 10/Half
> 
> With the sbc8349 being diskless, I simply used an scp of /proc/kcore
> to the connected x86 box as a rudimentary Tx heavy workload.
> 
> BQL data was collected by changing into the dir:
> 
>   /sys/devices/e0000000.soc8349/e0024000.ethernet/net/eth0/queues/tx-0/byte_queue_limits
> 
> and running the following:
> 
>   for i in * ; do echo -n $i": " ; cat $i ; done
> 
> Running with the defaults, data like below was typical:
> 
> hold_time: 1000
> inflight: 4542
> limit: 3456
> limit_max: 1879048192
> limit_min: 0
> 
> hold_time: 1000
> inflight: 4542
> limit: 3378
> limit_max: 1879048192
> limit_min: 0
> 
> i.e. 2 or 3 MTU sized packets in flight and the limit value lying
> somewhere between those two values.
> 
> The interesting thing is that the interactive speed reported by scp
> seemed somewhat erratic, ranging from ~450 to ~700kB/s. (This was
> the only traffic on the old junk - perhaps expected oscillations such
> as those seen in isolated ARED tests?)  Average speed for 100M was:
> 
> 104857600 bytes (105 MB) copied, 172.616 s, 607 kB/s
> 

Still half duplex, or full duplex ?

Limiting to one packet on half duplex might avoid collisions :)

> Anyway, back to BQL testing; setting the values as follows:
> 
> hold_time: 1000
> inflight: 1514
> limit: 1400
> limit_max: 1400
> limit_min: 1000
> 
> had the effect of serializing the interface to a single packet, and
> the crusty old hub seemed much happier with this arrangement, keeping
> a constant speed and achieving the following on a 100MB Tx block:
> 
> 104857600 bytes (105 MB) copied, 112.52 s, 932 kB/s
> 
> It might be interesting to know more about why the defaults suffer
> the slowdown, but the hub could possibly be ancient spec violating
> trash.  Definitely something that nobody would ever use for anything
> today. (aside from contrived tests like this)
> 
> But it did give me an example of where I could see the effects of
> changing the BQL settings, and I'm reasonably confident they are
> working as expected.
> 

Seems pretty good to me !

Re: [PATCH net-next 0/3] Gianfar byte queue limits

[Paul Gortmaker]

On Sun, Mar 18, 2012 at 4:30 PM, Eric Dumazet <eric.dumazet@gmail.com> wrot=
e:
> Le dimanche 18 mars 2012 =E0 12:56 -0400, Paul Gortmaker a =E9crit :
>> The BQL support here is unchanged from what I posted earlier as an
>> RFC[1] -- with the exception of the fact that I'm now happier with
>> the runtime testing vs. the simple "hey it boots" that I'd done
>> for the RFC. =A0Plus I added a couple trivial cleanup patches.
>>
>> For testing, I made a couple spiders homeless by reviving an ancient
>> 10baseT hub. =A0I connected an sbc8349 into that, and connected the
>> yellowing hub into a GigE 16port, which was also connected to the
>> recipient x86 box.
>>
>> Gianfar saw the interface as follows:
>>
>> fsl-gianfar e0024000.ethernet: eth0: mac: 00:a0:1e:a0:26:5a
>> fsl-gianfar e0024000.ethernet: eth0: Running with NAPI enabled
>> fsl-gianfar e0024000.ethernet: eth0: RX BD ring size for Q[0]: 256
>> fsl-gianfar e0024000.ethernet: eth0: TX BD ring size for Q[0]: 256
>> PHY: mdio@e0024520:19 - Link is Up - 10/Half
>>
>> With the sbc8349 being diskless, I simply used an scp of /proc/kcore
>> to the connected x86 box as a rudimentary Tx heavy workload.
>>
>> BQL data was collected by changing into the dir:
>>
>> =A0 /sys/devices/e0000000.soc8349/e0024000.ethernet/net/eth0/queues/tx-0=
/byte_queue_limits
>>
>> and running the following:
>>
>> =A0 for i in * ; do echo -n $i": " ; cat $i ; done
>>
>> Running with the defaults, data like below was typical:
>>
>> hold_time: 1000
>> inflight: 4542
>> limit: 3456
>> limit_max: 1879048192
>> limit_min: 0
>>
>> hold_time: 1000
>> inflight: 4542
>> limit: 3378
>> limit_max: 1879048192
>> limit_min: 0
>>
>> i.e. 2 or 3 MTU sized packets in flight and the limit value lying
>> somewhere between those two values.
>>
>> The interesting thing is that the interactive speed reported by scp
>> seemed somewhat erratic, ranging from ~450 to ~700kB/s. (This was
>> the only traffic on the old junk - perhaps expected oscillations such
>> as those seen in isolated ARED tests?) =A0Average speed for 100M was:
>>
>> 104857600 bytes (105 MB) copied, 172.616 s, 607 kB/s
>>
>
> Still half duplex, or full duplex ?
>
> Limiting to one packet on half duplex might avoid collisions :)

Ah yes.  It was even in the text I'd had above!

  PHY: mdio@e0024520:19 - Link is Up - 10/Half

Now the slowdown makes sense to me.

Thanks for the review as well.

Paul.

>
>> Anyway, back to BQL testing; setting the values as follows:
>>
>> hold_time: 1000
>> inflight: 1514
>> limit: 1400
>> limit_max: 1400
>> limit_min: 1000
>>
>> had the effect of serializing the interface to a single packet, and
>> the crusty old hub seemed much happier with this arrangement, keeping
>> a constant speed and achieving the following on a 100MB Tx block:
>>
>> 104857600 bytes (105 MB) copied, 112.52 s, 932 kB/s
>>
>> It might be interesting to know more about why the defaults suffer
>> the slowdown, but the hub could possibly be ancient spec violating
>> trash. =A0Definitely something that nobody would ever use for anything
>> today. (aside from contrived tests like this)
>>
>> But it did give me an example of where I could see the effects of
>> changing the BQL settings, and I'm reasonably confident they are
>> working as expected.
>>
>
> Seems pretty good to me !
>
>
> --
> 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 =A0http://vger.kernel.org/majordomo-info.html

[PATCH v2 net-next 0/4] Gianfar byte queue limits

[Paul Gortmaker]

Identical to v1 but with the additional patch suggested by Eric.
Compile tested.  The v1 text follows below the pull request.

Sorry for the near back-to-back sends; I would have liked to have
got this out earlier in the week and not so close to net-next
closing, but that just didn't happen...

Thanks,
Paul
---

The following changes since commit cdf485be3a63d1f34293740fb726088c6840ceea:

  ixgbe: dcb: use DCB config values for FCoE traffic class on open (2012-03-14 00:49:10 -0700)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/paulg/linux.git gianfar-bql

for you to fetch changes up to 5407b14c6792d6ff122ecb1a2a6acffad60ef389:

  gianfar: use netif_tx_queue_stopped instead of __netif_subqueue_stopped (2012-03-18 17:11:22 -0400)

----------------------------------------------------------------
Paul Gortmaker (4):
      gianfar: Add support for byte queue limits.
      gianfar: constify giant block of status descriptor strings
      gianfar: delete orphaned version strings and dead macros
      gianfar: use netif_tx_queue_stopped instead of __netif_subqueue_stopped

 drivers/net/ethernet/freescale/gianfar.c         |   22 ++++++++++++++++------
 drivers/net/ethernet/freescale/gianfar.h         |    3 ---
 drivers/net/ethernet/freescale/gianfar_ethtool.c |    2 +-
 3 files changed, 17 insertions(+), 10 deletions(-)


> The BQL support here is unchanged from what I posted earlier as an
> RFC[1] -- with the exception of the fact that I'm now happier with
> the runtime testing vs. the simple "hey it boots" that I'd done
> for the RFC.  Plus I added a couple trivial cleanup patches.
> 
> For testing, I made a couple spiders homeless by reviving an ancient
> 10baseT hub.  I connected an sbc8349 into that, and connected the
> yellowing hub into a GigE 16port, which was also connected to the
> recipient x86 box.
> 
> Gianfar saw the interface as follows:
> 
> fsl-gianfar e0024000.ethernet: eth0: mac: 00:a0:1e:a0:26:5a
> fsl-gianfar e0024000.ethernet: eth0: Running with NAPI enabled
> fsl-gianfar e0024000.ethernet: eth0: RX BD ring size for Q[0]: 256
> fsl-gianfar e0024000.ethernet: eth0: TX BD ring size for Q[0]: 256
> PHY: mdio@e0024520:19 - Link is Up - 10/Half
> 
> With the sbc8349 being diskless, I simply used an scp of /proc/kcore
> to the connected x86 box as a rudimentary Tx heavy workload.
> 
> BQL data was collected by changing into the dir:
> 
>   /sys/devices/e0000000.soc8349/e0024000.ethernet/net/eth0/queues/tx-0/byte_queue_limits
> 
> and running the following:
> 
>   for i in * ; do echo -n $i": " ; cat $i ; done
> 
> Running with the defaults, data like below was typical:
> 
> hold_time: 1000
> inflight: 4542
> limit: 3456
> limit_max: 1879048192
> limit_min: 0
> 
> hold_time: 1000
> inflight: 4542
> limit: 3378
> limit_max: 1879048192
> limit_min: 0
> 
> i.e. 2 or 3 MTU sized packets in flight and the limit value lying
> somewhere between those two values.
> 
> The interesting thing is that the interactive speed reported by scp
> seemed somewhat erratic, ranging from ~450 to ~700kB/s. (This was
> the only traffic on the old junk - perhaps expected oscillations such
> as those seen in isolated ARED tests?)  Average speed for 100M was:
> 
> 104857600 bytes (105 MB) copied, 172.616 s, 607 kB/s
> 
> Anyway, back to BQL testing; setting the values as follows:
> 
> hold_time: 1000
> inflight: 1514
> limit: 1400
> limit_max: 1400
> limit_min: 1000
> 
> had the effect of serializing the interface to a single packet, and
> the crusty old hub seemed much happier with this arrangement, keeping
> a constant speed and achieving the following on a 100MB Tx block:
> 
> 104857600 bytes (105 MB) copied, 112.52 s, 932 kB/s
> 
> It might be interesting to know more about why the defaults suffer
> the slowdown, but the hub could possibly be ancient spec violating
> trash.  Definitely something that nobody would ever use for anything
> today. (aside from contrived tests like this)
> 
> But it did give me an example of where I could see the effects of
> changing the BQL settings, and I'm reasonably confident they are
> working as expected.
> 
> Paul.
> ---
> 
> [1] http://lists.openwall.net/netdev/2012/01/06/64
> 
> Paul Gortmaker (3):
>   gianfar: Add support for byte queue limits.
>   gianfar: constify giant block of status descriptor strings
>   gianfar: delete orphaned version strings and dead macros
> 
>  drivers/net/ethernet/freescale/gianfar.c         |   22 ++++++++++++++++------
>  drivers/net/ethernet/freescale/gianfar.h         |    3 ---
>  drivers/net/ethernet/freescale/gianfar_ethtool.c |    2 +-
>  3 files changed, 17 insertions(+), 10 deletions(-)
> 
-- 
1.7.9.1

[PATCH net-next 1/4] gianfar: Add support for byte queue limits.

[Paul Gortmaker]

Add support for byte queue limits (BQL), based on the similar
modifications made to intel/igb/igb_main.c from Eric Dumazet
in commit bdbc063129e811264cd6c311d8c2d9b95de01231
	"igb: Add support for byte queue limits."

A local variable for tx_queue->qindex was introduced in
gfar_clean_tx_ring, since it is now used often enough to warrant it,
and it cleans up the readability somewhat as well.

Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/net/ethernet/freescale/gianfar.c |   19 ++++++++++++++++---
 1 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index adb0ae4..a4c934b 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -1755,9 +1755,12 @@ static void free_skb_resources(struct gfar_private *priv)
 
 	/* Go through all the buffer descriptors and free their data buffers */
 	for (i = 0; i < priv->num_tx_queues; i++) {
+		struct netdev_queue *txq;
 		tx_queue = priv->tx_queue[i];
+		txq = netdev_get_tx_queue(tx_queue->dev, tx_queue->qindex);
 		if(tx_queue->tx_skbuff)
 			free_skb_tx_queue(tx_queue);
+		netdev_tx_reset_queue(txq);
 	}
 
 	for (i = 0; i < priv->num_rx_queues; i++) {
@@ -2217,6 +2220,8 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev)
 		lstatus |= BD_LFLAG(TXBD_CRC | TXBD_READY) | skb_headlen(skb);
 	}
 
+	netdev_tx_sent_queue(txq, skb->len);
+
 	/*
 	 * We can work in parallel with gfar_clean_tx_ring(), except
 	 * when modifying num_txbdfree. Note that we didn't grab the lock
@@ -2460,6 +2465,7 @@ static void gfar_align_skb(struct sk_buff *skb)
 static int gfar_clean_tx_ring(struct gfar_priv_tx_q *tx_queue)
 {
 	struct net_device *dev = tx_queue->dev;
+	struct netdev_queue *txq;
 	struct gfar_private *priv = netdev_priv(dev);
 	struct gfar_priv_rx_q *rx_queue = NULL;
 	struct txbd8 *bdp, *next = NULL;
@@ -2471,10 +2477,13 @@ static int gfar_clean_tx_ring(struct gfar_priv_tx_q *tx_queue)
 	int frags = 0, nr_txbds = 0;
 	int i;
 	int howmany = 0;
+	int tqi = tx_queue->qindex;
+	unsigned int bytes_sent = 0;
 	u32 lstatus;
 	size_t buflen;
 
-	rx_queue = priv->rx_queue[tx_queue->qindex];
+	rx_queue = priv->rx_queue[tqi];
+	txq = netdev_get_tx_queue(dev, tqi);
 	bdp = tx_queue->dirty_tx;
 	skb_dirtytx = tx_queue->skb_dirtytx;
 
@@ -2533,6 +2542,8 @@ static int gfar_clean_tx_ring(struct gfar_priv_tx_q *tx_queue)
 			bdp = next_txbd(bdp, base, tx_ring_size);
 		}
 
+		bytes_sent += skb->len;
+
 		/*
 		 * If there's room in the queue (limit it to rx_buffer_size)
 		 * we add this skb back into the pool, if it's the right size
@@ -2557,13 +2568,15 @@ static int gfar_clean_tx_ring(struct gfar_priv_tx_q *tx_queue)
 	}
 
 	/* If we freed a buffer, we can restart transmission, if necessary */
-	if (__netif_subqueue_stopped(dev, tx_queue->qindex) && tx_queue->num_txbdfree)
-		netif_wake_subqueue(dev, tx_queue->qindex);
+	if (__netif_subqueue_stopped(dev, tqi) && tx_queue->num_txbdfree)
+		netif_wake_subqueue(dev, tqi);
 
 	/* Update dirty indicators */
 	tx_queue->skb_dirtytx = skb_dirtytx;
 	tx_queue->dirty_tx = bdp;
 
+	netdev_tx_completed_queue(txq, howmany, bytes_sent);
+
 	return howmany;
 }
 
-- 
1.7.9.1

[PATCH net-next 2/4] gianfar: constify giant block of status descriptor strings

[Paul Gortmaker]

Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/net/ethernet/freescale/gianfar_ethtool.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar_ethtool.c b/drivers/net/ethernet/freescale/gianfar_ethtool.c
index 5a78d55..8d74efd 100644
--- a/drivers/net/ethernet/freescale/gianfar_ethtool.c
+++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c
@@ -58,7 +58,7 @@ static void gfar_gringparam(struct net_device *dev, struct ethtool_ringparam *rv
 static int gfar_sringparam(struct net_device *dev, struct ethtool_ringparam *rvals);
 static void gfar_gdrvinfo(struct net_device *dev, struct ethtool_drvinfo *drvinfo);
 
-static char stat_gstrings[][ETH_GSTRING_LEN] = {
+static const char stat_gstrings[][ETH_GSTRING_LEN] = {
 	"rx-dropped-by-kernel",
 	"rx-large-frame-errors",
 	"rx-short-frame-errors",
-- 
1.7.9.1

[PATCH net-next 3/4] gianfar: delete orphaned version strings and dead macros

[Paul Gortmaker]

There were two version strings, and neither one was being used.
Also in the same proximity were some unused #define that were
left over from the past.  Delete them all.

Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/net/ethernet/freescale/gianfar.c |    3 ---
 drivers/net/ethernet/freescale/gianfar.h |    3 ---
 2 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index a4c934b..6e66cc3 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -104,10 +104,7 @@
 #include "fsl_pq_mdio.h"
 
 #define TX_TIMEOUT      (1*HZ)
-#undef BRIEF_GFAR_ERRORS
-#undef VERBOSE_GFAR_ERRORS
 
-const char gfar_driver_name[] = "Gianfar Ethernet";
 const char gfar_driver_version[] = "1.3";
 
 static int gfar_enet_open(struct net_device *dev);
diff --git a/drivers/net/ethernet/freescale/gianfar.h b/drivers/net/ethernet/freescale/gianfar.h
index 4fe0f34..fc2488a 100644
--- a/drivers/net/ethernet/freescale/gianfar.h
+++ b/drivers/net/ethernet/freescale/gianfar.h
@@ -78,11 +78,8 @@ struct ethtool_rx_list {
 #define INCREMENTAL_BUFFER_SIZE 512
 
 #define PHY_INIT_TIMEOUT 100000
-#define GFAR_PHY_CHANGE_TIME 2
 
-#define DEVICE_NAME "%s: Gianfar Ethernet Controller Version 1.2, "
 #define DRV_NAME "gfar-enet"
-extern const char gfar_driver_name[];
 extern const char gfar_driver_version[];
 
 /* MAXIMUM NUMBER OF QUEUES SUPPORTED */
-- 
1.7.9.1

[PATCH net-next 4/4] gianfar: use netif_tx_queue_stopped instead of __netif_subqueue_stopped

[Paul Gortmaker]

The __netif_subqueue_stopped() just does the following:

        struct netdev_queue *txq = netdev_get_tx_queue(dev, queue_index);
        return netif_tx_queue_stopped(txq);

and since we already have the txq in scope, we can just call that
directly in this case.

Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/net/ethernet/freescale/gianfar.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index 6e66cc3..d9428f0 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -2565,7 +2565,7 @@ static int gfar_clean_tx_ring(struct gfar_priv_tx_q *tx_queue)
 	}
 
 	/* If we freed a buffer, we can restart transmission, if necessary */
-	if (__netif_subqueue_stopped(dev, tqi) && tx_queue->num_txbdfree)
+	if (netif_tx_queue_stopped(txq) && tx_queue->num_txbdfree)
 		netif_wake_subqueue(dev, tqi);
 
 	/* Update dirty indicators */
-- 
1.7.9.1

Re: [PATCH net-next 4/4] gianfar: use netif_tx_queue_stopped instead of __netif_subqueue_stopped

[Eric Dumazet]

On Sun, 2012-03-18 at 17:39 -0400, Paul Gortmaker wrote:
> The __netif_subqueue_stopped() just does the following:
> 
>         struct netdev_queue *txq = netdev_get_tx_queue(dev, queue_index);
>         return netif_tx_queue_stopped(txq);
> 
> and since we already have the txq in scope, we can just call that
> directly in this case.
> 
> Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> ---
>  drivers/net/ethernet/freescale/gianfar.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
> index 6e66cc3..d9428f0 100644
> --- a/drivers/net/ethernet/freescale/gianfar.c
> +++ b/drivers/net/ethernet/freescale/gianfar.c
> @@ -2565,7 +2565,7 @@ static int gfar_clean_tx_ring(struct gfar_priv_tx_q *tx_queue)
>  	}
>  
>  	/* If we freed a buffer, we can restart transmission, if necessary */
> -	if (__netif_subqueue_stopped(dev, tqi) && tx_queue->num_txbdfree)
> +	if (netif_tx_queue_stopped(txq) && tx_queue->num_txbdfree)
>  		netif_wake_subqueue(dev, tqi);
>  
>  	/* Update dirty indicators */

Please change netif_wake_subqueue() as well ;)

Re: [PATCH net-next 4/4] gianfar: use netif_tx_queue_stopped instead of __netif_subqueue_stopped

[Paul Gortmaker]

On Sun, Mar 18, 2012 at 5:55 PM, Eric Dumazet <eric.dumazet@gmail.com> wrot=
e:
> On Sun, 2012-03-18 at 17:39 -0400, Paul Gortmaker wrote:
>> The __netif_subqueue_stopped() just does the following:
>>
>> =A0 =A0 =A0 =A0 struct netdev_queue *txq =3D netdev_get_tx_queue(dev, qu=
eue_index);
>> =A0 =A0 =A0 =A0 return netif_tx_queue_stopped(txq);
>>
>> and since we already have the txq in scope, we can just call that
>> directly in this case.
>>
>> Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
>> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
>> ---
>> =A0drivers/net/ethernet/freescale/gianfar.c | =A0 =A02 +-
>> =A01 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethe=
rnet/freescale/gianfar.c
>> index 6e66cc3..d9428f0 100644
>> --- a/drivers/net/ethernet/freescale/gianfar.c
>> +++ b/drivers/net/ethernet/freescale/gianfar.c
>> @@ -2565,7 +2565,7 @@ static int gfar_clean_tx_ring(struct gfar_priv_tx_=
q *tx_queue)
>> =A0 =A0 =A0 }
>>
>> =A0 =A0 =A0 /* If we freed a buffer, we can restart transmission, if nec=
essary */
>> - =A0 =A0 if (__netif_subqueue_stopped(dev, tqi) && tx_queue->num_txbdfr=
ee)
>> + =A0 =A0 if (netif_tx_queue_stopped(txq) && tx_queue->num_txbdfree)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 netif_wake_subqueue(dev, tqi);
>>
>> =A0 =A0 =A0 /* Update dirty indicators */
>
> Please change netif_wake_subqueue() as well ;)

I looked at this earlier when I added patch #4 but I was concerned about
the different semantics.

The netif_wake_subqueue() just returns on a netpoll_trap but the other
netif_tx_wake_queue() actually calls netif_tx_start_queue() for the same
netpoll_trap instance.  Maybe that is OK, but I didn't want to be changing
the behaviour of subtleties in stuff where I am clearly still learning.

Thanks,
Paul.

>
> --
> 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 =A0http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next 4/4] gianfar: use netif_tx_queue_stopped instead of __netif_subqueue_stopped

[Eric Dumazet]

Le dimanche 18 mars 2012 à 19:24 -0400, Paul Gortmaker a écrit :
> On Sun, Mar 18, 2012 at 5:55 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > On Sun, 2012-03-18 at 17:39 -0400, Paul Gortmaker wrote:
> >> The __netif_subqueue_stopped() just does the following:
> >>
> >>         struct netdev_queue *txq = netdev_get_tx_queue(dev, queue_index);
> >>         return netif_tx_queue_stopped(txq);
> >>
> >> and since we already have the txq in scope, we can just call that
> >> directly in this case.
> >>
> >> Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
> >> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> >> ---
> >>  drivers/net/ethernet/freescale/gianfar.c |    2 +-
> >>  1 files changed, 1 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
> >> index 6e66cc3..d9428f0 100644
> >> --- a/drivers/net/ethernet/freescale/gianfar.c
> >> +++ b/drivers/net/ethernet/freescale/gianfar.c
> >> @@ -2565,7 +2565,7 @@ static int gfar_clean_tx_ring(struct gfar_priv_tx_q *tx_queue)
> >>       }
> >>
> >>       /* If we freed a buffer, we can restart transmission, if necessary */
> >> -     if (__netif_subqueue_stopped(dev, tqi) && tx_queue->num_txbdfree)
> >> +     if (netif_tx_queue_stopped(txq) && tx_queue->num_txbdfree)
> >>               netif_wake_subqueue(dev, tqi);
> >>
> >>       /* Update dirty indicators */
> >
> > Please change netif_wake_subqueue() as well ;)
> 
> I looked at this earlier when I added patch #4 but I was concerned about
> the different semantics.
> 
> The netif_wake_subqueue() just returns on a netpoll_trap but the other
> netif_tx_wake_queue() actually calls netif_tx_start_queue() for the same
> netpoll_trap instance.  Maybe that is OK, but I didn't want to be changing
> the behaviour of subtleties in stuff where I am clearly still learning.
> 

I see... commit 7b3d3e4fc68 added a small difference here...

Re: [PATCH v2 net-next 0/4] Gianfar byte queue limits

[David Miller]

From: Paul Gortmaker <paul.gortmaker@windriver.com>
Date: Sun, 18 Mar 2012 17:39:17 -0400

> The following changes since commit cdf485be3a63d1f34293740fb726088c6840ceea:
> 
>   ixgbe: dcb: use DCB config values for FCoE traffic class on open (2012-03-14 00:49:10 -0700)
> 
> are available in the git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/paulg/linux.git gianfar-bql
> 
> for you to fetch changes up to 5407b14c6792d6ff122ecb1a2a6acffad60ef389:
> 
>   gianfar: use netif_tx_queue_stopped instead of __netif_subqueue_stopped (2012-03-18 17:11:22 -0400)

Pulled, thanks.