[PATCH] ucc_geth: Rework the TX logic.

[PATCH] ucc_geth: Rework the TX logic.

[Joakim Tjernlund]

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

Reworked the patch according to Antons comments.

 drivers/net/ucc_geth.c |   66 +++++++++++++++++++++++++++--------------------
 1 files changed, 38 insertions(+), 28 deletions(-)

diff --git a/drivers/net/ucc_geth.c b/drivers/net/ucc_geth.c
index 7fc91aa..465de3a 100644
--- a/drivers/net/ucc_geth.c
+++ b/drivers/net/ucc_geth.c
@@ -161,6 +161,17 @@ static struct ucc_geth_info ugeth_primary_info = {
 
 static struct ucc_geth_info ugeth_info[8];
 
+
+static inline u32 __iomem *bd2buf(u8 __iomem *bd)
+{
+	return (u32 __iomem *)(bd+4);
+}
+
+static inline u32 __iomem *bd2status(u8 __iomem *bd)
+{
+	return (u32 __iomem *)bd;
+}
+
 #ifdef DEBUG
 static void mem_disp(u8 *addr, int size)
 {
@@ -3048,6 +3059,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 tx_ind;
 
 	ugeth_vdbg("%s: IN", __func__);
 
@@ -3057,21 +3069,19 @@ static int ucc_geth_start_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	/* Start from the next BD that should be filled */
 	bd = ugeth->txBd[txQ];
-	bd_status = in_be32((u32 __iomem *)bd);
+	bd_status = in_be32(bd2status(bd));
 	/* Save the skb pointer so we can free it later */
-	ugeth->tx_skbuff[txQ][ugeth->skb_curtx[txQ]] = skb;
+	tx_ind = ugeth->skb_curtx[txQ];
+	ugeth->tx_skbuff[txQ][tx_ind] = 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]);
+	tx_ind = (tx_ind + 1) & TX_RING_MOD_MASK(ugeth->ug_info->bdRingLenTx[txQ]);
+	ugeth->skb_curtx[txQ] = tx_ind;
 
 	/* set up the buffer descriptor */
-	out_be32(&((struct qe_bd __iomem *)bd)->buf,
-		      dma_map_single(&ugeth->dev->dev, skb->data,
-			      skb->len, DMA_TO_DEVICE));
-
-	/* printk(KERN_DEBUG"skb->data is 0x%x\n",skb->data); */
+	out_be32(bd2buf(bd),
+		 dma_map_single(&ugeth->dev->dev, skb->data,
+				skb->len, DMA_TO_DEVICE));
 
 	bd_status = (bd_status & T_W) | T_R | T_I | T_L | skb->len;
 
@@ -3088,9 +3098,9 @@ 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(bd2buf(bd))) {
+		netif_stop_queue(dev);
 	}
 
 	ugeth->txBd[txQ] = bd;
@@ -3198,41 +3208,41 @@ 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 tx_ind, num_freed;
 
 	bd = ugeth->confBd[txQ];
-	bd_status = in_be32((u32 __iomem *)bd);
-
+	bd_status = in_be32(bd2status(bd));
+	tx_ind = 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 (bd == ugeth->txBd[txQ])
+			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][tx_ind]);
+		ugeth->tx_skbuff[txQ][tx_ind] = NULL;
+		out_be32(bd2buf(bd), 0); /* Mark it free */
+		num_freed++;
+		tx_ind = (tx_ind + 1) & TX_RING_MOD_MASK(ugeth->ug_info->bdRingLenTx[txQ]);
 
 		/* Advance the confirmation BD pointer */
 		if (!(bd_status & T_W))
 			bd += sizeof(struct qe_bd);
 		else
 			bd = ugeth->p_tx_bd_ring[txQ];
-		bd_status = in_be32((u32 __iomem *)bd);
+		bd_status = in_be32(bd2status(bd));
 	}
 	ugeth->confBd[txQ] = bd;
+	ugeth->skb_dirtytx[txQ] = tx_ind;
+	if (num_freed)
+		netif_wake_queue(dev); /* We freed some buffers, so restart transmission */
 	return 0;
 }
 
-- 
1.6.1.3

Re: [PATCH] ucc_geth: Rework the TX logic.

[Anton Vorontsov]

