From owner-freebsd-current@FreeBSD.ORG Thu Oct 2 17:24:25 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 E9D1E7A0; Thu, 2 Oct 2014 17:24:24 +0000 (UTC) Received: from mail-wi0-x22f.google.com (mail-wi0-x22f.google.com [IPv6:2a00:1450:400c:c05::22f]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 6109D351; Thu, 2 Oct 2014 17:24:24 +0000 (UTC) Received: by mail-wi0-f175.google.com with SMTP id d1so4685336wiv.8 for ; Thu, 02 Oct 2014 10:24:22 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; bh=6+EKjKUcCYhm6qGEVAHJft0U1U3Ed7hiyjC85K1qPfA=; b=ECy9Ayo4nmwr4rd4wwoGNQ4uz+qKyCJzIoKBiGLGeiJgmlk3r9n1XQlbJtc2CrNr/I 2PrIoCW8J5HbyME/pRT7xuwc7BzD8to1uMxkb44d9Ebike+RyShG1cnhmTe1+myWxxsB LoVOn6EU5i5DEjPwkTahvn70cZImvpoWrKKTtm3Sbgt1QvyUyVkgKTagvuMahYB2sRmg 3Z8N5EWGe5dzPVHrI1gwaktaK6FDNRXg+RFZ4a0chaHmmXQEvhG4S1RaR49dKc1gN15M qVEfM+pSrx894zhPLjvL6Eq/yqCFOusxo7xxAC560bbWwqt5kRdT1fCTODshRqHUfRjc 2i4g== MIME-Version: 1.0 X-Received: by 10.180.9.73 with SMTP id x9mr5939534wia.20.1412270662529; Thu, 02 Oct 2014 10:24:22 -0700 (PDT) Sender: adrian.chadd@gmail.com Received: by 10.216.106.136 with HTTP; Thu, 2 Oct 2014 10:24:22 -0700 (PDT) In-Reply-To: <201410021116.27583.jhb@freebsd.org> References: <2113392.UOaBFTpimf@ralph.baldwin.cx> <201410021116.27583.jhb@freebsd.org> Date: Thu, 2 Oct 2014 10:24:22 -0700 X-Google-Sender-Auth: UWrRQfSSSTFfeS9iL5ZOYdEv3c4 Message-ID: Subject: Re: [PATCH] Fix OACTIVE for an(4) From: Adrian Chadd To: John Baldwin Content-Type: text/plain; charset=UTF-8 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 17:24:25 -0000 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. -a