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

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

[Joakim Tjernlund]

>From 1c2f23b1f37f4818c0fd0217b93eb38ab6564840 Mon Sep 17 00:00:00 2001
From: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
Date: Tue, 24 Mar 2009 10:19:27 +0100
Subject: [PATCH] ucc_geth: Move freeing of TX packets to NAPI context.
 Also increase NAPI weight somewhat.
 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 |   30 +++++++++++-------------------
 drivers/net/ucc_geth.h |    2 +-
 2 files changed, 12 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ucc_geth.c b/drivers/net/ucc_geth.c
index 097aed8..7d5d110 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)
diff --git a/drivers/net/ucc_geth.h b/drivers/net/ucc_geth.h
index 44218b8..ea30aa7 100644
--- a/drivers/net/ucc_geth.h
+++ b/drivers/net/ucc_geth.h
@@ -843,7 +843,7 @@ 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 UCC_GETH_DEV_WEIGHT                     (RX_BD_RING_LEN+TX_BD_RING_LEN/2)
 
 #define TX_RING_MOD_MASK(size)                  (size-1)
 #define RX_RING_MOD_MASK(size)                  (size-1)
-- 
1.6.1.3

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

[Eric Dumazet]

Joakim Tjernlund a =E9crit :
>>From 1c2f23b1f37f4818c0fd0217b93eb38ab6564840 Mon Sep 17 00:00:00 2001
> From: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> Date: Tue, 24 Mar 2009 10:19:27 +0100
> Subject: [PATCH] ucc_geth: Move freeing of TX packets to NAPI context.
>  Also increase NAPI weight somewhat.
>  This will make the system alot more responsive while
>  ping flooding the ucc_geth ethernet interaface.
>=20
>=20
> Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> ---
>  drivers/net/ucc_geth.c |   30 +++++++++++-------------------
>  drivers/net/ucc_geth.h |    2 +-
>  2 files changed, 12 insertions(+), 20 deletions(-)
>=20
> diff --git a/drivers/net/ucc_geth.c b/drivers/net/ucc_geth.c
> index 097aed8..7d5d110 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);
> =20

Cant you test (ucce & UCCE_TX_EVENTS) or something here to avoid
taking lock and checking queues if not necessary ?

> +	/* 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);
> +

Why tx completions dont change "howmany" ?
It seems strange you changed UCC_GETH_DEV_WEIGHT if not taking into accou=
nt tx event above...


>  	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)
> diff --git a/drivers/net/ucc_geth.h b/drivers/net/ucc_geth.h
> index 44218b8..ea30aa7 100644
> --- a/drivers/net/ucc_geth.h
> +++ b/drivers/net/ucc_geth.h
> @@ -843,7 +843,7 @@ 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 UCC_GETH_DEV_WEIGHT                     (RX_BD_RING_LEN+TX_BD_=
RING_LEN/2)
> =20
>  #define TX_RING_MOD_MASK(size)                  (size-1)
>  #define RX_RING_MOD_MASK(size)                  (size-1)

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

[Joakim Tjernlund]

Eric Dumazet <dada1@cosmosbay.com> wrote on 25/03/2009 15:04:26:
> Joakim Tjernlund a =E9crit :
> >>From 1c2f23b1f37f4818c0fd0217b93eb38ab6564840 Mon Sep 17 00:00:00 2001
> > From: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> > Date: Tue, 24 Mar 2009 10:19:27 +0100
> > Subject: [PATCH] ucc=5Fgeth: Move freeing of TX packets to NAPI context.
> >  Also increase NAPI weight somewhat.
> >  This will make the system alot more responsive while
> >  ping flooding the ucc=5Fgeth ethernet interaface.
> >=20
> >=20
> > Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> > ---
> >  drivers/net/ucc=5Fgeth.c |   30 +++++++++++-------------------
> >  drivers/net/ucc=5Fgeth.h |    2 +-
> >  2 files changed, 12 insertions(+), 20 deletions(-)
> >=20
> > diff --git a/drivers/net/ucc=5Fgeth.c b/drivers/net/ucc=5Fgeth.c
> > index 097aed8..7d5d110 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)
> >     for (i =3D 0; i < ug=5Finfo->numQueuesRx; i++)
> >        howmany +=3D ucc=5Fgeth=5Frx(ugeth, i, budget - howmany);
> >=20
>=20
> Cant you test (ucce & UCCE=5FTX=5FEVENTS) or something here to avoid
> taking lock and checking queues if not necessary ?

Probably, but I want this patch as simple as possible. There
are lots of optimizations left to do in this driver.

>=20
> > +   /* Tx event processing */
> > +   spin=5Flock(&ugeth->lock);
> > +   for (i =3D 0; i < ug=5Finfo->numQueuesTx; i++) {
> > +      ucc=5Fgeth=5Ftx(ugeth->dev, i);
> > +   }
> > +   spin=5Funlock(&ugeth->lock);
> > +
>=20
> Why tx completions dont change "howmany" ?
> It seems strange you changed UCC=5FGETH=5FDEV=5FWEIGHT if not taking into=
=20
account tx event above...

