On 25/04/16(Mon) 11:35, Visa Hankala wrote:
> On Mon, Apr 25, 2016 at 10:20:32AM +0200, Martin Pieuchot wrote:
> > On 24/04/16(Sun) 16:13, Visa Hankala wrote:
> > > This adds MP-safe TX for cnmac(4). OK?
> >
> > Would it be possible to do that without mutex? Having the same pattern
> > across most of our drivers would reduce the maintenance effort.
>
> The only real use of the mutex is to keep octeon_eth_tick_free() away
> from sc_sendq while the start routine is running. The queue tracks mbufs
> that need to be freed after transmission. Unlike many other drivers,
> there is no TX ring. As long as there is at least a little bit of
> traffic, octeon_eth_start() can take care of draining the queue. The
> hardware does not have a transmission complete interrupt, so the timer
> is there to free transmitted mbufs in case traffic stops altogether for
> a long time.
>
> What driver would be a good example for this case?
I don't think we have any example for this case. But it looks like the
watchdog problem for which we schedule a task and call intr_barrier()
after removing the IFF_RUNNING flag. Now if you think it doesn't make
sense I'm fine with your approach.
> > > Index: arch/octeon/dev/if_cnmac.c
> > > ===================================================================
> > > RCS file: src/sys/arch/octeon/dev/if_cnmac.c,v
> > > retrieving revision 1.38
> > > diff -u -p -r1.38 if_cnmac.c
> > > --- arch/octeon/dev/if_cnmac.c 13 Apr 2016 11:34:00 -0000 1.38
> > > +++ arch/octeon/dev/if_cnmac.c 24 Apr 2016 15:35:04 -0000
> > > @@ -285,6 +285,7 @@ octeon_eth_attach(struct device *parent,
> > > octeon_eth_gsc[sc->sc_port] = sc;
> > >
> > > ml_init(&sc->sc_sendq);
> > > + mtx_init(&sc->sc_sendq_mtx, IPL_NET);
> > > sc->sc_soft_req_thresh = 15/* XXX */;
> > > sc->sc_ext_callback_cnt = 0;
> > >
> > > @@ -317,6 +318,7 @@ octeon_eth_attach(struct device *parent,
> > > strncpy(ifp->if_xname, sc->sc_dev.dv_xname, sizeof(ifp->if_xname));
> > > ifp->if_softc = sc;
> > > ifp->if_flags = IFF_BROADCAST | IFF_SIMPLEX | IFF_MULTICAST;
> > > + ifp->if_xflags = IFXF_MPSAFE;
> > > ifp->if_ioctl = octeon_eth_ioctl;
> > > ifp->if_start = octeon_eth_start;
> > > ifp->if_watchdog = octeon_eth_watchdog;
> > > @@ -742,7 +744,7 @@ octeon_eth_ioctl(struct ifnet *ifp, u_lo
> > > error = 0;
> > > }
> > >
> > > - octeon_eth_start(ifp);
> > > + if_start(ifp);
> > >
> > > splx(s);
> > > return (error);
> > > @@ -959,18 +961,19 @@ octeon_eth_start(struct ifnet *ifp)
> > > struct octeon_eth_softc *sc = ifp->if_softc;
> > > struct mbuf *m;
> > >
> > > + if (__predict_false(!cn30xxgmx_link_status(sc->sc_gmx_port))) {
> > > + IFQ_PURGE(&ifp->if_snd);
> > > + return;
> > > + }
> > > +
> > > + mtx_enter(&sc->sc_sendq_mtx);
> > > +
> > > /*
> > > * performance tuning
> > > * presend iobdma request
> > > */
> > > octeon_eth_send_queue_flush_prefetch(sc);
> > >
> > > - if (!(ifp->if_flags & IFF_RUNNING) || ifq_is_oactive(&ifp->if_snd))
> > > - goto last;
> > > -
> > > - if (__predict_false(!cn30xxgmx_link_status(sc->sc_gmx_port)))
> > > - goto last;
> > > -
> > > for (;;) {
> > > octeon_eth_send_queue_flush_fetch(sc); /* XXX */
> > >
> > > @@ -980,13 +983,16 @@ octeon_eth_start(struct ifnet *ifp)
> > > * and bail out.
> > > */
> > > if (octeon_eth_send_queue_is_full(sc)) {
> > > + mtx_leave(&sc->sc_sendq_mtx);
> > > return;
> > > }
> > > /* XXX */
> > >
> > > IFQ_DEQUEUE(&ifp->if_snd, m);
> > > - if (m == NULL)
> > > + if (m == NULL) {
> > > + mtx_leave(&sc->sc_sendq_mtx);
> > > return;
> > > + }
> > >
> > > OCTEON_ETH_TAP(ifp, m, BPF_DIRECTION_OUT);
> > >
> > > @@ -1008,8 +1014,9 @@ octeon_eth_start(struct ifnet *ifp)
> > > octeon_eth_send_queue_flush_prefetch(sc);
> > > }
> > >
> > > -last:
> > > octeon_eth_send_queue_flush_fetch(sc);
> > > +
> > > + mtx_leave(&sc->sc_sendq_mtx);
> > > }
> > >
> > > void
> > > @@ -1025,7 +1032,7 @@ octeon_eth_watchdog(struct ifnet *ifp)
> > > ifq_clr_oactive(&ifp->if_snd);
> > > ifp->if_timer = 0;
> > >
> > > - octeon_eth_start(ifp);
> > > + if_start(ifp);
> > > }
> > >
> > > int
> > > @@ -1066,6 +1073,8 @@ octeon_eth_stop(struct ifnet *ifp, int d
> > > {
> > > struct octeon_eth_softc *sc = ifp->if_softc;
> > >
> > > + CLR(ifp->if_flags, IFF_RUNNING);
> > > +
> > > timeout_del(&sc->sc_tick_misc_ch);
> > > timeout_del(&sc->sc_tick_free_ch);
> > > timeout_del(&sc->sc_resume_ch);
> > > @@ -1074,13 +1083,12 @@ octeon_eth_stop(struct ifnet *ifp, int d
> > >
> > > cn30xxgmx_port_enable(sc->sc_gmx_port, 0);
> > >
> > > - /* Mark the interface as down and cancel the watchdog timer. */
> > > - CLR(ifp->if_flags, IFF_RUNNING);
> > > + intr_barrier(octeon_eth_pow_recv_ih);
> > > + ifq_barrier(&ifp->if_snd);
> > > +
> > > ifq_clr_oactive(&ifp->if_snd);
> > > ifp->if_timer = 0;
> > >
> > > - intr_barrier(octeon_eth_pow_recv_ih);
> > > -
> > > return 0;
> > > }
> > >
> > > @@ -1372,9 +1380,8 @@ octeon_eth_tick_free(void *arg)
> > > {
> > > struct octeon_eth_softc *sc = arg;
> > > int timo;
> > > - int s;
> > >
> > > - s = splnet();
> > > + mtx_enter(&sc->sc_sendq_mtx);
> > > /* XXX */
> > > if (ml_len(&sc->sc_sendq) > 0) {
> > > octeon_eth_send_queue_flush_prefetch(sc);
> > > @@ -1389,7 +1396,7 @@ octeon_eth_tick_free(void *arg)
> > > timo = 10;
> > > timeout_add_msec(&sc->sc_tick_free_ch, 1000 * timo / hz);
> > > /* XXX */
> > > - splx(s);
> > > + mtx_leave(&sc->sc_sendq_mtx);
> > > }
> > >
> > > /*
> > > Index: arch/octeon/dev/if_cnmacvar.h
> > > ===================================================================
> > > RCS file: src/sys/arch/octeon/dev/if_cnmacvar.h,v
> > > retrieving revision 1.7
> > > diff -u -p -r1.7 if_cnmacvar.h
> > > --- arch/octeon/dev/if_cnmacvar.h 8 Oct 2015 14:24:32 -0000 1.7
> > > +++ arch/octeon/dev/if_cnmacvar.h 24 Apr 2016 15:35:04 -0000
> > > @@ -80,6 +80,7 @@ struct octeon_eth_softc {
> > > int64_t sc_hard_done_cnt;
> > > int sc_prefetch;
> > > struct mbuf_list sc_sendq;
> > > + struct mutex sc_sendq_mtx;
> > > uint64_t sc_ext_callback_cnt;
> > >
> > > uint32_t sc_port;
> > >
> >
>
