[v21,9/9] tests: Add dirty page rate limit test

From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>

Add dirty page rate limit test if kernel support dirty ring,
create a standalone file to implement the test case.

The following qmp commands are covered by this test case:
"calc-dirty-rate", "query-dirty-rate", "set-vcpu-dirty-limit",
"cancel-vcpu-dirty-limit" and "query-vcpu-dirty-limit".

Signed-off-by: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
---
 tests/qtest/dirtylimit-test.c | 327 ++++++++++++++++++++++++++++++++++++++++++
 tests/qtest/meson.build       |   2 +
 2 files changed, 329 insertions(+)
 create mode 100644 tests/qtest/dirtylimit-test.c

On Wed, Mar 16, 2022 at 09:07:21PM +0800, huangy81@chinatelecom.cn wrote:
> From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
> 
> Add dirty page rate limit test if kernel support dirty ring,
> create a standalone file to implement the test case.
> 
> The following qmp commands are covered by this test case:
> "calc-dirty-rate", "query-dirty-rate", "set-vcpu-dirty-limit",
> "cancel-vcpu-dirty-limit" and "query-vcpu-dirty-limit".
> 
> Signed-off-by: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
> ---
>  tests/qtest/dirtylimit-test.c | 327 ++++++++++++++++++++++++++++++++++++++++++
>  tests/qtest/meson.build       |   2 +
>  2 files changed, 329 insertions(+)
>  create mode 100644 tests/qtest/dirtylimit-test.c
> 
> diff --git a/tests/qtest/dirtylimit-test.c b/tests/qtest/dirtylimit-test.c
> new file mode 100644
> index 0000000..b8d9960
> --- /dev/null
> +++ b/tests/qtest/dirtylimit-test.c
> @@ -0,0 +1,327 @@
> +/*
> + * QTest testcase for Dirty Page Rate Limit
> + *
> + * Copyright (c) 2022 CHINA TELECOM CO.,LTD.
> + *
> + * Authors:
> + *  Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "libqos/libqtest.h"
> +#include "qapi/qmp/qdict.h"
> +#include "qapi/qmp/qlist.h"
> +#include "qapi/qobject-input-visitor.h"
> +#include "qapi/qobject-output-visitor.h"
> +
> +#include "migration-helpers.h"
> +#include "tests/migration/i386/a-b-bootblock.h"
> +
> +/*
> + * Dirtylimit stop working if dirty page rate error
> + * value less than DIRTYLIMIT_TOLERANCE_RANGE
> + */
> +#define DIRTYLIMIT_TOLERANCE_RANGE  25  /* MB/s */
> +
> +static const char *tmpfs;
> +
> +static QDict *qmp_command(QTestState *who, const char *command, ...)
> +{
> +    va_list ap;
> +    QDict *resp, *ret;
> +
> +    va_start(ap, command);
> +    resp = qtest_vqmp(who, command, ap);
> +    va_end(ap);
> +
> +    g_assert(!qdict_haskey(resp, "error"));
> +    g_assert(qdict_haskey(resp, "return"));
> +
> +    ret = qdict_get_qdict(resp, "return");
> +    qobject_ref(ret);
> +    qobject_unref(resp);
> +
> +    return ret;
> +}
> +
> +static void calc_dirty_rate(QTestState *who, uint64_t calc_time)
> +{
> +    qobject_unref(qmp_command(who,
> +                  "{ 'execute': 'calc-dirty-rate',"
> +                  "'arguments': { "
> +                  "'calc-time': %ld,"
> +                  "'mode': 'dirty-ring' }}",
> +                  calc_time));
> +}
> +
> +static QDict *query_dirty_rate(QTestState *who)
> +{
> +    return qmp_command(who, "{ 'execute': 'query-dirty-rate' }");
> +}
> +
> +static void dirtylimit_set_all(QTestState *who, uint64_t dirtyrate)
> +{
> +    qobject_unref(qmp_command(who,
> +                  "{ 'execute': 'set-vcpu-dirty-limit',"
> +                  "'arguments': { "
> +                  "'dirty-rate': %ld } }",
> +                  dirtyrate));
> +}
> +
> +static void cancel_vcpu_dirty_limit(QTestState *who)
> +{
> +    qobject_unref(qmp_command(who,
> +                  "{ 'execute': 'cancel-vcpu-dirty-limit' }"));
> +}
> +
> +static QDict *query_vcpu_dirty_limit(QTestState *who)
> +{
> +    QDict *rsp;
> +
> +    rsp = qtest_qmp(who, "{ 'execute': 'query-vcpu-dirty-limit' }");
> +    g_assert(!qdict_haskey(rsp, "error"));
> +    g_assert(qdict_haskey(rsp, "return"));
> +
> +    return rsp;
> +}
> +
> +static bool calc_dirtyrate_ready(QTestState *who)
> +{
> +    QDict *rsp_return;
> +    gchar *status;
> +
> +    rsp_return = query_dirty_rate(who);
> +    g_assert(rsp_return);
> +
> +    status = g_strdup(qdict_get_str(rsp_return, "status"));
> +    g_assert(status);
> +
> +    return g_strcmp0(status, "measuring");
> +}
> +
> +static void wait_for_calc_dirtyrate_complete(QTestState *who,
> +                                             int64_t calc_time)
> +{
> +    int max_try_count = 200;
> +    usleep(calc_time);
> +
> +    while (!calc_dirtyrate_ready(who) && max_try_count--) {
> +        usleep(1000);
> +    }
> +
> +    /*
> +     * Set the timeout with 200 ms(max_try_count * 1000us),
> +     * if dirtyrate measurement not complete, test failed.
> +     */
> +    g_assert_cmpint(max_try_count, !=, 0);

200ms might be still too challenging for busy systems?  How about make it
in seconds (e.g. 10 seconds)?

> +}
> +
> +static int64_t get_dirty_rate(QTestState *who)
> +{
> +    QDict *rsp_return;
> +    gchar *status;
> +    QList *rates;
> +    const QListEntry *entry;
> +    QDict *rate;
> +    int64_t dirtyrate;
> +
> +    rsp_return = query_dirty_rate(who);
> +    g_assert(rsp_return);
> +
> +    status = g_strdup(qdict_get_str(rsp_return, "status"));
> +    g_assert(status);
> +    g_assert_cmpstr(status, ==, "measured");
> +
> +    rates = qdict_get_qlist(rsp_return, "vcpu-dirty-rate");
> +    g_assert(rates && !qlist_empty(rates));
> +
> +    entry = qlist_first(rates);
> +    g_assert(entry);
> +
> +    rate = qobject_to(QDict, qlist_entry_obj(entry));
> +    g_assert(rate);
> +
> +    dirtyrate = qdict_get_try_int(rate, "dirty-rate", -1);
> +
> +    qobject_unref(rsp_return);
> +    return dirtyrate;
> +}
> +
> +static int64_t get_limit_rate(QTestState *who)
> +{
> +    QDict *rsp_return;
> +    QList *rates;
> +    const QListEntry *entry;
> +    QDict *rate;
> +    int64_t dirtyrate;
> +
> +    rsp_return = query_vcpu_dirty_limit(who);
> +    g_assert(rsp_return);
> +
> +    rates = qdict_get_qlist(rsp_return, "return");
> +    g_assert(rates && !qlist_empty(rates));
> +
> +    entry = qlist_first(rates);
> +    g_assert(entry);
> +
> +    rate = qobject_to(QDict, qlist_entry_obj(entry));
> +    g_assert(rate);
> +
> +    dirtyrate = qdict_get_try_int(rate, "limit-rate", -1);
> +
> +    qobject_unref(rsp_return);
> +    return dirtyrate;
> +}
> +
> +static QTestState *start_vm(void)
> +{
> +    QTestState *vm = NULL;
> +    g_autofree gchar *cmd = NULL;
> +    const char *arch = qtest_get_arch();
> +    g_autofree char *bootpath = NULL;
> +
> +    assert((strcmp(arch, "x86_64") == 0));
> +    bootpath = g_strdup_printf("%s/bootsect", tmpfs);
> +    assert(sizeof(x86_bootsect) == 512);
> +    init_bootfile(bootpath, x86_bootsect, sizeof(x86_bootsect));
> +
> +    cmd = g_strdup_printf("-accel kvm,dirty-ring-size=4096 "
> +                          "-name dirtylimit-test,debug-threads=on "
> +                          "-m 150M -smp 1 "
> +                          "-serial file:%s/vm_serial "
> +                          "-drive file=%s,format=raw ",
> +                          tmpfs, bootpath);
> +
> +    vm = qtest_init(cmd);
> +    return vm;
> +}
> +
> +static void cleanup(const char *filename)
> +{
> +    g_autofree char *path = g_strdup_printf("%s/%s", tmpfs, filename);
> +    unlink(path);
> +}

