[patch 09/11] forcedeth: improve NAPI logic

[patch 09/11] forcedeth: improve NAPI logic

[akpm]

From: Ingo Molnar <mingo@elte.hu>

Another forcedeth.c thing: i noticed that its NAPI handler does not do
tx-ring processing.  The patch below implements this - tested on DESC_VER_2
hardware, with CONFIG_FORCEDETH_NAPI=y.

Signed-off-by: Ingo Molnar <mingo@elte.hu>
Cc: Ayaz Abdulla <aabdulla@nvidia.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 drivers/net/forcedeth.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff -puN drivers/net/forcedeth.c~forcedeth-improve-napi-logic drivers/net/forcedeth.c
--- a/drivers/net/forcedeth.c~forcedeth-improve-napi-logic
+++ a/drivers/net/forcedeth.c
@@ -3108,9 +3108,17 @@ static int nv_napi_poll(struct net_devic
 	int retcode;
 
 	if (np->desc_ver == DESC_VER_1 || np->desc_ver == DESC_VER_2) {
+		spin_lock_irqsave(&np->lock, flags);
+		nv_tx_done(dev);
+		spin_unlock_irqrestore(&np->lock, flags);
+
 		pkts = nv_rx_process(dev, limit);
 		retcode = nv_alloc_rx(dev);
 	} else {
+		spin_lock_irqsave(&np->lock, flags);
+		nv_tx_done_optimized(dev, np->tx_ring_size);
+		spin_unlock_irqrestore(&np->lock, flags);
+
 		pkts = nv_rx_process_optimized(dev, limit);
 		retcode = nv_alloc_rx_optimized(dev);
 	}
_

Re: [patch 09/11] forcedeth: improve NAPI logic

[Ayaz Abdulla]

I don't see why the NAPI handler needs to process tx packets. The ISR 
will handle all tx processing.


akpm@linux-foundation.org wrote:
> From: Ingo Molnar <mingo@elte.hu>
> 
> Another forcedeth.c thing: i noticed that its NAPI handler does not do
> tx-ring processing.  The patch below implements this - tested on DESC_VER_2
> hardware, with CONFIG_FORCEDETH_NAPI=y.
> 
> Signed-off-by: Ingo Molnar <mingo@elte.hu>
> Cc: Ayaz Abdulla <aabdulla@nvidia.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  drivers/net/forcedeth.c |    8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff -puN drivers/net/forcedeth.c~forcedeth-improve-napi-logic drivers/net/forcedeth.c
> --- a/drivers/net/forcedeth.c~forcedeth-improve-napi-logic
> +++ a/drivers/net/forcedeth.c
> @@ -3108,9 +3108,17 @@ static int nv_napi_poll(struct net_devic
>  	int retcode;
>  
>  	if (np->desc_ver == DESC_VER_1 || np->desc_ver == DESC_VER_2) {
> +		spin_lock_irqsave(&np->lock, flags);
> +		nv_tx_done(dev);
> +		spin_unlock_irqrestore(&np->lock, flags);
> +
>  		pkts = nv_rx_process(dev, limit);
>  		retcode = nv_alloc_rx(dev);
>  	} else {
> +		spin_lock_irqsave(&np->lock, flags);
> +		nv_tx_done_optimized(dev, np->tx_ring_size);
> +		spin_unlock_irqrestore(&np->lock, flags);
> +
>  		pkts = nv_rx_process_optimized(dev, limit);
>  		retcode = nv_alloc_rx_optimized(dev);
>  	}
> _

Re: [patch 09/11] forcedeth: improve NAPI logic

[Ayaz Abdulla]

Jeff Garzik wrote:
> Ayaz Abdulla wrote:
> 
>> I don't see why the NAPI handler needs to process tx packets. The ISR 
>> will handle all tx processing.
> 
> 
> It is a design choice, not a requirement.
> 
> Moving non-RX interrupt processing to the NAPI handler can help as loads 
> increase.  The basic idea is to do as much work as possible in the NAPI 
> handler with NIC interrupts masked.  That mitigates global system 
> per-interrupt overhead even more than an only-RX NAPI scheme.
> 
> Several net drivers do TX completion handling in the NAPI handler.

Ok. In that case, the patch needs to be improved.

The following needs to be done when NAPI is enabled:
- remove the tx handling within the ISRs
- mask off the tx interrupts within the ISRs that handle tx processing
- re-enable tx interrupts within the NAPI handler
- add tx handling within the NAPI handler (this patch covers it)

> 
>     Jeff
> 
> 

-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

Re: [patch 09/11] forcedeth: improve NAPI logic

[Jeff Garzik]

Ayaz Abdulla wrote:
> I don't see why the NAPI handler needs to process tx packets. The ISR 
> will handle all tx processing.

It is a design choice, not a requirement.

Moving non-RX interrupt processing to the NAPI handler can help as loads 
increase.  The basic idea is to do as much work as possible in the NAPI 
handler with NIC interrupts masked.  That mitigates global system 
per-interrupt overhead even more than an only-RX NAPI scheme.

Several net drivers do TX completion handling in the NAPI handler.

	Jeff

Re: [patch 09/11] forcedeth: improve NAPI logic

[Jeff Garzik]

Ayaz Abdulla wrote:
> 
> 
> Jeff Garzik wrote:
>> Ayaz Abdulla wrote:
>>
>>> I don't see why the NAPI handler needs to process tx packets. The ISR 
>>> will handle all tx processing.
>>
>>
>> It is a design choice, not a requirement.
>>
>> Moving non-RX interrupt processing to the NAPI handler can help as 
>> loads increase.  The basic idea is to do as much work as possible in 
>> the NAPI handler with NIC interrupts masked.  That mitigates global 
>> system per-interrupt overhead even more than an only-RX NAPI scheme.
>>
>> Several net drivers do TX completion handling in the NAPI handler.
> 
> Ok. In that case, the patch needs to be improved.
> 
> The following needs to be done when NAPI is enabled:
> - remove the tx handling within the ISRs
> - mask off the tx interrupts within the ISRs that handle tx processing
> - re-enable tx interrupts within the NAPI handler
> - add tx handling within the NAPI handler (this patch covers it)

Agreed.

	Jeff

Re: [patch 09/11] forcedeth: improve NAPI logic

[Lennart Sorensen]

On Thu, Apr 26, 2007 at 10:53:04AM -0400, Ayaz Abdulla wrote:
> Ok. In that case, the patch needs to be improved.
> 
> The following needs to be done when NAPI is enabled:
> - remove the tx handling within the ISRs
> - mask off the tx interrupts within the ISRs that handle tx processing
> - re-enable tx interrupts within the NAPI handler
> - add tx handling within the NAPI handler (this patch covers it)

I thought a number of drivers handled tx from napi while receives were
happening, but went to plain interrupts if no receives were happening.
Maybe I misread the code (I have mainly dealt with pcnet32 so far).
Certainly for gigabit I would think napi all the time would be much more
efficient.

--
Len Sorensen

Re: [patch 09/11] forcedeth: improve NAPI logic

[Jeff Garzik]

akpm@linux-foundation.org wrote:
> From: Ingo Molnar <mingo@elte.hu>
> 
> Another forcedeth.c thing: i noticed that its NAPI handler does not do
> tx-ring processing.  The patch below implements this - tested on DESC_VER_2
> hardware, with CONFIG_FORCEDETH_NAPI=y.
> 
> Signed-off-by: Ingo Molnar <mingo@elte.hu>
> Cc: Ayaz Abdulla <aabdulla@nvidia.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  drivers/net/forcedeth.c |    8 ++++++++
>  1 file changed, 8 insertions(+)

See thread comments -- this patch should be expanded.

Re: [patch 09/11] forcedeth: improve NAPI logic

[Andrew Morton]

On Fri, 27 Apr 2007 20:10:54 -0400
Jeff Garzik <jeff@garzik.org> wrote:

> akpm@linux-foundation.org wrote:
> > From: Ingo Molnar <mingo@elte.hu>
> > 
> > Another forcedeth.c thing: i noticed that its NAPI handler does not do
> > tx-ring processing.  The patch below implements this - tested on DESC_VER_2
> > hardware, with CONFIG_FORCEDETH_NAPI=y.
> > 
> > Signed-off-by: Ingo Molnar <mingo@elte.hu>
> > Cc: Ayaz Abdulla <aabdulla@nvidia.com>
> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> > ---
> > 
> >  drivers/net/forcedeth.c |    8 ++++++++
> >  1 file changed, 8 insertions(+)
> 
> See thread comments -- this patch should be expanded.
> 

yeah, I added the comments to the changelog for you all to read next
time I send it ;)