[PATCH 1/2] ucc_geth: Move freeing of TX packets to NAPI context.

[PATCH 1/2] ucc_geth: Move freeing of TX packets to NAPI context.

[Joakim Tjernlund]

Also set NAPI weight to 64 as this is a common value.
This will make the system alot more responsive while
ping flooding the ucc_geth ethernet interaface.

Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
---
 drivers/net/ucc_geth.c |   32 ++++++++++++--------------------
 drivers/net/ucc_geth.h |    1 -
 2 files changed, 12 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ucc_geth.c b/drivers/net/ucc_geth.c
index 097aed8..7fc91aa 100644
--- a/drivers/net/ucc_geth.c
+++ b/drivers/net/ucc_geth.c
@@ -3214,7 +3214,7 @@ static int ucc_geth_tx(struct net_device *dev, u8 txQ)
 		dev->stats.tx_packets++;
 
 		/* Free the sk buffer associated with this TxBD */
-		dev_kfree_skb_irq(ugeth->
+		dev_kfree_skb(ugeth->
 				  tx_skbuff[txQ][ugeth->skb_dirtytx[txQ]]);
 		ugeth->tx_skbuff[txQ][ugeth->skb_dirtytx[txQ]] = NULL;
 		ugeth->skb_dirtytx[txQ] =
@@ -3248,9 +3248,16 @@ static int ucc_geth_poll(struct napi_struct *napi, int budget)
 	for (i = 0; i < ug_info->numQueuesRx; i++)
 		howmany += ucc_geth_rx(ugeth, i, budget - howmany);
 
+	/* Tx event processing */
+	spin_lock(&ugeth->lock);
+	for (i = 0; i < ug_info->numQueuesTx; i++) {
+		ucc_geth_tx(ugeth->dev, i);
+	}
+	spin_unlock(&ugeth->lock);
+
 	if (howmany < budget) {
 		netif_rx_complete(napi);
-		setbits32(ugeth->uccf->p_uccm, UCCE_RX_EVENTS);
+		setbits32(ugeth->uccf->p_uccm, UCCE_RX_EVENTS | UCCE_TX_EVENTS);
 	}
 
 	return howmany;
@@ -3264,8 +3271,6 @@ static irqreturn_t ucc_geth_irq_handler(int irq, void *info)
 	struct ucc_geth_info *ug_info;
 	register u32 ucce;
 	register u32 uccm;
-	register u32 tx_mask;
-	u8 i;
 
 	ugeth_vdbg("%s: IN", __func__);
 
@@ -3279,27 +3284,14 @@ static irqreturn_t ucc_geth_irq_handler(int irq, void *info)
 	out_be32(uccf->p_ucce, ucce);
 
 	/* check for receive events that require processing */
-	if (ucce & UCCE_RX_EVENTS) {
+	if (ucce & (UCCE_RX_EVENTS | UCCE_TX_EVENTS)) {
 		if (netif_rx_schedule_prep(&ugeth->napi)) {
-			uccm &= ~UCCE_RX_EVENTS;
+			uccm &= ~(UCCE_RX_EVENTS | UCCE_TX_EVENTS);
 			out_be32(uccf->p_uccm, uccm);
 			__netif_rx_schedule(&ugeth->napi);
 		}
 	}
 
-	/* Tx event processing */
-	if (ucce & UCCE_TX_EVENTS) {
-		spin_lock(&ugeth->lock);
-		tx_mask = UCC_GETH_UCCE_TXB0;
-		for (i = 0; i < ug_info->numQueuesTx; i++) {
-			if (ucce & tx_mask)
-				ucc_geth_tx(dev, i);
-			ucce &= ~tx_mask;
-			tx_mask <<= 1;
-		}
-		spin_unlock(&ugeth->lock);
-	}
-
 	/* Errors and other events */
 	if (ucce & UCCE_OTHER) {
 		if (ucce & UCC_GETH_UCCE_BSY)
@@ -3733,7 +3725,7 @@ static int ucc_geth_probe(struct of_device* ofdev, const struct of_device_id *ma
 	dev->netdev_ops = &ucc_geth_netdev_ops;
 	dev->watchdog_timeo = TX_TIMEOUT;
 	INIT_WORK(&ugeth->timeout_work, ucc_geth_timeout_work);
-	netif_napi_add(dev, &ugeth->napi, ucc_geth_poll, UCC_GETH_DEV_WEIGHT);
+	netif_napi_add(dev, &ugeth->napi, ucc_geth_poll, 64);
 	dev->mtu = 1500;
 
 	ugeth->msg_enable = netif_msg_init(debug.msg_enable, UGETH_MSG_DEFAULT);
diff --git a/drivers/net/ucc_geth.h b/drivers/net/ucc_geth.h
index 44218b8..50bad53 100644
--- a/drivers/net/ucc_geth.h
+++ b/drivers/net/ucc_geth.h
@@ -843,7 +843,6 @@ struct ucc_geth_hardware_statistics {
 /* Driver definitions */
 #define TX_BD_RING_LEN                          0x10
 #define RX_BD_RING_LEN                          0x10
-#define UCC_GETH_DEV_WEIGHT                     TX_BD_RING_LEN
 
 #define TX_RING_MOD_MASK(size)                  (size-1)
 #define RX_RING_MOD_MASK(size)                  (size-1)
-- 
1.6.1.3

[PATCH 2/2] 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.

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;
 
 	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;
 }
 
-- 
1.6.1.3

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

[Anton Vorontsov]

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,

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

Re: [PATCH 1/2] ucc_geth: Move freeing of TX packets to NAPI context.

[Eric Dumazet]

Joakim Tjernlund a =E9crit :
> Also set NAPI weight to 64 as this is a common value.
> This will make the system alot more responsive while
> ping flooding the ucc_geth ethernet interaface.
>=20
> Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> ---
>  drivers/net/ucc_geth.c |   32 ++++++++++++--------------------
>  drivers/net/ucc_geth.h |    1 -
>  2 files changed, 12 insertions(+), 21 deletions(-)
>=20
> diff --git a/drivers/net/ucc_geth.c b/drivers/net/ucc_geth.c
> index 097aed8..7fc91aa 100644
> --- a/drivers/net/ucc_geth.c
> +++ b/drivers/net/ucc_geth.c
> @@ -3214,7 +3214,7 @@ static int ucc_geth_tx(struct net_device *dev, u8=
 txQ)
>  		dev->stats.tx_packets++;
> =20
>  		/* Free the sk buffer associated with this TxBD */
> -		dev_kfree_skb_irq(ugeth->
> +		dev_kfree_skb(ugeth->
>  				  tx_skbuff[txQ][ugeth->skb_dirtytx[txQ]]);
>  		ugeth->tx_skbuff[txQ][ugeth->skb_dirtytx[txQ]] =3D NULL;
>  		ugeth->skb_dirtytx[txQ] =3D
> @@ -3248,9 +3248,16 @@ static int ucc_geth_poll(struct napi_struct *nap=
i, int budget)

>  	for (i =3D 0; i < ug_info->numQueuesRx; i++)
>  		howmany +=3D ucc_geth_rx(ugeth, i, budget - howmany);

Not related to your patch, but seeing above code, I can understand
you might have a problem in flood situation : Only first queue(s) are
depleted, and last one cannot be because howmany >=3D budget.

This driver might have to remember last queue it handled at previous
call ucc_geth_poll(), so that all rxqueues have same probability to be sc=
anned.

j =3D ug->lastslot;
for (i =3D 0; ug_info->numQueuesRx; i++) {
	if (++j >=3D ug_info->numQueuesRx)
		j =3D 0;
	howmany +=3D ucc_geth_rx(ugeth, j, budget - howmany);
	if (howmany >=3D budget)
		break;
}
ug->lastslot =3D j;


> =20
> +	/* Tx event processing */
> +	spin_lock(&ugeth->lock);
> +	for (i =3D 0; i < ug_info->numQueuesTx; i++) {
> +		ucc_geth_tx(ugeth->dev, i);
> +	}
> +	spin_unlock(&ugeth->lock);
> +
>  	if (howmany < budget) {
>  		netif_rx_complete(napi);
> -		setbits32(ugeth->uccf->p_uccm, UCCE_RX_EVENTS);
> +		setbits32(ugeth->uccf->p_uccm, UCCE_RX_EVENTS | UCCE_TX_EVENTS);
>  	}
> =20
>  	return howmany;
> @@ -3264,8 +3271,6 @@ static irqreturn_t ucc_geth_irq_handler(int irq, =
void *info)
>  	struct ucc_geth_info *ug_info;
>  	register u32 ucce;
>  	register u32 uccm;
> -	register u32 tx_mask;
> -	u8 i;
> =20
>  	ugeth_vdbg("%s: IN", __func__);
> =20
> @@ -3279,27 +3284,14 @@ static irqreturn_t ucc_geth_irq_handler(int irq=
, void *info)
>  	out_be32(uccf->p_ucce, ucce);
> =20
>  	/* check for receive events that require processing */
> -	if (ucce & UCCE_RX_EVENTS) {
> +	if (ucce & (UCCE_RX_EVENTS | UCCE_TX_EVENTS)) {
>  		if (netif_rx_schedule_prep(&ugeth->napi)) {
> -			uccm &=3D ~UCCE_RX_EVENTS;
> +			uccm &=3D ~(UCCE_RX_EVENTS | UCCE_TX_EVENTS);
>  			out_be32(uccf->p_uccm, uccm);
>  			__netif_rx_schedule(&ugeth->napi);
>  		}
>  	}
> =20
> -	/* Tx event processing */
> -	if (ucce & UCCE_TX_EVENTS) {
> -		spin_lock(&ugeth->lock);
> -		tx_mask =3D UCC_GETH_UCCE_TXB0;
> -		for (i =3D 0; i < ug_info->numQueuesTx; i++) {
> -			if (ucce & tx_mask)
> -				ucc_geth_tx(dev, i);
> -			ucce &=3D ~tx_mask;
> -			tx_mask <<=3D 1;
> -		}
> -		spin_unlock(&ugeth->lock);
> -	}
> -
>  	/* Errors and other events */
>  	if (ucce & UCCE_OTHER) {
>  		if (ucce & UCC_GETH_UCCE_BSY)
> @@ -3733,7 +3725,7 @@ static int ucc_geth_probe(struct of_device* ofdev=
, const struct of_device_id *ma
>  	dev->netdev_ops =3D &ucc_geth_netdev_ops;
>  	dev->watchdog_timeo =3D TX_TIMEOUT;
>  	INIT_WORK(&ugeth->timeout_work, ucc_geth_timeout_work);
> -	netif_napi_add(dev, &ugeth->napi, ucc_geth_poll, UCC_GETH_DEV_WEIGHT)=
;
> +	netif_napi_add(dev, &ugeth->napi, ucc_geth_poll, 64);
>  	dev->mtu =3D 1500;
> =20
>  	ugeth->msg_enable =3D netif_msg_init(debug.msg_enable, UGETH_MSG_DEFA=
ULT);
> diff --git a/drivers/net/ucc_geth.h b/drivers/net/ucc_geth.h
> index 44218b8..50bad53 100644
> --- a/drivers/net/ucc_geth.h
> +++ b/drivers/net/ucc_geth.h
> @@ -843,7 +843,6 @@ struct ucc_geth_hardware_statistics {
>  /* Driver definitions */
>  #define TX_BD_RING_LEN                          0x10
>  #define RX_BD_RING_LEN                          0x10
> -#define UCC_GETH_DEV_WEIGHT                     TX_BD_RING_LEN
> =20
>  #define TX_RING_MOD_MASK(size)                  (size-1)
>  #define RX_RING_MOD_MASK(size)                  (size-1)

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

[Joakim Tjernlund]

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.

Re: [PATCH 1/2] ucc_geth: Move freeing of TX packets to NAPI context.

[Joakim Tjernlund]

Eric Dumazet <dada1@cosmosbay.com> wrote on 26/03/2009 15:15:25:
>=20
> Joakim Tjernlund a =E9crit :
> > Also set NAPI weight to 64 as this is a common value.
> > This will make the system alot more responsive while
> > ping flooding the ucc=5Fgeth ethernet interaface.
> >=20
> > Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> > ---
> >  drivers/net/ucc=5Fgeth.c |   32 ++++++++++++--------------------
> >  drivers/net/ucc=5Fgeth.h |    1 -
> >  2 files changed, 12 insertions(+), 21 deletions(-)
> >=20
> > diff --git a/drivers/net/ucc=5Fgeth.c b/drivers/net/ucc=5Fgeth.c
> > index 097aed8..7fc91aa 100644
> > --- a/drivers/net/ucc=5Fgeth.c
> > +++ b/drivers/net/ucc=5Fgeth.c
> > @@ -3214,7 +3214,7 @@ static int ucc=5Fgeth=5Ftx(struct net=5Fdevice *d=
ev,=20
u8 txQ)
> >        dev->stats.tx=5Fpackets++;
> >=20
> >        /* Free the sk buffer associated with this TxBD */
> > -      dev=5Fkfree=5Fskb=5Firq(ugeth->
> > +      dev=5Fkfree=5Fskb(ugeth->
> >                tx=5Fskbuff[txQ][ugeth->skb=5Fdirtytx[txQ]]);
> >        ugeth->tx=5Fskbuff[txQ][ugeth->skb=5Fdirtytx[txQ]] =3D NULL;
> >        ugeth->skb=5Fdirtytx[txQ] =3D
> > @@ -3248,9 +3248,16 @@ static int ucc=5Fgeth=5Fpoll(struct napi=5Fstruc=
t=20
*napi, int budget)
>=20
> >     for (i =3D 0; i < ug=5Finfo->numQueuesRx; i++)
> >        howmany +=3D ucc=5Fgeth=5Frx(ugeth, i, budget - howmany);
>=20
> Not related to your patch, but seeing above code, I can understand
> you might have a problem in flood situation : Only first queue(s) are
> depleted, and last one cannot be because howmany >=3D budget.
>=20
> This driver might have to remember last queue it handled at previous
> call ucc=5Fgeth=5Fpoll(), so that all rxqueues have same probability to b=
e=20
scanned.
>=20
> j =3D ug->lastslot;
> for (i =3D 0; ug=5Finfo->numQueuesRx; i++) {
>    if (++j >=3D ug=5Finfo->numQueuesRx)
>       j =3D 0;
>    howmany +=3D ucc=5Fgeth=5Frx(ugeth, j, budget - howmany);
>    if (howmany >=3D budget)
>       break;
> }
> ug->lastslot =3D j;

yes, or scan the RX queues in prio order. However ATM there is only
one queue so it won't be a problem until one extends the driver with
several queues.

Thanks,
        Jocke

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

[Anton Vorontsov]

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

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

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

[Joakim Tjernlund]

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

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

[David Miller]

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.

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

[Joakim Tjernlund]

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

Re: [PATCH 1/2] ucc_geth: Move freeing of TX packets to NAPI context.

[Li Yang]

On Thu, Mar 26, 2009 at 8:54 PM, Joakim Tjernlund
<Joakim.Tjernlund@transmode.se> wrote:
> Also set NAPI weight to 64 as this is a common value.
> This will make the system alot more responsive while
> ping flooding the ucc_geth ethernet interaface.
>
> Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> ---
> =C2=A0drivers/net/ucc_geth.c | =C2=A0 32 ++++++++++++--------------------
> =C2=A0drivers/net/ucc_geth.h | =C2=A0 =C2=A01 -
> =C2=A02 files changed, 12 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/net/ucc_geth.c b/drivers/net/ucc_geth.c
> index 097aed8..7fc91aa 100644
> --- a/drivers/net/ucc_geth.c
> +++ b/drivers/net/ucc_geth.c
> @@ -3214,7 +3214,7 @@ static int ucc_geth_tx(struct net_device *dev, u8 t=
xQ)
> =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_irq(uget=
h->
> + =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 =C2=A0tx_skbuff[txQ][ugeth->skb_dirt=
ytx[txQ]]);
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ugeth->tx_skbuff[t=
xQ][ugeth->skb_dirtytx[txQ]] =3D NULL;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ugeth->skb_dirtytx=
[txQ] =3D
> @@ -3248,9 +3248,16 @@ static int ucc_geth_poll(struct napi_struct *napi,=
 int budget)
> =C2=A0 =C2=A0 =C2=A0 =C2=A0for (i =3D 0; i < ug_info->numQueuesRx; i++)
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0howmany +=3D ucc_g=
eth_rx(ugeth, i, budget - howmany);
>
> + =C2=A0 =C2=A0 =C2=A0 /* Tx event processing */
> + =C2=A0 =C2=A0 =C2=A0 spin_lock(&ugeth->lock);
> + =C2=A0 =C2=A0 =C2=A0 for (i =3D 0; i < ug_info->numQueuesTx; i++) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ucc_geth_tx(ugeth->dev=
, i);
> + =C2=A0 =C2=A0 =C2=A0 }
> + =C2=A0 =C2=A0 =C2=A0 spin_unlock(&ugeth->lock);
> +
> =C2=A0 =C2=A0 =C2=A0 =C2=A0if (howmany < budget) {
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0netif_rx_complete(=
napi);
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 setbits32(ugeth->uccf-=
>p_uccm, UCCE_RX_EVENTS);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 setbits32(ugeth->uccf-=
>p_uccm, UCCE_RX_EVENTS | UCCE_TX_EVENTS);
> =C2=A0 =C2=A0 =C2=A0 =C2=A0}
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0return howmany;
> @@ -3264,8 +3271,6 @@ static irqreturn_t ucc_geth_irq_handler(int irq, vo=
id *info)
> =C2=A0 =C2=A0 =C2=A0 =C2=A0struct ucc_geth_info *ug_info;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0register u32 ucce;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0register u32 uccm;
> - =C2=A0 =C2=A0 =C2=A0 register u32 tx_mask;
> - =C2=A0 =C2=A0 =C2=A0 u8 i;
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0ugeth_vdbg("%s: IN", __func__);
>
> @@ -3279,27 +3284,14 @@ static irqreturn_t ucc_geth_irq_handler(int irq, =
void *info)
> =C2=A0 =C2=A0 =C2=A0 =C2=A0out_be32(uccf->p_ucce, ucce);
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0/* check for receive events that require proce=
ssing */
> - =C2=A0 =C2=A0 =C2=A0 if (ucce & UCCE_RX_EVENTS) {
> + =C2=A0 =C2=A0 =C2=A0 if (ucce & (UCCE_RX_EVENTS | UCCE_TX_EVENTS)) {
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (netif_rx_sched=
ule_prep(&ugeth->napi)) {
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 uccm &=3D ~UCCE_RX_EVENTS;
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 uccm &=3D ~(UCCE_RX_EVENTS | UCCE_TX_EVENTS);
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0out_be32(uccf->p_uccm, uccm);
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0__netif_rx_schedule(&ugeth->napi);
> =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 event processing */
> - =C2=A0 =C2=A0 =C2=A0 if (ucce & UCCE_TX_EVENTS) {
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 spin_lock(&ugeth->lock=
);
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 tx_mask =3D UCC_GETH_U=
CCE_TXB0;
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 for (i =3D 0; i < ug_i=
nfo->numQueuesTx; i++) {
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 if (ucce & tx_mask)
> - =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 ucc_geth_tx(dev, i);
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 ucce &=3D ~tx_mask;
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 tx_mask <<=3D 1;
> - =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 spin_unlock(&ugeth->lo=
ck);
> - =C2=A0 =C2=A0 =C2=A0 }
> -
> =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Errors and other events */
> =C2=A0 =C2=A0 =C2=A0 =C2=A0if (ucce & UCCE_OTHER) {
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (ucce & UCC_GET=
H_UCCE_BSY)
> @@ -3733,7 +3725,7 @@ static int ucc_geth_probe(struct of_device* ofdev, =
const struct of_device_id *ma
> =C2=A0 =C2=A0 =C2=A0 =C2=A0dev->netdev_ops =3D &ucc_geth_netdev_ops;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0dev->watchdog_timeo =3D TX_TIMEOUT;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0INIT_WORK(&ugeth->timeout_work, ucc_geth_timeo=
ut_work);
> - =C2=A0 =C2=A0 =C2=A0 netif_napi_add(dev, &ugeth->napi, ucc_geth_poll, U=
CC_GETH_DEV_WEIGHT);
> + =C2=A0 =C2=A0 =C2=A0 netif_napi_add(dev, &ugeth->napi, ucc_geth_poll, 6=
4);

It doesn't make sense to have larger napi budget than the size of RX
BD ring.  You can't have more BDs than RX_BD_RING_LEN in backlog for
napi_poll to process.  Increase the RX_BD_RING_LEN if you want to
increase UCC_GETH_DEV_WEIGHT.  However please also provide the
performance comparison for this kind of change.  Thanks

- Leo

Re: [PATCH 1/2] ucc_geth: Move freeing of TX packets to NAPI context.

[Joakim Tjernlund]

pku.leo@gmail.com wrote on 27/03/2009 11:50:09:
> 
> On Thu, Mar 26, 2009 at 8:54 PM, Joakim Tjernlund
> <Joakim.Tjernlund@transmode.se> wrote:
> > Also set NAPI weight to 64 as this is a common value.
> > This will make the system alot more responsive while
> > ping flooding the ucc_geth ethernet interaface.
> >
> > Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> > ---
> >        /* Errors and other events */
> >        if (ucce & UCCE_OTHER) {
> >                if (ucce & UCC_GETH_UCCE_BSY)
> > @@ -3733,7 +3725,7 @@ static int ucc_geth_probe(struct of_device* 
ofdev, const struct of_device_id *ma
> >        dev->netdev_ops = &ucc_geth_netdev_ops;
> >        dev->watchdog_timeo = TX_TIMEOUT;
> >        INIT_WORK(&ugeth->timeout_work, ucc_geth_timeout_work);
> > -       netif_napi_add(dev, &ugeth->napi, ucc_geth_poll, 
UCC_GETH_DEV_WEIGHT);
> > +       netif_napi_add(dev, &ugeth->napi, ucc_geth_poll, 64);
> 
> It doesn't make sense to have larger napi budget than the size of RX
> BD ring.  You can't have more BDs than RX_BD_RING_LEN in backlog for
> napi_poll to process.  Increase the RX_BD_RING_LEN if you want to
> increase UCC_GETH_DEV_WEIGHT.  However please also provide the
> performance comparison for this kind of change.  Thanks

Bring it up with David Miller. After my initial attempt to just increase
weight somewhat, he requested that I hardcoded it to 64. Just read the 
whole thread.
If I don't increase weight somewhat, ping -f -l 3 almost halts the board. 
Logging
in takes forever. These are my "performance numbers".

weight theory:
Before the drivers gets to the end of a full BD ring, new pkgs arrives so 
that
even if the DB ring is only 16, the driver wants to process 17 or more 
pkgs.

Re: [PATCH 1/2] ucc_geth: Move freeing of TX packets to NAPI context.

[David Miller]

From: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
Date: Fri, 27 Mar 2009 12:52:41 +0100

> pku.leo@gmail.com wrote on 27/03/2009 11:50:09:
> > It doesn't make sense to have larger napi budget than the size of RX
> > BD ring.  You can't have more BDs than RX_BD_RING_LEN in backlog for
> > napi_poll to process.  Increase the RX_BD_RING_LEN if you want to
> > increase UCC_GETH_DEV_WEIGHT.  However please also provide the
> > performance comparison for this kind of change.  Thanks
> 
> Bring it up with David Miller.

Right I told him to make this very change.

Pretty soon the driver's won't be able to select this value
and it will default to 64 everwhere anyways.

"performance comparison"?  He can't do that unless he tests
performance with two active NAPI devices on the same exact
cpu as the weight value is for fairness between devices.

So you don't tweak "weight" to make one device perform better,
you tweak it to eliminate unfairness between multiple devices.
And the only way to achieve that is to use similar values
across all devices, perhaps link-speed rated which we can't
do just yet, and that's why I told him to use 64 just like
every other driver basically does.

Re: [PATCH 1/2] ucc_geth: Move freeing of TX packets to NAPI context.

[Li Yang]

On Fri, Mar 27, 2009 at 7:52 PM, Joakim Tjernlund
<Joakim.Tjernlund@transmode.se> wrote:
> pku.leo@gmail.com wrote on 27/03/2009 11:50:09:
>>
>> On Thu, Mar 26, 2009 at 8:54 PM, Joakim Tjernlund
>> <Joakim.Tjernlund@transmode.se> wrote:
>> > Also set NAPI weight to 64 as this is a common value.
>> > This will make the system alot more responsive while
>> > ping flooding the ucc_geth ethernet interaface.
>> >
>> > Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
>> > ---
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Errors and other events */
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0if (ucce & UCCE_OTHER) {
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (ucce & UCC_=
GETH_UCCE_BSY)
>> > @@ -3733,7 +3725,7 @@ static int ucc_geth_probe(struct of_device*
> ofdev, const struct of_device_id *ma
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0dev->netdev_ops =3D &ucc_geth_netdev_ops;
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0dev->watchdog_timeo =3D TX_TIMEOUT;
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0INIT_WORK(&ugeth->timeout_work, ucc_geth_ti=
meout_work);
>> > - =C2=A0 =C2=A0 =C2=A0 netif_napi_add(dev, &ugeth->napi, ucc_geth_poll=
,
> UCC_GETH_DEV_WEIGHT);
>> > + =C2=A0 =C2=A0 =C2=A0 netif_napi_add(dev, &ugeth->napi, ucc_geth_poll=
, 64);
>>
>> It doesn't make sense to have larger napi budget than the size of RX
>> BD ring. =C2=A0You can't have more BDs than RX_BD_RING_LEN in backlog fo=
r
>> napi_poll to process. =C2=A0Increase the RX_BD_RING_LEN if you want to
>> increase UCC_GETH_DEV_WEIGHT. =C2=A0However please also provide the
>> performance comparison for this kind of change. =C2=A0Thanks
>
> Bring it up with David Miller. After my initial attempt to just increase
> weight somewhat, he requested that I hardcoded it to 64. Just read the
> whole thread.
> If I don't increase weight somewhat, ping -f -l 3 almost halts the board.
> Logging
> in takes forever. These are my "performance numbers".

Faster response time is surely good.  But it might also mean CPU is
not fully loaded.  IMHO, throughput is a more important factor for
network devices.  When you try to optimize the driver, please also
consider the throughput change.  Thanks.

- Leo

Re: [PATCH 1/2] ucc_geth: Move freeing of TX packets to NAPI context.

[Joakim Tjernlund]

pku.leo@gmail.com wrote on 30/03/2009 09:36:21:
> 
> On Fri, Mar 27, 2009 at 7:52 PM, Joakim Tjernlund
> <Joakim.Tjernlund@transmode.se> wrote:
> > pku.leo@gmail.com wrote on 27/03/2009 11:50:09:
> >>
> >> On Thu, Mar 26, 2009 at 8:54 PM, Joakim Tjernlund
> >> <Joakim.Tjernlund@transmode.se> wrote:
> >> > Also set NAPI weight to 64 as this is a common value.
> >> > This will make the system alot more responsive while
> >> > ping flooding the ucc_geth ethernet interaface.
> >> >
> >> > Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> >> > ---
> >> >        /* Errors and other events */
> >> >        if (ucce & UCCE_OTHER) {
> >> >                if (ucce & UCC_GETH_UCCE_BSY)
> >> > @@ -3733,7 +3725,7 @@ static int ucc_geth_probe(struct of_device*
> > ofdev, const struct of_device_id *ma
> >> >        dev->netdev_ops = &ucc_geth_netdev_ops;
> >> >        dev->watchdog_timeo = TX_TIMEOUT;
> >> >        INIT_WORK(&ugeth->timeout_work, ucc_geth_timeout_work);
> >> > -       netif_napi_add(dev, &ugeth->napi, ucc_geth_poll,
> > UCC_GETH_DEV_WEIGHT);
> >> > +       netif_napi_add(dev, &ugeth->napi, ucc_geth_poll, 64);
> >>
> >> It doesn't make sense to have larger napi budget than the size of RX
> >> BD ring.  You can't have more BDs than RX_BD_RING_LEN in backlog for
> >> napi_poll to process.  Increase the RX_BD_RING_LEN if you want to
> >> increase UCC_GETH_DEV_WEIGHT.  However please also provide the
> >> performance comparison for this kind of change.  Thanks
> >
> > Bring it up with David Miller. After my initial attempt to just 
increase
> > weight somewhat, he requested that I hardcoded it to 64. Just read the
> > whole thread.
> > If I don't increase weight somewhat, ping -f -l 3 almost halts the 
board.
> > Logging
> > in takes forever. These are my "performance numbers".
> 
> Faster response time is surely good.  But it might also mean CPU is
> not fully loaded.  IMHO, throughput is a more important factor for
> network devices.  When you try to optimize the driver, please also
> consider the throughput change.  Thanks.

This particular change isn't about performance, it is about not
"bricking" the board during heavy traffic. Next step is to optimize
the driver.

 Jocke

Re: [PATCH 1/2] ucc_geth: Move freeing of TX packets to NAPI context.

[Li Yang]

On Mon, Mar 30, 2009 at 3:48 PM, Joakim Tjernlund
<Joakim.Tjernlund@transmode.se> wrote:
> pku.leo@gmail.com wrote on 30/03/2009 09:36:21:
>>
>> On Fri, Mar 27, 2009 at 7:52 PM, Joakim Tjernlund
>> <Joakim.Tjernlund@transmode.se> wrote:
>> > pku.leo@gmail.com wrote on 27/03/2009 11:50:09:
>> >>
>> >> On Thu, Mar 26, 2009 at 8:54 PM, Joakim Tjernlund
>> >> <Joakim.Tjernlund@transmode.se> wrote:
>> >> > Also set NAPI weight to 64 as this is a common value.
>> >> > This will make the system alot more responsive while
>> >> > ping flooding the ucc_geth ethernet interaface.
>> >> >
>> >> > Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
>> >> > ---
>> >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Errors and other events */
>> >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0if (ucce & UCCE_OTHER) {
>> >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (ucce & U=
CC_GETH_UCCE_BSY)
>> >> > @@ -3733,7 +3725,7 @@ static int ucc_geth_probe(struct of_device*
>> > ofdev, const struct of_device_id *ma
>> >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0dev->netdev_ops =3D &ucc_geth_netdev_ops=
;
>> >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0dev->watchdog_timeo =3D TX_TIMEOUT;
>> >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0INIT_WORK(&ugeth->timeout_work, ucc_geth=
_timeout_work);
>> >> > - =C2=A0 =C2=A0 =C2=A0 netif_napi_add(dev, &ugeth->napi, ucc_geth_p=
oll,
>> > UCC_GETH_DEV_WEIGHT);
>> >> > + =C2=A0 =C2=A0 =C2=A0 netif_napi_add(dev, &ugeth->napi, ucc_geth_p=
oll, 64);
>> >>
>> >> It doesn't make sense to have larger napi budget than the size of RX
>> >> BD ring. =C2=A0You can't have more BDs than RX_BD_RING_LEN in backlog=
 for
>> >> napi_poll to process. =C2=A0Increase the RX_BD_RING_LEN if you want t=
o
>> >> increase UCC_GETH_DEV_WEIGHT. =C2=A0However please also provide the
>> >> performance comparison for this kind of change. =C2=A0Thanks
>> >
>> > Bring it up with David Miller. After my initial attempt to just
> increase
>> > weight somewhat, he requested that I hardcoded it to 64. Just read the
>> > whole thread.
>> > If I don't increase weight somewhat, ping -f -l 3 almost halts the
> board.
>> > Logging
>> > in takes forever. These are my "performance numbers".
>>
>> Faster response time is surely good. =C2=A0But it might also mean CPU is
>> not fully loaded. =C2=A0IMHO, throughput is a more important factor for
>> network devices. =C2=A0When you try to optimize the driver, please also
>> consider the throughput change. =C2=A0Thanks.
>
> This particular change isn't about performance, it is about not
> "bricking" the board during heavy traffic. Next step is to optimize
> the driver.

Sure.  I mean for other changes like tx NAPI, ring size tweak and tx logic.

- Leo