netdev - Re: [PATCH] [-MM, FIX V3] e1000e: incorporate napi

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <46E816F5.9010409@intel.com>
Date:	Wed, 12 Sep 2007 09:42:29 -0700
From:	"Kok, Auke" <auke-jan.h.kok@...el.com>
To:	David Miller <davem@...emloft.net>
CC:	Robert.Olsson@...a.slu.se, akpm@...ux-foundation.org,
	jeff@...zik.org, netdev@...r.kernel.org
Subject: Re: [PATCH] [-MM, FIX V3] e1000e: incorporate napi_struct changes
 from net-2.6.24.git

David Miller wrote:
> From: Robert Olsson <Robert.Olsson@...a.slu.se>
> Date: Sat, 8 Sep 2007 09:53:49 +0200
> 
>>  Yes a correct observation. I've spotted this bug too and it caused by the 
>>  policy change in the NAPI scheduling. Look at tx_cleaned. 
>>
>>  I suggest we revert this change for now.
> 
> The tx_cleaned logic change was not intentional, and
> that's the bug that makes e1000 spin endlessly in NAPI.
> 
> The other part, the work_done < budget part, was intentional
> so I'm going to keep it in there for now.  I've checked
> in the patch below to deal with this.
> 
> I suspect the check "work_done == 0" is some shamans dance
> to get slightly better performance, but it's 1) wrong and
> 2) at best needs to be explained in a comment and fully
> quantified.

it probably gives us one more poll, so it might help, this isn't crucial and I 
agree that it might offset the budgetting.

> From e8cbb449155000eecc6e855ea71510fecfc7d5ee Mon Sep 17 00:00:00 2001
> From: David S. Miller <davem@...chee.(none)>
> Date: Wed, 12 Sep 2007 16:50:32 +0200
> Subject: [PATCH] [E1000]: Fix unintended NAPI breakout logic change.
> 
> The inversion of the !tx_cleaned test in e1000_clean()
> was not intentional, we just wanted to change the
> "work_done == 0" to "work_done < budget"
> 
> Noticed by Robert Olsson.
> 
> Signed-off-by: David S. Miller <davem@...emloft.net>
> ---
>  drivers/net/e1000/e1000_main.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
> index 7b0bcdb..58bb758 100644
> --- a/drivers/net/e1000/e1000_main.c
> +++ b/drivers/net/e1000/e1000_main.c
> @@ -3944,7 +3944,7 @@ e1000_clean(struct napi_struct *napi, int budget)
>  	                  &work_done, budget);
>  
>  	/* If no Tx and not enough Rx work done, exit the polling mode */
> -	if ((tx_cleaned && (work_done < budget)) ||
> +	if ((!tx_cleaned && (work_done < budget)) ||
>  	   !netif_running(poll_dev)) {
>  quit_polling:
>  		if (likely(adapter->itr_setting & 3))


Ack, this is exactly what I did to fix e1000e as well.

Auke
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html