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

2016-10-12 Thread Konrad Rzeszutek Wilk
> > > ### Data ring
> > > 
> > > Data rings are used for sending and receiving data over a connected 
> > > socket. They
> > > are created upon a successful **accept** or **connect** command.
> > > 
> > > A data ring is composed of two pieces: the interface and the **in** and 
> > > **out**
> > > buffers. The interface, represented by `struct pvcalls_ring_intf` is 
> > > shared
> > > first and resides on the page whose grant reference is passed by 
> > > **accept** and
> > > **connect** as parameter. `struct pvcalls_ring_intf` contains the list of 
> > > grant
> > > references which constitute the **in** and **out** data buffers.
> > > 
> > >  Data ring interface
> > > 
> > > struct pvcalls_data_intf {
> > >   PVCALLS_RING_IDX in_cons, in_prod;
> > 
> > Do you want to add some spacing between them so that you don't
> > use the same cacheline?
> 
> Let me get this straight. in_cons and in_prod should be on the same
> cacheline.  Similarly out_cons and out_prod should be on the same
> cacheline, but on a different cacheline compared to in_cons and in_prod.
> 
> So I should add padding between the two pairs of indices. Something
> like:
> uint8_t pad[cache_line_size - 8];
> 
> Where cache_line_size is usually 64 bytes. So this would be:
> 
> uint8_t pad[56];
> 
> Is that right?

Sounds right. But pahole will for sure confirm :-) I was too lazy to copy this 
in code and see
what it gives you.
> 
> 
> > >   PVCALLS_RING_IDX out_cons, out_prod;
> > >   int32_t in_error, out_error;
> > > 
> > >   uint32_t ring_order;
> > >   grant_ref_t ref[];
> > > };
> > 
> > That does not look like it would be to the power of two? Perhaps
> > some padding?
> 
> I don't understand the suggestion about the padding here.

You pad[56] takes care of the issue I had.

..snipp.
> > > The binary layout of `struct pvcalls_data_intf` follows:
> > > 
> > > 0 4 8 12162024
> > > 28
> > > 
> > > +-+-+-+-+-+-+--+
> > > | in_cons | in_prod |out_cons |out_prod |in_error 
> > > |out_error|ring_order|
> > > 
> > > +-+-+-+-+-+-+--+
> > > 
> > > 283236404092 4096
> > > +-+-+-+//---+-+
> > > |  ref[0] |  ref[1] |  ref[2] | |  ref[N] |
> > > +-+-+-+//---+-+
> > 
> > Perhaps you can say:
> > 
> > **N.B** For one page, the N would be 2034.
> 
> I can do that but actually N would be (4096-28)/4 = 1017. Ring refs are
> 4 bytes each.

I somehow had uint16_t on my mind! Thanks for checking me!

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


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

2016-10-11 Thread Stefano Stabellini
On Wed, 28 Sep 2016, Konrad Rzeszutek Wilk wrote:
> On Wed, Sep 14, 2016 at 04:45:01PM -0700, Stefano Stabellini wrote:
> > Hi all,
> 
> Hey!
> 
> I took a look at the spec. It overall looks good with a detail
> of how each of the sub-commands works in exhaustive detail (which
> is awesome!).

Thank you! I really appreciate the review, and sorry for taking so long
to reply (last week I was in a conference).


