Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 16 Mar 2012 11:29:52 -0400
From:      John Baldwin <jhb@freebsd.org>
To:        Sean Bruno <seanbru@yahoo-inc.com>
Cc:        "freebsd-net@freebsd.org" <freebsd-net@freebsd.org>, Adrian Chadd <adrian@freebsd.org>, Hooman Fazaeli <hoomanfazaeli@gmail.com>, Jason Wolfe <nitroboost@gmail.com>
Subject:   Re: Intel 82574L interface wedging - em7.3.2/8.2-STABLE
Message-ID:  <201203161129.53034.jhb@freebsd.org>
In-Reply-To: <1331854869.3317.15.camel@powernoodle-l7.corp.yahoo.com>
References:  <CAAAm0r3Qj%2B2rf8cx54bcyAXGQezcE8J=xXYPq4W-jDy75r8qew@mail.gmail.com> <201203151417.04507.jhb@freebsd.org> <1331854869.3317.15.camel@powernoodle-l7.corp.yahoo.com>

next in thread | previous in thread | raw e-mail | index | archive | help
On Thursday, March 15, 2012 7:41:09 pm Sean Bruno wrote:
> 
> > 
> > Hmm, so I have yet to test this, but I found several bugs related to 
transmit 
> > in em(4) and igb(4) recently just reading the code.  (Mostly unnecessary
> > scheduling of tasks for transmit.)  I've included your change of 
restarting
> > TX when link becomes active.  I've also updated it to fix resume for em
> > and igb to DTRT when buf_ring is used, and to not include old-style start
> > routines at all when using multiq.  It is at 
> > http://www.freebsd.org/~jhb/patches/e1000_txeof2.patch
> > 
> 
> I think that some of the code being removed originated from our universe
> over here at Yahoo.  We were seeing the driver assert IFF_OACTIVE and
> never clearing out.
> 
> Reviewing this patch at a glance I note that the check of IFF_OACTIVE
> was removed, if the kernel can get us out of that state without the
> IFF_OACTIVE checks, then I'm good with it.

Yes, it was buggy before in that it would just sit and poll unnecessarily.  
The problem was that it wasn't actually kicking off retransmits in some cases
(e.g. igb_msix_que and em_msix_tx).  That was the real cause of it hanging
on OACTIVE.  The current code schedules more tasks as a much more expensive
workaround and I remove all that.

> Sean
> 
> ref:
> 
> @@ -1497,10 +1509,11 @@
>  		if (!drbr_empty(ifp, txr->br))
>  			em_mq_start_locked(ifp, txr, NULL);
>  #else
> -		em_start_locked(ifp, txr);
> +		if (!IFQ_DRV_IS_EMPTY(&ifp->if_snd))
> +			em_start_locked(ifp, txr);
>  #endif
>  		EM_TX_UNLOCK(txr);
> -		if (more || (ifp->if_drv_flags & IFF_DRV_OACTIVE)) {
> +		if (more) {
>  			taskqueue_enqueue(adapter->tq, &adapter->que_task);
>  			return;
> 
> 
> 
> 

-- 
John Baldwin



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201203161129.53034.jhb>