[PATCH v3 6/7] virsh: Add mode option to domdirtyrate-calc virsh api

[Michal Prívozník]

On 1/28/22 08:35, huangy81 at chinatelecom.cn wrote:
> From: Hyman Huang(黄勇) <huangy81 at chinatelecom.cn>
> 
> Extend domdirtyrate-calc virsh api with mode option, either
> of these three options "page-sampling,dirty-bitmap,dirty-ring"
> can be specified when calculating dirty page rate.
> 
> Signed-off-by: Hyman Huang(黄勇) <huangy81 at chinatelecom.cn>
> ---
>  docs/manpages/virsh.rst |  7 +++++--
>  src/libvirt-domain.c    | 12 +++++++++++-
>  src/qemu/qemu_driver.c  | 24 +++++++++++++++++++++++-
>  tools/virsh-domain.c    | 28 +++++++++++++++++++++++++++-
>  4 files changed, 66 insertions(+), 5 deletions(-)

These changes need to be broken into smaller patches. Modifying virsh is
one patch, qemu driver is the other.

> 
> diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst
> index e28927e..e09703c 100644
> --- a/docs/manpages/virsh.rst
> +++ b/docs/manpages/virsh.rst
> @@ -1714,13 +1714,16 @@ domdirtyrate-calc
>  ::
>  
>     domdirtyrate-calc <domain> [--seconds <sec>]
> +      [{--page-sampling | --dirty-bitmap | --dirty-ring}]

I'm wondering whether we should go with domdirtyrate-calc $dom
--mode=[page-samling|dirty-bitmap|dirty-ring] instead. My reasoning is
that it's more user friendly, IMO. But I don't have strong opinion, your
version is good too.

>  
>  Calculate an active domain's memory dirty rate which may be expected by
>  user in order to decide whether it's proper to be migrated out or not.
>  The ``seconds`` parameter can be used to calculate dirty rate in a
>  specific time which allows 60s at most now and would be default to 1s
> -if missing. The calculated dirty rate information is available by calling
> -'domstats --dirtyrate'.
> +if missing. These three *--page-sampling, --dirty-bitmap, --dirty-ring*
> +paremeters are mutually exclusive and used to specify calculation mode,
> +*--page-sampling* is the default mode if missing. The calculated dirty
> +rate information is available by calling 'domstats --dirtyrate'.
>  
>  
>  domdisplay
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 5912551..4caa740 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -13298,7 +13298,7 @@ virDomainGetMessages(virDomainPtr domain,
>   * virDomainStartDirtyRateCalc:
>   * @domain: a domain object
>   * @seconds: specified calculating time in seconds
> - * @flags: extra flags; not used yet, so callers should always pass 0
> + * @flags: bitwise-OR of supported virDomainDirtyRateCalcFlags
>   *
>   * Calculate the current domain's memory dirty rate in next @seconds.
>   * The calculated dirty rate information is available by calling
> @@ -13322,6 +13322,16 @@ virDomainStartDirtyRateCalc(virDomainPtr domain,
>  
>      virCheckReadOnlyGoto(conn->flags, error);
>  
> +    VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_DIRTYRATE_MODE_PAGE_SAMPLING,
> +                             VIR_DOMAIN_DIRTYRATE_MODE_DIRTY_BITMAP,
> +                             error);
> +    VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_DIRTYRATE_MODE_PAGE_SAMPLING,
> +                             VIR_DOMAIN_DIRTYRATE_MODE_DIRTY_RING,
> +                             error);
> +    VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_DIRTYRATE_MODE_DIRTY_BITMAP,
> +                             VIR_DOMAIN_DIRTYRATE_MODE_DIRTY_RING,
> +                             error);
> +
>      if (conn->driver->domainStartDirtyRateCalc) {
>          int ret;
>          ret = conn->driver->domainStartDirtyRateCalc(domain, seconds, flags);
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index cb2af61..5c2f289 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -20651,7 +20651,9 @@ qemuDomainStartDirtyRateCalc(virDomainPtr dom,
>          VIR_DOMAIN_DIRTYRATE_CALC_MODE_PAGE_SAMPLING;
>      int ret = -1;
>  
> -    virCheckFlags(0, -1);
> +    virCheckFlags(VIR_DOMAIN_DIRTYRATE_MODE_PAGE_SAMPLING |
> +                  VIR_DOMAIN_DIRTYRATE_MODE_DIRTY_BITMAP |
> +                  VIR_DOMAIN_DIRTYRATE_MODE_DIRTY_RING, -1);
>  
>      if (seconds < MIN_DIRTYRATE_CALC_PERIOD ||
>          seconds > MAX_DIRTYRATE_CALC_PERIOD) {
> @@ -20687,6 +20689,26 @@ qemuDomainStartDirtyRateCalc(virDomainPtr dom,
>          goto endjob;
>      }
>  
> +    if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DIRTYRATE_MODE)) {
> +        /* libvirt-domain.c already guaranteed these two flags are exclusive.  */
> +        if (flags & VIR_DOMAIN_DIRTYRATE_MODE_DIRTY_BITMAP) {
> +            mode = VIR_DOMAIN_DIRTYRATE_CALC_MODE_DIRTY_BITMAP;
> +        } else if (flags & VIR_DOMAIN_DIRTYRATE_MODE_DIRTY_RING) {
> +            if (vm->def->features[VIR_DOMAIN_FEATURE_KVM] != VIR_TRISTATE_SWITCH_ON ||
> +                vm->def->kvm_features->features[VIR_DOMAIN_KVM_DIRTY_RING] != VIR_TRISTATE_SWITCH_ON) {
> +                virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> +                               _("dirty-ring calculation mode requires dirty-ring feature enabled."));
> +                goto endjob;
> +            }
> +            mode = VIR_DOMAIN_DIRTYRATE_CALC_MODE_DIRTY_RING;
> +        }
> +    } else if ((flags & VIR_DOMAIN_DIRTYRATE_MODE_DIRTY_BITMAP ||
> +               (flags & VIR_DOMAIN_DIRTYRATE_MODE_DIRTY_RING))) {
> +        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> +                       _("QEMU does not support dirty page rate calculation mode"));
> +        goto endjob;
> +    }
> +

Again, this is something that could be done before acquiring domain job.

And if I may suggest slightly different implementation: have the code
that sets @mode first, without checking for qemuCaps and save that bit
for the end:

if (mode != VIR_DOMAIN_DIRTYRATE_MODE_PAGE_SAMPLING &&
    virQEMUCapsGet(priv->qemuCaps)) {
    error
}

That allows you to drop one level of nesting, which again improves
readability.

>      qemuDomainObjEnterMonitor(driver, vm);
>      ret = qemuMonitorStartDirtyRateCalc(priv->mon, seconds, mode);

Michal