On Thu, Mar 26, 2009 at 06:44:05PM +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.
> ---
> 
> Reworked the patch according to Antons comments.
> 
>  drivers/net/ucc_geth.c |   66 +++++++++++++++++++++++++++--------------------
>  1 files changed, 38 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/net/ucc_geth.c b/drivers/net/ucc_geth.c
> index 7fc91aa..465de3a 100644
> --- a/drivers/net/ucc_geth.c
> +++ b/drivers/net/ucc_geth.c
> @@ -161,6 +161,17 @@ static struct ucc_geth_info ugeth_primary_info = {
>  
>  static struct ucc_geth_info ugeth_info[8];
>  
> +
> +static inline u32 __iomem *bd2buf(u8 __iomem *bd)
> +{
> +	return (u32 __iomem *)(bd+4);

Spaces around "+"...

Also, now it's obvious that we're just reading status or buf.

So instead of these inlines we could use struct qe_bd as described
in asm/qe.h.

I.e.

struct qe_bd *bd = ...;

in_be32(&bd->buf);
in_be16(&bd->status);

Oh, wait. We can't, ucc_geth assumes status is u32, which includes
the length field, i.e. ucc_fast.h defines:

#define T_W     0x20000000

:-(

The cleanup work surely desires a separate patch, so bd2buf and
bd2status are OK, for now.

I'll test the patch as soon as I'll get some QE board back on
my table (actually I have one QE board handy, but it's a 1GHz
one, while I'd like to test the patch on a slow machine, where
we'll actually see performance regressions).

[...]
> +               tx_ind = (tx_ind + 1) & TX_RING_MOD_MASK(ugeth->ug_info->bdRingLenTx[txQ]);

Line over 80 columns.

[...]
> +       if (num_freed)
> +               netif_wake_queue(dev); /* We freed some buffers, so restart transmission */

Ditto.

Please make sure your patches pass scripts/checkpatch.pl.

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH] ucc_geth: Rework the TX logic.

[Joakim Tjernlund]

Anton Vorontsov <avorontsov@ru.mvista.com> wrote on 26/03/2009 19:03:54:
> 
> On Thu, Mar 26, 2009 at 06:44:05PM +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.
> > ---
> > 
> > Reworked the patch according to Antons comments.
> > 
> >  drivers/net/ucc_geth.c |   66 
+++++++++++++++++++++++++++--------------------
> >  1 files changed, 38 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/net/ucc_geth.c b/drivers/net/ucc_geth.c
> > index 7fc91aa..465de3a 100644
> > --- a/drivers/net/ucc_geth.c
> > +++ b/drivers/net/ucc_geth.c
> > @@ -161,6 +161,17 @@ static struct ucc_geth_info ugeth_primary_info = 
{
> > 
> >  static struct ucc_geth_info ugeth_info[8];
> > 
> > +
> > +static inline u32 __iomem *bd2buf(u8 __iomem *bd)
> > +{
> > +   return (u32 __iomem *)(bd+4);
> 
> Spaces around "+"...

Bugger, I forgot when I moved it.

> 
> Also, now it's obvious that we're just reading status or buf.
> 
> So instead of these inlines we could use struct qe_bd as described
> in asm/qe.h.
> 
> I.e.
> 
> struct qe_bd *bd = ...;
> 
> in_be32(&bd->buf);
> in_be16(&bd->status);
> 
> Oh, wait. We can't, ucc_geth assumes status is u32, which includes
> the length field, i.e. ucc_fast.h defines:
> 
> #define T_W     0x20000000
> 
> :-(
> 
> The cleanup work surely desires a separate patch, so bd2buf and
> bd2status are OK, for now.

Exactly, I did look at using struct qe_bd *bd, but that will
affect lots of stuff. Reading the status and len in one go is
also quite handy so I not sure it is a good idea to convert.

> 
> I'll test the patch as soon as I'll get some QE board back on
> my table (actually I have one QE board handy, but it's a 1GHz
> one, while I'd like to test the patch on a slow machine, where
> we'll actually see performance regressions).

Good, I have more coming but it touches the same code so I really want
to sort out this one first.

 Jocke

Re: [PATCH] ucc_geth: Rework the TX logic.

[Li Yang]

On Fri, Mar 27, 2009 at 1:44 AM, Joakim Tjernlund
<Joakim.Tjernlund@transmode.se> 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.
> ---
>
> Reworked the patch according to Antons comments.
>
>  drivers/net/ucc_geth.c |   66 +++++++++++++++++++++++++++--------------------
>  1 files changed, 38 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/net/ucc_geth.c b/drivers/net/ucc_geth.c
> index 7fc91aa..465de3a 100644
> --- a/drivers/net/ucc_geth.c
> +++ b/drivers/net/ucc_geth.c
> @@ -161,6 +161,17 @@ static struct ucc_geth_info ugeth_primary_info = {
>
>  static struct ucc_geth_info ugeth_info[8];
>
> +
> +static inline u32 __iomem *bd2buf(u8 __iomem *bd)
> +{
> +       return (u32 __iomem *)(bd+4);
> +}
> +
> +static inline u32 __iomem *bd2status(u8 __iomem *bd)
> +{
> +       return (u32 __iomem *)bd;
> +}


When the driver was reviewed for upstream merge, I was told to remove
this kind of thing in favor of direct struct qe_bd access.  So do we
really have a guideline on this?  Or it's just a matter of personal
preference?

> +
>  #ifdef DEBUG
>  static void mem_disp(u8 *addr, int size)
>  {
> @@ -3048,6 +3059,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 tx_ind;
>
>        ugeth_vdbg("%s: IN", __func__);
>
> @@ -3057,21 +3069,19 @@ static int ucc_geth_start_xmit(struct sk_buff *skb, struct net_device *dev)
>
>        /* Start from the next BD that should be filled */
>        bd = ugeth->txBd[txQ];
> -       bd_status = in_be32((u32 __iomem *)bd);
> +       bd_status = in_be32(bd2status(bd));
>        /* Save the skb pointer so we can free it later */
> -       ugeth->tx_skbuff[txQ][ugeth->skb_curtx[txQ]] = skb;
> +       tx_ind = ugeth->skb_curtx[txQ];
> +       ugeth->tx_skbuff[txQ][tx_ind] = 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]);
> +       tx_ind = (tx_ind + 1) & TX_RING_MOD_MASK(ugeth->ug_info->bdRingLenTx[txQ]);
> +       ugeth->skb_curtx[txQ] = tx_ind;
>
>        /* set up the buffer descriptor */
> -       out_be32(&((struct qe_bd __iomem *)bd)->buf,
> -                     dma_map_single(&ugeth->dev->dev, skb->data,
> -                             skb->len, DMA_TO_DEVICE));
> -
> -       /* printk(KERN_DEBUG"skb->data is 0x%x\n",skb->data); */
> +       out_be32(bd2buf(bd),
> +                dma_map_single(&ugeth->dev->dev, skb->data,
> +                               skb->len, DMA_TO_DEVICE));
>
>        bd_status = (bd_status & T_W) | T_R | T_I | T_L | skb->len;
>
> @@ -3088,9 +3098,9 @@ 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(bd2buf(bd))) {
> +               netif_stop_queue(dev);

Why does this make more sense?  The BD with a NULL buffer means the
ring is full?  It's not the normal case.

>        }
>
>        ugeth->txBd[txQ] = bd;
> @@ -3198,41 +3208,41 @@ 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 tx_ind, num_freed;
>
>        bd = ugeth->confBd[txQ];
> -       bd_status = in_be32((u32 __iomem *)bd);
> -
> +       bd_status = in_be32(bd2status(bd));
> +       tx_ind = 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 (bd == ugeth->txBd[txQ])
> +                       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][tx_ind]);
> +               ugeth->tx_skbuff[txQ][tx_ind] = NULL;
> +               out_be32(bd2buf(bd), 0); /* Mark it free */

Now I see why you depend on the buffer to judge if the BD ring is
full.  If you want to do this, make sure it's well documented in code,
or others will be confused.  And as Anton already commented, in/out
access is slow.  I think the original way can be fixed if you think
it's broken, and will have better performance.

> +               num_freed++;
> +               tx_ind = (tx_ind + 1) & TX_RING_MOD_MASK(ugeth->ug_info->bdRingLenTx[txQ]);
>
>                /* Advance the confirmation BD pointer */
>                if (!(bd_status & T_W))
>                        bd += sizeof(struct qe_bd);
>                else
>                        bd = ugeth->p_tx_bd_ring[txQ];
> -               bd_status = in_be32((u32 __iomem *)bd);
> +               bd_status = in_be32(bd2status(bd));
>        }
>        ugeth->confBd[txQ] = bd;
> +       ugeth->skb_dirtytx[txQ] = tx_ind;
> +       if (num_freed)
> +               netif_wake_queue(dev); /* We freed some buffers, so restart transmission */
>        return 0;
>  }
>
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH] ucc_geth: Rework the TX logic.

[Li Yang]

On Fri, Mar 27, 2009 at 1:44 AM, Joakim Tjernlund
<Joakim.Tjernlund@transmode.se> wrote:
> The line:
> =C2=A0if ((bd =3D=3D ugeth->txBd[txQ]) && (netif_queue_stopped(dev) =3D=
=3D 0))
> =C2=A0 =C2=A0 =C2=A0 break;
> in ucc_geth_tx() didn not make sense to me. Rework & cleanup
> this logic to something understandable.
> ---
>
> Reworked the patch according to Antons comments.
>
> =C2=A0drivers/net/ucc_geth.c | =C2=A0 66 +++++++++++++++++++++++++++-----=
---------------
> =C2=A01 files changed, 38 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/net/ucc_geth.c b/drivers/net/ucc_geth.c
> index 7fc91aa..465de3a 100644
> --- a/drivers/net/ucc_geth.c
> +++ b/drivers/net/ucc_geth.c
> @@ -161,6 +161,17 @@ static struct ucc_geth_info ugeth_primary_info =3D {
>
> =C2=A0static struct ucc_geth_info ugeth_info[8];
>
> +
> +static inline u32 __iomem *bd2buf(u8 __iomem *bd)
> +{
> + =C2=A0 =C2=A0 =C2=A0 return (u32 __iomem *)(bd+4);
> +}
> +
> +static inline u32 __iomem *bd2status(u8 __iomem *bd)
> +{
> + =C2=A0 =C2=A0 =C2=A0 return (u32 __iomem *)bd;
> +}


When the driver was reviewed for upstream merge, I was told to remove
this kind of thing in favor of direct struct qe_bd access.  So do we
really have a guideline on this?  Or it's just a matter of personal
preference?

> +
> =C2=A0#ifdef DEBUG
> =C2=A0static void mem_disp(u8 *addr, int size)
> =C2=A0{
> @@ -3048,6 +3059,7 @@ static int ucc_geth_start_xmit(struct sk_buff *skb,=
 struct net_device *dev)
> =C2=A0 =C2=A0 =C2=A0 =C2=A0u8 __iomem *bd; =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* BD pointer */
> =C2=A0 =C2=A0 =C2=A0 =C2=A0u32 bd_status;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0u8 txQ =3D 0;
> + =C2=A0 =C2=A0 =C2=A0 int tx_ind;
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0ugeth_vdbg("%s: IN", __func__);
>
> @@ -3057,21 +3069,19 @@ static int ucc_geth_start_xmit(struct sk_buff *sk=
b, struct net_device *dev)
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Start from the next BD that should be fille=
d */
> =C2=A0 =C2=A0 =C2=A0 =C2=A0bd =3D ugeth->txBd[txQ];
> - =C2=A0 =C2=A0 =C2=A0 bd_status =3D in_be32((u32 __iomem *)bd);
> + =C2=A0 =C2=A0 =C2=A0 bd_status =3D in_be32(bd2status(bd));
> =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Save the skb pointer so we can free it late=
r */
> - =C2=A0 =C2=A0 =C2=A0 ugeth->tx_skbuff[txQ][ugeth->skb_curtx[txQ]] =3D s=
kb;
> + =C2=A0 =C2=A0 =C2=A0 tx_ind =3D ugeth->skb_curtx[txQ];
> + =C2=A0 =C2=A0 =C2=A0 ugeth->tx_skbuff[txQ][tx_ind] =3D skb;
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Update the current skb pointer (wrapping if=
 this was the last) */
> - =C2=A0 =C2=A0 =C2=A0 ugeth->skb_curtx[txQ] =3D
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (ugeth->skb_curtx[txQ] +
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A01) & TX_RING_MOD_MASK(ugeth->u=
g_info->bdRingLenTx[txQ]);
> + =C2=A0 =C2=A0 =C2=A0 tx_ind =3D (tx_ind + 1) & TX_RING_MOD_MASK(ugeth->=
ug_info->bdRingLenTx[txQ]);
> + =C2=A0 =C2=A0 =C2=A0 ugeth->skb_curtx[txQ] =3D tx_ind;
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0/* set up the buffer descriptor */
> - =C2=A0 =C2=A0 =C2=A0 out_be32(&((struct qe_bd __iomem *)bd)->buf,
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 d=
ma_map_single(&ugeth->dev->dev, skb->data,
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 skb->len, DMA_TO_DEVICE));
> -
> - =C2=A0 =C2=A0 =C2=A0 /* printk(KERN_DEBUG"skb->data is 0x%x\n",skb->dat=
a); */
> + =C2=A0 =C2=A0 =C2=A0 out_be32(bd2buf(bd),
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0dma_map_single(&=
ugeth->dev->dev, skb->data,
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 skb->len, DMA_TO_DEVICE));
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0bd_status =3D (bd_status & T_W) | T_R | T_I | =
T_L | skb->len;
>
> @@ -3088,9 +3098,9 @@ static int ucc_geth_start_xmit(struct sk_buff *skb,=
 struct net_device *dev)
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0/* If the next BD still needs to be cleaned up=
, then the bds
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 are full. =C2=A0We need to tell the ke=
rnel to stop sending us stuff. */
> - =C2=A0 =C2=A0 =C2=A0 if (bd =3D=3D ugeth->confBd[txQ]) {
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (!netif_queue_stopp=
ed(dev))
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 netif_stop_queue(dev);
> +
> + =C2=A0 =C2=A0 =C2=A0 if (!in_be32(bd2buf(bd))) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 netif_stop_queue(dev);

Why does this make more sense?  The BD with a NULL buffer means the
ring is full?  It's not the normal case.

> =C2=A0 =C2=A0 =C2=A0 =C2=A0}
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0ugeth->txBd[txQ] =3D bd;
> @@ -3198,41 +3208,41 @@ static int ucc_geth_tx(struct net_device *dev, u8=
 txQ)
> =C2=A0 =C2=A0 =C2=A0 =C2=A0struct ucc_geth_private *ugeth =3D netdev_priv=
(dev);
> =C2=A0 =C2=A0 =C2=A0 =C2=A0u8 __iomem *bd; =C2=A0 =C2=A0 =C2=A0 =C2=A0 /*=
 BD pointer */
> =C2=A0 =C2=A0 =C2=A0 =C2=A0u32 bd_status;
> + =C2=A0 =C2=A0 =C2=A0 int tx_ind, num_freed;
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0bd =3D ugeth->confBd[txQ];
> - =C2=A0 =C2=A0 =C2=A0 bd_status =3D in_be32((u32 __iomem *)bd);
> -
> + =C2=A0 =C2=A0 =C2=A0 bd_status =3D in_be32(bd2status(bd));
> + =C2=A0 =C2=A0 =C2=A0 tx_ind =3D ugeth->skb_dirtytx[txQ];
> + =C2=A0 =C2=A0 =C2=A0 num_freed =3D 0;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Normal processing. */
> =C2=A0 =C2=A0 =C2=A0 =C2=A0while ((bd_status & T_R) =3D=3D 0) {
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* BD contains alr=
eady transmitted buffer. =C2=A0 */
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Handle the tran=
smitted buffer and release */
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* the BD to be us=
ed with the current frame =C2=A0*/
>
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if ((bd =3D=3D ugeth->=
txBd[txQ]) && (netif_queue_stopped(dev) =3D=3D 0))
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 break;
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (bd =3D=3D ugeth->t=
xBd[txQ])
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 break; /* Queue is empty */
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0dev->stats.tx_pack=
ets++;
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Free the sk buf=
fer associated with this TxBD */
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 dev_kfree_skb(ugeth->
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 tx_skbuff[txQ][ugeth->skb_dirtytx=
[txQ]]);
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ugeth->tx_skbuff[txQ][=
ugeth->skb_dirtytx[txQ]] =3D NULL;
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ugeth->skb_dirtytx[txQ=
] =3D
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (ugeth->=
skb_dirtytx[txQ] +
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A01)=
 & TX_RING_MOD_MASK(ugeth->ug_info->bdRingLenTx[txQ]);
> -
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* We freed a buffer, =
so now we can restart transmission */
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (netif_queue_stoppe=
d(dev))
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 netif_wake_queue(dev);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 dev_kfree_skb(ugeth->t=
x_skbuff[txQ][tx_ind]);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ugeth->tx_skbuff[txQ][=
tx_ind] =3D NULL;
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 out_be32(bd2buf(bd), 0=
); /* Mark it free */

Now I see why you depend on the buffer to judge if the BD ring is
full.  If you want to do this, make sure it's well documented in code,
or others will be confused.  And as Anton already commented, in/out
access is slow.  I think the original way can be fixed if you think
it's broken, and will have better performance.

> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 num_freed++;
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 tx_ind =3D (tx_ind + 1=
) & TX_RING_MOD_MASK(ugeth->ug_info->bdRingLenTx[txQ]);
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Advance the con=
firmation BD pointer */
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (!(bd_status & =
T_W))
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0bd +=3D sizeof(struct qe_bd);
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0else
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0bd =3D ugeth->p_tx_bd_ring[txQ];
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 bd_status =3D in_be32(=
(u32 __iomem *)bd);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 bd_status =3D in_be32(=
bd2status(bd));
> =C2=A0 =C2=A0 =C2=A0 =C2=A0}
> =C2=A0 =C2=A0 =C2=A0 =C2=A0ugeth->confBd[txQ] =3D bd;
> + =C2=A0 =C2=A0 =C2=A0 ugeth->skb_dirtytx[txQ] =3D tx_ind;
> + =C2=A0 =C2=A0 =C2=A0 if (num_freed)
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 netif_wake_queue(dev);=
 /* We freed some buffers, so restart transmission */
> =C2=A0 =C2=A0 =C2=A0 =C2=A0return 0;
> =C2=A0}
>

Re: [PATCH] ucc_geth: Rework the TX logic.

[Joakim Tjernlund]

pku.leo@gmail.com wrote on 27/03/2009 10:45:12:
> On Fri, Mar 27, 2009 at 1:44 AM, Joakim Tjernlund
> <Joakim.Tjernlund@transmode.se> 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.
> > ---
> >
> > Reworked the patch according to Antons comments.
> >
> >  drivers/net/ucc_geth.c |   66 
+++++++++++++++++++++++++++--------------------
> >  1 files changed, 38 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/net/ucc_geth.c b/drivers/net/ucc_geth.c
> > index 7fc91aa..465de3a 100644
> > --- a/drivers/net/ucc_geth.c
> > +++ b/drivers/net/ucc_geth.c
> > @@ -161,6 +161,17 @@ static struct ucc_geth_info ugeth_primary_info = 
{
> >
> >  static struct ucc_geth_info ugeth_info[8];
> >
> > +
> > +static inline u32 __iomem *bd2buf(u8 __iomem *bd)
> > +{
> > +       return (u32 __iomem *)(bd+4);
> > +}
> > +
> > +static inline u32 __iomem *bd2status(u8 __iomem *bd)
> > +{
> > +       return (u32 __iomem *)bd;
> > +}
> 
> 
> When the driver was reviewed for upstream merge, I was told to remove
> this kind of thing in favor of direct struct qe_bd access.  So do we
> really have a guideline on this?  Or it's just a matter of personal
> preference?

This is a bit better than the current type casts. Moving over to qe_bd
accesses is a bigger cleanup that will have to be a seperate patch.
I am not sure it is an all win as you probably loose the ability
to read both status and len in one access.

> 
> > +
> >  #ifdef DEBUG
> >  static void mem_disp(u8 *addr, int size)
> >  {
> > @@ -3048,6 +3059,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 tx_ind;
> >
> >        ugeth_vdbg("%s: IN", __func__);
> >
> > @@ -3057,21 +3069,19 @@ static int ucc_geth_start_xmit(struct sk_buff 
*skb, struct net_device *dev)
> >
> >        /* Start from the next BD that should be filled */
> >        bd = ugeth->txBd[txQ];
> > -       bd_status = in_be32((u32 __iomem *)bd);
> > +       bd_status = in_be32(bd2status(bd));
> >        /* Save the skb pointer so we can free it later */
> > -       ugeth->tx_skbuff[txQ][ugeth->skb_curtx[txQ]] = skb;
> > +       tx_ind = ugeth->skb_curtx[txQ];
> > +       ugeth->tx_skbuff[txQ][tx_ind] = 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]);
> > +       tx_ind = (tx_ind + 1) & 
TX_RING_MOD_MASK(ugeth->ug_info->bdRingLenTx[txQ]);
> > +       ugeth->skb_curtx[txQ] = tx_ind;
> >
> >        /* set up the buffer descriptor */
> > -       out_be32(&((struct qe_bd __iomem *)bd)->buf,
> > -                     dma_map_single(&ugeth->dev->dev, skb->data,
> > -                             skb->len, DMA_TO_DEVICE));
> > -
> > -       /* printk(KERN_DEBUG"skb->data is 0x%x\n",skb->data); */
> > +       out_be32(bd2buf(bd),
> > +                dma_map_single(&ugeth->dev->dev, skb->data,
> > +                               skb->len, DMA_TO_DEVICE));
> >
> >        bd_status = (bd_status & T_W) | T_R | T_I | T_L | skb->len;
> >
> > @@ -3088,9 +3098,9 @@ 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(bd2buf(bd))) {
> > +               netif_stop_queue(dev);
> 
> Why does this make more sense?  The BD with a NULL buffer means the
> ring is full?  It's not the normal case.
> 
> >        }
> >
> >        ugeth->txBd[txQ] = bd;
> > @@ -3198,41 +3208,41 @@ 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 tx_ind, num_freed;
> >
> >        bd = ugeth->confBd[txQ];
> > -       bd_status = in_be32((u32 __iomem *)bd);
> > -
> > +       bd_status = in_be32(bd2status(bd));
> > +       tx_ind = 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 (bd == ugeth->txBd[txQ])
> > +                       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][tx_ind]);
> > +               ugeth->tx_skbuff[txQ][tx_ind] = NULL;
> > +               out_be32(bd2buf(bd), 0); /* Mark it free */
> 
> Now I see why you depend on the buffer to judge if the BD ring is
> full.  If you want to do this, make sure it's well documented in code,
> or others will be confused.  And as Anton already commented, in/out
> access is slow.  I think the original way can be fixed if you think
> it's broken, and will have better performance.

