Re: [Xen-devel] [DOC v1] Xen transport for 9pfs

[Stefano Stabellini <stefano@xxxxxxxxxxx>]

On Wed, 30 Nov 2016, David Vrabel wrote:
> On 29/11/16 23:34, Stefano Stabellini wrote:
> > 
> > The producer (the backend for **in**, the frontend for **out**) writes to 
> > the
> > array in the following way:
> > 
> - read memory barrier
> > - read *cons*, *prod* from shared memory
> > - write to array at position *prod* up to *cons*, wrapping around the 
> > circular
> >   buffer when necessary
> - write memory barrier
> > - increase *prod*
> > - notify the other end via evtchn
> > 
> > The consumer (the backend for **out**, the frontend for **in**) reads from 
> > the
> > array in the following way:
> 
> - read memory barrier
> > - read *prod*, *cons* from shared memory
> > - read from array at position *cons* up to *prod*, wrapping around the 
> > circular
> >   buffer when necessary
> > - memory barrier
> > - increase *cons*
> > - notify the other end via evtchn
> 
> Your barriers are wrong (see corrections above).

Thanks for the review. Sorry for not specifying the type of memory
barriers. I agree with some of your suggestions, but I don't understand
why you moved the read memory barrier before reading prod and cons.

Shouldn't it be for a producer:
  - read *cons*, *prod* from shared memory
  - read memory barrier
  - write to array at position *prod* up to *cons*, wrapping around the circular
    buffer when necessary
  - write memory barrier
  - increase *prod*
  - notify the other end via evtchn
 
and for a consumer:
  - read *prod*, *cons* from shared memory
  - read memory barrier
  - read from array at position *cons* up to *prod*, wrapping around the 
circular
    buffer when necessary
  - general memory barrier
  - increase *cons*
  - notify the other end via evtchn

If this is wrong, could you please explain why?


> I think you should use a private copy of cons/prod in the
> consumer/producer and use this to validate that the shared prod/cons is
> within a sensible range.

With the masking macro provided (MASK_XEN_9PFS_IDX), indices cannot go
out of range:

#define MASK_XEN_9PFS_IDX(idx) ((idx) & (XEN_9PFS_RING_SIZE - 1))

That said, it could be a cheap additional validation.


> You're missing a mechanism to omit unnecessary evtchn notifications
> (like the standard ring protocol has).

Yes, indeed. I'll see what I can do.


> This all looks like a generic "transfer byte stream" mechanism which
> could be usefully made generic and not specific to 9pfs.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel