Date: Thu, 2 Oct 2014 10:24:22 -0700 From: Adrian Chadd <adrian@freebsd.org> To: John Baldwin <jhb@freebsd.org> Cc: freebsd-current <freebsd-current@freebsd.org> Subject: Re: [PATCH] Fix OACTIVE for an(4) Message-ID: <CAJ-VmokW0kkqjv=qr-jxqRnprmcSkiirtxMis%2BGefUzwiUYsrw@mail.gmail.com> In-Reply-To: <201410021116.27583.jhb@freebsd.org> References: <2113392.UOaBFTpimf@ralph.baldwin.cx> <CAJ-VmomwiRRfHAGMn6onqQMXBEze40Nmyqya-5v8wMgZFC7iBg@mail.gmail.com> <201410021116.27583.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
-a
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJ-VmokW0kkqjv=qr-jxqRnprmcSkiirtxMis%2BGefUzwiUYsrw>