From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50859) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bmegC-0000xf-Lq for qemu-devel@nongnu.org; Wed, 21 Sep 2016 06:23:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bmeg8-0008DF-Bm for qemu-devel@nongnu.org; Wed, 21 Sep 2016 06:23:04 -0400 Received: from lhrrgout.huawei.com ([194.213.3.17]:3646) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bmeg7-0008C9-Rz for qemu-devel@nongnu.org; Wed, 21 Sep 2016 06:23:00 -0400 References: <1474014817-41572-1-git-send-email-pradeep.jagadeesh@huawei.com> <20160921001500.0de87dd0@bahia.lab.toulouse-stg.fr.ibm.com> From: Pradeep Jagadeesh Message-ID: <33d1fc4d-8c04-ada5-4f5a-a4396678e4d4@huawei.com> Date: Wed, 21 Sep 2016 12:22:25 +0200 MIME-Version: 1.0 In-Reply-To: <20160921001500.0de87dd0@bahia.lab.toulouse-stg.fr.ibm.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3] fsdev: add IO throttle support to fsdev devices List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Greg Kurz , Pradeep Jagadeesh Cc: "Aneesh Kumar K.V" , Alberto Garcia , qemu-devel@nongnu.org, Claudio Fontana Hi Greg, Thanks for having a look at patchset. See the replies below. > On Fri, 16 Sep 2016 04:33:36 -0400 > Pradeep Jagadeesh wrote: > >> Uses throttling APIs to limit I/O bandwidth and number of operations on the >> fsdev devices. >> >> Signed-off-by: Pradeep Jagadeesh >> --- > > Hi Pradeep, > > Please find some comments below. > >> fsdev/Makefile.objs | 1 + >> fsdev/file-op-9p.h | 3 + >> fsdev/qemu-fsdev-opts.c | 76 ++++++++++++++++++ >> fsdev/qemu-fsdev-throttle.c | 191 ++++++++++++++++++++++++++++++++++++++++++++ >> fsdev/qemu-fsdev-throttle.h | 42 ++++++++++ > > Is the qemu- prefix really needed ? Just followed the previous file naming convention, like qemu-fsdev-opts.c > >> hw/9pfs/9p-local.c | 11 ++- >> hw/9pfs/9p.c | 7 ++ >> hw/9pfs/cofile.c | 3 + >> 8 files changed, 332 insertions(+), 2 deletions(-) >> create mode 100644 fsdev/qemu-fsdev-throttle.c >> create mode 100644 fsdev/qemu-fsdev-throttle.h >> >> This adds the support for the 9p-local driver. >> For now this functionality can be enabled only through qemu cli options. >> QMP interface and support to other drivers need further extensions. >> To make it simple for other drivers, the throttle code has been put in >> separate files. >> >> v1 -> v2: >> >> -Fixed FsContext redeclaration issue >> -Removed couple of function declarations from 9p-throttle.h >> -Fixed some of the .help messages >> >> v2 -> v3: >> >> -Addressed follwing comments by Claudio Fontana >> -Removed redundant memset calls in fsdev_throttle_configure_iolimits function >> -Checking throttle structure validity before initializing other structures >> in fsdev_throttle_configure_iolimits >> >> -Addressed following comments by Greg Kurz >> -Moved the code from 9pfs directory to fsdev directory, because the throttling >> is for the fsdev devices.Renamed the files and functions to fsdev_ from 9pfs_ >> -Renamed throttling cli options to throttling.*, as in QMP cli options >> -Removed some of the unwanted .h files from qemu-fsdev-throttle.[ch] >> -Using throttle_enabled() function to set the thottle enabled flag for fsdev. >> >> diff --git a/fsdev/Makefile.objs b/fsdev/Makefile.objs >> index 1b120a4..2c6da2d 100644 >> --- a/fsdev/Makefile.objs >> +++ b/fsdev/Makefile.objs >> @@ -7,6 +7,7 @@ common-obj-y = qemu-fsdev-dummy.o >> endif >> common-obj-y += qemu-fsdev-opts.o >> >> +common-obj-y += qemu-fsdev-throttle.o >> # Toplevel always builds this; targets without virtio will put it in >> # common-obj-y >> common-obj-$(CONFIG_ALL) += qemu-fsdev-dummy.o >> diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h >> index 6db9fea..33fe822 100644 >> --- a/fsdev/file-op-9p.h >> +++ b/fsdev/file-op-9p.h >> @@ -17,6 +17,7 @@ >> #include >> #include >> #include >> +#include "qemu-fsdev-throttle.h" >> >> #define SM_LOCAL_MODE_BITS 0600 >> #define SM_LOCAL_DIR_MODE_BITS 0700 >> @@ -74,6 +75,7 @@ typedef struct FsDriverEntry { >> char *path; >> int export_flags; >> FileOperations *ops; >> + FsThrottle fst; >> } FsDriverEntry; >> >> typedef struct FsContext >> @@ -83,6 +85,7 @@ typedef struct FsContext >> int export_flags; >> struct xattr_operations **xops; >> struct extended_ops exops; >> + FsThrottle *fst; >> /* fs driver specific data */ >> void *private; >> } FsContext; >> diff --git a/fsdev/qemu-fsdev-opts.c b/fsdev/qemu-fsdev-opts.c >> index 1dd8c7a..395d497 100644 >> --- a/fsdev/qemu-fsdev-opts.c >> +++ b/fsdev/qemu-fsdev-opts.c >> @@ -37,6 +37,82 @@ static QemuOptsList qemu_fsdev_opts = { >> }, { >> .name = "sock_fd", >> .type = QEMU_OPT_NUMBER, >> + }, { >> + .name = "throttling.iops-total", >> + .type = QEMU_OPT_NUMBER, >> + .help = "limit total I/O operations per second", >> + },{ >> + .name = "throttling.iops-read", >> + .type = QEMU_OPT_NUMBER, >> + .help = "limit read operations per second", >> + },{ >> + .name = "throttling.iops-write", >> + .type = QEMU_OPT_NUMBER, >> + .help = "limit write operations per second", >> + },{ >> + .name = "throttling.bps-total", >> + .type = QEMU_OPT_NUMBER, >> + .help = "limit total bytes per second", >> + },{ >> + .name = "throttling.bps-read", >> + .type = QEMU_OPT_NUMBER, >> + .help = "limit read bytes per second", >> + },{ >> + .name = "throttling.bps-write", >> + .type = QEMU_OPT_NUMBER, >> + .help = "limit write bytes per second", >> + },{ >> + .name = "throttling.iops-total-max", >> + .type = QEMU_OPT_NUMBER, >> + .help = "I/O operations burst", >> + },{ >> + .name = "throttling.iops-read-max", >> + .type = QEMU_OPT_NUMBER, >> + .help = "I/O operations read burst", >> + },{ >> + .name = "throttling.iops-write-max", >> + .type = QEMU_OPT_NUMBER, >> + .help = "I/O operations write burst", >> + },{ >> + .name = "throttling.bps-total-max", >> + .type = QEMU_OPT_NUMBER, >> + .help = "total bytes burst", >> + },{ >> + .name = "throttling.bps-read-max", >> + .type = QEMU_OPT_NUMBER, >> + .help = "total bytes read burst", >> + },{ >> + .name = "throttling.bps-write-max", >> + .type = QEMU_OPT_NUMBER, >> + .help = "total bytes write burst", >> + },{ >> + .name = "throttling.iops-total-max-length", >> + .type = QEMU_OPT_NUMBER, >> + .help = "length of the iops-total-max burst period, in seconds", >> + },{ >> + .name = "throttling.iops-read-max-length", >> + .type = QEMU_OPT_NUMBER, >> + .help = "length of the iops-read-max burst period, in seconds", >> + },{ >> + .name = "throttling.iops-write-max-length", >> + .type = QEMU_OPT_NUMBER, >> + .help = "length of the iops-write-max burst period, in seconds", >> + },{ >> + .name = "throttling.bps-total-max-length", >> + .type = QEMU_OPT_NUMBER, >> + .help = "length of the bps-total-max burst period, in seconds", >> + },{ >> + .name = "throttling.bps-read-max-length", >> + .type = QEMU_OPT_NUMBER, >> + .help = "length of the bps-read-max burst period, in seconds", >> + },{ >> + .name = "throttling.bps-write-max-length", >> + .type = QEMU_OPT_NUMBER, >> + .help = "length of the bps-write-max burst period, in seconds", >> + },{ >> + .name = "throttling.iops-size", >> + .type = QEMU_OPT_NUMBER, >> + .help = "when limiting by iops max size of an I/O in bytes", >> }, >> >> { /*End of list */ } >> diff --git a/fsdev/qemu-fsdev-throttle.c b/fsdev/qemu-fsdev-throttle.c >> new file mode 100644 >> index 0000000..0adccd1 >> --- /dev/null >> +++ b/fsdev/qemu-fsdev-throttle.c >> @@ -0,0 +1,191 @@ >> +/* >> + * Fsdev Throttle >> + * >> + * Copyright (C) 2016 Huawei Technologies Duesseldorf GmbH >> + * >> + * Author: Pradeep Jagadeesh >> + * >> + * This work is licensed under the terms of the GNU GPL, version 2 or >> + * (at your option) any later version. >> + * >> + * See the COPYING file in the top-level directory for details. >> + * >> + */ >> + >> +#include "qemu/osdep.h" >> +#include "qemu/error-report.h" >> +#include "qapi/error.h" >> +#include "qemu-fsdev-throttle.h" >> + >> +static bool fsdev_throttle_check_for_wait(FsThrottle *fst, bool is_write) >> +{ >> + if (fst->any_timer_armed[is_write]) { >> + return true; >> + } else { >> + return throttle_schedule_timer(&fst->ts, &fst->tt, is_write); >> + } >> +} >> + >> +static void fsdev_throttle_schedule_next_request(FsThrottle *fst, bool is_write) >> +{ >> + bool must_wait = fsdev_throttle_check_for_wait(fst, is_write); > > Let's add a newline here. Done! > >> + if (!fst->pending_reqs[is_write]) { >> + return; >> + } >> + if (!must_wait) { >> + if (qemu_in_coroutine() && >> + qemu_co_queue_next(&fst->throttled_reqs[is_write])) { >> + ; >> + } else { >> + int64_t now = qemu_clock_get_ns(fst->tt.clock_type); >> + timer_mod(fst->tt.timers[is_write], now + 1); >> + fst->any_timer_armed[is_write] = true; >> + } >> + } >> +} >> + >> +static void fsdev_throttle_timer_cb(FsThrottle *fst, bool is_write) >> +{ >> + bool empty_queue; > > Same here. Done! > >> + qemu_mutex_lock(&fst->lock); >> + fst->any_timer_armed[is_write] = false; >> + qemu_mutex_unlock(&fst->lock); >> + empty_queue = !qemu_co_enter_next(&fst->throttled_reqs[is_write]); >> + if (empty_queue) { >> + qemu_mutex_lock(&fst->lock); >> + fsdev_throttle_schedule_next_request(fst, is_write); >> + qemu_mutex_unlock(&fst->lock); >> + } >> +} >> + >> +bool fsdev_throttle_enabled(FsThrottle *fst) >> +{ >> + return fst->enabled; >> +} >> + >> +static void fsdev_iolimits_enable(FsThrottle *fst) >> +{ >> + fst->enabled = true; >> +} >> + >> +static void fsdev_iolimits_disable(FsThrottle *fst) >> +{ >> + fst->enabled = false; >> +} >> + > > fsdev_throttle_configure_iolimits() is the only function that will ever > write to fst->enabled. The helpers seem to indicate the opposite. It is > better to open code in fsdev_throttle_configure_iolimits(). You mean just use fst->enabled = true; inside the fsdev_throttle_configure_iolimits().? If that is the case, I thought these helpers are useful in future when we have qmp interfaces to enable,disable the throttle dynamically/run-time. > >> +static void fsdev_throttle_read_timer_cb(void *opaque) >> +{ >> + fsdev_throttle_timer_cb(opaque, false); >> +} >> + >> +static void fsdev_throttle_write_timer_cb(void *opaque) >> +{ >> + fsdev_throttle_timer_cb(opaque, true); >> +} >> + >> +static void fsdev_parse_io_limit_opts(QemuOpts *opts, FsThrottle *fst) >> +{ >> + fst->cfg.buckets[THROTTLE_BPS_TOTAL].avg = >> + qemu_opt_get_number(opts, "throttling.bps-total", 0); >> + fst->cfg.buckets[THROTTLE_BPS_READ].avg = >> + qemu_opt_get_number(opts, "throttling.bps-read", 0); >> + fst->cfg.buckets[THROTTLE_BPS_WRITE].avg = >> + qemu_opt_get_number(opts, "throttling.bps-write", 0); >> + fst->cfg.buckets[THROTTLE_OPS_TOTAL].avg = >> + qemu_opt_get_number(opts, "throttling.iops-total", 0); >> + fst->cfg.buckets[THROTTLE_OPS_READ].avg = >> + qemu_opt_get_number(opts, "throttling.iops-read", 0); >> + fst->cfg.buckets[THROTTLE_OPS_WRITE].avg = >> + qemu_opt_get_number(opts, "throttling.iops-write", 0); >> + >> + fst->cfg.buckets[THROTTLE_BPS_TOTAL].max = >> + qemu_opt_get_number(opts, "throttling.bps-total-max", 0); >> + fst->cfg.buckets[THROTTLE_BPS_READ].max = >> + qemu_opt_get_number(opts, "throttling.bps-read-max", 0); >> + fst->cfg.buckets[THROTTLE_BPS_WRITE].max = >> + qemu_opt_get_number(opts, "throttling.bps-write-max", 0); >> + fst->cfg.buckets[THROTTLE_OPS_TOTAL].max = >> + qemu_opt_get_number(opts, "throttling.iops-total-max", 0); >> + fst->cfg.buckets[THROTTLE_OPS_READ].max = >> + qemu_opt_get_number(opts, "throttling.iops-read-max", 0); >> + fst->cfg.buckets[THROTTLE_OPS_WRITE].max = >> + qemu_opt_get_number(opts, "throttling.iops-write-max", 0); >> + >> + fst->cfg.buckets[THROTTLE_BPS_TOTAL].burst_length = >> + qemu_opt_get_number(opts, "throttling.bps-total-max-length", 1); >> + fst->cfg.buckets[THROTTLE_BPS_READ].burst_length = >> + qemu_opt_get_number(opts, "throttling.bps-read-max-length", 1); >> + fst->cfg.buckets[THROTTLE_BPS_WRITE].burst_length = >> + qemu_opt_get_number(opts, "throttling.bps-write-max-length", 1); >> + fst->cfg.buckets[THROTTLE_OPS_TOTAL].burst_length = >> + qemu_opt_get_number(opts, "throttling.iops-total-max-length", 1); >> + fst->cfg.buckets[THROTTLE_OPS_READ].burst_length = >> + qemu_opt_get_number(opts, "throttling.iops-read-max-length", 1); >> + fst->cfg.buckets[THROTTLE_OPS_WRITE].burst_length = >> + qemu_opt_get_number(opts, "throttling.iops-write-max-length", 1); >> + fst->cfg.op_size = >> + qemu_opt_get_number(opts, "throttling.iops-size", 0); >> +} >> + >> +int fsdev_throttle_configure_iolimits(QemuOpts *opts, FsThrottle *fst) >> +{ >> + Error *err = NULL; >> + >> + /* Parse and set populate the cfg structure */ >> + fsdev_parse_io_limit_opts(opts, fst); >> + >> + if (!throttle_is_valid(&fst->cfg, &err)) { >> + error_report("Throttle configuration is not valid"); > > throttle_is_valid() already populates err with a meaningful message and > you should use it: > > error_reportf_err(err, "Throttle configuration is not valid: "); > OK,fixed. >> + return -1; >> + } >> + if (throttle_enabled(&fst->cfg)) { >> + fst->aioctx = qemu_get_aio_context(); >> + if (!fst->aioctx) { >> + error_report("Failed to create AIO Context"); >> + return -1; >> + } > > qemu_get_aio_context() returns qemu_aio_context which is initialized in > qemu_init_main_loop(). It is guarantied to be non NULL and if it is, we'd > better dump core. > OK, Is there any specific dump api that exists in qemu? >> + throttle_init(&fst->ts); >> + throttle_timers_init(&fst->tt, >> + fst->aioctx, >> + QEMU_CLOCK_REALTIME, >> + fsdev_throttle_read_timer_cb, >> + fsdev_throttle_write_timer_cb, >> + fst); >> + throttle_config(&fst->ts, &fst->tt, &fst->cfg); >> + qemu_co_queue_init(&fst->throttled_reqs[0]); >> + qemu_co_queue_init(&fst->throttled_reqs[1]); >> + fst->pending_reqs[0] = 0; >> + fst->pending_reqs[1] = 0; >> + fst->any_timer_armed[0] = false; >> + fst->any_timer_armed[1] = false; >> + qemu_mutex_init(&fst->lock); >> + fsdev_iolimits_enable(fst); >> + } else { >> + fsdev_iolimits_disable(fst); >> + } >> + return 0; >> +} >> + >> +void fsdev_throttle_request(FsThrottle *fst, bool is_write, ssize_t bytes) >> +{ >> + if (fsdev_throttle_enabled(fst)) { >> + qemu_mutex_lock(&fst->lock); >> + bool must_wait = fsdev_throttle_check_for_wait(fst, is_write); > > Even if the QEMU code is supposed to follow C99, I think it is prettier to > declare variables at the beginning of the code block. > > bool must_wait; OK,fixed. > > qemu_mutex_lock(&fst->lock); > must_wait = fsdev_throttle_check_for_wait(fst, is_write); > >> + if (must_wait || fst->pending_reqs[is_write]) { >> + fst->pending_reqs[is_write]++; >> + qemu_mutex_unlock(&fst->lock); >> + qemu_co_queue_wait(&fst->throttled_reqs[is_write]); >> + qemu_mutex_lock(&fst->lock); >> + fst->pending_reqs[is_write]--; >> + } >> + throttle_account(&fst->ts, is_write, bytes); >> + fsdev_throttle_schedule_next_request(fst, is_write); >> + qemu_mutex_unlock(&fst->lock); >> + } >> +} >> + >> +void fsdev_throttle_cleanup(FsThrottle *fst) >> +{ >> + throttle_timers_destroy(&fst->tt); >> + qemu_mutex_destroy(&fst->lock); >> +} >> diff --git a/fsdev/qemu-fsdev-throttle.h b/fsdev/qemu-fsdev-throttle.h >> new file mode 100644 >> index 0000000..73dd367 >> --- /dev/null >> +++ b/fsdev/qemu-fsdev-throttle.h >> @@ -0,0 +1,42 @@ >> +/* >> + * Fsdev Throttle >> + * >> + * Copyright (C) 2016 Huawei Technologies Duesseldorf GmbH >> + * >> + * Author: Pradeep Jagadeesh >> + * >> + * This work is licensed under the terms of the GNU GPL, version 2 or >> + * (at your option) any later version. >> + * >> + * See the COPYING file in the top-level directory for details. >> + * >> + */ >> + >> +#ifndef _FSDEV_THROTTLE_H >> +#define _FSDEV_THROTTLE_H >> + >> +#include "block/aio.h" >> +#include "qemu/main-loop.h" >> +#include "qemu/coroutine.h" >> +#include "qemu/throttle.h" >> + >> +typedef struct FsThrottle { >> + ThrottleState ts; >> + ThrottleTimers tt; >> + AioContext *aioctx; >> + ThrottleConfig cfg; >> + bool enabled; >> + CoQueue throttled_reqs[2]; >> + unsigned pending_reqs[2]; >> + bool any_timer_armed[2]; >> + QemuMutex lock; >> +} FsThrottle; >> + >> +bool fsdev_throttle_enabled(FsThrottle *); >> + >> +int fsdev_throttle_configure_iolimits(QemuOpts *, FsThrottle *); >> + >> +void fsdev_throttle_request(FsThrottle *, bool , ssize_t); >> + >> +void fsdev_throttle_cleanup(FsThrottle *); >> +#endif /* _FSDEV_THROTTLE_H */ >> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c >> index 3f271fc..b1009fc 100644 >> --- a/hw/9pfs/9p-local.c >> +++ b/hw/9pfs/9p-local.c >> @@ -436,8 +436,8 @@ static ssize_t local_pwritev(FsContext *ctx, V9fsFidOpenState *fs, >> const struct iovec *iov, >> int iovcnt, off_t offset) >> { >> - ssize_t ret >> -; >> + ssize_t ret; >> + >> #ifdef CONFIG_PREADV >> ret = pwritev(fs->fd, iov, iovcnt, offset); >> #else >> @@ -1213,6 +1213,8 @@ static int local_parse_opts(QemuOpts *opts, struct FsDriverEntry *fse) >> const char *sec_model = qemu_opt_get(opts, "security_model"); >> const char *path = qemu_opt_get(opts, "path"); >> >> + FsThrottle *fst = &fse->fst; >> + >> if (!sec_model) { >> error_report("Security model not specified, local fs needs security model"); >> error_printf("valid options are:" >> @@ -1240,6 +1242,11 @@ static int local_parse_opts(QemuOpts *opts, struct FsDriverEntry *fse) >> error_report("fsdev: No path specified"); >> return -1; >> } >> + >> + if (fsdev_throttle_configure_iolimits(opts, fst) < 0) { >> + return -1; >> + } >> + >> fse->path = g_strdup(path); >> >> return 0; >> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c >> index dfe293d..3748ba9 100644 >> --- a/hw/9pfs/9p.c >> +++ b/hw/9pfs/9p.c >> @@ -3490,6 +3490,10 @@ int v9fs_device_realize_common(V9fsState *s, Error **errp) >> error_setg(errp, "share path %s is not a directory", fse->path); >> goto out; >> } >> + >> + /* Throttle structure initialization */ > > I'm still thinking this comment paraphrases the code :) I missed it!:) > >> + s->ctx.fst = &fse->fst; >> + >> v9fs_path_free(&path); >> >> rc = 0; >> @@ -3504,6 +3508,9 @@ out: >> >> void v9fs_device_unrealize_common(V9fsState *s, Error **errp) >> { >> + if (fsdev_throttle_enabled(s->ctx.fst)) { >> + fsdev_throttle_cleanup(s->ctx.fst); >> + } >> g_free(s->ctx.fs_root); >> g_free(s->tag); >> } >> diff --git a/hw/9pfs/cofile.c b/hw/9pfs/cofile.c >> index 10343c0..65897d5 100644 >> --- a/hw/9pfs/cofile.c >> +++ b/hw/9pfs/cofile.c >> @@ -16,6 +16,7 @@ >> #include "qemu/thread.h" >> #include "qemu/coroutine.h" >> #include "coth.h" >> +#include "fsdev/qemu-fsdev-throttle.h" >> >> int v9fs_co_st_gen(V9fsPDU *pdu, V9fsPath *path, mode_t st_mode, >> V9fsStatDotl *v9stat) >> @@ -242,6 +243,7 @@ int v9fs_co_pwritev(V9fsPDU *pdu, V9fsFidState *fidp, >> int err; >> V9fsState *s = pdu->s; >> >> + fsdev_throttle_request(s->ctx.fst, true, iov->iov_len); > > I think it would make more sense to move this after the cancellation point. Done! > >> if (v9fs_request_cancelled(pdu)) { >> return -EINTR; >> } >> @@ -261,6 +263,7 @@ int v9fs_co_preadv(V9fsPDU *pdu, V9fsFidState *fidp, >> int err; >> V9fsState *s = pdu->s; >> >> + fsdev_throttle_request(s->ctx.fst, false, iov->iov_len); > > Same here. > Done! >> if (v9fs_request_cancelled(pdu)) { >> return -EINTR; >> } > > Cheers. > > -- > Greg > Regards, Pradeep