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

[Stefano Stabellini <stefano@xxxxxxxxxxx>]

On Fri, 2 Dec 2016, David Vrabel wrote:
> On 02/12/16 00:29, Stefano Stabellini wrote:
> > 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
> 
> Barriers must be paired to enforce the required ordering. So where you
> have a barrier before writing cons, you need another barrier before
> reading cons on the other side.
> 
> This barrier here does nothing because the following access is a write
> and there's a data dependency here anyway.

You are right that this should be a general memory barrier as the
following is a write. You are also right that it looks superfluous
because there is a data dependency, but it is necessary to match the
barrier on the consumer side, see below.


> Without a barrier P before reading cons it can see the value after C as
> written cons but before C has actually read the requests.
> 
> The Linux barrier documentation explains this better than I can.

This is an example from Documentation/memory-barriers.txt:

        CPU 1                               CPU 2
        ===================                 ===================
        WRITE_ONCE(a, 1);    }----   --->{  v = READ_ONCE(c);
        WRITE_ONCE(b, 2);    }    \ /    {  w = READ_ONCE(d);
        <write barrier>            \        <read barrier>
        WRITE_ONCE(c, 3);    }    / \    {  x = READ_ONCE(a);
        WRITE_ONCE(d, 4);    }----   --->{  y = READ_ONCE(b);

I'll adapt this example to our case:

    CPU1 == Producer                    CPU2 == Consumer
    ===================                 ===================
    WRITE DATA TO RING                  READ PROD
    <write barrier>                     <read barrier>
    WRITE PROD                          READ DATA FROM RING

It is exactly the same. If you agree that this is correct so far, I'll
add the other half of it:

    CPU1 == Producer                    CPU2 == Consumer
    ===================                 ===================
    READ CONS                           READ DATA FROM RING
    <general barrier>                   <general barrier>
    WRITE DATA TO RING                  WRITE CONS

There isn't exactly a matching example in
Documentation/memory-barriers.txt, but I think is correct.
All together:

    CPU1 == Producer                    CPU2 == Consumer
    ===================                 ===================
    READ CONS                           READ PROD
    <general barrier 1>                 <read barrier 3>
    WRITE DATA TO RING                  READ DATA FROM RING
    <write barrier 2>                   <general barrier 4>
    WRITE PROD                          WRITE CONS

1 is pared with 4
2 is pared with 3

If there is a problem with this, I don't see it, please explain.


> >> 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))
> 
> For example, the producer writes cons = 0 and prod = 100000000 so
> although you will not index off the array, the consumer will end up
> processing 1000s of bogus entries.

I see, I'll write something about it.


> There have been XSAs where backends did not validate prod in this way.

Really? I thought that the XSAs were about reading the same field twice.
So for example:

cons = ring->cons;
prod = ring->prod;
<some barrier>
do_stuff1();
do_stuff2(ring->prod);

Maybe I am thinking of a different XSA?

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