From: SeongJae Park <[email protected]>
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 <[email protected]>
Signed-off-by: SeongJae Park <[email protected]>
---
.../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 <[email protected]>
+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
On 22.09.20 09:01, SeongJae Park wrote:
> From: SeongJae Park <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: SeongJae Park <[email protected]>
> ---
> .../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 <[email protected]>
> +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
On Tue, 22 Sep 2020 09:18:05 +0200 "Jürgen Groß" <[email protected]> wrote:
> On 22.09.20 09:01, SeongJae Park wrote:
> > From: SeongJae Park <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: SeongJae Park <[email protected]>
> > ---
> > .../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 <[email protected]>
> > +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
On Tue, 22 Sep 2020 09:32:02 +0200 "Roger Pau Monné" <[email protected]> wrote:
> On Tue, Sep 22, 2020 at 09:01:25AM +0200, SeongJae Park wrote:
> > From: SeongJae Park <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: SeongJae Park <[email protected]>
> > ---
> > .../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 <[email protected]>
> > +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
On Tue, Sep 22, 2020 at 09:01:25AM +0200, SeongJae Park wrote:
> From: SeongJae Park <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: SeongJae Park <[email protected]>
> ---
> .../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 <[email protected]>
> +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
>
On Wed, 23 Sep 2020 16:09:30 -0400 Konrad Rzeszutek Wilk <[email protected]> wrote:
> On Tue, Sep 22, 2020 at 09:01:25AM +0200, SeongJae Park wrote:
> > From: SeongJae Park <[email protected]>
> >
> > 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/[email protected]/
Thanks,
SeongJae Park
On Thu, 24 Sep 2020 12:13:44 +0200 "Roger Pau Monné" <[email protected]> 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 <[email protected]>
> > >
> > > 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
.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>