Re: [PATCH v2 8/9] async: update documentation of the memory barriers

[Richard Henderson]

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~