Re: [Qemu-block] [PATCH 1/3] xen-disk: only advertize feature-persistent if grant copy is not available

[Roger Pau Monné]

On Tue, Jun 20, 2017 at 03:19:33PM -0700, Stefano Stabellini wrote:
> On Tue, 20 Jun 2017, Paul Durrant wrote:
> > If grant copy is available then it will always be used in preference to
> > persistent maps. In this case feature-persistent should not be advertized
> > to the frontend, otherwise it may needlessly copy data into persistently
> > granted buffers.
> > 
> > Signed-off-by: Paul Durrant 
> 
> CC'ing Roger.
> 
> It is true that using feature-persistent together with grant copies is a
> a very bad idea.
> 
> But this change enstablishes an explicit preference of
> feature_grant_copy over feature-persistent in the xen_disk backend. It
> is not obvious to me that it should be the case.
> 
> Why is feature_grant_copy (without feature-persistent) better than
> feature-persistent (without feature_grant_copy)? Shouldn't we simply
> avoid grant copies to copy data to persistent grants?

When using persistent grants the frontend must always copy data from
the buffer to the persistent grant, there's no way to avoid this.

Using grant_copy we move the copy from the frontend to the backend,
which means the CPU time of the copy is accounted to the backend. This
is not ideal, but IMHO it's better than persistent grants because it
avoids keeping a pool of mapped grants that consume memory and make
the code more complex.

Do you have some performance data showing the difference between
persistent grants vs grant copy?

Roger.

Re: [Qemu-block] [PATCH 1/3] xen-disk: only advertize feature-persistent if grant copy is not available

[Paul Durrant]

> -Original Message-
> From: Roger Pau Monne
> Sent: 21 June 2017 10:18
> To: Stefano Stabellini 
> Cc: Paul Durrant ; xen-de...@lists.xenproject.org;
> qemu-de...@nongnu.org; qemu-block@nongnu.org; Anthony Perard
> ; Kevin Wolf ; Max Reitz
> 
> Subject: Re: [PATCH 1/3] xen-disk: only advertize feature-persistent if grant
> copy is not available
> 
> On Tue, Jun 20, 2017 at 03:19:33PM -0700, Stefano Stabellini wrote:
> > On Tue, 20 Jun 2017, Paul Durrant wrote:
> > > If grant copy is available then it will always be used in preference to
> > > persistent maps. In this case feature-persistent should not be advertized
> > > to the frontend, otherwise it may needlessly copy data into persistently
> > > granted buffers.
> > >
> > > Signed-off-by: Paul Durrant 
> >
> > CC'ing Roger.
> >
> > It is true that using feature-persistent together with grant copies is a
> > a very bad idea.
> >
> > But this change enstablishes an explicit preference of
> > feature_grant_copy over feature-persistent in the xen_disk backend. It
> > is not obvious to me that it should be the case.
> >
> > Why is feature_grant_copy (without feature-persistent) better than
> > feature-persistent (without feature_grant_copy)? Shouldn't we simply
> > avoid grant copies to copy data to persistent grants?
> 
> When using persistent grants the frontend must always copy data from
> the buffer to the persistent grant, there's no way to avoid this.
> 
> Using grant_copy we move the copy from the frontend to the backend,
> which means the CPU time of the copy is accounted to the backend. This
> is not ideal, but IMHO it's better than persistent grants because it
> avoids keeping a pool of mapped grants that consume memory and make
> the code more complex.
> 
> Do you have some performance data showing the difference between
> persistent grants vs grant copy?
> 

No, but I can get some :-)

For a little background... I've been trying to push throughput of fio running 
in a debian stretch guest on my skull canyon NUC. When I started out, I was 
getting ~100MBbs. When I finished, with this patch, the IOThreads one, the 
multi-page ring one and a bit of hackery to turn off all the aio flushes that 
seem to occur even if the image is opened with O_DIRECT, I was getting 
~960Mbps... which is about line rate for the SSD in the in NUC.

So, I'll force use of persistent grants on and see what sort of throughput I 
get.

Cheers,

  Paul

> Roger.

Re: [Qemu-block] [PATCH 1/3] xen-disk: only advertize feature-persistent if grant copy is not available

[Stefano Stabellini]

On Tue, 20 Jun 2017, Paul Durrant wrote:
> If grant copy is available then it will always be used in preference to
> persistent maps. In this case feature-persistent should not be advertized
> to the frontend, otherwise it may needlessly copy data into persistently
> granted buffers.
> 
> Signed-off-by: Paul Durrant 

CC'ing Roger.

It is true that using feature-persistent together with grant copies is a
a very bad idea.