This is unclear and last I checked not very common amongst other drivers=20
in the
tree.

UCC=5FGETH=5FDEV=5FWEIGHT needs to be a bit bigger than the number of RX HW=
=20
buffers avaliable,
otherwise one won't be able to drain the whole queue in one go. Changing
weight to something bigger made a big difference.

 Jocke

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

[David Miller]

From: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
Date: Wed, 25 Mar 2009 16:16:24 +0100

> UCC_GETH_DEV_WEIGHT needs to be a bit bigger than the number of RX
> HW buffers avaliable, otherwise one won't be able to drain the whole
> queue in one go. Changing weight to something bigger made a big
> difference.

You're not supposed to "drain the whole queue in one go", that
is not the goal of the weight value.

The goal of the weight value is that it is low enough such that
other devices also scheduled for NAPI on the current processor
can get some fair time to process packets too.

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

[David Miller]

From: Eric Dumazet <dada1@cosmosbay.com>
Date: Wed, 25 Mar 2009 15:04:26 +0100

> Joakim Tjernlund a =E9crit :
> > +	/* 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);
> > +
> =

> Why tx completions dont change "howmany" ?
> It seems strange you changed UCC_GETH_DEV_WEIGHT if not taking into a=
ccount tx event above...

He should leave howmany alone for TX work and use a weight
value of 64 just like most other drivers in the tree do.

Due to the abuse and random ignorant fiddling of the
weight value, I am going to make it something the core
rather than drivers choose.

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

[Joakim Tjernlund]

David Miller <davem@davemloft.net> wrote on 25/03/2009 22:40:41:
>=20
> From: Eric Dumazet <dada1@cosmosbay.com>
> Date: Wed, 25 Mar 2009 15:04:26 +0100
>=20
> > Joakim Tjernlund a =E9crit :
> > > +   /* Tx event processing */
> > > +   spin=5Flock(&ugeth->lock);
> > > +   for (i =3D 0; i < ug=5Finfo->numQueuesTx; i++) {
> > > +      ucc=5Fgeth=5Ftx(ugeth->dev, i);
> > > +   }
> > > +   spin=5Funlock(&ugeth->lock);
> > > +
> >=20
> > Why tx completions dont change "howmany" ?
> > It seems strange you changed UCC=5FGETH=5FDEV=5FWEIGHT if not taking in=
to=20
account tx event above...
>=20
> He should leave howmany alone for TX work and use a weight
> value of 64 just like most other drivers in the tree do.

OK, will change this to 64.

>=20
> Due to the abuse and random ignorant fiddling of the
> weight value, I am going to make it something the core
> rather than drivers choose.

Good, I had a hard time finding info how one should use it anyway.=20

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

[Anton Vorontsov]

On Wed, Mar 25, 2009 at 02:30:49PM +0100, Joakim Tjernlund wrote:
> >>From 1c2f23b1f37f4818c0fd0217b93eb38ab6564840 Mon Sep 17 00:00:00 2001
> From: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> Date: Tue, 24 Mar 2009 10:19:27 +0100
> Subject: [PATCH] ucc_geth: Move freeing of TX packets to NAPI context.
>  Also increase NAPI weight somewhat.
>  This will make the system alot more responsive while
>  ping flooding the ucc_geth ethernet interaface.

