Re: [PATCH] mini-os: xenbus: support large messages

[Juergen Gross <jgross@xxxxxxxx>]

On 15.09.21 01:17, Samuel Thibault wrote:
> Hello,
>
> Thanks for having worked on this!
>
> Juergen Gross, le mer. 18 août 2021 17:26:10 +0200, a ecrit:
> > +static void xenbus_read_data(char *buf, unsigned int len)
> > +{
> > +    unsigned int off = 0;
> > +    unsigned int prod;
> > +    unsigned int size;
> > +    int notify;
> > +
> > +    while (off != len)
> > +    {
> > +        if (xenstore_buf->rsp_prod == xenstore_buf->rsp_cons)
> > +            wait_event(xb_waitq,
> > +                       xenstore_buf->rsp_prod != xenstore_buf->rsp_cons);
> The if is redundant since wait_event already tests it.
Ah, right.

> > +        prod = xenstore_buf->rsp_prod;
> > +        DEBUG("Rsp_cons %d, rsp_prod %d.\n", xenstore_buf->rsp_cons, prod);
> > +        size = min(len - off, prod - xenstore_buf->rsp_cons);
> > +        memcpy_from_ring(xenstore_buf->rsp, buf + off,
> > +                         MASK_XENSTORE_IDX(xenstore_buf->rsp_cons), size);
> > +        off += size;
> > +        notify = (xenstore_buf->rsp_cons + XENSTORE_RING_SIZE ==
> > +                  xenstore_buf->rsp_prod);
> This looks odd to me?  We want to notify as soon as the ring is empty,
> which can happen at any place in the ring right?
No, we want to notify if the ring was full and is about to gain some
space again, as the other side was probably not able to put all data
in and is now waiting for more space to become available.

> Linux' code uses (intf->rsp_prod - cons >= XENSTORE_RING_SIZE), *after*
> the rsp_cons increase.
>
> > +        rmb();
> rmb() must be placed before memcpy_from_ring, to make sure that the data
> we read from the buffer is up-to-date according to the read we made from
> rsp_prod.
Ah, yes. Thanks for spotting that one!

> The rest seems ok to me.
Thanks,


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature