Date: Thu, 2 Oct 2014 13:40:22 -0400 From: John Baldwin <jhb@freebsd.org> To: Adrian Chadd <adrian@freebsd.org> Cc: freebsd-current <freebsd-current@freebsd.org> Subject: Re: [PATCH] Fix OACTIVE for an(4) Message-ID: <201410021340.22447.jhb@freebsd.org> In-Reply-To: <CAJ-VmokW0kkqjv=qr-jxqRnprmcSkiirtxMis%2BGefUzwiUYsrw@mail.gmail.com> References: <2113392.UOaBFTpimf@ralph.baldwin.cx> <201410021116.27583.jhb@freebsd.org> <CAJ-VmokW0kkqjv=qr-jxqRnprmcSkiirtxMis%2BGefUzwiUYsrw@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thursday, October 02, 2014 1:24:22 pm Adrian Chadd wrote:
> On 2 October 2014 08:16, John Baldwin <jhb@freebsd.org> wrote:
> > On Wednesday, October 01, 2014 2:58:38 pm Adrian Chadd wrote:
> >> Hi,
> >>
> >> On 1 October 2014 07:14, John Baldwin <jhb@freebsd.org> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201410021340.22447.jhb>