Some time ago I've tried a similar thing for this driver, but during
tcp (or udp I don't quite remember) netperf tests I was getting tx
watchdog timeouts after ~2-5 minutes of work. I was testing with a
gigabit and 100 Mbit link, with 100 Mbit link the issue was not
reproducible.

Though, I recalling I was doing a bit more than your patch: I was
also clearing the TX events in the ucce register before calling
ucc_geth_tx, that way I was trying to avoid stale interrupts. That
helped to increase an overall performance (not only responsiveness),
but as I said my approach didn't pass the tests.

I don't really think that your patch may cause this, but can you
try netperf w/ this patch applied anyway? And see if it really
doesn't cause any issues under stress?

Thanks,

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

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

[Joakim Tjernlund]

Anton Vorontsov <avorontsov@ru.mvista.com> wrote on 25/03/2009 15:25:40:
> 
> On Wed, Mar 25, 2009 at 02:30:49PM +0100, Joakim Tjernlund wrote:
> > >>From 1c2f23b1f37f4818c0fd0217b93eb38ab6564840 Mon Sep 17 00:00:00 
2001
> > From: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> > Date: Tue, 24 Mar 2009 10:19:27 +0100
> > Subject: [PATCH] ucc_geth: Move freeing of TX packets to NAPI context.
> >  Also increase NAPI weight somewhat.
> >  This will make the system alot more responsive while
> >  ping flooding the ucc_geth ethernet interaface.
> 
> Some time ago I've tried a similar thing for this driver, but during
> tcp (or udp I don't quite remember) netperf tests I was getting tx
> watchdog timeouts after ~2-5 minutes of work. I was testing with a
> gigabit and 100 Mbit link, with 100 Mbit link the issue was not
> reproducible.
> 
> Though, I recalling I was doing a bit more than your patch: I was
> also clearing the TX events in the ucce register before calling
> ucc_geth_tx, that way I was trying to avoid stale interrupts. That

Sure, but that is another patch I think.

> helped to increase an overall performance (not only responsiveness),
> but as I said my approach didn't pass the tests.
> 
> I don't really think that your patch may cause this, but can you
> try netperf w/ this patch applied anyway? And see if it really
> doesn't cause any issues under stress?

Ran this on my host against my target board:
 netperf -t UDP_RR  -H 192.168.1.16
 netperf -t UDP_STREAM -H 192.168.1.16
 netperf -t TCP_STREAM -H 192.168.1.16
 netperf -t TCP_SENDFILE -H 192.168.1.16
 netperf -t TCP_MAERTS -H 192.168.1.16
 netperf -t TCP_RR -H 192.168.1.16
 netperf -t TCP_CRR -H 192.168.1.16

Didn't notice any timeouts, but I only have 100Mbit interfaces.

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

[Joakim Tjernlund]

Anton Vorontsov <avorontsov@ru.mvista.com> wrote on 25/03/2009 15:25:40:
> On Wed, Mar 25, 2009 at 02:30:49PM +0100, Joakim Tjernlund wrote:
> > >>From 1c2f23b1f37f4818c0fd0217b93eb38ab6564840 Mon Sep 17 00:00:00 
2001
> > From: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> > Date: Tue, 24 Mar 2009 10:19:27 +0100
> > Subject: [PATCH] ucc_geth: Move freeing of TX packets to NAPI context.
> >  Also increase NAPI weight somewhat.
> >  This will make the system alot more responsive while
> >  ping flooding the ucc_geth ethernet interaface.
> 
> Some time ago I've tried a similar thing for this driver, but during
> tcp (or udp I don't quite remember) netperf tests I was getting tx
> watchdog timeouts after ~2-5 minutes of work. I was testing with a
> gigabit and 100 Mbit link, with 100 Mbit link the issue was not
> reproducible.
> 
> Though, I recalling I was doing a bit more than your patch: I was
> also clearing the TX events in the ucce register before calling
> ucc_geth_tx, that way I was trying to avoid stale interrupts. That
> helped to increase an overall performance (not only responsiveness),
> but as I said my approach didn't pass the tests.
> 
> I don't really think that your patch may cause this, but can you
> try netperf w/ this patch applied anyway? And see if it really
> doesn't cause any issues under stress?

Does the line(in ucc_geth_tx()) look OK to you:
        if ((bd == ugeth->txBd[txQ]) && (netif_queue_stopped(dev) == 0))
                        break;

Sure does look fishy to me.

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

[Li Yang]

On Thu, Mar 26, 2009 at 1:51 AM, Joakim Tjernlund
<Joakim.Tjernlund@transmode.se> wrote:
> Anton Vorontsov <avorontsov@ru.mvista.com> wrote on 25/03/2009 15:25:40:
>> On Wed, Mar 25, 2009 at 02:30:49PM +0100, Joakim Tjernlund wrote:
>> > >>From 1c2f23b1f37f4818c0fd0217b93eb38ab6564840 Mon Sep 17 00:00:00
> 2001
>> > From: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
>> > Date: Tue, 24 Mar 2009 10:19:27 +0100
>> > Subject: [PATCH] ucc_geth: Move freeing of TX packets to NAPI context.
>> > =C2=A0Also increase NAPI weight somewhat.
>> > =C2=A0This will make the system alot more responsive while
>> > =C2=A0ping flooding the ucc_geth ethernet interaface.
>>
>> Some time ago I've tried a similar thing for this driver, but during
>> tcp (or udp I don't quite remember) netperf tests I was getting tx
>> watchdog timeouts after ~2-5 minutes of work. I was testing with a
>> gigabit and 100 Mbit link, with 100 Mbit link the issue was not
>> reproducible.
>>
>> Though, I recalling I was doing a bit more than your patch: I was
>> also clearing the TX events in the ucce register before calling
>> ucc_geth_tx, that way I was trying to avoid stale interrupts. That
>> helped to increase an overall performance (not only responsiveness),
>> but as I said my approach didn't pass the tests.
>>
>> I don't really think that your patch may cause this, but can you
>> try netperf w/ this patch applied anyway? And see if it really
>> doesn't cause any issues under stress?
>
> Does the line(in ucc_geth_tx()) look OK to you:
> =C2=A0 =C2=A0 =C2=A0 =C2=A0if ((bd =3D=3D ugeth->txBd[txQ]) && (netif_que=
ue_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 =C2=A0break;
>
> Sure does look fishy to me.

There are two cases when txBd=3DConfBd: the BD ring is full or empty.
The condition used here ensures that it is the empty case.  Because in
hard_start_xmit, the queue will be stopped when the BD ring is full.
Maybe some comment is needed here.

- Leo

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

[Joakim Tjernlund]

pku.leo@gmail.com wrote on 30/03/2009 10:34:47:
> 
> On Thu, Mar 26, 2009 at 1:51 AM, Joakim Tjernlund
> <Joakim.Tjernlund@transmode.se> wrote:
> > Anton Vorontsov <avorontsov@ru.mvista.com> wrote on 25/03/2009 
15:25:40:
> >> On Wed, Mar 25, 2009 at 02:30:49PM +0100, Joakim Tjernlund wrote:
> >> > >>From 1c2f23b1f37f4818c0fd0217b93eb38ab6564840 Mon Sep 17 00:00:00
> > 2001
> >> > From: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> >> > Date: Tue, 24 Mar 2009 10:19:27 +0100
> >> > Subject: [PATCH] ucc_geth: Move freeing of TX packets to NAPI 
context.
> >> >  Also increase NAPI weight somewhat.
> >> >  This will make the system alot more responsive while
> >> >  ping flooding the ucc_geth ethernet interaface.
> >>
> >> Some time ago I've tried a similar thing for this driver, but during
> >> tcp (or udp I don't quite remember) netperf tests I was getting tx
> >> watchdog timeouts after ~2-5 minutes of work. I was testing with a
> >> gigabit and 100 Mbit link, with 100 Mbit link the issue was not
> >> reproducible.
> >>
> >> Though, I recalling I was doing a bit more than your patch: I was
> >> also clearing the TX events in the ucce register before calling
> >> ucc_geth_tx, that way I was trying to avoid stale interrupts. That
> >> helped to increase an overall performance (not only responsiveness),
> >> but as I said my approach didn't pass the tests.
> >>
> >> I don't really think that your patch may cause this, but can you
> >> try netperf w/ this patch applied anyway? And see if it really
> >> doesn't cause any issues under stress?
> >
> > Does the line(in ucc_geth_tx()) look OK to you:
> >        if ((bd == ugeth->txBd[txQ]) && (netif_queue_stopped(dev) == 
0))
> >                        break;
> >
> > Sure does look fishy to me.
> 
> There are two cases when txBd=ConfBd: the BD ring is full or empty.
> The condition used here ensures that it is the empty case.  Because in
> hard_start_xmit, the queue will be stopped when the BD ring is full.
> Maybe some comment is needed here.

But how do you know that the queue hasn't been stopped by someone else 
than
the driver? 
If it is stopped by higher layers, the if stmt will fail.

 Jocke

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

[Li Yang]

On Mon, Mar 30, 2009 at 5:21 PM, Joakim Tjernlund
<Joakim.Tjernlund@transmode.se> wrote:
> pku.leo@gmail.com wrote on 30/03/2009 10:34:47:
>>
>> On Thu, Mar 26, 2009 at 1:51 AM, Joakim Tjernlund
>> <Joakim.Tjernlund@transmode.se> wrote:
>> > Anton Vorontsov <avorontsov@ru.mvista.com> wrote on 25/03/2009
> 15:25:40:
>> >> On Wed, Mar 25, 2009 at 02:30:49PM +0100, Joakim Tjernlund wrote:
>> >> > >>From 1c2f23b1f37f4818c0fd0217b93eb38ab6564840 Mon Sep 17 00:00:00
>> > 2001
>> >> > From: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
>> >> > Date: Tue, 24 Mar 2009 10:19:27 +0100
>> >> > Subject: [PATCH] ucc_geth: Move freeing of TX packets to NAPI
> context.
>> >> > =C2=A0Also increase NAPI weight somewhat.
>> >> > =C2=A0This will make the system alot more responsive while
>> >> > =C2=A0ping flooding the ucc_geth ethernet interaface.
>> >>
>> >> Some time ago I've tried a similar thing for this driver, but during
>> >> tcp (or udp I don't quite remember) netperf tests I was getting tx
>> >> watchdog timeouts after ~2-5 minutes of work. I was testing with a
>> >> gigabit and 100 Mbit link, with 100 Mbit link the issue was not
>> >> reproducible.
>> >>
>> >> Though, I recalling I was doing a bit more than your patch: I was
>> >> also clearing the TX events in the ucce register before calling
>> >> ucc_geth_tx, that way I was trying to avoid stale interrupts. That
>> >> helped to increase an overall performance (not only responsiveness),
>> >> but as I said my approach didn't pass the tests.
>> >>
>> >> I don't really think that your patch may cause this, but can you
>> >> try netperf w/ this patch applied anyway? And see if it really
>> >> doesn't cause any issues under stress?
>> >
>> > Does the line(in ucc_geth_tx()) look OK to you:
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0if ((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 =C2=A0break;
>> >
>> > Sure does look fishy to me.
>>
>> There are two cases when txBd=3DConfBd: the BD ring is full or empty.
>> The condition used here ensures that it is the empty case. =C2=A0Because=
 in
>> hard_start_xmit, the queue will be stopped when the BD ring is full.
>> Maybe some comment is needed here.
>
> But how do you know that the queue hasn't been stopped by someone else
> than
> the driver?
> If it is stopped by higher layers, the if stmt will fail.

It looks like from existing code that only the driver can legally stop
the queue.  I'm not 100% sure though.  Correct me if I'm wrong.

- Leo

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

[Joakim Tjernlund]

pku.leo@gmail.com wrote on 30/03/2009 11:36:36:
> 
> On Mon, Mar 30, 2009 at 5:21 PM, Joakim Tjernlund
> <Joakim.Tjernlund@transmode.se> wrote:
> > pku.leo@gmail.com wrote on 30/03/2009 10:34:47:
> >>
> >> On Thu, Mar 26, 2009 at 1:51 AM, Joakim Tjernlund
> >> <Joakim.Tjernlund@transmode.se> wrote:
> >> > Anton Vorontsov <avorontsov@ru.mvista.com> wrote on 25/03/2009
> > 15:25:40:
> >> >> On Wed, Mar 25, 2009 at 02:30:49PM +0100, Joakim Tjernlund wrote:
> >> >> > >>From 1c2f23b1f37f4818c0fd0217b93eb38ab6564840 Mon Sep 17 
00:00:00
> >> > 2001
> >> >> > From: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> >> >> > Date: Tue, 24 Mar 2009 10:19:27 +0100
> >> >> > Subject: [PATCH] ucc_geth: Move freeing of TX packets to NAPI
> > context.
> >> >> >  Also increase NAPI weight somewhat.
> >> >> >  This will make the system alot more responsive while
> >> >> >  ping flooding the ucc_geth ethernet interaface.
> >> >>
> >> >> Some time ago I've tried a similar thing for this driver, but 
during
> >> >> tcp (or udp I don't quite remember) netperf tests I was getting tx
> >> >> watchdog timeouts after ~2-5 minutes of work. I was testing with a
> >> >> gigabit and 100 Mbit link, with 100 Mbit link the issue was not
> >> >> reproducible.
> >> >>
> >> >> Though, I recalling I was doing a bit more than your patch: I was
> >> >> also clearing the TX events in the ucce register before calling
> >> >> ucc_geth_tx, that way I was trying to avoid stale interrupts. That
> >> >> helped to increase an overall performance (not only 
responsiveness),
> >> >> but as I said my approach didn't pass the tests.
> >> >>
> >> >> I don't really think that your patch may cause this, but can you
> >> >> try netperf w/ this patch applied anyway? And see if it really
> >> >> doesn't cause any issues under stress?
> >> >
> >> > Does the line(in ucc_geth_tx()) look OK to you:
> >> >        if ((bd == ugeth->txBd[txQ]) && (netif_queue_stopped(dev) ==
> > 0))
> >> >                        break;
> >> >
> >> > Sure does look fishy to me.
> >>
> >> There are two cases when txBd=ConfBd: the BD ring is full or empty.
> >> The condition used here ensures that it is the empty case.  Because 
in
> >> hard_start_xmit, the queue will be stopped when the BD ring is full.
> >> Maybe some comment is needed here.
> >
> > But how do you know that the queue hasn't been stopped by someone else
> > than
> > the driver?
> > If it is stopped by higher layers, the if stmt will fail.
> 
> It looks like from existing code that only the driver can legally stop
> the queue.  I'm not 100% sure though.  Correct me if I'm wrong.

I don't know. But the question you should ask is: Does the networking
code promise this now and for the future? If not, you should
fix the driver not to relay on netif_queue_stopped() here.

 Jocke

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

[Li Yang]

On Mon, Mar 30, 2009 at 6:01 PM, Joakim Tjernlund
<Joakim.Tjernlund@transmode.se> wrote:
> pku.leo@gmail.com wrote on 30/03/2009 11:36:36:
>>
>> On Mon, Mar 30, 2009 at 5:21 PM, Joakim Tjernlund
>> <Joakim.Tjernlund@transmode.se> wrote:
>> > pku.leo@gmail.com wrote on 30/03/2009 10:34:47:
>> >>
>> >> On Thu, Mar 26, 2009 at 1:51 AM, Joakim Tjernlund
>> >> <Joakim.Tjernlund@transmode.se> wrote:
>> >> > Anton Vorontsov <avorontsov@ru.mvista.com> wrote on 25/03/2009
>> > 15:25:40:
>> >> >> On Wed, Mar 25, 2009 at 02:30:49PM +0100, Joakim Tjernlund wrote:
>> >> >> > >>From 1c2f23b1f37f4818c0fd0217b93eb38ab6564840 Mon Sep 17
> 00:00:00
>> >> > 2001
>> >> >> > From: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
>> >> >> > Date: Tue, 24 Mar 2009 10:19:27 +0100
>> >> >> > Subject: [PATCH] ucc_geth: Move freeing of TX packets to NAPI
>> > context.
>> >> >> > =C2=A0Also increase NAPI weight somewhat.
>> >> >> > =C2=A0This will make the system alot more responsive while
>> >> >> > =C2=A0ping flooding the ucc_geth ethernet interaface.
>> >> >>
>> >> >> Some time ago I've tried a similar thing for this driver, but
> during
>> >> >> tcp (or udp I don't quite remember) netperf tests I was getting tx
>> >> >> watchdog timeouts after ~2-5 minutes of work. I was testing with a
>> >> >> gigabit and 100 Mbit link, with 100 Mbit link the issue was not
>> >> >> reproducible.
>> >> >>
>> >> >> Though, I recalling I was doing a bit more than your patch: I was
>> >> >> also clearing the TX events in the ucce register before calling
>> >> >> ucc_geth_tx, that way I was trying to avoid stale interrupts. That
>> >> >> helped to increase an overall performance (not only
> responsiveness),
>> >> >> but as I said my approach didn't pass the tests.
>> >> >>
>> >> >> I don't really think that your patch may cause this, but can you
>> >> >> try netperf w/ this patch applied anyway? And see if it really
>> >> >> doesn't cause any issues under stress?
>> >> >
>> >> > Does the line(in ucc_geth_tx()) look OK to you:
>> >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0if ((bd =3D=3D ugeth->txBd[txQ]) && (net=
if_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 =C2=A0break;
>> >> >
>> >> > Sure does look fishy to me.
>> >>
>> >> There are two cases when txBd=3DConfBd: the BD ring is full or empty.
>> >> The condition used here ensures that it is the empty case. =C2=A0Beca=
use
> in
>> >> hard_start_xmit, the queue will be stopped when the BD ring is full.
>> >> Maybe some comment is needed here.
>> >
>> > But how do you know that the queue hasn't been stopped by someone else
>> > than
>> > the driver?
>> > If it is stopped by higher layers, the if stmt will fail.
>>
>> It looks like from existing code that only the driver can legally stop
>> the queue. =C2=A0I'm not 100% sure though. =C2=A0Correct me if I'm wrong=
.
>
> I don't know. But the question you should ask is: Does the networking
> code promise this now and for the future?

Right.  But it's beyond my knowledge to answer this question.  If not,
adding a device specific flag is not very costing.

Hi Dave,

Can we assume that the netif_stop_queue() and netif_wake_queue() are
only used by the netdev driver?  And the queue state will not be
changed by other part of the networking subsystem?

- Leo

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

[David Miller]

From: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
Date: Mon, 30 Mar 2009 12:01:33 +0200

> I don't know. But the question you should ask is: Does the networking
> code promise this now and for the future? If not, you should
> fix the driver not to relay on netif_queue_stopped() here.

Stop this nonsense talk.

If the driver can't be the one and only controller of the TX queue
state, everything would essentially explode.

The fact is, it does.

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

[David Miller]

From: Joakim Tjernlund <joakim.tjernlund@transmode.se>
Date: Wed, 25 Mar 2009 14:30:49 +0100

> >From 1c2f23b1f37f4818c0fd0217b93eb38ab6564840 Mon Sep 17 00:00:00 2001
> From: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> Date: Tue, 24 Mar 2009 10:19:27 +0100
> Subject: [PATCH] ucc_geth: Move freeing of TX packets to NAPI context.
>  Also increase NAPI weight somewhat.
>  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>

The weight is not for the sake of your device, it's for
the sake of fairness with others.

Please just use 64, like every other driver does.

I'm not applying this.

[PATCH] ucc_geth: Move freeing of TX packets to NAPI context

[Anton Vorontsov]

From: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>

This will make the system alot more responsive while ping flooding the
ucc_geth ethernet interface.

Also set NAPI weight to 64 as this is a common value.

Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/net/ucc_geth.c |   31 +++++++++++--------------------
 drivers/net/ucc_geth.h |    1 -
 2 files changed, 11 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ucc_geth.c b/drivers/net/ucc_geth.c
index d3f39e8..98f961e 100644
--- a/drivers/net/ucc_geth.c
+++ b/drivers/net/ucc_geth.c
@@ -3216,7 +3216,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] =
@@ -3250,9 +3250,15 @@ 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->ndev, i);
+	spin_unlock(&ugeth->lock);
+
 	if (howmany < budget) {
 		napi_complete(napi);
-		setbits32(ugeth->uccf->p_uccm, UCCE_RX_EVENTS);
+		setbits32(ugeth->uccf->p_uccm, UCCE_RX_EVENTS | UCCE_TX_EVENTS);
 	}
 
 	return howmany;
@@ -3266,8 +3272,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__);
 
@@ -3281,27 +3285,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 (napi_schedule_prep(&ugeth->napi)) {
-			uccm &= ~UCCE_RX_EVENTS;
+			uccm &= ~(UCCE_RX_EVENTS | UCCE_TX_EVENTS);
 			out_be32(uccf->p_uccm, uccm);
 			__napi_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)
@@ -3734,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 2f8ee7c..6027647 100644
--- a/drivers/net/ucc_geth.h
+++ b/drivers/net/ucc_geth.h
@@ -852,7 +852,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.2.2

Re: [PATCH] ucc_geth: Move freeing of TX packets to NAPI context

[David Miller]

From: Anton Vorontsov <avorontsov@ru.mvista.com>
Date: Sat, 18 Apr 2009 02:03:48 +0400

> This will make the system alot more responsive while ping flooding the
> ucc_geth ethernet interface.
> 
> Also set NAPI weight to 64 as this is a common value.
> 
> Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>

Applied to net-next-2.6, thank you.