From owner-freebsd-current@FreeBSD.ORG Thu Oct 2 15:59:51 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 D7BB29CD; Thu, 2 Oct 2014 15:59:51 +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 B09AC817; Thu, 2 Oct 2014 15:59:51 +0000 (UTC) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 52471B989; Thu, 2 Oct 2014 11:59:50 -0400 (EDT) From: John Baldwin To: Adrian Chadd Subject: Re: [PATCH] Fix OACTIVE for an(4) Date: Thu, 2 Oct 2014 11:16:27 -0400 User-Agent: KMail/1.13.5 (FreeBSD/8.4-CBSD-20140415; KDE/4.5.5; amd64; ; ) References: <2113392.UOaBFTpimf@ralph.baldwin.cx> In-Reply-To: MIME-Version: 1.0 Content-Type: Text/Plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <201410021116.27583.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Thu, 02 Oct 2014 11:59:50 -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 15:59:51 -0000 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). -- John Baldwin