Duplicated code - again, I'd suggest we drop previous patch and simply add
a new test into migration-test.c.  We could do the split later at any time,
but we'll need to think about how to split, not do that randomly..

> +
> +static void stop_vm(QTestState *vm)
> +{
> +    qtest_quit(vm);
> +    cleanup("bootsect");
> +    cleanup("vm_serial");
> +}
> +
> +static void test_vcpu_dirty_limit(void)
> +{
> +    QTestState *vm;
> +    int64_t origin_rate;
> +    int64_t quota_rate;
> +    int64_t rate ;
> +    int max_try_count = 5;
> +    int hit = 0;
> +
> +    vm = start_vm();
> +    if (!vm) {
> +        return;
> +    }

vm should always exist.

> +
> +    /* Wait for the first serial output from the vm*/
> +    wait_for_serial(tmpfs, "vm_serial");
> +
> +    /* Do dirtyrate measurement with calc time equals 1s */
> +    calc_dirty_rate(vm, 1);
> +
> +    /* Sleep a calc time and wait for calc dirtyrate complete */
> +    wait_for_calc_dirtyrate_complete(vm, 1 * 1000000);

1*1000000 reads odd..  I'd pass in 1 here and make the variable called
"seconds", then multiply there in the helper.

> +
> +    /* Query original dirty page rate */
> +    origin_rate = get_dirty_rate(vm);
> +
> +    /* VM booted from bootsect should dirty memory */
> +    assert(origin_rate != 0);
> +
> +    /* Setup quota dirty page rate at one-third of origin */
> +    quota_rate = origin_rate / 3;
> +
> +    /* Set dirtylimit and wait a bit to check if it take effect */
> +    dirtylimit_set_all(vm, quota_rate);
> +    usleep(2000000);

Nit: could move this to be after the g_assert check, because the limit
should apply immediately.

> +
> +    /*
> +     * Check if set-vcpu-dirty-limit and query-vcpu-dirty-limit
> +     * works literally
> +     */
> +    g_assert_cmpint(quota_rate, ==, get_limit_rate(vm));
> +
> +    /* Check if dirtylimit take effect realistically */
> +    while (--max_try_count) {
> +        calc_dirty_rate(vm, 1);
> +        wait_for_calc_dirtyrate_complete(vm, 1 * 1000000);
> +        rate = get_dirty_rate(vm);
> +
> +        /*
> +         * Assume hitting if current rate is less
> +         * than quota rate (within accepting error)
> +         */
> +        if (rate < (quota_rate + DIRTYLIMIT_TOLERANCE_RANGE)) {
> +            hit = 1;
> +            break;
> +        }
> +    }

I'm not sure 5 loops would be enough; bigger? I'd try running this test in
parallel with e.g. 20 instances on the host and see how slow it could
be. :)

