From nobody Mon Aug 19 11:03:27 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 4WnV804MRXz5Sp3k; Mon, 19 Aug 2024 11:03:28 +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 4WnV800lV8z4S4g; Mon, 19 Aug 2024 11:03:28 +0000 (UTC) (envelope-from git@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1724065408; 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=Q2qKiVXgt2UGnU1IZ/WjHqpqFxnoShxJVjCCCDLXbhA=; b=vSoA5Q2miQ0XiA0xc4s72lv5a+XCnei8inW7il62XNzNUcNj868pJ5hSeloBIkY/LgkDnX Z54yWUExS6CbvoyxSpoBywllSK5S8aLl2z9kHQQ8+OxEF8er/yeC3PhoWHIiEC7mdvuiqY LS/MHUh2rSWcOPEFq5AetEuOzZsMBPwPfmf/oFrtCtIAVzlCFLtu5h8zfrYo9sd1DhESXt UOKBr/rw67vvl7pg8I20hkwpzujKyZG+UX0hhSTxAthBktJqPDGJ9qhIqsF0hXmDwjeLvM NRoW1Fb1cYuyKkOZyVkhO/5XTkmASjqytGM2ArVS661Y622XcJRNp8vuEpvW9g== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1724065408; a=rsa-sha256; cv=none; b=lNavcJKwOVugYAsIHJSEwBi+qcGCYAeEUjSrajpV2RSZAQC9MyUGNrm4+thpcD3L6BCBnc BNzI8HvzGyRzukAXMCctSe+V0goCXhRquu37GTTg7KODC2+t4PO+0XK5l6CfNd93LFSb9t g97YHvjJZ4QN6wr6uSROHOL4fVlxlrsnAWnOXx7FH/mRpaS2H8ARGWC9Z0fFkhdESU0a5M vcBOpI2IZNKH0Y41EmcEZ6GnRGiT0KdO58OsUMxW9N5EgnByV/q1vC68OE8Sr8rflB0QI/ 26g8fyfiiV4vfbuKU8yxVDtO79tSGAF/qGWUge2uGFiOka0pC/N2I3X9+Rl53g== 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=1724065408; 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=Q2qKiVXgt2UGnU1IZ/WjHqpqFxnoShxJVjCCCDLXbhA=; b=Ts+Zct1aQaJ4iKPny4y2+pbM+KLziLPFlBBx3p091FUe4+GBIsmY3VKwUQiwGe1FlDpp1m c+eIeoUASCZ/TOTgI0ra7/mZAyTvGTEJSCVh84QEjV8IEfkGYOkdVb7DZ17fdb+WhMAo0y GTAe7JR+IsuMH7Hb+uOzmIbw5fOQI8q8B58rfWDcm0gPs8TrIvs/mkXUPzulnCyUeIE5QT Bab3SSLdTTyB+YimiXafNHUP6iT2DBtorErVMLEQpJwIZmtW/BGhMat9GfyM1wyyoOzDqc p+z+pz1WmixcpBut6+7iZM/hQZfpsNUd/HFYgmww3wXcPsD3aR5FIBkHT0Flig== 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 4WnV800HKTzF9m; Mon, 19 Aug 2024 11:03:28 +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 47JB3RvH037590; Mon, 19 Aug 2024 11:03:27 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.18.1/8.18.1/Submit) id 47JB3RwZ037587; Mon, 19 Aug 2024 11:03:27 GMT (envelope-from git) Date: Mon, 19 Aug 2024 11:03:27 GMT Message-Id: <202408191103.47JB3RwZ037587@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org From: Andrew Turner Subject: git: 3cc603909e09 - main - 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/main X-Git-Reftype: branch X-Git-Commit: 3cc603909e09c958e20dd5a8a341f62f29e33a07 Auto-Submitted: auto-generated The branch main has been updated by andrew: URL: https://cgit.FreeBSD.org/src/commit/?id=3cc603909e09c958e20dd5a8a341f62f29e33a07 commit 3cc603909e09c958e20dd5a8a341f62f29e33a07 Author: Andrew Turner AuthorDate: 2024-08-19 09:06:44 +0000 Commit: Andrew Turner CommitDate: 2024-08-19 10:53:11 +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 --- 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