From 07aff471c0de2de9a1dc5c7749c46b525bdd0201 Mon Sep 17 00:00:00 2001 From: Artur Rojek Date: Thu, 12 Aug 2021 10:34:28 +0200 Subject: ena: Share ena_global_lock between driver instances In order to use `ena_global_lock` in sysctl context, it must be kept outside the driver instance's software context, as sysctls can be called before attach and after detach, leading to lock use before sx_init and after sx_destroy otherwise. Solve this issue by turning `ena_global_lock` into a file scope variable, shared between all instances of the driver and associated sysctl context, and in turn initialized/destroyed in dedicated SYSINIT/SYSUNINIT functions. As a side effect, this change also fixes existing race in the reset routine, when simultaneously accessing sysctl exposed properties. Obtained from: Semihalf MFC after: 2 weeks Sponsored by: Amazon, Inc. --- sys/dev/ena/ena.c | 74 ++++++++++++++++++++++++------------------------ sys/dev/ena/ena.h | 17 ++++++----- sys/dev/ena/ena_netmap.c | 4 +-- sys/dev/ena/ena_sysctl.c | 62 +++++++++++++++++++++++++++++++++------- 4 files changed, 99 insertions(+), 58 deletions(-) diff --git a/sys/dev/ena/ena.c b/sys/dev/ena/ena.c index 7dff59043bb2..c1b770926b0f 100644 --- a/sys/dev/ena/ena.c +++ b/sys/dev/ena/ena.c @@ -181,6 +181,8 @@ static ena_vendor_info_t ena_vendor_info_array[] = { { 0, 0, 0 } }; +struct sx ena_global_lock; + /* * Contains pointers to event handlers, e.g. link state chage. */ @@ -1127,8 +1129,6 @@ ena_update_buf_ring_size(struct ena_adapter *adapter, int rc = 0; bool dev_was_up; - ENA_LOCK_LOCK(adapter); - old_buf_ring_size = adapter->buf_ring_size; adapter->buf_ring_size = new_buf_ring_size; @@ -1163,8 +1163,6 @@ ena_update_buf_ring_size(struct ena_adapter *adapter, } } - ENA_LOCK_UNLOCK(adapter); - return (rc); } @@ -1176,8 +1174,6 @@ ena_update_queue_size(struct ena_adapter *adapter, uint32_t new_tx_size, int rc = 0; bool dev_was_up; - ENA_LOCK_LOCK(adapter); - old_tx_size = adapter->requested_tx_ring_size; old_rx_size = adapter->requested_rx_ring_size; adapter->requested_tx_ring_size = new_tx_size; @@ -1218,8 +1214,6 @@ ena_update_queue_size(struct ena_adapter *adapter, uint32_t new_tx_size, } } - ENA_LOCK_UNLOCK(adapter); - return (rc); } @@ -1242,8 +1236,6 @@ ena_update_io_queue_nb(struct ena_adapter *adapter, uint32_t new_num) int rc = 0; bool dev_was_up; - ENA_LOCK_LOCK(adapter); - dev_was_up = ENA_FLAG_ISSET(ENA_FLAG_DEV_UP, adapter); old_num = adapter->num_io_queues; ena_down(adapter); @@ -1273,8 +1265,6 @@ ena_update_io_queue_nb(struct ena_adapter *adapter, uint32_t new_num) } } - ENA_LOCK_UNLOCK(adapter); - return (rc); } @@ -2029,7 +2019,7 @@ ena_up(struct ena_adapter *adapter) { int rc = 0; - ENA_LOCK_ASSERT(adapter); + ENA_LOCK_ASSERT(); if (unlikely(device_is_attached(adapter->pdev) == 0)) { ena_log(adapter->pdev, ERR, "device is not attached!\n"); @@ -2148,13 +2138,13 @@ ena_media_status(if_t ifp, struct ifmediareq *ifmr) struct ena_adapter *adapter = if_getsoftc(ifp); ena_log(adapter->pdev, DBG, "Media status update\n"); - ENA_LOCK_LOCK(adapter); + ENA_LOCK_LOCK(); ifmr->ifm_status = IFM_AVALID; ifmr->ifm_active = IFM_ETHER; if (!ENA_FLAG_ISSET(ENA_FLAG_LINK_UP, adapter)) { - ENA_LOCK_UNLOCK(adapter); + ENA_LOCK_UNLOCK(); ena_log(adapter->pdev, INFO, "Link is down\n"); return; } @@ -2162,7 +2152,7 @@ ena_media_status(if_t ifp, struct ifmediareq *ifmr) ifmr->ifm_status |= IFM_ACTIVE; ifmr->ifm_active |= IFM_UNKNOWN | IFM_FDX; - ENA_LOCK_UNLOCK(adapter); + ENA_LOCK_UNLOCK(); } static void @@ -2171,9 +2161,9 @@ ena_init(void *arg) struct ena_adapter *adapter = (struct ena_adapter *)arg; if (!ENA_FLAG_ISSET(ENA_FLAG_DEV_UP, adapter)) { - ENA_LOCK_LOCK(adapter); + ENA_LOCK_LOCK(); ena_up(adapter); - ENA_LOCK_UNLOCK(adapter); + ENA_LOCK_UNLOCK(); } } @@ -2195,13 +2185,13 @@ ena_ioctl(if_t ifp, u_long command, caddr_t data) case SIOCSIFMTU: if (ifp->if_mtu == ifr->ifr_mtu) break; - ENA_LOCK_LOCK(adapter); + ENA_LOCK_LOCK(); ena_down(adapter); ena_change_mtu(ifp, ifr->ifr_mtu); rc = ena_up(adapter); - ENA_LOCK_UNLOCK(adapter); + ENA_LOCK_UNLOCK(); break; case SIOCSIFFLAGS: @@ -2213,15 +2203,15 @@ ena_ioctl(if_t ifp, u_long command, caddr_t data) "ioctl promisc/allmulti\n"); } } else { - ENA_LOCK_LOCK(adapter); + ENA_LOCK_LOCK(); rc = ena_up(adapter); - ENA_LOCK_UNLOCK(adapter); + ENA_LOCK_UNLOCK(); } } else { if ((if_getdrvflags(ifp) & IFF_DRV_RUNNING) != 0) { - ENA_LOCK_LOCK(adapter); + ENA_LOCK_LOCK(); ena_down(adapter); - ENA_LOCK_UNLOCK(adapter); + ENA_LOCK_UNLOCK(); } } break; @@ -2246,10 +2236,10 @@ ena_ioctl(if_t ifp, u_long command, caddr_t data) if ((reinit != 0) && ((if_getdrvflags(ifp) & IFF_DRV_RUNNING) != 0)) { - ENA_LOCK_LOCK(adapter); + ENA_LOCK_LOCK(); ena_down(adapter); rc = ena_up(adapter); - ENA_LOCK_UNLOCK(adapter); + ENA_LOCK_UNLOCK(); } } @@ -2404,7 +2394,7 @@ ena_down(struct ena_adapter *adapter) { int rc; - ENA_LOCK_ASSERT(adapter); + ENA_LOCK_ASSERT(); if (!ENA_FLAG_ISSET(ENA_FLAG_DEV_UP, adapter)) return; @@ -3400,12 +3390,12 @@ ena_reset_task(void *arg, int pending) { struct ena_adapter *adapter = (struct ena_adapter *)arg; - ENA_LOCK_LOCK(adapter); + ENA_LOCK_LOCK(); if (likely(ENA_FLAG_ISSET(ENA_FLAG_TRIGGER_RESET, adapter))) { ena_destroy_device(adapter, false); ena_restore_device(adapter); } - ENA_LOCK_UNLOCK(adapter); + ENA_LOCK_UNLOCK(); } /** @@ -3434,8 +3424,6 @@ ena_attach(device_t pdev) adapter = device_get_softc(pdev); adapter->pdev = pdev; - ENA_LOCK_INIT(adapter); - /* * Set up the timer service - driver is responsible for avoiding * concurrency, as the callout won't be using any locking inside. @@ -3677,19 +3665,19 @@ ena_detach(device_t pdev) ether_ifdetach(adapter->ifp); /* Stop timer service */ - ENA_LOCK_LOCK(adapter); + ENA_LOCK_LOCK(); callout_drain(&adapter->timer_service); - ENA_LOCK_UNLOCK(adapter); + ENA_LOCK_UNLOCK(); /* 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); - ENA_LOCK_LOCK(adapter); + ENA_LOCK_LOCK(); ena_down(adapter); ena_destroy_device(adapter, true); - ENA_LOCK_UNLOCK(adapter); + ENA_LOCK_UNLOCK(); /* Restore unregistered sysctl queue nodes. */ ena_sysctl_update_queue_node_nb(adapter, adapter->num_io_queues, @@ -3723,8 +3711,6 @@ ena_detach(device_t pdev) ena_com_delete_host_info(ena_dev); - ENA_LOCK_DESTROY(adapter); - if_free(adapter->ifp); free(ena_dev->bus, M_DEVBUF); @@ -3790,6 +3776,20 @@ static void ena_notification(void *adapter_data, } } +static void +ena_lock_init(void *arg) +{ + ENA_LOCK_INIT(); +} +SYSINIT(ena_lock_init, SI_SUB_LOCK, SI_ORDER_FIRST, ena_lock_init, NULL); + +static void +ena_lock_uninit(void *arg) +{ + ENA_LOCK_DESTROY(); +} +SYSUNINIT(ena_lock_uninit, SI_SUB_LOCK, SI_ORDER_FIRST, ena_lock_uninit, NULL); + /** * This handler will called for unknown event group or unimplemented handlers **/ diff --git a/sys/dev/ena/ena.h b/sys/dev/ena/ena.h index c7e952f5b571..0e85cb39b001 100644 --- a/sys/dev/ena/ena.h +++ b/sys/dev/ena/ena.h @@ -401,8 +401,6 @@ struct ena_adapter { struct resource *msix; int msix_rid; - struct sx global_lock; - /* MSI-X */ struct msix_entry *msix_entries; int msix_vecs; @@ -482,17 +480,18 @@ struct ena_adapter { #define ENA_RING_MTX_ASSERT(_ring) \ mtx_assert(&(_ring)->ring_mtx, MA_OWNED) -#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) -#define ENA_LOCK_ASSERT(adapter) \ - sx_assert(&(adapter)->global_lock, SA_XLOCKED) +#define ENA_LOCK_INIT() \ + sx_init(&ena_global_lock, "ENA global lock") +#define ENA_LOCK_DESTROY() sx_destroy(&ena_global_lock) +#define ENA_LOCK_LOCK() sx_xlock(&ena_global_lock) +#define ENA_LOCK_UNLOCK() sx_unlock(&ena_global_lock) +#define ENA_LOCK_ASSERT() sx_assert(&ena_global_lock, SA_XLOCKED) #define clamp_t(type, _x, min, max) min_t(type, max_t(type, _x, min), max) #define clamp_val(val, lo, hi) clamp_t(__typeof(val), val, lo, hi) +extern struct sx ena_global_lock; + static inline int ena_mbuf_count(struct mbuf *mbuf) { int count = 1; diff --git a/sys/dev/ena/ena_netmap.c b/sys/dev/ena/ena_netmap.c index 74cdea6c34fa..1525b1efd954 100644 --- a/sys/dev/ena/ena_netmap.c +++ b/sys/dev/ena/ena_netmap.c @@ -278,7 +278,7 @@ ena_netmap_reg(struct netmap_adapter *na, int onoff) enum txrx t; int rc, i; - ENA_LOCK_LOCK(adapter); + ENA_LOCK_LOCK(); ENA_FLAG_CLEAR_ATOMIC(ENA_FLAG_TRIGGER_RESET, adapter); ena_down(adapter); @@ -315,7 +315,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); } - ENA_LOCK_UNLOCK(adapter); + ENA_LOCK_UNLOCK(); return (rc); } diff --git a/sys/dev/ena/ena_sysctl.c b/sys/dev/ena/ena_sysctl.c index c9a5419811a8..cc8dff4af0c0 100644 --- a/sys/dev/ena/ena_sysctl.c +++ b/sys/dev/ena/ena_sysctl.c @@ -442,6 +442,12 @@ ena_sysctl_buf_ring_size(SYSCTL_HANDLER_ARGS) uint32_t val; int error; + ENA_LOCK_LOCK(); + if (unlikely(!ENA_FLAG_ISSET(ENA_FLAG_DEVICE_RUNNING, adapter))) { + error = EINVAL; + goto unlock; + } + val = 0; error = sysctl_wire_old_buffer(req, sizeof(val)); if (error == 0) { @@ -449,13 +455,14 @@ ena_sysctl_buf_ring_size(SYSCTL_HANDLER_ARGS) error = sysctl_handle_32(oidp, &val, 0, req); } if (error != 0 || req->newptr == NULL) - return (error); + goto unlock; if (!powerof2(val) || val == 0) { ena_log(adapter->pdev, ERR, "Requested new Tx buffer ring size (%u) is not a power of 2\n", val); - return (EINVAL); + error = EINVAL; + goto unlock; } if (val != adapter->buf_ring_size) { @@ -470,6 +477,9 @@ ena_sysctl_buf_ring_size(SYSCTL_HANDLER_ARGS) adapter->buf_ring_size); } +unlock: + ENA_LOCK_UNLOCK(); + return (error); } @@ -480,6 +490,12 @@ ena_sysctl_rx_queue_size(SYSCTL_HANDLER_ARGS) uint32_t val; int error; + ENA_LOCK_LOCK(); + if (unlikely(!ENA_FLAG_ISSET(ENA_FLAG_DEVICE_RUNNING, adapter))) { + error = EINVAL; + goto unlock; + } + val = 0; error = sysctl_wire_old_buffer(req, sizeof(val)); if (error == 0) { @@ -487,13 +503,14 @@ ena_sysctl_rx_queue_size(SYSCTL_HANDLER_ARGS) error = sysctl_handle_32(oidp, &val, 0, req); } if (error != 0 || req->newptr == NULL) - return (error); + goto unlock; if (val < ENA_MIN_RING_SIZE || val > adapter->max_rx_ring_size) { ena_log(adapter->pdev, ERR, "Requested new Rx queue size (%u) is out of range: [%u, %u]\n", val, ENA_MIN_RING_SIZE, adapter->max_rx_ring_size); - return (EINVAL); + error = EINVAL; + goto unlock; } /* Check if the parameter is power of 2 */ @@ -501,7 +518,8 @@ ena_sysctl_rx_queue_size(SYSCTL_HANDLER_ARGS) ena_log(adapter->pdev, ERR, "Requested new Rx queue size (%u) is not a power of 2\n", val); - return (EINVAL); + error = EINVAL; + goto unlock; } if (val != adapter->requested_rx_ring_size) { @@ -517,6 +535,9 @@ ena_sysctl_rx_queue_size(SYSCTL_HANDLER_ARGS) adapter->requested_rx_ring_size); } +unlock: + ENA_LOCK_UNLOCK(); + return (error); } @@ -530,18 +551,25 @@ ena_sysctl_io_queues_nb(SYSCTL_HANDLER_ARGS) uint32_t old_num_queues, tmp = 0; int error; + ENA_LOCK_LOCK(); + if (unlikely(!ENA_FLAG_ISSET(ENA_FLAG_DEVICE_RUNNING, adapter))) { + error = EINVAL; + goto unlock; + } + error = sysctl_wire_old_buffer(req, sizeof(tmp)); if (error == 0) { tmp = adapter->num_io_queues; error = sysctl_handle_int(oidp, &tmp, 0, req); } if (error != 0 || req->newptr == NULL) - return (error); + goto unlock; if (tmp == 0) { ena_log(adapter->pdev, ERR, "Requested number of IO queues is zero\n"); - return (EINVAL); + error = EINVAL; + goto unlock; } /* @@ -555,7 +583,8 @@ ena_sysctl_io_queues_nb(SYSCTL_HANDLER_ARGS) ena_log(adapter->pdev, ERR, "Requested number of IO queues is higher than maximum " "allowed (%u)\n", adapter->msix_vecs - ENA_ADMIN_MSIX_VEC); - return (EINVAL); + error = EINVAL; + goto unlock; } if (tmp == adapter->num_io_queues) { ena_log(adapter->pdev, ERR, @@ -574,6 +603,9 @@ ena_sysctl_io_queues_nb(SYSCTL_HANDLER_ARGS) ena_sysctl_update_queue_node_nb(adapter, old_num_queues, tmp); } +unlock: + ENA_LOCK_UNLOCK(); + return (error); } @@ -584,19 +616,26 @@ ena_sysctl_eni_metrics_interval(SYSCTL_HANDLER_ARGS) uint16_t interval; int error; + ENA_LOCK_LOCK(); + if (unlikely(!ENA_FLAG_ISSET(ENA_FLAG_DEVICE_RUNNING, adapter))) { + error = EINVAL; + goto unlock; + } + error = sysctl_wire_old_buffer(req, sizeof(interval)); if (error == 0) { interval = adapter->eni_metrics_sample_interval; error = sysctl_handle_16(oidp, &interval, 0, req); } if (error != 0 || req->newptr == NULL) - return (error); + goto unlock; if (interval > ENI_METRICS_MAX_SAMPLE_INTERVAL) { ena_log(adapter->pdev, ERR, "ENI metrics update interval is out of range - maximum allowed value: %d seconds\n", ENI_METRICS_MAX_SAMPLE_INTERVAL); - return (EINVAL); + error = EINVAL; + goto unlock; } if (interval == 0) { @@ -611,5 +650,8 @@ ena_sysctl_eni_metrics_interval(SYSCTL_HANDLER_ARGS) adapter->eni_metrics_sample_interval = interval; +unlock: + ENA_LOCK_UNLOCK(); + return (0); } -- cgit v1.2.3