netdev - Re: [PATCH] r8169: Reduce looping in the interrupt handler.

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]

Date:	Wed, 26 Aug 2009 09:56:14 -0400
From:	David Dillow <dave@...dillows.org>
To:	"Eric W. Biederman" <ebiederm@...ssion.com>
Cc:	Francois Romieu <romieu@...zoreil.com>,
	Michael Riepe <michael.riepe@...glemail.com>,
	Michael Buesch <mb@...sch.de>,
	Rui Santos <rsantos@...popie.com>,
	Michael B??ker <m.bueker@...lin.de>,
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH] r8169: Reduce looping in the interrupt handler.

On Wed, 2009-08-26 at 00:58 -0700, Eric W. Biederman wrote:
> As of 2.6.30 I have been observing soft lockups and netdev watchdog
> timeouts caused by looping in the r8169 interrupt handler.
> 
> - Introduce a hard limit to the maximum number of times we will loop in
>   the interrupt handler, and print a message when we hit it.
> 
> - Break out of the loop if after looking none of the events in status
>   are events we expect to be delivered by an interrupt.
> 
> With just the hard limit and message bits of my patch in my test case
> I get hit my limit of 10 loops 12 times.  After filtering by intr_mask
> and intr_event I don't get any warnings.
> 
> Any complaints from those who know the driver better than I?

Have you tried this under a sustained heavy transmit load? I think you
may have reintroduced the problem the original patch was trying to
prevent -- wedging the MSI interrupt by not ACKing all outstanding
interrupt sources, masked or otherwise. I'll try to test this out on the
machine where I've seen the problem consistently, but it may be this
weekend.

> diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
> index 3b19e0c..2214945 100644
> --- a/drivers/net/r8169.c
> +++ b/drivers/net/r8169.c

> +
> +		/* Ignore the parts of status that reflect more than
> +		 * the enabled interrupts.
> +		 */
> +		smp_rmb();
> +		if (!(status & tp->intr_mask & tp->intr_event))
> +			break;
>  	}

This looks like an odd construct, since we're just about to go back the
while condition up top -- why not just mask it here and let the loop
handle it naturally?


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