| Message ID | 20200922070125.27251-1-sjpark@amazon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen-blkback: add a parameter for disabling of persistent grants | expand |
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
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
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.
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
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 >
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
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.
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
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
.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>
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;