RE: [PATCH v3 1/6] net: fec: Factorize the .xmit transmit function

From: fugang.duan@freescale.

quoted

From: Fugang Duan

quoted

Make the code more readable and easy to support other features like
SG, TSO, moving the common transmit function to one api.

And the patch also factorize the getting BD index to it own function.

Signed-off-by: Fugang Duan <redacted>
---
 drivers/net/ethernet/freescale/fec_main.c |   87 ++++++++++++++++++-----

------

quoted

 1 files changed, 54 insertions(+), 33 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c

b/drivers/net/ethernet/freescale/fec_main.c
index 802be17..32c2276 100644

--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c

@@ -287,6 +287,25 @@ struct bufdesc *fec_enet_get_prevdesc(struct bufdesc

*bdp, struct fec_enet_priva

quoted

 		return (new_bd < base) ? (new_bd + ring_size) : new_bd;  }

+static inline
+int fec_enet_get_bd_index(struct bufdesc *bdp, struct
+fec_enet_private *fep) {
+	struct bufdesc *base;
+	int index;
+
+	if (bdp >= fep->tx_bd_base)
+		base = fep->tx_bd_base;
+	else
+		base = fep->rx_bd_base;

You really don't want the above conditional.
It is known from the call site - but the compiler can't determine that and
remove the test.

Can you give me more training for this ?  Sorry, I don't understand the means.

Single 'if' statements can have measurable performance impact on ethernet
code paths - so it isn't a good idea to add ones that aren't strictly required.

If the function had an extra argument 'tx_ring' (don't add one) which
would be constant at all the call sites and the code read:
	base = tx_ring ? fep->tx_bd_base : fep->rx_bd_base;
when the compiler inlines the function calls it would be able to
optimise away the conditional.

quoted

You may not want to rely on the tx and rx descriptors being allocated as a
single entity - particularly now that they (probably) consume more than a
single page.

BDs consumes more than single page, but it use " dma_alloc_coherent()" to allocate continuous memory,
I don't know why it is cannot do it like this.

quoted

quoted

+
+	if (fep->bufdesc_ex)
+		index = (struct bufdesc_ex *)bdp - (struct bufdesc_ex *)base;
+	else
+		index = bdp - base;

Save the sizeof the descriptor structure as (say) fep->bufdesc_size
(Possibly replacing bufdesc_ex) and then just calculate:
	index = ((const char *)bdp - (const char *)base)/fep->bufdesc_size;

	David

You means:
if (fep->bufdesc_ex)
	bufdesc_size = sizeof(struct bufdesc_ex);
else
	bufdesc_size = sizeof(struct bufdesc);
index = ((const char *)bdp - (const char *)base)/bufdesc_size;

No, that is still a conditional in the normal path.
Assign fep->bufdesc_size during initialisation.

Then end up with something like:
static
int fec_enet_get_bd_index(struct bufdesc *base, struct bufdesc *bdp,
		struct fec_enet_private *fep)
{
	return ((const char *)bdp - (const char *)base)/fep->bufdesc_size;
}

	David