The code could use a better comment, but I really think this method
is superior and more robust. The problem is that in/out functions is
much slower than it has to be for QE IO memory.

The original way is broken and I tired to fix it but it always broke
as soon one applied some load.

 Jocke 

Re: [PATCH] ucc_geth: Rework the TX logic.

[Joakim Tjernlund]

pku.leo@gmail.com wrote on 27/03/2009 10:45:12:
> On Fri, Mar 27, 2009 at 1:44 AM, Joakim Tjernlund
> <Joakim.Tjernlund@transmode.se> 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.
> > ---
> >
> > Reworked the patch according to Antons comments.
> >
> >  drivers/net/ucc_geth.c |   66 
+++++++++++++++++++++++++++--------------------
> >  1 files changed, 38 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/net/ucc_geth.c b/drivers/net/ucc_geth.c
> > index 7fc91aa..465de3a 100644
> > --- a/drivers/net/ucc_geth.c
> > +++ b/drivers/net/ucc_geth.c
> > @@ -161,6 +161,17 @@ static struct ucc_geth_info ugeth_primary_info = 
{
> >
> >  static struct ucc_geth_info ugeth_info[8];
> >
> > +
> > +static inline u32 __iomem *bd2buf(u8 __iomem *bd)
> > +{
> > +       return (u32 __iomem *)(bd+4);
> > +}
> > +
> > +static inline u32 __iomem *bd2status(u8 __iomem *bd)
> > +{
> > +       return (u32 __iomem *)bd;
> > +}
> 
> 
> When the driver was reviewed for upstream merge, I was told to remove
> this kind of thing in favor of direct struct qe_bd access.  So do we
> really have a guideline on this?  Or it's just a matter of personal
> preference?

This is a bit better than the current type casts. Moving over to qe_bd
accesses is a bigger cleanup that will have to be a seperate patch.
I am not sure it is an all win as you probably loose the ability
to read both status and len in one access.

> 
> > +
> >  #ifdef DEBUG
> >  static void mem_disp(u8 *addr, int size)
> >  {
> > @@ -3048,6 +3059,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 tx_ind;
> >
> >        ugeth_vdbg("%s: IN", __func__);
> >
> > @@ -3057,21 +3069,19 @@ static int ucc_geth_start_xmit(struct sk_buff 
*skb, struct net_device *dev)
> >
> >        /* Start from the next BD that should be filled */
> >        bd = ugeth->txBd[txQ];
> > -       bd_status = in_be32((u32 __iomem *)bd);
> > +       bd_status = in_be32(bd2status(bd));
> >        /* Save the skb pointer so we can free it later */
> > -       ugeth->tx_skbuff[txQ][ugeth->skb_curtx[txQ]] = skb;
> > +       tx_ind = ugeth->skb_curtx[txQ];
> > +       ugeth->tx_skbuff[txQ][tx_ind] = 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]);
> > +       tx_ind = (tx_ind + 1) & 
TX_RING_MOD_MASK(ugeth->ug_info->bdRingLenTx[txQ]);
> > +       ugeth->skb_curtx[txQ] = tx_ind;
> >
> >        /* set up the buffer descriptor */
> > -       out_be32(&((struct qe_bd __iomem *)bd)->buf,
> > -                     dma_map_single(&ugeth->dev->dev, skb->data,
> > -                             skb->len, DMA_TO_DEVICE));
> > -
> > -       /* printk(KERN_DEBUG"skb->data is 0x%x\n",skb->data); */
> > +       out_be32(bd2buf(bd),
> > +                dma_map_single(&ugeth->dev->dev, skb->data,
> > +                               skb->len, DMA_TO_DEVICE));
> >
> >        bd_status = (bd_status & T_W) | T_R | T_I | T_L | skb->len;
> >
> > @@ -3088,9 +3098,9 @@ 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(bd2buf(bd))) {
> > +               netif_stop_queue(dev);
> 
> Why does this make more sense?  The BD with a NULL buffer means the
> ring is full?  It's not the normal case.
> 
> >        }
> >
> >        ugeth->txBd[txQ] = bd;
> > @@ -3198,41 +3208,41 @@ 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 tx_ind, num_freed;
> >
> >        bd = ugeth->confBd[txQ];
> > -       bd_status = in_be32((u32 __iomem *)bd);
> > -
> > +       bd_status = in_be32(bd2status(bd));
> > +       tx_ind = 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 (bd == ugeth->txBd[txQ])
> > +                       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][tx_ind]);
> > +               ugeth->tx_skbuff[txQ][tx_ind] = NULL;
> > +               out_be32(bd2buf(bd), 0); /* Mark it free */
> 
> Now I see why you depend on the buffer to judge if the BD ring is
> full.  If you want to do this, make sure it's well documented in code,
> or others will be confused.  And as Anton already commented, in/out
> access is slow.  I think the original way can be fixed if you think
> it's broken, and will have better performance.

