On 3/7/23 02:49, Paolo Bonzini wrote:
On 3/7/23 00:39, Richard Henderson wrote:
On 3/6/23 14:33, Paolo Bonzini wrote:
@@ -107,11 +114,8 @@ static QEMUBH *aio_bh_dequeue(BHList *head, unsigned
*flags)
QSLIST_REMOVE_HEAD(head, next);
/*
- * The qatomic_and is paired with aio_bh_enqueue(). The implicit memory
- * barrier ensures that the callback sees all writes done by the scheduling
- * thread. It also ensures that the scheduling thread sees the cleared
- * flag before bh->cb has run, and thus will call aio_notify again if
- * necessary.
+ * Synchronizes with qatomic_fetch_or() in aio_bh_enqueue(), ensuring that
+ * the removal finishes before BH_PENDING is reset.
*/
*flags = qatomic_fetch_and(&bh->flags,
Per this new comment, about the remove finishing first, it would seem that we need
smp_mb__before_rmw here, because QSLIST_REMOVE_HEAD is not SEQCST.
Only release-acquire is needed for consistent access to the list, seqcst and
smp_mb__before/after_rmw() are only needed (as expected) to handle wakeup.
Just to be safe, I tried modeling this with cppmem
(http://svr-pes20-cppmem.cl.cam.ac.uk/cppmem/); support for compare-and-swap is very
limited, therefore the test looks nothing like the C code(*), but it should be ok:
You do realize that QSLIST_REMOVE_HEAD is not a compare-and-swap, right?
#define QSLIST_REMOVE_HEAD(head, field) do { \
typeof((head)->slh_first) elm = (head)->slh_first; \
(head)->slh_first = elm->field.sle_next; \
elm->field.sle_next = NULL; \
} while (/*CONSTCOND*/0)
As I read aio_bh_queue, this is exactly the situation you describe in patch 1 justifying
the introduction of the new barriers.
r~
