[PATCH v2 5/6] virsh: Extend domdirtyrate-calc virsh api

[Peter Krempa]

On Thu, Jan 27, 2022 at 15:25:21 +0800, 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 +++++++++++-
>  tools/virsh-domain.c    | 28 +++++++++++++++++++++++++++-
>  3 files changed, 43 insertions(+), 4 deletions(-)
> 
> 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}]
>  
>  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);

Okay, so this clarifies what happened in the last commit. You've
misplaced this here.

Ideally separate the libvirt-domain.c/h changes into a separate commit,
then after them do the virsh commit and after that the commit adding
implementation in the qemu driver.

> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index b56f6a9..a032f70 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c

[...]

> @@ -14473,6 +14487,7 @@ cmdDomDirtyRateCalc(vshControl *ctl, const vshCmd *cmd)
>  {
>      g_autoptr(virshDomain) dom = NULL;
>      int seconds = 1; /* the default value is 1 */
> +    unsigned int flags = 0;
>  
>      if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
>          return false;
> @@ -14480,7 +14495,18 @@ cmdDomDirtyRateCalc(vshControl *ctl, const vshCmd *cmd)
>      if (vshCommandOptInt(ctl, cmd, "seconds", &seconds) < 0)
>          return false;
>  
> -    if (virDomainStartDirtyRateCalc(dom, seconds, 0) < 0)
> +    if (vshCommandOptBool(cmd, "page-sampling"))
> +        flags |= VIR_DOMAIN_DIRTYRATE_MODE_PAGE_SAMPLING;
> +    if (vshCommandOptBool(cmd, "dirty-bitmap"))
> +        flags |= VIR_DOMAIN_DIRTYRATE_MODE_DIRTY_BITMAP;;
> +    if (vshCommandOptBool(cmd, "dirty-ring"))
> +        flags |= VIR_DOMAIN_DIRTYRATE_MODE_DIRTY_RING;;
> +
> +    VSH_EXCLUSIVE_OPTIONS("page-sampling", "dirty-bitmap");
> +    VSH_EXCLUSIVE_OPTIONS("page-sampling", "dirty-ring");
> +    VSH_EXCLUSIVE_OPTIONS("dirty-bitmap", "dirty-ring");

We usually do these validations first, before using the options to
calculate flags.

> +
> +    if (virDomainStartDirtyRateCalc(dom, seconds, flags) < 0)
>          return false;
>  
>      vshPrintExtra(ctl, _("Start to calculate domain's memory "
> -- 
> 1.8.3.1
> 
>