Date: Wed, 11 Apr 2018 13:12:55 -0400 From: "Jonathan T. Looney" <jtl@freebsd.org> To: Stephen Hurd <shurd@freebsd.org> Cc: src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r332389 - head/sys/net Message-ID: <CADrOrms2rfuM9A%2BMQhKG-gQ0E5iBe5YyJfoDvDgXiD=CTBORfA@mail.gmail.com> In-Reply-To: <201804101948.w3AJmOt2066564@repo.freebsd.org> References: <201804101948.w3AJmOt2066564@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
It appears this is causing panics (see the emails on freebsd-current@). >From a brief glance, it appears that the new STATE_LOCK macros is used (and causes panics), but the STATE_LOCK_INIT macro is not used. Is it possible the code is missing an initialization call for the new mutex? Jonathan On Tue, Apr 10, 2018 at 3:48 PM, Stephen Hurd <shurd@freebsd.org> wrote: > Author: shurd > Date: Tue Apr 10 19:48:24 2018 > New Revision: 332389 > URL: https://svnweb.freebsd.org/changeset/base/332389 > > Log: > Split out flag manipulation from general context manipulation in iflib > > To avoid blocking on the context lock in the swi thread and risk > potential > deadlocks, this change protects lighter weight updates that only need to > be consistent with each other with their own lock. > > Submitted by: Matthew Macy <mmacy@mattmacy.io> > Reviewed by: shurd > Sponsored by: Limelight Networks > Differential Revision: https://reviews.freebsd.org/D14967 > > Modified: > head/sys/net/iflib.c > > Modified: head/sys/net/iflib.c > ============================================================ > ================== > --- head/sys/net/iflib.c Tue Apr 10 19:42:50 2018 (r332388) > +++ head/sys/net/iflib.c Tue Apr 10 19:48:24 2018 (r332389) > @@ -1,5 +1,5 @@ > /*- > - * Copyright (c) 2014-2017, Matthew Macy <mmacy@mattmacy.io> > + * Copyright (c) 2014-2018, Matthew Macy <mmacy@mattmacy.io> > * All rights reserved. > * > * Redistribution and use in source and binary forms, with or without > @@ -163,7 +163,8 @@ struct iflib_ctx { > if_shared_ctx_t ifc_sctx; > struct if_softc_ctx ifc_softc_ctx; > > - struct mtx ifc_mtx; > + struct mtx ifc_ctx_mtx; > + struct mtx ifc_state_mtx; > > uint16_t ifc_nhwtxqs; > uint16_t ifc_nhwrxqs; > @@ -318,8 +319,10 @@ typedef struct iflib_sw_tx_desc_array { > #define IFC_INIT_DONE 0x020 > #define IFC_PREFETCH 0x040 > #define IFC_DO_RESET 0x080 > -#define IFC_CHECK_HUNG 0x100 > +#define IFC_DO_WATCHDOG 0x100 > +#define IFC_CHECK_HUNG 0x200 > > + > #define CSUM_OFFLOAD (CSUM_IP_TSO|CSUM_IP6_TSO|CSUM_IP| \ > CSUM_IP_UDP|CSUM_IP_TCP|CSUM_IP_SCTP| \ > CSUM_IP6_UDP|CSUM_IP6_TCP|CSUM_IP6_SCTP) > @@ -535,13 +538,19 @@ rxd_info_zero(if_rxd_info_t ri) > > #define CTX_ACTIVE(ctx) ((if_getdrvflags((ctx)->ifc_ifp) & > IFF_DRV_RUNNING)) > > -#define CTX_LOCK_INIT(_sc, _name) mtx_init(&(_sc)->ifc_mtx, _name, > "iflib ctx lock", MTX_DEF) > +#define CTX_LOCK_INIT(_sc, _name) mtx_init(&(_sc)->ifc_ctx_mtx, _name, > "iflib ctx lock", MTX_DEF) > +#define CTX_LOCK(ctx) mtx_lock(&(ctx)->ifc_ctx_mtx) > +#define CTX_UNLOCK(ctx) mtx_unlock(&(ctx)->ifc_ctx_mtx) > +#define CTX_LOCK_DESTROY(ctx) mtx_destroy(&(ctx)->ifc_ctx_mtx) > > -#define CTX_LOCK(ctx) mtx_lock(&(ctx)->ifc_mtx) > -#define CTX_UNLOCK(ctx) mtx_unlock(&(ctx)->ifc_mtx) > -#define CTX_LOCK_DESTROY(ctx) mtx_destroy(&(ctx)->ifc_mtx) > > +#define STATE_LOCK_INIT(_sc, _name) mtx_init(&(_sc)->ifc_state_mtx, > _name, "iflib state lock", MTX_DEF) > +#define STATE_LOCK(ctx) mtx_lock(&(ctx)->ifc_state_mtx) > +#define STATE_UNLOCK(ctx) mtx_unlock(&(ctx)->ifc_state_mtx) > +#define STATE_LOCK_DESTROY(ctx) mtx_destroy(&(ctx)->ifc_state_mtx) > > + > + > #define CALLOUT_LOCK(txq) mtx_lock(&txq->ift_mtx) > #define CALLOUT_UNLOCK(txq) mtx_unlock(&txq->ift_mtx) > > @@ -2144,18 +2153,14 @@ iflib_timer(void *arg) > if (if_getdrvflags(ctx->ifc_ifp) & IFF_DRV_RUNNING) > callout_reset_on(&txq->ift_timer, hz/2, iflib_timer, txq, > txq->ift_timer.c_cpu); > return; > -hung: > - CTX_LOCK(ctx); > - if_setdrvflagbits(ctx->ifc_ifp, IFF_DRV_OACTIVE, IFF_DRV_RUNNING); > + hung: > device_printf(ctx->ifc_dev, "TX(%d) desc avail = %d, pidx = %d\n", > txq->ift_id, TXQ_AVAIL(txq), > txq->ift_pidx); > - > - IFDI_WATCHDOG_RESET(ctx); > - ctx->ifc_watchdog_events++; > - > - ctx->ifc_flags |= IFC_DO_RESET; > + STATE_LOCK(ctx); > + if_setdrvflagbits(ctx->ifc_ifp, IFF_DRV_OACTIVE, IFF_DRV_RUNNING); > + ctx->ifc_flags |= (IFC_DO_WATCHDOG|IFC_DO_RESET); > iflib_admin_intr_deferred(ctx); > - CTX_UNLOCK(ctx); > + STATE_UNLOCK(ctx); > } > > static void > @@ -2673,10 +2678,10 @@ iflib_rxeof(iflib_rxq_t rxq, qidx_t budget) > return true; > return (iflib_rxd_avail(ctx, rxq, *cidxp, 1)); > err: > - CTX_LOCK(ctx); > + STATE_LOCK(ctx); > ctx->ifc_flags |= IFC_DO_RESET; > iflib_admin_intr_deferred(ctx); > - CTX_UNLOCK(ctx); > + STATE_UNLOCK(ctx); > return (false); > } > > @@ -3706,27 +3711,35 @@ _task_fn_admin(void *context) > if_softc_ctx_t sctx = &ctx->ifc_softc_ctx; > iflib_txq_t txq; > int i; > + bool oactive, running, do_reset, do_watchdog; > > - if (!(if_getdrvflags(ctx->ifc_ifp) & IFF_DRV_RUNNING)) { > - if (!(if_getdrvflags(ctx->ifc_ifp) & IFF_DRV_OACTIVE)) { > - return; > - } > - } > + STATE_LOCK(ctx); > + running = (if_getdrvflags(ctx->ifc_ifp) & IFF_DRV_RUNNING); > + oactive = (if_getdrvflags(ctx->ifc_ifp) & IFF_DRV_OACTIVE); > + do_reset = (ctx->ifc_flags & IFC_DO_RESET); > + do_watchdog = (ctx->ifc_flags & IFC_DO_WATCHDOG); > + ctx->ifc_flags &= ~(IFC_DO_RESET|IFC_DO_WATCHDOG); > + STATE_UNLOCK(ctx); > > + if (!running & !oactive) > + return; > + > CTX_LOCK(ctx); > for (txq = ctx->ifc_txqs, i = 0; i < sctx->isc_ntxqsets; i++, > txq++) { > CALLOUT_LOCK(txq); > callout_stop(&txq->ift_timer); > CALLOUT_UNLOCK(txq); > } > + if (do_watchdog) { > + ctx->ifc_watchdog_events++; > + IFDI_WATCHDOG_RESET(ctx); > + } > IFDI_UPDATE_ADMIN_STATUS(ctx); > for (txq = ctx->ifc_txqs, i = 0; i < sctx->isc_ntxqsets; i++, > txq++) > callout_reset_on(&txq->ift_timer, hz/2, iflib_timer, txq, > txq->ift_timer.c_cpu); > IFDI_LINK_INTR_ENABLE(ctx); > - if (ctx->ifc_flags & IFC_DO_RESET) { > - ctx->ifc_flags &= ~IFC_DO_RESET; > + if (do_reset) > iflib_if_init_locked(ctx); > - } > CTX_UNLOCK(ctx); > > if (LINK_ACTIVE(ctx) == 0) > @@ -3870,15 +3883,15 @@ iflib_if_qflush(if_t ifp) > iflib_txq_t txq = ctx->ifc_txqs; > int i; > > - CTX_LOCK(ctx); > + STATE_LOCK(ctx); > ctx->ifc_flags |= IFC_QFLUSH; > - CTX_UNLOCK(ctx); > + STATE_UNLOCK(ctx); > for (i = 0; i < NTXQSETS(ctx); i++, txq++) > while (!(ifmp_ring_is_idle(txq->ift_br) || > ifmp_ring_is_stalled(txq->ift_br))) > iflib_txq_check_drain(txq, 0); > - CTX_LOCK(ctx); > + STATE_LOCK(ctx); > ctx->ifc_flags &= ~IFC_QFLUSH; > - CTX_UNLOCK(ctx); > + STATE_UNLOCK(ctx); > > if_qflush(ifp); > } > @@ -3935,14 +3948,18 @@ iflib_if_ioctl(if_t ifp, u_long command, caddr_t > data) > iflib_stop(ctx); > > if ((err = IFDI_MTU_SET(ctx, ifr->ifr_mtu)) == 0) { > + STATE_LOCK(ctx); > if (ifr->ifr_mtu > ctx->ifc_max_fl_buf_size) > ctx->ifc_flags |= IFC_MULTISEG; > else > ctx->ifc_flags &= ~IFC_MULTISEG; > + STATE_UNLOCK(ctx); > err = if_setmtu(ifp, ifr->ifr_mtu); > } > iflib_init_locked(ctx); > + STATE_LOCK(ctx); > if_setdrvflags(ifp, bits); > + STATE_UNLOCK(ctx); > CTX_UNLOCK(ctx); > break; > case SIOCSIFFLAGS: > @@ -4026,10 +4043,14 @@ iflib_if_ioctl(if_t ifp, u_long command, caddr_t > data) > bits = if_getdrvflags(ifp); > if (bits & IFF_DRV_RUNNING) > iflib_stop(ctx); > + STATE_LOCK(ctx); > if_togglecapenable(ifp, setmask); > + STATE_UNLOCK(ctx); > if (bits & IFF_DRV_RUNNING) > iflib_init_locked(ctx); > + STATE_LOCK(ctx); > if_setdrvflags(ifp, bits); > + STATE_UNLOCK(ctx); > CTX_UNLOCK(ctx); > } > break; > @@ -5431,9 +5452,11 @@ iflib_link_state_change(if_ctx_t ctx, int > link_state, > iflib_txq_t txq = ctx->ifc_txqs; > > if_setbaudrate(ifp, baudrate); > - if (baudrate >= IF_Gbps(10)) > + if (baudrate >= IF_Gbps(10)) { > + STATE_LOCK(ctx); > ctx->ifc_flags |= IFC_PREFETCH; > - > + STATE_UNLOCK(ctx); > + } > /* If link down, disable watchdog */ > if ((ctx->ifc_link_state == LINK_STATE_UP) && (link_state == > LINK_STATE_DOWN)) { > for (int i = 0; i < ctx->ifc_softc_ctx.isc_ntxqsets; i++, > txq++) > @@ -5492,7 +5515,7 @@ struct mtx * > iflib_ctx_lock_get(if_ctx_t ctx) > { > > - return (&ctx->ifc_mtx); > + return (&ctx->ifc_ctx_mtx); > } > > static int > >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CADrOrms2rfuM9A%2BMQhKG-gQ0E5iBe5YyJfoDvDgXiD=CTBORfA>