From owner-svn-src-all@freebsd.org Wed Apr 11 17:19:12 2018 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id B478EF80993; Wed, 11 Apr 2018 17:19:11 +0000 (UTC) (envelope-from jonlooney@gmail.com) Received: from mail-wm0-f49.google.com (mail-wm0-f49.google.com [74.125.82.49]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 1EFF36A8FF; Wed, 11 Apr 2018 17:19:10 +0000 (UTC) (envelope-from jonlooney@gmail.com) Received: by mail-wm0-f49.google.com with SMTP id i3so5098956wmf.3; Wed, 11 Apr 2018 10:19:10 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=08vCITbl7YQtwje6mYNfBwFTZqyOKHbjRfe8cfAnNO4=; b=AAH6YD5pOrPIJ2fc+Q0+ZjlL7JDS2/n0ltyCyKIaCm2bPWJQYFVWqpGTqSp9zxhoBg 89HOgrgA+lIwDztUq6w4DkISb8uyZ6R+EevVeMXP2P1ippsy+Wlk0flxyprvhlMQ1F6T zICDoEYUUfTfP59bp72jE96oFeMHvnjNXdBkng+YWJTb9Tu6hzeb5QivaVyDqneoOfw0 EyiAL3Ex6+zgnOt9K9eDT2m8hAezQxrj5qccsCYEYdYmW/t6SDkOO19qKc7GVs7C49Dy fRkjMdL4RnzI5s5+pu5h1HOJezNMdsuWrSmf8Xm4VX1WxJVf/hiS24J0su2unweaPfbX su0A== X-Gm-Message-State: ALQs6tAC/85xXP/WIBxqfPV12QfD+e7Tg5f4jstk/emwTenEs4UUYctU mCBZ59xXyveRw//71rSib+KqS9wm X-Google-Smtp-Source: AIpwx48QlTFskSdC6diraXqjPfESsG7Z25641R1m4ViLqKGwH73HLTWuCFFosvpoadpLSeCF/bgEYw== X-Received: by 10.80.235.72 with SMTP id z8mr11141077edp.170.1523466776548; Wed, 11 Apr 2018 10:12:56 -0700 (PDT) Received: from mail-wm0-f51.google.com (mail-wm0-f51.google.com. [74.125.82.51]) by smtp.gmail.com with ESMTPSA id y4sm1033779edi.4.2018.04.11.10.12.56 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 11 Apr 2018 10:12:56 -0700 (PDT) Received: by mail-wm0-f51.google.com with SMTP id x4so5044617wmh.5; Wed, 11 Apr 2018 10:12:56 -0700 (PDT) X-Received: by 10.28.118.3 with SMTP id r3mr3256482wmc.90.1523466776178; Wed, 11 Apr 2018 10:12:56 -0700 (PDT) MIME-Version: 1.0 Received: by 10.223.199.203 with HTTP; Wed, 11 Apr 2018 10:12:55 -0700 (PDT) In-Reply-To: <201804101948.w3AJmOt2066564@repo.freebsd.org> References: <201804101948.w3AJmOt2066564@repo.freebsd.org> From: "Jonathan T. Looney" Date: Wed, 11 Apr 2018 13:12:55 -0400 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: svn commit: r332389 - head/sys/net To: Stephen Hurd Cc: src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.25 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 11 Apr 2018 17:19:12 -0000 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 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 > 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 > + * Copyright (c) 2014-2018, Matthew Macy > * 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 > >