Skip site navigation (1)Skip section navigation (2) 
Date:      Tue, 26 May 2020 15:39:41 +0000 (UTC)
From:      Marcin Wojtas <mw@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r361516 - head/sys/dev/ena
Message-ID:  <202005261539.04QFdfkJ064351@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help 
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 <mk@semihalf.com>
  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);
 }

Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202005261539.04QFdfkJ064351>