RE: [PATCH v4 5/6] net: fec: Add Scatter/gather support

From: David Laight <redacted> Data: Tuesday, June 10, 2014 8:03 PM

To: Duan Fugang-B38611; davem@davemloft.net
Cc: netdev@vger.kernel.org; ezequiel.garcia@free-electrons.com; Li Frank-
B20596; eric.dumazet@gmail.com
Subject: RE: [PATCH v4 5/6] net: fec: Add Scatter/gather support

From: fugang.duan@freescale.com

quoted

From: David Laight <redacted> Sent: Tuesday, June 10,
2014 7:11 PM

quoted

To: Duan Fugang-B38611; davem@davemloft.net
Cc: netdev@vger.kernel.org; ezequiel.garcia@free-electrons.com; Li
Frank- B20596; eric.dumazet@gmail.com
Subject: RE: [PATCH v4 5/6] net: fec: Add Scatter/gather support

From: Fugang Duan

quoted

Add Scatter/gather support for FEC.
This feature allows to improve outbound throughput performance.

...

quoted

+static int fec_enet_get_free_txdesc_num(struct fec_enet_private
+*fep) {
+	int entries;
+
+	entries = ((const char *)fep->dirty_tx -
+			(const char *)fep->cur_tx) / fep->bufdesc_size;
+
+	return (fep->cur_tx >= fep->dirty_tx) ?
+		entries += fep->tx_ring_size : entries; }

...

quoted

quoted

Actually do you ever use the last entry?
If you do then the full and empty conditions are not separated.

quoted

Don't use the last entry. When it is full condition, cur_tx is the last

entry.

quoted

If (fep->cur_tx == fep->dirty_tx)
	Empty
If (fep->cur_tx == fep->dirty_tx - 1)
	Full

Since the patch set introduces tx_stop_threshold and
tx_wake_threshold, it never uses the rest of entry while Free entry
number is less than tx_stop_threshold.

In which case I'd expect fec_enet_get_free_txdesc_num() to return one less
than it does currently.
Maybe the check for 'enough space' is 'needed <
fec_enet_get_free_txdesc_num()'
(rather than <=)

David

Return space available, 0..size-1.  We always leave one free entry. Which is same as linux circ_buf.
static int fec_enet_get_free_txdesc_num(struct fec_enet_private *fep)
{
	int entries;

	entries = ((const char *)fep->dirty_tx - (const char *)fep->cur_tx) / fep->bufdesc_size - 1;

	return entries > 0 ? entries : entries + fep->tx_ring_size;
}

For SG,
entries_free = fec_enet_get_free_txdesc_num(fep);
=> stop queue:
if (entries_free < MAX_SKB_FRAGS + 1) 
     netif_stop_queue(ndev);
=>wake queue:
if (entries >= MAX_SKB_FRAGS + 1)
	netif_wake_queue(ndev);

Thanks,
Andy