But this change enstablishes an explicit preference of
feature_grant_copy over feature-persistent in the xen_disk backend. It
is not obvious to me that it should be the case.

Why is feature_grant_copy (without feature-persistent) better than
feature-persistent (without feature_grant_copy)? Shouldn't we simply
avoid grant copies to copy data to persistent grants?


> ---
> Cc: Stefano Stabellini 
> Cc: Anthony Perard 
> Cc: Kevin Wolf 
> Cc: Max Reitz 
> ---
>  hw/block/xen_disk.c | 15 ---
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
> index 3a22805fbc..9b06e3aa81 100644
> --- a/hw/block/xen_disk.c
> +++ b/hw/block/xen_disk.c
> @@ -1023,11 +1023,18 @@ static int blk_init(struct XenDevice *xendev)
>  
>  blkdev->file_blk  = BLOCK_SIZE;
>  
> +blkdev->feature_grant_copy =
> +(xengnttab_grant_copy(blkdev->xendev.gnttabdev, 0, NULL) == 
> 0);
> +
> +xen_pv_printf(>xendev, 3, "grant copy operation %s\n",
> +  blkdev->feature_grant_copy ? "enabled" : "disabled");
> +
>  /* fill info
>   * blk_connect supplies sector-size and sectors
>   */
>  xenstore_write_be_int(>xendev, "feature-flush-cache", 1);
> -xenstore_write_be_int(>xendev, "feature-persistent", 1);
> +xenstore_write_be_int(>xendev, "feature-persistent",
> +  !blkdev->feature_grant_copy);
>  xenstore_write_be_int(>xendev, "info", info);
>  
>  blk_parse_discard(blkdev);
> @@ -1202,12 +1209,6 @@ static int blk_connect(struct XenDevice *xendev)
>  
>  xen_be_bind_evtchn(>xendev);
>  
> -blkdev->feature_grant_copy =
> -(xengnttab_grant_copy(blkdev->xendev.gnttabdev, 0, NULL) == 
> 0);
> -
> -xen_pv_printf(>xendev, 3, "grant copy operation %s\n",
> -  blkdev->feature_grant_copy ? "enabled" : "disabled");
> -
>  xen_pv_printf(>xendev, 1, "ok: proto %s, ring-ref %d, "
>"remote port %d, local port %d\n",
>blkdev->xendev.protocol, blkdev->ring_ref,
> -- 
> 2.11.0
> 

[Qemu-block] [PATCH 1/3] xen-disk: only advertize feature-persistent if grant copy is not available

[Paul Durrant]

If grant copy is available then it will always be used in preference to
persistent maps. In this case feature-persistent should not be advertized
to the frontend, otherwise it may needlessly copy data into persistently
granted buffers.

Signed-off-by: Paul Durrant 
---
Cc: Stefano Stabellini 
Cc: Anthony Perard 
Cc: Kevin Wolf 
Cc: Max Reitz 
---
 hw/block/xen_disk.c | 15 ---
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index 3a22805fbc..9b06e3aa81 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -1023,11 +1023,18 @@ static int blk_init(struct XenDevice *xendev)
 
 blkdev->file_blk  = BLOCK_SIZE;
 
+blkdev->feature_grant_copy =
+(xengnttab_grant_copy(blkdev->xendev.gnttabdev, 0, NULL) == 0);
+
+xen_pv_printf(>xendev, 3, "grant copy operation %s\n",
+  blkdev->feature_grant_copy ? "enabled" : "disabled");
+
 /* fill info
  * blk_connect supplies sector-size and sectors
  */
 xenstore_write_be_int(>xendev, "feature-flush-cache", 1);
-xenstore_write_be_int(>xendev, "feature-persistent", 1);
+xenstore_write_be_int(>xendev, "feature-persistent",
+  !blkdev->feature_grant_copy);
 xenstore_write_be_int(>xendev, "info", info);
 
 blk_parse_discard(blkdev);
@@ -1202,12 +1209,6 @@ static int blk_connect(struct XenDevice *xendev)
 
 xen_be_bind_evtchn(>xendev);
 
-blkdev->feature_grant_copy =
-(xengnttab_grant_copy(blkdev->xendev.gnttabdev, 0, NULL) == 0);
-
-xen_pv_printf(>xendev, 3, "grant copy operation %s\n",
-  blkdev->feature_grant_copy ? "enabled" : "disabled");
-
 xen_pv_printf(>xendev, 1, "ok: proto %s, ring-ref %d, "
   "remote port %d, local port %d\n",
   blkdev->xendev.protocol, blkdev->ring_ref,
-- 
2.11.0