The code could use a better comment, but I really think this method
is superior and more robust. The problem is that in/out functions is
much slower than it has to be for QE IO memory.

The original way is broken and I tired to fix it but it always broke
as soon one applied some load.

 Jocke 

Re: [PATCH] ucc_geth: Rework the TX logic.

[Li Yang]

On Fri, Mar 27, 2009 at 6:23 PM, Joakim Tjernlund
<Joakim.Tjernlund@transmode.se> wrote:
> pku.leo@gmail.com wrote on 27/03/2009 10:45:12:
>> On Fri, Mar 27, 2009 at 1:44 AM, Joakim Tjernlund
>> <Joakim.Tjernlund@transmode.se> 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.
>> > ---
>> >
>> > Reworked the patch according to Antons comments.
>> >
>> >  drivers/net/ucc_geth.c |   66
> +++++++++++++++++++++++++++--------------------
>> >  1 files changed, 38 insertions(+), 28 deletions(-)
>> >
>> > diff --git a/drivers/net/ucc_geth.c b/drivers/net/ucc_geth.c
>> > index 7fc91aa..465de3a 100644
>> > --- a/drivers/net/ucc_geth.c
>> > +++ b/drivers/net/ucc_geth.c
>> > @@ -161,6 +161,17 @@ static struct ucc_geth_info ugeth_primary_info =
> {
>> >
>> >  static struct ucc_geth_info ugeth_info[8];
>> >
>> > +
>> > +static inline u32 __iomem *bd2buf(u8 __iomem *bd)
>> > +{
>> > +       return (u32 __iomem *)(bd+4);
>> > +}
>> > +
>> > +static inline u32 __iomem *bd2status(u8 __iomem *bd)
>> > +{
>> > +       return (u32 __iomem *)bd;
>> > +}
>>
>>
>> When the driver was reviewed for upstream merge, I was told to remove
>> this kind of thing in favor of direct struct qe_bd access.  So do we
>> really have a guideline on this?  Or it's just a matter of personal
>> preference?
>
> This is a bit better than the current type casts. Moving over to qe_bd
> accesses is a bigger cleanup that will have to be a seperate patch.
> I am not sure it is an all win as you probably loose the ability
> to read both status and len in one access.
>
>>
>> > +
>> >  #ifdef DEBUG
>> >  static void mem_disp(u8 *addr, int size)
>> >  {
>> > @@ -3048,6 +3059,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 tx_ind;
>> >
>> >        ugeth_vdbg("%s: IN", __func__);
>> >
>> > @@ -3057,21 +3069,19 @@ static int ucc_geth_start_xmit(struct sk_buff
> *skb, struct net_device *dev)
>> >
>> >        /* Start from the next BD that should be filled */
>> >        bd = ugeth->txBd[txQ];
>> > -       bd_status = in_be32((u32 __iomem *)bd);
>> > +       bd_status = in_be32(bd2status(bd));
>> >        /* Save the skb pointer so we can free it later */
>> > -       ugeth->tx_skbuff[txQ][ugeth->skb_curtx[txQ]] = skb;
>> > +       tx_ind = ugeth->skb_curtx[txQ];
>> > +       ugeth->tx_skbuff[txQ][tx_ind] = 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]);
>> > +       tx_ind = (tx_ind + 1) &
> TX_RING_MOD_MASK(ugeth->ug_info->bdRingLenTx[txQ]);
>> > +       ugeth->skb_curtx[txQ] = tx_ind;
>> >
>> >        /* set up the buffer descriptor */
>> > -       out_be32(&((struct qe_bd __iomem *)bd)->buf,
>> > -                     dma_map_single(&ugeth->dev->dev, skb->data,
>> > -                             skb->len, DMA_TO_DEVICE));
>> > -
>> > -       /* printk(KERN_DEBUG"skb->data is 0x%x\n",skb->data); */
>> > +       out_be32(bd2buf(bd),
>> > +                dma_map_single(&ugeth->dev->dev, skb->data,
>> > +                               skb->len, DMA_TO_DEVICE));
>> >
>> >        bd_status = (bd_status & T_W) | T_R | T_I | T_L | skb->len;
>> >
>> > @@ -3088,9 +3098,9 @@ 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(bd2buf(bd))) {
>> > +               netif_stop_queue(dev);
>>
>> Why does this make more sense?  The BD with a NULL buffer means the
>> ring is full?  It's not the normal case.
>>
>> >        }
>> >
>> >        ugeth->txBd[txQ] = bd;
>> > @@ -3198,41 +3208,41 @@ 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 tx_ind, num_freed;
>> >
>> >        bd = ugeth->confBd[txQ];
>> > -       bd_status = in_be32((u32 __iomem *)bd);
>> > -
>> > +       bd_status = in_be32(bd2status(bd));
>> > +       tx_ind = 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 (bd == ugeth->txBd[txQ])
>> > +                       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][tx_ind]);
>> > +               ugeth->tx_skbuff[txQ][tx_ind] = NULL;
>> > +               out_be32(bd2buf(bd), 0); /* Mark it free */
>>
>> Now I see why you depend on the buffer to judge if the BD ring is
>> full.  If you want to do this, make sure it's well documented in code,
>> or others will be confused.  And as Anton already commented, in/out
>> access is slow.  I think the original way can be fixed if you think
>> it's broken, and will have better performance.
>
> The code could use a better comment, but I really think this method
> is superior and more robust. The problem is that in/out functions is
> much slower than it has to be for QE IO memory.
>
> The original way is broken and I tired to fix it but it always broke
> as soon one applied some load.

