From owner-svn-src-all@freebsd.org Tue May 26 15:39:42 2020 Return-Path: Delivered-To: svn-src-all@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 62B722CB719; Tue, 26 May 2020 15:39:42 +0000 (UTC) (envelope-from mw@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 49WdSG22QKz4NJH; Tue, 26 May 2020 15:39:42 +0000 (UTC) (envelope-from mw@FreeBSD.org) Received: from repo.freebsd.org (repo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:0]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 3CC2E25B16; Tue, 26 May 2020 15:39:42 +0000 (UTC) (envelope-from mw@FreeBSD.org) Received: from repo.freebsd.org ([127.0.1.37]) by repo.freebsd.org (8.15.2/8.15.2) with ESMTP id 04QFdgA6064354; Tue, 26 May 2020 15:39:42 GMT (envelope-from mw@FreeBSD.org) Received: (from mw@localhost) by repo.freebsd.org (8.15.2/8.15.2/Submit) id 04QFdfkJ064351; Tue, 26 May 2020 15:39:41 GMT (envelope-from mw@FreeBSD.org) Message-Id: <202005261539.04QFdfkJ064351@repo.freebsd.org> X-Authentication-Warning: repo.freebsd.org: mw set sender to mw@FreeBSD.org using -f From: Marcin Wojtas Date: Tue, 26 May 2020 15:39:41 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r361516 - head/sys/dev/ena X-SVN-Group: head X-SVN-Commit-Author: mw X-SVN-Commit-Paths: head/sys/dev/ena X-SVN-Commit-Revision: 361516 X-SVN-Commit-Repository: base MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.33 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: Tue, 26 May 2020 15:39:42 -0000 Author: mw Date: Tue May 26 15:39:41 2020 New Revision: 361516 URL: https://svnweb.freebsd.org/changeset/base/361516 Log: Use single global lock in the ENA driver Currently, the driver had 2 global locks - one was sx lock used for up/down synchronization and the second one was mutex, which was used for link configuration and timer service callout. It is better to have single lock for that. We cannot use mutex, as it can sleep and cause witness errors in up/down configuration, so sx lock seems to be the only choice. Callout cannot use sx lock, but the timer service is MP safe, so we just need to avoid race between ena_down() and ena_detach(). It can be avoided by acquiring sx lock. Simple macros were added that are encapsulating implementation of the lock and makes the code cleaner. Submitted by: Michal Krawczyk Obtained from: Semihalf Sponsored by: Amazon, Inc. Modified: head/sys/dev/ena/ena.c head/sys/dev/ena/ena.h head/sys/dev/ena/ena_netmap.c Modified: head/sys/dev/ena/ena.c ============================================================================== --- head/sys/dev/ena/ena.c Tue May 26 15:37:55 2020 (r361515) +++ head/sys/dev/ena/ena.c Tue May 26 15:39:41 2020 (r361516) @@ -1907,13 +1907,13 @@ ena_media_status(if_t ifp, struct ifmediareq *ifmr) struct ena_adapter *adapter = if_getsoftc(ifp); ena_trace(ENA_DBG, "enter\n"); - mtx_lock(&adapter->global_mtx); + ENA_LOCK_LOCK(adapter); ifmr->ifm_status = IFM_AVALID; ifmr->ifm_active = IFM_ETHER; if (!ENA_FLAG_ISSET(ENA_FLAG_LINK_UP, adapter)) { - mtx_unlock(&adapter->global_mtx); + ENA_LOCK_UNLOCK(adapter); ena_trace(ENA_INFO, "Link is down\n"); return; } @@ -1921,7 +1921,7 @@ ena_media_status(if_t ifp, struct ifmediareq *ifmr) ifmr->ifm_status |= IFM_ACTIVE; ifmr->ifm_active |= IFM_UNKNOWN | IFM_FDX; - mtx_unlock(&adapter->global_mtx); + ENA_LOCK_UNLOCK(adapter); } static void @@ -1930,9 +1930,9 @@ ena_init(void *arg) struct ena_adapter *adapter = (struct ena_adapter *)arg; if (!ENA_FLAG_ISSET(ENA_FLAG_DEV_UP, adapter)) { - sx_xlock(&adapter->ioctl_sx); + ENA_LOCK_LOCK(adapter); ena_up(adapter); - sx_unlock(&adapter->ioctl_sx); + ENA_LOCK_UNLOCK(adapter); } } @@ -1954,13 +1954,13 @@ ena_ioctl(if_t ifp, u_long command, caddr_t data) case SIOCSIFMTU: if (ifp->if_mtu == ifr->ifr_mtu) break; - sx_xlock(&adapter->ioctl_sx); + ENA_LOCK_LOCK(adapter); ena_down(adapter); ena_change_mtu(ifp, ifr->ifr_mtu); rc = ena_up(adapter); - sx_unlock(&adapter->ioctl_sx); + ENA_LOCK_UNLOCK(adapter); break; case SIOCSIFFLAGS: @@ -1972,15 +1972,15 @@ ena_ioctl(if_t ifp, u_long command, caddr_t data) "ioctl promisc/allmulti\n"); } } else { - sx_xlock(&adapter->ioctl_sx); + ENA_LOCK_LOCK(adapter); rc = ena_up(adapter); - sx_unlock(&adapter->ioctl_sx); + ENA_LOCK_UNLOCK(adapter); } } else { if ((if_getdrvflags(ifp) & IFF_DRV_RUNNING) != 0) { - sx_xlock(&adapter->ioctl_sx); + ENA_LOCK_LOCK(adapter); ena_down(adapter); - sx_unlock(&adapter->ioctl_sx); + ENA_LOCK_UNLOCK(adapter); } } break; @@ -2005,10 +2005,10 @@ ena_ioctl(if_t ifp, u_long command, caddr_t data) if ((reinit != 0) && ((if_getdrvflags(ifp) & IFF_DRV_RUNNING) != 0)) { - sx_xlock(&adapter->ioctl_sx); + ENA_LOCK_LOCK(adapter); ena_down(adapter); rc = ena_up(adapter); - sx_unlock(&adapter->ioctl_sx); + ENA_LOCK_UNLOCK(adapter); } } @@ -3180,10 +3180,10 @@ ena_reset_task(void *arg, int pending) return; } - sx_xlock(&adapter->ioctl_sx); + ENA_LOCK_LOCK(adapter); ena_destroy_device(adapter, false); ena_restore_device(adapter); - sx_unlock(&adapter->ioctl_sx); + ENA_LOCK_UNLOCK(adapter); } /** @@ -3212,11 +3212,13 @@ ena_attach(device_t pdev) adapter = device_get_softc(pdev); adapter->pdev = pdev; - mtx_init(&adapter->global_mtx, "ENA global mtx", NULL, MTX_DEF); - sx_init(&adapter->ioctl_sx, "ENA ioctl sx"); + ENA_LOCK_INIT(adapter); - /* Set up the timer service */ - callout_init_mtx(&adapter->timer_service, &adapter->global_mtx, 0); + /* + * Set up the timer service - driver is responsible for avoiding + * concurrency, as the callout won't be using any locking inside. + */ + callout_init(&adapter->timer_service, true); adapter->keep_alive_timeout = DEFAULT_KEEP_ALIVE_TO; adapter->missing_tx_timeout = DEFAULT_TX_CMP_TO; adapter->missing_tx_max_queues = DEFAULT_TX_MONITORED_QUEUES; @@ -3435,16 +3437,20 @@ ena_detach(device_t pdev) ether_ifdetach(adapter->ifp); - /* Free reset task and callout */ + /* Stop timer service */ + ENA_LOCK_LOCK(adapter); callout_drain(&adapter->timer_service); + ENA_LOCK_UNLOCK(adapter); + + /* Release reset task */ while (taskqueue_cancel(adapter->reset_tq, &adapter->reset_task, NULL)) taskqueue_drain(adapter->reset_tq, &adapter->reset_task); taskqueue_free(adapter->reset_tq); - sx_xlock(&adapter->ioctl_sx); + ENA_LOCK_LOCK(adapter); ena_down(adapter); ena_destroy_device(adapter, true); - sx_unlock(&adapter->ioctl_sx); + ENA_LOCK_UNLOCK(adapter); #ifdef DEV_NETMAP netmap_detach(adapter->ifp); @@ -3476,8 +3482,7 @@ ena_detach(device_t pdev) ena_com_delete_host_info(ena_dev); - mtx_destroy(&adapter->global_mtx); - sx_destroy(&adapter->ioctl_sx); + ENA_LOCK_DESTROY(adapter); if_free(adapter->ifp); Modified: head/sys/dev/ena/ena.h ============================================================================== --- head/sys/dev/ena/ena.h Tue May 26 15:37:55 2020 (r361515) +++ head/sys/dev/ena/ena.h Tue May 26 15:39:41 2020 (r361516) @@ -396,8 +396,7 @@ struct ena_adapter { struct resource *memory; struct resource *registers; - struct mtx global_mtx; - struct sx ioctl_sx; + struct sx global_lock; /* MSI-X */ struct msix_entry *msix_entries; @@ -467,6 +466,12 @@ struct ena_adapter { #define ENA_RING_MTX_LOCK(_ring) mtx_lock(&(_ring)->ring_mtx) #define ENA_RING_MTX_TRYLOCK(_ring) mtx_trylock(&(_ring)->ring_mtx) #define ENA_RING_MTX_UNLOCK(_ring) mtx_unlock(&(_ring)->ring_mtx) + +#define ENA_LOCK_INIT(adapter) \ + sx_init(&(adapter)->global_lock, "ENA global lock") +#define ENA_LOCK_DESTROY(adapter) sx_destroy(&(adapter)->global_lock) +#define ENA_LOCK_LOCK(adapter) sx_xlock(&(adapter)->global_lock) +#define ENA_LOCK_UNLOCK(adapter) sx_unlock(&(adapter)->global_lock) static inline int ena_mbuf_count(struct mbuf *mbuf) { Modified: head/sys/dev/ena/ena_netmap.c ============================================================================== --- head/sys/dev/ena/ena_netmap.c Tue May 26 15:37:55 2020 (r361515) +++ head/sys/dev/ena/ena_netmap.c Tue May 26 15:39:41 2020 (r361516) @@ -277,7 +277,7 @@ ena_netmap_reg(struct netmap_adapter *na, int onoff) enum txrx t; int rc, i; - sx_xlock(&adapter->ioctl_sx); + ENA_LOCK_LOCK(adapter); ENA_FLAG_CLEAR_ATOMIC(ENA_FLAG_TRIGGER_RESET, adapter); ena_down(adapter); @@ -314,7 +314,7 @@ ena_netmap_reg(struct netmap_adapter *na, int onoff) ENA_FLAG_SET_ATOMIC(ENA_FLAG_DEV_UP_BEFORE_RESET, adapter); rc = ena_restore_device(adapter); } - sx_unlock(&adapter->ioctl_sx); + ENA_LOCK_UNLOCK(adapter); return (rc); }