Re: [GIT]: Networking

[Ayaz Abdulla]

Eric Dumazet wrote:
> Eric Dumazet a écrit :
>
> > Ingo Molnar a écrit :
> >
> > > > The following changes since commit 52989765629e7d182b4f146050ebba0abf2cb0b7:
> > > >  Linus Torvalds (1):
> > > >        Merge git://git.kernel.org/.../davem/net-2.6
> > > >
> > > > are available in the git repository at:
> > > >
> > > >  master.kernel.org:/pub/scm/linux/kernel/git/davem/net-2.6.git master
> > >
> > > Hm, something in this lot quickly wrecked networking here - see the 
> > > tx timeout dump below. It starts with:
> > >
> > > [  351.004596] WARNING: at net/sched/sch_generic.c:246 dev_watchdog+0x10b/0x19c()
> > > [  351.011815] Hardware name: System Product Name
> > > [  351.016220] NETDEV WATCHDOG: eth0 (forcedeth): transmit queue 0 timed out
> > >
> > > Config attached. Unfortunately i've got no time to do bisection 
> > > today.
> >
> >
> >
> > forcedeth might have a problem, in its netif_wake_queue() logic, but
> > I could not see why a recent patch could make this problem visible now.
> >
> > CPU0/1: AMD Athlon(tm) 64 X2 Dual Core Processor 3800+ stepping 02
> > is not a new cpu either :)
> >
> > forcedeth uses an internal tx_stop without appropriate barrier.
> >
> > Could you try following patch ?
> >
> > (random guess as I dont have much time right now)
>
>
> Oh well this patch was soooo stupid, sorry Ingo.
>
>
> We might have a race in napi_schedule(), leaving interrupts disabled forever.
> I cannot test this patch, I dont have the hardware...
>
> Thanks
>
> diff --git a/drivers/net/forcedeth.c b/drivers/net/forcedeth.c
> index 1094d29..3b4e076 100644
> --- a/drivers/net/forcedeth.c
> +++ b/drivers/net/forcedeth.c
> @@ -3514,11 +3514,13 @@ static irqreturn_t nv_nic_irq(int foo, void *data)
>  	nv_msi_workaround(np);
>  
>  #ifdef CONFIG_FORCEDETH_NAPI
> -	napi_schedule(&np->napi);
> -
> -	/* Disable furthur irq's
> -	   (msix not enabled with napi) */
> -	writel(0, base + NvRegIrqMask);
> +	if (napi_schedule_prep(&np->napi)) {
> +		/*
> +		 * Disable further irq's (msix not enabled with napi)
> +		 */
> +		writel(0, base + NvRegIrqMask);
> +		__napi_schedule(&np->napi);
> +	}

Yes, good catch. There is a race condition here with napi poll.

I would prefer to do the following to keep the code simple and clean.

writel(0, base + NvRegIrqMask);
napi_schedule(&np->napi);

.....

>  
>  #else
>  	do
> @@ -3615,12 +3617,13 @@ static irqreturn_t nv_nic_irq_optimized(int foo, void *data)
>  	nv_msi_workaround(np);
>  
>  #ifdef CONFIG_FORCEDETH_NAPI
> -	napi_schedule(&np->napi);
> -
> -	/* Disable furthur irq's
> -	   (msix not enabled with napi) */
> -	writel(0, base + NvRegIrqMask);
> -
> +	if (napi_schedule_prep(&np->napi)) {
> +		/*
> +		 * Disable further irq's (msix not enabled with napi)
> +		 */
> +		writel(0, base + NvRegIrqMask);
> +		__napi_schedule(&np->napi);
> +	}
>  #else
>  	do
>  	{
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/