The rest keeps look good to me.

Thanks,

> +
> +    g_assert_cmpint(hit, ==, 1);
> +
> +    hit = 0;
> +    max_try_count = 5;
> +
> +    /* Check if dirtylimit cancellation take effect */
> +    cancel_vcpu_dirty_limit(vm);
> +    while (--max_try_count) {
> +        calc_dirty_rate(vm, 1);
> +        wait_for_calc_dirtyrate_complete(vm, 1 * 1000000);
> +        rate = get_dirty_rate(vm);
> +
> +        /*
> +         * Assume dirtylimit be canceled if current rate is
> +         * greater than quota rate (within accepting error)
> +         */
> +        if (rate > (quota_rate + DIRTYLIMIT_TOLERANCE_RANGE)) {
> +            hit = 1;
> +            break;
> +        }
> +    }
> +
> +    g_assert_cmpint(hit, ==, 1);
> +    stop_vm(vm);
> +}
> +
> +int main(int argc, char **argv)
> +{
> +    char template[] = "/tmp/dirtylimit-test-XXXXXX";
> +    int ret;
> +
> +    tmpfs = mkdtemp(template);
> +    if (!tmpfs) {
> +        g_test_message("mkdtemp on path (%s): %s", template, strerror(errno));
> +    }
> +    g_assert(tmpfs);
> +
> +    if (!kvm_dirty_ring_supported()) {
> +        return 0;
> +    }
> +
> +    g_test_init(&argc, &argv, NULL);
> +    qtest_add_func("/dirtylimit/test", test_vcpu_dirty_limit);
> +    ret = g_test_run();
> +
> +    g_assert_cmpint(ret, ==, 0);
> +
> +    ret = rmdir(tmpfs);
> +    if (ret != 0) {
> +        g_test_message("unable to rmdir: path (%s): %s",
> +                       tmpfs, strerror(errno));
> +    }
> +
> +    return ret;
> +}
> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> index d25f82b..6b041e0 100644
> --- a/tests/qtest/meson.build
> +++ b/tests/qtest/meson.build
> @@ -32,6 +32,7 @@ qtests_generic = \
>    'qom-test',
>    'test-hmp',
>    'qos-test',
> +  'dirtylimit-test',
>  ]
>  if config_host.has_key('CONFIG_MODULES')
>    qtests_generic += [ 'modules-test' ]
> @@ -296,6 +297,7 @@ qtests = {
>    'tpm-tis-device-swtpm-test': [io, tpmemu_files, 'tpm-tis-util.c'],
>    'tpm-tis-device-test': [io, tpmemu_files, 'tpm-tis-util.c'],
>    'vmgenid-test': files('boot-sector.c', 'acpi-utils.c'),
> +  'dirtylimit-test': files('migration-helpers.c'),
>  }
>  
>  if dbus_display
> -- 
> 1.8.3.1
>

