Re: [PATCH] Fix OACTIVE for an(4)

On 2 October 2014 08:16, John Baldwin <jhb_at_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_at_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)
>> > _at__at_ -2906,11 +2906,11 _at__at_
>> >                 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