* Re: [PATCH] xen-blkback: add a parameter for disabling of persistent grants
2020-09-22 7:01 [PATCH] xen-blkback: add a parameter for disabling of persistent grants SeongJae Park
@ 2020-09-22 7:18 ` Jürgen Groß
2020-09-22 7:24 ` SeongJae Park
2020-09-22 7:32 ` Roger Pau Monné
2020-09-23 20:09 ` Konrad Rzeszutek Wilk
2 siblings, 1 reply; 11+ messages in thread
From: Jürgen Groß @ 2020-09-22 7:18 UTC (permalink / raw)
To: SeongJae Park, konrad.wilk, roger.pau
Cc: SeongJae Park, axboe, aliguori, amit, mheyne, linux-block,
xen-devel, linux-kernel
On 22.09.20 09:01, SeongJae Park wrote:
> From: SeongJae Park <sjpark@amazon.de>
>
> Persistent grants feature provides high scalability. On some small
> systems, however, it could incur data copy overhead[1] and thus it is
> required to be disabled. But, there is no option to disable it. For
> the reason, this commit adds a module parameter for disabling of the
> feature.
>
> [1] https://wiki.xen.org/wiki/Xen_4.3_Block_Protocol_Scalability
>
> Signed-off-by: Anthony Liguori <aliguori@amazon.com>
> Signed-off-by: SeongJae Park <sjpark@amazon.de>
> ---
> .../ABI/testing/sysfs-driver-xen-blkback | 8 ++++++++
> drivers/block/xen-blkback/xenbus.c | 17 ++++++++++++++---
> 2 files changed, 22 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-driver-xen-blkback b/Documentation/ABI/testing/sysfs-driver-xen-blkback
> index ecb7942ff146..0c42285c75ee 100644
> --- a/Documentation/ABI/testing/sysfs-driver-xen-blkback
> +++ b/Documentation/ABI/testing/sysfs-driver-xen-blkback
> @@ -35,3 +35,11 @@ Description:
> controls the duration in milliseconds that blkback will not
> cache any page not backed by a grant mapping.
> The default is 10ms.
> +
> +What: /sys/module/xen_blkback/parameters/feature_persistent
> +Date: September 2020
> +KernelVersion: 5.10
> +Contact: SeongJae Park <sjpark@amazon.de>
> +Description:
> + Whether to enable the persistent grants feature or not.
> + The default is 1 (enable).
> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
> index b9aa5d1ac10b..9c03d70469f4 100644
> --- a/drivers/block/xen-blkback/xenbus.c
> +++ b/drivers/block/xen-blkback/xenbus.c
> @@ -879,6 +879,12 @@ static void reclaim_memory(struct xenbus_device *dev)
>
> /* ** Connection ** */
>
> +/* Enable the persistent grants feature. */
> +static unsigned int feature_persistent = 1;
Use bool, please.
> +module_param_named(feature_persistent, feature_persistent, int, 0644);
module_param()
> +MODULE_PARM_DESC(feature_persistent,
> + "Enables the persistent grants feature");
> +
> /*
> * Write the physical details regarding the block device to the store, and
> * switch to Connected state.
> @@ -906,7 +912,8 @@ static void connect(struct backend_info *be)
>
> xen_blkbk_barrier(xbt, be, be->blkif->vbd.flush_support);
>
> - err = xenbus_printf(xbt, dev->nodename, "feature-persistent", "%u", 1);
> + err = xenbus_printf(xbt, dev->nodename, "feature-persistent", "%u",
> + feature_persistent ? 1 : 0);
Using bool above should allow to just use the value of
feature_persistent here.
Juergen
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] xen-blkback: add a parameter for disabling of persistent grants
2020-09-22 7:18 ` Jürgen Groß
@ 2020-09-22 7:24 ` SeongJae Park
0 siblings, 0 replies; 11+ messages in thread
From: SeongJae Park @ 2020-09-22 7:24 UTC (permalink / raw)
To: Jürgen Groß
Cc: SeongJae Park, konrad.wilk, roger.pau, SeongJae Park, axboe,
aliguori, amit, mheyne, linux-block, xen-devel, linux-kernel
On Tue, 22 Sep 2020 09:18:05 +0200 "Jürgen Groß" <jgross@suse.com> wrote:
> On 22.09.20 09:01, SeongJae Park wrote:
> > From: SeongJae Park <sjpark@amazon.de>
> >
> > Persistent grants feature provides high scalability. On some small
> > systems, however, it could incur data copy overhead[1] and thus it is
> > required to be disabled. But, there is no option to disable it. For
> > the reason, this commit adds a module parameter for disabling of the
> > feature.
> >
> > [1] https://wiki.xen.org/wiki/Xen_4.3_Block_Protocol_Scalability
> >
> > Signed-off-by: Anthony Liguori <aliguori@amazon.com>
> > Signed-off-by: SeongJae Park <sjpark@amazon.de>
> > ---
> > .../ABI/testing/sysfs-driver-xen-blkback | 8 ++++++++
> > drivers/block/xen-blkback/xenbus.c | 17 ++++++++++++++---
> > 2 files changed, 22 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-driver-xen-blkback b/Documentation/ABI/testing/sysfs-driver-xen-blkback
> > index ecb7942ff146..0c42285c75ee 100644
> > --- a/Documentation/ABI/testing/sysfs-driver-xen-blkback
> > +++ b/Documentation/ABI/testing/sysfs-driver-xen-blkback
> > @@ -35,3 +35,11 @@ Description:
> > controls the duration in milliseconds that blkback will not
> > cache any page not backed by a grant mapping.
> > The default is 10ms.
> > +
> > +What: /sys/module/xen_blkback/parameters/feature_persistent
> > +Date: September 2020
> > +KernelVersion: 5.10
> > +Contact: SeongJae Park <sjpark@amazon.de>
> > +Description:
> > + Whether to enable the persistent grants feature or not.
> > + The default is 1 (enable).
> > diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
> > index b9aa5d1ac10b..9c03d70469f4 100644
> > --- a/drivers/block/xen-blkback/xenbus.c
> > +++ b/drivers/block/xen-blkback/xenbus.c
> > @@ -879,6 +879,12 @@ static void reclaim_memory(struct xenbus_device *dev)
> >
> > /* ** Connection ** */
> >
> > +/* Enable the persistent grants feature. */
> > +static unsigned int feature_persistent = 1;
>
> Use bool, please.
Oops, I will.
>
> > +module_param_named(feature_persistent, feature_persistent, int, 0644);
>
> module_param()
>
> > +MODULE_PARM_DESC(feature_persistent,
> > + "Enables the persistent grants feature");
> > +
> > /*
> > * Write the physical details regarding the block device to the store, and
> > * switch to Connected state.
> > @@ -906,7 +912,8 @@ static void connect(struct backend_info *be)
> >
> > xen_blkbk_barrier(xbt, be, be->blkif->vbd.flush_support);
> >
> > - err = xenbus_printf(xbt, dev->nodename, "feature-persistent", "%u", 1);
> > + err = xenbus_printf(xbt, dev->nodename, "feature-persistent", "%u",
> > + feature_persistent ? 1 : 0);
>
> Using bool above should allow to just use the value of
> feature_persistent here.
Indeed. I will fix these as you recommended in the next spin.
Thanks,
SeongJae Park
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] xen-blkback: add a parameter for disabling of persistent grants
2020-09-22 7:01 [PATCH] xen-blkback: add a parameter for disabling of persistent grants SeongJae Park
2020-09-22 7:18 ` Jürgen Groß
@ 2020-09-22 7:32 ` Roger Pau Monné
2020-09-22 7:57 ` SeongJae Park
2020-09-23 20:09 ` Konrad Rzeszutek Wilk
2 siblings, 1 reply; 11+ messages in thread
From: Roger Pau Monné @ 2020-09-22 7:32 UTC (permalink / raw)
To: SeongJae Park
Cc: konrad.wilk, SeongJae Park, axboe, aliguori, amit, mheyne,
linux-block, xen-devel, linux-kernel
On Tue, Sep 22, 2020 at 09:01:25AM +0200, SeongJae Park wrote:
> From: SeongJae Park <sjpark@amazon.de>
>
> Persistent grants feature provides high scalability. On some small
> systems, however, it could incur data copy overhead[1] and thus it is
> required to be disabled. But, there is no option to disable it. For
> the reason, this commit adds a module parameter for disabling of the
> feature.
Have you considered adding a similar option for blkfront?
>
> [1] https://wiki.xen.org/wiki/Xen_4.3_Block_Protocol_Scalability
>
> Signed-off-by: Anthony Liguori <aliguori@amazon.com>
> Signed-off-by: SeongJae Park <sjpark@amazon.de>
> ---
> .../ABI/testing/sysfs-driver-xen-blkback | 8 ++++++++
> drivers/block/xen-blkback/xenbus.c | 17 ++++++++++++++---
> 2 files changed, 22 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-driver-xen-blkback b/Documentation/ABI/testing/sysfs-driver-xen-blkback
> index ecb7942ff146..0c42285c75ee 100644
> --- a/Documentation/ABI/testing/sysfs-driver-xen-blkback
> +++ b/Documentation/ABI/testing/sysfs-driver-xen-blkback
> @@ -35,3 +35,11 @@ Description:
> controls the duration in milliseconds that blkback will not
> cache any page not backed by a grant mapping.
> The default is 10ms.
> +
> +What: /sys/module/xen_blkback/parameters/feature_persistent
> +Date: September 2020
> +KernelVersion: 5.10
> +Contact: SeongJae Park <sjpark@amazon.de>
> +Description:
> + Whether to enable the persistent grants feature or not.
> + The default is 1 (enable).
I would add that this option only takes effect on newly created
backends, existing backends when the option is set will continue to
use persistent grants.
For already running backends you could drain the buffer of persistent
grants and flip the option, but that's more complex and not required.
> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
> index b9aa5d1ac10b..9c03d70469f4 100644
> --- a/drivers/block/xen-blkback/xenbus.c
> +++ b/drivers/block/xen-blkback/xenbus.c
> @@ -879,6 +879,12 @@ static void reclaim_memory(struct xenbus_device *dev)
>
> /* ** Connection ** */
>
> +/* Enable the persistent grants feature. */
> +static unsigned int feature_persistent = 1;
> +module_param_named(feature_persistent, feature_persistent, int, 0644);
> +MODULE_PARM_DESC(feature_persistent,
> + "Enables the persistent grants feature");
> +
> /*
> * Write the physical details regarding the block device to the store, and
> * switch to Connected state.
> @@ -906,7 +912,8 @@ static void connect(struct backend_info *be)
>
> xen_blkbk_barrier(xbt, be, be->blkif->vbd.flush_support);
>
> - err = xenbus_printf(xbt, dev->nodename, "feature-persistent", "%u", 1);
> + err = xenbus_printf(xbt, dev->nodename, "feature-persistent", "%u",
> + feature_persistent ? 1 : 0);
You can avoid writing the feature altogether if it's not enabled,
there's no need to set feature-persistent = 0.
Thanks, Roger.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] xen-blkback: add a parameter for disabling of persistent grants
2020-09-22 7:32 ` Roger Pau Monné
@ 2020-09-22 7:57 ` SeongJae Park
0 siblings, 0 replies; 11+ messages in thread
From: SeongJae Park @ 2020-09-22 7:57 UTC (permalink / raw)
To: Roger Pau Monné
Cc: SeongJae Park, konrad.wilk, SeongJae Park, axboe, aliguori, amit,
mheyne, linux-block, xen-devel, linux-kernel
On Tue, 22 Sep 2020 09:32:02 +0200 "Roger Pau Monné" <roger.pau@citrix.com> wrote:
> On Tue, Sep 22, 2020 at 09:01:25AM +0200, SeongJae Park wrote:
> > From: SeongJae Park <sjpark@amazon.de>
> >
> > Persistent grants feature provides high scalability. On some small
> > systems, however, it could incur data copy overhead[1] and thus it is
> > required to be disabled. But, there is no option to disable it. For
> > the reason, this commit adds a module parameter for disabling of the
> > feature.
>
> Have you considered adding a similar option for blkfront?
I will add yet another option for blkfront in the next spin.
>
> >
> > [1] https://wiki.xen.org/wiki/Xen_4.3_Block_Protocol_Scalability
> >
> > Signed-off-by: Anthony Liguori <aliguori@amazon.com>
> > Signed-off-by: SeongJae Park <sjpark@amazon.de>
> > ---
> > .../ABI/testing/sysfs-driver-xen-blkback | 8 ++++++++
> > drivers/block/xen-blkback/xenbus.c | 17 ++++++++++++++---
> > 2 files changed, 22 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-driver-xen-blkback b/Documentation/ABI/testing/sysfs-driver-xen-blkback
> > index ecb7942ff146..0c42285c75ee 100644
> > --- a/Documentation/ABI/testing/sysfs-driver-xen-blkback
> > +++ b/Documentation/ABI/testing/sysfs-driver-xen-blkback
> > @@ -35,3 +35,11 @@ Description:
> > controls the duration in milliseconds that blkback will not
> > cache any page not backed by a grant mapping.
> > The default is 10ms.
> > +
> > +What: /sys/module/xen_blkback/parameters/feature_persistent
> > +Date: September 2020
> > +KernelVersion: 5.10
> > +Contact: SeongJae Park <sjpark@amazon.de>
> > +Description:
> > + Whether to enable the persistent grants feature or not.
> > + The default is 1 (enable).
>
> I would add that this option only takes effect on newly created
> backends, existing backends when the option is set will continue to
> use persistent grants.
>
> For already running backends you could drain the buffer of persistent
> grants and flip the option, but that's more complex and not required.
You're right, I will add the description.
>
> > diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
> > index b9aa5d1ac10b..9c03d70469f4 100644
> > --- a/drivers/block/xen-blkback/xenbus.c
> > +++ b/drivers/block/xen-blkback/xenbus.c
> > @@ -879,6 +879,12 @@ static void reclaim_memory(struct xenbus_device *dev)
> >
> > /* ** Connection ** */
> >
> > +/* Enable the persistent grants feature. */
> > +static unsigned int feature_persistent = 1;
> > +module_param_named(feature_persistent, feature_persistent, int, 0644);
> > +MODULE_PARM_DESC(feature_persistent,
> > + "Enables the persistent grants feature");
> > +
> > /*
> > * Write the physical details regarding the block device to the store, and
> > * switch to Connected state.
> > @@ -906,7 +912,8 @@ static void connect(struct backend_info *be)
> >
> > xen_blkbk_barrier(xbt, be, be->blkif->vbd.flush_support);
> >
> > - err = xenbus_printf(xbt, dev->nodename, "feature-persistent", "%u", 1);
> > + err = xenbus_printf(xbt, dev->nodename, "feature-persistent", "%u",
> > + feature_persistent ? 1 : 0);
>
> You can avoid writing the feature altogether if it's not enabled,
> there's no need to set feature-persistent = 0.
Agreed. I will do so in the next spin.
Thanks,
SeongJae Park
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] xen-blkback: add a parameter for disabling of persistent grants
2020-09-22 7:01 [PATCH] xen-blkback: add a parameter for disabling of persistent grants SeongJae Park
2020-09-22 7:18 ` Jürgen Groß
2020-09-22 7:32 ` Roger Pau Monné
@ 2020-09-23 20:09 ` Konrad Rzeszutek Wilk
2020-09-24 6:26 ` SeongJae Park
2020-09-24 10:13 ` Roger Pau Monné
2 siblings, 2 replies; 11+ messages in thread
From: Konrad Rzeszutek Wilk @ 2020-09-23 20:09 UTC (permalink / raw)
To: SeongJae Park
Cc: roger.pau, SeongJae Park, axboe, aliguori, amit, mheyne,
linux-block, xen-devel, linux-kernel
On Tue, Sep 22, 2020 at 09:01:25AM +0200, SeongJae Park wrote:
> From: SeongJae Park <sjpark@amazon.de>
>
> Persistent grants feature provides high scalability. On some small
> systems, however, it could incur data copy overhead[1] and thus it is
> required to be disabled. But, there is no option to disable it. For
> the reason, this commit adds a module parameter for disabling of the
> feature.
Would it be better suited to have it per guest?
>
> [1] https://wiki.xen.org/wiki/Xen_4.3_Block_Protocol_Scalability
>
> Signed-off-by: Anthony Liguori <aliguori@amazon.com>
> Signed-off-by: SeongJae Park <sjpark@amazon.de>
> ---
> .../ABI/testing/sysfs-driver-xen-blkback | 8 ++++++++
> drivers/block/xen-blkback/xenbus.c | 17 ++++++++++++++---
> 2 files changed, 22 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-driver-xen-blkback b/Documentation/ABI/testing/sysfs-driver-xen-blkback
> index ecb7942ff146..0c42285c75ee 100644
> --- a/Documentation/ABI/testing/sysfs-driver-xen-blkback
> +++ b/Documentation/ABI/testing/sysfs-driver-xen-blkback
> @@ -35,3 +35,11 @@ Description:
> controls the duration in milliseconds that blkback will not
> cache any page not backed by a grant mapping.
> The default is 10ms.
> +
> +What: /sys/module/xen_blkback/parameters/feature_persistent
> +Date: September 2020
> +KernelVersion: 5.10
> +Contact: SeongJae Park <sjpark@amazon.de>
> +Description:
> + Whether to enable the persistent grants feature or not.
> + The default is 1 (enable).
> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
> index b9aa5d1ac10b..9c03d70469f4 100644
> --- a/drivers/block/xen-blkback/xenbus.c
> +++ b/drivers/block/xen-blkback/xenbus.c
> @@ -879,6 +879,12 @@ static void reclaim_memory(struct xenbus_device *dev)
>
> /* ** Connection ** */
>
> +/* Enable the persistent grants feature. */
> +static unsigned int feature_persistent = 1;
> +module_param_named(feature_persistent, feature_persistent, int, 0644);
> +MODULE_PARM_DESC(feature_persistent,
> + "Enables the persistent grants feature");
> +
> /*
> * Write the physical details regarding the block device to the store, and
> * switch to Connected state.
> @@ -906,7 +912,8 @@ static void connect(struct backend_info *be)
>
> xen_blkbk_barrier(xbt, be, be->blkif->vbd.flush_support);
>
> - err = xenbus_printf(xbt, dev->nodename, "feature-persistent", "%u", 1);
> + err = xenbus_printf(xbt, dev->nodename, "feature-persistent", "%u",
> + feature_persistent ? 1 : 0);
> if (err) {
> xenbus_dev_fatal(dev, err, "writing %s/feature-persistent",
> dev->nodename);
> @@ -1093,8 +1100,12 @@ static int connect_ring(struct backend_info *be)
> xenbus_dev_fatal(dev, err, "unknown fe protocol %s", protocol);
> return -ENOSYS;
> }
> - pers_grants = xenbus_read_unsigned(dev->otherend, "feature-persistent",
> - 0);
> + if (feature_persistent)
> + pers_grants = xenbus_read_unsigned(dev->otherend,
> + "feature-persistent", 0);
> + else
> + pers_grants = 0;
> +
> blkif->vbd.feature_gnt_persistent = pers_grants;
> blkif->vbd.overflow_max_grants = 0;
>
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] xen-blkback: add a parameter for disabling of persistent grants
2020-09-23 20:09 ` Konrad Rzeszutek Wilk
@ 2020-09-24 6:26 ` SeongJae Park
2020-09-24 10:13 ` Roger Pau Monné
1 sibling, 0 replies; 11+ messages in thread
From: SeongJae Park @ 2020-09-24 6:26 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: SeongJae Park, roger.pau, SeongJae Park, axboe, aliguori, amit,
mheyne, linux-block, xen-devel, linux-kernel
On Wed, 23 Sep 2020 16:09:30 -0400 Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> On Tue, Sep 22, 2020 at 09:01:25AM +0200, SeongJae Park wrote:
> > From: SeongJae Park <sjpark@amazon.de>
> >
> > Persistent grants feature provides high scalability. On some small
> > systems, however, it could incur data copy overhead[1] and thus it is
> > required to be disabled. But, there is no option to disable it. For
> > the reason, this commit adds a module parameter for disabling of the
> > feature.
>
> Would it be better suited to have it per guest?
The latest version of this patchset[1] supports blkfront side disablement.
Could that partially solves your concern?
[1] https://lore.kernel.org/xen-devel/20200923061841.20531-1-sjpark@amazon.com/
Thanks,
SeongJae Park
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] xen-blkback: add a parameter for disabling of persistent grants
2020-09-23 20:09 ` Konrad Rzeszutek Wilk
2020-09-24 6:26 ` SeongJae Park
@ 2020-09-24 10:13 ` Roger Pau Monné
2020-09-24 10:27 ` SeongJae Park
1 sibling, 1 reply; 11+ messages in thread
From: Roger Pau Monné @ 2020-09-24 10:13 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: SeongJae Park, SeongJae Park, axboe, aliguori, amit, mheyne,
linux-block, xen-devel, linux-kernel
On Wed, Sep 23, 2020 at 04:09:30PM -0400, Konrad Rzeszutek Wilk wrote:
> On Tue, Sep 22, 2020 at 09:01:25AM +0200, SeongJae Park wrote:
> > From: SeongJae Park <sjpark@amazon.de>
> >
> > Persistent grants feature provides high scalability. On some small
> > systems, however, it could incur data copy overhead[1] and thus it is
> > required to be disabled. But, there is no option to disable it. For
> > the reason, this commit adds a module parameter for disabling of the
> > feature.
>
> Would it be better suited to have it per guest?
I think having a per-backend policy that could be specified at the
toolstack level would be nice, but I see that as a further
improvement.
Having a global backend domain policy of whether persistent grants are
enabled or not seems desirable, and if someone wants even more fine
grained control this change is AFAICT not incompatible with a
per-backend option anyway.
Roger.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] xen-blkback: add a parameter for disabling of persistent grants
2020-09-24 10:13 ` Roger Pau Monné
@ 2020-09-24 10:27 ` SeongJae Park
2020-09-24 10:47 ` Roger Pau Monné
0 siblings, 1 reply; 11+ messages in thread
From: SeongJae Park @ 2020-09-24 10:27 UTC (permalink / raw)
To: Roger Pau Monné
Cc: Konrad Rzeszutek Wilk, SeongJae Park, SeongJae Park, axboe,
aliguori, amit, mheyne, linux-block, xen-devel, linux-kernel
On Thu, 24 Sep 2020 12:13:44 +0200 "Roger Pau Monné" <roger.pau@citrix.com> wrote:
> On Wed, Sep 23, 2020 at 04:09:30PM -0400, Konrad Rzeszutek Wilk wrote:
> > On Tue, Sep 22, 2020 at 09:01:25AM +0200, SeongJae Park wrote:
> > > From: SeongJae Park <sjpark@amazon.de>
> > >
> > > Persistent grants feature provides high scalability. On some small
> > > systems, however, it could incur data copy overhead[1] and thus it is
> > > required to be disabled. But, there is no option to disable it. For
> > > the reason, this commit adds a module parameter for disabling of the
> > > feature.
> >
> > Would it be better suited to have it per guest?
>
> I think having a per-backend policy that could be specified at the
> toolstack level would be nice, but I see that as a further
> improvement.
Agreed.
>
> Having a global backend domain policy of whether persistent grants are
> enabled or not seems desirable, and if someone wants even more fine
> grained control this change is AFAICT not incompatible with a
> per-backend option anyway.
I think we could extend this design by receiving list of exceptional domains.
For example, if 'feature_persistent' is True and exceptions list has '123,
456', domains of domid 123 and 456 will not use persistent grants, and vice
versa.
I could implement this, but... to be honest, I don't really understand the
needs of the fine-grained control. AFAIU, the problem is 'scalability' vs
'data copy overhead'. So, only small systems would want to turn persistent
grants off. In such a small system, why would we need fine-grained control?
I'm worrying if I would implement and maintain a feature without real use case.
For the reason, I'd like to suggest to keep this as is for now and expand it
with the 'exceptions list' idea or something better, if a real use case comes
out later.
Thanks,
SeongJae Park
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] xen-blkback: add a parameter for disabling of persistent grants
2020-09-24 10:27 ` SeongJae Park
@ 2020-09-24 10:47 ` Roger Pau Monné
2020-09-24 15:59 ` Konrad Rzeszutek Wilk
0 siblings, 1 reply; 11+ messages in thread
From: Roger Pau Monné @ 2020-09-24 10:47 UTC (permalink / raw)
To: SeongJae Park
Cc: Konrad Rzeszutek Wilk, SeongJae Park, axboe, aliguori, amit,
mheyne, linux-block, xen-devel, linux-kernel
On Thu, Sep 24, 2020 at 12:27:14PM +0200, SeongJae Park wrote:
> On Thu, 24 Sep 2020 12:13:44 +0200 "Roger Pau Monné" <roger.pau@citrix.com> wrote:
>
> > On Wed, Sep 23, 2020 at 04:09:30PM -0400, Konrad Rzeszutek Wilk wrote:
> > > On Tue, Sep 22, 2020 at 09:01:25AM +0200, SeongJae Park wrote:
> > > > From: SeongJae Park <sjpark@amazon.de>
> > > >
> > > > Persistent grants feature provides high scalability. On some small
> > > > systems, however, it could incur data copy overhead[1] and thus it is
> > > > required to be disabled. But, there is no option to disable it. For
> > > > the reason, this commit adds a module parameter for disabling of the
> > > > feature.
> > >
> > > Would it be better suited to have it per guest?
> >
> > I think having a per-backend policy that could be specified at the
> > toolstack level would be nice, but I see that as a further
> > improvement.
>
> Agreed.
>
> >
> > Having a global backend domain policy of whether persistent grants are
> > enabled or not seems desirable, and if someone wants even more fine
> > grained control this change is AFAICT not incompatible with a
> > per-backend option anyway.
>
> I think we could extend this design by receiving list of exceptional domains.
> For example, if 'feature_persistent' is True and exceptions list has '123,
> 456', domains of domid 123 and 456 will not use persistent grants, and vice
> versa.
I think that would be quite fragile IMO, I wouldn't recommend relying
on domain IDs.
What I would do instead is add a new attribute to
xl-disk-configuration [0] that allows setting the persistent grants
usage on a per-disk basis, and that should be passed to blkback in a
xenstore node.
> I could implement this, but... to be honest, I don't really understand the
> needs of the fine-grained control. AFAIU, the problem is 'scalability' vs
> 'data copy overhead'. So, only small systems would want to turn persistent
> grants off. In such a small system, why would we need fine-grained control?
> I'm worrying if I would implement and maintain a feature without real use case.
>
> For the reason, I'd like to suggest to keep this as is for now and expand it
> with the 'exceptions list' idea or something better, if a real use case comes
> out later.
I agree. I'm happy to take patches to implement more fine grained
control, but that shouldn't prevent us from having a global policy if
that's useful to users.
Roger.
[0] https://xenbits.xen.org/docs/unstable/man/xl-disk-configuration.5.html
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] xen-blkback: add a parameter for disabling of persistent grants
2020-09-24 10:47 ` Roger Pau Monné
@ 2020-09-24 15:59 ` Konrad Rzeszutek Wilk
0 siblings, 0 replies; 11+ messages in thread
From: Konrad Rzeszutek Wilk @ 2020-09-24 15:59 UTC (permalink / raw)
To: Roger Pau Monné
Cc: SeongJae Park, SeongJae Park, axboe, aliguori, amit, mheyne,
linux-block, xen-devel, linux-kernel
.snip..
> > For the reason, I'd like to suggest to keep this as is for now and expand it
> > with the 'exceptions list' idea or something better, if a real use case comes
> > out later.
>
> I agree. I'm happy to take patches to implement more fine grained
> control, but that shouldn't prevent us from having a global policy if
> that's useful to users.
<nods>
^ permalink raw reply [flat|nested] 11+ messages in thread