The only difference I see between your method and the original code is
that you update the cleanup progress on every BD.  The original code
updates progress only after all sent BDs are processed.  You can try
move ugeth->confBd[txQ] = bd; into the loop then it will be the same
as your proposed code.

- Leo

Re: [PATCH] ucc_geth: Rework the TX logic.

[Li Yang]

On Fri, Mar 27, 2009 at 6:23 PM, Joakim Tjernlund
<Joakim.Tjernlund@transmode.se> wrote:
> pku.leo@gmail.com wrote on 27/03/2009 10:45:12:
>> On Fri, Mar 27, 2009 at 1:44 AM, Joakim Tjernlund
>> <Joakim.Tjernlund@transmode.se> wrote:
>> > The line:
>> > =C2=A0if ((bd =3D=3D ugeth->txBd[txQ]) && (netif_queue_stopped(dev) =
=3D=3D 0))
>> > =C2=A0 =C2=A0 =C2=A0 break;
>> > in ucc_geth_tx() didn not make sense to me. Rework & cleanup
>> > this logic to something understandable.
>> > ---
>> >
>> > Reworked the patch according to Antons comments.
>> >
>> > =C2=A0drivers/net/ucc_geth.c | =C2=A0 66
> +++++++++++++++++++++++++++--------------------
>> > =C2=A01 files changed, 38 insertions(+), 28 deletions(-)
>> >
>> > diff --git a/drivers/net/ucc_geth.c b/drivers/net/ucc_geth.c
>> > index 7fc91aa..465de3a 100644
>> > --- a/drivers/net/ucc_geth.c
>> > +++ b/drivers/net/ucc_geth.c
>> > @@ -161,6 +161,17 @@ static struct ucc_geth_info ugeth_primary_info =
=3D
> {
>> >
>> > =C2=A0static struct ucc_geth_info ugeth_info[8];
>> >
>> > +
>> > +static inline u32 __iomem *bd2buf(u8 __iomem *bd)
>> > +{
>> > + =C2=A0 =C2=A0 =C2=A0 return (u32 __iomem *)(bd+4);
>> > +}
>> > +
>> > +static inline u32 __iomem *bd2status(u8 __iomem *bd)
>> > +{
>> > + =C2=A0 =C2=A0 =C2=A0 return (u32 __iomem *)bd;
>> > +}
>>
>>
>> When the driver was reviewed for upstream merge, I was told to remove
>> this kind of thing in favor of direct struct qe_bd access. =C2=A0So do w=
e
>> really have a guideline on this? =C2=A0Or it's just a matter of personal
>> preference?
>
> This is a bit better than the current type casts. Moving over to qe_bd
> accesses is a bigger cleanup that will have to be a seperate patch.
> I am not sure it is an all win as you probably loose the ability
> to read both status and len in one access.
>
>>
>> > +
>> > =C2=A0#ifdef DEBUG
>> > =C2=A0static void mem_disp(u8 *addr, int size)
>> > =C2=A0{
>> > @@ -3048,6 +3059,7 @@ static int ucc_geth_start_xmit(struct sk_buff
> *skb, struct net_device *dev)
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0u8 __iomem *bd; =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* BD pointer */
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0u32 bd_status;
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0u8 txQ =3D 0;
>> > + =C2=A0 =C2=A0 =C2=A0 int tx_ind;
>> >
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0ugeth_vdbg("%s: IN", __func__);
>> >
>> > @@ -3057,21 +3069,19 @@ static int ucc_geth_start_xmit(struct sk_buff
> *skb, struct net_device *dev)
>> >
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Start from the next BD that should be fi=
lled */
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0bd =3D ugeth->txBd[txQ];
>> > - =C2=A0 =C2=A0 =C2=A0 bd_status =3D in_be32((u32 __iomem *)bd);
>> > + =C2=A0 =C2=A0 =C2=A0 bd_status =3D in_be32(bd2status(bd));
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Save the skb pointer so we can free it l=
ater */
>> > - =C2=A0 =C2=A0 =C2=A0 ugeth->tx_skbuff[txQ][ugeth->skb_curtx[txQ]] =
=3D skb;
>> > + =C2=A0 =C2=A0 =C2=A0 tx_ind =3D ugeth->skb_curtx[txQ];
>> > + =C2=A0 =C2=A0 =C2=A0 ugeth->tx_skbuff[txQ][tx_ind] =3D skb;
>> >
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Update the current skb pointer (wrapping=
 if this was the
> last) */
>> > - =C2=A0 =C2=A0 =C2=A0 ugeth->skb_curtx[txQ] =3D
>> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (ugeth->skb_curtx[txQ] +
>> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A01) & TX_RING_MOD_MASK(ugeth=
->ug_info->bdRingLenTx[txQ]);
>> > + =C2=A0 =C2=A0 =C2=A0 tx_ind =3D (tx_ind + 1) &
> TX_RING_MOD_MASK(ugeth->ug_info->bdRingLenTx[txQ]);
>> > + =C2=A0 =C2=A0 =C2=A0 ugeth->skb_curtx[txQ] =3D tx_ind;
>> >
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0/* set up the buffer descriptor */
>> > - =C2=A0 =C2=A0 =C2=A0 out_be32(&((struct qe_bd __iomem *)bd)->buf,
>> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 dma_map_single(&ugeth->dev->dev, skb->data,
>> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 skb->len, DMA_TO_DEVICE));
>> > -
>> > - =C2=A0 =C2=A0 =C2=A0 /* printk(KERN_DEBUG"skb->data is 0x%x\n",skb->=
data); */
>> > + =C2=A0 =C2=A0 =C2=A0 out_be32(bd2buf(bd),
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0dma_map_singl=
e(&ugeth->dev->dev, skb->data,
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 skb->len, DMA_TO_DEVICE));
>> >
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0bd_status =3D (bd_status & T_W) | T_R | T_I=
 | T_L | skb->len;
>> >
>> > @@ -3088,9 +3098,9 @@ static int ucc_geth_start_xmit(struct sk_buff
> *skb, struct net_device *dev)
>> >
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0/* If the next BD still needs to be cleaned=
 up, then the bds
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 are full. =C2=A0We need to tell the=
 kernel to stop sending us
> stuff. */
>> > - =C2=A0 =C2=A0 =C2=A0 if (bd =3D=3D ugeth->confBd[txQ]) {
>> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (!netif_queue_st=
opped(dev))
>> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 netif_stop_queue(dev);
>> > +
>> > + =C2=A0 =C2=A0 =C2=A0 if (!in_be32(bd2buf(bd))) {
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 netif_stop_queue(de=
v);
>>
>> Why does this make more sense? =C2=A0The BD with a NULL buffer means the
>> ring is full? =C2=A0It's not the normal case.
>>
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0}
>> >
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0ugeth->txBd[txQ] =3D bd;
>> > @@ -3198,41 +3208,41 @@ static int ucc_geth_tx(struct net_device *dev,
> u8 txQ)
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0struct ucc_geth_private *ugeth =3D netdev_p=
riv(dev);
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0u8 __iomem *bd; =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 /* BD pointer */
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0u32 bd_status;
>> > + =C2=A0 =C2=A0 =C2=A0 int tx_ind, num_freed;
>> >
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0bd =3D ugeth->confBd[txQ];
>> > - =C2=A0 =C2=A0 =C2=A0 bd_status =3D in_be32((u32 __iomem *)bd);
>> > -
>> > + =C2=A0 =C2=A0 =C2=A0 bd_status =3D in_be32(bd2status(bd));
>> > + =C2=A0 =C2=A0 =C2=A0 tx_ind =3D ugeth->skb_dirtytx[txQ];
>> > + =C2=A0 =C2=A0 =C2=A0 num_freed =3D 0;
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Normal processing. */
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0while ((bd_status & T_R) =3D=3D 0) {
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* BD contains =
already transmitted buffer. =C2=A0 */
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Handle the t=
ransmitted buffer and release */
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* the BD to be=
 used with the current frame =C2=A0*/
>> >
>> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if ((bd =3D=3D uget=
h->txBd[txQ]) &&
> (netif_queue_stopped(dev) =3D=3D 0))
>> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 break;
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (bd =3D=3D ugeth=
->txBd[txQ])
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 break; /* Queue is empty */
>> >
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0dev->stats.tx_p=
ackets++;
>> >
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Free the sk =
buffer associated with this TxBD */
>> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 dev_kfree_skb(ugeth=
->
>> > -
> tx_skbuff[txQ][ugeth->skb_dirtytx[txQ]]);
>> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ugeth->tx_skbuff[tx=
Q][ugeth->skb_dirtytx[txQ]] =3D NULL;
>> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ugeth->skb_dirtytx[=
txQ] =3D
>> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (uget=
h->skb_dirtytx[txQ] +
>> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A01) &
> TX_RING_MOD_MASK(ugeth->ug_info->bdRingLenTx[txQ]);
>> > -
>> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* We freed a buffe=
r, so now we can restart
> transmission */
>> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (netif_queue_sto=
pped(dev))
>> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 netif_wake_queue(dev);
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 dev_kfree_skb(ugeth=
->tx_skbuff[txQ][tx_ind]);
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ugeth->tx_skbuff[tx=
Q][tx_ind] =3D NULL;
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 out_be32(bd2buf(bd)=
, 0); /* Mark it free */
>>
>> Now I see why you depend on the buffer to judge if the BD ring is
>> full. =C2=A0If you want to do this, make sure it's well documented in co=
de,
>> or others will be confused. =C2=A0And as Anton already commented, in/out
>> access is slow. =C2=A0I think the original way can be fixed if you think
>> it's broken, and will have better performance.
>
> The code could use a better comment, but I really think this method
> is superior and more robust. The problem is that in/out functions is
> much slower than it has to be for QE IO memory.
>
> The original way is broken and I tired to fix it but it always broke
> as soon one applied some load.

The only difference I see between your method and the original code is
that you update the cleanup progress on every BD.  The original code
updates progress only after all sent BDs are processed.  You can try
move ugeth->confBd[txQ] =3D bd; into the loop then it will be the same
as your proposed code.

- Leo

Re: [PATCH] ucc_geth: Rework the TX logic.

[Joakim Tjernlund]

pku.leo@gmail.com wrote on 27/03/2009 11:39:07:
> On Fri, Mar 27, 2009 at 6:23 PM, Joakim Tjernlund
> >> > -               /* 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][tx_ind]);
> >> > +               ugeth->tx_skbuff[txQ][tx_ind] = NULL;
> >> > +               out_be32(bd2buf(bd), 0); /* Mark it free */
> >>
> >> Now I see why you depend on the buffer to judge if the BD ring is
> >> full.  If you want to do this, make sure it's well documented in 
code,
> >> or others will be confused.  And as Anton already commented, in/out
> >> access is slow.  I think the original way can be fixed if you think
> >> it's broken, and will have better performance.
> >
> > The code could use a better comment, but I really think this method
> > is superior and more robust. The problem is that in/out functions is
> > much slower than it has to be for QE IO memory.
> >
> > The original way is broken and I tired to fix it but it always broke
> > as soon one applied some load.
> 
> The only difference I see between your method and the original code is
> that you update the cleanup progress on every BD.  The original code
> updates progress only after all sent BDs are processed.  You can try
> move ugeth->confBd[txQ] = bd; into the loop then it will be the same
> as your proposed code.

Already tried that+lots of other combinatios. Doesn't work

Re: [PATCH] ucc_geth: Rework the TX logic.

[Scott Wood]

Joakim Tjernlund wrote:
> This is a bit better than the current type casts. Moving over to qe_bd
> accesses is a bigger cleanup that will have to be a seperate patch.
> I am not sure it is an all win as you probably loose the ability
> to read both status and len in one access.

Not if you define the struct to have one status_len field (or a union of 
that with separate fields), as gianfar does.

-Scott

Re: [PATCH] ucc_geth: Rework the TX logic.

[Joakim Tjernlund]

Scott Wood <scottwood@freescale.com> wrote on 27/03/2009 14:26:00:
> 
> Joakim Tjernlund wrote:
> > This is a bit better than the current type casts. Moving over to qe_bd
> > accesses is a bigger cleanup that will have to be a seperate patch.
> > I am not sure it is an all win as you probably loose the ability
> > to read both status and len in one access.
> 
> Not if you define the struct to have one status_len field (or a union of 

> that with separate fields), as gianfar does.

gianfar does not seem to use in_/out_ functions for the BDs. Works just
fine that too it seems. I always felt that the in_/out_ functions
was extra baggage that the Freescale CPUs don't need. There is
something in between that is missing.

 Jocke

Re: [PATCH] ucc_geth: Rework the TX logic.

[Joakim Tjernlund]

Scott Wood <scottwood@freescale.com> wrote on 27/03/2009 14:26:00:
> 
> Joakim Tjernlund wrote:
> > This is a bit better than the current type casts. Moving over to qe_bd
> > accesses is a bigger cleanup that will have to be a seperate patch.
> > I am not sure it is an all win as you probably loose the ability
> > to read both status and len in one access.
> 
> Not if you define the struct to have one status_len field (or a union of 

> that with separate fields), as gianfar does.

gianfar does not seem to use in_/out_ functions for the BDs. Works just
fine that too it seems. I always felt that the in_/out_ functions
was extra baggage that the Freescale CPUs don't need. There is
something in between that is missing.

 Jocke

Re: [PATCH] ucc_geth: Rework the TX logic.

[Scott Wood]

Joakim Tjernlund wrote:
> gianfar does not seem to use in_/out_ functions for the BDs. Works just
> fine that too it seems.

It does now that it has explicit barriers in a few places.  Before they 
were added, it would sometimes fail under load.  That was due to a 
compiler reordering, but CPU reordering was possible as well.

-Scott

Re: [PATCH] ucc_geth: Rework the TX logic.

[Joakim Tjernlund]

Scott Wood <scottwood@freescale.com> wrote on 30/03/2009 19:22:03:
> 
> Joakim Tjernlund wrote:
> > gianfar does not seem to use in_/out_ functions for the BDs. Works 
just
> > fine that too it seems.
> 
> It does now that it has explicit barriers in a few places.  Before they

In 2.6.29 or later?
 
> were added, it would sometimes fail under load.  That was due to a 
> compiler reordering, but CPU reordering was possible as well.

Does not the CPU skip reordering if the guarded bit is set?

Could we not use the same in ucc_geth as well? Then people would
not need to worry about "performance issues".

 Jocke

Re: [PATCH] ucc_geth: Rework the TX logic.

[Joakim Tjernlund]

Scott Wood <scottwood@freescale.com> wrote on 30/03/2009 19:22:03:
> 
> Joakim Tjernlund wrote:
> > gianfar does not seem to use in_/out_ functions for the BDs. Works 
just
> > fine that too it seems.
> 
> It does now that it has explicit barriers in a few places.  Before they

In 2.6.29 or later?
 
> were added, it would sometimes fail under load.  That was due to a 
> compiler reordering, but CPU reordering was possible as well.

Does not the CPU skip reordering if the guarded bit is set?

Could we not use the same in ucc_geth as well? Then people would
not need to worry about "performance issues".

 Jocke

Re: [PATCH] ucc_geth: Rework the TX logic.

[Scott Wood]

Joakim Tjernlund wrote:
> Scott Wood <scottwood@freescale.com> wrote on 30/03/2009 19:22:03:
>> Joakim Tjernlund wrote:
>>> gianfar does not seem to use in_/out_ functions for the BDs. Works 
> just
>>> fine that too it seems.
>> It does now that it has explicit barriers in a few places.  Before they
> 
> In 2.6.29 or later?

No, it was earlier.

>> were added, it would sometimes fail under load.  That was due to a 
>> compiler reordering, but CPU reordering was possible as well.
> 
> Does not the CPU skip reordering if the guarded bit is set?

The guarded bit is typically not set for DMA buffers.  ucc_geth is a bit 
different since descriptors are in MURAM which is ioremap()ed -- though 
switching to a cacheable mapping with barriers should be a performance 
improvement.

-Scott

Re: [PATCH] ucc_geth: Rework the TX logic.

[Joakim Tjernlund]

Scott Wood <scottwood@freescale.com> wrote on 30/03/2009 19:45:17:
> 
> Joakim Tjernlund wrote:
> > Scott Wood <scottwood@freescale.com> wrote on 30/03/2009 19:22:03:
> >> Joakim Tjernlund wrote:
> >>> gianfar does not seem to use in_/out_ functions for the BDs. Works 
> > just
> >>> fine that too it seems.
> >> It does now that it has explicit barriers in a few places.  Before 
they
> > 
> > In 2.6.29 or later?
> 
> No, it was earlier.

Ah, I see now. The eieio() stuff.

> 
> >> were added, it would sometimes fail under load.  That was due to a 
> >> compiler reordering, but CPU reordering was possible as well.
> > 
> > Does not the CPU skip reordering if the guarded bit is set?
> 
> The guarded bit is typically not set for DMA buffers.  ucc_geth is a bit 

> different since descriptors are in MURAM which is ioremap()ed -- though 
> switching to a cacheable mapping with barriers should be a performance 
> improvement.

I always thought that MURAM was very fast. The whole reason to have BDs in
MURAM is that it is faster than normal RAM, at least that is what I 
thought.

 Jocke

Re: [PATCH] ucc_geth: Rework the TX logic.

[Scott Wood]

Joakim Tjernlund wrote:
>> different since descriptors are in MURAM which is ioremap()ed -- though 
>> switching to a cacheable mapping with barriers should be a performance 
>> improvement.
> 
> I always thought that MURAM was very fast. The whole reason to have BDs in
> MURAM is that it is faster than normal RAM, at least that is what I 
> thought.

Yeah, on second thought it probably wouldn't be worth it.  There's also 
the question of under what circumstances the QE's MURAM accesses will be 
cache-coherent.

As for the CPU not reordering guarded+cache inhibited accesses, that 
initially seemed to be true for the new arch stuff (book3e/book3s, but 
not really, see below), but the classic arch documentation only 
guarantees stores to such regions to be in-order (and the 
explicitly-specified operation of eieio on I+G accesses wouldn't make 
much sense if they were already guaranteed to be in-order).

Then I looked at the EREF to see what older book E documents had to say 
on the issue, and it suggests that when the architecture document says 
"out of order", it really means "speculative" (and reading the arch 
doc's definition of "out of order" seems to confirm this -- redefining 
terms is bad, m'kay?).  So it seems that the simple answer is no, 
guarded storage is not guaranteed to be in order, unless the only thing 
that can cause an out-of-order access is speculative execution.

-Scott

Re: [PATCH] ucc_geth: Rework the TX logic.

[Scott Wood]

Joakim Tjernlund wrote:
>> different since descriptors are in MURAM which is ioremap()ed -- though 
>> switching to a cacheable mapping with barriers should be a performance 
>> improvement.
> 
> I always thought that MURAM was very fast. The whole reason to have BDs in
> MURAM is that it is faster than normal RAM, at least that is what I 
> thought.

Yeah, on second thought it probably wouldn't be worth it.  There's also 
the question of under what circumstances the QE's MURAM accesses will be 
cache-coherent.

As for the CPU not reordering guarded+cache inhibited accesses, that 
initially seemed to be true for the new arch stuff (book3e/book3s, but 
not really, see below), but the classic arch documentation only 
guarantees stores to such regions to be in-order (and the 
explicitly-specified operation of eieio on I+G accesses wouldn't make 
much sense if they were already guaranteed to be in-order).

Then I looked at the EREF to see what older book E documents had to say 
on the issue, and it suggests that when the architecture document says 
"out of order", it really means "speculative" (and reading the arch 
doc's definition of "out of order" seems to confirm this -- redefining 
terms is bad, m'kay?).  So it seems that the simple answer is no, 
guarded storage is not guaranteed to be in order, unless the only thing 
that can cause an out-of-order access is speculative execution.

-Scott

Re: [PATCH] ucc_geth: Rework the TX logic.

[Joakim Tjernlund]

Scott Wood <scottwood@freescale.com> wrote on 30/03/2009 21:32:23:
> 
> Joakim Tjernlund wrote:
> >> different since descriptors are in MURAM which is ioremap()ed -- 
though 
> >> switching to a cacheable mapping with barriers should be a 
performance 
> >> improvement.
> > 
> > I always thought that MURAM was very fast. The whole reason to have 
BDs in
> > MURAM is that it is faster than normal RAM, at least that is what I 
> > thought.
> 
> Yeah, on second thought it probably wouldn't be worth it.  There's also 
> the question of under what circumstances the QE's MURAM accesses will be 

> cache-coherent.

I am a bit confused, what isn't worth it? Currently MURAM isn't used by 
ucc_geth, but
it is easy to change. Swap MEM_PART_SYSTEM to MEM_PART_MURAM, however, 
just
tried that and the driver stopped working. I known this worked earlier 
because
I tried it and I even think I sent a patch to Leo.

What choices do we have, I see three:

1) MEM_PART_SYSTEM, as today.
2) MEM_PART_MURAM. I guess this should be uncacheable memory?
3) as gianfar, dma_alloc_coherent(). I presume this is uncacheable memory?

My guess would be 2 or 3. Do they have the same synchronization
sematics?

> 
> As for the CPU not reordering guarded+cache inhibited accesses, that 
> initially seemed to be true for the new arch stuff (book3e/book3s, but 
> not really, see below), but the classic arch documentation only 
> guarantees stores to such regions to be in-order (and the 
> explicitly-specified operation of eieio on I+G accesses wouldn't make 
> much sense if they were already guaranteed to be in-order).
> 
> Then I looked at the EREF to see what older book E documents had to say 
> on the issue, and it suggests that when the architecture document says 
> "out of order", it really means "speculative" (and reading the arch 
> doc's definition of "out of order" seems to confirm this -- redefining 
> terms is bad, m'kay?).  So it seems that the simple answer is no, 
> guarded storage is not guaranteed to be in order, unless the only thing 
> that can cause an out-of-order access is speculative execution.

Very informative, thanks.

Re: [PATCH] ucc_geth: Rework the TX logic.

[Joakim Tjernlund]

Scott Wood <scottwood@freescale.com> wrote on 30/03/2009 21:32:23:
> 
> Joakim Tjernlund wrote:
> >> different since descriptors are in MURAM which is ioremap()ed -- 
though 
> >> switching to a cacheable mapping with barriers should be a 
performance 
> >> improvement.
> > 
> > I always thought that MURAM was very fast. The whole reason to have 
BDs in
> > MURAM is that it is faster than normal RAM, at least that is what I 
> > thought.
> 
> Yeah, on second thought it probably wouldn't be worth it.  There's also 
> the question of under what circumstances the QE's MURAM accesses will be 

> cache-coherent.

I am a bit confused, what isn't worth it? Currently MURAM isn't used by 
ucc_geth, but
it is easy to change. Swap MEM_PART_SYSTEM to MEM_PART_MURAM, however, 
just
tried that and the driver stopped working. I known this worked earlier 
because
I tried it and I even think I sent a patch to Leo.

What choices do we have, I see three:

1) MEM_PART_SYSTEM, as today.
2) MEM_PART_MURAM. I guess this should be uncacheable memory?
3) as gianfar, dma_alloc_coherent(). I presume this is uncacheable memory?

My guess would be 2 or 3. Do they have the same synchronization
sematics?

> 
> As for the CPU not reordering guarded+cache inhibited accesses, that 
> initially seemed to be true for the new arch stuff (book3e/book3s, but 
> not really, see below), but the classic arch documentation only 
> guarantees stores to such regions to be in-order (and the 
> explicitly-specified operation of eieio on I+G accesses wouldn't make 
> much sense if they were already guaranteed to be in-order).
> 
> Then I looked at the EREF to see what older book E documents had to say 
> on the issue, and it suggests that when the architecture document says 
> "out of order", it really means "speculative" (and reading the arch 
> doc's definition of "out of order" seems to confirm this -- redefining 
> terms is bad, m'kay?).  So it seems that the simple answer is no, 
> guarded storage is not guaranteed to be in order, unless the only thing 
> that can cause an out-of-order access is speculative execution.

Very informative, thanks.

Re: [PATCH] ucc_geth: Rework the TX logic.

[Li Yang]

On Tue, Mar 31, 2009 at 5:07 PM, Joakim Tjernlund
<Joakim.Tjernlund@transmode.se> wrote:
> Scott Wood <scottwood@freescale.com> wrote on 30/03/2009 21:32:23:
>>
>> Joakim Tjernlund wrote:
>> >> different since descriptors are in MURAM which is ioremap()ed --
> though
>> >> switching to a cacheable mapping with barriers should be a
> performance
>> >> improvement.
>> >
>> > I always thought that MURAM was very fast. The whole reason to have
> BDs in
>> > MURAM is that it is faster than normal RAM, at least that is what I
>> > thought.
>>
>> Yeah, on second thought it probably wouldn't be worth it.  There's also
>> the question of under what circumstances the QE's MURAM accesses will be
>
>> cache-coherent.
>
> I am a bit confused, what isn't worth it? Currently MURAM isn't used by
> ucc_geth, but
> it is easy to change. Swap MEM_PART_SYSTEM to MEM_PART_MURAM, however,
> just
> tried that and the driver stopped working. I known this worked earlier
> because
> I tried it and I even think I sent a patch to Leo.
>
> What choices do we have, I see three:
>
> 1) MEM_PART_SYSTEM, as today.
> 2) MEM_PART_MURAM. I guess this should be uncacheable memory?
> 3) as gianfar, dma_alloc_coherent(). I presume this is uncacheable memory?
>