> Please see below for some questions, comments.
> > 
> > This is the design document of the PV Calls protocol. You can find
> > prototypes of the Linux frontend and backend drivers here:
> > 
> > git://git.kernel.org/pub/scm/linux/kernel/git/sstabellini/xen.git pvcalls-6
> > 
> > To use them, make sure to enable CONFIG_XEN_PVCALLS in your kernel
> > config and add "pvcalls=1" to the command line of your DomU Linux
> > kernel. You also need the toolstack to create the initial xenstore nodes
> > for the protocol. To do that, please apply the attached patch to libxl
> > (the patch is based on Xen 4.7.0-rc3) and add "pvcalls=1" to your DomU
> > config file.
> > 
> > 
> > In this version I added a "reuse" field to the release command, to give
> > an hint to the backend that data ring pages and evtchn will be reused.
> > The backend is free to unmap them immediately as usual or keep them
> > mapped until they get reused (or until it decides it is time to free
> > them). Thanks to this optimization pv calls can handle more requests per
> > second than Docker with apachebench in my test environment.
> > 
> > I also added a "networking-calls" backend node on xenstore to allow the
> > backend to specify that is supporting networking calls. In a future when
> > other POSIX syscall sets are supported, a backend could decide to
> > implement only one set.
> > 
> > 
> > Please review!
> > 
> > Cheers,
> > 
> > Stefano
> > 
> > 
> > Changes in v6:
> > - add reuse field in release command
> > - add "networking-calls" backend node on xenstore
> > - fixed tab/whitespace indentation
> > 
> > Changes in v5:
> > - clarify text
> > - rename id to req_id
> > - rename sockid to id
> > - move id to request and response specific fields
> > - add version node to xenstore
> > 
> > Changes in v4:
> > - rename xensock to pvcalls
> > 
> > Changes in v3:
> > - add a dummy element to struct xen_xensock_request to make sure the
> >   size of the struct is the same on both x86_32 and x86_64
> > 
> > Changes in v2:
> > - add max-dataring-page-order
> > - add "Publish backend features and transport parameters" to backend
> >   xenbus workflow
> > - update new cmd values
> > - update xen_xensock_request
> > - add backlog parameter to listen and binary layout
> > - add description of new data ring format (interface+data)
> > - modify connect and accept to reflect new data ring format
> > - add link to POSIX docs
> > - add error numbers
> > - add address format section and relevant numeric definitions
> > - add explicit mention of unimplemented commands
> > - add protocol node name
> > - add xenbus shutdown diagram
> > - add socket operation
> > 
> > ---
> > 
> > # PV Calls Protocol version 1
> > 
> > ## Rationale
> > 
> > PV Calls is a paravirtualized protocol that allows the implementation of
> > a set of POSIX functions in a different domain. The PV Calls frontend
> 
> For folks not used to Xen, the word 'domain' may be confusing. They
> may think PCI domain (segments) or such.
> 
> Perhaps it may be good to replace the word domain with container?
> Or guest?
>
> Or have an section explaining what some of these words mean.
> > sends POSIX function calls to the backend, which implements them and
> 
> Like backend for example. It is crystal clear to me what this is, but
> for somebody who has been brewing in networking the word 'backend'
> may imply a web-server to them.
>
> > returns a value to the frontend.
> 
> And this they may associate with a client. Or a client application (e.g curl).

You are right about all of these points. I think it might be best to add
a glossary to the doc. I'll do that.


> > This version of the document covers networking function calls, such as
> > connect, accept, bind, release, listen, poll, recvmsg and sendmsg; but
> 
> NIT: Sort these alphabetically. It will be easier to expand this in the
> future if you have it in that order.

I ordered them by call order, which I think will make them easier to
understand. More on this later.


> > the protocol is meant to be easily extended to cover different sets of
> > calls. Unimplemented commands return ENOTSUPP.
> 
> Surely EOPNOTSUPP ?

Both exists, but I think that ENOTSUPP is more appropriate. More on this
later.


> > PV Calls provide the following benefits:
> > * full visibility of the guest behavior on the backend domain, allowing
> >   for inexpensive filtering and manipulation of any guest calls
> > * excellent performance
> 
> NIT: (feel free to ignore it) Missing periods or commans at the end of the 
> lines.

I did it on purpose for readability.


> 

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

2016-09-28 Thread Konrad Rzeszutek Wilk
On Wed, Sep 14, 2016 at 04:45:01PM -0700, Stefano Stabellini wrote:
> Hi all,

Hey!

I took a look at the spec. It overall looks good with a detail
of how each of the sub-commands works in exhaustive detail (which
is awesome!).

