[Qemu-devel] [Bug 888016] [NEW] RHEL 6.1 guest fails to boot with vhost
Public bug reported: Tried to boot 6.1 guest on hs22 blade with/without vhost enabled. With vhost enabled, guest aborted with core dump. installed guest with autotest. Command : /usr/local/bin/qemu-system-x86_64 -name 'vm1' -nodefaults -vga std -monitor unix:'/tmp/monitor-humanmonitor1-2008-193209-fc6O',server,nowait -serial unix:'/tmp/serial-2008-193209-fc6O',server,nowait -drive file='/home/pradeep/autotest/client/tests/kvm/images/rhel6.1-64.qed',index=0,if=virtio,cache=none -device virtio-net-pci,netdev=idQhUaOc,mac='9a:b7:ea:c9:0e:0d',id='idVR6XQz' -netdev tap,id=idQhUaOc,vhost=on,script=/home/pradeep/qemu-ifup-latest -m 1024 -smp 8 -vnc :0 -monitor stdio QEMU 0.15.91 monitor - type 'help' for more information (qemu) Aborted (core dumped) host: 2.6.32-214 m/c: hs22 vhost modules are loaded. ** Affects: qemu Importance: Undecided Status: New -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/888016 Title: RHEL 6.1 guest fails to boot with vhost Status in QEMU: New Bug description: Tried to boot 6.1 guest on hs22 blade with/without vhost enabled. With vhost enabled, guest aborted with core dump. installed guest with autotest. Command : /usr/local/bin/qemu-system-x86_64 -name 'vm1' -nodefaults -vga std -monitor unix:'/tmp/monitor-humanmonitor1-2008-193209-fc6O',server,nowait -serial unix:'/tmp/serial-2008-193209-fc6O',server,nowait -drive file='/home/pradeep/autotest/client/tests/kvm/images/rhel6.1-64.qed',index=0,if=virtio,cache=none -device virtio-net-pci,netdev=idQhUaOc,mac='9a:b7:ea:c9:0e:0d',id='idVR6XQz' -netdev tap,id=idQhUaOc,vhost=on,script=/home/pradeep/qemu-ifup-latest -m 1024 -smp 8 -vnc :0 -monitor stdio QEMU 0.15.91 monitor - type 'help' for more information (qemu) Aborted (core dumped) host: 2.6.32-214 m/c: hs22 vhost modules are loaded. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/888016/+subscriptions
[Qemu-devel] [Bug 888016] Re: RHEL 6.1 guest fails to boot with vhost
i hits this issue on host: 3.1.0+ also -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/888016 Title: RHEL 6.1 guest fails to boot with vhost Status in QEMU: New Bug description: Tried to boot 6.1 guest on hs22 blade with/without vhost enabled. With vhost enabled, guest aborted with core dump. installed guest with autotest. Command : /usr/local/bin/qemu-system-x86_64 -name 'vm1' -nodefaults -vga std -monitor unix:'/tmp/monitor-humanmonitor1-2008-193209-fc6O',server,nowait -serial unix:'/tmp/serial-2008-193209-fc6O',server,nowait -drive file='/home/pradeep/autotest/client/tests/kvm/images/rhel6.1-64.qed',index=0,if=virtio,cache=none -device virtio-net-pci,netdev=idQhUaOc,mac='9a:b7:ea:c9:0e:0d',id='idVR6XQz' -netdev tap,id=idQhUaOc,vhost=on,script=/home/pradeep/qemu-ifup-latest -m 1024 -smp 8 -vnc :0 -monitor stdio QEMU 0.15.91 monitor - type 'help' for more information (qemu) Aborted (core dumped) host: 2.6.32-214 m/c: hs22 vhost modules are loaded. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/888016/+subscriptions
[Qemu-devel] [Bug 888016] Re: RHEL 6.1 guest fails to boot with vhost
** Changed in: qemu Assignee: (unassigned) => pradeep (psuriset) -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/888016 Title: RHEL 6.1 guest fails to boot with vhost Status in QEMU: New Bug description: Tried to boot 6.1 guest on hs22 blade with/without vhost enabled. With vhost enabled, guest aborted with core dump. installed guest with autotest. Command : /usr/local/bin/qemu-system-x86_64 -name 'vm1' -nodefaults -vga std -monitor unix:'/tmp/monitor-humanmonitor1-2008-193209-fc6O',server,nowait -serial unix:'/tmp/serial-2008-193209-fc6O',server,nowait -drive file='/home/pradeep/autotest/client/tests/kvm/images/rhel6.1-64.qed',index=0,if=virtio,cache=none -device virtio-net-pci,netdev=idQhUaOc,mac='9a:b7:ea:c9:0e:0d',id='idVR6XQz' -netdev tap,id=idQhUaOc,vhost=on,script=/home/pradeep/qemu-ifup-latest -m 1024 -smp 8 -vnc :0 -monitor stdio QEMU 0.15.91 monitor - type 'help' for more information (qemu) Aborted (core dumped) host: 2.6.32-214 m/c: hs22 vhost modules are loaded. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/888016/+subscriptions
[Qemu-devel] qemu aborts if i add a already registered device from qemu monitor ..
Hi I tried to add a device to guest from upstream qemu monitor using "device_add". Unknowingly i try to add already registered devices from qemu monitor, my qemu monitor is aborted. I don't see a reason to kill monitor. I think abort() is a bit rough. we need a better way to handle it. If a user try to add a already registered device, qemu should convey this to user saying that, this device already registered and an error message should be fine than aborting qemu. QLIST_FOREACH(block, &ram_list.blocks, next) { if (!strcmp(block->idstr, new_block->idstr)) { fprintf(stderr, "RAMBlock \"%s\" already registered, abort!\n", new_block->idstr); abort(); } If i return some other value in above code, instead of abort(), I would need change the code for every device, which i dont want to. Is there a way to check, if device is already enrolled or not in the very beginning of "device_add" call. Thanks Pradeep
[Qemu-devel] [KVM] guest remote migration fails
Migration of guest(both local and remote) fails with 2.6.37-rc8 host Test Procedure: --- A is the source host, B is the destination host: 1. Start the VM on B with the exact same parameters as the VM on A, in migration-listen mode: B: -incoming tcp:0: (or other PORT)) 2. Start the migration (always on the source host): A: migrate -d tcp:B: (or other PORT) 3. Check the status (on A only): A: (qemu) info migrate Expected results: - Migration should complete without any error Actual results: qemu-system-x86_64: VQ 2 size 0x40 Guest index 0xf000 inconsistent with Host index 0x0: delta 0xf000 load of migration failed Guest OS: -- windows Host Kernel: --- 2.6.37.rc8 on HS22 command used: - Source(A): /usr/local/bin/qemu-system-x86_64 -enable-kvm -m 4096 -smp 4 -name win32 -monitor stdio -boot c -drive file=/home/storage/yogi/kvm_autotest_root/images/win7-32.raw,if=none,id=drive-ide0-0-0,boot=on,format=raw,cache=none -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 -netdev tap,script=/home/yog/autotest/client/tests/kvm/scripts/qemu-ifup,id=hostnet0 -device virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:86:e4:97 -usb -device usb-tablet,id=input0 -vnc :0 Destination (B): /usr/local/bin/qemu-system-x86_64 -enable-kvm -m 4096 -smp 4 -name win32 -monitor stdio -boot c -drive file=/home/storage/yogi/kvm_autotest_root/images /win7-32.raw,if=none,id=drive-ide0-0-0,boot=on,format=raw,cache=none -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 -netdev tap,script=/home/yog/autot est/client/tests/kvm/scripts/qemu-ifup,id=hostnet0 -device virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:86:e4:97 -usb -device usb-tablet,id=input0 -vnc :5 -incoming tcp:0:
[Qemu-devel] Re: [PATCH] vhost: disable on tap link down
On Mon, 7 Feb 2011 15:50:01 +0200 "Michael S. Tsirkin" wrote: > qemu makes it possible to disable link at tap > which is not communicated to the guest but > causes all packets to be dropped. > > Handle this with vhost simply by moving to the userspace emulation. > > Note: it might be a good idea to make peer link status match > tap in this case, so the guest gets an event > and updates the carrier state. For now > stay bug for bug compatible with what we used to have. > > Signed-off-by: Michael S. Tsirkin > Reported-by: pradeep > --- > > Untested. > Pradeep, mind trying this patch out and reporting? Hi mst This patch works. Thanks --Pradeep > Thanks! > > hw/virtio-net.c |4 > net.c |7 +++ > 2 files changed, 11 insertions(+), 0 deletions(-) > > diff --git a/hw/virtio-net.c b/hw/virtio-net.c > index 671d952..fc2d6f5 100644 > --- a/hw/virtio-net.c > +++ b/hw/virtio-net.c > @@ -112,6 +112,10 @@ static void virtio_net_vhost_status(VirtIONet > *n, uint8_t status) return; > } > > +if (n->nic->nc.peer->link_down) { > +return; > +} > + > if (!tap_get_vhost_net(n->nic->nc.peer)) { > return; > } > diff --git a/net.c b/net.c > index 9ba5be2..57ee997 100644 > --- a/net.c > +++ b/net.c > @@ -1324,6 +1324,13 @@ done: > if (vc->info->link_status_changed) { > vc->info->link_status_changed(vc); > } > + > +/* Notify peer. Don't update peer link status: this makes it > possible to > + * disconnect from host network without notifying the guest. > + * FIXME: is this useful? Could just be an artifact of vlan > support. */ > +if (vc->peer && vc->peer->info->link_status_changed) { > +vc->peer->info->link_status_changed(vc->peer); > +} > return 0; > } >
[Qemu-devel] Re: [PATCH] vhost: disable on tap link down
On Tue, 8 Feb 2011 17:41:12 +0200 "Michael S. Tsirkin" wrote: > On Tue, Feb 08, 2011 at 05:40:58PM +0530, pradeep wrote: > > On Mon, 7 Feb 2011 15:50:01 +0200 > > "Michael S. Tsirkin" wrote: > > > > > qemu makes it possible to disable link at tap > > > which is not communicated to the guest but > > > causes all packets to be dropped. > > > > > > Handle this with vhost simply by moving to the userspace > > > emulation. > > > > > > Note: it might be a good idea to make peer link status match > > > tap in this case, so the guest gets an event > > > and updates the carrier state. For now > > > stay bug for bug compatible with what we used to have. > > > > > > Signed-off-by: Michael S. Tsirkin > > > Reported-by: pradeep > > > --- > > > > > > Untested. > > > Pradeep, mind trying this patch out and reporting? > > > > Hi mst > > > > This patch works. Thanks > > Strange actually. Did you put the link down before guest booted? I > went to test it with set link after guest is up, and it didn't work, > I needed this on top - can you ack this in your setup as well pls? a. Old one dint. New patch works for me. Thanks mst --Pradeep > > diff --git a/hw/virtio-net.c b/hw/virtio-net.c > index fc2d6f5..3e3d73a 100644 > --- a/hw/virtio-net.c > +++ b/hw/virtio-net.c > @@ -112,14 +112,11 @@ static void virtio_net_vhost_status(VirtIONet > *n, uint8_t status) return; > } > > -if (n->nic->nc.peer->link_down) { > -return; > -} > - > if (!tap_get_vhost_net(n->nic->nc.peer)) { > return; > } > -if (!!n->vhost_started == virtio_net_started(n, status)) { > +if (!!n->vhost_started == virtio_net_started(n, status) && > +!n->nic->nc.peer->link_down) { > return; > } > if (!n->vhost_started) {
[Qemu-devel] [PATCH] 9pfs: add support for IO limits to 9p-local driver
Uses throttling APIs to limit I/O bandwidth and number of operations on the devices which use 9p-local driver. Signed-off-by: Pradeep fsdev/file-op-9p.h | 3 + fsdev/qemu-fsdev-opts.c | 52 + hw/9pfs/9p-local.c | 18 - hw/9pfs/9p-throttle.c | 199 hw/9pfs/9p-throttle.h | 55 + hw/9pfs/9p.c| 7 ++ hw/9pfs/Makefile.objs | 1 + 7 files changed, 333 insertions(+), 2 deletions(-) create mode 100644 hw/9pfs/9p-throttle.c create mode 100644 hw/9pfs/9p-throttle.h Hi All, 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. Cheers, Pradeep diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h index 6db9fea..e86b91a 100644 --- a/fsdev/file-op-9p.h +++ b/fsdev/file-op-9p.h @@ -17,6 +17,7 @@ #include #include #include +#include "hw/9pfs/9p-throttle.h" #define SM_LOCAL_MODE_BITS0600 #define SM_LOCAL_DIR_MODE_BITS0700 @@ -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..f5e5e67 100644 --- a/fsdev/qemu-fsdev-opts.c +++ b/fsdev/qemu-fsdev-opts.c @@ -37,6 +37,58 @@ static QemuOptsList qemu_fsdev_opts = { }, { .name = "sock_fd", .type = QEMU_OPT_NUMBER, +}, { +.name = "bps", +.type = QEMU_OPT_NUMBER, +.help = "total bytes burst", +}, { +.name = "bps_rd", +.type = QEMU_OPT_NUMBER, +.help = "total bytes read burst", +}, { +.name = "bps_wr", +.type = QEMU_OPT_NUMBER, +.help = "total bytes write burst", +}, { +.name = "iops", +.type = QEMU_OPT_NUMBER, +.help = "total io operations burst", +}, { +.name = "iops_rd", +.type = QEMU_OPT_NUMBER, +.help = "total io operations read burst", +}, { +.name = "iops_wr", +.type = QEMU_OPT_NUMBER, +.help = "total io operations write burst", +}, { +.name = "bps_max", +.type = QEMU_OPT_NUMBER, +.help = "maximum bytes burst", +}, { +.name = "bps_rd_max", +.type = QEMU_OPT_NUMBER, +.help = "maximum bytes read burst", +}, { +.name = "bps_wr_max", +.type = QEMU_OPT_NUMBER, +.help = "maximum bytes write burst", +}, { +.name = "iops_max", +.type = QEMU_OPT_NUMBER, +.help = "maximum io operations burst", +}, { +.name = "iops_rd_max", +.type = QEMU_OPT_NUMBER, +.help = "maximum io operations read burst", +}, { +.name = "iops_wr_max", +.type = QEMU_OPT_NUMBER, +.help = "maximum io operations write burst", +}, { +.name = "iops_size", +.type = QEMU_OPT_NUMBER, +.help = "io iops-size", }, { /*End of list */ } diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c index 3f271fc..7097f46 100644 --- a/hw/9pfs/9p-local.c +++ b/hw/9pfs/9p-local.c @@ -420,6 +420,8 @@ static ssize_t local_preadv(FsContext *ctx, V9fsFidOpenState *fs, const struct iovec *iov, int iovcnt, off_t offset) { +throttle_request(ctx, false, iov->iov_len); + #ifdef CONFIG_PREADV return preadv(fs->fd, iov, iovcnt, offset); #else @@ -436,8 +438,10 @@ static ssize_t local_pwritev(FsContext *ctx, V9fsFidOpenState *fs, const struct iovec *iov, int iovcnt, off_t offset) { -ssize_t ret -; +ssize_t ret; + +throttle_request(ctx, true, iov->iov_len); + #ifdef CONFIG_PREADV ret = pwritev(fs->fd, iov, iovcnt, offset); #else @@ -1213,6 +1217,9 @@ static int local_parse_opts(QemuOpts *opts, struct FsDriverEntry *fse) const char *sec_model = qemu_opt_get(opts, "security_model"); const char *path = qemu_
[Qemu-devel] Guest bandwidth setting
Hi All, I am new to QEMU. I am implementing some functionality where in I need to set the bandwidth control for my guest vms.Please let me know how can I do it. I am looking into net/net.c file,I dint find any leads.I tried two ways. I am trying to find how to set virtual nics(in my case e1000) speed,I did not find any ways to do that. If I devide the packet into smaller packets in qemu_deliver_packet() function I am not able to transfer the other half of the packet. Please let me know how can I resolve this. Thanks in advance Pradeep
[Qemu-devel] qemu projects
Hi All, I am new to qemu-kvm.But have good knowledge about virtulization and cloud computing.I would like to do some projects in qemu-kvm.Please let me know how can I start. Please suggest me some small projects which I can take up and finish in 2-3 weeks. Regards, Pradeep
Re: [Qemu-devel] qemu projects
Hi Stefan, Thanks I will follow your suggestion. Regards, Pradeep On 24 April 2014 14:10, Stefan Hajnoczi wrote: > On Wed, Apr 09, 2014 at 07:28:15PM +0200, Pradeep Kiruvale wrote: > > I am new to qemu-kvm.But have good knowledge about virtulization and > cloud > > computing.I would like to do some projects in qemu-kvm.Please let me know > > how can I start. > > > > Please suggest me some small projects which I can take up and finish in > 2-3 > > weeks. > > Hi Pradeep, > This request is too general and you will probably not receive the kinds > of responses you hoped for. > > If you want to get into QEMU development but don't know where to start, > follow the mailing list and bug tracker. Maybe you'll find bug reports > that look interesting and that you can fix. > > Good luck! > Stefan >
[Qemu-devel] DMAR not present in guest
Hi All, I am using linux 4.0 kernel on host side and 3.19 on guest side. I have enabled the iommu and nested virtualization on host. The DMAR is visible in the host side and when I start a VM using qemu-kvm, the same DMAR not visible on the guest side. But it says IOMMU is enabled when I look at dmesg. The qemu verion I am using is 2.3.94 The cli is as below sudo qemu-system-x86_64 -machine q35 -m 3G -enable-kvm -smp 4 \ -cpu kvm64,-kvm_pv_eoi,-kvm_steal_time,-kvm_asyncpf,-kvmclock,+vmx,+x2apic \ -net nic,macaddr=52:54:00:37:4d:10 \ -net tap,script=/etc/qemu-ifup,downscript=/etc/qemu-ifdown \ -drive file=ubuntu_15.qcow,id=disk,if=none \ -device ide-hd,drive=disk -serial stdio -serial vc \ -device intel-hda,addr=1b.0 -device hda-duplex Please let me know am I missing something here? how can I resolve this. Thanks & Regards, Pradeep
[Qemu-devel] Virtio-9p
Hi All, Is virtio-9p-pci device only supports the fsdev deices? I am trying to use -drive option for applying QoS for block device using Virtio-9p-pci device, but failing to create/add a device other than fsdev. Can you please help me on this? Regards, Pradeep
Re: [Qemu-devel] Virtio-9p
Hi Greg, Thanks for the reply. Let me put it this way, virtio-blk-pci is used for block IO on the devices shared between the guest and the host. Here I want to share the file and have QoS between the guests. So I am using the Virtio-9p-pci. Basically I want to have QoS for virtio-9p-pci. Regards, Pradeep On 30 March 2016 at 16:13, Greg Kurz wrote: > On Wed, 30 Mar 2016 14:10:38 +0200 > Pradeep Kiruvale wrote: > > > Hi All, > > > > Is virtio-9p-pci device only supports the fsdev deices? I am trying to > use > > -drive option for applying QoS for block device using Virtio-9p-pci > device, > > but failing to create/add a device other than fsdev. Can you please help > me > > on this? > > > > Regards, > > Pradeep > > Hi Pradeep, > > Not sure to catch what you want to do but I confirm that virti-9p-pci only > supports > fsdev... if you want a block device, why don't you use virtio-blk-pci ? > > Cheers. > > -- > Greg > >
Re: [Qemu-devel] Virtio-9p
Hi Greg, Thanks for your reply. > > I'm Cc'ing qemu-devel like in your previous posts, so QEMU experts may > jump in. > > > What I understand from the requirement for our project is if we use > > virtio-blk it caches the pages in the guest. We would like to avoid that > > AFAIK this is true when you pass cache=none to -drive on the QEMU command > line. But there are other options such as writeback or writethrough, which > rely on the host page cache. > > Yes, we did explore these options already. > > and also we want to share those pages across multiple guests and when > they > > need some data they should get it from the host instead of using the > cached > > data at each guest. > > > > So you want all the guests to attach to the same block device or backing > file in > the host, correct ? AFAIK we cannot do that with virtio-blk indeed... and > virtio-9p > is only about sharing files, not block devices. > > Yes, we want to share the files. > Maybe you could share a big file between the host and all guests with 9p, > and > each guest can use the loop device to access the file as a block device... > but > even then, you'd have to deal with concurrent accesses... > > > Basically we are trying to cut down the memory foot print of the guests. > > > > If you're using KVM and your guests run the same distro or application, > you may try to use KSM (Kernel Same-page Merging) in the host. > > We are using these things also, we want to reduce the foot print as much as possible. Regards, Pradeep > > Regards, > > Pradeep > > > > On 31 March 2016 at 18:12, Greg Kurz wrote: > > > > > On Wed, 30 Mar 2016 16:27:48 +0200 > > > Pradeep Kiruvale wrote: > > > > > > > Hi Greg, > > > > > > > > > > Hi Pradeep, > > > > > > > Thanks for the reply. > > > > > > > > Let me put it this way, virtio-blk-pci is used for block IO on the > > > devices > > > > shared between the guest and the host. > > > > > > I don't really understand the "devices shared between the guest and the > > > host" wording... virtio-blk-pci exposes a virtio-blk device through PCI > > > to the guest. The virtio-blk device can be backed by a file or a block > > > device from the host. > > > > > > > Here I want to share the file and have QoS between the guests. So I > am > > > > using the Virtio-9p-pci. > > > > > > > > > > What file ? > > > > > > > Basically I want to have QoS for virtio-9p-pci. > > > > > > > > > > Can you provide a more detailed scenario on the result you want to > reach ? > > > > > > > Regards, > > > > Pradeep > > > > > > > > > > Cheers. > > > > > > -- > > > Greg > > > > > > > On 30 March 2016 at 16:13, Greg Kurz > wrote: > > > > > > > > > On Wed, 30 Mar 2016 14:10:38 +0200 > > > > > Pradeep Kiruvale wrote: > > > > > > > > > > > Hi All, > > > > > > > > > > > > Is virtio-9p-pci device only supports the fsdev deices? I am > trying > > > to > > > > > use > > > > > > -drive option for applying QoS for block device using > Virtio-9p-pci > > > > > device, > > > > > > but failing to create/add a device other than fsdev. Can you > please > > > help > > > > > me > > > > > > on this? > > > > > > > > > > > > Regards, > > > > > > Pradeep > > > > > > > > > > Hi Pradeep, > > > > > > > > > > Not sure to catch what you want to do but I confirm that > virti-9p-pci > > > only > > > > > supports > > > > > fsdev... if you want a block device, why don't you use > virtio-blk-pci ? > > > > > > > > > > Cheers. > > > > > > > > > > -- > > > > > Greg > > > > > > > > > > > > > > > > > >
[Qemu-devel] Virtio-9p and cgroup io-throttling
Hi All, I am using virtio-9p for sharing the file between host and guest. To test the shared file I do read/write options in the guest.To have controlled io, I am using cgroup blkio. While using cgroup I am facing two issues,Please find the issues below. 1. When I do IO throttling using the cgroup the read throttling works fine but the write throttling does not wok. It still bypasses these throttling control and does the default, am I missing something here? I use the following commands to create VM, share the files and to read/write from guest. *Create vm* qemu-system-x86_64 -balloon none ...-name vm0 -cpu host -m 128 -smp 1 -enable-kvm -parallel -fsdev local,id=sdb1,path=/mnt/sdb1,security_model=none,writeout=immediate -device virtio-9p-pci,fsdev=sdb1,mount_tag=sdb1 *Mount file* mount -t 9p -o trans=virtio,version=9p2000.L sdb1 /sdb1_ext4 2>>dd.log && sync touch /sdb1_ext4/dddrive *Write test* dd if=/dev/zero of=/sdb1_ext4/dddrive bs=4k count=80 oflag=direct >> dd.log 2>&1 && sync *Read test* dd if=/sdb1_ext4/dddrive of=/dev/null >> dd.log 2>&1 && sync 2. The other issue is when I run "dd" command inside guest it creates multiple threads to write/read. I can see those on host using iotop is this expected behavior? Regards, Pradeep
Re: [Qemu-devel] Virtio-9p and cgroup io-throttling
Hi Greg, Thanks for your reply. Below is the way how I add to blkio echo "8:16 8388608" > /sys/fs/cgroup/blkio/test/blkio.throttle.write_bps_device The problem I guess is adding these task ids to the "tasks" file in cgroup These threads are started randomly and even then I add the PIDs to the tasks file the cgroup still does not do IO control. Is it possible to reduce these number of threads? I see different number of threads doing IO at different runs. Regards, Pradeep On 8 April 2016 at 10:10, Greg Kurz wrote: > On Thu, 7 Apr 2016 11:48:27 +0200 > Pradeep Kiruvale wrote: > > > Hi All, > > > > I am using virtio-9p for sharing the file between host and guest. To test > > the shared file I do read/write options in the guest.To have controlled > io, > > I am using cgroup blkio. > > > > While using cgroup I am facing two issues,Please find the issues below. > > > > 1. When I do IO throttling using the cgroup the read throttling works > fine > > but the write throttling does not wok. It still bypasses these throttling > > control and does the default, am I missing something here? > > > > Hi, > > Can you provide details on your blkio setup ? > > > I use the following commands to create VM, share the files and to > > read/write from guest. > > > > *Create vm* > > qemu-system-x86_64 -balloon none ...-name vm0 -cpu host -m 128 -smp 1 > > -enable-kvm -parallel -fsdev > > local,id=sdb1,path=/mnt/sdb1,security_model=none,writeout=immediate > -device > > virtio-9p-pci,fsdev=sdb1,mount_tag=sdb1 > > > > *Mount file* > > mount -t 9p -o trans=virtio,version=9p2000.L sdb1 /sdb1_ext4 2>>dd.log && > > sync > > > > touch /sdb1_ext4/dddrive > > > > *Write test* > > dd if=/dev/zero of=/sdb1_ext4/dddrive bs=4k count=80 oflag=direct >> > > dd.log 2>&1 && sync > > > > *Read test* > > dd if=/sdb1_ext4/dddrive of=/dev/null >> dd.log 2>&1 && sync > > > > 2. The other issue is when I run "dd" command inside guest it creates > > multiple threads to write/read. I can see those on host using iotop is > this > > expected behavior? > > > > Yes. QEMU uses a thread pool to handle 9p requests. > > > Regards, > > Pradeep > > Cheers. > > -- > Greg > >
Re: [Qemu-devel] Virtio-9p and cgroup io-throttling
Hi Greg, FInd my replies inline > > > Below is the way how I add to blkio > > > > echo "8:16 8388608" > > > /sys/fs/cgroup/blkio/test/blkio.throttle.write_bps_device > > > > Ok, this just puts a limit of 8MB/s when writing to /dev/sdb for all > tasks in the test cgroup... but what about the tasks themselves ? > > > The problem I guess is adding these task ids to the "tasks" file in > cgroup > > > > Exactly. :) > > > These threads are started randomly and even then I add the PIDs to the > > tasks file the cgroup still does not do IO control. > > > > How did you get the PIDs ? Are you sure these threads you have added to the > cgroup are the ones that write to /dev/sdb ? > *Yes, I get PIDs from /proc/Qemu_PID/task* > > > Is it possible to reduce these number of threads? I see different number > of > > threads doing IO at different runs. > > > > AFAIK, no. > > Why don't you simply start QEMU in the cgroup ? Unless I miss something, > all > children threads, including the 9p ones, will be in the cgroup and honor > the > throttle setttings. > *I started the qemu with cgroup as below* *cgexec -g blkio:/test qemu...* *Is there any other way of starting the qemu in cgroup?* Regards, Pradeep > > > Regards, > > Pradeep > > > > Cheers. > > -- > Greg > > > > > On 8 April 2016 at 10:10, Greg Kurz wrote: > > > > > On Thu, 7 Apr 2016 11:48:27 +0200 > > > Pradeep Kiruvale wrote: > > > > > > > Hi All, > > > > > > > > I am using virtio-9p for sharing the file between host and guest. To > test > > > > the shared file I do read/write options in the guest.To have > controlled > > > io, > > > > I am using cgroup blkio. > > > > > > > > While using cgroup I am facing two issues,Please find the issues > below. > > > > > > > > 1. When I do IO throttling using the cgroup the read throttling works > > > fine > > > > but the write throttling does not wok. It still bypasses these > throttling > > > > control and does the default, am I missing something here? > > > > > > > > > > Hi, > > > > > > Can you provide details on your blkio setup ? > > > > > > > I use the following commands to create VM, share the files and to > > > > read/write from guest. > > > > > > > > *Create vm* > > > > qemu-system-x86_64 -balloon none ...-name vm0 -cpu host -m 128 > -smp 1 > > > > -enable-kvm -parallel -fsdev > > > > local,id=sdb1,path=/mnt/sdb1,security_model=none,writeout=immediate > > > -device > > > > virtio-9p-pci,fsdev=sdb1,mount_tag=sdb1 > > > > > > > > *Mount file* > > > > mount -t 9p -o trans=virtio,version=9p2000.L sdb1 /sdb1_ext4 > 2>>dd.log && > > > > sync > > > > > > > > touch /sdb1_ext4/dddrive > > > > > > > > *Write test* > > > > dd if=/dev/zero of=/sdb1_ext4/dddrive bs=4k count=80 > oflag=direct >> > > > > dd.log 2>&1 && sync > > > > > > > > *Read test* > > > > dd if=/sdb1_ext4/dddrive of=/dev/null >> dd.log 2>&1 && sync > > > > > > > > 2. The other issue is when I run "dd" command inside guest it > creates > > > > multiple threads to write/read. I can see those on host using iotop > is > > > this > > > > expected behavior? > > > > > > > > > > Yes. QEMU uses a thread pool to handle 9p requests. > > > > > > > Regards, > > > > Pradeep > > > > > > Cheers. > > > > > > -- > > > Greg > > > > > > > >
Re: [Qemu-devel] Virtio-9p and cgroup io-throttling
Hi Greg, Yes, it was nothing to do with the virtio-9p. I was writing at 4k Blocks now I changed it to 1K Block. This works fine for me. Thanks for your help. Regards, Pradeep On 8 April 2016 at 16:58, Greg Kurz wrote: > On Fri, 8 Apr 2016 14:55:29 +0200 > Pradeep Kiruvale wrote: > > > Hi Greg, > > > > FInd my replies inline > > > > > > > > > Below is the way how I add to blkio > > > > > > > > echo "8:16 8388608" > > > > > /sys/fs/cgroup/blkio/test/blkio.throttle.write_bps_device > > > > > > > > > > Ok, this just puts a limit of 8MB/s when writing to /dev/sdb for all > > > tasks in the test cgroup... but what about the tasks themselves ? > > > > > > > The problem I guess is adding these task ids to the "tasks" file in > > > cgroup > > > > > > > > > > Exactly. :) > > > > > > > These threads are started randomly and even then I add the PIDs to > the > > > > tasks file the cgroup still does not do IO control. > > > > > > > > > > How did you get the PIDs ? Are you sure these threads you have added > to the > > > cgroup are the ones that write to /dev/sdb ? > > > > > > > *Yes, I get PIDs from /proc/Qemu_PID/task* > > > > And then you echoed the PIDs to /sys/fs/cgroup/blkio/test/tasks ? > > This is racy... another IO thread may be started to do some work on > /dev/sdb > just after you've read PIDs from /proc/Qemu_PID/task, and it won't be part > of the cgroup. > > > > > > > > > > > > Is it possible to reduce these number of threads? I see different > number > > > of > > > > threads doing IO at different runs. > > > > > > > > > > AFAIK, no. > > > > > > Why don't you simply start QEMU in the cgroup ? Unless I miss > something, > > > all > > > children threads, including the 9p ones, will be in the cgroup and > honor > > > the > > > throttle setttings. > > > > > > > > > *I started the qemu with cgroup as below* > > > > *cgexec -g blkio:/test qemu...* > > *Is there any other way of starting the qemu in cgroup?* > > > > Maybe you can pass --sticky to cgexec to prevent cgred from moving > children tasks to other cgroups... > > There's also the old fashion method: > > # echo $$ > /sys/fs/cgroup/blkio/test/tasks > # qemu. > > This being said, QEMU is a regular userspace program that is completely > cgroup > agnostic. It won't behave differently than 'dd if=/dev/sdb of=/dev/null'. > > This really doesn't look like a QEMU related issue to me. > > > Regards, > > Pradeep > > > > Cheers. > > -- > Greg > > > > > > > > > > Regards, > > > > Pradeep > > > > > > > > > > Cheers. > > > > > > -- > > > Greg > > > > > > > > > > > On 8 April 2016 at 10:10, Greg Kurz > wrote: > > > > > > > > > On Thu, 7 Apr 2016 11:48:27 +0200 > > > > > Pradeep Kiruvale wrote: > > > > > > > > > > > Hi All, > > > > > > > > > > > > I am using virtio-9p for sharing the file between host and > guest. To > > > test > > > > > > the shared file I do read/write options in the guest.To have > > > controlled > > > > > io, > > > > > > I am using cgroup blkio. > > > > > > > > > > > > While using cgroup I am facing two issues,Please find the issues > > > below. > > > > > > > > > > > > 1. When I do IO throttling using the cgroup the read throttling > works > > > > > fine > > > > > > but the write throttling does not wok. It still bypasses these > > > throttling > > > > > > control and does the default, am I missing something here? > > > > > > > > > > > > > > > > Hi, > > > > > > > > > > Can you provide details on your blkio setup ? > > > > > > > > > > > I use the following commands to create VM, share the files and to > > > > > > read/write from guest. > > > > > > > > > > > > *Create vm* > > > > > > qemu-system-x86_64 -balloon none ...-name vm0 -cpu host -m > 128 > > > -smp 1 > > > > > > -enable-kvm -parallel -fsdev > > > > > > > local,id=sdb1,path=/mnt/sdb1,security_model=none,writeout=immediate > > > > > -device > > > > > > virtio-9p-pci,fsdev=sdb1,mount_tag=sdb1 > > > > > > > > > > > > *Mount file* > > > > > > mount -t 9p -o trans=virtio,version=9p2000.L sdb1 /sdb1_ext4 > > > 2>>dd.log && > > > > > > sync > > > > > > > > > > > > touch /sdb1_ext4/dddrive > > > > > > > > > > > > *Write test* > > > > > > dd if=/dev/zero of=/sdb1_ext4/dddrive bs=4k count=80 > > > oflag=direct >> > > > > > > dd.log 2>&1 && sync > > > > > > > > > > > > *Read test* > > > > > > dd if=/sdb1_ext4/dddrive of=/dev/null >> dd.log 2>&1 && sync > > > > > > > > > > > > 2. The other issue is when I run "dd" command inside guest it > > > creates > > > > > > multiple threads to write/read. I can see those on host using > iotop > > > is > > > > > this > > > > > > expected behavior? > > > > > > > > > > > > > > > > Yes. QEMU uses a thread pool to handle 9p requests. > > > > > > > > > > > Regards, > > > > > > Pradeep > > > > > > > > > > Cheers. > > > > > > > > > > -- > > > > > Greg > > > > > > > > > > > > > > > > > > >
[Qemu-devel] iolimits for virtio-9p
Hi All, We are planning to implement the io-limits for the virtio-9p driver i.e for fsdev devices. So, I am looking into the code base and how it has done for the block io devices. I would like to know how difficult is this and is there some one out there who has any plan to do this? Any suggestions/opinions are very much helpful. Regards, Pradeep
Re: [Qemu-devel] iolimits for virtio-9p
Hi Stefan, Thanks for the reply and adding Alberto. I am going through those APIs already. Thanks & Regards, Pradeep On 26 April 2016 at 11:21, Stefan Hajnoczi wrote: > On Tue, Apr 19, 2016 at 02:09:24PM +0200, Pradeep Kiruvale wrote: > > We are planning to implement the io-limits for the virtio-9p driver i.e > for > > fsdev devices. > > So, I am looking into the code base and how it has done for the block io > > devices. > > > > I would like to know how difficult is this and is there some one out > there > > who has any plan to do this? > > CCing Alberto Garcia, QEMU I/O throttling maintainer. > > Take a look at include/qemu/throttle.h. That is the common throttling > API. > > Stefan >
Re: [Qemu-devel] [Qemu-discuss] iolimits for virtio-9p
On 26 April 2016 at 14:08, Alberto Garcia wrote: > On Tue 19 Apr 2016 02:09:24 PM CEST, Pradeep Kiruvale wrote: > > > We are planning to implement the io-limits for the virtio-9p driver > > i.e for fsdev devices. > > So, I am looking into the code base and how it has done for the block > > io devices. > > > > I would like to know how difficult is this and is there some one out > > there who has any plan to do this? > > Hi, > > as Stefan said already, the common API is in throttle.h. > > It should be generic enough to be used in other parts of QEMU, but tell > me if you feel that it needs changes. > > Once you configure the throttling settings you essentially only need to > call throttle_schedule_timer() to see if a request needs to be > throttled, and afterwards throttle_account() to register the I/O that > has been peformed. > > You'll see that there's also throttle-group.c, but that's specific to > the block layer and not meant to be generic. > > > Hi Alberto, Thanks for the reply. I am still in the early phase, I will let you know if any changes are needed for the APIs. We might also have to implement throttle-group.c for 9p devices, if we want to apply throttle for group of devices. I just want some one from virtio-9p side to jump in to the discussion as well. That will help me a lot to understand from virtio-9p perspective. Regards, Pradeep
Re: [Qemu-devel] [Qemu-discuss] iolimits for virtio-9p
On 27 April 2016 at 10:38, Alberto Garcia wrote: > On Wed, Apr 27, 2016 at 09:29:02AM +0200, Pradeep Kiruvale wrote: > > > Thanks for the reply. I am still in the early phase, I will let you > > know if any changes are needed for the APIs. > > > > We might also have to implement throttle-group.c for 9p devices, if > > we want to apply throttle for group of devices. > > Fair enough, but again please note that: > > - throttle-group.c is not meant to be generic, but it's tied to > BlockDriverState / BlockBackend. > - it is currently being rewritten: > https://lists.gnu.org/archive/html/qemu-block/2016-04/msg00645.html > > If you can explain your use case with a bit more detail we can try to > see what can be done about it. > > We want to use virtio-9p for block io instead of virtio-blk-pci. But in case of virtio-9p we can just use fsdev devices, so we want to apply throttling (QoS) on these devices and as of now the io throttling only possible with the -drive option. As a work around we are doing the throttling using cgroup. It has its own costs. So, we want to have throttling for fsdev devices inside the qemu itself. I am just trying to understand and estimate time required for implementing it for the fsdevices. -Pradeep
Re: [Qemu-devel] [Qemu-discuss] iolimits for virtio-9p
On 27 April 2016 at 19:12, Greg Kurz wrote: > On Wed, 27 Apr 2016 16:39:58 +0200 > Pradeep Kiruvale wrote: > > > On 27 April 2016 at 10:38, Alberto Garcia wrote: > > > > > On Wed, Apr 27, 2016 at 09:29:02AM +0200, Pradeep Kiruvale wrote: > > > > > > > Thanks for the reply. I am still in the early phase, I will let you > > > > know if any changes are needed for the APIs. > > > > > > > > We might also have to implement throttle-group.c for 9p devices, if > > > > we want to apply throttle for group of devices. > > > > > > Fair enough, but again please note that: > > > > > > - throttle-group.c is not meant to be generic, but it's tied to > > > BlockDriverState / BlockBackend. > > > - it is currently being rewritten: > > > https://lists.gnu.org/archive/html/qemu-block/2016-04/msg00645.html > > > > > > If you can explain your use case with a bit more detail we can try to > > > see what can be done about it. > > > > > > > > We want to use virtio-9p for block io instead of virtio-blk-pci. But in > > case of > > 9p is mostly aimed at sharing files... why would you want to use it for > block io instead of a true block device ? And how would you do that ? > *Yes, we want to share the files itself. So we are using the virtio-9p.* *We want to have QoS on these files access for every VM.* > > > virtio-9p we can just use fsdev devices, so we want to apply throttling > > (QoS) > > on these devices and as of now the io throttling only possible with the > > -drive option. > > > > Indeed. > > > As a work around we are doing the throttling using cgroup. It has its own > > costs. > > Can you elaborate ? > *We saw that we need to create cgroups and set it and also we observed lot of iowaits * *compared to implementing the throttling inside the qemu.* *This we did observe by using the virtio-blk-pci devices. (Using cgroups Vs qemu throttling)* Thanks, Pradeep
Re: [Qemu-devel] [Qemu-discuss] iolimits for virtio-9p
On 2 May 2016 at 14:57, Greg Kurz wrote: > On Thu, 28 Apr 2016 11:45:41 +0200 > Pradeep Kiruvale wrote: > > > On 27 April 2016 at 19:12, Greg Kurz wrote: > > > > > On Wed, 27 Apr 2016 16:39:58 +0200 > > > Pradeep Kiruvale wrote: > > > > > > > On 27 April 2016 at 10:38, Alberto Garcia wrote: > > > > > > > > > On Wed, Apr 27, 2016 at 09:29:02AM +0200, Pradeep Kiruvale wrote: > > > > > > > > > > > Thanks for the reply. I am still in the early phase, I will let > you > > > > > > know if any changes are needed for the APIs. > > > > > > > > > > > > We might also have to implement throttle-group.c for 9p devices, > if > > > > > > we want to apply throttle for group of devices. > > > > > > > > > > Fair enough, but again please note that: > > > > > > > > > > - throttle-group.c is not meant to be generic, but it's tied to > > > > > BlockDriverState / BlockBackend. > > > > > - it is currently being rewritten: > > > > > > https://lists.gnu.org/archive/html/qemu-block/2016-04/msg00645.html > > > > > > > > > > If you can explain your use case with a bit more detail we can try > to > > > > > see what can be done about it. > > > > > > > > > > > > > > We want to use virtio-9p for block io instead of virtio-blk-pci. > But in > > > > case of > > > > > > 9p is mostly aimed at sharing files... why would you want to use it for > > > block io instead of a true block device ? And how would you do that ? > > > > > > > *Yes, we want to share the files itself. So we are using the virtio-9p.* > > You want to pass a disk image to the guest as a plain file on a 9p mount ? > And then, what do you do in the guest ? Attach it to a loop device ? > Yes, would like to mount as a 9p drive and create file inside that and read/write. This was the experiment we are doing, actual use case no idea. My work is to do a feasibility test does it work or not. > > > *We want to have QoS on these files access for every VM.* > > > > You won't be able to have QoS on selected files, but it may be possible to > introduce limits at the fsdev level: control all write accesses to all > files > and all read accesses to all files for a 9p device. > That is right, I do not want to have QoS for individual files but to whole fsdev device. > > > > > > > > > virtio-9p we can just use fsdev devices, so we want to apply > throttling > > > > (QoS) > > > > on these devices and as of now the io throttling only possible with > the > > > > -drive option. > > > > > > > > > > Indeed. > > > > > > > As a work around we are doing the throttling using cgroup. It has > its own > > > > costs. > > > > > > Can you elaborate ? > > > > > > > *We saw that we need to create cgroups and set it and also we observed > lot > > of iowaits * > > *compared to implementing the throttling inside the qemu.* > > *This we did observe by using the virtio-blk-pci devices. (Using cgroups > Vs > > qemu throttling)* > > > > > Just to be sure I get it right. > > You tried both: > 1) run QEMU with -device virtio-blk-pci and -drive throttling.* > 2) run QEMU with -device virtio-blk-pci in its own cgroup > > And 1) has better performance and is easier to use than 2) ? > > And what do you expect with 9p compared to 1) ? > > That was just to understand the cost of cpu io throttling inside the qemu vs using cgroup. The bench-marking we did to reproduce the numbers and understand the cost mentioned in http://www.linux-kvm.org/images/7/72/2011-forum-keep-a-limit-on-it-io-throttling-in-qemu.pdf Thanks, Pradeep > > > > Thanks, > > Pradeep > >
Re: [Qemu-devel] [Qemu-discuss] iolimits for virtio-9p
On 4 May 2016 at 17:40, Greg Kurz wrote: > On Mon, 2 May 2016 17:49:26 +0200 > Pradeep Kiruvale wrote: > > > On 2 May 2016 at 14:57, Greg Kurz wrote: > > > > > On Thu, 28 Apr 2016 11:45:41 +0200 > > > Pradeep Kiruvale wrote: > > > > > > > On 27 April 2016 at 19:12, Greg Kurz > wrote: > > > > > > > > > On Wed, 27 Apr 2016 16:39:58 +0200 > > > > > Pradeep Kiruvale wrote: > > > > > > > > > > > On 27 April 2016 at 10:38, Alberto Garcia > wrote: > > > > > > > > > > > > > On Wed, Apr 27, 2016 at 09:29:02AM +0200, Pradeep Kiruvale > wrote: > > > > > > > > > > > > > > > Thanks for the reply. I am still in the early phase, I will > let > > > you > > > > > > > > know if any changes are needed for the APIs. > > > > > > > > > > > > > > > > We might also have to implement throttle-group.c for 9p > devices, > > > if > > > > > > > > we want to apply throttle for group of devices. > > > > > > > > > > > > > > Fair enough, but again please note that: > > > > > > > > > > > > > > - throttle-group.c is not meant to be generic, but it's tied to > > > > > > > BlockDriverState / BlockBackend. > > > > > > > - it is currently being rewritten: > > > > > > > > > > https://lists.gnu.org/archive/html/qemu-block/2016-04/msg00645.html > > > > > > > > > > > > > > If you can explain your use case with a bit more detail we can > try > > > to > > > > > > > see what can be done about it. > > > > > > > > > > > > > > > > > > > > We want to use virtio-9p for block io instead of virtio-blk-pci. > > > But in > > > > > > case of > > > > > > > > > > 9p is mostly aimed at sharing files... why would you want to use > it for > > > > > block io instead of a true block device ? And how would you do > that ? > > > > > > > > > > > > > *Yes, we want to share the files itself. So we are using the > virtio-9p.* > > > > > > You want to pass a disk image to the guest as a plain file on a 9p > mount ? > > > And then, what do you do in the guest ? Attach it to a loop device ? > > > > > > > Yes, would like to mount as a 9p drive and create file inside that and > > read/write. > > This was the experiment we are doing, actual use case no idea. My work is > > to do > > a feasibility test does it work or not. > > > > > > > > > > > *We want to have QoS on these files access for every VM.* > > > > > > > > > > You won't be able to have QoS on selected files, but it may be > possible to > > > introduce limits at the fsdev level: control all write accesses to all > > > files > > > and all read accesses to all files for a 9p device. > > > > > > > That is right, I do not want to have QoS for individual files but to > whole > > fsdev device. > > > > > > > > > > > > > > > > > > > virtio-9p we can just use fsdev devices, so we want to apply > > > throttling > > > > > > (QoS) > > > > > > on these devices and as of now the io throttling only possible > with > > > the > > > > > > -drive option. > > > > > > > > > > > > > > > > Indeed. > > > > > > > > > > > As a work around we are doing the throttling using cgroup. It has > > > its own > > > > > > costs. > > > > > > > > > > Can you elaborate ? > > > > > > > > > > > > > *We saw that we need to create cgroups and set it and also we > observed > > > lot > > > > of iowaits * > > > > *compared to implementing the throttling inside the qemu.* > > > > *This we did observe by using the virtio-blk-pci devices. (Using > cgroups > > > Vs > > > > qemu throttling)* > > > > > > > > > > > > > > > > > Just to be sure I get it right. > > > > > > You tried both: > > > 1) run QEMU with -device virtio-blk-pci and -drive throttling.* > > > 2) run QEMU with -device virtio-blk-pci in its own cgroup > > > > > > And 1) has better performance and is easier to use than 2) ? > > > > > > And what do you expect with 9p compared to 1) ? > > > > > > > > That was just to understand the cost of cpu > > io throttling inside the qemu vs using cgroup. > > > > The bench-marking we did to reproduce the numbers and understand the cost > > mentioned in > > > > > http://www.linux-kvm.org/images/7/72/2011-forum-keep-a-limit-on-it-io-throttling-in-qemu.pdf > > > > Thanks, > > Pradeep > > > > Ok. So you did compare current QEMU block I/O throttling with cgroup ? And > you observed numbers > similar to the link above ? > *Yes, I did, I did run DD command in guest to do IO. The recent QEMU is in par with cgroups in terms * *of CPU utilization.* > > And now you would like to run the same test on a file in a 9p mount with > experimental 9p QoS ? > > *Yes, you are right.* > Maybe possible to reuse the throttle.h API and hack v9fs_write() and > v9fs_read() in 9p.c then. > > *OK, I am looking into it. Are there any sample test cases or something about how to apply the* *throttling APIs to a device?* Regards, Pradeep > Cheers. > > -- > Greg > > > > > > > > > > > Thanks, > > > > Pradeep > > > > > > > > >
Re: [Qemu-devel] [Qemu-discuss] iolimits for virtio-9p
On 6 May 2016 at 09:02, Greg Kurz wrote: > On Fri, 6 May 2016 08:01:09 +0200 > Pradeep Kiruvale wrote: > > > On 4 May 2016 at 17:40, Greg Kurz wrote: > > > > > On Mon, 2 May 2016 17:49:26 +0200 > > > Pradeep Kiruvale wrote: > > > > > > > On 2 May 2016 at 14:57, Greg Kurz wrote: > > > > > > > > > On Thu, 28 Apr 2016 11:45:41 +0200 > > > > > Pradeep Kiruvale wrote: > > > > > > > > > > > On 27 April 2016 at 19:12, Greg Kurz > > > wrote: > > > > > > > > > > > > > On Wed, 27 Apr 2016 16:39:58 +0200 > > > > > > > Pradeep Kiruvale wrote: > > > > > > > > > > > > > > > On 27 April 2016 at 10:38, Alberto Garcia > > > wrote: > > > > > > > > > > > > > > > > > On Wed, Apr 27, 2016 at 09:29:02AM +0200, Pradeep Kiruvale > > > wrote: > > > > > > > > > > > > > > > > > > > Thanks for the reply. I am still in the early phase, I > will > > > let > > > > > you > > > > > > > > > > know if any changes are needed for the APIs. > > > > > > > > > > > > > > > > > > > > We might also have to implement throttle-group.c for 9p > > > devices, > > > > > if > > > > > > > > > > we want to apply throttle for group of devices. > > > > > > > > > > > > > > > > > > Fair enough, but again please note that: > > > > > > > > > > > > > > > > > > - throttle-group.c is not meant to be generic, but it's > tied to > > > > > > > > > BlockDriverState / BlockBackend. > > > > > > > > > - it is currently being rewritten: > > > > > > > > > > > > > > > https://lists.gnu.org/archive/html/qemu-block/2016-04/msg00645.html > > > > > > > > > > > > > > > > > > If you can explain your use case with a bit more detail we > can > > > try > > > > > to > > > > > > > > > see what can be done about it. > > > > > > > > > > > > > > > > > > > > > > > > > > We want to use virtio-9p for block io instead of > virtio-blk-pci. > > > > > But in > > > > > > > > case of > > > > > > > > > > > > > > 9p is mostly aimed at sharing files... why would you want to > use > > > it for > > > > > > > block io instead of a true block device ? And how would you do > > > that ? > > > > > > > > > > > > > > > > > > > *Yes, we want to share the files itself. So we are using the > > > virtio-9p.* > > > > > > > > > > You want to pass a disk image to the guest as a plain file on a 9p > > > mount ? > > > > > And then, what do you do in the guest ? Attach it to a loop device > ? > > > > > > > > > > > > > Yes, would like to mount as a 9p drive and create file inside that > and > > > > read/write. > > > > This was the experiment we are doing, actual use case no idea. My > work is > > > > to do > > > > a feasibility test does it work or not. > > > > > > > > > > > > > > > > > > > *We want to have QoS on these files access for every VM.* > > > > > > > > > > > > > > > > You won't be able to have QoS on selected files, but it may be > > > possible to > > > > > introduce limits at the fsdev level: control all write accesses to > all > > > > > files > > > > > and all read accesses to all files for a 9p device. > > > > > > > > > > > > > That is right, I do not want to have QoS for individual files but to > > > whole > > > > fsdev device. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > virtio-9p we can just use fsdev devices, so we want to apply > > > > > throttling > > > > > > > > (QoS) > > > > > > > > on these devices and as of now the io throttling only
Re: [Qemu-devel] [Qemu-discuss] iolimits for virtio-9p
Thanks Alberto. Regards, Pradeep On 6 May 2016 at 14:16, Alberto Garcia wrote: > On Fri 06 May 2016 09:39:13 AM CEST, Pradeep Kiruvale wrote: > > >> The throttling API is currently only used by block devices, and the > >> only documentation out-there is the code itself... > > > > Thanks, I will have a look and get back to you if I have any further > > questions regarding this. > > As I said in an earlier e-mail: > > 1. the common API is in throttle.h. > > 2. Once you configure the throttling settings you essentially only need >to call throttle_schedule_timer() to see if a request needs to be >throttled, and afterwards throttle_account() to register the I/O that >has been peformed. > > Berto >
[Qemu-devel] Adding throttling to virtio-9p
Hi All, I am trying to add the throttle to the virtio-9p devices using the throttle APIs that are already exists in the qemu. I need help to understand the device model and where to add the throttling. I am digging through the code since a week or so but failed to understand how to tell the driver (virtio-9p) about the throttle enablement for this specific fs device. I am planning to enable and configure the throttle for that specific device at int qemu_fsdev_add(QemuOpts *opts) in qemu-fsdev.c file. After that I would like to just add throttle facility to just virtio-9p-local.c (i.e read/wrote calls). Though there are other drivers, I just want to add to this specific driver as of now. This is where I am missing the link like how to get to know the device configuration for this specific device throttle enabled or not and carry out the operations accordingly. Please help me to understand. Regards, Pradeep
Re: [Qemu-devel] Adding throttling to virtio-9p
On 7 June 2016 at 15:48, Greg Kurz wrote: > On Tue, 7 Jun 2016 10:20:51 +0200 > Pradeep Kiruvale wrote: > > Hi All, > > > > I am trying to add the throttle to the virtio-9p devices using the > throttle > > APIs that are already exists in the qemu. > > > > I need help to understand the device model and where to add the > throttling. > > I am digging through the code since a week or so but failed to understand > > how to tell the driver (virtio-9p) about the throttle enablement for > this > > specific fs device. > > > > I am planning to enable and configure the throttle for that specific > device > > at > > int qemu_fsdev_add(QemuOpts *opts) in qemu-fsdev.c file. > > > > After that I would like to just add throttle facility to just > > virtio-9p-local.c (i.e read/wrote calls). > > Though there are other drivers, I just want to add to this specific > driver > > as of now. > > > > This is where I am missing the link like how to get to know the device > > configuration for this specific device throttle enabled or not and carry > > out the operations accordingly. > > > > Please help me to understand. > > > > Drivers can register a parse_opts operation to handle specific command line > options. Since you want to make this feature specific to the local driver, > then you should handle the options in local_parse_opts(), not in > qemu_fsdev_add(). > > Thanks Greg, that was helpful. Regards, Pradeep > > Regards, > > Pradeep > > Cheers. > > -- > Greg >
Re: [Qemu-devel] [V7 1/1] fsdev: add IO throttle support to fsdev devices
On 10/24/2016 3:00 PM, Alberto Garcia wrote: On Sat 22 Oct 2016 05:07:22 PM CEST, Pradeep Jagadeesh wrote: 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. Ok, I think this is almost ready. There's only a few corrections and style fixes left. Hi Alberto, thanks for having a look at the patch. I will send the patch early next week as I am out of office. -Pradeep 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 (Optional) You can put these two in the same line: common-obj-y += qemu-fsdev-opts.o qemu-fsdev-throttle.o +++ b/fsdev/qemu-fsdev-throttle.c In this file there's many lines that are incorrectly indented. Indentation should be four spaces. The following functions have lines that need to be corrected: fsdev_throttle_check_for_wait fsdev_throttle_schedule_next_request fsdev_parse_io_limit_opts fsdev_throttle_configure_iolimits fsdev_co_throttle_request Check also the other changes in your patch in case there are more lines with wrong indentation. +static bool fsdev_throttle_check_for_wait(FsThrottle *fst, bool is_write) "Check for wait" doesn't sound like correct English to me. How about simply fsdev_throttle_schedule_timer()? Or fsdev_throttle_must_wait() +static void fsdev_throttle_schedule_next_request(FsThrottle *fst, bool is_write) +{ + if (!qemu_co_queue_empty(&fst->throttled_reqs[is_write])) { Here's an example of a line with the wrong indentation. It uses three spaces, it should use four. + qemu_co_queue_next(&fst->throttled_reqs[is_write]); And this one uses seven, it should use eight. There are more cases like this, I only highlighted a couple of them. +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_reportf_err(err, "Throttle configuration is not valid: "); +return -1; +} +if (throttle_enabled(&fst->cfg)) { +g_assert((fst->aioctx = qemu_get_aio_context())); +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]); + } + return 0; +} Here you're parsing the command-line options and initializing the throtting structures in local_parse_opts(). Then you're cleaning up in v9fs_device_unrealize_common(). I have two questions: a) You could split fsdev_throttle_configure_iolimits() in two: 1) fsdev_throttle_parse_opts(), called from local_parse_opts() 2) fsdev_throttle_init(), called from v9fs_device_realize_common(). b) Why are you only doing this for the "local" fsdriver, and not for "handle" or "proxy"? Isn't it possible to do throttling with those as well? +static int get_num_bytes(struct iovec *iov, int iovcnt) +{ +int i; +unsigned int bytes = 0; You changed 'bytes' from int to unsigned as I suggested, but you still return an int, and in fsdev_co_throttle_request() you also get an int. Please use unsigned in all these cases. +int fsdev_throttle_configure_iolimits(QemuOpts *, FsThrottle *); + +void coroutine_fn fsdev_co_throttle_request(FsThrottle *fst, bool is_write, +struct iovec *iov, int iovcnt); Please align this line with the parameters of the first line. Also, why do you include the name of the parameters in this function prototype but not in the other two? @@ -1240,6 +1240,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, &fse->fst) < 0) { +return -1; +} If you create fsdev_throttle_parse_opts() as I suggest, I'd rather print the error message here as the other error messages in this function. I think I haven't forgotten anything, everything else looks fine. I'd still like to have all the throtting parameters merged so we don't need to duplicate them, but that can be done in the future (I can even take care of it if I find a bit of time). Thanks for the patch! Berto
Re: [Qemu-devel] [V7 1/1] fsdev: add IO throttle support to fsdev devices
On 10/24/2016 11:28 PM, Greg Kurz wrote: Re-post (I had hit the send button by error :) On Sat, 22 Oct 2016 11:07:22 -0400 Pradeep Jagadeesh wrote: Signed-off-by: Pradeep Jagadeesh --- Hi Pradeep, I see that Berto already did a thorough review for this patch and I agree for all the suggestions he made. I have some more to add. First: this patch doesn't apply, please rebase. Hi Greg, Thanks for having a look at the patch. I will incorporate the comments and send the patch early next week as I am out of office till then. -Pradeep More remarks below. fsdev/Makefile.objs | 1 + fsdev/file-op-9p.h | 3 + fsdev/qemu-fsdev-opts.c | 76 +++ fsdev/qemu-fsdev-throttle.c | 147 fsdev/qemu-fsdev-throttle.h | 37 +++ hw/9pfs/9p-local.c | 9 ++- hw/9pfs/9p.c| 6 ++ hw/9pfs/cofile.c| 5 ++ 8 files changed, 282 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. The above lines are the changelog for the patch. We want this to be displayed when running 'git log'. For this to happen, please move these lines above your SoB tag. Only the vN -> vN+1 changes are not relevant (we don't need to record all the intermediate reviews in git) and should stay here. 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. v3 -> v4: -Addressed following comments by Alberto Garcia -Removed the unwanted locking and other data structures in qemu-fsdev-throttle.[ch] -Addressed following comments by Greg Kurz -Removed fsdev_iolimitsenable/disable functions, instead using throttle_enabled function v4 -> V5: -Fixed the issue with the larger block size accounting. (i.e, when the 9pfs mounted using msize=xxx option) V5 -> V6: -Addressed the comments by Alberto Garcia -Removed the fsdev_throttle_timer_cb() -Simplified the fsdev_throttle_schedule_next_request() as suggested V6 -> V7: -Addressed the comments by Alberto Garcia -changed the fsdev_throttle_schedule_next_request() as suggested 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_BITS0600 #define SM_LOCAL_DIR_MODE_BITS0700 @@ -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&q
Re: [Qemu-devel] [V7 1/1] fsdev: add IO throttle support to fsdev devices
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 (Optional) You can put these two in the same line: common-obj-y += qemu-fsdev-opts.o qemu-fsdev-throttle.o +++ b/fsdev/qemu-fsdev-throttle.c In this file there's many lines that are incorrectly indented. Indentation should be four spaces. The following functions have lines that need to be corrected: fsdev_throttle_check_for_wait fsdev_throttle_schedule_next_request fsdev_parse_io_limit_opts fsdev_throttle_configure_iolimits fsdev_co_throttle_request Check also the other changes in your patch in case there are more lines with wrong indentation. Fixed. +static bool fsdev_throttle_check_for_wait(FsThrottle *fst, bool is_write) "Check for wait" doesn't sound like correct English to me. How about simply fsdev_throttle_schedule_timer()? Or fsdev_throttle_must_wait() +static void fsdev_throttle_schedule_next_request(FsThrottle *fst, bool is_write) +{ + if (!qemu_co_queue_empty(&fst->throttled_reqs[is_write])) { Here's an example of a line with the wrong indentation. It uses three spaces, it should use four. + qemu_co_queue_next(&fst->throttled_reqs[is_write]); And this one uses seven, it should use eight. There are more cases like this, I only highlighted a couple of them. Done! +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_reportf_err(err, "Throttle configuration is not valid: "); +return -1; +} +if (throttle_enabled(&fst->cfg)) { +g_assert((fst->aioctx = qemu_get_aio_context())); +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]); + } + return 0; +} Here you're parsing the command-line options and initializing the throtting structures in local_parse_opts(). Then you're cleaning up in v9fs_device_unrealize_common(). I have two questions: a) You could split fsdev_throttle_configure_iolimits() in two: 1) fsdev_throttle_parse_opts(), called from local_parse_opts() 2) fsdev_throttle_init(), called from v9fs_device_realize_common(). Done as you suggested. Now parsing happens in local_parse_opts() and initialization in v9fs_device_realize_common() b) Why are you only doing this for the "local" fsdriver, and not for "handle" or "proxy"? Isn't it possible to do throttling with those as well? Yes, its possible. As of now I wanted to go with just with 9p-local because, we had a use case for only that. I feel with existing throttle apis (fsdev_throttle), it is easy enable for proxy and handle drivers also. +static int get_num_bytes(struct iovec *iov, int iovcnt) +{ +int i; +unsigned int bytes = 0; You changed 'bytes' from int to unsigned as I suggested, but you still return an int, and in fsdev_co_throttle_request() you also get an int. Please use unsigned in all these cases. done +int fsdev_throttle_configure_iolimits(QemuOpts *, FsThrottle *); + +void coroutine_fn fsdev_co_throttle_request(FsThrottle *fst, bool is_write, +struct iovec *iov, int iovcnt); Please align this line with the parameters of the first line. Also, why do you include the name of the parameters in this function prototype but not in the other two? I removed them. @@ -1240,6 +1240,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, &fse->fst) < 0) { +return -1; +} If you create fsdev_throttle_parse_opts() as I suggest, I'd rather print the error message here as the other error messages in this function. I re-wrote as suggested by you. I think I haven't forgotten anything, everything else looks fine. I'd still like to have all the throtting parameters merged so we don't need to duplicate them, but that can be done in the future (I can even take care of it if I find a bit of time). Thanks for the patch! -Pradeep Berto
Re: [Qemu-devel] [V7 1/1] fsdev: add IO throttle support to fsdev devices
cond", +},{ +.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 000..d48d031 --- /dev/null +++ b/fsdev/qemu-fsdev-throttle.c @@ -0,0 +1,147 @@ +/* + * 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) +{ + return throttle_schedule_timer(&fst->ts, &fst->tt, is_write); +} + +static void fsdev_throttle_schedule_next_request(FsThrottle *fst, bool is_write) +{ + if (!qemu_co_queue_empty(&fst->throttled_reqs[is_write])) { +if (fsdev_throttle_check_for_wait(fst, is_write)) { +return; +} + qemu_co_queue_next(&fst->throttled_reqs[is_write]); This calls for a coroutine_fn tag but... + } +} + ... as Berto already suggested in v6, these functions aren't needed and your "I just would like to keep these functions" answer isn't convincing enough. Personally, I'd greatly really prefer the code to be inlined in the callers, so that we can see the whole throttling/queue logic in one place. In lined the code fsdev_throttle_check_for_wait(). +static void fsdev_throttle_read_timer_cb(void *opaque) +{ +FsThrottle *fst = opaque; +qemu_co_enter_next(&fst->throttled_reqs[false]); +} + +static void fsdev_throttle_write_timer_cb(void *opaque) +{ +FsThrottle *fst = opaque; +qemu_co_enter_next(&fst->throttled_reqs[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
Re: [Qemu-devel] [V7 1/1] fsdev: add IO throttle support to fsdev devices
On 10/30/2016 2:35 PM, Greg Kurz wrote: On Sun, 30 Oct 2016 14:04:49 +0100 Pradeep Jagadeesh wrote: More remarks below. fsdev/Makefile.objs | 1 + fsdev/file-op-9p.h | 3 + fsdev/qemu-fsdev-opts.c | 76 +++ fsdev/qemu-fsdev-throttle.c | 147 fsdev/qemu-fsdev-throttle.h | 37 +++ hw/9pfs/9p-local.c | 9 ++- hw/9pfs/9p.c| 6 ++ hw/9pfs/cofile.c| 5 ++ 8 files changed, 282 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. The above lines are the changelog for the patch. We want this to be displayed when running 'git log'. For this to happen, please move these lines above your SoB tag. OK Only the vN -> vN+1 changes are not relevant (we don't need to record all the intermediate reviews in git) and should stay here. I just put there just keep track of what comments I fixed from which reviewer in recent patches. This we can take off when we have the final patch right? Also I did not understand your comment about vN -> vN+1. Heh sorry for not being clear :) I was saying only the v1->v2, v2->v3 (i.e. vN -> vN+1) parts must stay below the --- so that git am doesn't copy them to the commit log. So, you're good with this part :) OK, thanks. 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. v3 -> v4: -Addressed following comments by Alberto Garcia -Removed the unwanted locking and other data structures in qemu-fsdev-throttle.[ch] -Addressed following comments by Greg Kurz -Removed fsdev_iolimitsenable/disable functions, instead using throttle_enabled function v4 -> V5: -Fixed the issue with the larger block size accounting. (i.e, when the 9pfs mounted using msize=xxx option) V5 -> V6: -Addressed the comments by Alberto Garcia -Removed the fsdev_throttle_timer_cb() -Simplified the fsdev_throttle_schedule_next_request() as suggested V6 -> V7: -Addressed the comments by Alberto Garcia -changed the fsdev_throttle_schedule_next_request() as suggested 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_BITS0600 #define SM_LOCAL_DIR_MODE_BITS0700 @@ -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 operat
[Qemu-devel] (no subject)
Date: Sun, 30 Oct 2016 10:53:16 -0400 Subject: [PATCH V8] fsdev: add IO throttle support to fsdev devices Uses throttling APIs to limit I/O bandwidth and number of operations on the devices which use 9p-local driver. 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. Signed-off-by: Pradeep Jagadeesh --- fsdev/Makefile.objs | 2 +- fsdev/file-op-9p.h | 3 + fsdev/qemu-fsdev-opts.c | 76 fsdev/qemu-fsdev-throttle.c | 139 fsdev/qemu-fsdev-throttle.h | 39 + hw/9pfs/9p-local.c | 2 + hw/9pfs/9p.c| 10 hw/9pfs/cofile.c| 2 + 8 files changed, 272 insertions(+), 1 deletion(-) create mode 100644 fsdev/qemu-fsdev-throttle.c create mode 100644 fsdev/qemu-fsdev-throttle.h 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. v3 -> v4: -Addressed following comments by Alberto Garcia -Removed the unwanted locking and other data structures in qemu-fsdev-throttle.[ch] -Addressed following comments by Greg Kurz -Removed fsdev_iolimitsenable/disable functions, instead using throttle_enabled function v4 -> V5: -Fixed the issue with the larger block size accounting. (i.e, when the 9pfs mounted using msize=xxx option) V5 -> V6: -Addressed the comments by Alberto Garcia -Removed the fsdev_throttle_timer_cb() -Simplified the fsdev_throttle_schedule_next_request() as suggested V6 -> V7: -Addressed the comments by Alberto Garcia -changed the fsdev_throttle_schedule_next_request() as suggested v7 -> v8: -Addressed comments by Alberto Garcia -Fixed some indentation issues and split the configure_io_limit function -Inlined throttle_timer_check code diff --git a/fsdev/Makefile.objs b/fsdev/Makefile.objs index 1b120a4..659df6e 100644 --- a/fsdev/Makefile.objs +++ b/fsdev/Makefile.objs @@ -5,7 +5,7 @@ common-obj-y = qemu-fsdev.o 9p-marshal.o 9p-iov-marshal.o else common-obj-y = qemu-fsdev-dummy.o endif -common-obj-y += qemu-fsdev-opts.o +common-obj-y += qemu-fsdev-opts.o qemu-fsdev-throttle.o # Toplevel always builds this; targets without virtio will put it in # common-obj-y 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_BITS0600 #define SM_LOCAL_DIR_MODE_BITS0700 @@ -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", +.
[Qemu-devel] Throttle in virtio-net
Hi All, I am planning to implement throttling functionality for virtio-net driver using the throttling APIs that exist inside qemu. Please let me know if someone already working on the same. Asking because I do not want to just replicate the work. Thanks & Regards, Pradeep
Re: [Qemu-devel] [Qemu-discuss] Throttle in virtio-net
Hi Alberto, Thanks for your reply. > > > I am planning to implement throttling functionality for virtio-net > > driver using the throttling APIs that exist inside qemu. > > Hi Pradeep, > > the problem with implementing throttling for the network is that > it's useless if you use the vhost_net kernel accelerator, because it > bypasses QEMU entirely: > >https://access.redhat.com/documentation/en-US/Red_Hat_ > Enterprise_Linux/7/html/Virtualization_Tuning_and_Optimization_Guide/sect- > Virtualization_Tuning_Optimization_Guide-Networking- > Virtio_and_vhostnet.html Thanks for this very valuable information. What if someone wants to just use virtio-net without the vhost acceleration? Any idea how it will be done in this case? It still uses cgroup or some other mechanism to do throttling? > > libvirt implements this using tc: > >https://libvirt.org/formatdomain.html#elementDomain >http://luxik.cdi.cz/~devik/qos/htb/manual/userg.htm#ceiling Here it uses the cgroup inside the libvirt dirver to control the packet rate right? Thanks, Pradeep
[Qemu-devel] [PATCH v2] 9pfs: add support for IO limits to 9p-local driver
Uses throttling APIs to limit I/O bandwidth and number of operations on the devices which use 9p-local driver. Signed-off-by: Pradeep Jagadeesh --- fsdev/file-op-9p.h | 3 + fsdev/qemu-fsdev-opts.c | 52 + hw/9pfs/9p-local.c | 18 - hw/9pfs/9p-throttle.c | 201 hw/9pfs/9p-throttle.h | 46 +++ hw/9pfs/9p.c| 7 ++ hw/9pfs/Makefile.objs | 1 + 7 files changed, 326 insertions(+), 2 deletions(-) create mode 100644 hw/9pfs/9p-throttle.c create mode 100644 hw/9pfs/9p-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 diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h index 6db9fea..e86b91a 100644 --- a/fsdev/file-op-9p.h +++ b/fsdev/file-op-9p.h @@ -17,6 +17,7 @@ #include #include #include +#include "hw/9pfs/9p-throttle.h" #define SM_LOCAL_MODE_BITS0600 #define SM_LOCAL_DIR_MODE_BITS0700 @@ -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..2774855 100644 --- a/fsdev/qemu-fsdev-opts.c +++ b/fsdev/qemu-fsdev-opts.c @@ -37,6 +37,58 @@ static QemuOptsList qemu_fsdev_opts = { }, { .name = "sock_fd", .type = QEMU_OPT_NUMBER, +}, { +.name = "bps", +.type = QEMU_OPT_NUMBER, +.help = "limit total bytes per second", +}, { +.name = "bps_rd", +.type = QEMU_OPT_NUMBER, +.help = "limit read bytes per second", +}, { +.name = "bps_wr", +.type = QEMU_OPT_NUMBER, +.help = "limit write bytes per second", +}, { +.name = "iops", +.type = QEMU_OPT_NUMBER, +.help = "limit total io operations per second", +}, { +.name = "iops_rd", +.type = QEMU_OPT_NUMBER, +.help = "limit read operations per second ", +}, { +.name = "iops_wr", +.type = QEMU_OPT_NUMBER, +.help = "limit write operations per second", +}, { +.name = "bps_max", +.type = QEMU_OPT_NUMBER, +.help = "maximum bytes burst", +}, { +.name = "bps_rd_max", +.type = QEMU_OPT_NUMBER, +.help = "maximum bytes read burst", +}, { +.name = "bps_wr_max", +.type = QEMU_OPT_NUMBER, +.help = "maximum bytes write burst", +}, { +.name = "iops_max", +.type = QEMU_OPT_NUMBER, +.help = "maximum io operations burst", +}, { +.name = "iops_rd_max", +.type = QEMU_OPT_NUMBER, +.help = "maximum io operations read burst", +}, { +.name = "iops_wr_max", +.type = QEMU_OPT_NUMBER, +.help = "maximum io operations write burst", +}, { +.name = "iops_size", +.type = QEMU_OPT_NUMBER, +.help = "io iops-size", }, { /*End of list */ } diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c index 3f271fc..49c2819 100644 --- a/hw/9pfs/9p-local.c +++ b/hw/9pfs/9p-local.c @@ -420,6 +420,8 @@ static ssize_t local_preadv(FsContext *ctx, V9fsFidOpenState *fs, const struct iovec *iov, int iovcnt, off_t offset) { +throttle9p_request(ctx->fst, false, iov->iov_len); + #ifdef CONFIG_PREADV return preadv(fs->fd, iov, iovcnt, offset); #else @@ -436,8 +438,10 @@ static ssize_t local_pwritev(FsContext *ctx, V9fsFidOpenState *fs, const struct iovec *iov, int iovcnt, off_t offset) { -ssize_t ret -; +ssize_t ret; + +throttle9p_request(ctx->fst, true, iov->iov_len); + #ifdef CONFIG_PREADV ret = pwritev(fs->fd, iov, iovcnt, offset); #else @@ -121
Re: [Qemu-devel] [RFC PATCH] throttle: move throttling cmdline options to a separate header file
On 22 November 2016 at 14:55, Greg Kurz wrote: > On Tue, 22 Nov 2016 14:51:12 +0100 > Alberto Garcia wrote: > > > On Tue 22 Nov 2016 01:49:51 PM CET, Greg Kurz wrote: > > > > > This will allow other subsystems (i.e. fsdev) to implement throttling > > > without duplicating the command line options. > > > > > > Signed-off-by: Greg Kurz > > > > Did you check if it's possible / easy to do defining a normal array > > instead of a macro and using qemu_opts_append() or something similar? > > > > Berto > > No I didn't due to lack of time but maybe Pradeep may have a look in this > direction ? > Only may be next year :) -Pradeep > > Cheers. > > -- > Greg >
Re: [Qemu-devel] [RFC PATCH] throttle: move throttling cmdline options to a separate header file
On 23 November 2016 at 13:13, Greg Kurz wrote: > On Tue, 22 Nov 2016 16:02:12 +0100 > Pradeep Kiruvale wrote: > > > On 22 November 2016 at 14:55, Greg Kurz wrote: > > > > > On Tue, 22 Nov 2016 14:51:12 +0100 > > > Alberto Garcia wrote: > > > > > > > On Tue 22 Nov 2016 01:49:51 PM CET, Greg Kurz wrote: > > > > > > > > > This will allow other subsystems (i.e. fsdev) to implement > throttling > > > > > without duplicating the command line options. > > > > > > > > > > Signed-off-by: Greg Kurz > > > > > > > > Did you check if it's possible / easy to do defining a normal array > > > > instead of a macro and using qemu_opts_append() or something similar? > > > > > > > > Berto > > > > > > No I didn't due to lack of time but maybe Pradeep may have a look in > this > > > direction ? > > > > > > > Only may be next year :) > > > > Ok, and also, it will be the occasion to add the throttling options to > the legacy -virtfs command line option as well. > Ok, also I think I have to add QMP interfaces for 9p case. -Pradeep > Cheers. > > -- > Greg > > > -Pradeep > > > > > > > > > > Cheers. > > > > > > -- > > > Greg > > > > >
Re: [Qemu-devel] [PATCH v2] 9pfs: add support for IO limits to 9p-local driver
Hi Greg, Thanks for looking into the patch. Please look at the replies inline. Regards, Pradeep -Original Message- From: Greg Kurz [mailto:gr...@kaod.org] Sent: Friday, September 09, 2016 5:29 PM To: Pradeep Jagadeesh Cc: Aneesh Kumar K.V; Pradeep Jagadeesh; Alberto Garcia; qemu-devel@nongnu.org; Claudio Fontana; Eric Blake Subject: Re: [PATCH v2] 9pfs: add support for IO limits to 9p-local driver On Fri, 9 Sep 2016 05:10:27 -0400 Pradeep Jagadeesh wrote: > Uses throttling APIs to limit I/O bandwidth and number of operations > on the devices which use 9p-local driver. > > Signed-off-by: Pradeep Jagadeesh > --- Hi Pradeep, Please find some remarks below. I haven't dived deep enough to see if this actually works. Maybe Berto can provide some feedback ? Cheers. -- Greg > fsdev/file-op-9p.h | 3 + > fsdev/qemu-fsdev-opts.c | 52 + > hw/9pfs/9p-local.c | 18 - > hw/9pfs/9p-throttle.c | 201 > > hw/9pfs/9p-throttle.h | 46 +++ > hw/9pfs/9p.c| 7 ++ > hw/9pfs/Makefile.objs | 1 + > 7 files changed, 326 insertions(+), 2 deletions(-) create mode > 100644 hw/9pfs/9p-throttle.c create mode 100644 hw/9pfs/9p-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 > > diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h index > 6db9fea..e86b91a 100644 > --- a/fsdev/file-op-9p.h > +++ b/fsdev/file-op-9p.h > @@ -17,6 +17,7 @@ > #include > #include > #include > +#include "hw/9pfs/9p-throttle.h" > > #define SM_LOCAL_MODE_BITS0600 > #define SM_LOCAL_DIR_MODE_BITS0700 > @@ -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..2774855 100644 > --- a/fsdev/qemu-fsdev-opts.c > +++ b/fsdev/qemu-fsdev-opts.c > @@ -37,6 +37,58 @@ static QemuOptsList qemu_fsdev_opts = { > }, { > .name = "sock_fd", > .type = QEMU_OPT_NUMBER, > +}, { > +.name = "", > +.type = QEMU_OPT_NUMBER, > +.help = "limit total bytes per second", > +}, { > +.name = "bps_rd", > +.type = QEMU_OPT_NUMBER, > +.help = "limit read bytes per second", > +}, { > +.name = "bps_wr", > +.type = QEMU_OPT_NUMBER, > +.help = "limit write bytes per second", > +}, { > +.name = "iops", > +.type = QEMU_OPT_NUMBER, > +.help = "limit total io operations per second", > +}, { > +.name = "iops_rd", > +.type = QEMU_OPT_NUMBER, > +.help = "limit read operations per second ", > +}, { > +.name = "iops_wr", > +.type = QEMU_OPT_NUMBER, > +.help = "limit write operations per second", > +}, { > +.name = "bps_max", > +.type = QEMU_OPT_NUMBER, > +.help = "maximum bytes burst", > +}, { > +.name = "bps_rd_max", > +.type = QEMU_OPT_NUMBER, > +.help = "maximum bytes read burst", > +}, { > +.name = "bps_wr_max", > +.type = QEMU_OPT_NUMBER, > +.help = "maximum bytes write burst", > +}, { > +.name = "iops_max", > +.type = QEMU_OPT_NUMBER, > +.help = "maximum io operations burst", > +}, { > +.name = "iops_rd_max", > +.type = QEMU_OPT_NUMBER, > + .help = "maximum io operations read burst", > +}, { > +
Re: [Qemu-devel] [PATCH v2] 9pfs: add support for IO limits to 9p-local driver
Replies inline Greg. Thanks & Regards, Pradeep -Original Message- From: Greg Kurz [mailto:gr...@kaod.org] Sent: Monday, September 12, 2016 4:19 PM To: Pradeep Jagadeesh Cc: Pradeep Jagadeesh; Aneesh Kumar K.V; Alberto Garcia; qemu-devel@nongnu.org; Claudio Fontana; Eric Blake Subject: Re: [PATCH v2] 9pfs: add support for IO limits to 9p-local driver On Mon, 12 Sep 2016 12:52:55 +0000 Pradeep Jagadeesh wrote: > Hi Greg, > > Thanks for looking into the patch. Please look at the replies inline. > > Regards, > Pradeep > Hi Pradeep, Remarks and answers below. Cheers. -- Greg > -Original Message- > From: Greg Kurz [mailto:gr...@kaod.org] > Sent: Friday, September 09, 2016 5:29 PM > To: Pradeep Jagadeesh > Cc: Aneesh Kumar K.V; Pradeep Jagadeesh; Alberto Garcia; > qemu-devel@nongnu.org; Claudio Fontana; Eric Blake > Subject: Re: [PATCH v2] 9pfs: add support for IO limits to 9p-local > driver > > On Fri, 9 Sep 2016 05:10:27 -0400 > Pradeep Jagadeesh wrote: > > > Uses throttling APIs to limit I/O bandwidth and number of operations > > on the devices which use 9p-local driver. > > > > Signed-off-by: Pradeep Jagadeesh > > --- > > Hi Pradeep, > > Please find some remarks below. I haven't dived deep enough to see if this > actually works. Maybe Berto can provide some feedback ? > > Cheers. > > -- > Greg > > > fsdev/file-op-9p.h | 3 + > > fsdev/qemu-fsdev-opts.c | 52 + > > hw/9pfs/9p-local.c | 18 - > > hw/9pfs/9p-throttle.c | 201 > > > > hw/9pfs/9p-throttle.h | 46 +++ I forgot to mention it, but this throttling implementation isn't related to the 9P device but to the fsdev device: 9p-throttle.[ch] should move to the fsdev directory and be renamed to fsdev-throttle.[ch]. [Pradeep Jagadeesh] Absolutely right and make sense, I will move the throttle code to fsdev directory. > > hw/9pfs/9p.c| 7 ++ > > hw/9pfs/Makefile.objs | 1 + > > 7 files changed, 326 insertions(+), 2 deletions(-) create mode > > 100644 hw/9pfs/9p-throttle.c create mode 100644 > > hw/9pfs/9p-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 > > > > diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h index > > 6db9fea..e86b91a 100644 > > --- a/fsdev/file-op-9p.h > > +++ b/fsdev/file-op-9p.h > > @@ -17,6 +17,7 @@ > > #include > > #include > > #include > > +#include "hw/9pfs/9p-throttle.h" > > > > #define SM_LOCAL_MODE_BITS0600 > > #define SM_LOCAL_DIR_MODE_BITS0700 > > @@ -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..2774855 100644 > > --- a/fsdev/qemu-fsdev-opts.c > > +++ b/fsdev/qemu-fsdev-opts.c > > @@ -37,6 +37,58 @@ static QemuOptsList qemu_fsdev_opts = { > > }, { > > .name = "sock_fd", > > .type = QEMU_OPT_NUMBER, > > +}, { > > +.name = "", > > +.type = QEMU_OPT_NUMBER, > > +.help = "limit total bytes per second", > > +}, { > > +.name = "bps_rd", > > +.type = QEMU_OPT_NUMBER, > > +.help = "limit read bytes per second", > > +}, { > > +.name = "bps_wr", > > +.type = QEMU_OPT_NUMBER, > > +.help = "limit write bytes per second", > > +}, { > > +.name = "iops", > > +.type = QEMU_OPT_NUMBER, >
Re: [Qemu-devel] [PATCH v2] 9pfs: add support for IO limits to 9p-local driver
Hi Greg, Replies inline Cheers, Pradeep -Original Message- From: Greg Kurz [mailto:gr...@kaod.org] Sent: Tuesday, September 13, 2016 10:52 AM To: Pradeep Jagadeesh Cc: Pradeep Jagadeesh; Aneesh Kumar K.V; Alberto Garcia; qemu-devel@nongnu.org; Claudio Fontana; Eric Blake Subject: Re: [PATCH v2] 9pfs: add support for IO limits to 9p-local driver On Mon, 12 Sep 2016 16:08:43 + Pradeep Jagadeesh wrote: > Replies inline Greg. > > Thanks & Regards, > Pradeep > Hi Pradeep, > -Original Message- > From: Greg Kurz [mailto:gr...@kaod.org] > Sent: Monday, September 12, 2016 4:19 PM > To: Pradeep Jagadeesh > Cc: Pradeep Jagadeesh; Aneesh Kumar K.V; Alberto Garcia; > qemu-devel@nongnu.org; Claudio Fontana; Eric Blake > Subject: Re: [PATCH v2] 9pfs: add support for IO limits to 9p-local > driver > > On Mon, 12 Sep 2016 12:52:55 + > Pradeep Jagadeesh wrote: > > > Hi Greg, > > > > Thanks for looking into the patch. Please look at the replies inline. > > > > Regards, > > Pradeep > > > > Hi Pradeep, > > Remarks and answers below. > > Cheers. > > -- > Greg > > > -Original Message- > > From: Greg Kurz [mailto:gr...@kaod.org] > > Sent: Friday, September 09, 2016 5:29 PM > > To: Pradeep Jagadeesh > > Cc: Aneesh Kumar K.V; Pradeep Jagadeesh; Alberto Garcia; > > qemu-devel@nongnu.org; Claudio Fontana; Eric Blake > > Subject: Re: [PATCH v2] 9pfs: add support for IO limits to 9p-local > > driver > > > > On Fri, 9 Sep 2016 05:10:27 -0400 > > Pradeep Jagadeesh wrote: > > > > > Uses throttling APIs to limit I/O bandwidth and number of > > > operations on the devices which use 9p-local driver. > > > > > > Signed-off-by: Pradeep Jagadeesh > > > --- > > > > Hi Pradeep, > > > > Please find some remarks below. I haven't dived deep enough to see if this > > actually works. Maybe Berto can provide some feedback ? > > > > Cheers. > > > > -- > > Greg > > > > > fsdev/file-op-9p.h | 3 + > > > fsdev/qemu-fsdev-opts.c | 52 + > > > hw/9pfs/9p-local.c | 18 - > > > hw/9pfs/9p-throttle.c | 201 > > > > > > hw/9pfs/9p-throttle.h | 46 +++ > > I forgot to mention it, but this throttling implementation isn't related to > the 9P device but to the fsdev device: 9p-throttle.[ch] should move to the > fsdev directory and be renamed to fsdev-throttle.[ch]. > > [Pradeep Jagadeesh] Absolutely right and make sense, I will move the throttle > code to fsdev directory. > > > > hw/9pfs/9p.c| 7 ++ > > > hw/9pfs/Makefile.objs | 1 + > > > 7 files changed, 326 insertions(+), 2 deletions(-) create mode > > > 100644 hw/9pfs/9p-throttle.c create mode 100644 > > > hw/9pfs/9p-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 > > > > > > diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h index > > > 6db9fea..e86b91a 100644 > > > --- a/fsdev/file-op-9p.h > > > +++ b/fsdev/file-op-9p.h > > > @@ -17,6 +17,7 @@ > > > #include > > > #include > > > #include > > > +#include "hw/9pfs/9p-throttle.h" > > > > > > #define SM_LOCAL_MODE_BITS0600 > > > #define SM_LOCAL_DIR_MODE_BITS0700 > > > @@ -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; >
Re: [Qemu-devel] [PATCH v2] 9pfs: add support for IO limits to 9p-local driver
-Original Message- From: Greg Kurz [mailto:gr...@kaod.org] Sent: Tuesday, September 13, 2016 2:30 PM To: Pradeep Jagadeesh Cc: Pradeep Jagadeesh; Aneesh Kumar K.V; Alberto Garcia; qemu-devel@nongnu.org; Claudio Fontana; Eric Blake Subject: Re: [PATCH v2] 9pfs: add support for IO limits to 9p-local driver On Tue, 13 Sep 2016 09:17:49 + Pradeep Jagadeesh wrote: > Hi Greg, > > Replies inline > > Cheers, > Pradeep > -Original Message- > From: Greg Kurz [mailto:gr...@kaod.org] > Sent: Tuesday, September 13, 2016 10:52 AM > To: Pradeep Jagadeesh > Cc: Pradeep Jagadeesh; Aneesh Kumar K.V; Alberto Garcia; > qemu-devel@nongnu.org; Claudio Fontana; Eric Blake > Subject: Re: [PATCH v2] 9pfs: add support for IO limits to 9p-local > driver > > On Mon, 12 Sep 2016 16:08:43 + > Pradeep Jagadeesh wrote: > > > Replies inline Greg. > > > > Thanks & Regards, > > Pradeep > > > > Hi Pradeep, > > > -Original Message- > > From: Greg Kurz [mailto:gr...@kaod.org] > > Sent: Monday, September 12, 2016 4:19 PM > > To: Pradeep Jagadeesh > > Cc: Pradeep Jagadeesh; Aneesh Kumar K.V; Alberto Garcia; > > qemu-devel@nongnu.org; Claudio Fontana; Eric Blake > > Subject: Re: [PATCH v2] 9pfs: add support for IO limits to 9p-local > > driver > > > > On Mon, 12 Sep 2016 12:52:55 + > > Pradeep Jagadeesh wrote: > > > > > Hi Greg, > > > > > > Thanks for looking into the patch. Please look at the replies inline. > > > > > > Regards, > > > Pradeep > > > > > > > Hi Pradeep, > > > > Remarks and answers below. > > > > Cheers. > > > > -- > > Greg > > > > > -Original Message- > > > From: Greg Kurz [mailto:gr...@kaod.org] > > > Sent: Friday, September 09, 2016 5:29 PM > > > To: Pradeep Jagadeesh > > > Cc: Aneesh Kumar K.V; Pradeep Jagadeesh; Alberto Garcia; > > > qemu-devel@nongnu.org; Claudio Fontana; Eric Blake > > > Subject: Re: [PATCH v2] 9pfs: add support for IO limits to > > > 9p-local driver > > > > > > On Fri, 9 Sep 2016 05:10:27 -0400 Pradeep Jagadeesh > > > wrote: > > > > > > > Uses throttling APIs to limit I/O bandwidth and number of > > > > operations on the devices which use 9p-local driver. > > > > > > > > Signed-off-by: Pradeep Jagadeesh > > > > --- > > > > > > Hi Pradeep, > > > > > > Please find some remarks below. I haven't dived deep enough to see if > > > this actually works. Maybe Berto can provide some feedback ? > > > > > > Cheers. > > > > > > -- > > > Greg > > > > > > > fsdev/file-op-9p.h | 3 + > > > > fsdev/qemu-fsdev-opts.c | 52 + > > > > hw/9pfs/9p-local.c | 18 - > > > > hw/9pfs/9p-throttle.c | 201 > > > > > > > > hw/9pfs/9p-throttle.h | 46 +++ > > > > I forgot to mention it, but this throttling implementation isn't related to > > the 9P device but to the fsdev device: 9p-throttle.[ch] should move to the > > fsdev directory and be renamed to fsdev-throttle.[ch]. > > > > [Pradeep Jagadeesh] Absolutely right and make sense, I will move the > > throttle code to fsdev directory. > > > > > > hw/9pfs/9p.c| 7 ++ > > > > hw/9pfs/Makefile.objs | 1 + > > > > 7 files changed, 326 insertions(+), 2 deletions(-) create mode > > > > 100644 hw/9pfs/9p-throttle.c create mode 100644 > > > > hw/9pfs/9p-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 > > > > > > > > diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h index > > > > 6db9fea..e86b91a 100644 > > > > --- a/fsdev/file-op-9p.h > > > > +
[Qemu-devel] [PATCH v3] fsdev: add IO throttle support to fsdev devices
Uses throttling APIs to limit I/O bandwidth and number of operations on the fsdev devices. Signed-off-by: Pradeep Jagadeesh --- 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 ++ 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_BITS0600 #define SM_LOCAL_DIR_MODE_BITS0700 @@ -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, +
Re: [Qemu-devel] [PATCH v3] fsdev: add IO throttle support to fsdev devices
Hi Alberto, Thanks for having look at the patch. My replies are inline. On Fri 16 Sep 2016 10:33:36 AM CEST, Pradeep Jagadeesh wrote: Hi, first of all, sorry for the late reply! Here are my comments: No problem! --- 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-size", +.type = QEMU_OPT_NUMBER, +.help = "when limiting by iops max size of an I/O in bytes", It would be nice if we could factor these so we don't have to have them twice in the code. I think it should be doable with qemu_opts_append(). It can be done later in a separate patch if you prefer. OK,as of now I have just copied. We can rework and remove the redundant code later. +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; You based your implementation on block/throttle-groups.c. I think yours can be made simpler because one of the problems with mine is that it needs to support multiple parallel I/O operations on the same throttling group, and that's why the locking rules are more complex. With a single user per ThrottleConfig this is not necessary. Have you checked if you really need them? My impression is that you might be able to get rid of the 'lock', 'any_timer_armed' and 'pending_reqs' fields. Please check commit 76f4afb40fa076ed23fe0ab42c7a768ddb71123f to see how was the transition from single-drive throttling to group throttling, specifically the bdrv_io_limits_intercept() function. You will see that it was simpler. OK,I am not sure,I will have to try. I will look into this and let you know. @@ -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; + This could go to a separate patch, but I'm fine if you include it in this one. I have already included as Greg suggested. @@ -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; + I'd remove the empty line between the declarations of 'path' and 'fst', however... OK 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; +} ...you don't need to declare fst here, you could also pass &fse->fst directly. OK -Pradeep Berto
Re: [Qemu-devel] [PATCH v3] fsdev: add IO throttle support to fsdev devices
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_BITS0600 #define SM_LOCAL_DIR_MODE_BITS0700 @@ -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
Re: [Qemu-devel] [PATCH V8] fsdev: add IO throttle support to fsdev devices
Hi Greg, Sorry for the late reply, was out of office. I agree with all comments and fix them all. Also will send the patch soon. -Pradeep Hi Pradeep, There are still a couple of issues to address with this v8, even if we're not that far from the final version. On Sun, 30 Oct 2016 11:30:43 -0400 Pradeep Jagadeesh wrote: Date: Sun, 30 Oct 2016 10:53:16 -0400 Subject: [PATCH V8] fsdev: add IO throttle support to fsdev devices It is weird to have the Subject: in the mail body... what happened ? No idea what happened. Uses throttling APIs to limit I/O bandwidth and number of operations on the devices which use 9p-local driver. 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. Signed-off-by: Pradeep Jagadeesh --- fsdev/Makefile.objs | 2 +- fsdev/file-op-9p.h | 3 + fsdev/qemu-fsdev-opts.c | 76 fsdev/qemu-fsdev-throttle.c | 139 fsdev/qemu-fsdev-throttle.h | 39 + hw/9pfs/9p-local.c | 2 + hw/9pfs/9p.c| 10 hw/9pfs/cofile.c| 2 + 8 files changed, 272 insertions(+), 1 deletion(-) create mode 100644 fsdev/qemu-fsdev-throttle.c create mode 100644 fsdev/qemu-fsdev-throttle.h 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. v3 -> v4: -Addressed following comments by Alberto Garcia -Removed the unwanted locking and other data structures in qemu-fsdev-throttle.[ch] -Addressed following comments by Greg Kurz -Removed fsdev_iolimitsenable/disable functions, instead using throttle_enabled function v4 -> V5: -Fixed the issue with the larger block size accounting. (i.e, when the 9pfs mounted using msize=xxx option) V5 -> V6: -Addressed the comments by Alberto Garcia -Removed the fsdev_throttle_timer_cb() -Simplified the fsdev_throttle_schedule_next_request() as suggested V6 -> V7: -Addressed the comments by Alberto Garcia -changed the fsdev_throttle_schedule_next_request() as suggested v7 -> v8: -Addressed comments by Alberto Garcia -Fixed some indentation issues and split the configure_io_limit function -Inlined throttle_timer_check code diff --git a/fsdev/Makefile.objs b/fsdev/Makefile.objs index 1b120a4..659df6e 100644 --- a/fsdev/Makefile.objs +++ b/fsdev/Makefile.objs @@ -5,7 +5,7 @@ common-obj-y = qemu-fsdev.o 9p-marshal.o 9p-iov-marshal.o else common-obj-y = qemu-fsdev-dummy.o endif -common-obj-y += qemu-fsdev-opts.o +common-obj-y += qemu-fsdev-opts.o qemu-fsdev-throttle.o # Toplevel always builds this; targets without virtio will put it in # common-obj-y 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_BITS0600 #define SM_LOCAL_DIR_MODE_BITS0700 @@ -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 secon
[Qemu-devel] [V9] fsdev: add IO throttle support to fsdev devices
Uses throttling APIs to limit I/O bandwidth and number of operations on the devices which use 9p-local driver. Signed-off-by: Pradeep Jagadeesh --- fsdev/Makefile.objs | 2 +- fsdev/file-op-9p.h | 3 ++ fsdev/qemu-fsdev-opts.c | 77 +++- fsdev/qemu-fsdev-throttle.c | 119 fsdev/qemu-fsdev-throttle.h | 39 +++ hw/9pfs/9p-local.c | 8 +++ hw/9pfs/9p.c| 5 ++ hw/9pfs/cofile.c| 2 + 8 files changed, 253 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. v3 -> v4: -Addressed following comments by Alberto Garcia -Removed the unwanted locking and other data structures in qemu-fsdev-throttle.[ch] -Addressed following comments by Greg Kurz -Removed fsdev_iolimitsenable/disable functions, instead using throttle_enabled function v4 -> V5: -Fixed the issue with the larger block size accounting. (i.e, when the 9pfs mounted using msize=xxx option) V5 -> V6: -Addressed the comments by Alberto Garcia -Removed the fsdev_throttle_timer_cb() -Simplified the fsdev_throttle_schedule_next_request() as suggested V6 -> V7: -Addressed the comments by Alberto Garcia -changed the fsdev_throttle_schedule_next_request() as suggested v7 -> v8: -Addressed comments by Alberto Garcia -Fixed some indentation issues and split the configure_io_limit function -Inlined throttle_timer_check code V8 -> v9: -Addressed the comments by Greg Kurz -Inlined the fsdev_throttle_schedule_next_request() code into fsdev_co_throttle_request () diff --git a/fsdev/Makefile.objs b/fsdev/Makefile.objs index 1b120a4..659df6e 100644 --- a/fsdev/Makefile.objs +++ b/fsdev/Makefile.objs @@ -5,7 +5,7 @@ common-obj-y = qemu-fsdev.o 9p-marshal.o 9p-iov-marshal.o else common-obj-y = qemu-fsdev-dummy.o endif -common-obj-y += qemu-fsdev-opts.o +common-obj-y += qemu-fsdev-opts.o qemu-fsdev-throttle.o # Toplevel always builds this; targets without virtio will put it in # common-obj-y 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_BITS0600 #define SM_LOCAL_DIR_MODE_BITS0700 @@ -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..acb9485 100644 --- a/fsdev/qemu-fsdev-opts.c +++ b/fsdev/qemu-fsdev-opts.c @@ -37,8 +37,83 @@ 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", +},{ +
Re: [Qemu-devel] [V9] fsdev: add IO throttle support to fsdev devices
On Mon 07 Nov 2016 04:45:55 PM CET, Pradeep Jagadeesh wrote: --- a/fsdev/qemu-fsdev-opts.c +++ b/fsdev/qemu-fsdev-opts.c @@ -37,8 +37,83 @@ 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", +},{ Not very important, but since this version also needs to be corrected (as you'll see below) you can update this part to keep the sytle consisent: all other entries in this array have a space between the command and the opening brace ("}, {"). Most of your entries don't have that space ("},{") OK +void fsdev_throttle_parse_opts(QemuOpts *opts, FsThrottle *fst, Error *err) +{ +fst->cfg.buckets[THROTTLE_BPS_TOTAL].avg = +qemu_opt_get_number(opts, "throttling.bps-total", 0); /* ... */ +fst->cfg.op_size = +qemu_opt_get_number(opts, "throttling.iops-size", 0); + +if (!throttle_is_valid(&fst->cfg, &err)) { +return; +} +} That last 'if' is a bit odd :) but I won't object if you want to keep it. You can simply leave the throttle_is_valid() call and a comment that explains what it does ("Check that the config is valid and update errp otherwise"). I will better remove that. :) +void fsdev_throttle_init(FsThrottle *fst) +{ +if (throttle_enabled(&fst->cfg)) { +throttle_init(&fst->ts); This block is indented with 8 whitespaces, not with 4 as it should be. Hmm,I will fix it --- a/hw/9pfs/9p-local.c +++ b/hw/9pfs/9p-local.c @@ -1209,6 +1209,7 @@ 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"); +Error *err = NULL; if (!sec_model) { error_report("Security model not specified, local fs needs security model"); @@ -1237,6 +1238,13 @@ static int local_parse_opts(QemuOpts *opts, struct FsDriverEntry *fse) error_report("fsdev: No path specified"); return -1; } + +fsdev_throttle_parse_opts(opts, &fse->fst, err); +if (err) { +error_reportf_err(err, "Throttle configuration is not valid: "); +return -1; +} + This is the important part that needs to be changed: 'err' here is NULL, so you're passing a NULL pointer to fsdev_throttle_parse_opts() and invalid throttling configurations are not being reported. You should pass &err instead, and fsdev_throttle_parse_opts() must receive Error **errp. OK -Pradeep Berto
[Qemu-devel] [PATCH V10] fsdev: add IO throttle support to fsdev devices
Uses throttling APIs to limit I/O bandwidth and number of operations on the devices which use 9p-local driver. Signed-off-by: Pradeep Jagadeesh --- fsdev/Makefile.objs | 2 +- fsdev/file-op-9p.h | 3 ++ fsdev/qemu-fsdev-opts.c | 77 - fsdev/qemu-fsdev-throttle.c | 117 fsdev/qemu-fsdev-throttle.h | 39 +++ hw/9pfs/9p-local.c | 8 +++ hw/9pfs/9p.c| 5 ++ hw/9pfs/cofile.c| 2 + 8 files changed, 251 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. v3 -> v4: -Addressed following comments by Alberto Garcia -Removed the unwanted locking and other data structures in qemu-fsdev-throttle.[ch] -Addressed following comments by Greg Kurz -Removed fsdev_iolimitsenable/disable functions, instead using throttle_enabled function v4 -> V5: -Fixed the issue with the larger block size accounting. (i.e, when the 9pfs mounted using msize=xxx option) V5 -> V6: -Addressed the comments by Alberto Garcia -Removed the fsdev_throttle_timer_cb() -Simplified the fsdev_throttle_schedule_next_request() as suggested V6 -> V7: -Addressed the comments by Alberto Garcia -changed the fsdev_throttle_schedule_next_request() as suggested v7 -> v8: -Addressed comments by Alberto Garcia -Fixed some indentation issues and split the configure_io_limit function -Inlined throttle_timer_check code V8 -> v9: -Addressed the comments by Greg Kurz -Inlined the fsdev_throttle_schedule_next_request() code into fsdev_co_throttle_request () V9 -> V10: -Addressed the comments by Alberto Garcia -Fixed the indentation issues and minor issues diff --git a/fsdev/Makefile.objs b/fsdev/Makefile.objs index 1b120a4..659df6e 100644 --- a/fsdev/Makefile.objs +++ b/fsdev/Makefile.objs @@ -5,7 +5,7 @@ common-obj-y = qemu-fsdev.o 9p-marshal.o 9p-iov-marshal.o else common-obj-y = qemu-fsdev-dummy.o endif -common-obj-y += qemu-fsdev-opts.o +common-obj-y += qemu-fsdev-opts.o qemu-fsdev-throttle.o # Toplevel always builds this; targets without virtio will put it in # common-obj-y 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_BITS0600 #define SM_LOCAL_DIR_MODE_BITS0700 @@ -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..385423f0 100644 --- a/fsdev/qemu-fsdev-opts.c +++ b/fsdev/qemu-fsdev-opts.c @@ -37,8 +37,83 @@ 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
Re: [Qemu-devel] [PATCH V10] fsdev: add IO throttle support to fsdev devices
On 11/9/2016 11:23 AM, Alberto Garcia wrote: On Wed 09 Nov 2016 10:50:40 AM CET, Pradeep Jagadeesh wrote: Uses throttling APIs to limit I/O bandwidth and number of operations on the devices which use 9p-local driver. Signed-off-by: Pradeep Jagadeesh It looks good now, thanks! +void fsdev_throttle_parse_opts(QemuOpts *opts, FsThrottle *fst, Error **err) +{ [...] +throttle_is_valid(&fst->cfg, err); +} Following the QEMU conventions, I would still rename 'err' to 'errp' in this function (since it's an Error **). Otherwise, Thanks, I will change it. -Pradeep Reviewed-by: Alberto Garcia Berto
[Qemu-devel] [PATCH V11 1/1] fsdev: add IO throttle support to fsdev devices
Uses throttling APIs to limit I/O bandwidth and number of operations on the devices which use 9p-local driver. Signed-off-by: Pradeep Jagadeesh Reviewed-by: Alberto Garcia" --- fsdev/Makefile.objs | 2 +- fsdev/file-op-9p.h | 3 ++ fsdev/qemu-fsdev-opts.c | 77 - fsdev/qemu-fsdev-throttle.c | 117 fsdev/qemu-fsdev-throttle.h | 39 +++ hw/9pfs/9p-local.c | 8 +++ hw/9pfs/9p.c| 5 ++ hw/9pfs/cofile.c| 2 + 8 files changed, 251 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. v3 -> v4: -Addressed following comments by Alberto Garcia -Removed the unwanted locking and other data structures in qemu-fsdev-throttle.[ch] -Addressed following comments by Greg Kurz -Removed fsdev_iolimitsenable/disable functions, instead using throttle_enabled function v4 -> V5: -Fixed the issue with the larger block size accounting. (i.e, when the 9pfs mounted using msize=xxx option) V5 -> V6: -Addressed the comments by Alberto Garcia -Removed the fsdev_throttle_timer_cb() -Simplified the fsdev_throttle_schedule_next_request() as suggested V6 -> V7: -Addressed the comments by Alberto Garcia -changed the fsdev_throttle_schedule_next_request() as suggested v7 -> v8: -Addressed comments by Alberto Garcia -Fixed some indentation issues and split the configure_io_limit function -Inlined throttle_timer_check code V8 -> v9: -Addressed the comments by Greg Kurz -Inlined the fsdev_throttle_schedule_next_request() code into fsdev_co_throttle_request () v9 -> v10: -Addressed the comments by alberto garcia -fixed the indentation issues and minor issues v10 -> v11: -Addressed the comments by alberto garcia -renamed err variable to errp issues diff --git a/fsdev/Makefile.objs b/fsdev/Makefile.objs index 1b120a4..659df6e 100644 --- a/fsdev/Makefile.objs +++ b/fsdev/Makefile.objs @@ -5,7 +5,7 @@ common-obj-y = qemu-fsdev.o 9p-marshal.o 9p-iov-marshal.o else common-obj-y = qemu-fsdev-dummy.o endif -common-obj-y += qemu-fsdev-opts.o +common-obj-y += qemu-fsdev-opts.o qemu-fsdev-throttle.o # Toplevel always builds this; targets without virtio will put it in # common-obj-y 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_BITS0600 #define SM_LOCAL_DIR_MODE_BITS0700 @@ -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..385423f0 100644 --- a/fsdev/qemu-fsdev-opts.c +++ b/fsdev/qemu-fsdev-opts.c @@ -37,8 +37,83 @@ 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 = "lim
Re: [Qemu-devel] [PATCH V11 1/1] fsdev: add IO throttle support to fsdev devices
On 11/12/2016 3:13 PM, Greg Kurz wrote: On Fri, 11 Nov 2016 03:54:27 -0500 Pradeep Jagadeesh wrote: Uses throttling APIs to limit I/O bandwidth and number of operations on the devices which use 9p-local driver. Signed-off-by: Pradeep Jagadeesh Reviewed-by: Alberto Garcia" --- Hi Pradeep, I'll have a look next week but I'm not sure this can go to 2.8 since we're already in soft feature freeze (only bug fixes are accepted). Hi Greg, It is ok even if it does not make it to 2.8.I just want to complete this work from my side. Thanks, Pradeep Cheers. -- Greg fsdev/Makefile.objs | 2 +- fsdev/file-op-9p.h | 3 ++ fsdev/qemu-fsdev-opts.c | 77 - fsdev/qemu-fsdev-throttle.c | 117 fsdev/qemu-fsdev-throttle.h | 39 +++ hw/9pfs/9p-local.c | 8 +++ hw/9pfs/9p.c| 5 ++ hw/9pfs/cofile.c| 2 + 8 files changed, 251 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. v3 -> v4: -Addressed following comments by Alberto Garcia -Removed the unwanted locking and other data structures in qemu-fsdev-throttle.[ch] -Addressed following comments by Greg Kurz -Removed fsdev_iolimitsenable/disable functions, instead using throttle_enabled function v4 -> V5: -Fixed the issue with the larger block size accounting. (i.e, when the 9pfs mounted using msize=xxx option) V5 -> V6: -Addressed the comments by Alberto Garcia -Removed the fsdev_throttle_timer_cb() -Simplified the fsdev_throttle_schedule_next_request() as suggested V6 -> V7: -Addressed the comments by Alberto Garcia -changed the fsdev_throttle_schedule_next_request() as suggested v7 -> v8: -Addressed comments by Alberto Garcia -Fixed some indentation issues and split the configure_io_limit function -Inlined throttle_timer_check code V8 -> v9: -Addressed the comments by Greg Kurz -Inlined the fsdev_throttle_schedule_next_request() code into fsdev_co_throttle_request () v9 -> v10: -Addressed the comments by alberto garcia -fixed the indentation issues and minor issues v10 -> v11: -Addressed the comments by alberto garcia -renamed err variable to errp issues diff --git a/fsdev/Makefile.objs b/fsdev/Makefile.objs index 1b120a4..659df6e 100644 --- a/fsdev/Makefile.objs +++ b/fsdev/Makefile.objs @@ -5,7 +5,7 @@ common-obj-y = qemu-fsdev.o 9p-marshal.o 9p-iov-marshal.o else common-obj-y = qemu-fsdev-dummy.o endif -common-obj-y += qemu-fsdev-opts.o +common-obj-y += qemu-fsdev-opts.o qemu-fsdev-throttle.o # Toplevel always builds this; targets without virtio will put it in # common-obj-y 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_BITS0600 #define SM_LOCAL_DIR_MODE_BITS0700 @@ -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..385423f0 100644 --- a/fsdev/qemu-fsdev-opts.c +++ b/fsdev/qemu-fsdev-opts.c @@ -37,8 +37,83 @@ static QemuOptsList qemu_fsdev_opts = { }, { .name = "sock_fd", .type = QEMU_OPT_NUMBER, +}, { +.name = "throttling.iops-total", +.typ
Re: [Qemu-devel] [PATCH V11 1/1] fsdev: add IO throttle support to fsdev devices
On 11/14/2016 6:57 PM, Greg Kurz wrote: On Mon, 14 Nov 2016 10:03:40 +0100 Pradeep Jagadeesh wrote: On 11/12/2016 3:13 PM, Greg Kurz wrote: On Fri, 11 Nov 2016 03:54:27 -0500 Pradeep Jagadeesh wrote: Uses throttling APIs to limit I/O bandwidth and number of operations on the devices which use 9p-local driver. Signed-off-by: Pradeep Jagadeesh Reviewed-by: Alberto Garcia" --- Hi Pradeep, I'll have a look next week but I'm not sure this can go to 2.8 since we're already in soft feature freeze (only bug fixes are accepted). Hi Greg, It is ok even if it does not make it to 2.8.I just want to complete this work from my side. Hi Pradeep, The patch looks good to me now. Since we're no more in a hurry to get this merged, maybe you can now try to address the code duplication issue with the command line options ? Ideally this should be done in a preparatory patch. OK,I will start that work in couple of weeks. As I am busy with some other work. Cheers, Pradeep Cheers. -- Greg Thanks, Pradeep Cheers. -- Greg fsdev/Makefile.objs | 2 +- fsdev/file-op-9p.h | 3 ++ fsdev/qemu-fsdev-opts.c | 77 - fsdev/qemu-fsdev-throttle.c | 117 fsdev/qemu-fsdev-throttle.h | 39 +++ hw/9pfs/9p-local.c | 8 +++ hw/9pfs/9p.c| 5 ++ hw/9pfs/cofile.c| 2 + 8 files changed, 251 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. v3 -> v4: -Addressed following comments by Alberto Garcia -Removed the unwanted locking and other data structures in qemu-fsdev-throttle.[ch] -Addressed following comments by Greg Kurz -Removed fsdev_iolimitsenable/disable functions, instead using throttle_enabled function v4 -> V5: -Fixed the issue with the larger block size accounting. (i.e, when the 9pfs mounted using msize=xxx option) V5 -> V6: -Addressed the comments by Alberto Garcia -Removed the fsdev_throttle_timer_cb() -Simplified the fsdev_throttle_schedule_next_request() as suggested V6 -> V7: -Addressed the comments by Alberto Garcia -changed the fsdev_throttle_schedule_next_request() as suggested v7 -> v8: -Addressed comments by Alberto Garcia -Fixed some indentation issues and split the configure_io_limit function -Inlined throttle_timer_check code V8 -> v9: -Addressed the comments by Greg Kurz -Inlined the fsdev_throttle_schedule_next_request() code into fsdev_co_throttle_request () v9 -> v10: -Addressed the comments by alberto garcia -fixed the indentation issues and minor issues v10 -> v11: -Addressed the comments by alberto garcia -renamed err variable to errp issues diff --git a/fsdev/Makefile.objs b/fsdev/Makefile.objs index 1b120a4..659df6e 100644 --- a/fsdev/Makefile.objs +++ b/fsdev/Makefile.objs @@ -5,7 +5,7 @@ common-obj-y = qemu-fsdev.o 9p-marshal.o 9p-iov-marshal.o else common-obj-y = qemu-fsdev-dummy.o endif -common-obj-y += qemu-fsdev-opts.o +common-obj-y += qemu-fsdev-opts.o qemu-fsdev-throttle.o # Toplevel always builds this; targets without virtio will put it in # common-obj-y 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_BITS0600 #define SM_LOCAL_DIR_MODE_BITS0700 @@ -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; +FsThr
Re: [Qemu-devel] [PATCH V11 1/1] fsdev: add IO throttle support to fsdev devices
On 11/15/2016 10:58 AM, Greg Kurz wrote: On Tue, 15 Nov 2016 10:13:59 +0100 Pradeep Jagadeesh wrote: On 11/14/2016 6:57 PM, Greg Kurz wrote: On Mon, 14 Nov 2016 10:03:40 +0100 Pradeep Jagadeesh wrote: On 11/12/2016 3:13 PM, Greg Kurz wrote: On Fri, 11 Nov 2016 03:54:27 -0500 Pradeep Jagadeesh wrote: Uses throttling APIs to limit I/O bandwidth and number of operations on the devices which use 9p-local driver. Signed-off-by: Pradeep Jagadeesh Reviewed-by: Alberto Garcia" --- Hi Pradeep, I'll have a look next week but I'm not sure this can go to 2.8 since we're already in soft feature freeze (only bug fixes are accepted). Hi Greg, It is ok even if it does not make it to 2.8.I just want to complete this work from my side. Hi Pradeep, The patch looks good to me now. Since we're no more in a hurry to get this merged, maybe you can now try to address the code duplication issue with the command line options ? Ideally this should be done in a preparatory patch. OK,I will start that work in couple of weeks. As I am busy with some other work. Ok. It looks like we can factor two things: - the set of common throttle options in the QemuOptsList (I already have patch for that I can send to be part of your patchset) OK - the initialization of the throttle config OK BTW, looking at extract_common_blockdev_options(), I have the impression that a call to throttle_config_init() is missing... Cheers, Pradeep [...] +void fsdev_throttle_parse_opts(QemuOpts *opts, FsThrottle *fst, Error **errp) +{ ... here. good catch, I just put that line. -Pradeep +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); + +throttle_is_valid(&fst->cfg, errp); +} + +void fsdev_throttle_init(FsThrottle *fst) +{ +if (throttle_enabled(&fst->cfg)) { +throttle_init(&fst->ts); +throttle_timers_init(&fst->tt, + qemu_get_aio_context(), + 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]); +} +} + +void coroutine_fn fsdev_co_throttle_request(FsThrottle *fst, bool is_write, +struct iovec *iov, int iovcnt) +{ +
Re: [Qemu-devel] [PATCH v4] fsdev: add IO throttle support to fsdev devices
Hi Greg, On 10/7/2016 9:48 AM, Greg Kurz wrote: On Thu, 22 Sep 2016 07:59:19 -0400 Pradeep Jagadeesh wrote: Uses throttling APIs to limit I/O bandwidth and number of operations on the devices which use 9p-local driver. Signed-off-by: Pradeep Jagadeesh --- Hi Pradeep, So where are we with this patch ? Have you solved the issues you mentioned in ? I was busy with the Linuxcon in Berlin last week. The patch is going good.I am trying to find a fix for that issue I found before.I will try this week. I don't have much time so I'll wait for v5 to comment. Sure And maybe you can push the option list to a THROTTLE_COMMON_OPTS macro in a include/qemu/throttle-options.h header file, to be included by both blockdev.c and fsdev/qemu-fsdev-throttle.c. Ok, I will have a look. Regards, Pradeep Cheers. -- Greg fsdev/Makefile.objs | 1 + fsdev/file-op-9p.h | 3 + fsdev/qemu-fsdev-opts.c | 76 +++ fsdev/qemu-fsdev-throttle.c | 146 fsdev/qemu-fsdev-throttle.h | 36 +++ hw/9pfs/9p-local.c | 9 ++- hw/9pfs/9p.c| 6 ++ hw/9pfs/cofile.c| 3 + 8 files changed, 278 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. v3 -> v4: -Addressed following comments by Alberto Garcia -Removed the unwanted locking and other data structures in qemu-fsdev-throttle.[ch] -Addressed following comments by Greg Kurz -Removed fsdev_iolimitsenable/disable functions, instead using throttle_enabled function 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_BITS0600 #define SM_LOCAL_DIR_MODE_BITS0700 @@ -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, +.hel
Re: [Qemu-devel] Virtio-net cli parameters
Hi Stefan, Thanks for the reply. On 30 September 2016 at 15:49, Stefan Hajnoczi wrote: > On Thu, Sep 29, 2016 at 04:11:27PM +0200, Pradeep Kiruvale wrote: > > Hi Stefan, > > > > On 28 September 2016 at 10:29, Stefan Hajnoczi > wrote: > > > > > On Mon, Sep 26, 2016 at 05:41:55PM +0200, Pradeep Kiruvale wrote: > > > > I want to add couple of new cli options for the virtio-net driver > and use > > > > them inside the > > > > virtio-net driver to throttle the packets. I did go through the code > and > > > > did single stepping > > > > using the gdb, but still could not find the place where the > virtio-net > > > > parameters are getting > > > > parsed and populated into the virtio-net structures. > > > > Could some one please guide/suggest where to look into, in qemu code > > > base? > > > > > > Take a look at virtio_net_properties[] in hw/net/virtio-net.c. > > > > > > > These variables/properties are static variables. What I am looking about > is > > how can I pass a cli option from qemu into virtio-net driver. > > When you say "virtio-net driver" I think you mean the virtio-net > device emulation code in QEMU? > Yes, which the virtio-net driver uses to do the network I/O. > > These *are* the properties that can be set from the QEMU command-line. > For example: > > -device virtio-net-pci,csum=off > This is the clue, I was looking for. I will see how to proceed from here on. > > The command-line -device option creates device objects using > qdev_device_add() and then loops over the command-line parameters > calling set_property() on them. > > Hope this helps you understand the code. > Thanks, it will really help me to proceed with my work. Regards, Pradeep > > Stefan >
Re: [Qemu-devel] Virtio-net cli parameters
On 10 October 2016 at 11:26, Pradeep Kiruvale wrote: > Hi Stefan, > > Thanks for the reply. > > On 30 September 2016 at 15:49, Stefan Hajnoczi wrote: > >> On Thu, Sep 29, 2016 at 04:11:27PM +0200, Pradeep Kiruvale wrote: >> > Hi Stefan, >> > >> > On 28 September 2016 at 10:29, Stefan Hajnoczi >> wrote: >> > >> > > On Mon, Sep 26, 2016 at 05:41:55PM +0200, Pradeep Kiruvale wrote: >> > > > I want to add couple of new cli options for the virtio-net driver >> and use >> > > > them inside the >> > > > virtio-net driver to throttle the packets. I did go through the >> code and >> > > > did single stepping >> > > > using the gdb, but still could not find the place where the >> virtio-net >> > > > parameters are getting >> > > > parsed and populated into the virtio-net structures. >> > > > Could some one please guide/suggest where to look into, in qemu code >> > > base? >> > > >> > > Take a look at virtio_net_properties[] in hw/net/virtio-net.c. >> > > >> > >> > These variables/properties are static variables. What I am looking >> about is >> > how can I pass a cli option from qemu into virtio-net driver. >> >> When you say "virtio-net driver" I think you mean the virtio-net >> device emulation code in QEMU? >> > Yes, which the virtio-net driver uses to do the network I/O. > >> >> These *are* the properties that can be set from the QEMU command-line. >> For example: >> >> -device virtio-net-pci,csum=off >> > This is the clue, I was looking for. I will see how to proceed from here > on. > But when I have a cli option like below, how can pass above options? -net nic,macaddr=52:54:00:37:4d:10,model=virtio -Pradeep >> The command-line -device option creates device objects using >> qdev_device_add() and then loops over the command-line parameters >> calling set_property() on them. >> >> Hope this helps you understand the code. >> > Thanks, it will really help me to proceed with my work. > > Regards, > Pradeep > >> >> Stefan >> > >
Re: [Qemu-devel] Virtio-net cli parameters
On 14 October 2016 at 15:26, Stefan Hajnoczi wrote: > On Mon, Oct 10, 2016 at 03:00:20PM +0200, Pradeep Kiruvale wrote: > > On 10 October 2016 at 11:26, Pradeep Kiruvale > > > wrote: > > > > > Hi Stefan, > > > > > > Thanks for the reply. > > > > > > On 30 September 2016 at 15:49, Stefan Hajnoczi > wrote: > > > > > >> On Thu, Sep 29, 2016 at 04:11:27PM +0200, Pradeep Kiruvale wrote: > > >> > Hi Stefan, > > >> > > > >> > On 28 September 2016 at 10:29, Stefan Hajnoczi > > >> wrote: > > >> > > > >> > > On Mon, Sep 26, 2016 at 05:41:55PM +0200, Pradeep Kiruvale wrote: > > >> > > > I want to add couple of new cli options for the virtio-net > driver > > >> and use > > >> > > > them inside the > > >> > > > virtio-net driver to throttle the packets. I did go through the > > >> code and > > >> > > > did single stepping > > >> > > > using the gdb, but still could not find the place where the > > >> virtio-net > > >> > > > parameters are getting > > >> > > > parsed and populated into the virtio-net structures. > > >> > > > Could some one please guide/suggest where to look into, in qemu > code > > >> > > base? > > >> > > > > >> > > Take a look at virtio_net_properties[] in hw/net/virtio-net.c. > > >> > > > > >> > > > >> > These variables/properties are static variables. What I am looking > > >> about is > > >> > how can I pass a cli option from qemu into virtio-net driver. > > >> > > >> When you say "virtio-net driver" I think you mean the virtio-net > > >> device emulation code in QEMU? > > >> > > > Yes, which the virtio-net driver uses to do the network I/O. > > > > > >> > > >> These *are* the properties that can be set from the QEMU command-line. > > >> For example: > > >> > > >> -device virtio-net-pci,csum=off > > >> > > > This is the clue, I was looking for. I will see how to proceed from > here > > > on. > > > > > But when I have a cli option like below, how can pass above options? > > -net nic,macaddr=52:54:00:37:4d:10,model=virtio > > Please look at "Network Devices" in docs/qdev-device-use.txt for > information on modern -netdev/-device syntax. > > The old -net syntax should be replaced with the more powerful > -netdev/-device syntax. > > Thanks Stefan for suggestions. I will have a look. Regards, Pradeep Stefan >
Re: [Qemu-devel] [PATCH v3] fsdev: add IO throttle support to fsdev devices
Hi Alberto, On Fri 16 Sep 2016 10:33:36 AM CEST, Pradeep Jagadeesh wrote: Hi, first of all, sorry for the late reply! Here are my comments: --- 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-size", +.type = QEMU_OPT_NUMBER, +.help = "when limiting by iops max size of an I/O in bytes", It would be nice if we could factor these so we don't have to have them twice in the code. I think it should be doable with qemu_opts_append(). It can be done later in a separate patch if you prefer. +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; You based your implementation on block/throttle-groups.c. I think yours can be made simpler because one of the problems with mine is that it needs to support multiple parallel I/O operations on the same throttling group, and that's why the locking rules are more complex. With a single user per ThrottleConfig this is not necessary. Have you checked if you really need them? My impression is that you might be able to get rid of the 'lock', 'any_timer_armed' and 'pending_reqs' fields. Please check commit 76f4afb40fa076ed23fe0ab42c7a768ddb71123f to see how was the transition from single-drive throttling to group throttling, specifically the bdrv_io_limits_intercept() function. You will see that it was simpler. I tried removing the lock, I got into rcu issues, and the qemu hangs. Once I put them back, it works fine. -Pradeep @@ -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; + This could go to a separate patch, but I'm fine if you include it in this one. @@ -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; + I'd remove the empty line between the declarations of 'path' and 'fst', however... 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; +} ...you don't need to declare fst here, you could also pass &fse->fst directly. Berto
Re: [Qemu-devel] [PATCH v3] fsdev: add IO throttle support to fsdev devices
On 10/18/2016 6:19 PM, Alberto Garcia wrote: On Mon 17 Oct 2016 11:19:11 AM CEST, Pradeep Jagadeesh wrote: +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; You based your implementation on block/throttle-groups.c. I think yours can be made simpler because one of the problems with mine is that it needs to support multiple parallel I/O operations on the same throttling group, and that's why the locking rules are more complex. With a single user per ThrottleConfig this is not necessary. Have you checked if you really need them? My impression is that you might be able to get rid of the 'lock', 'any_timer_armed' and 'pending_reqs' fields. Please check commit 76f4afb40fa076ed23fe0ab42c7a768ddb71123f to see how was the transition from single-drive throttling to group throttling, specifically the bdrv_io_limits_intercept() function. You will see that it was simpler. I tried removing the lock, I got into rcu issues, and the qemu hangs. Once I put them back, it works fine. Did you figure out why the lock is needed in this case? In the case of disk group throttling, it is because there is data shared by several disks which may be running at the same time. If I remove the locks and run the IO workloads. I am get a message, "main-loop: WARNING: I/O thread spun for 1000 iterations" and then the qemu hangs. Even the monitor does not respond. I found that similar issues faced by many people, also some fixes, none of them worked for me. Regards, Pradeep Berto
Re: [Qemu-devel] [PATCH v3] fsdev: add IO throttle support to fsdev devices
On 10/18/2016 6:19 PM, Alberto Garcia wrote: On Mon 17 Oct 2016 11:19:11 AM CEST, Pradeep Jagadeesh wrote: +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; You based your implementation on block/throttle-groups.c. I think yours can be made simpler because one of the problems with mine is that it needs to support multiple parallel I/O operations on the same throttling group, and that's why the locking rules are more complex. With a single user per ThrottleConfig this is not necessary. Have you checked if you really need them? My impression is that you might be able to get rid of the 'lock', 'any_timer_armed' and 'pending_reqs' fields. Please check commit 76f4afb40fa076ed23fe0ab42c7a768ddb71123f to see how was the transition from single-drive throttling to group throttling, specifically the bdrv_io_limits_intercept() function. You will see that it was simpler. I tried removing the lock, I got into rcu issues, and the qemu hangs. Once I put them back, it works fine. Did you figure out why the lock is needed in this case? In the case of disk group throttling, it is because there is data shared by several disks which may be running at the same time. One more update is, I had a look at bdrv_io_limits_intercept and I just removed lock and retained other two things pending_reqs,any_timer_armed It works fine. -Pradeep Berto
Re: [Qemu-devel] [PATCH v3] fsdev: add IO throttle support to fsdev devices
On 10/20/2016 3:32 PM, Greg Kurz wrote: On Thu, 20 Oct 2016 14:50:29 +0200 Alberto Garcia wrote: On Wed 19 Oct 2016 06:29:45 PM CEST, Pradeep Jagadeesh wrote: One more update is, I had a look at bdrv_io_limits_intercept and I just removed lock and retained other two things pending_reqs,any_timer_armed It works fine. So you managed to make it work without the lock? Can you post the new version then? Thanks, Berto Pradeep, Just a reminder: soft freeze is coming (http://wiki.qemu.org/Planning/2.8). Unless this series is posted/reviewed in time (especially all the issues about locking and throttling not behaving as expected have been addressed and well tested), I won't be able to push it to 2.8. Hi Greg, Thanks for the release info.I am sending out patch today.Lets see how things go after that. Regards, Pradeep Cheers. -- Greg
Re: [Qemu-devel] [PATCH v3] fsdev: add IO throttle support to fsdev devices
On 10/20/2016 2:50 PM, Alberto Garcia wrote: On Wed 19 Oct 2016 06:29:45 PM CEST, Pradeep Jagadeesh wrote: One more update is, I had a look at bdrv_io_limits_intercept and I just removed lock and retained other two things pending_reqs,any_timer_armed It works fine. So you managed to make it work without the lock? Can you post the new version then? Sure, I will send soon. -Pradeep Thanks, Berto
[Qemu-devel] [PATCH V5] fsdev: add IO throttle support to fsdev devices
Signed-off-by: Pradeep Jagadeesh --- fsdev/Makefile.objs | 1 + fsdev/file-op-9p.h | 3 + fsdev/qemu-fsdev-opts.c | 76 fsdev/qemu-fsdev-throttle.c | 166 fsdev/qemu-fsdev-throttle.h | 37 ++ hw/9pfs/9p-local.c | 9 ++- hw/9pfs/9p.c| 6 ++ hw/9pfs/cofile.c| 5 ++ 8 files changed, 301 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. v3 -> v4: -Addressed following comments by Alberto Garcia -Removed the unwanted locking and other data structures in qemu-fsdev-throttle.[ch] -Addressed following comments by Greg Kurz -Removed fsdev_iolimitsenable/disable functions, instead using throttle_enabled function v4 -> V5: -Fixed the issue with the larger block size accounting. (i.e, when the 9pfs mounted using msize=xxx option) 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_BITS0600 #define SM_LOCAL_DIR_MODE_BITS0700 @@ -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 = &
Re: [Qemu-devel] [PATCH v4] fsdev: add IO throttle support to fsdev devices
On 10/7/2016 9:48 AM, Greg Kurz wrote: On Thu, 22 Sep 2016 07:59:19 -0400 Pradeep Jagadeesh wrote: Uses throttling APIs to limit I/O bandwidth and number of operations on the devices which use 9p-local driver. Signed-off-by: Pradeep Jagadeesh --- Hi Pradeep, So where are we with this patch ? Have you solved the issues you mentioned in ? I don't have much time so I'll wait for v5 to comment. And maybe you can push the option list to a THROTTLE_COMMON_OPTS macro in a include/qemu/throttle-options.h header file, to be included by both blockdev.c and fsdev/qemu-fsdev-throttle.c. As I understand correctly, this is about removing the duplication right? I did not take care of this comment. I would like to push it as part of may be next patch. Is that OK with you? Regards, Pradeep Cheers. -- Greg fsdev/Makefile.objs | 1 + fsdev/file-op-9p.h | 3 + fsdev/qemu-fsdev-opts.c | 76 +++ fsdev/qemu-fsdev-throttle.c | 146 fsdev/qemu-fsdev-throttle.h | 36 +++ hw/9pfs/9p-local.c | 9 ++- hw/9pfs/9p.c| 6 ++ hw/9pfs/cofile.c| 3 + 8 files changed, 278 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. v3 -> v4: -Addressed following comments by Alberto Garcia -Removed the unwanted locking and other data structures in qemu-fsdev-throttle.[ch] -Addressed following comments by Greg Kurz -Removed fsdev_iolimitsenable/disable functions, instead using throttle_enabled function 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_BITS0600 #define SM_LOCAL_DIR_MODE_BITS0700 @@ -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, +.hel
Re: [Qemu-devel] [PATCH V5] fsdev: add IO throttle support to fsdev devices
Alberto, Thanks for reviewing the patch. On Thu 20 Oct 2016 04:14:23 PM CEST, Pradeep Jagadeesh wrote: +static void fsdev_throttle_schedule_next_request(FsThrottle *fst, bool is_write) +{ +bool must_wait; + +if (qemu_co_queue_empty(&fst->throttled_reqs[is_write])) { +return; +} +must_wait = fsdev_throttle_check_for_wait(fst, is_write); +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); + } + } +} I think you don't need to schedule the timer here. This was added for group throttling for disks because once a disk completes an I/O request, the next request might be in a different disk (due to the round-robin algorithm). So we put a timer in that disk to handle that. In your case that cannot happen, see fsdev_throttle_request() below[1] for a suggestion of how it can look like. OK +static void fsdev_throttle_timer_cb(FsThrottle *fst, bool is_write) +{ +bool empty_queue; +empty_queue = !qemu_co_enter_next(&fst->throttled_reqs[is_write]); +if (empty_queue) { +fsdev_throttle_schedule_next_request(fst, is_write); +} +} This doesn't make any sense, if the throttled_reqs queue is empty then you're calling fsdev_throttle_schedule_next_request(), but how can a next request be scheduled if the queue is empty? :-) You don't need fsdev_throttle_timer_cb() at all, but you can simply do this instead: OK +static void fsdev_throttle_read_timer_cb(void *opaque) +{ FsThrottle *fst = opaque; qemu_co_enter_next(&fst->throttled_reqs[false]); +} + +static void fsdev_throttle_write_timer_cb(void *opaque) +{ FsThrottle *fst = opaque; qemu_co_enter_next(&fst->throttled_reqs[true]); +} +static int get_num_bytes(struct iovec *iov, int iovcnt) +{ +int index = 0; +int bytes = 0; + +while (index < iovcnt) { +bytes += iov[index].iov_len; +index++; +} +return bytes; You're looping over the iov vector, use a for loop instead, no?: for (i = 0; i < iovcnt; i++) { bytes += iov[i].iov_len; } OK +void fsdev_throttle_request(FsThrottle *fst, bool is_write, +struct iovec *iov, int iovcnt) First, mark the function as running in a coroutine context: OK void coroutine_fn fsdev_throttle_request(FsThrottle *fst, bool is_write, struct iovec *iov, int iovcnt) +{ +int bytes; + +if (throttle_enabled(&fst->cfg)) { I think it's probably better to check that in the caller, but I don't have a strong opinion. If you keep it like this, put the definition of 'bytes' inside the if block instead. I just thought its better to check for throttle enabled or not. +bytes = get_num_bytes(iov, iovcnt); +bool must_wait = fsdev_throttle_check_for_wait(fst, is_write); +if (must_wait || !qemu_co_queue_empty(&fst->throttled_reqs[is_write])) { +qemu_co_queue_wait(&fst->throttled_reqs[is_write]); + } + throttle_account(&fst->ts, is_write, bytes); + + fsdev_throttle_schedule_next_request(fst, is_write); So this is the [1] that I mentioned earlier. In your case instead of fsdev_throttle_schedule_next_request() I think you simply something like: if (!qemu_co_queue_empty(&fst->throttled_reqs[is_write])) { /* If there's a timer set, don't do anything else, the timer callback will schedule the next request */ if (fsdev_throttle_check_for_wait(fst, is_write)) { return; } /* If a timer is not set, then schedule the next request now */ qemu_co_queue_next(&fst->throttled_reqs[is_write]); } OK, also I changed the name as you suggested. -Pradeep Berto
[Qemu-devel] [V6] fsdev: add IO throttle support to fsdev devices
Signed-off-by: Pradeep Jagadeesh --- fsdev/Makefile.objs | 1 + fsdev/file-op-9p.h | 3 + fsdev/qemu-fsdev-opts.c | 76 +++ fsdev/qemu-fsdev-throttle.c | 147 fsdev/qemu-fsdev-throttle.h | 37 +++ hw/9pfs/9p-local.c | 9 ++- hw/9pfs/9p.c| 6 ++ hw/9pfs/cofile.c| 5 ++ 8 files changed, 282 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. v3 -> v4: -Addressed following comments by Alberto Garcia -Removed the unwanted locking and other data structures in qemu-fsdev-throttle.[ch] -Addressed following comments by Greg Kurz -Removed fsdev_iolimitsenable/disable functions, instead using throttle_enabled function v4 -> V5: -Fixed the issue with the larger block size accounting. (i.e, when the 9pfs mounted using msize=xxx option) V5 -> V6: -Addressed the comments by Alberto Garcia -Removed the fsdev_throttle_timer_cb() -Simplified the fsdev_throttle_schedule_next_request() as suggested 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_BITS0600 #define SM_LOCAL_DIR_MODE_BITS0700 @@ -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, +.
[Qemu-devel] [V7 1/1] fsdev: add IO throttle support to fsdev devices
Signed-off-by: Pradeep Jagadeesh --- fsdev/Makefile.objs | 1 + fsdev/file-op-9p.h | 3 + fsdev/qemu-fsdev-opts.c | 76 +++ fsdev/qemu-fsdev-throttle.c | 147 fsdev/qemu-fsdev-throttle.h | 37 +++ hw/9pfs/9p-local.c | 9 ++- hw/9pfs/9p.c| 6 ++ hw/9pfs/cofile.c| 5 ++ 8 files changed, 282 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. v3 -> v4: -Addressed following comments by Alberto Garcia -Removed the unwanted locking and other data structures in qemu-fsdev-throttle.[ch] -Addressed following comments by Greg Kurz -Removed fsdev_iolimitsenable/disable functions, instead using throttle_enabled function v4 -> V5: -Fixed the issue with the larger block size accounting. (i.e, when the 9pfs mounted using msize=xxx option) V5 -> V6: -Addressed the comments by Alberto Garcia -Removed the fsdev_throttle_timer_cb() -Simplified the fsdev_throttle_schedule_next_request() as suggested V6 -> V7: -Addressed the comments by Alberto Garcia -changed the fsdev_throttle_schedule_next_request() as suggested 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_BITS0600 #define SM_LOCAL_DIR_MODE_BITS0700 @@ -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", +
Re: [Qemu-devel] [V6] fsdev: add IO throttle support to fsdev devices
On Fri 21 Oct 2016 06:21:59 PM CEST, Pradeep Jagadeesh wrote: +static bool fsdev_throttle_check_for_wait(FsThrottle *fst, bool is_write) +{ + return throttle_schedule_timer(&fst->ts, &fst->tt, is_write); ^ There's an extra whitespace there. Removed +static void fsdev_throttle_schedule_next_request(FsThrottle *fst, bool is_write) +{ + if (!qemu_co_queue_empty(&fst->throttled_reqs[is_write])) { +if (fsdev_throttle_check_for_wait(fst, is_write)) { +return; +} + } + qemu_co_queue_next(&fst->throttled_reqs[is_write]); +} That is wrong, you're calling qemu_co_queue_next() even if the queue is empty. That line should be inside the 'if' block. Done! I still think that -after the changes you made in v6- you could put the contents of that whole function inside fsdev_co_throttle_request(), and at the same time get rid of fsdev_throttle_check_for_wait() and call throttle_schedule_timer() directly. The resulting code will be smaller and probably easier to read. But those are just style decisions. I just would like to keep these functions. +static int get_num_bytes(struct iovec *iov, int iovcnt) +{ +int i; +int bytes = 0; + +for (i = 0; i < iovcnt; i++) { +bytes += iov[i].iov_len; +} +return bytes; +} I overlooked that in my previous review, but 'bytes' should be unsigned (both here and in fsdev_co_throttle_request). Done! I still have to review the init/cleanup functions (I'll try to do it on monday) but else the code looks fine now. Ok, I am out of office for whole week. I can send the next patch only on next saturday/sunday. Please let me know all your comments by then. -Pradeep Berto
[Qemu-devel] [PATCH v5 1/5] throttle: factor out duplicate code
This patch factor out the duplicate throttle code that was present in block and fsdev devices. Signed-off-by: Pradeep Jagadeesh --- blockdev.c | 44 +--- fsdev/qemu-fsdev-throttle.c | 43 +-- fsdev/qemu-fsdev-throttle.h | 1 + include/qemu/throttle-options.h | 4 util/throttle.c | 50 + 5 files changed, 57 insertions(+), 85 deletions(-) diff --git a/blockdev.c b/blockdev.c index 6472548..5db9e5c 100644 --- a/blockdev.c +++ b/blockdev.c @@ -386,49 +386,7 @@ static void extract_common_blockdev_options(QemuOpts *opts, int *bdrv_flags, } if (throttle_cfg) { -throttle_config_init(throttle_cfg); -throttle_cfg->buckets[THROTTLE_BPS_TOTAL].avg = -qemu_opt_get_number(opts, "throttling.bps-total", 0); -throttle_cfg->buckets[THROTTLE_BPS_READ].avg = -qemu_opt_get_number(opts, "throttling.bps-read", 0); -throttle_cfg->buckets[THROTTLE_BPS_WRITE].avg = -qemu_opt_get_number(opts, "throttling.bps-write", 0); -throttle_cfg->buckets[THROTTLE_OPS_TOTAL].avg = -qemu_opt_get_number(opts, "throttling.iops-total", 0); -throttle_cfg->buckets[THROTTLE_OPS_READ].avg = -qemu_opt_get_number(opts, "throttling.iops-read", 0); -throttle_cfg->buckets[THROTTLE_OPS_WRITE].avg = -qemu_opt_get_number(opts, "throttling.iops-write", 0); - -throttle_cfg->buckets[THROTTLE_BPS_TOTAL].max = -qemu_opt_get_number(opts, "throttling.bps-total-max", 0); -throttle_cfg->buckets[THROTTLE_BPS_READ].max = -qemu_opt_get_number(opts, "throttling.bps-read-max", 0); -throttle_cfg->buckets[THROTTLE_BPS_WRITE].max = -qemu_opt_get_number(opts, "throttling.bps-write-max", 0); -throttle_cfg->buckets[THROTTLE_OPS_TOTAL].max = -qemu_opt_get_number(opts, "throttling.iops-total-max", 0); -throttle_cfg->buckets[THROTTLE_OPS_READ].max = -qemu_opt_get_number(opts, "throttling.iops-read-max", 0); -throttle_cfg->buckets[THROTTLE_OPS_WRITE].max = -qemu_opt_get_number(opts, "throttling.iops-write-max", 0); - -throttle_cfg->buckets[THROTTLE_BPS_TOTAL].burst_length = -qemu_opt_get_number(opts, "throttling.bps-total-max-length", 1); -throttle_cfg->buckets[THROTTLE_BPS_READ].burst_length = -qemu_opt_get_number(opts, "throttling.bps-read-max-length", 1); -throttle_cfg->buckets[THROTTLE_BPS_WRITE].burst_length = -qemu_opt_get_number(opts, "throttling.bps-write-max-length", 1); -throttle_cfg->buckets[THROTTLE_OPS_TOTAL].burst_length = -qemu_opt_get_number(opts, "throttling.iops-total-max-length", 1); -throttle_cfg->buckets[THROTTLE_OPS_READ].burst_length = -qemu_opt_get_number(opts, "throttling.iops-read-max-length", 1); -throttle_cfg->buckets[THROTTLE_OPS_WRITE].burst_length = -qemu_opt_get_number(opts, "throttling.iops-write-max-length", 1); - -throttle_cfg->op_size = -qemu_opt_get_number(opts, "throttling.iops-size", 0); - +throttle_parse_options(throttle_cfg, opts); if (!throttle_is_valid(throttle_cfg, errp)) { return; } diff --git a/fsdev/qemu-fsdev-throttle.c b/fsdev/qemu-fsdev-throttle.c index 7ae4e86..da9c225 100644 --- a/fsdev/qemu-fsdev-throttle.c +++ b/fsdev/qemu-fsdev-throttle.c @@ -31,48 +31,7 @@ static void fsdev_throttle_write_timer_cb(void *opaque) void fsdev_throttle_parse_opts(QemuOpts *opts, FsThrottle *fst, Error **errp) { -throttle_config_init(&fst->cfg); -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, "thrott
[Qemu-devel] [PATCH v5 2/5] qmp: Create IOThrottle structure
This patch enables qmp interfaces for the fsdev devices. This provides two interfaces one for querying info of all the fsdev devices. The second one to set the IO limits for the required fsdev device. Signed-off-by: Pradeep Jagadeesh Reviewed-by: Greg Kurz Reviewed-by: Eric Blake --- qapi/block-core.json | 76 ++--- qapi/iothrottle.json | 88 2 files changed, 91 insertions(+), 73 deletions(-) create mode 100644 qapi/iothrottle.json diff --git a/qapi/block-core.json b/qapi/block-core.json index f85c223..9320974 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -6,6 +6,7 @@ # QAPI common definitions { 'include': 'common.json' } +{ 'include': 'iothrottle.json' } ## # @SnapshotInfo: @@ -1761,84 +1762,13 @@ # # @device: Block device name (deprecated, use @id instead) # -# @id: The name or QOM path of the guest device (since: 2.8) -# -# @bps: total throughput limit in bytes per second -# -# @bps_rd: read throughput limit in bytes per second -# -# @bps_wr: write throughput limit in bytes per second -# -# @iops: total I/O operations per second -# -# @iops_rd: read I/O operations per second -# -# @iops_wr: write I/O operations per second -# -# @bps_max: total throughput limit during bursts, -# in bytes (Since 1.7) -# -# @bps_rd_max: read throughput limit during bursts, -#in bytes (Since 1.7) -# -# @bps_wr_max: write throughput limit during bursts, -#in bytes (Since 1.7) -# -# @iops_max: total I/O operations per second during bursts, -# in bytes (Since 1.7) -# -# @iops_rd_max: read I/O operations per second during bursts, -# in bytes (Since 1.7) -# -# @iops_wr_max: write I/O operations per second during bursts, -# in bytes (Since 1.7) -# -# @bps_max_length: maximum length of the @bps_max burst -#period, in seconds. It must only -#be set if @bps_max is set as well. -#Defaults to 1. (Since 2.6) -# -# @bps_rd_max_length: maximum length of the @bps_rd_max -# burst period, in seconds. It must only -# be set if @bps_rd_max is set as well. -# Defaults to 1. (Since 2.6) -# -# @bps_wr_max_length: maximum length of the @bps_wr_max -# burst period, in seconds. It must only -# be set if @bps_wr_max is set as well. -# Defaults to 1. (Since 2.6) -# -# @iops_max_length: maximum length of the @iops burst -# period, in seconds. It must only -# be set if @iops_max is set as well. -# Defaults to 1. (Since 2.6) -# -# @iops_rd_max_length: maximum length of the @iops_rd_max -#burst period, in seconds. It must only -#be set if @iops_rd_max is set as well. -#Defaults to 1. (Since 2.6) -# -# @iops_wr_max_length: maximum length of the @iops_wr_max -#burst period, in seconds. It must only -#be set if @iops_wr_max is set as well. -#Defaults to 1. (Since 2.6) -# -# @iops_size: an I/O size in bytes (Since 1.7) -# # @group: throttle group name (Since 2.4) # # Since: 1.1 ## { 'struct': 'BlockIOThrottle', - 'data': { '*device': 'str', '*id': 'str', 'bps': 'int', 'bps_rd': 'int', -'bps_wr': 'int', 'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int', -'*bps_max': 'int', '*bps_rd_max': 'int', -'*bps_wr_max': 'int', '*iops_max': 'int', -'*iops_rd_max': 'int', '*iops_wr_max': 'int', -'*bps_max_length': 'int', '*bps_rd_max_length': 'int', -'*bps_wr_max_length': 'int', '*iops_max_length': 'int', -'*iops_rd_max_length': 'int', '*iops_wr_max_length': 'int', -'*iops_size': 'int', '*group': 'str' } } + 'base': 'IOThrottle', + 'data': { '*device': 'str', '*group': 'str' } } ## # @block-stream: diff --git a/qapi/iothrottle.json b/qapi/iothrottle.json new file mode 100644 index 000..0
[Qemu-devel] [PATCH v5 0/5] fsdev: qmp interface for io throttling
These patches provide the qmp interface, to query the io throttle status of the all fsdev devices that are present in a vm. also, it provides an interface to set the io throttle parameters of a fsdev to a required value. some of the patches also remove the duplicate code that was present in block and fsdev files. Pradeep Jagadeesh (5): throttle: factor out duplicate code qmp: Create IOThrottle structure qmp: refactor duplicate code fsdev: hmp interface for throttling fsdev: QMP interface for throttling Makefile| 3 + blockdev.c | 97 ++--- fsdev/qemu-fsdev-dummy.c| 10 fsdev/qemu-fsdev-throttle.c | 118 ++-- fsdev/qemu-fsdev-throttle.h | 13 + fsdev/qemu-fsdev.c | 37 + hmp-commands-info.hx| 18 ++ hmp-commands.hx | 19 +++ hmp.c | 87 +++-- hmp.h | 4 ++ include/qemu/throttle-options.h | 7 +++ monitor.c | 5 ++ qapi-schema.json| 3 + qapi/block-core.json| 76 +- qapi/fsdev.json | 84 qapi/iothrottle.json| 88 ++ qmp.c | 15 + util/throttle.c | 110 + 18 files changed, 580 insertions(+), 214 deletions(-) create mode 100644 qapi/fsdev.json create mode 100644 qapi/iothrottle.json v0 -> v1: Addressed comments from Eric Blake, Greg Kurz and Daniel P.Berrange Mainly renaming the functions and removing the redundant code. v1 -> v2: Addressed comments from Eric Blake and Greg Kurz. As per the suggestion I split the patches into smaller patches. Removed some more duplicate code. v2 -> v3: Addresssed comments from Alberto Garcia. Changed the comment from block to iothrottle in the iothrottle.json Added the dummy functions in qemu-fsdev-dummy.c to address the compilation issues that were observed. v3 -> v4: Addressed comments from Eric Blake and Greg Kurz Re-ordered the patches Added the dummy functions in qmp.c to address the cross compilation issues v4 -> v5: Addressed comments from Eric Blake and Greg Kurz Split the fsdev qmp patch into hmp and qmp related patches Moved the common functionalities to throttle.c instead of creating a new file -- 1.8.3.1
[Qemu-devel] [PATCH v5 5/5] fsdev: QMP interface for throttling
This patch enables qmp interfaces for the fsdev devices. This provides two interfaces one for querying info of all the fsdev devices. The second one to set the IO limits for the required fsdev device. Signed-off-by: Pradeep Jagadeesh --- Makefile| 3 ++ fsdev/qemu-fsdev-dummy.c| 10 ++ fsdev/qemu-fsdev-throttle.c | 75 fsdev/qemu-fsdev-throttle.h | 12 +++ fsdev/qemu-fsdev.c | 37 monitor.c | 5 +++ qapi-schema.json| 3 ++ qapi/fsdev.json | 84 + qmp.c | 15 9 files changed, 244 insertions(+) create mode 100644 qapi/fsdev.json diff --git a/Makefile b/Makefile index c830d7a..996b1cf 100644 --- a/Makefile +++ b/Makefile @@ -414,6 +414,9 @@ qapi-modules = $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/qapi/common.json \ $(SRC_PATH)/qapi/event.json $(SRC_PATH)/qapi/introspect.json \ $(SRC_PATH)/qapi/crypto.json $(SRC_PATH)/qapi/rocker.json \ $(SRC_PATH)/qapi/trace.json +ifdef CONFIG_VIRTFS +qapi-modules += $(SRC_PATH)/qapi/fsdev.json +endif qapi-types.c qapi-types.h :\ $(qapi-modules) $(SRC_PATH)/scripts/qapi-types.py $(qapi-py) diff --git a/fsdev/qemu-fsdev-dummy.c b/fsdev/qemu-fsdev-dummy.c index 6dc0fbc..f33305d 100644 --- a/fsdev/qemu-fsdev-dummy.c +++ b/fsdev/qemu-fsdev-dummy.c @@ -19,3 +19,13 @@ int qemu_fsdev_add(QemuOpts *opts) { return 0; } + +void qmp_fsdev_set_io_throttle(IOThrottle *arg, Error **errp) +{ + return; +} + +IOThrottleList *qmp_query_fsdev_io_throttle(Error **errp) +{ +abort(); +} diff --git a/fsdev/qemu-fsdev-throttle.c b/fsdev/qemu-fsdev-throttle.c index da9c225..4483533 100644 --- a/fsdev/qemu-fsdev-throttle.c +++ b/fsdev/qemu-fsdev-throttle.c @@ -29,6 +29,81 @@ static void fsdev_throttle_write_timer_cb(void *opaque) qemu_co_enter_next(&fst->throttled_reqs[true]); } +void fsdev_set_io_throttle(IOThrottle *arg, FsThrottle *fst, Error **errp) +{ +ThrottleConfig cfg; + +throttle_set_io_limits(&cfg, arg); + +if (throttle_is_valid(&cfg, errp)) { +fst->cfg = cfg; +fsdev_throttle_init(fst); +} +} + +void fsdev_get_io_throttle(FsThrottle *fst, IOThrottle **fs9pcfg, + char *fsdevice, Error **errp) +{ + +ThrottleConfig cfg = fst->cfg; +IOThrottle *fscfg = g_malloc0(sizeof(*fscfg)); + +fscfg->has_id = true; +fscfg->id = g_strdup(fsdevice); +fscfg->bps = cfg.buckets[THROTTLE_BPS_TOTAL].avg; +fscfg->bps_rd = cfg.buckets[THROTTLE_BPS_READ].avg; +fscfg->bps_wr = cfg.buckets[THROTTLE_BPS_WRITE].avg; + +fscfg->iops = cfg.buckets[THROTTLE_OPS_TOTAL].avg; +fscfg->iops_rd = cfg.buckets[THROTTLE_OPS_READ].avg; +fscfg->iops_wr = cfg.buckets[THROTTLE_OPS_WRITE].avg; + +fscfg->has_bps_max = cfg.buckets[THROTTLE_BPS_TOTAL].max; +fscfg->bps_max = cfg.buckets[THROTTLE_BPS_TOTAL].max; +fscfg->has_bps_rd_max = cfg.buckets[THROTTLE_BPS_READ].max; +fscfg->bps_rd_max = cfg.buckets[THROTTLE_BPS_READ].max; +fscfg->has_bps_wr_max = cfg.buckets[THROTTLE_BPS_WRITE].max; +fscfg->bps_wr_max = cfg.buckets[THROTTLE_BPS_WRITE].max; + +fscfg->has_iops_max= cfg.buckets[THROTTLE_OPS_TOTAL].max; +fscfg->iops_max= cfg.buckets[THROTTLE_OPS_TOTAL].max; +fscfg->has_iops_rd_max = cfg.buckets[THROTTLE_OPS_READ].max; +fscfg->iops_rd_max = cfg.buckets[THROTTLE_OPS_READ].max; +fscfg->has_iops_wr_max = cfg.buckets[THROTTLE_OPS_WRITE].max; +fscfg->iops_wr_max = cfg.buckets[THROTTLE_OPS_WRITE].max; + +fscfg->has_bps_max_length = fscfg->has_bps_max; +fscfg->bps_max_length = + cfg.buckets[THROTTLE_BPS_TOTAL].burst_length; +fscfg->has_bps_rd_max_length = fscfg->has_bps_rd_max; +fscfg->bps_rd_max_length = + cfg.buckets[THROTTLE_BPS_READ].burst_length; +fscfg->has_bps_wr_max_length = fscfg->has_bps_wr_max; +fscfg->bps_wr_max_length = + cfg.buckets[THROTTLE_BPS_WRITE].burst_length; + +fscfg->has_iops_max_length= fscfg->has_iops_max; +fscfg->iops_max_length= + cfg.buckets[THROTTLE_OPS_TOTAL].burst_length; +fscfg->has_iops_rd_max_length = fscfg->has_iops_rd_max; +fscfg->iops_rd_max_length = + cfg.buckets[THROTTLE_OPS_READ].burst_length; +fscfg->has_iops_wr_max_length = fscfg->has_iops_wr_max; +fscfg->iops_wr_max_length = + cfg.buckets[THROTTLE_OPS_WRITE].burst_length; + +fscfg->bps_max_length = cfg.buckets[THROTTLE_BPS_TOTAL].burst_length; +fscfg->bps_rd_max_length = cfg.buckets[THROTTLE_BPS_READ].burst_length; +fscfg->bps_wr_max_length = cfg.buckets[THROTTLE_BPS_WRITE].burst_le
[Qemu-devel] [PATCH v5 4/5] fsdev: hmp interface for throttling
This patch introduces hmp interfaces for the fsdev throttle functionality Signed-off-by: Pradeep Jagadeesh --- hmp-commands-info.hx | 18 ++ hmp-commands.hx | 19 +++ hmp.c| 66 hmp.h| 4 4 files changed, 107 insertions(+) diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx index ae16901..f23b627 100644 --- a/hmp-commands-info.hx +++ b/hmp-commands-info.hx @@ -84,6 +84,24 @@ STEXI Show block device statistics. ETEXI +#if defined(CONFIG_VIRTFS) + +{ +.name = "query-fsdev-iothrottle", +.args_type = "", +.params = "", +.help = "show fsdev device throttle information", +.cmd= hmp_fsdev_get_io_throttle, +}, + +#endif + +STEXI +@item info fsdev throttle +@findex fsdevthrottleinfo +Show fsdev device throttleinfo. +ETEXI + { .name = "block-jobs", .args_type = "", diff --git a/hmp-commands.hx b/hmp-commands.hx index e763606..c60fd7e 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -1662,6 +1662,25 @@ STEXI Change I/O throttle limits for a block drive to @var{bps} @var{bps_rd} @var{bps_wr} @var{iops} @var{iops_rd} @var{iops_wr} ETEXI +#if defined(CONFIG_VIRTFS) + +{ +.name = "fsdev-set-io-throttle", +.args_type = "device:B,bps:l,bps_rd:l,bps_wr:l,iops:l,iops_rd:l,iops_wr:l", +.params = "device bps bps_rd bps_wr iops iops_rd iops_wr", +.help = "change I/O throttle limits for a fs devices", +.cmd= hmp_fsdev_set_io_throttle, +}, + +#endif + +STEXI +@item fsdev_set_io_throttle @var{device} @var{bps} @var{bps_rd} @var{bps_wr} @var{iops} @var{iops_rd} @var{iops_wr} +@findex fsdev_set_io_throttle +Change I/O throttle limits for a fs devices to @var{bps} @var{bps_rd} @var{bps_wr} @var{iops} @var{iops_rd} @var{iops_wr} +ETEXI + + { .name = "set_password", .args_type = "protocol:s,password:s,connected:s?", diff --git a/hmp.c b/hmp.c index 220d301..b1c698b 100644 --- a/hmp.c +++ b/hmp.c @@ -1776,6 +1776,72 @@ void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict) hmp_handle_error(mon, &err); } +#ifdef CONFIG_VIRTFS + +void hmp_fsdev_set_io_throttle(Monitor *mon, const QDict *qdict) +{ +Error *err = NULL; +IOThrottle throttle; + +hmp_initialize_io_throttle(&throttle, qdict); +qmp_fsdev_set_io_throttle(&throttle, &err); +hmp_handle_error(mon, &err); +} + +static void print_fsdev_throttle_config(Monitor *mon, IOThrottle *fscfg, + Error *err) +{ +if (fscfg->bps || fscfg->bps_rd || fscfg->bps_wr || +fscfg->iops || fscfg->iops_rd || fscfg->iops_wr) +{ +monitor_printf(mon, "%s", fscfg->id); +monitor_printf(mon, "I/O throttling:" +" bps=%" PRId64 +" bps_rd=%" PRId64 " bps_wr=%" PRId64 +" bps_max=%" PRId64 +" bps_rd_max=%" PRId64 +" bps_wr_max=%" PRId64 +" iops=%" PRId64 " iops_rd=%" PRId64 +" iops_wr=%" PRId64 +" iops_max=%" PRId64 +" iops_rd_max=%" PRId64 +" iops_wr_max=%" PRId64 +" iops_size=%" PRId64, +fscfg->bps, +fscfg->bps_rd, +fscfg->bps_wr, +fscfg->bps_max, +fscfg->bps_rd_max, +fscfg->bps_wr_max, +fscfg->iops, +fscfg->iops_rd, +fscfg->iops_wr, +fscfg->iops_max, +fscfg->iops_rd_max, +fscfg->iops_wr_max, +fscfg->iops_size); + } + hmp_handle_error(mon, &err); +} + +void hmp_fsdev_get_io_throttle(Monitor *mon, const QDict *qdict) +{ +Error *err = NULL; +IOThrottleList *fs9p_list, *info; +fs9p_list = qmp_query_fsdev_io_throttle(&err); + +for (info = fs9p_list; info; info = info->next) { +if (info != fs9p_list) { +monitor_printf(mon, "\n"); +} +print_fsdev_throttle_config(mon, info->value, err); +qapi_free_IOThrottle(info->value); +} +qapi_free_IOThrottleList(fs9p_list); +} + +#endif + void hmp_block_stream(Monitor *mon, const QDict *qdict)
[Qemu-devel] [PATCH v5 3/5] qmp: refactor duplicate code
This patch factor out the duplicate qmp throttle interface code that was present in both block and fsdev device files. Signed-off-by: Pradeep Jagadeesh --- blockdev.c | 53 +++- hmp.c | 21 ++- include/qemu/throttle-options.h | 3 +++ util/throttle.c | 60 + 4 files changed, 81 insertions(+), 56 deletions(-) diff --git a/blockdev.c b/blockdev.c index 5db9e5c..3d06e9e 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2593,6 +2593,7 @@ void qmp_block_set_io_throttle(BlockIOThrottle *arg, Error **errp) BlockDriverState *bs; BlockBackend *blk; AioContext *aio_context; +IOThrottle *iothrottle; blk = qmp_get_blk(arg->has_device ? arg->device : NULL, arg->has_id ? arg->id : NULL, @@ -2610,56 +2611,8 @@ void qmp_block_set_io_throttle(BlockIOThrottle *arg, Error **errp) goto out; } -throttle_config_init(&cfg); -cfg.buckets[THROTTLE_BPS_TOTAL].avg = arg->bps; -cfg.buckets[THROTTLE_BPS_READ].avg = arg->bps_rd; -cfg.buckets[THROTTLE_BPS_WRITE].avg = arg->bps_wr; - -cfg.buckets[THROTTLE_OPS_TOTAL].avg = arg->iops; -cfg.buckets[THROTTLE_OPS_READ].avg = arg->iops_rd; -cfg.buckets[THROTTLE_OPS_WRITE].avg = arg->iops_wr; - -if (arg->has_bps_max) { -cfg.buckets[THROTTLE_BPS_TOTAL].max = arg->bps_max; -} -if (arg->has_bps_rd_max) { -cfg.buckets[THROTTLE_BPS_READ].max = arg->bps_rd_max; -} -if (arg->has_bps_wr_max) { -cfg.buckets[THROTTLE_BPS_WRITE].max = arg->bps_wr_max; -} -if (arg->has_iops_max) { -cfg.buckets[THROTTLE_OPS_TOTAL].max = arg->iops_max; -} -if (arg->has_iops_rd_max) { -cfg.buckets[THROTTLE_OPS_READ].max = arg->iops_rd_max; -} -if (arg->has_iops_wr_max) { -cfg.buckets[THROTTLE_OPS_WRITE].max = arg->iops_wr_max; -} - -if (arg->has_bps_max_length) { -cfg.buckets[THROTTLE_BPS_TOTAL].burst_length = arg->bps_max_length; -} -if (arg->has_bps_rd_max_length) { -cfg.buckets[THROTTLE_BPS_READ].burst_length = arg->bps_rd_max_length; -} -if (arg->has_bps_wr_max_length) { -cfg.buckets[THROTTLE_BPS_WRITE].burst_length = arg->bps_wr_max_length; -} -if (arg->has_iops_max_length) { -cfg.buckets[THROTTLE_OPS_TOTAL].burst_length = arg->iops_max_length; -} -if (arg->has_iops_rd_max_length) { -cfg.buckets[THROTTLE_OPS_READ].burst_length = arg->iops_rd_max_length; -} -if (arg->has_iops_wr_max_length) { -cfg.buckets[THROTTLE_OPS_WRITE].burst_length = arg->iops_wr_max_length; -} - -if (arg->has_iops_size) { -cfg.op_size = arg->iops_size; -} +iothrottle = qapi_BlockIOThrottle_base(arg); +throttle_set_io_limits(&cfg, iothrottle); if (!throttle_is_valid(&cfg, errp)) { goto out; diff --git a/hmp.c b/hmp.c index 8c72c58..220d301 100644 --- a/hmp.c +++ b/hmp.c @@ -1749,20 +1749,29 @@ void hmp_change(Monitor *mon, const QDict *qdict) hmp_handle_error(mon, &err); } +static void hmp_initialize_io_throttle(IOThrottle *iot, const QDict *qdict) +{ +iot->has_id = true; +iot->id = (char *) qdict_get_str(qdict, "id"); +iot->bps = qdict_get_int(qdict, "bps"); +iot->bps_rd = qdict_get_int(qdict, "bps_rd"); +iot->bps_wr = qdict_get_int(qdict, "bps_wr"); +iot->iops = qdict_get_int(qdict, "iops"); +iot->iops_rd = qdict_get_int(qdict, "iops_rd"); +iot->iops_wr = qdict_get_int(qdict, "iops_wr"); +} + void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict) { Error *err = NULL; +IOThrottle *iothrottle; BlockIOThrottle throttle = { .has_device = true, .device = (char *) qdict_get_str(qdict, "device"), -.bps = qdict_get_int(qdict, "bps"), -.bps_rd = qdict_get_int(qdict, "bps_rd"), -.bps_wr = qdict_get_int(qdict, "bps_wr"), -.iops = qdict_get_int(qdict, "iops"), -.iops_rd = qdict_get_int(qdict, "iops_rd"), -.iops_wr = qdict_get_int(qdict, "iops_wr"), }; +iothrottle = qapi_BlockIOThrottle_base(&throttle); +hmp_initialize_io_throttle(iothrottle, qdict); qmp_block_set_io_throttle(&throttle, &err); hmp_handle_error(mon, &err); } diff --git a/include/qemu/throttle-options.h b/include/qemu/throttle-options.h index 565553a..e94ea39 100644 --- a/include/qemu/throttle-options.h +++ b/include/qemu/throttle-options.h @@ -11,6 +11,7 @@ #define THROTTLE_OPTIONS_H #include "qemu/throttle.h" +#include "qmp-commands.h" #def
Re: [Qemu-devel] [PATCH v5 3/5] qmp: refactor duplicate code
On 6/20/2017 6:05 PM, Greg Kurz wrote: On Mon, 19 Jun 2017 09:11:34 -0400 Pradeep Jagadeesh wrote: This patch factor out the duplicate qmp throttle interface code that was present in both block and fsdev device files. The text is obviously wrong. I don't see any duplicate code below. OK, I will fix this. It is more something like: let's factor out the code that will be used by the existing QMP throttling API for block devices and the future QMP throttling API for fs devices. Please move the HMP part to another patch, as asked during v4 review. I have moved the hmp patches for the fsdev into a separate patch. Do you want me to push this also into a separate patch? Also, blockdev.c and hmp.c do have maintainers. You should Cc: them each time because they know better than me and even if these patches are carried through my tree, I won't do it without an ack from them. OK, I will add them next time on. -Pradeep Cc'ing Markus for blockdev.c and David for hmp.c. Signed-off-by: Pradeep Jagadeesh --- blockdev.c | 53 +++- hmp.c | 21 ++- include/qemu/throttle-options.h | 3 +++ util/throttle.c | 60 + 4 files changed, 81 insertions(+), 56 deletions(-) diff --git a/blockdev.c b/blockdev.c index 5db9e5c..3d06e9e 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2593,6 +2593,7 @@ void qmp_block_set_io_throttle(BlockIOThrottle *arg, Error **errp) BlockDriverState *bs; BlockBackend *blk; AioContext *aio_context; +IOThrottle *iothrottle; blk = qmp_get_blk(arg->has_device ? arg->device : NULL, arg->has_id ? arg->id : NULL, @@ -2610,56 +2611,8 @@ void qmp_block_set_io_throttle(BlockIOThrottle *arg, Error **errp) goto out; } -throttle_config_init(&cfg); -cfg.buckets[THROTTLE_BPS_TOTAL].avg = arg->bps; -cfg.buckets[THROTTLE_BPS_READ].avg = arg->bps_rd; -cfg.buckets[THROTTLE_BPS_WRITE].avg = arg->bps_wr; - -cfg.buckets[THROTTLE_OPS_TOTAL].avg = arg->iops; -cfg.buckets[THROTTLE_OPS_READ].avg = arg->iops_rd; -cfg.buckets[THROTTLE_OPS_WRITE].avg = arg->iops_wr; - -if (arg->has_bps_max) { -cfg.buckets[THROTTLE_BPS_TOTAL].max = arg->bps_max; -} -if (arg->has_bps_rd_max) { -cfg.buckets[THROTTLE_BPS_READ].max = arg->bps_rd_max; -} -if (arg->has_bps_wr_max) { -cfg.buckets[THROTTLE_BPS_WRITE].max = arg->bps_wr_max; -} -if (arg->has_iops_max) { -cfg.buckets[THROTTLE_OPS_TOTAL].max = arg->iops_max; -} -if (arg->has_iops_rd_max) { -cfg.buckets[THROTTLE_OPS_READ].max = arg->iops_rd_max; -} -if (arg->has_iops_wr_max) { -cfg.buckets[THROTTLE_OPS_WRITE].max = arg->iops_wr_max; -} - -if (arg->has_bps_max_length) { -cfg.buckets[THROTTLE_BPS_TOTAL].burst_length = arg->bps_max_length; -} -if (arg->has_bps_rd_max_length) { -cfg.buckets[THROTTLE_BPS_READ].burst_length = arg->bps_rd_max_length; -} -if (arg->has_bps_wr_max_length) { -cfg.buckets[THROTTLE_BPS_WRITE].burst_length = arg->bps_wr_max_length; -} -if (arg->has_iops_max_length) { -cfg.buckets[THROTTLE_OPS_TOTAL].burst_length = arg->iops_max_length; -} -if (arg->has_iops_rd_max_length) { -cfg.buckets[THROTTLE_OPS_READ].burst_length = arg->iops_rd_max_length; -} -if (arg->has_iops_wr_max_length) { -cfg.buckets[THROTTLE_OPS_WRITE].burst_length = arg->iops_wr_max_length; -} - -if (arg->has_iops_size) { -cfg.op_size = arg->iops_size; -} +iothrottle = qapi_BlockIOThrottle_base(arg); +throttle_set_io_limits(&cfg, iothrottle); if (!throttle_is_valid(&cfg, errp)) { goto out; diff --git a/hmp.c b/hmp.c index 8c72c58..220d301 100644 --- a/hmp.c +++ b/hmp.c @@ -1749,20 +1749,29 @@ void hmp_change(Monitor *mon, const QDict *qdict) hmp_handle_error(mon, &err); } +static void hmp_initialize_io_throttle(IOThrottle *iot, const QDict *qdict) +{ +iot->has_id = true; +iot->id = (char *) qdict_get_str(qdict, "id"); +iot->bps = qdict_get_int(qdict, "bps"); +iot->bps_rd = qdict_get_int(qdict, "bps_rd"); +iot->bps_wr = qdict_get_int(qdict, "bps_wr"); +iot->iops = qdict_get_int(qdict, "iops"); +iot->iops_rd = qdict_get_int(qdict, "iops_rd"); +iot->iops_wr = qdict_get_int(qdict, "iops_wr"); +} + void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict) { Error *err = NULL; +IOThrottle *iothrottle; BlockIOThrottle throttle = { .has_device = true, .device = (char *) qdict_get_str(qdict, "device&quo
Re: [Qemu-devel] [PATCH v2 1/4] Throttle: Create IOThrottle structure
On 4/12/2017 5:01 PM, Alberto Garcia wrote: On Thu 30 Mar 2017 02:10:10 PM CEST, Pradeep Jagadeesh wrote: +## +# == QAPI IOThrottle definitions +## +# @IOThrottle: +# +# A set of parameters describing block +# "describing block ..." ? There's missing text here. Hmm..I will fix this. Regards, Pradeep Berto
Re: [Qemu-devel] [PATCH v4 4/4] fsdev: QMP interface for throttling
On 6/8/2017 6:49 PM, Greg Kurz wrote: On Thu, 18 May 2017 15:30:06 +0200 Pradeep Jagadeesh wrote: On 5/17/2017 7:09 PM, Eric Blake wrote: On 05/17/2017 11:29 AM, Greg Kurz wrote: First point: is fsdev a Linux-only feature, or can it be compiled on BSD? If it is Linux-only, then compiling a stub for Windows will still leave BSD broken, and your #ifdef is wrong. Fixing compilation on mingw is nice, but not the only platform to worry about. fsdev compilation currently depends on CONFIG_VIRTFS which is a Linux-only feature for the moment. There was a tentative to support it on Windows hosts two years ago but it stayed at the RFC stage. But even on Linux hosts, the current fsdev implementation also depends on the target supporting PCI and VIRTIO. We have a fsdev/qemu-fsdev-dummy.c file to put stubs so that we don't pull all the code for such targets. Maybe this could be reused for the above stubs as well ? That helps. The stub should live in qemu-fsdev-dummy.c (where it shouldn't need any #ifdef, because that file is only compiled when the condition is false), and... Second point: if fsdev is indeed a platform-specific feature, then we don't want to advertise the QMP commands that are useless when running on a platform that doesn't support it. Anywhere you have to add a stub for compilation means you ALSO need to patch monitor.c to unregister the command from being advertised as a valid QMP command. (If you used #ifdef __LINUX__ to guard the working version, and #ifndef __LINUX__ to declare the stub, then the monitor.c needs an #ifndef section within qmp_unregister_commands_hack() to tell QMP to not advertise the stubs.) monitor.c should wrap the unregister under #ifndef CONFIG_VIRTFS (rather than a particular platform name). I did unregister the functions as mentioned above in monitor.c. But it does not address the issue when I run "make docker-test-mingw@fedora". What issue is this ? I cannot even run this with your unmodified series (please note that I always build out-of-tree). The issue is when I cross build the qemu, I was facing the compilation issue. Even it failed in patchew. So, I had to add the dummy functions in monitor.c and qmp.c. -Pradeep I think the VIRTFS is still enabled. Only having dummy functions even in qmp.c addresses the issue. Regards, Pradeep
Re: [Qemu-devel] [PATCH v4 3/4] qmp: refactor duplicate code
On 6/8/2017 4:20 PM, Greg Kurz wrote: On Fri, 12 May 2017 10:03:02 +0200 Pradeep Jagadeesh wrote: On 5/11/2017 9:50 AM, Greg Kurz wrote: On Wed, 10 May 2017 04:41:22 -0400 Pradeep Jagadeesh wrote: This patch factor out the duplicate qmp throttle interface code that was present in both block and fsdev device files. There isn't any duplicated code at this stage since you have changed the patch ordering. Also, this patch is factoring out several things: - factoring code out of qmp_block_set_io_throttle() - factoring code out of hmp_block_set_io_throttle() So I'm not sure the patch title is quite right either. This seems to be the same in patch 4: it only mentions QMP even though it adds QMP and HMP interfaces. Maybe you should change this part of the series. A patch to introduce the full QMP interface, including the associated code refactoring (whose need would then be obvious). And a similar patch for the HMP interface. But without hmp functionalities the qmp does not work. So, I just kept both together. This sounds weird... I would rather expect HMP to depend on QMP, not the contrary. Anyway, looking at this patch I still see two different things which deserve their own patch and appropriate description. OK, I can create different patches. Regards, Pradeep Signed-off-by: Pradeep Jagadeesh --- blockdev.c | 53 +++- hmp.c | 21 +++- include/qemu/throttle-options.h | 2 ++ util/throttle-options.c | 54 + 4 files changed, 74 insertions(+), 56 deletions(-) diff --git a/blockdev.c b/blockdev.c index a0eb2ed..a55f6be 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2593,6 +2593,7 @@ void qmp_block_set_io_throttle(BlockIOThrottle *arg, Error **errp) BlockDriverState *bs; BlockBackend *blk; AioContext *aio_context; +IOThrottle *iothrottle; blk = qmp_get_blk(arg->has_device ? arg->device : NULL, arg->has_id ? arg->id : NULL, @@ -2610,56 +2611,8 @@ void qmp_block_set_io_throttle(BlockIOThrottle *arg, Error **errp) goto out; } -throttle_config_init(&cfg); -cfg.buckets[THROTTLE_BPS_TOTAL].avg = arg->bps; -cfg.buckets[THROTTLE_BPS_READ].avg = arg->bps_rd; -cfg.buckets[THROTTLE_BPS_WRITE].avg = arg->bps_wr; - -cfg.buckets[THROTTLE_OPS_TOTAL].avg = arg->iops; -cfg.buckets[THROTTLE_OPS_READ].avg = arg->iops_rd; -cfg.buckets[THROTTLE_OPS_WRITE].avg = arg->iops_wr; - -if (arg->has_bps_max) { -cfg.buckets[THROTTLE_BPS_TOTAL].max = arg->bps_max; -} -if (arg->has_bps_rd_max) { -cfg.buckets[THROTTLE_BPS_READ].max = arg->bps_rd_max; -} -if (arg->has_bps_wr_max) { -cfg.buckets[THROTTLE_BPS_WRITE].max = arg->bps_wr_max; -} -if (arg->has_iops_max) { -cfg.buckets[THROTTLE_OPS_TOTAL].max = arg->iops_max; -} -if (arg->has_iops_rd_max) { -cfg.buckets[THROTTLE_OPS_READ].max = arg->iops_rd_max; -} -if (arg->has_iops_wr_max) { -cfg.buckets[THROTTLE_OPS_WRITE].max = arg->iops_wr_max; -} - -if (arg->has_bps_max_length) { -cfg.buckets[THROTTLE_BPS_TOTAL].burst_length = arg->bps_max_length; -} -if (arg->has_bps_rd_max_length) { -cfg.buckets[THROTTLE_BPS_READ].burst_length = arg->bps_rd_max_length; -} -if (arg->has_bps_wr_max_length) { -cfg.buckets[THROTTLE_BPS_WRITE].burst_length = arg->bps_wr_max_length; -} -if (arg->has_iops_max_length) { -cfg.buckets[THROTTLE_OPS_TOTAL].burst_length = arg->iops_max_length; -} -if (arg->has_iops_rd_max_length) { -cfg.buckets[THROTTLE_OPS_READ].burst_length = arg->iops_rd_max_length; -} -if (arg->has_iops_wr_max_length) { -cfg.buckets[THROTTLE_OPS_WRITE].burst_length = arg->iops_wr_max_length; -} - -if (arg->has_iops_size) { -cfg.op_size = arg->iops_size; -} +iothrottle = qapi_BlockIOThrottle_base(arg); +qmp_set_io_throttle(&cfg, iothrottle); if (!throttle_is_valid(&cfg, errp)) { goto out; diff --git a/hmp.c b/hmp.c index ab407d6..9660373 100644 --- a/hmp.c +++ b/hmp.c @@ -1552,20 +1552,29 @@ void hmp_change(Monitor *mon, const QDict *qdict) hmp_handle_error(mon, &err); } +static void hmp_initialize_io_throttle(IOThrottle *iot, const QDict *qdict) +{ +iot->has_id = true; +iot->id = (char *) qdict_get_str(qdict, "id"); +iot->bps = qdict_get_int(qdict, "bps"); +iot->bps_rd = qdict_get_int(qdict, "bps_rd"); +iot->bps_wr = qdict_get_int(qdict, "bps_wr"); +iot->iops = qdict_get_int(qdict, "iops"); +iot->iops_rd = qdict_get_int(qdict, "iops_rd"); +iot->iops_wr = qdi
Re: [Qemu-devel] [PATCH v4 4/4] fsdev: QMP interface for throttling
On 6/9/2017 11:07 AM, Greg Kurz wrote: GEN qmp-commands.h /tmp/qemu-test/src/qapi-schema.json:85: No such file or directory: qapi/fsdev.json Makefile:438: recipe for target 'qmp-commands.h' failed Please fix that. Ho ok, I will have a look. How are you trying to compile, using "make docker" options? Thanks Pradeep
Re: [Qemu-devel] [PATCH 0/2 v14] fsdev: add IO throttle support to fsdev devices
On 2/1/2017 3:44 PM, Alberto Garcia wrote: On Tue 24 Jan 2017 10:24:06 AM CET, Pradeep Jagadeesh wrote: Pradeep Jagadeesh (2): fsdev: add IO throttle support to fsdev devices throttle: removed duplicate throtlle code from block and fsdev files I cannot find the second patch in this thread (v14), did you forget to send it? https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg04913.html Sorry for the late reply. I did send all the both patch files and the cover letter. Don't know how the patch missed in that link. I have received independent e-mails for each patch, with version 14. Am I missing something here? Regards, Pradeep Berto
Re: [Qemu-devel] [PATCH 1/2 v14] fsdev: add IO throttle support to fsdev devices
On 2/1/2017 4:08 PM, Alberto Garcia wrote: Hello, and sorry for being late reviewing these patches :) On Tue 24 Jan 2017 10:24:07 AM CET, Pradeep Jagadeesh wrote: This patchset adds the io throttle support for the 9p-local driver. For now this functionality can be used only through qemu cli options. QMP interface and support to other 9p drivers need further extensions. To make it simple for other 9p drivers, the throttle code has been put in separate files. Compared to the previous patch that I reviewed, you removed the fsdev_throttle_cleanup() call from v9fs_device_unrealize_common(). Why did you do that? I actually missed while merging the code to newer version. Instead of putting inside the unrealize it went some other function. I miss the list of changes in the cover letter --- a/hw/9pfs/9p.c +++ b/hw/9pfs/9p.c @@ -3520,6 +3520,10 @@ int v9fs_device_realize_common(V9fsState *s, Error **errp) error_setg(errp, "share path %s is not a directory", fse->path); goto out; } + +s->ctx.fst = &fse->fst; +fsdev_throttle_init(s->ctx.fst); + v9fs_path_free(&path); rc = 0; @@ -3528,6 +3532,7 @@ out: if (s->ops && s->ops->cleanup && s->ctx.private) { s->ops->cleanup(&s->ctx); } +fsdev_throttle_cleanup(s->ctx.fst); g_free(s->tag); g_free(s->ctx.fs_root); v9fs_path_free(&path); You also added the fsdev_throttle_cleanup() here, but look at the code: fsdev_throttle_init(s->ctx.fst); v9fs_path_free(&path); rc = 0; out: if (rc) { /* ... */ fsdev_throttle_cleanup(s->ctx.fst); /* ... */ } If rc != 0 you don't need to do the clean up, because there was never an initialization. Fixed it, it was because of merging issue as I answered above. -Pradeep Berto
[Qemu-devel] [PATCH 0/2 v15] fsdev: add IO throttle support to fsdev devices
This patch set adds the IO throttling functionality to fsdev/9p devices. So far cgroups were used for throttling IO opertions on the fsdev/9p devices. It is difficult to use cgroups for throttling because we have to set up cgroups externally before we start the qemu process. Qemu provides the throttling apis for implementing the throttling. Block devices already make use of these APIs for throtting the IO operations. So, we use the same APIs to enable the throttling functionality for fsdevices.As of now the feature is enabled only on 9p-local driver. This feature can be used as shown in the below example: -fsdev local,id=sdb1,path=PATH_TO_DEVICE,security_model=none,writeout=immediate, throttling.bps-read=4194304,throttling.bps-write=4194304 -device virtio-9p-pci,fsdev=sdb1,mount_tag=sdb1 The main advantages are: - Easy to use because the throttling options are part of qemu cli options - Provides a uniform way of using throttling options across block and fsdev/9p devices - No need to setup cgroup to provide throttling functionality for the fsdev devices. - Removes the redundant throttling code that was present in block and fsdev files Missing features: -QMP support -Throttling support for other fsdev/9p drivers. Thanks, Pradeep Pradeep Jagadeesh (2): fsdev: add IO throttle support to fsdev devices throttle: removed duplicate throtlle code from block and fsdev files blockdev.c | 81 ++- fsdev/Makefile.objs | 2 +- fsdev/file-op-9p.h | 3 + fsdev/qemu-fsdev-opts.c | 3 + fsdev/qemu-fsdev-throttle.c | 118 fsdev/qemu-fsdev-throttle.h | 39 + hw/9pfs/9p-local.c | 8 +++ hw/9pfs/9p.c| 5 ++ hw/9pfs/cofile.c| 2 + include/qemu/throttle-options.h | 92 +++ 10 files changed, 275 insertions(+), 78 deletions(-) create mode 100644 fsdev/qemu-fsdev-throttle.c create mode 100644 fsdev/qemu-fsdev-throttle.h create mode 100644 include/qemu/throttle-options.h -- 1.8.3.1
[Qemu-devel] [PATCH 2/2 v15] throttle: factor out duplicate code
This patch removes the redundant throttle code that was present in block and fsdev device files. Now the common code is moved to a single file. Signed-off-by: Pradeep Jagadeesh https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg04637.html --- blockdev.c | 81 ++-- fsdev/qemu-fsdev-opts.c | 80 ++- hw/9pfs/9p.c| 2 +- include/qemu/throttle-options.h | 92 + 4 files changed, 101 insertions(+), 154 deletions(-) create mode 100644 include/qemu/throttle-options.h diff --git a/blockdev.c b/blockdev.c index 245e1e1..9320c8a 100644 --- a/blockdev.c +++ b/blockdev.c @@ -52,6 +52,7 @@ #include "sysemu/arch_init.h" #include "qemu/cutils.h" #include "qemu/help_option.h" +#include "qemu/throttle-options.h" static QTAILQ_HEAD(, BlockDriverState) monitor_bdrv_states = QTAILQ_HEAD_INITIALIZER(monitor_bdrv_states); @@ -3999,83 +4000,9 @@ QemuOptsList qemu_common_drive_opts = { .name = BDRV_OPT_READ_ONLY, .type = QEMU_OPT_BOOL, .help = "open drive file as read-only", -},{ -.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", -},{ +}, +THROTTLE_OPTS, + { .name = "throttling.group", .type = QEMU_OPT_STRING, .help = "name of the block throttling group", diff --git a/fsdev/qemu-fsdev-opts.c b/fsdev/qemu-fsdev-opts.c index 385423f0..b
[Qemu-devel] [PATCH 1/2 v15] fsdev: add IO throttle support to fsdev devices
This patchset adds the io throttle support for the 9p-local driver. For now this functionality can be used only through qemu cli options. QMP interface and support to other 9p drivers need further extensions. To make it simple for other 9p drivers, the throttle code has been put in separate files. Signed-off-by: Pradeep Jagadeesh --- fsdev/Makefile.objs | 2 +- fsdev/file-op-9p.h | 3 ++ fsdev/qemu-fsdev-opts.c | 77 - fsdev/qemu-fsdev-throttle.c | 118 fsdev/qemu-fsdev-throttle.h | 39 +++ hw/9pfs/9p-local.c | 8 +++ hw/9pfs/9p.c| 5 ++ hw/9pfs/cofile.c| 2 + 8 files changed, 252 insertions(+), 2 deletions(-) create mode 100644 fsdev/qemu-fsdev-throttle.c create mode 100644 fsdev/qemu-fsdev-throttle.h diff --git a/fsdev/Makefile.objs b/fsdev/Makefile.objs index 1b120a4..659df6e 100644 --- a/fsdev/Makefile.objs +++ b/fsdev/Makefile.objs @@ -5,7 +5,7 @@ common-obj-y = qemu-fsdev.o 9p-marshal.o 9p-iov-marshal.o else common-obj-y = qemu-fsdev-dummy.o endif -common-obj-y += qemu-fsdev-opts.o +common-obj-y += qemu-fsdev-opts.o qemu-fsdev-throttle.o # Toplevel always builds this; targets without virtio will put it in # common-obj-y diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h index a56dc84..0844a40 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_BITS0600 #define SM_LOCAL_DIR_MODE_BITS0700 @@ -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..385423f0 100644 --- a/fsdev/qemu-fsdev-opts.c +++ b/fsdev/qemu-fsdev-opts.c @@ -37,8 +37,83 @@ 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 secon
Re: [Qemu-devel] [PATCH 1/2 v15] fsdev: add IO throttle support to fsdev devices
On 2/3/2017 11:10 AM, Alberto Garcia wrote: On Fri 03 Feb 2017 11:04:57 AM CET, Pradeep Jagadeesh wrote: This patchset adds the io throttle support for the 9p-local driver. For now this functionality can be used only through qemu cli options. QMP interface and support to other 9p drivers need further extensions. To make it simple for other 9p drivers, the throttle code has been put in separate files. Signed-off-by: Pradeep Jagadeesh I don't see any difference between this patch and the one in v14... I moved the fsdev_throttle_cleanup() inside the unrealize() function You want me to clean up even here ? out: if (rc) { /* ... */ fsdev_throttle_cleanup(s->ctx.fst); /* ... */ } I thought one place is enough. Or I did not understand your comment :) -Pradeep + +s->ctx.fst = &fse->fst; +fsdev_throttle_init(s->ctx.fst); + v9fs_path_free(&path); rc = 0; @@ -3528,6 +3532,7 @@ out: if (s->ops && s->ops->cleanup && s->ctx.private) { s->ops->cleanup(&s->ctx); } +fsdev_throttle_cleanup(s->ctx.fst); g_free(s->tag); g_free(s->ctx.fs_root); v9fs_path_free(&path); Berto
[Qemu-devel] [PATCH 0/2 v16] fsdev: add IO throttle support to fsdev devices
This patch set adds the IO throttling functionality to fsdev/9p devices. So far cgroups were used for throttling IO opertions on the fsdev/9p devices. It is difficult to use cgroups for throttling because we have to set up cgroups externally before we start the qemu process. Qemu provides the throttling apis for implementing the throttling. Block devices already make use of these APIs for throtting the IO operations. So, we use the same APIs to enable the throttling functionality for fsdevices.As of now the feature is enabled only on 9p-local driver. This feature can be used as shown in the below example: -fsdev local,id=sdb1,path=PATH_TO_DEVICE,security_model=none,writeout=immediate, throttling.bps-read=4194304,throttling.bps-write=4194304 -device virtio-9p-pci,fsdev=sdb1,mount_tag=sdb1 The main advantages are: - Easy to use because the throttling options are part of qemu cli options - Provides a uniform way of using throttling options across block and fsdev/9p devices - No need to setup cgroup to provide throttling functionality for the fsdev devices. - Removes the redundant throttling code that was present in block and fsdev files Missing features: -QMP support -Throttling support for other fsdev/9p drivers. Thanks, Pradeep Pradeep Jagadeesh (2): fsdev: add IO throttle support to fsdev devices throttle: factor out duplicate code blockdev.c | 81 ++- fsdev/Makefile.objs | 2 +- fsdev/file-op-9p.h | 3 + fsdev/qemu-fsdev-opts.c | 3 + fsdev/qemu-fsdev-throttle.c | 118 fsdev/qemu-fsdev-throttle.h | 39 + hw/9pfs/9p-local.c | 8 +++ hw/9pfs/9p.c| 5 ++ hw/9pfs/cofile.c| 2 + include/qemu/throttle-options.h | 92 +++ 10 files changed, 275 insertions(+), 78 deletions(-) create mode 100644 fsdev/qemu-fsdev-throttle.c create mode 100644 fsdev/qemu-fsdev-throttle.h create mode 100644 include/qemu/throttle-options.h -- 1.8.3.1
[Qemu-devel] [PATCH 2/2 v16] throttle: factor out duplicate code
This patch removes the redundant throttle code that was present in block and fsdev device files. Now the common code is moved to a single file. Signed-off-by: Pradeep Jagadeesh https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg04637.html --- blockdev.c | 81 ++-- fsdev/qemu-fsdev-opts.c | 80 ++- include/qemu/throttle-options.h | 92 + 3 files changed, 100 insertions(+), 153 deletions(-) create mode 100644 include/qemu/throttle-options.h diff --git a/blockdev.c b/blockdev.c index 245e1e1..9320c8a 100644 --- a/blockdev.c +++ b/blockdev.c @@ -52,6 +52,7 @@ #include "sysemu/arch_init.h" #include "qemu/cutils.h" #include "qemu/help_option.h" +#include "qemu/throttle-options.h" static QTAILQ_HEAD(, BlockDriverState) monitor_bdrv_states = QTAILQ_HEAD_INITIALIZER(monitor_bdrv_states); @@ -3999,83 +4000,9 @@ QemuOptsList qemu_common_drive_opts = { .name = BDRV_OPT_READ_ONLY, .type = QEMU_OPT_BOOL, .help = "open drive file as read-only", -},{ -.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", -},{ +}, +THROTTLE_OPTS, + { .name = "throttling.group", .type = QEMU_OPT_STRING, .help = "name of the block throttling group", diff --git a/fsdev/qemu-fsdev-opts.c b/fsdev/qemu-fsdev-opts.c index 385423f0..bf57130 100644 --- a/fsdev/qemu-fsdev-opts.c ++
[Qemu-devel] [PATCH 1/2 v16] fsdev: add IO throttle support to fsdev devices
This patchset adds the throttle 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 9p drivers, the throttle code has been put in separate files. Signed-off-by: Pradeep Jagadeesh --- fsdev/Makefile.objs | 2 +- fsdev/file-op-9p.h | 3 ++ fsdev/qemu-fsdev-opts.c | 77 - fsdev/qemu-fsdev-throttle.c | 118 fsdev/qemu-fsdev-throttle.h | 39 +++ hw/9pfs/9p-local.c | 8 +++ hw/9pfs/9p.c| 5 ++ hw/9pfs/cofile.c| 2 + 8 files changed, 252 insertions(+), 2 deletions(-) create mode 100644 fsdev/qemu-fsdev-throttle.c create mode 100644 fsdev/qemu-fsdev-throttle.h diff --git a/fsdev/Makefile.objs b/fsdev/Makefile.objs index 1b120a4..659df6e 100644 --- a/fsdev/Makefile.objs +++ b/fsdev/Makefile.objs @@ -5,7 +5,7 @@ common-obj-y = qemu-fsdev.o 9p-marshal.o 9p-iov-marshal.o else common-obj-y = qemu-fsdev-dummy.o endif -common-obj-y += qemu-fsdev-opts.o +common-obj-y += qemu-fsdev-opts.o qemu-fsdev-throttle.o # Toplevel always builds this; targets without virtio will put it in # common-obj-y diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h index a56dc84..0844a40 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_BITS0600 #define SM_LOCAL_DIR_MODE_BITS0700 @@ -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..385423f0 100644 --- a/fsdev/qemu-fsdev-opts.c +++ b/fsdev/qemu-fsdev-opts.c @@ -37,8 +37,83 @@ 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",
Re: [Qemu-devel] [PATCH 2/2 v15] throttle: factor out duplicate code
On 2/3/2017 11:38 AM, Alberto Garcia wrote: On Fri 03 Feb 2017 11:04:58 AM CET, Pradeep Jagadeesh wrote: This patch removes the redundant throttle code that was present in block and fsdev device files. Now the common code is moved to a single file. Here it says that this patch moves common code to a separate file, however: Hmm, sorry. I just messed it up while merging. Regards, Pradeep diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index 920eb05..22a6a99 100644 --- a/hw/9pfs/9p.c +++ b/hw/9pfs/9p.c @@ -3532,7 +3532,6 @@ out: if (s->ops && s->ops->cleanup && s->ctx.private) { s->ops->cleanup(&s->ctx); } -fsdev_throttle_cleanup(s->ctx.fst); g_free(s->tag); g_free(s->ctx.fs_root); v9fs_path_free(&path); @@ -3545,6 +3544,7 @@ void v9fs_device_unrealize_common(V9fsState *s, Error **errp) if (s->ops->cleanup) { s->ops->cleanup(&s->ctx); } +fsdev_throttle_cleanup(s->ctx.fst); g_free(s->tag); g_free(s->ctx.fs_root); } This doesn't belong here. In patch 1 you add a fsdev_throttle_cleanup() in v9fs_device_realize_common(), and in patch 2 you remove it from there and put it in v9fs_device_unrealize_common(). It looks like you wanted to fix patch 1 but added the fix to patch 2 instead. Berto
Re: [Qemu-devel] [PATCH 2/2 v16] throttle: factor out duplicate code
On 2/3/2017 1:22 PM, Alberto Garcia wrote: On Fri 03 Feb 2017 12:57:23 PM CET, Pradeep Jagadeesh wrote: This patch removes the redundant throttle code that was present in block and fsdev device files. Now the common code is moved to a single file. Signed-off-by: Pradeep Jagadeesh Reviewed-by: Alberto Garcia https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg04637.html This line doesn't belong in the commit message. There is one false positive error in the second patch. So Greg asked me to put this line after SoB. Just to say that I had a positive review comment. Regards. Pradeep Berto
Re: [Qemu-devel] [PATCH 2/2 v16] throttle: factor out duplicate code
On 2/3/2017 1:44 PM, Alberto Garcia wrote: On Fri 03 Feb 2017 01:27:25 PM CET, Pradeep Jagadeesh wrote: Signed-off-by: Pradeep Jagadeesh Reviewed-by: Alberto Garcia https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg04637.html This line doesn't belong in the commit message. There is one false positive error in the second patch. So Greg asked me to put this line after SoB. Just to say that I had a positive review comment. The false positive is not related to what Greg said :-) What you have to put is not that URL but the Reviewed-by: line with Stefan's name and e-mail. Haha OK. I will change it and send new patch set again. -Pradeep Berto
Re: [Qemu-devel] [PATCH 2/2 v16] throttle: factor out duplicate code
On 2/3/2017 1:53 PM, Alberto Garcia wrote: On Fri 03 Feb 2017 01:46:31 PM CET, Pradeep Jagadeesh wrote: The false positive is not related to what Greg said :-) What you have to put is not that URL but the Reviewed-by: line with Stefan's name and e-mail. Haha OK. I will change it and send new patch set again. I don't know if it's necessary now, but if you need to send the patch again you have to keep your Signed-off-by line and my and Stefan's Reviewed-by lines. OK sure!. If it is required, then I will add Reviewed by Line. Regards, Pradeep Berto
Re: [Qemu-devel] [PATCH 2/2 v16] throttle: factor out duplicate code
On 2/6/2017 3:58 PM, Greg Kurz wrote: On Fri, 3 Feb 2017 06:57:23 -0500 Pradeep Jagadeesh wrote: This patch removes the redundant throttle code that was present in block and fsdev device files. Now the common code is moved to a single file. Signed-off-by: Pradeep Jagadeesh https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg04637.html --- blockdev.c | 81 ++-- fsdev/qemu-fsdev-opts.c | 80 ++- include/qemu/throttle-options.h | 92 + 3 files changed, 100 insertions(+), 153 deletions(-) create mode 100644 include/qemu/throttle-options.h diff --git a/blockdev.c b/blockdev.c index 245e1e1..9320c8a 100644 --- a/blockdev.c +++ b/blockdev.c @@ -52,6 +52,7 @@ #include "sysemu/arch_init.h" #include "qemu/cutils.h" #include "qemu/help_option.h" +#include "qemu/throttle-options.h" static QTAILQ_HEAD(, BlockDriverState) monitor_bdrv_states = QTAILQ_HEAD_INITIALIZER(monitor_bdrv_states); @@ -3999,83 +4000,9 @@ QemuOptsList qemu_common_drive_opts = { .name = BDRV_OPT_READ_ONLY, .type = QEMU_OPT_BOOL, .help = "open drive file as read-only", -},{ -.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", -},{ +}, + THROTTLE_OPTS, + { Indent nit ^^. Also this could be surrounded by some empty lines as you did in fsdev/qemu-fsdev-opts.c below. I wouldn't ask you to send a v17 for this though and I've updated
Re: [Qemu-devel] [PATCH 1/2 v16] fsdev: add IO throttle support to fsdev devices
On 2/6/2017 8:36 PM, Eric Blake wrote: On 02/03/2017 05:57 AM, Pradeep Jagadeesh wrote: This patchset adds the throttle 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. This part is a bit scary - if 2.9 is released with just the cli option and not the QMP interface, then how does someone like libvirt introspect whether the feature is available for use? I do have plans to extend the qmp interface for this feature. Already started looking into it.Do not know, I can push it for 2.9. Because I am busy with some other work. Please make sure we don't reach 2.9 with only a half-baked feature; whether that means finishing the QMP work or temporarily disabling the cli additions until a later release can finish the work.
Re: [Qemu-devel] [PATCH v3 2/4] fsdev: QMP interface for throttling
On 5/4/2017 5:19 PM, Eric Blake wrote: On 05/04/2017 10:12 AM, Pradeep Jagadeesh wrote: +IOThrottle throttle = { +.has_id = true, +.id = (char *) qdict_get_str(qdict, "id"), +.bps = qdict_get_int(qdict, "bps"), +.bps_rd = qdict_get_int(qdict, "bps_rd"), +.bps_wr = qdict_get_int(qdict, "bps_wr"), +.iops = qdict_get_int(qdict, "iops"), +.iops_rd = qdict_get_int(qdict, "iops_rd"), +.iops_wr = qdict_get_int(qdict, "iops_wr"), Again, don't you need to be setting .has_bps=true and so on? Same as above. I have not set any max burst values here, because I wanted to keep it in line with the block device. May be there is a room to enable these max values in both in future. I don't think you were getting the point of my question. Since 'bps' is an optional member of struct IOThrottle, you have to set 'has_bps' at the same time to make it obvious that 'bps' should affect things. I need some more clarifications here. I did not understand what do you mean by an optional parameter? You mean I need to set "has_*" for all the parameters? Yes, any optional parameter (one named '*foo' in the .json file) has a counterpart has_foo boolean, which should be set to true when the parameter is in use (and thus making it obvious when it is set to false that it is not in use). OK, but here the id is only with * (Optional), others are not. Still need them to set? I understand that you aren't setting 'bps_max', nor it's counterpart 'has_bps_max', and that's not what I was asking about. +++ b/qapi/iothrottle.json @@ -3,6 +3,7 @@ ## # == QAPI IOThrottle definitions ## +## # @IOThrottle: This looks like a spurious change You mean the below sentence is not required? No, the above addition of a second ## is not required. If I remove that I will get the compilation error. I think the parser needs that. If the parser chokes without it, then that implies patch 1 is broken, and the fix should be squashed there for full bisectability. Either way, it looks like a spurious change when it is occurring in patch 3. Yes, I reordered the patches and fixed this issue. -Pradeep
[Qemu-devel] [PATCH v4 3/4] qmp: refactor duplicate code
This patch factor out the duplicate qmp throttle interface code that was present in both block and fsdev device files. Signed-off-by: Pradeep Jagadeesh --- blockdev.c | 53 +++- hmp.c | 21 +++- include/qemu/throttle-options.h | 2 ++ util/throttle-options.c | 54 + 4 files changed, 74 insertions(+), 56 deletions(-) diff --git a/blockdev.c b/blockdev.c index a0eb2ed..a55f6be 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2593,6 +2593,7 @@ void qmp_block_set_io_throttle(BlockIOThrottle *arg, Error **errp) BlockDriverState *bs; BlockBackend *blk; AioContext *aio_context; +IOThrottle *iothrottle; blk = qmp_get_blk(arg->has_device ? arg->device : NULL, arg->has_id ? arg->id : NULL, @@ -2610,56 +2611,8 @@ void qmp_block_set_io_throttle(BlockIOThrottle *arg, Error **errp) goto out; } -throttle_config_init(&cfg); -cfg.buckets[THROTTLE_BPS_TOTAL].avg = arg->bps; -cfg.buckets[THROTTLE_BPS_READ].avg = arg->bps_rd; -cfg.buckets[THROTTLE_BPS_WRITE].avg = arg->bps_wr; - -cfg.buckets[THROTTLE_OPS_TOTAL].avg = arg->iops; -cfg.buckets[THROTTLE_OPS_READ].avg = arg->iops_rd; -cfg.buckets[THROTTLE_OPS_WRITE].avg = arg->iops_wr; - -if (arg->has_bps_max) { -cfg.buckets[THROTTLE_BPS_TOTAL].max = arg->bps_max; -} -if (arg->has_bps_rd_max) { -cfg.buckets[THROTTLE_BPS_READ].max = arg->bps_rd_max; -} -if (arg->has_bps_wr_max) { -cfg.buckets[THROTTLE_BPS_WRITE].max = arg->bps_wr_max; -} -if (arg->has_iops_max) { -cfg.buckets[THROTTLE_OPS_TOTAL].max = arg->iops_max; -} -if (arg->has_iops_rd_max) { -cfg.buckets[THROTTLE_OPS_READ].max = arg->iops_rd_max; -} -if (arg->has_iops_wr_max) { -cfg.buckets[THROTTLE_OPS_WRITE].max = arg->iops_wr_max; -} - -if (arg->has_bps_max_length) { -cfg.buckets[THROTTLE_BPS_TOTAL].burst_length = arg->bps_max_length; -} -if (arg->has_bps_rd_max_length) { -cfg.buckets[THROTTLE_BPS_READ].burst_length = arg->bps_rd_max_length; -} -if (arg->has_bps_wr_max_length) { -cfg.buckets[THROTTLE_BPS_WRITE].burst_length = arg->bps_wr_max_length; -} -if (arg->has_iops_max_length) { -cfg.buckets[THROTTLE_OPS_TOTAL].burst_length = arg->iops_max_length; -} -if (arg->has_iops_rd_max_length) { -cfg.buckets[THROTTLE_OPS_READ].burst_length = arg->iops_rd_max_length; -} -if (arg->has_iops_wr_max_length) { -cfg.buckets[THROTTLE_OPS_WRITE].burst_length = arg->iops_wr_max_length; -} - -if (arg->has_iops_size) { -cfg.op_size = arg->iops_size; -} +iothrottle = qapi_BlockIOThrottle_base(arg); +qmp_set_io_throttle(&cfg, iothrottle); if (!throttle_is_valid(&cfg, errp)) { goto out; diff --git a/hmp.c b/hmp.c index ab407d6..9660373 100644 --- a/hmp.c +++ b/hmp.c @@ -1552,20 +1552,29 @@ void hmp_change(Monitor *mon, const QDict *qdict) hmp_handle_error(mon, &err); } +static void hmp_initialize_io_throttle(IOThrottle *iot, const QDict *qdict) +{ +iot->has_id = true; +iot->id = (char *) qdict_get_str(qdict, "id"); +iot->bps = qdict_get_int(qdict, "bps"); +iot->bps_rd = qdict_get_int(qdict, "bps_rd"); +iot->bps_wr = qdict_get_int(qdict, "bps_wr"); +iot->iops = qdict_get_int(qdict, "iops"); +iot->iops_rd = qdict_get_int(qdict, "iops_rd"); +iot->iops_wr = qdict_get_int(qdict, "iops_wr"); +} + void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict) { Error *err = NULL; +IOThrottle *iothrottle; BlockIOThrottle throttle = { .has_device = true, .device = (char *) qdict_get_str(qdict, "device"), -.bps = qdict_get_int(qdict, "bps"), -.bps_rd = qdict_get_int(qdict, "bps_rd"), -.bps_wr = qdict_get_int(qdict, "bps_wr"), -.iops = qdict_get_int(qdict, "iops"), -.iops_rd = qdict_get_int(qdict, "iops_rd"), -.iops_wr = qdict_get_int(qdict, "iops_wr"), }; +iothrottle = qapi_BlockIOThrottle_base(&throttle); +hmp_initialize_io_throttle(iothrottle, qdict); qmp_block_set_io_throttle(&throttle, &err); hmp_handle_error(mon, &err); } diff --git a/include/qemu/throttle-options.h b/include/qemu/throttle-options.h index 9b68eb8..ccf10cc 100644 --- a/include/qemu/throttle-options.h +++ b/include/qemu/throttle-options.h @@ -94,4 +94,6 @@ void parse_io_throttle_options(ThrottleConfig *, QemuOpts *); +void qmp_set_io_throttle(ThrottleConfig *, IOTh