1 and 3 are the same.  All of them use cacheable memory as we have a
hardware coherency module to take care of the cache coherency problem.
 However it might be better to use dma_alloc_coherent() for the code
to be more readible.  Thanks.

- Leo
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH] ucc_geth: Rework the TX logic.

[Li Yang]

On Tue, Mar 31, 2009 at 5:07 PM, Joakim Tjernlund
<Joakim.Tjernlund@transmode.se> wrote:
> Scott Wood <scottwood@freescale.com> wrote on 30/03/2009 21:32:23:
>>
>> Joakim Tjernlund wrote:
>> >> different since descriptors are in MURAM which is ioremap()ed --
> though
>> >> switching to a cacheable mapping with barriers should be a
> performance
>> >> improvement.
>> >
>> > I always thought that MURAM was very fast. The whole reason to have
> BDs in
>> > MURAM is that it is faster than normal RAM, at least that is what I
>> > thought.
>>
>> Yeah, on second thought it probably wouldn't be worth it. =C2=A0There's =
also
>> the question of under what circumstances the QE's MURAM accesses will be
>
>> cache-coherent.
>
> I am a bit confused, what isn't worth it? Currently MURAM isn't used by
> ucc_geth, but
> it is easy to change. Swap MEM_PART_SYSTEM to MEM_PART_MURAM, however,
> just
> tried that and the driver stopped working. I known this worked earlier
> because
> I tried it and I even think I sent a patch to Leo.
>
> What choices do we have, I see three:
>
> 1) MEM_PART_SYSTEM, as today.
> 2) MEM_PART_MURAM. I guess this should be uncacheable memory?
> 3) as gianfar, dma_alloc_coherent(). I presume this is uncacheable memory=
?
>