Please see below for some questions, comments.
> 
> This is the design document of the PV Calls protocol. You can find
> prototypes of the Linux frontend and backend drivers here:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/sstabellini/xen.git pvcalls-6
> 
> To use them, make sure to enable CONFIG_XEN_PVCALLS in your kernel
> config and add "pvcalls=1" to the command line of your DomU Linux
> kernel. You also need the toolstack to create the initial xenstore nodes
> for the protocol. To do that, please apply the attached patch to libxl
> (the patch is based on Xen 4.7.0-rc3) and add "pvcalls=1" to your DomU
> config file.
> 
> 
> In this version I added a "reuse" field to the release command, to give
> an hint to the backend that data ring pages and evtchn will be reused.
> The backend is free to unmap them immediately as usual or keep them
> mapped until they get reused (or until it decides it is time to free
> them). Thanks to this optimization pv calls can handle more requests per
> second than Docker with apachebench in my test environment.
> 
> I also added a "networking-calls" backend node on xenstore to allow the
> backend to specify that is supporting networking calls. In a future when
> other POSIX syscall sets are supported, a backend could decide to
> implement only one set.
> 
> 
> Please review!
> 
> Cheers,
> 
> Stefano
> 
> 
> Changes in v6:
> - add reuse field in release command
> - add "networking-calls" backend node on xenstore
> - fixed tab/whitespace indentation
> 
> Changes in v5:
> - clarify text
> - rename id to req_id
> - rename sockid to id
> - move id to request and response specific fields
> - add version node to xenstore
> 
> Changes in v4:
> - rename xensock to pvcalls
> 
> Changes in v3:
> - add a dummy element to struct xen_xensock_request to make sure the
>   size of the struct is the same on both x86_32 and x86_64
> 
> Changes in v2:
> - add max-dataring-page-order
> - add "Publish backend features and transport parameters" to backend
>   xenbus workflow
> - update new cmd values
> - update xen_xensock_request
> - add backlog parameter to listen and binary layout
> - add description of new data ring format (interface+data)
> - modify connect and accept to reflect new data ring format
> - add link to POSIX docs
> - add error numbers
> - add address format section and relevant numeric definitions
> - add explicit mention of unimplemented commands
> - add protocol node name
> - add xenbus shutdown diagram
> - add socket operation
> 
> ---
> 
> # PV Calls Protocol version 1
> 
> ## Rationale
> 
> PV Calls is a paravirtualized protocol that allows the implementation of
> a set of POSIX functions in a different domain. The PV Calls frontend

For folks not used to Xen, the word 'domain' may be confusing. They
may think PCI domain (segments) or such.

Perhaps it may be good to replace the word domain with container?
Or guest?

Or have an section explaining what some of these words mean.
> sends POSIX function calls to the backend, which implements them and

Like backend for example. It is crystal clear to me what this is, but
for somebody who has been brewing in networking the word 'backend'
may imply a web-server to them.

> returns a value to the frontend.

And this they may associate with a client. Or a client application (e.g curl).

> 
> This version of the document covers networking function calls, such as
> connect, accept, bind, release, listen, poll, recvmsg and sendmsg; but

NIT: Sort these alphabetically. It will be easier to expand this in the
future if you have it in that order.

> the protocol is meant to be easily extended to cover different sets of
> calls. Unimplemented commands return ENOTSUPP.

Surely EOPNOTSUPP ?
> 
> PV Calls provide the following benefits:
> * full visibility of the guest behavior on the backend domain, allowing
>   for inexpensive filtering and manipulation of any guest calls
> * excellent performance

NIT: (feel free to ignore it) Missing periods or commans at the end of the 
lines.
> 
> Specifically, PV Calls for networking offer these advantages:
> * guest networking works out of the box with VPNs, wireless networks and
>   any other complex configurations on the host
> * guest services listen on ports bound directly to the backend domain IP
>   addresses
> * localhost becomes a secure namespace for inter-VMs communications
> 
> 
> ## Design
>

Would it make sense to mention the pre-requisite of why Xen is used?
Your slides at the XPDS where quite good and perhaps some of the content
here (VMCS, PV guests, etc) could be put here.

Naturally this is more folks who haven't been marinating in Xen for years
to get an idea why it was choosen.

> ### Xenstore
> 
> The frontend and the backend connect to each