svn commit: r332389 - head/sys/net
Jonathan T. Looney
jtl at freebsd.org
Wed Apr 11 17:19:12 UTC 2018
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 at 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 at 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 at mattmacy.io>
> + * Copyright (c) 2014-2018, Matthew Macy <mmacy at 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
>
>
More information about the svn-src-head
mailing list