From owner-freebsd-current@FreeBSD.ORG Thu Oct 2 19:35:20 2014 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id E7473D31; Thu, 2 Oct 2014 19:35:19 +0000 (UTC) Received: from bigwig.baldwin.cx (bigwig.baldwin.cx [IPv6:2001:470:1f11:75::1]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id A635374C; Thu, 2 Oct 2014 19:35:19 +0000 (UTC) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 3F5A0B93C; Thu, 2 Oct 2014 15:35:18 -0400 (EDT) From: John Baldwin To: Adrian Chadd Subject: Re: [PATCH] Fix OACTIVE for an(4) Date: Thu, 2 Oct 2014 13:40:22 -0400 User-Agent: KMail/1.13.5 (FreeBSD/8.4-CBSD-20140415; KDE/4.5.5; amd64; ; ) References: <2113392.UOaBFTpimf@ralph.baldwin.cx> <201410021116.27583.jhb@freebsd.org> In-Reply-To: MIME-Version: 1.0 Content-Type: Text/Plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <201410021340.22447.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Thu, 02 Oct 2014 15:35:18 -0400 (EDT) Cc: freebsd-current X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 02 Oct 2014 19:35:20 -0000 On Thursday, October 02, 2014 1:24:22 pm Adrian Chadd wrote: > On 2 October 2014 08:16, John Baldwin wrote: > > On Wednesday, October 01, 2014 2:58:38 pm Adrian Chadd wrote: > >> Hi, > >> > >> On 1 October 2014 07:14, John Baldwin wrote: > >> > This small patch correctly sets OACTIVE when an(4) gets backed up. Right > > now > >> > I believe it will never set the flag. It is only an optimization, it > > should > >> > not affect correctness. > >> > > >> > Index: an/if_an.c > >> > =================================================================== > >> > --- an/if_an.c (revision 270968) > >> > +++ an/if_an.c (working copy) > >> > @@ -2906,11 +2906,11 @@ > >> > CSR_WRITE_2(sc, AN_INT_EN(sc->mpi350), AN_INTRS(sc- > >>mpi350)); > >> > } > >> > > >> > - if (m0 != NULL) > >> > + if (sc->an_rdata.an_tx_prod != idx) { > >> > ifp->if_drv_flags |= IFF_DRV_OACTIVE; > >> > + sc->an_rdata.an_tx_prod = idx; > >> > + } > >> > > >> > - sc->an_rdata.an_tx_prod = idx; > >> > - > >> > return; > >> > } > >> > >> I haven't looked at the rest of the driver; is everything else around > >> OACTIVE locked correctly and consistently? > > > > As well as OACTIVE is for any other driver. > > > >> There's no single-entry into if_start(). It can be called from > >> multiple paths at the same time. > > > > Yes, I know. However, this bug is more that it will never set OACTIVE > > currently (m0 is always set to NULL before it gets to this point). > > > > This code in its stats timer is also dubious: > > > > /* Don't do this while we're transmitting */ > > if (ifp->if_drv_flags & IFF_DRV_OACTIVE) { > > callout_reset(&sc->an_stat_ch, hz, an_stats_update, sc); > > return; > > } > > > > sc->an_stats.an_len = sizeof(struct an_ltv_stats); > > sc->an_stats.an_type = AN_RID_32BITS_CUM; > > if (an_read_record(sc, (struct an_ltv_gen *)&sc->an_stats.an_len)) > > return; > > > > callout_reset(&sc->an_stat_ch, hz, an_stats_update, sc); > > > > First, the callout_reset() can just be moved earlier. > > > > Second, OACTIVE doesn't mean that anything is transmitting, it means the ring > > is full (at least in terms of how all other drivers use it). > > yes, but if you don't absolutely handle it in a race-free situation, > you end up never having if_start() called. > > IFQ_HANDOFF() -> IFQ_HANDOFF_ADJ() -> enqueue, then if it worked AND > OACTIVE==0, call if_start() > > Then you hit the stupid situation where OACTIVE was set just long > enough for the queue to fill up without calling if_start(), then once > it's filled if_start() won't ever be called from the transmitter > context. It has to be called from the completion context or the > receive context. Or, well, anywhere. > > All of that stuff needs to die. Yes, but this is true of all if_start-using drivers currently, and it cannot be fixed in the drivers themselves. It is orthogonal to whether or not an(4) correctly sets OACTIVE. -- John Baldwin