在 2022/3/30 3:54, Peter Xu 写道:
> On Wed, Mar 16, 2022 at 09:07:21PM +0800, huangy81@chinatelecom.cn wrote:
>> From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
>>
>> Add dirty page rate limit test if kernel support dirty ring,
>> create a standalone file to implement the test case.
>>
>> The following qmp commands are covered by this test case:
>> "calc-dirty-rate", "query-dirty-rate", "set-vcpu-dirty-limit",
>> "cancel-vcpu-dirty-limit" and "query-vcpu-dirty-limit".
>>
>> Signed-off-by: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
>> ---
>>   tests/qtest/dirtylimit-test.c | 327 ++++++++++++++++++++++++++++++++++++++++++
>>   tests/qtest/meson.build       |   2 +
>>   2 files changed, 329 insertions(+)
>>   create mode 100644 tests/qtest/dirtylimit-test.c
>>
>> diff --git a/tests/qtest/dirtylimit-test.c b/tests/qtest/dirtylimit-test.c
>> new file mode 100644
>> index 0000000..b8d9960
>> --- /dev/null
>> +++ b/tests/qtest/dirtylimit-test.c
>> @@ -0,0 +1,327 @@
>> +/*
>> + * QTest testcase for Dirty Page Rate Limit
>> + *
>> + * Copyright (c) 2022 CHINA TELECOM CO.,LTD.
>> + *
>> + * Authors:
>> + *  Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "libqos/libqtest.h"
>> +#include "qapi/qmp/qdict.h"
>> +#include "qapi/qmp/qlist.h"
>> +#include "qapi/qobject-input-visitor.h"
>> +#include "qapi/qobject-output-visitor.h"
>> +
>> +#include "migration-helpers.h"
>> +#include "tests/migration/i386/a-b-bootblock.h"
>> +
>> +/*
>> + * Dirtylimit stop working if dirty page rate error
>> + * value less than DIRTYLIMIT_TOLERANCE_RANGE
>> + */
>> +#define DIRTYLIMIT_TOLERANCE_RANGE  25  /* MB/s */
>> +
>> +static const char *tmpfs;
>> +
>> +static QDict *qmp_command(QTestState *who, const char *command, ...)
>> +{
>> +    va_list ap;
>> +    QDict *resp, *ret;
>> +
>> +    va_start(ap, command);
>> +    resp = qtest_vqmp(who, command, ap);
>> +    va_end(ap);
>> +
>> +    g_assert(!qdict_haskey(resp, "error"));
>> +    g_assert(qdict_haskey(resp, "return"));
>> +
>> +    ret = qdict_get_qdict(resp, "return");
>> +    qobject_ref(ret);
>> +    qobject_unref(resp);
>> +
>> +    return ret;
>> +}
>> +
>> +static void calc_dirty_rate(QTestState *who, uint64_t calc_time)
>> +{
>> +    qobject_unref(qmp_command(who,
>> +                  "{ 'execute': 'calc-dirty-rate',"
>> +                  "'arguments': { "
>> +                  "'calc-time': %ld,"
>> +                  "'mode': 'dirty-ring' }}",
>> +                  calc_time));
>> +}
>> +
>> +static QDict *query_dirty_rate(QTestState *who)
>> +{
>> +    return qmp_command(who, "{ 'execute': 'query-dirty-rate' }");
>> +}
>> +
>> +static void dirtylimit_set_all(QTestState *who, uint64_t dirtyrate)
>> +{
>> +    qobject_unref(qmp_command(who,
>> +                  "{ 'execute': 'set-vcpu-dirty-limit',"
>> +                  "'arguments': { "
>> +                  "'dirty-rate': %ld } }",
>> +                  dirtyrate));
>> +}
>> +
>> +static void cancel_vcpu_dirty_limit(QTestState *who)
>> +{
>> +    qobject_unref(qmp_command(who,
>> +                  "{ 'execute': 'cancel-vcpu-dirty-limit' }"));
>> +}
>> +
>> +static QDict *query_vcpu_dirty_limit(QTestState *who)
>> +{
>> +    QDict *rsp;
>> +
>> +    rsp = qtest_qmp(who, "{ 'execute': 'query-vcpu-dirty-limit' }");
>> +    g_assert(!qdict_haskey(rsp, "error"));
>> +    g_assert(qdict_haskey(rsp, "return"));
>> +
>> +    return rsp;
>> +}
>> +
>> +static bool calc_dirtyrate_ready(QTestState *who)
>> +{
>> +    QDict *rsp_return;
>> +    gchar *status;
>> +
>> +    rsp_return = query_dirty_rate(who);
>> +    g_assert(rsp_return);
>> +
>> +    status = g_strdup(qdict_get_str(rsp_return, "status"));
>> +    g_assert(status);
>> +
>> +    return g_strcmp0(status, "measuring");
>> +}
>> +
>> +static void wait_for_calc_dirtyrate_complete(QTestState *who,
>> +                                             int64_t calc_time)
>> +{
>> +    int max_try_count = 200;
>> +    usleep(calc_time);
>> +
>> +    while (!calc_dirtyrate_ready(who) && max_try_count--) {
>> +        usleep(1000);
>> +    }
>> +
>> +    /*
>> +     * Set the timeout with 200 ms(max_try_count * 1000us),
>> +     * if dirtyrate measurement not complete, test failed.
>> +     */
>> +    g_assert_cmpint(max_try_count, !=, 0);
> 
> 200ms might be still too challenging for busy systems?  How about make it
> in seconds (e.g. 10 seconds)?
> 
>> +}
>> +
>> +static int64_t get_dirty_rate(QTestState *who)
>> +{
>> +    QDict *rsp_return;
>> +    gchar *status;
>> +    QList *rates;
>> +    const QListEntry *entry;
>> +    QDict *rate;
>> +    int64_t dirtyrate;
>> +
>> +    rsp_return = query_dirty_rate(who);
>> +    g_assert(rsp_return);
>> +
>> +    status = g_strdup(qdict_get_str(rsp_return, "status"));
>> +    g_assert(status);
>> +    g_assert_cmpstr(status, ==, "measured");
>> +
>> +    rates = qdict_get_qlist(rsp_return, "vcpu-dirty-rate");
>> +    g_assert(rates && !qlist_empty(rates));
>> +
>> +    entry = qlist_first(rates);
>> +    g_assert(entry);
>> +
>> +    rate = qobject_to(QDict, qlist_entry_obj(entry));
>> +    g_assert(rate);
>> +
>> +    dirtyrate = qdict_get_try_int(rate, "dirty-rate", -1);
>> +
>> +    qobject_unref(rsp_return);
>> +    return dirtyrate;
>> +}
>> +
>> +static int64_t get_limit_rate(QTestState *who)
>> +{
>> +    QDict *rsp_return;
>> +    QList *rates;
>> +    const QListEntry *entry;
>> +    QDict *rate;
>> +    int64_t dirtyrate;
>> +
>> +    rsp_return = query_vcpu_dirty_limit(who);
>> +    g_assert(rsp_return);
>> +
>> +    rates = qdict_get_qlist(rsp_return, "return");
>> +    g_assert(rates && !qlist_empty(rates));
>> +
>> +    entry = qlist_first(rates);
>> +    g_assert(entry);
>> +
>> +    rate = qobject_to(QDict, qlist_entry_obj(entry));
>> +    g_assert(rate);
>> +
>> +    dirtyrate = qdict_get_try_int(rate, "limit-rate", -1);
>> +
>> +    qobject_unref(rsp_return);
>> +    return dirtyrate;
>> +}
>> +
>> +static QTestState *start_vm(void)
>> +{
>> +    QTestState *vm = NULL;
>> +    g_autofree gchar *cmd = NULL;
>> +    const char *arch = qtest_get_arch();
>> +    g_autofree char *bootpath = NULL;
>> +
>> +    assert((strcmp(arch, "x86_64") == 0));
>> +    bootpath = g_strdup_printf("%s/bootsect", tmpfs);
>> +    assert(sizeof(x86_bootsect) == 512);
>> +    init_bootfile(bootpath, x86_bootsect, sizeof(x86_bootsect));
>> +
>> +    cmd = g_strdup_printf("-accel kvm,dirty-ring-size=4096 "
>> +                          "-name dirtylimit-test,debug-threads=on "
>> +                          "-m 150M -smp 1 "
>> +                          "-serial file:%s/vm_serial "
>> +                          "-drive file=%s,format=raw ",
>> +                          tmpfs, bootpath);
>> +
>> +    vm = qtest_init(cmd);
>> +    return vm;
>> +}
>> +
>> +static void cleanup(const char *filename)
>> +{
>> +    g_autofree char *path = g_strdup_printf("%s/%s", tmpfs, filename);
>> +    unlink(path);
>> +}
> 
> Duplicated code - again, I'd suggest we drop previous patch and simply add
> a new test into migration-test.c.  We could do the split later at any time,
> but we'll need to think about how to split, not do that randomly..
> 
Ok.
>> +
>> +static void stop_vm(QTestState *vm)
>> +{
>> +    qtest_quit(vm);
>> +    cleanup("bootsect");
>> +    cleanup("vm_serial");
>> +}
>> +
>> +static void test_vcpu_dirty_limit(void)
>> +{
>> +    QTestState *vm;
>> +    int64_t origin_rate;
>> +    int64_t quota_rate;
>> +    int64_t rate ;
>> +    int max_try_count = 5;
>> +    int hit = 0;
>> +
>> +    vm = start_vm();
>> +    if (!vm) {
>> +        return;
>> +    }
> 
> vm should always exist.
Ok, i'll remove it.
> 
>> +
>> +    /* Wait for the first serial output from the vm*/
>> +    wait_for_serial(tmpfs, "vm_serial");
>> +
>> +    /* Do dirtyrate measurement with calc time equals 1s */
>> +    calc_dirty_rate(vm, 1);
>> +
>> +    /* Sleep a calc time and wait for calc dirtyrate complete */
>> +    wait_for_calc_dirtyrate_complete(vm, 1 * 1000000);
> 
> 1*1000000 reads odd..  I'd pass in 1 here and make the variable called
> "seconds", then multiply there in the helper.
Sound good.
> 
>> +
>> +    /* Query original dirty page rate */
>> +    origin_rate = get_dirty_rate(vm);
>> +
>> +    /* VM booted from bootsect should dirty memory */
>> +    assert(origin_rate != 0);
>> +
>> +    /* Setup quota dirty page rate at one-third of origin */
>> +    quota_rate = origin_rate / 3;
>> +
>> +    /* Set dirtylimit and wait a bit to check if it take effect */
>> +    dirtylimit_set_all(vm, quota_rate);
>> +    usleep(2000000);
> 
> Nit: could move this to be after the g_assert check, because the limit
> should apply immediately.
Make sense.
> 
>> +
>> +    /*
>> +     * Check if set-vcpu-dirty-limit and query-vcpu-dirty-limit
>> +     * works literally
>> +     */
>> +    g_assert_cmpint(quota_rate, ==, get_limit_rate(vm));
>> +
>> +    /* Check if dirtylimit take effect realistically */
>> +    while (--max_try_count) {
>> +        calc_dirty_rate(vm, 1);
>> +        wait_for_calc_dirtyrate_complete(vm, 1 * 1000000);
>> +        rate = get_dirty_rate(vm);
>> +
>> +        /*
>> +         * Assume hitting if current rate is less
>> +         * than quota rate (within accepting error)
>> +         */
>> +        if (rate < (quota_rate + DIRTYLIMIT_TOLERANCE_RANGE)) {
>> +            hit = 1;
>> +            break;
>> +        }
>> +    }
> 
> I'm not sure 5 loops would be enough; bigger? I'd try running this test in
> parallel with e.g. 20 instances on the host and see how slow it could
> be. :)Indeed your test policy is right, 5 may be not enough in special 
scenarios. Extreme scene such as busy systems should be taken into 
consideration since in that scene test fails doesn't mean it doesn't 
work functionally. I'll make the max_try_count larger in functions: 
wait_for_calc_dirtyrate_complete, test_vcpu_dirty_limit.
> 
> The rest keeps look good to me.
> 
> Thanks,
> 
>> +
>> +    g_assert_cmpint(hit, ==, 1);
>> +
>> +    hit = 0;
>> +    max_try_count = 5;
>> +
>> +    /* Check if dirtylimit cancellation take effect */
>> +    cancel_vcpu_dirty_limit(vm);
>> +    while (--max_try_count) {
>> +        calc_dirty_rate(vm, 1);
>> +        wait_for_calc_dirtyrate_complete(vm, 1 * 1000000);
>> +        rate = get_dirty_rate(vm);
>> +
>> +        /*
>> +         * Assume dirtylimit be canceled if current rate is
>> +         * greater than quota rate (within accepting error)
>> +         */
>> +        if (rate > (quota_rate + DIRTYLIMIT_TOLERANCE_RANGE)) {
>> +            hit = 1;
>> +            break;
>> +        }
>> +    }
>> +
>> +    g_assert_cmpint(hit, ==, 1);
>> +    stop_vm(vm);
>> +}
>> +
>> +int main(int argc, char **argv)
>> +{
>> +    char template[] = "/tmp/dirtylimit-test-XXXXXX";
>> +    int ret;
>> +
>> +    tmpfs = mkdtemp(template);
>> +    if (!tmpfs) {
>> +        g_test_message("mkdtemp on path (%s): %s", template, strerror(errno));
>> +    }
>> +    g_assert(tmpfs);
>> +
>> +    if (!kvm_dirty_ring_supported()) {
>> +        return 0;
>> +    }
>> +
>> +    g_test_init(&argc, &argv, NULL);
>> +    qtest_add_func("/dirtylimit/test", test_vcpu_dirty_limit);
>> +    ret = g_test_run();
>> +
>> +    g_assert_cmpint(ret, ==, 0);
>> +
>> +    ret = rmdir(tmpfs);
>> +    if (ret != 0) {
>> +        g_test_message("unable to rmdir: path (%s): %s",
>> +                       tmpfs, strerror(errno));
>> +    }
>> +
>> +    return ret;
>> +}
>> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
>> index d25f82b..6b041e0 100644
>> --- a/tests/qtest/meson.build
>> +++ b/tests/qtest/meson.build
>> @@ -32,6 +32,7 @@ qtests_generic = \
>>     'qom-test',
>>     'test-hmp',
>>     'qos-test',
>> +  'dirtylimit-test',
>>   ]
>>   if config_host.has_key('CONFIG_MODULES')
>>     qtests_generic += [ 'modules-test' ]
>> @@ -296,6 +297,7 @@ qtests = {
>>     'tpm-tis-device-swtpm-test': [io, tpmemu_files, 'tpm-tis-util.c'],
>>     'tpm-tis-device-test': [io, tpmemu_files, 'tpm-tis-util.c'],
>>     'vmgenid-test': files('boot-sector.c', 'acpi-utils.c'),
>> +  'dirtylimit-test': files('migration-helpers.c'),
>>   }
>>   
>>   if dbus_display
>> -- 
>> 1.8.3.1
>>
>