1 and 3 are the same.  All of them use cacheable memory as we have a
hardware coherency module to take care of the cache coherency problem.
 However it might be better to use dma_alloc_coherent() for the code
to be more readible.  Thanks.

- Leo

Re: [PATCH] ucc_geth: Rework the TX logic.

[Scott Wood]

Joakim Tjernlund wrote:
> I am a bit confused, what isn't worth it?

Enabling cacheing on MURAM, at least when used for buffer descriptors. 
The cache line ping-pong would probably outweigh the cost of the 
uncached accesses.

> Currently MURAM isn't used by ucc_geth,

Hmm.  I looked in the driver and saw numerous muram allocations, but I 
didn't try to follow the driver enough to ensure that they were for the 
ring.  I'd assumed it was similar to the CPM1/CPM2 driver.

> 3) as gianfar, dma_alloc_coherent(). I presume this is uncacheable memory?

It would be uncacheable on systems without coherent DMA, but I don't 
think there are any such systems that use gianfar.

> My guess would be 2 or 3. Do they have the same synchronization
> sematics?

No, unfortunately.  PowerPC sync instructions are a bit complicated. 
For example, you can use eieio to sync between reading the interrupt 
status register and checking the ring buffer, if they're both mapped 
I+G, but not if the former is I+G and the latter is cacheable (you need 
a full sync in that case).

