| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20160722074506.l5nfcmqg3jzsmxzi@mac>
Date: Fri, 22 Jul 2016 09:45:06 +0200
From: Roger Pau Monné <roger.pau@...rix.com>
To: Bob Liu <bob.liu@...cle.com>
CC: <linux-kernel@...r.kernel.org>, <xen-devel@...ts.xenproject.org>,
<konrad.wilk@...cle.com>, <jgross@...e.com>
Subject: Re: [PATCH 3/3] xen-blkfront: dynamic configuration of per-vbd
resources
On Thu, Jul 21, 2016 at 06:08:05PM +0800, Bob Liu wrote:
>
> On 07/21/2016 04:57 PM, Roger Pau Monné wrote:
> > On Fri, Jul 15, 2016 at 05:31:49PM +0800, Bob Liu wrote:
> >> The current VBD layer reserves buffer space for each attached device based on
> >> three statically configured settings which are read at boot time.
> >> * max_indirect_segs: Maximum amount of segments.
> >> * max_ring_page_order: Maximum order of pages to be used for the shared ring.
> >> * max_queues: Maximum of queues(rings) to be used.
> >>
> >> But the storage backend, workload, and guest memory result in very different
> >> tuning requirements. It's impossible to centrally predict application
> >> characteristics so it's best to leave allow the settings can be dynamiclly
> >> adjusted based on workload inside the Guest.
> >>
> >> Usage:
> >> Show current values:
> >> cat /sys/devices/vbd-xxx/max_indirect_segs
> >> cat /sys/devices/vbd-xxx/max_ring_page_order
> >> cat /sys/devices/vbd-xxx/max_queues
> >>
> >> Write new values:
> >> echo <new value> > /sys/devices/vbd-xxx/max_indirect_segs
> >> echo <new value> > /sys/devices/vbd-xxx/max_ring_page_order
> >> echo <new value> > /sys/devices/vbd-xxx/max_queues
> >>
> >> Signed-off-by: Bob Liu <bob.liu@...cle.com>
> >> --
> >> v2: Add device lock and other comments from Konrad.
> >> ---
> >> drivers/block/xen-blkfront.c | 285 ++++++++++++++++++++++++++++++++++++++++++-
> >> 1 file changed, 283 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> >> index 10f46a8..9a5ed22 100644
> >> --- a/drivers/block/xen-blkfront.c
> >> +++ b/drivers/block/xen-blkfront.c
> >> @@ -46,6 +46,7 @@
> >> #include <linux/scatterlist.h>
> >> #include <linux/bitmap.h>
> >> #include <linux/list.h>
> >> +#include <linux/delay.h>
> >>
> >> #include <xen/xen.h>
> >> #include <xen/xenbus.h>
> >> @@ -212,6 +213,11 @@ struct blkfront_info
> >> /* Save uncomplete reqs and bios for migration. */
> >> struct list_head requests;
> >> struct bio_list bio_list;
> >> + /* For dynamic configuration. */
> >> + unsigned int reconfiguring:1;
> >> + int new_max_indirect_segments;
> >> + int new_max_ring_page_order;
> >> + int new_max_queues;
> >> };
> >>
> >> static unsigned int nr_minors;
> >> @@ -1350,6 +1356,31 @@ static void blkif_free(struct blkfront_info *info, int suspend)
> >> for (i = 0; i < info->nr_rings; i++)
> >> blkif_free_ring(&info->rinfo[i]);
> >>
> >> + /* Remove old xenstore nodes. */
> >> + if (info->nr_ring_pages > 1)
> >> + xenbus_rm(XBT_NIL, info->xbdev->nodename, "ring-page-order");
> >> +
> >> + if (info->nr_rings == 1) {
> >> + if (info->nr_ring_pages == 1) {
> >> + xenbus_rm(XBT_NIL, info->xbdev->nodename, "ring-ref");
> >> + } else {
> >> + for (i = 0; i < info->nr_ring_pages; i++) {
> >> + char ring_ref_name[RINGREF_NAME_LEN];
> >> +
> >> + snprintf(ring_ref_name, RINGREF_NAME_LEN, "ring-ref%u", i);
> >> + xenbus_rm(XBT_NIL, info->xbdev->nodename, ring_ref_name);
> >> + }
> >> + }
> >> + } else {
> >> + xenbus_rm(XBT_NIL, info->xbdev->nodename, "multi-queue-num-queues");
> >> +
> >> + for (i = 0; i < info->nr_rings; i++) {
> >> + char queuename[QUEUE_NAME_LEN];
> >> +
> >> + snprintf(queuename, QUEUE_NAME_LEN, "queue-%u", i);
> >> + xenbus_rm(XBT_NIL, info->xbdev->nodename, queuename);
> >> + }
> >> + }
> >> kfree(info->rinfo);
> >> info->rinfo = NULL;
> >> info->nr_rings = 0;
> >> @@ -1772,6 +1803,10 @@ static int talk_to_blkback(struct xenbus_device *dev,
> >> info->nr_ring_pages = 1;
> >> else {
> >> ring_page_order = min(xen_blkif_max_ring_order, max_page_order);
> >> + if (info->new_max_ring_page_order) {
> >
> > Instead of calling this "new_max_ring_page_order", could you just call it
> > max_ring_page_order, iniitalize it to xen_blkif_max_ring_order by default
>
>
> Sure, I can do that.
>
>
> > and use it everywhere instead of xen_blkif_max_ring_order?
>
>
> But "xen_blkif_max_ring_order" still have to be used here, this is the only place "xen_blkif_max_ring_order" is used(except checking the value of it in xlblk_init()).
>
>
> >
> >> + BUG_ON(info->new_max_ring_page_order > max_page_order);
> >> + ring_page_order = info->new_max_ring_page_order;
> >> + }
> >> info->nr_ring_pages = 1 << ring_page_order;
> >> }
> >>
> >> @@ -1895,6 +1930,10 @@ static int negotiate_mq(struct blkfront_info *info)
> >> backend_max_queues = 1;
> >>
> >> info->nr_rings = min(backend_max_queues, xen_blkif_max_queues);
> >> + if (info->new_max_queues) {
> >
> > Same here IMHO, this is going to make the code flow slightly easier to
> > understand.
> >
> >> + BUG_ON(info->new_max_queues > backend_max_queues);
> >> + info->nr_rings = info->new_max_queues;
> >> + }
> >> /* We need at least one ring. */
> >> if (!info->nr_rings)
> >> info->nr_rings = 1;
> >> @@ -2352,11 +2391,227 @@ static void blkfront_gather_backend_features(struct blkfront_info *info)
> >> NULL);
> >> if (err)
> >> info->max_indirect_segments = 0;
> >> - else
> >> + else {
> >> info->max_indirect_segments = min(indirect_segments,
> >> xen_blkif_max_segments);
> >> + if (info->new_max_indirect_segments) {
> >> + BUG_ON(info->new_max_indirect_segments > indirect_segments);
> >> + info->max_indirect_segments = info->new_max_indirect_segments;
> >> + }
> >> + }
> >> +}
> >> +
> >> +static ssize_t max_ring_page_order_show(struct device *dev,
> >> + struct device_attribute *attr, char *page)
> >> +{
> >> + struct blkfront_info *info = dev_get_drvdata(dev);
> >> +
> >> + return sprintf(page, "%u\n", get_order(info->nr_ring_pages * XEN_PAGE_SIZE));
> >> +}
> >> +
> >> +static ssize_t max_indirect_segs_show(struct device *dev,
> >> + struct device_attribute *attr, char *page)
> >> +{
> >> + struct blkfront_info *info = dev_get_drvdata(dev);
> >> +
> >> + return sprintf(page, "%u\n", info->max_indirect_segments);
> >> +}
> >> +
> >> +static ssize_t max_queues_show(struct device *dev,
> >> + struct device_attribute *attr, char *page)
> >> +{
> >> + struct blkfront_info *info = dev_get_drvdata(dev);
> >> +
> >> + return sprintf(page, "%u\n", info->nr_rings);
> >> +}
> >> +
> >> +static ssize_t dynamic_reconfig_device(struct blkfront_info *info, ssize_t count)
> >> +{
> >> + unsigned int i;
> >> + int err = -EBUSY;
> >> +
> >> + /*
> >> + * Make sure no migration in parallel, device lock is actually a
> >> + * mutex.
> >> + */
> >> + if (!device_trylock(&info->xbdev->dev)) {
> >> + pr_err("Fail to acquire dev:%s lock, may be in migration.\n",
> >> + dev_name(&info->xbdev->dev));
> >> + return err;
> >> + }
> >> +
> >> + /*
> >> + * Prevent new requests and guarantee no uncompleted reqs.
> >> + */
> >> + blk_mq_freeze_queue(info->rq);
> >> + if (part_in_flight(&info->gd->part0))
> >> + goto out;
> >> +
> >> + /*
> >> + * Front Backend
> >> + * Switch to XenbusStateClosed
> >> + * frontend_changed():
> >> + * case XenbusStateClosed:
> >> + * xen_blkif_disconnect()
> >> + * Switch to XenbusStateClosed
> >> + * blkfront_resume():
> >> + * frontend_changed():
> >> + * reconnect
> >> + * Wait until XenbusStateConnected
> >> + */
> >> + info->reconfiguring = true;
> >> + xenbus_switch_state(info->xbdev, XenbusStateClosed);
> >> +
> >> + /* Poll every 100ms, 1 minute timeout. */
> >> + for (i = 0; i < 600; i++) {
> >> + /*
> >> + * Wait backend enter XenbusStateClosed, blkback_changed()
> >> + * will clear reconfiguring.
> >> + */
> >> + if (!info->reconfiguring)
> >> + goto resume;
> >> + schedule_timeout_interruptible(msecs_to_jiffies(100));
> >> + }
> >
> > Instead of having this wait, could you just set info->reconfiguring = 1, set
> > the frontend state to XenbusStateClosed and mimic exactly what a resume from
> > suspension does? blkback_changed would have to set the frontend state to
> > InitWait when it detects that the backend has switched to Closed, and call
> > blkfront_resume.
>
>
> I think that won't work.
> In the real "resume" case, the power management system will trigger all ->resume() path.
> But there is no place for dynamic configuration.
Hello,
I think it should be possible to set info->reconfiguring and wait for the
backend to switch to state Closed, at that point we should call blkif_resume
(from blkback_changed) and the backend will follow the reconection.
Roger.
Powered by blists - more mailing lists