On Thu, Apr 11, 2024 at 19:01:55 -0700, w...@linux.ibm.com wrote: > From: Chun Feng Wu <w...@linux.ibm.com> > > * Add new elements '<throttlegroups>' and '<throttlefilters>' > * <ThrottleGroups> contains <ThrottleGroup> defintions > * <ThrottleFilters> can include multiple throttlegroup references to form > filter chain in qemu > * Chained throttle filters feature in qemu is described at > https://github.com/qemu/qemu/blob/master/docs/throttle.txt > > Signed-off-by: Chun Feng Wu <w...@linux.ibm.com> > ---
Please separate the <throttlegroups> changes into a separate patch from throttlefilters. As I've noted previously you'll also need to add test XML files, ideally for qemuxmlconftest, which will apart from testing the commandline output test also the XML schema. > docs/formatdomain.rst | 48 +++++++++ > src/conf/schemas/domaincommon.rng | 164 +++++++++++++++++++++++++++++- > 2 files changed, 211 insertions(+), 1 deletion(-) > > diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst > index e2f66b982c..ee9ee8b10c 100644 > --- a/docs/formatdomain.rst > +++ b/docs/formatdomain.rst > @@ -1957,6 +1957,34 @@ advertisements to the guest OS. (NB: Only qemu driver > support) > the guest OS itself can choose to circumvent the unavailability of the > sleep > states (e.g. S4 by turning off completely). > > +Throttle Group Management > +------------------------- Please make it more obvious that this is for disk throughput shaping ... > + > +:since:`Since 10.3.0` it is possible to create multiple named throttle groups > +and then reference them within ``throttlefilters`` to form filter chain in > QEMU for > +specific disk. The limits(throttlegroups) are shared within domain, hence > the same group ... as the word 'disk' appears only here. > +can be referenced by different filters. > + > +:: > + > + <domain> > + ... > + <throttlegroups> > + <throttlegroup> > + <group_name>limit0</group_name> > + <total_bytes_sec>10000000</total_bytes_sec> > + <read_iops_sec>400000</read_iops_sec> > + <write_iops_sec>100000</write_iops_sec> > + </throttlegroup> > + </throttlegroups> > + ... > + </domain> > + > +All throttlegroups are listed within the ``throttlegroups`` element This feels redundndant. > + > +``throttlegroup`` > + It has the same sub-elements as ``iotune`` (See `Hard drives, floppy > disks, CDROMs`_), > + The difference is that <group_name> is required. I already forgot how this is formatted, but you'll need to make sure that the group name is formatted first as in the example. > > Hypervisor features > ------------------- > @@ -2704,6 +2732,15 @@ paravirtualized driver is specified via the ``disk`` > element. > <source dev='/dev/vhost-vdpa-0' /> > <target dev='vdg' bus='virtio'/> > </disk> > + <disk type='file' device='disk'> > + <driver name='qemu' type='qcow2' /> > + <source file='/var/lib/libvirt/images/disk.qcow2'/> > + <target dev='vdh' bus='virtio'/> > + <throttlefilters> > + <throttlefilter group='limit2'/> > + <throttlefilter group='limit012'/> > + </throttlefilters> > + </disk> > </devices> > ... > > @@ -3185,6 +3222,17 @@ paravirtualized driver is specified via the ``disk`` > element. > :since:`since after 0.4.4`; "sata" attribute value :since:`since 0.9.7`; > "removable" attribute value :since:`since 1.1.3`; > "rotation_rate" attribute value :since:`since 7.3.0` > +``throttlefilters`` > + The optional ``throttlefilters`` element provides the ability to provide > additional > + per-device throttle chain :since:`Since 10.3.0` > + For example, if we have four different disks and we want to limit I/O for > each one > + and we also want to limit combined I/O of all four disks, we can leverage > + ``throttlefilters`` to achieve this goal by setting two > ``throttlefilter`` for > + each disk: disk's own filter and combined filter. ``throttlefilters`` and > ``iotune`` > + should be used exclusively. Please also document how the group chaining is applied in terms of > + > + ``throttlefilter`` > + The optional ``throttlefilter`` element is to reference defined > throttle group. > ``iotune`` > The optional ``iotune`` element provides the ability to provide additional > per-device I/O tuning, with values that can vary for each device (contrast [...] > @@ -6637,6 +6641,164 @@ > </interleave> > </element> > </define> > + <!-- > + throttlegroup is similar to <iotune>, however, group_name is required, > which is different from <iotune> > + --> > + <define name="throttlegroup"> > + <element name="throttlegroup"> > + <interleave> > + <!-- required --> > + <element name="group_name"> > + <text/> > + </element> > + <choice> > + <element name="total_bytes_sec"> > + <data type="unsignedLong"/> > + </element> Preferrably all these definitions should be shared with <iotune> as any change would now need to change two places. It is okay if the schema is more lax (e.g. allowing 'group_name' to be optional in the schema, which is then strictly required by the parser), but it's not okay to have to change two distinct places. > + <group> > + <interleave> > + <optional> > + <element name="read_bytes_sec"> > + <data type="unsignedLong"/> > + </element>
