From nobody Mon Sep 02 08:50:52 2024 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4Wy2XY0cwwz5Mp21; Mon, 02 Sep 2024 08:50:53 +0000 (UTC) (envelope-from git@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 "R11" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4Wy2XX4XG2z42KY; Mon, 2 Sep 2024 08:50:52 +0000 (UTC) (envelope-from git@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1725267052; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=aOMO3ZOq0qcGEScm3ooxL+ZR68Fehsvhi+5f0Agj9dw=; b=iBTFBSPMQwmkQdfBQIfv/3mYK8cXFraZl73JvFDUvzV9bl6DIpcYV/3dwvL7NSeYQnLWJz EcjlevDBerXiU1EcCXCjLbmTPNDHF8l52IrZmT2Tcb+k60R2eqD6eL63TIS7eqgpJ99HeU tXMtiS6jhjAJKWjKzdaOO/xJpkuyT4jeIAIwe/Q54KM3ZSX3sCs6xRu3khvGqX+D2g0hGi 1EdSALog/kVK6Cork4fjJWDCf5VqffSYOjwiAc8AkxNFDei+f/M2ri30C78QKpANWYlx26 bKyCcr/aALbqN65Q/menBnd8C+w/iNC8javwnLggbXbmgl3nQSreNrMNJ2NVpw== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1725267052; a=rsa-sha256; cv=none; b=xvKlR2zwcVsUskPiwEI/TvBU+kfGUV3gEqvllNGulRR3ejHJaLfmbLtRzrsiLVr3TDUZpS 8b9IaDYzWhh/rud2H+q28oTQv5kh2hYdkjYArol9vwyagDWepLV868BIiiYJhgCuLuzxpn KiD4rm9IxKKj6NlPOpkAs4sxRXxPLzFMVzujcz+GIIhcrQRzCYBTFMBeABYH4rvAaUuZl4 3nrXDcHDOJCN8T6lDrMxKfL9i5/lPV01UCbGXJ848vkgbf0zCB9U+RQNzO92BfisRw46BC 6Ur++zcnBQfsjfQrZTYKkiEgsa36Mrix4IQo4i4eUXCXlDjXFJjgTMn9UtF8Bg== ARC-Authentication-Results: i=1; mx1.freebsd.org; none ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1725267052; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=aOMO3ZOq0qcGEScm3ooxL+ZR68Fehsvhi+5f0Agj9dw=; b=px9DjQ1OVrC4a0Hz4jhQzj+PkrKbk9bMSq2TnWi5sT/JQiQ8c3thQO4mnGZNvkaAfkq7Ba lXvjFd7gBR3JZXJ2IPvHKmF9oA253UjJNndI9ZZa8dfapSPivV6qK7tKyPoPOQN4v+N1e/ SAKB90aV5pQNTfuEe+sNS4baDy6iPTN02XMh5Prs6PKAxaloyZL1ldeMMTDJqb+MUWmttq F+7BD9LRm4GxQKLYLSPgf/l6c9ekzkU3Ddu4zQ/aupqO+1MDEl1jHM/vJTpO/kMU9Uiyku BMgNkLFt7/ugbPhyvsMSvacoR7myre+vyS3oUXxbo2HqqssNrmwOtMOF0Y1/Zg== Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) (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 did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 4Wy2XX43gDz14qP; Mon, 2 Sep 2024 08:50:52 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.18.1/8.18.1) with ESMTP id 4828oqlE007554; Mon, 2 Sep 2024 08:50:52 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.18.1/8.18.1/Submit) id 4828oq8x007551; Mon, 2 Sep 2024 08:50:52 GMT (envelope-from git) Date: Mon, 2 Sep 2024 08:50:52 GMT Message-Id: <202409020850.4828oq8x007551@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org From: Andrew Turner Subject: git: 56e8c3ec5241 - stable/14 - buf_ring: Keep the full head and tail values List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-BeenThere: dev-commits-src-all@freebsd.org Sender: owner-dev-commits-src-all@FreeBSD.org MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: andrew X-Git-Repository: src X-Git-Refname: refs/heads/stable/14 X-Git-Reftype: branch X-Git-Commit: 56e8c3ec5241ec4747b8fdd42e0172325c7fc378 Auto-Submitted: auto-generated The branch stable/14 has been updated by andrew: URL: https://cgit.FreeBSD.org/src/commit/?id=56e8c3ec5241ec4747b8fdd42e0172325c7fc378 commit 56e8c3ec5241ec4747b8fdd42e0172325c7fc378 Author: Andrew Turner AuthorDate: 2024-08-19 09:06:44 +0000 Commit: Andrew Turner CommitDate: 2024-09-02 08:49:38 +0000 buf_ring: Keep the full head and tail values If a thread reads the head but then sleeps for long enough that another thread fills the ring and leaves the new head with the expected value then the cmpset can pass when it should have failed. To work around this keep the full head and tail value and use the upper bits as a generation count. Reviewed by: kib Sponsored by: Arm Ltd Differential Revision: https://reviews.freebsd.org/D46151 (cherry picked from commit 3cc603909e09c958e20dd5a8a341f62f29e33a07) --- sys/sys/buf_ring.h | 87 +++++++++++++++++++++++++++++++++++------------------- 1 file changed, 56 insertions(+), 31 deletions(-) diff --git a/sys/sys/buf_ring.h b/sys/sys/buf_ring.h index 47ed04f570c1..dec0f971ae44 100644 --- a/sys/sys/buf_ring.h +++ b/sys/sys/buf_ring.h @@ -40,6 +40,15 @@ #include #endif +/* + * We only apply the mask to the head and tail values when calculating the + * index into br_ring to access. This means the upper bits can be used as + * epoch to reduce the chance the atomic_cmpset succeedes when it should + * fail, e.g. when the head wraps while the CPU is in an interrupt. This + * is a probablistic fix as there is still a very unlikely chance the + * value wraps back to the expected value. + * + */ struct buf_ring { volatile uint32_t br_prod_head; volatile uint32_t br_prod_tail; @@ -63,28 +72,28 @@ struct buf_ring { static __inline int buf_ring_enqueue(struct buf_ring *br, void *buf) { - uint32_t prod_head, prod_next, cons_tail; -#ifdef DEBUG_BUFRING - int i; + uint32_t prod_head, prod_next, prod_idx; + uint32_t cons_tail, mask; + mask = br->br_prod_mask; +#ifdef DEBUG_BUFRING /* * Note: It is possible to encounter an mbuf that was removed * via drbr_peek(), and then re-added via drbr_putback() and * trigger a spurious panic. */ - for (i = br->br_cons_head; i != br->br_prod_head; - i = ((i + 1) & br->br_cons_mask)) - if (br->br_ring[i] == buf) + for (uint32_t i = br->br_cons_head; i != br->br_prod_head; i++) + if (br->br_ring[i & mask] == buf) panic("buf=%p already enqueue at %d prod=%d cons=%d", buf, i, br->br_prod_tail, br->br_cons_tail); #endif critical_enter(); do { prod_head = br->br_prod_head; - prod_next = (prod_head + 1) & br->br_prod_mask; + prod_next = prod_head + 1; cons_tail = br->br_cons_tail; - if (prod_next == cons_tail) { + if ((int32_t)(cons_tail + br->br_prod_size - prod_next) < 1) { rmb(); if (prod_head == br->br_prod_head && cons_tail == br->br_cons_tail) { @@ -95,11 +104,12 @@ buf_ring_enqueue(struct buf_ring *br, void *buf) continue; } } while (!atomic_cmpset_acq_32(&br->br_prod_head, prod_head, prod_next)); + prod_idx = prod_head & mask; #ifdef DEBUG_BUFRING - if (br->br_ring[prod_head] != NULL) + if (br->br_ring[prod_idx] != NULL) panic("dangling value in enqueue"); #endif - br->br_ring[prod_head] = buf; + br->br_ring[prod_idx] = buf; /* * If there are other enqueues in progress @@ -120,23 +130,26 @@ buf_ring_enqueue(struct buf_ring *br, void *buf) static __inline void * buf_ring_dequeue_mc(struct buf_ring *br) { - uint32_t cons_head, cons_next; + uint32_t cons_head, cons_next, cons_idx; + uint32_t mask; void *buf; critical_enter(); + mask = br->br_cons_mask; do { cons_head = br->br_cons_head; - cons_next = (cons_head + 1) & br->br_cons_mask; + cons_next = cons_head + 1; if (cons_head == br->br_prod_tail) { critical_exit(); return (NULL); } } while (!atomic_cmpset_acq_32(&br->br_cons_head, cons_head, cons_next)); + cons_idx = cons_head & mask; - buf = br->br_ring[cons_head]; + buf = br->br_ring[cons_idx]; #ifdef DEBUG_BUFRING - br->br_ring[cons_head] = NULL; + br->br_ring[cons_idx] = NULL; #endif /* * If there are other dequeues in progress @@ -160,8 +173,8 @@ buf_ring_dequeue_mc(struct buf_ring *br) static __inline void * buf_ring_dequeue_sc(struct buf_ring *br) { - uint32_t cons_head, cons_next; - uint32_t prod_tail; + uint32_t cons_head, cons_next, cons_idx; + uint32_t prod_tail, mask; void *buf; /* @@ -189,6 +202,7 @@ buf_ring_dequeue_sc(struct buf_ring *br) * * <1> Load (on core 1) from br->br_ring[cons_head] can be reordered (speculative readed) by CPU. */ + mask = br->br_cons_mask; #if defined(__arm__) || defined(__aarch64__) cons_head = atomic_load_acq_32(&br->br_cons_head); #else @@ -196,16 +210,17 @@ buf_ring_dequeue_sc(struct buf_ring *br) #endif prod_tail = atomic_load_acq_32(&br->br_prod_tail); - cons_next = (cons_head + 1) & br->br_cons_mask; + cons_next = cons_head + 1; - if (cons_head == prod_tail) + if (cons_head == prod_tail) return (NULL); + cons_idx = cons_head & mask; br->br_cons_head = cons_next; - buf = br->br_ring[cons_head]; + buf = br->br_ring[cons_idx]; #ifdef DEBUG_BUFRING - br->br_ring[cons_head] = NULL; + br->br_ring[cons_idx] = NULL; #ifdef _KERNEL if (!mtx_owned(br->br_lock)) panic("lock not held on single consumer dequeue"); @@ -226,18 +241,21 @@ buf_ring_dequeue_sc(struct buf_ring *br) static __inline void buf_ring_advance_sc(struct buf_ring *br) { - uint32_t cons_head, cons_next; - uint32_t prod_tail; + uint32_t cons_head, cons_next, prod_tail; +#ifdef DEBUG_BUFRING + uint32_t mask; + mask = br->br_cons_mask; +#endif cons_head = br->br_cons_head; prod_tail = br->br_prod_tail; - cons_next = (cons_head + 1) & br->br_cons_mask; - if (cons_head == prod_tail) + cons_next = cons_head + 1; + if (cons_head == prod_tail) return; br->br_cons_head = cons_next; #ifdef DEBUG_BUFRING - br->br_ring[cons_head] = NULL; + br->br_ring[cons_head & mask] = NULL; #endif br->br_cons_tail = cons_next; } @@ -261,9 +279,12 @@ buf_ring_advance_sc(struct buf_ring *br) static __inline void buf_ring_putback_sc(struct buf_ring *br, void *new) { - KASSERT(br->br_cons_head != br->br_prod_tail, + uint32_t mask; + + mask = br->br_cons_mask; + KASSERT((br->br_cons_head & mask) != (br->br_prod_tail & mask), ("Buf-Ring has none in putback")) ; - br->br_ring[br->br_cons_head] = new; + br->br_ring[br->br_cons_head & mask] = new; } /* @@ -274,11 +295,13 @@ buf_ring_putback_sc(struct buf_ring *br, void *new) static __inline void * buf_ring_peek(struct buf_ring *br) { + uint32_t mask; #if defined(DEBUG_BUFRING) && defined(_KERNEL) if ((br->br_lock != NULL) && !mtx_owned(br->br_lock)) panic("lock not held on single consumer dequeue"); #endif + mask = br->br_cons_mask; /* * I believe it is safe to not have a memory barrier * here because we control cons and tail is worst case @@ -288,12 +311,13 @@ buf_ring_peek(struct buf_ring *br) if (br->br_cons_head == br->br_prod_tail) return (NULL); - return (br->br_ring[br->br_cons_head]); + return (br->br_ring[br->br_cons_head & mask]); } static __inline void * buf_ring_peek_clear_sc(struct buf_ring *br) { + uint32_t mask; void *ret; #if defined(DEBUG_BUFRING) && defined(_KERNEL) @@ -301,6 +325,7 @@ buf_ring_peek_clear_sc(struct buf_ring *br) panic("lock not held on single consumer dequeue"); #endif + mask = br->br_cons_mask; if (br->br_cons_head == br->br_prod_tail) return (NULL); @@ -318,13 +343,13 @@ buf_ring_peek_clear_sc(struct buf_ring *br) atomic_thread_fence_acq(); #endif - ret = br->br_ring[br->br_cons_head]; + ret = br->br_ring[br->br_cons_head & mask]; #ifdef DEBUG_BUFRING /* * Single consumer, i.e. cons_head will not move while we are * running, so atomic_swap_ptr() is not necessary here. */ - br->br_ring[br->br_cons_head] = NULL; + br->br_ring[br->br_cons_head & mask] = NULL; #endif return (ret); } @@ -333,7 +358,7 @@ static __inline int buf_ring_full(struct buf_ring *br) { - return (((br->br_prod_head + 1) & br->br_prod_mask) == br->br_cons_tail); + return (br->br_prod_head == br->br_cons_tail + br->br_cons_size - 1); } static __inline int