Patchwork [2/2] ucc_geth: Rework the TX logic.

login
register
mail settings
Submitter Joakim Tjernlund
Date March 26, 2009, 12:54 p.m.
Message ID <1238072077-27044-2-git-send-email-Joakim.Tjernlund@transmode.se>
Download mbox | patch
Permalink /patch/25146/
State Superseded
Delegated to: David Miller
Headers show

Comments

Joakim Tjernlund - March 26, 2009, 12:54 p.m.
The line:
 if ((bd == ugeth->txBd[txQ]) && (netif_queue_stopped(dev) == 0))
       break;
in ucc_geth_tx() didn not make sense to me. Rework & cleanup
this logic to something understandable.

Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
---
 drivers/net/ucc_geth.c |   40 ++++++++++++++++++++--------------------
 1 files changed, 20 insertions(+), 20 deletions(-)
Anton Vorontsov - March 26, 2009, 1:39 p.m.
Hi Joakim,

On Thu, Mar 26, 2009 at 01:54:37PM +0100, Joakim Tjernlund wrote:
> The line:
>  if ((bd == ugeth->txBd[txQ]) && (netif_queue_stopped(dev) == 0))
>        break;
> in ucc_geth_tx() didn not make sense to me. Rework & cleanup
> this logic to something understandable.
> 
> Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> ---
>  drivers/net/ucc_geth.c |   40 ++++++++++++++++++++--------------------
>  1 files changed, 20 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/ucc_geth.c b/drivers/net/ucc_geth.c
> index 7fc91aa..b6ebefd 100644
> --- a/drivers/net/ucc_geth.c
> +++ b/drivers/net/ucc_geth.c
> @@ -3048,6 +3048,7 @@ static int ucc_geth_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  	u8 __iomem *bd;			/* BD pointer */
>  	u32 bd_status;
>  	u8 txQ = 0;
> +	int txInd;

camelCase should not be used in Linux.

Surely, the driver is full of camelCases... though, it should be
fixed, not encouraged further.

