[PATCH] xen: blkif.h: document linux xen-block multi-page ring implementation

[PATCH] xen: blkif.h: document linux xen-block multi-page ring implementation

[Bob Liu]

After commit 1b1586eeeb8c ("xenbus_client: Extend interface to
support multi-page ring"), Linux xenbus driver can support multi-page ring.

Based on this interface, we got some impressive improvements by using multi-page
ring in xen-block driver. If using 64 pages as the ring, the IOPS increased
about 15 times for the throughput testing.

The Linux implementation reuses two 'DEPRECATED' nodes('max-ring-pages' and
'num-ring-pages), so that nothing would be broken.
Also removed the power of 2 limit and updated the default/max value accordingly.

Signed-off-by: Bob Liu <bob.liu@oracle.com>
---
 xen/include/public/io/blkif.h |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/xen/include/public/io/blkif.h b/xen/include/public/io/blkif.h
index 6baf7fb..0e34ae6 100644
--- a/xen/include/public/io/blkif.h
+++ b/xen/include/public/io/blkif.h
@@ -189,11 +189,11 @@
  *
  * max-ring-pages
  *      Values:         <uint32_t>
- *      Default Value:  1
- *      Notes:          DEPRECATED, 2, 3
+ *      Default Value:  32
+ *      Notes:          2, 3
  *
  *      The maximum supported size of the request ring buffer in units of
- *      machine pages.  The value must be a power of 2.
+ *      machine pages.
  *
  *------------------------- Backend Device Properties -------------------------
  *
@@ -302,11 +302,11 @@
  * num-ring-pages
  *      Values:         <uint32_t>
  *      Default Value:  1
- *      Maximum Value:  MAX(max-ring-pages,(0x1 << max-ring-page-order))
- *      Notes:          DEPRECATED, 2, 3
+ *      Maximum Value:  max-ring-pages
+ *      Notes:          2, 3
  *
  *      The size of the frontend allocated request ring buffer in units of
- *      machine pages.  The value must be a power of 2.
+ *      machine pages.
  *
  * feature-persistent
  *      Values:         0/1 (boolean)
-- 
1.7.10.4

Re: [PATCH] xen: blkif.h: document linux xen-block multi-page ring implementation

[Roger Pau Monné]

El 12/05/15 a les 12.58, Bob Liu ha escrit:
> After commit 1b1586eeeb8c ("xenbus_client: Extend interface to
> support multi-page ring"), Linux xenbus driver can support multi-page ring.
> 
> Based on this interface, we got some impressive improvements by using multi-page
> ring in xen-block driver. If using 64 pages as the ring, the IOPS increased
> about 15 times for the throughput testing.
> 
> The Linux implementation reuses two 'DEPRECATED' nodes('max-ring-pages' and
> 'num-ring-pages), so that nothing would be broken.
> Also removed the power of 2 limit and updated the default/max value accordingly.
>
> Signed-off-by: Bob Liu <bob.liu@oracle.com>

This looks fine IMHO, the only caveat I see is that you are revamping an
old feature so we need to be careful that the implementation that's
merged on Linux is compatible with the previous implementation.

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Roger.

Re: [PATCH] xen: blkif.h: document linux xen-block multi-page ring implementation

[David Vrabel]

On 12/05/15 11:58, Bob Liu wrote:
> After commit 1b1586eeeb8c ("xenbus_client: Extend interface to
> support multi-page ring"), Linux xenbus driver can support multi-page ring.
> 
> Based on this interface, we got some impressive improvements by using multi-page
> ring in xen-block driver. If using 64 pages as the ring, the IOPS increased
> about 15 times for the throughput testing.
> 
> The Linux implementation reuses two 'DEPRECATED' nodes('max-ring-pages' and
> 'num-ring-pages), so that nothing would be broken.
> Also removed the power of 2 limit and updated the default/max value accordingly.

You can't drop the power of 2 restriction as there may be frontends that
support this old option (from before it was deprecated) and these may
not support non-powers of 2.

David

Re: [PATCH] xen: blkif.h: document linux xen-block multi-page ring implementation

[Bob Liu]

On 05/15/2015 05:51 PM, David Vrabel wrote:
> On 12/05/15 11:58, Bob Liu wrote:
>> After commit 1b1586eeeb8c ("xenbus_client: Extend interface to
>> support multi-page ring"), Linux xenbus driver can support multi-page ring.
>>
>> Based on this interface, we got some impressive improvements by using multi-page
>> ring in xen-block driver. If using 64 pages as the ring, the IOPS increased
>> about 15 times for the throughput testing.
>>
>> The Linux implementation reuses two 'DEPRECATED' nodes('max-ring-pages' and
>> 'num-ring-pages), so that nothing would be broken.
>> Also removed the power of 2 limit and updated the default/max value accordingly.
> 
> You can't drop the power of 2 restriction as there may be frontends that
> support this old option (from before it was deprecated) and these may
> not support non-powers of 2.
> 

After take a closer look I think we can fully reuse the current protocol which
only uses 'ring-page-order' and 'max-ring-page-order'.
And leave 'max-ring-pages' and 'num-ring-pages' to DEPRECATED.

In conclusion, the blkif.h don't need to be modified and I'll update Linux
implementation to use 'ring-page-order' and 'max-ring-page-order' too.
What do you think? Thank you!

Regards,
-Bob

Re: [PATCH] xen: blkif.h: document linux xen-block multi-page ring implementation

[David Vrabel]

On 15/05/15 11:39, Bob Liu wrote:
> 
> On 05/15/2015 05:51 PM, David Vrabel wrote:
>> On 12/05/15 11:58, Bob Liu wrote:
>>> After commit 1b1586eeeb8c ("xenbus_client: Extend interface to
>>> support multi-page ring"), Linux xenbus driver can support multi-page ring.
>>>
>>> Based on this interface, we got some impressive improvements by using multi-page
>>> ring in xen-block driver. If using 64 pages as the ring, the IOPS increased
>>> about 15 times for the throughput testing.
>>>
>>> The Linux implementation reuses two 'DEPRECATED' nodes('max-ring-pages' and
>>> 'num-ring-pages), so that nothing would be broken.
>>> Also removed the power of 2 limit and updated the default/max value accordingly.
>>
>> You can't drop the power of 2 restriction as there may be frontends that
>> support this old option (from before it was deprecated) and these may
>> not support non-powers of 2.
>>
> 
> After take a closer look I think we can fully reuse the current protocol which
> only uses 'ring-page-order' and 'max-ring-page-order'.
> And leave 'max-ring-pages' and 'num-ring-pages' to DEPRECATED.
> 
> In conclusion, the blkif.h don't need to be modified and I'll update Linux
> implementation to use 'ring-page-order' and 'max-ring-page-order' too.
> What do you think? Thank you!

This sounds fine.

David

Re: [PATCH] xen: blkif.h: document linux xen-block multi-page ring implementation

[Konrad Rzeszutek Wilk]

On Tue, May 12, 2015 at 06:58:49PM +0800, Bob Liu wrote:
> After commit 1b1586eeeb8c ("xenbus_client: Extend interface to
> support multi-page ring"), Linux xenbus driver can support multi-page ring.
> 
> Based on this interface, we got some impressive improvements by using multi-page
> ring in xen-block driver. If using 64 pages as the ring, the IOPS increased
> about 15 times for the throughput testing.
> 
> The Linux implementation reuses two 'DEPRECATED' nodes('max-ring-pages' and
> 'num-ring-pages), so that nothing would be broken.
> Also removed the power of 2 limit and updated the default/max value accordingly.

Doing an git annotate shows:

commit 829f2a9c6dfae13faee4e24b953441113338e0f2
Author: Justin T. Gibbs <justing@spectralogic.com>
Date:   Thu Feb 23 10:02:30 2012 +0000

    blkif.h: Document the Red Hat and Citrix blkif multi-page ring extensions
    
    No functional changes.


Which says:

 * (1) Multi-page ring buffer scheme first developed in the Citrix XenServer
+ *     PV drivers.
+ * (2) Multi-page ring buffer scheme first used in some RedHat distributions
+ *     including a distribution deployed on certain nodes of the Amazon
+ *     EC2 cluster.
+ * (3) Support for multi-page ring buffers was implemented independently,
+ *     in slightly different forms, by both Citrix and RedHat/Amazon.


And I double checked the linux-2.6.18.hg tree and I see no mention of 
multi-page. I wonder where such code exists?

CC-ing Matt, perhaps he can help.

Cc-ing Paul, perhaps he can help with the Citrix part.

Thank you!

P.S.
Also added Keir on the 'To' as scripts/get_maintainers.pl says he should be on it.

> 
> Signed-off-by: Bob Liu <bob.liu@oracle.com>
> ---
>  xen/include/public/io/blkif.h |   12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/include/public/io/blkif.h b/xen/include/public/io/blkif.h
> index 6baf7fb..0e34ae6 100644
> --- a/xen/include/public/io/blkif.h
> +++ b/xen/include/public/io/blkif.h
> @@ -189,11 +189,11 @@
>   *
>   * max-ring-pages
>   *      Values:         <uint32_t>
> - *      Default Value:  1
> - *      Notes:          DEPRECATED, 2, 3
> + *      Default Value:  32
> + *      Notes:          2, 3
>   *
>   *      The maximum supported size of the request ring buffer in units of
> - *      machine pages.  The value must be a power of 2.
> + *      machine pages.
>   *
>   *------------------------- Backend Device Properties -------------------------
>   *
> @@ -302,11 +302,11 @@
>   * num-ring-pages
>   *      Values:         <uint32_t>
>   *      Default Value:  1
> - *      Maximum Value:  MAX(max-ring-pages,(0x1 << max-ring-page-order))
> - *      Notes:          DEPRECATED, 2, 3
> + *      Maximum Value:  max-ring-pages
> + *      Notes:          2, 3
>   *
>   *      The size of the frontend allocated request ring buffer in units of
> - *      machine pages.  The value must be a power of 2.
> + *      machine pages.
>   *
>   * feature-persistent
>   *      Values:         0/1 (boolean)
> -- 
> 1.7.10.4
> 

Re: [PATCH] xen: blkif.h: document linux xen-block multi-page ring implementation

[Paul Durrant]

> -----Original Message-----
> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
> Sent: 15 May 2015 16:23
> To: Bob Liu; msw@amazon.com; Paul Durrant; Keir (Xen.org)
> Cc: xen-devel@lists.xen.org; David Vrabel; justing@spectralogic.com; Roger
> Pau Monne
> Subject: Re: [PATCH] xen: blkif.h: document linux xen-block multi-page ring
> implementation
> 
> On Tue, May 12, 2015 at 06:58:49PM +0800, Bob Liu wrote:
> > After commit 1b1586eeeb8c ("xenbus_client: Extend interface to
> > support multi-page ring"), Linux xenbus driver can support multi-page ring.
> >
> > Based on this interface, we got some impressive improvements by using
> multi-page
> > ring in xen-block driver. If using 64 pages as the ring, the IOPS increased
> > about 15 times for the throughput testing.
> >
> > The Linux implementation reuses two 'DEPRECATED' nodes('max-ring-
> pages' and
> > 'num-ring-pages), so that nothing would be broken.
> > Also removed the power of 2 limit and updated the default/max value
> accordingly.
> 
> Doing an git annotate shows:
> 
> commit 829f2a9c6dfae13faee4e24b953441113338e0f2
> Author: Justin T. Gibbs <justing@spectralogic.com>
> Date:   Thu Feb 23 10:02:30 2012 +0000
> 
>     blkif.h: Document the Red Hat and Citrix blkif multi-page ring extensions
> 
>     No functional changes.
> 
> 
> Which says:
> 
>  * (1) Multi-page ring buffer scheme first developed in the Citrix XenServer
> + *     PV drivers.
> + * (2) Multi-page ring buffer scheme first used in some RedHat distributions
> + *     including a distribution deployed on certain nodes of the Amazon
> + *     EC2 cluster.
> + * (3) Support for multi-page ring buffers was implemented independently,
> + *     in slightly different forms, by both Citrix and RedHat/Amazon.
> 
> 
> And I double checked the linux-2.6.18.hg tree and I see no mention of
> multi-page. I wonder where such code exists?
> 
> CC-ing Matt, perhaps he can help.
> 
> Cc-ing Paul, perhaps he can help with the Citrix part.
> 

The only code that has ever been deployed by Citrix, to my knowledge, is the Windows multi-page frontend code which uses the 'page-order' variants of the xenstore keys. I don't believe Citrix has ever shipped capable backends in XenServer, but it's possible Amazon implemented such a thing since they use Citrix drivers in their Windows guests.

  Paul

> Thank you!
> 
> P.S.
> Also added Keir on the 'To' as scripts/get_maintainers.pl says he should be
> on it.
> 
> >
> > Signed-off-by: Bob Liu <bob.liu@oracle.com>
> > ---
> >  xen/include/public/io/blkif.h |   12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/xen/include/public/io/blkif.h b/xen/include/public/io/blkif.h
> > index 6baf7fb..0e34ae6 100644
> > --- a/xen/include/public/io/blkif.h
> > +++ b/xen/include/public/io/blkif.h
> > @@ -189,11 +189,11 @@
> >   *
> >   * max-ring-pages
> >   *      Values:         <uint32_t>
> > - *      Default Value:  1
> > - *      Notes:          DEPRECATED, 2, 3
> > + *      Default Value:  32
> > + *      Notes:          2, 3
> >   *
> >   *      The maximum supported size of the request ring buffer in units of
> > - *      machine pages.  The value must be a power of 2.
> > + *      machine pages.
> >   *
> >   *------------------------- Backend Device Properties -------------------------
> >   *
> > @@ -302,11 +302,11 @@
> >   * num-ring-pages
> >   *      Values:         <uint32_t>
> >   *      Default Value:  1
> > - *      Maximum Value:  MAX(max-ring-pages,(0x1 << max-ring-page-
> order))
> > - *      Notes:          DEPRECATED, 2, 3
> > + *      Maximum Value:  max-ring-pages
> > + *      Notes:          2, 3
> >   *
> >   *      The size of the frontend allocated request ring buffer in units of
> > - *      machine pages.  The value must be a power of 2.
> > + *      machine pages.
> >   *
> >   * feature-persistent
> >   *      Values:         0/1 (boolean)
> > --
> > 1.7.10.4
> >

Re: [PATCH] xen: blkif.h: document linux xen-block multi-page ring implementation

[Jan Beulich]

>>> On 15.05.15 at 17:22, <konrad.wilk@oracle.com> wrote:
> Doing an git annotate shows:
> 
> commit 829f2a9c6dfae13faee4e24b953441113338e0f2
> Author: Justin T. Gibbs <justing@spectralogic.com>
> Date:   Thu Feb 23 10:02:30 2012 +0000
> 
>     blkif.h: Document the Red Hat and Citrix blkif multi-page ring extensions
>     
>     No functional changes.
> 
> 
> Which says:
> 
>  * (1) Multi-page ring buffer scheme first developed in the Citrix XenServer
> + *     PV drivers.
> + * (2) Multi-page ring buffer scheme first used in some RedHat distributions
> + *     including a distribution deployed on certain nodes of the Amazon
> + *     EC2 cluster.
> + * (3) Support for multi-page ring buffers was implemented independently,
> + *     in slightly different forms, by both Citrix and RedHat/Amazon.
> 
> 
> And I double checked the linux-2.6.18.hg tree and I see no mention of 
> multi-page. I wonder where such code exists?

We have support for multi page rings in our kernels, largely because
(iirc) Amazon had asked it.

Jan

Re: [PATCH] xen: blkif.h: document linux xen-block multi-page ring implementation

[Konrad Rzeszutek Wilk]

On Fri, May 15, 2015 at 04:38:58PM +0100, Jan Beulich wrote:
> >>> On 15.05.15 at 17:22, <konrad.wilk@oracle.com> wrote:
> > Doing an git annotate shows:
> > 
> > commit 829f2a9c6dfae13faee4e24b953441113338e0f2
> > Author: Justin T. Gibbs <justing@spectralogic.com>
> > Date:   Thu Feb 23 10:02:30 2012 +0000
> > 
> >     blkif.h: Document the Red Hat and Citrix blkif multi-page ring extensions
> >     
> >     No functional changes.
> > 
> > 
> > Which says:
> > 
> >  * (1) Multi-page ring buffer scheme first developed in the Citrix XenServer
> > + *     PV drivers.
> > + * (2) Multi-page ring buffer scheme first used in some RedHat distributions
> > + *     including a distribution deployed on certain nodes of the Amazon
> > + *     EC2 cluster.
> > + * (3) Support for multi-page ring buffers was implemented independently,
> > + *     in slightly different forms, by both Citrix and RedHat/Amazon.
> > 
> > 
> > And I double checked the linux-2.6.18.hg tree and I see no mention of 
> > multi-page. I wonder where such code exists?
> 
> We have support for multi page rings in our kernels, largely because
> (iirc) Amazon had asked it.

Aha!

Bob has found that he can use 'ring-page-order' and 'max-ring-page-order'
which are the new standard and not have to worry about breaking older
guests.
> 
> Jan
> 

Re: [PATCH] xen: blkif.h: document linux xen-block multi-page ring implementation

[Paul Durrant]

> -----Original Message-----
> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
> Sent: 15 May 2015 16:55
> To: Jan Beulich
> Cc: Bob Liu; msw@amazon.com; David Vrabel; Paul Durrant; Roger Pau
> Monne; xen-devel@lists.xen.org; justing@spectralogic.com; Keir (Xen.org)
> Subject: Re: [Xen-devel] [PATCH] xen: blkif.h: document linux xen-block
> multi-page ring implementation
> 
> On Fri, May 15, 2015 at 04:38:58PM +0100, Jan Beulich wrote:
> > >>> On 15.05.15 at 17:22, <konrad.wilk@oracle.com> wrote:
> > > Doing an git annotate shows:
> > >
> > > commit 829f2a9c6dfae13faee4e24b953441113338e0f2
> > > Author: Justin T. Gibbs <justing@spectralogic.com>
> > > Date:   Thu Feb 23 10:02:30 2012 +0000
> > >
> > >     blkif.h: Document the Red Hat and Citrix blkif multi-page ring
> extensions
> > >
> > >     No functional changes.
> > >
> > >
> > > Which says:
> > >
> > >  * (1) Multi-page ring buffer scheme first developed in the Citrix
> XenServer
> > > + *     PV drivers.
> > > + * (2) Multi-page ring buffer scheme first used in some RedHat
> distributions
> > > + *     including a distribution deployed on certain nodes of the Amazon
> > > + *     EC2 cluster.
> > > + * (3) Support for multi-page ring buffers was implemented
> independently,
> > > + *     in slightly different forms, by both Citrix and RedHat/Amazon.
> > >
> > >
> > > And I double checked the linux-2.6.18.hg tree and I see no mention of
> > > multi-page. I wonder where such code exists?
> >
> > We have support for multi page rings in our kernels, largely because
> > (iirc) Amazon had asked it.
> 
> Aha!
> 
> Bob has found that he can use 'ring-page-order' and 'max-ring-page-order'
> which are the new standard and not have to worry about breaking older
> guests.

The Xen Project Windows drivers use those keys too: see http://xenbits.xen.org/gitweb/?p=pvdrivers/win/xenvbd.git;a=blob;f=src/xenvbd/blockring.c;h=35ffb3b39d1478f08fbe016f992de152a494c0e1;hb=HEAD

  Paul

> >
> > Jan
> >