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

On Thursday, October 02, 2014 1:24:22 pm Adrian Chadd wrote:
> 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.

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