And btw, there is even Hungarian notation in the driver. :-(

[...]
>  	/* If the next BD still needs to be cleaned up, then the bds
>  	   are full.  We need to tell the kernel to stop sending us stuff. */
> -	if (bd == ugeth->confBd[txQ]) {
> -		if (!netif_queue_stopped(dev))
> -			netif_stop_queue(dev);
> +	if (!in_be32((u32 __iomem *)(bd+4))) {

bd == ugeth->confBd[txQ]
and
!in_be32((u32 __iomem *)(bd+4))

Are not equivalent wrt. speed. MMIO accessors should be rather
slow comparing to normal memory.

We should really do some performance tests (using gbit links).
I'll try to help you with the tests, but it might take some time.

[...]
> +		if (!in_be32((u32 __iomem *)(bd+4)))
[...]
> +		out_be32((u32 __iomem *)(bd+4), (int)NULL); /* Mark it free */

How about some inline function that will self-document the bd + 4
stuff? Plus that way we'll get rid of the casts.

Note that "bd+4" should be "bd + 4". And (int)NULL makes
little sense, just 0 will work.

Thanks,
Joakim Tjernlund - March 26, 2009, 4:43 p.m.
Anton Vorontsov <avorontsov@ru.mvista.com> wrote on 26/03/2009 14:39:18:
> 
> Hi Joakim,

Hi Anton

> 
> On Thu, Mar 26, 2009 at 01:54:37PM +0100, Joakim Tjernlund wrote:
> > The line:
> >  if ((bd == ugeth->txBd[txQ]) && (netif_queue_stopped(dev) == 0))
> >        break;
> > in ucc_geth_tx() didn not make sense to me. Rework & cleanup
> > this logic to something understandable.
> > 
> > Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> > ---
> >  drivers/net/ucc_geth.c |   40 
++++++++++++++++++++--------------------
> >  1 files changed, 20 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/net/ucc_geth.c b/drivers/net/ucc_geth.c
> > index 7fc91aa..b6ebefd 100644
> > --- a/drivers/net/ucc_geth.c
> > +++ b/drivers/net/ucc_geth.c
> > @@ -3048,6 +3048,7 @@ static int ucc_geth_start_xmit(struct sk_buff 
*skb, struct net_device *dev)
> >     u8 __iomem *bd;         /* BD pointer */
> >     u32 bd_status;
> >     u8 txQ = 0;
> > +   int txInd;
> 
> camelCase should not be used in Linux.
> 
> Surely, the driver is full of camelCases... though, it should be
> fixed, not encouraged further.

OK, I will rename to tx_ind instead.

> 
> And btw, there is even Hungarian notation in the driver. :-(

Hopefully that will go away in time.

> 
> [...]
> >     /* If the next BD still needs to be cleaned up, then the bds
> >        are full.  We need to tell the kernel to stop sending us stuff. 
*/
> > -   if (bd == ugeth->confBd[txQ]) {
> > -      if (!netif_queue_stopped(dev))
> > -         netif_stop_queue(dev);
> > +   if (!in_be32((u32 __iomem *)(bd+4))) {
> 
> bd == ugeth->confBd[txQ]
> and
> !in_be32((u32 __iomem *)(bd+4))
> 
> Are not equivalent wrt. speed. MMIO accessors should be rather
> slow comparing to normal memory.

Yes, I know. I did it this way because I something broke under stress
when ugeth->confBd[txQ] instead. The ucc_geth_tx() and 
ucc_geth_start_xmit()
gets out of sync somehow.

> 
> We should really do some performance tests (using gbit links).
> I'll try to help you with the tests, but it might take some time.

Good, because I don't have GBE links :(

> 
> [...]
> > +      if (!in_be32((u32 __iomem *)(bd+4)))
> [...]
> > +      out_be32((u32 __iomem *)(bd+4), (int)NULL); /* Mark it free */
> 
> How about some inline function that will self-document the bd + 4
> stuff? Plus that way we'll get rid of the casts.

Good idea, will look at that.

> 
> Note that "bd+4" should be "bd + 4". And (int)NULL makes
> little sense, just 0 will work.

Sure, done.

--
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
Anton Vorontsov - March 26, 2009, 6:05 p.m.
On Thu, Mar 26, 2009 at 05:43:25PM +0100, Joakim Tjernlund wrote:
[...]
> > bd == ugeth->confBd[txQ]
> > and
> > !in_be32((u32 __iomem *)(bd+4))
> > 
> > Are not equivalent wrt. speed. MMIO accessors should be rather
> > slow comparing to normal memory.
> 
> Yes, I know. I did it this way because I something broke under stress
> when ugeth->confBd[txQ] instead. The ucc_geth_tx() and 
> ucc_geth_start_xmit()
> gets out of sync somehow.

Would be great if you could investigate it more. Maybe there is
a serious bug somewhere, or maybe you're introducing another
(yet hidden) bug...
Joakim Tjernlund - March 26, 2009, 6:20 p.m.
Anton Vorontsov <avorontsov@ru.mvista.com> wrote on 26/03/2009 19:05:43:

> On Thu, Mar 26, 2009 at 05:43:25PM +0100, Joakim Tjernlund wrote:
> [...]
> > > bd == ugeth->confBd[txQ]
> > > and
> > > !in_be32((u32 __iomem *)(bd+4))
> > > 
> > > Are not equivalent wrt. speed. MMIO accessors should be rather
> > > slow comparing to normal memory.
> > 
> > Yes, I know. I did it this way because I something broke under stress
> > when ugeth->confBd[txQ] instead. The ucc_geth_tx() and 
> > ucc_geth_start_xmit()
> > gets out of sync somehow.
> 
> Would be great if you could investigate it more. Maybe there is
> a serious bug somewhere, or maybe you're introducing another
> (yet hidden) bug...

I have spent some time on it and didn't see how to do it. I don't think
the current method hides any bugs. I have used this method on 8xx for
many years and it works well.

Actually the current method might work without the 
spin_lock_irq(&ugeth->lock)/spin_unlock_irq(&ugeth->lock)
in ucc_geth_start_xmit(). I can't break it, can you?

 Jocke
--
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
David Miller - March 27, 2009, 7:42 a.m.
These patches don't apply to the current tree, could you please
respin your final version against at the very least Linus's tree?

Thanks.
--
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
Joakim Tjernlund - March 27, 2009, 9:37 a.m.
David Miller <davem@davemloft.net> wrote on 27/03/2009 08:42:37:
> 
> 
> These patches don't apply to the current tree, could you please
> respin your final version against at the very least Linus's tree?

That is strange, had a look at linus tree and I don't see any
changes in his tree that touches the same area/functions.

If I get the time I will look at it a bit more but for now I am
stuck on 2.6.29

 Jocke
--
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

Patch

diff --git a/drivers/net/ucc_geth.c b/drivers/net/ucc_geth.c
index 7fc91aa..b6ebefd 100644
--- a/drivers/net/ucc_geth.c
+++ b/drivers/net/ucc_geth.c
@@ -3048,6 +3048,7 @@  static int ucc_geth_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	u8 __iomem *bd;			/* BD pointer */
 	u32 bd_status;
 	u8 txQ = 0;
+	int txInd;
 
 	ugeth_vdbg("%s: IN", __func__);
 
@@ -3059,12 +3060,12 @@  static int ucc_geth_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	bd = ugeth->txBd[txQ];
 	bd_status = in_be32((u32 __iomem *)bd);
 	/* Save the skb pointer so we can free it later */
-	ugeth->tx_skbuff[txQ][ugeth->skb_curtx[txQ]] = skb;
+	txInd = ugeth->skb_curtx[txQ];
+	ugeth->tx_skbuff[txQ][txInd] = skb;
 
 	/* Update the current skb pointer (wrapping if this was the last) */
-	ugeth->skb_curtx[txQ] =
-	    (ugeth->skb_curtx[txQ] +
-	     1) & TX_RING_MOD_MASK(ugeth->ug_info->bdRingLenTx[txQ]);
+	txInd = (txInd + 1) & TX_RING_MOD_MASK(ugeth->ug_info->bdRingLenTx[txQ]);
+	ugeth->skb_curtx[txQ] = txInd;
 
 	/* set up the buffer descriptor */
 	out_be32(&((struct qe_bd __iomem *)bd)->buf,
@@ -3088,9 +3089,8 @@  static int ucc_geth_start_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	/* If the next BD still needs to be cleaned up, then the bds
 	   are full.  We need to tell the kernel to stop sending us stuff. */
-	if (bd == ugeth->confBd[txQ]) {
-		if (!netif_queue_stopped(dev))
-			netif_stop_queue(dev);
+	if (!in_be32((u32 __iomem *)(bd+4))) {
+		netif_stop_queue(dev);
 	}
 
 	ugeth->txBd[txQ] = bd;
@@ -3198,32 +3198,29 @@  static int ucc_geth_tx(struct net_device *dev, u8 txQ)
 	struct ucc_geth_private *ugeth = netdev_priv(dev);
 	u8 __iomem *bd;		/* BD pointer */
 	u32 bd_status;
+	int txInd, num_freed;
 
 	bd = ugeth->confBd[txQ];
 	bd_status = in_be32((u32 __iomem *)bd);
-
+	txInd = ugeth->skb_dirtytx[txQ];
+	num_freed = 0;
 	/* Normal processing. */
 	while ((bd_status & T_R) == 0) {
 		/* BD contains already transmitted buffer.   */
 		/* Handle the transmitted buffer and release */
 		/* the BD to be used with the current frame  */
 
-		if ((bd == ugeth->txBd[txQ]) && (netif_queue_stopped(dev) == 0))
-			break;
+		if (!in_be32((u32 __iomem *)(bd+4)))
+			break; /* Queue is empty */
 
 		dev->stats.tx_packets++;
 
 		/* Free the sk buffer associated with this TxBD */
-		dev_kfree_skb(ugeth->
-				  tx_skbuff[txQ][ugeth->skb_dirtytx[txQ]]);
-		ugeth->tx_skbuff[txQ][ugeth->skb_dirtytx[txQ]] = NULL;
-		ugeth->skb_dirtytx[txQ] =
-		    (ugeth->skb_dirtytx[txQ] +
-		     1) & TX_RING_MOD_MASK(ugeth->ug_info->bdRingLenTx[txQ]);
-
-		/* We freed a buffer, so now we can restart transmission */
-		if (netif_queue_stopped(dev))
-			netif_wake_queue(dev);
+		dev_kfree_skb(ugeth->tx_skbuff[txQ][txInd]);
+		ugeth->tx_skbuff[txQ][txInd] = NULL;
+		out_be32((u32 __iomem *)(bd+4), (int)NULL); /* Mark it free */
+		num_freed++;
+		txInd = (txInd + 1) & TX_RING_MOD_MASK(ugeth->ug_info->bdRingLenTx[txQ]);
 
 		/* Advance the confirmation BD pointer */
 		if (!(bd_status & T_W))
@@ -3233,6 +3230,9 @@  static int ucc_geth_tx(struct net_device *dev, u8 txQ)
 		bd_status = in_be32((u32 __iomem *)bd);
 	}
 	ugeth->confBd[txQ] = bd;
+	ugeth->skb_dirtytx[txQ] = txInd;
+	if (num_freed)
+		netif_wake_queue(dev); /* We freed some buffers, so restart transmission */
 	return 0;
 }