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?
> >
> > 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;
> >
>
