Module Name:    src
Committed By:   riastradh
Date:           Thu Feb 23 03:01:22 UTC 2023

Modified Files:
        src/sys/kern: subr_pcq.c

Log Message:
pcq(9): Fix consume operation in pcq_peek/get.

These use atomic_load_consume to match the atomic_store_release in
pcq_put for pcq->pcq_items[c].

Reading the snapshot of pcq->pcq_pc need not be ordered:

- The consumer side (pcq_peek/get) is serialized by the API contract
  (single-consumer, multi-producer), so no ordering is necessary.

- The producer side updates pcq->pcq_pc first; if the consumer side
  sees that before the producer side has stored pcq->pcq_items[c],
  there's no problem -- it's as if the consumer had happened just a
  moment earlier and the producer hadn't entered pcq_put yet.

However, it should be an atomic load, not a plain load.  So use
atomic_load_relaxed, if for no other reason than to pacify thread
sanitizers.

XXX pullup-9
XXX pullup-10


To generate a diff of this commit:
cvs rdiff -u -r1.14 -r1.15 src/sys/kern/subr_pcq.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.

Modified files:

Index: src/sys/kern/subr_pcq.c
diff -u src/sys/kern/subr_pcq.c:1.14 src/sys/kern/subr_pcq.c:1.15
--- src/sys/kern/subr_pcq.c:1.14	Thu Feb 23 03:00:53 2023
+++ src/sys/kern/subr_pcq.c	Thu Feb 23 03:01:22 2023
@@ -1,4 +1,4 @@
-/*	$NetBSD: subr_pcq.c,v 1.14 2023/02/23 03:00:53 riastradh Exp $	*/
+/*	$NetBSD: subr_pcq.c,v 1.15 2023/02/23 03:01:22 riastradh Exp $	*/
 
 /*-
  * Copyright (c) 2009, 2019 The NetBSD Foundation, Inc.
@@ -34,7 +34,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: subr_pcq.c,v 1.14 2023/02/23 03:00:53 riastradh Exp $");
+__KERNEL_RCSID(0, "$NetBSD: subr_pcq.c,v 1.15 2023/02/23 03:01:22 riastradh Exp $");
 
 #include <sys/param.h>
 #include <sys/types.h>
@@ -100,7 +100,7 @@ pcq_put(pcq_t *pcq, void *item)
 	KASSERT(item != NULL);
 
 	do {
-		v = pcq->pcq_pc;
+		v = atomic_load_relaxed(&pcq->pcq_pc);
 		pcq_split(v, &op, &c);
 		p = pcq_advance(pcq, op);
 		if (p == c) {
@@ -132,14 +132,13 @@ pcq_put(pcq_t *pcq, void *item)
 void *
 pcq_peek(pcq_t *pcq)
 {
-	const uint32_t v = pcq->pcq_pc;
+	const uint32_t v = atomic_load_relaxed(&pcq->pcq_pc);
 	u_int p, c;
 
 	pcq_split(v, &p, &c);
 
 	/* See comment on race below in pcq_get(). */
-	return (p == c) ? NULL :
-	    (membar_datadep_consumer(), pcq->pcq_items[c]);
+	return (p == c) ? NULL : atomic_load_consume(&pcq->pcq_items[c]);
 }
 
 /*
@@ -154,15 +153,13 @@ pcq_get(pcq_t *pcq)
 	u_int p, c;
 	void *item;
 
-	v = pcq->pcq_pc;
+	v = atomic_load_relaxed(&pcq->pcq_pc);
 	pcq_split(v, &p, &c);
 	if (p == c) {
 		/* Queue is empty: nothing to return. */
 		return NULL;
 	}
-	/* Make sure we read pcq->pcq_pc before pcq->pcq_items[c].  */
-	membar_datadep_consumer();
-	item = pcq->pcq_items[c];
+	item = atomic_load_consume(&pcq->pcq_items[c]);
 	if (item == NULL) {
 		/*
 		 * Raced with sender: we rely on a notification (e.g. softint
@@ -185,7 +182,7 @@ pcq_get(pcq_t *pcq)
 	membar_producer();
 #endif
 	while (__predict_false(atomic_cas_32(&pcq->pcq_pc, v, nv) != v)) {
-		v = pcq->pcq_pc;
+		v = atomic_load_relaxed(&pcq->pcq_pc);
 		pcq_split(v, &p, &c);
 		c = pcq_advance(pcq, c);
 		nv = pcq_combine(p, c);

Reply via email to