Re: [Xen-devel] [DOC v8] PV Calls protocol design

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Fri, Feb 10, 2017 at 12:09:36PM -0800, Stefano Stabellini wrote:
> On Fri, 10 Feb 2017, Konrad Rzeszutek Wilk wrote:
> > .snip..
> > > > > Request fields:
> > > > > 
> > > > > - **cmd** value: 0
> > > > > - additional fields:
> > > > >   - **id**: identifies the socket
> > > > >   - **addr**: address to connect to, see [Socket families and address 
> > > > > format]
> > > > 
> > > > 
> > > > Hm, so what do we do if we want to support AF_UNIX which has an addr of
> > > > 108 bytes?
> > > 
> > > We write a protocol extension and bump the protocol version. However, we
> > 
> > Right. How would you change the protocol for this?
> > 
> > I not asking to have this in this protocol but I just want us to think
> > of what we could do so that if somebody was to implement this - how
> > could we make this easier for this?
> > 
> > My initial thought was to spread the request over two "old" structures.
> > And if so .. would it make sense to include an extra flag or such?
> 
> That's a possibility, but I don't think we need an extra flag. It would
> be easier to introduce a new command, such as PVCALLS_CONNECT_EXTENDED
> or PVCALLS_CONNECT_V2, with the appropriate flags to say that it will
> make use of two request slots instead of one.

Fair enough. Perhaps include a section in the document about how one
could expand the protocol and include this? That would make it easier
for folks to follow an 'paved' way?


> 
> 
> > > could make the addr array size larger now to be more future proof, but
> > > it takes up memory and I have no use for it, given that we can use
> > > loopback for the same purpose.
> > > 
> > 
> > ..snip..
> > > > > #### Indexes Page Structure
> > > > > 
> > > > >     typedef uint32_t PVCALLS_RING_IDX;
> > > > > 
> > > > >     struct pvcalls_data_intf {
> > > > >       PVCALLS_RING_IDX in_cons, in_prod;
> > > > >       int32_t in_error;
> > > > 
> > > > You don't want to perhaps include in_event?
> > > > > 
> > > > >       uint8_t pad[52];
> > > > > 
> > > > >       PVCALLS_RING_IDX out_cons, out_prod;
> > > > >       int32_t out_error;
> > > > 
> > > > And out_event as way to do some form of interrupt mitigation
> > > > (similar to what you had proposed?)
> > > 
> > > Yes, the in_event / out_event optimization that I wrote for the 9pfs
> > > protocol could work here too. However, I thought you preferred to remove
> > > it for now as it is not required and increases complexity?
> > 
> > I did. But I am coming to it by looking at the ring.h header.
> > 
> > My recollection was that your optimization was a bit different than
> > what ring.h has.
> 
> Right. They are similar, but different because in this protocol we have
> two rings: the `in' ring and the `out' ring. Each ring is
> mono-directional and there is no static request size: the producer
> writes opaque data to the ring. In ring.h they are combined together and
> the request size is static and well-known. In PVCalls:
> 
> in -> backend to frontend only
> out-> frontend to backend only
> 
> Let talk about the `in' ring, where the frontend is the consumer
> and the backend is the producer. Everything is the same but mirrored for
> the `out' ring.
> 
> The producer doesn't need any notifications unless the ring is full.
> The producer, the backend in this case, never reads from the `in' ring.
> Thus, I disabled notifications to the producer by default and added an
> in_event field for the producer to ask for notifications only when
> necessary, that is when the ring is full.
> 
> On the other end, the consumer always require notifications, unless the
> consumer is already actively reading from the ring. The
> producer could figure it out without any additional fields in the
> protocol. It can simply compare the indexes at the beginning and at the
> end of the function, that's similar to what the ring protocol does.

I like your description! Could you include this in a section
titled 'Why ring.h macros are not needed.' please?

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