Re: [PATCH v3] xenbus: support large messages

[Samuel Thibault <samuel.thibault@xxxxxxxxxxxx>]

Re,

Juergen Gross, le mer. 15 sept. 2021 16:08:15 +0200, a ecrit:
> +    while (off != len)
> +    {
> +        wait_event(xb_waitq, xenstore_buf->rsp_prod != 
> xenstore_buf->rsp_cons);
> +
> +        prod = xenstore_buf->rsp_prod;
> +        cons = xenstore_buf->rsp_cons;
> +        DEBUG("Rsp_cons %d, rsp_prod %d.\n", cons, prod);
> +        size = min(len - off, prod - cons);
> +
> +        rmb();   /* Make sure data read from ring is ordered with rsp_prod. 
> */
> +        memcpy_from_ring(xenstore_buf->rsp, buf + off,
> +                         MASK_XENSTORE_IDX(cons), size);
> +        off += size;
> +        xenstore_buf->rsp_cons += size;
> +        wmb();
> +        if (xenstore_buf->rsp_prod - cons >= XENSTORE_RING_SIZE)
> +            notify_remote_via_evtchn(xenbus_evtchn);
> +    }

Reading it again, I'm still not convinced we covered all cases. There is
at least one thing: Linux does

        virt_rmb();
        memcpy(data, src, avail);
        /* Other side must not see free space until we've copied out */
        virt_mb();
        intf->rsp_cons += avail;

which makes sense to me: we don't want the compiler or anything to
reorder the write to rsp_cons respectively to the memcpy. So I believe
we also need a full barrier before 

        xenstore_buf->rsp_cons += size;

in mini-os.


Then there is

        xenstore_buf->rsp_cons += size;
        wmb();
        if (xenstore_buf->rsp_prod - cons >= XENSTORE_RING_SIZE)
            notify_remote_via_evtchn(xenbus_evtchn);

The Linux code does

        intf->rsp_cons += avail;
        
        /* Implies mb(): other side will see the updated consumer. */
        if (intf->rsp_prod - cons >= XENSTORE_RING_SIZE)
                notify_remote_via_evtchn(xen_store_evtchn);

(Note that the "Implies mb()" comment is about the notify_remote_via_evtchn 
call)

I believe the Linux code itself is missing a full barrier here: before
reading intf->rsp_prod, we want to make sure that our update of rsp_cons
is seen by the producer. Otherwise it may happen that the producer
doesn't see the rsp_cons and updates its rsp_prod (filling the ring
and going to sleep), and the consumer does not see the rsp_prod update
either, and thus we miss a notification and the producer stays stuck.

Extremely rare scenario etc. but I believe we do want a full barrier
between rsp_cons += and the if, both in mini-os and Linux.

Samuel