-Scott

Re: [PATCH] ucc_geth: Rework the TX logic.

[Li Yang]

On Tue, Mar 31, 2009 at 1:22 AM, Scott Wood <scottwood@freescale.com> wrote:
> Joakim Tjernlund wrote:
>>
>> gianfar does not seem to use in_/out_ functions for the BDs. Works just
>> fine that too it seems.
>
> It does now that it has explicit barriers in a few places.  Before they were
> added, it would sometimes fail under load.  That was due to a compiler
> reordering, but CPU reordering was possible as well.

I noticed that in gianfar these memory access is not protected by
"volatile".  Can this be the reason why the compiler did some unwanted
optimization?

- Leo
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH] ucc_geth: Rework the TX logic.

[Li Yang]

On Tue, Mar 31, 2009 at 1:22 AM, Scott Wood <scottwood@freescale.com> wrote=
:
> Joakim Tjernlund wrote:
>>
>> gianfar does not seem to use in_/out_ functions for the BDs. Works just
>> fine that too it seems.
>
> It does now that it has explicit barriers in a few places. =C2=A0Before t=
hey were
> added, it would sometimes fail under load. =C2=A0That was due to a compil=
er
> reordering, but CPU reordering was possible as well.

I noticed that in gianfar these memory access is not protected by
"volatile".  Can this be the reason why the compiler did some unwanted
optimization?

- Leo