[v19,7/8] tests/numa: Add case for QMP build HMAT

Commit Message

Tao Xu Nov. 28, 2019, 8:21 a.m. UTC

Check configuring HMAT usecase

Reviewed-by: Igor Mammedov <[e-mail]>
Suggested-by: Igor Mammedov <[e-mail]>
Signed-off-by: Tao Xu <[e-mail]>
---

Changes in v19:
    - Add some fail cases for hmat-cache when level=0

Changes in v18:
    - Rewrite the lines over 80 characters

Chenges in v17:
    - Add some fail test cases (Igor)
---
 tests/numa-test.c | 213 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 213 insertions(+)

Comments

Markus Armbruster Nov. 28, 2019, 11:49 a.m. UTC | #1

Tao Xu <[e-mail]> writes:

> Check configuring HMAT usecase
>
> Reviewed-by: Igor Mammedov <[e-mail]>
> Suggested-by: Igor Mammedov <[e-mail]>
> Signed-off-by: Tao Xu <[e-mail]>
> ---
>
> Changes in v19:
>     - Add some fail cases for hmat-cache when level=0
>
> Changes in v18:
>     - Rewrite the lines over 80 characters
>
> Chenges in v17:
>     - Add some fail test cases (Igor)
> ---
>  tests/numa-test.c | 213 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 213 insertions(+)
>
> diff --git a/tests/numa-test.c b/tests/numa-test.c
> index 8de8581231..aed7b2f31b 100644
> --- a/tests/numa-test.c
> +++ b/tests/numa-test.c
> @@ -327,6 +327,216 @@ static void pc_dynamic_cpu_cfg(const void *data)
>      qtest_quit(qs);
>  }
>  
> +static void pc_hmat_build_cfg(const void *data)
> +{
> +    QTestState *qs = qtest_initf("%s -nodefaults --preconfig -machine hmat=on "
> +                     "-smp 2,sockets=2 "
> +                     "-m 128M,slots=2,maxmem=1G "
> +                     "-object memory-backend-ram,size=64M,id=m0 "
> +                     "-object memory-backend-ram,size=64M,id=m1 "
> +                     "-numa node,nodeid=0,memdev=m0 "
> +                     "-numa node,nodeid=1,memdev=m1,initiator=0 "
> +                     "-numa cpu,node-id=0,socket-id=0 "
> +                     "-numa cpu,node-id=0,socket-id=1",
> +                     data ? (char *)data : "");
> +
> +    /* Fail: Initiator should be less than the number of nodes */
> +    g_assert(qmp_rsp_is_err(qtest_qmp(qs, "{ 'execute': 'set-numa-node',"
> +        " 'arguments': { 'type': 'hmat-lb', 'initiator': 2, 'target': 0,"
> +        " 'hierarchy': \"memory\", 'data-type': \"access-latency\" } }")));

Code smell: side effect within assert().

Harmless here, because compiling tests with NDEBUG is pointless.  Still,
it sets a bad example.  Not your idea, the pattern seems to go back to
commit c35665e1ee3 and fb1e58f72ba.

The non-smelly pattern would be

    resp = qtest_qmp(...);
    g_assert(resp);
    g_assert(qdict_haskey(rsp, "error"));
    qobject_unref(resp);

It's a bit longwinded.  We could create a suitable function, but then
the assertion points to the function, which is less useful than pointing
to the test.  A macro could avoid that.

Can be cleaned up on top.

[...]

Thomas Huth Nov. 28, 2019, 11:53 a.m. UTC | #2

On 28/11/2019 12.49, Markus Armbruster wrote:
> Tao Xu <[e-mail]> writes:
> 
>> Check configuring HMAT usecase
>>
>> Reviewed-by: Igor Mammedov <[e-mail]>
>> Suggested-by: Igor Mammedov <[e-mail]>
>> Signed-off-by: Tao Xu <[e-mail]>
>> ---
>>
>> Changes in v19:
>>      - Add some fail cases for hmat-cache when level=0
>>
>> Changes in v18:
>>      - Rewrite the lines over 80 characters
>>
>> Chenges in v17:
>>      - Add some fail test cases (Igor)
>> ---
>>   tests/numa-test.c | 213 ++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 213 insertions(+)
>>
>> diff --git a/tests/numa-test.c b/tests/numa-test.c
>> index 8de8581231..aed7b2f31b 100644
>> --- a/tests/numa-test.c
>> +++ b/tests/numa-test.c
>> @@ -327,6 +327,216 @@ static void pc_dynamic_cpu_cfg(const void *data)
>>       qtest_quit(qs);
>>   }
>>   
>> +static void pc_hmat_build_cfg(const void *data)
>> +{
>> +    QTestState *qs = qtest_initf("%s -nodefaults --preconfig -machine hmat=on "
>> +                     "-smp 2,sockets=2 "
>> +                     "-m 128M,slots=2,maxmem=1G "
>> +                     "-object memory-backend-ram,size=64M,id=m0 "
>> +                     "-object memory-backend-ram,size=64M,id=m1 "
>> +                     "-numa node,nodeid=0,memdev=m0 "
>> +                     "-numa node,nodeid=1,memdev=m1,initiator=0 "
>> +                     "-numa cpu,node-id=0,socket-id=0 "
>> +                     "-numa cpu,node-id=0,socket-id=1",
>> +                     data ? (char *)data : "");
>> +
>> +    /* Fail: Initiator should be less than the number of nodes */
>> +    g_assert(qmp_rsp_is_err(qtest_qmp(qs, "{ 'execute': 'set-numa-node',"
>> +        " 'arguments': { 'type': 'hmat-lb', 'initiator': 2, 'target': 0,"
>> +        " 'hierarchy': \"memory\", 'data-type': \"access-latency\" } }")));
> 
> Code smell: side effect within assert().
> 
> Harmless here, because compiling tests with NDEBUG is pointless.  Still,
> it sets a bad example.  Not your idea, the pattern seems to go back to
> commit c35665e1ee3 and fb1e58f72ba.

... maybe best to use g_assert_true() which can't be disabled and thus 
should be used in tests. See:

  https://developer.gnome.org/glib/unstable/glib-Testing.html#g-assert-true

  Thomas

Tao Xu Nov. 29, 2019, 3:17 a.m. UTC | #3

On 11/28/2019 7:53 PM, Thomas Huth wrote:
> On 28/11/2019 12.49, Markus Armbruster wrote:
>> Tao Xu <[e-mail]> writes:
>>
>>> Check configuring HMAT usecase
>>>
>>> Reviewed-by: Igor Mammedov <[e-mail]>
>>> Suggested-by: Igor Mammedov <[e-mail]>
>>> Signed-off-by: Tao Xu <[e-mail]>
>>> ---
>>>
>>> Changes in v19:
>>>       - Add some fail cases for hmat-cache when level=0
>>>
>>> Changes in v18:
>>>       - Rewrite the lines over 80 characters
>>>
>>> Chenges in v17:
>>>       - Add some fail test cases (Igor)
>>> ---
>>>    tests/numa-test.c | 213 ++++++++++++++++++++++++++++++++++++++++++++++
>>>    1 file changed, 213 insertions(+)
>>>
>>> diff --git a/tests/numa-test.c b/tests/numa-test.c
>>> index 8de8581231..aed7b2f31b 100644
>>> --- a/tests/numa-test.c
>>> +++ b/tests/numa-test.c
>>> @@ -327,6 +327,216 @@ static void pc_dynamic_cpu_cfg(const void *data)
>>>        qtest_quit(qs);
>>>    }
>>>    
>>> +static void pc_hmat_build_cfg(const void *data)
>>> +{
>>> +    QTestState *qs = qtest_initf("%s -nodefaults --preconfig -machine hmat=on "
>>> +                     "-smp 2,sockets=2 "
>>> +                     "-m 128M,slots=2,maxmem=1G "
>>> +                     "-object memory-backend-ram,size=64M,id=m0 "
>>> +                     "-object memory-backend-ram,size=64M,id=m1 "
>>> +                     "-numa node,nodeid=0,memdev=m0 "
>>> +                     "-numa node,nodeid=1,memdev=m1,initiator=0 "
>>> +                     "-numa cpu,node-id=0,socket-id=0 "
>>> +                     "-numa cpu,node-id=0,socket-id=1",
>>> +                     data ? (char *)data : "");
>>> +
>>> +    /* Fail: Initiator should be less than the number of nodes */
>>> +    g_assert(qmp_rsp_is_err(qtest_qmp(qs, "{ 'execute': 'set-numa-node',"
>>> +        " 'arguments': { 'type': 'hmat-lb', 'initiator': 2, 'target': 0,"
>>> +        " 'hierarchy': \"memory\", 'data-type': \"access-latency\" } }")));
>>
>> Code smell: side effect within assert().
>>
>> Harmless here, because compiling tests with NDEBUG is pointless.  Still,
>> it sets a bad example.  Not your idea, the pattern seems to go back to
>> commit c35665e1ee3 and fb1e58f72ba.
> 
> ... maybe best to use g_assert_true() which can't be disabled and thus
> should be used in tests. See:
> 
>    https://developer.gnome.org/glib/unstable/glib-Testing.html#g-assert-true
> 
>    Thomas
> 
Thank you for your suggestion. I will use g_assert_true and 
g_assert_false to replace g_assert

Patch

diff --git a/tests/numa-test.c b/tests/numa-test.c
index 8de8581231..aed7b2f31b 100644
--- a/tests/numa-test.c
+++ b/tests/numa-test.c
@@ -327,6 +327,216 @@  static void pc_dynamic_cpu_cfg(const void *data)
     qtest_quit(qs);
 }
 
+static void pc_hmat_build_cfg(const void *data)
+{
+    QTestState *qs = qtest_initf("%s -nodefaults --preconfig -machine hmat=on "
+                     "-smp 2,sockets=2 "
+                     "-m 128M,slots=2,maxmem=1G "
+                     "-object memory-backend-ram,size=64M,id=m0 "
+                     "-object memory-backend-ram,size=64M,id=m1 "
+                     "-numa node,nodeid=0,memdev=m0 "
+                     "-numa node,nodeid=1,memdev=m1,initiator=0 "
+                     "-numa cpu,node-id=0,socket-id=0 "
+                     "-numa cpu,node-id=0,socket-id=1",
+                     data ? (char *)data : "");
+
+    /* Fail: Initiator should be less than the number of nodes */
+    g_assert(qmp_rsp_is_err(qtest_qmp(qs, "{ 'execute': 'set-numa-node',"
+        " 'arguments': { 'type': 'hmat-lb', 'initiator': 2, 'target': 0,"
+        " 'hierarchy': \"memory\", 'data-type': \"access-latency\" } }")));
+
+    /* Fail: Target should be less than the number of nodes */
+    g_assert(qmp_rsp_is_err(qtest_qmp(qs, "{ 'execute': 'set-numa-node',"
+        " 'arguments': { 'type': 'hmat-lb', 'initiator': 0, 'target': 2,"
+        " 'hierarchy': \"memory\", 'data-type': \"access-latency\" } }")));
+
+    /* Fail: Initiator should contain cpu */
+    g_assert(qmp_rsp_is_err(qtest_qmp(qs, "{ 'execute': 'set-numa-node',"
+        " 'arguments': { 'type': 'hmat-lb', 'initiator': 1, 'target': 0,"
+        " 'hierarchy': \"memory\", 'data-type': \"access-latency\" } }")));
+
+    /* Fail: Data-type mismatch */
+    g_assert(qmp_rsp_is_err(qtest_qmp(qs, "{ 'execute': 'set-numa-node',"
+        " 'arguments': { 'type': 'hmat-lb', 'initiator': 0, 'target': 0,"
+        " 'hierarchy': \"memory\", 'data-type': \"write-latency\","
+        " 'bandwidth': 524288000 } }")));
+    g_assert(qmp_rsp_is_err(qtest_qmp(qs, "{ 'execute': 'set-numa-node',"
+        " 'arguments': { 'type': 'hmat-lb', 'initiator': 0, 'target': 0,"
+        " 'hierarchy': \"memory\", 'data-type': \"read-bandwidth\","
+        " 'latency': 5 } }")));
+
+    /* Fail: Bandwidth should be 1MB (1048576) aligned */
+    g_assert(qmp_rsp_is_err(qtest_qmp(qs, "{ 'execute': 'set-numa-node',"
+        " 'arguments': { 'type': 'hmat-lb', 'initiator': 0, 'target': 0,"
+        " 'hierarchy': \"memory\", 'data-type': \"access-bandwidth\","
+        " 'bandwidth': 1048575 } }")));
+
+    /* Configuring HMAT bandwidth and latency details */
+    g_assert(!qmp_rsp_is_err(qtest_qmp(qs, "{ 'execute': 'set-numa-node',"
+        " 'arguments': { 'type': 'hmat-lb', 'initiator': 0, 'target': 0,"
+        " 'hierarchy': \"memory\", 'data-type': \"access-latency\","
+        " 'latency': 1 } }")));    /* 1 ns */
+    g_assert(qmp_rsp_is_err(qtest_qmp(qs, "{ 'execute': 'set-numa-node',"
+        " 'arguments': { 'type': 'hmat-lb', 'initiator': 0, 'target': 0,"
+        " 'hierarchy': \"memory\", 'data-type': \"access-latency\","
+        " 'latency': 5 } }")));    /* Fail: Duplicate configuration */
+    g_assert(!qmp_rsp_is_err(qtest_qmp(qs, "{ 'execute': 'set-numa-node',"
+        " 'arguments': { 'type': 'hmat-lb', 'initiator': 0, 'target': 0,"
+        " 'hierarchy': \"memory\", 'data-type': \"access-bandwidth\","
+        " 'bandwidth': 68717379584 } }")));    /* 65534 MB/s */
+    g_assert(!qmp_rsp_is_err(qtest_qmp(qs, "{ 'execute': 'set-numa-node',"
+        " 'arguments': { 'type': 'hmat-lb', 'initiator': 0, 'target': 1,"
+        " 'hierarchy': \"memory\", 'data-type': \"access-latency\","
+        " 'latency': 65534 } }")));    /* 65534 ns */
+    g_assert(!qmp_rsp_is_err(qtest_qmp(qs, "{ 'execute': 'set-numa-node',"
+        " 'arguments': { 'type': 'hmat-lb', 'initiator': 0, 'target': 1,"
+        " 'hierarchy': \"memory\", 'data-type': \"access-bandwidth\","
+        " 'bandwidth': 34358689792 } }")));    /* 32767 MB/s */
+
+    /* Fail: node_id should be less than the number of nodes */
+    g_assert(qmp_rsp_is_err(qtest_qmp(qs, "{ 'execute': 'set-numa-node',"
+        " 'arguments': { 'type': 'hmat-cache', 'node-id': 2, 'size': 10240,"
+        " 'level': 1, 'assoc': \"direct\", 'policy': \"write-back\","
+        " 'line': 8 } }")));
+
+    /* Fail: level should be less than HMAT_LB_LEVELS (4) */
+    g_assert(qmp_rsp_is_err(qtest_qmp(qs, "{ 'execute': 'set-numa-node',"
+        " 'arguments': { 'type': 'hmat-cache', 'node-id': 0, 'size': 10240,"
+        " 'level': 4, 'assoc': \"direct\", 'policy': \"write-back\","
+        " 'line': 8 } }")));
+
+    /* Fail: assoc option should be 'none', if level is 0 */
+    g_assert(qmp_rsp_is_err(qtest_qmp(qs, "{ 'execute': 'set-numa-node',"
+        " 'arguments': { 'type': 'hmat-cache', 'node-id': 0, 'size': 10240,"
+        " 'level': 0, 'assoc': \"direct\", 'policy': \"none\","
+        " 'line': 0 } }")));
+    /* Fail: policy option should be 'none', if level is 0 */
+    g_assert(qmp_rsp_is_err(qtest_qmp(qs, "{ 'execute': 'set-numa-node',"
+        " 'arguments': { 'type': 'hmat-cache', 'node-id': 0, 'size': 10240,"
+        " 'level': 0, 'assoc': \"none\", 'policy': \"write-back\","
+        " 'line': 0 } }")));
+    /* Fail: line option should be 0, if level is 0 */
+    g_assert(qmp_rsp_is_err(qtest_qmp(qs, "{ 'execute': 'set-numa-node',"
+        " 'arguments': { 'type': 'hmat-cache', 'node-id': 0, 'size': 10240,"
+        " 'level': 0, 'assoc': \"none\", 'policy': \"none\","
+        " 'line': 8 } }")));
+
+    /* Configuring HMAT memory side cache attributes */
+    g_assert(!qmp_rsp_is_err(qtest_qmp(qs, "{ 'execute': 'set-numa-node',"
+        " 'arguments': { 'type': 'hmat-cache', 'node-id': 0, 'size': 10240,"
+        " 'level': 1, 'assoc': \"direct\", 'policy': \"write-back\","
+        " 'line': 8 } }")));
+    g_assert(qmp_rsp_is_err(qtest_qmp(qs, "{ 'execute': 'set-numa-node',"
+        " 'arguments': { 'type': 'hmat-cache', 'node-id': 0, 'size': 10240,"
+        " 'level': 1, 'assoc': \"direct\", 'policy': \"write-back\","
+        " 'line': 8 } }")));    /* Fail: Duplicate configuration */
+    /* Fail: The size of level 2 size should be small than level 1 */
+    g_assert(qmp_rsp_is_err(qtest_qmp(qs, "{ 'execute': 'set-numa-node',"
+        " 'arguments': { 'type': 'hmat-cache', 'node-id': 0, 'size': 10240,"
+        " 'level': 2, 'assoc': \"direct\", 'policy': \"write-back\","
+        " 'line': 8 } }")));
+    /* Fail: The size of level 0 size should be larger than level 1 */
+    g_assert(qmp_rsp_is_err(qtest_qmp(qs, "{ 'execute': 'set-numa-node',"
+        " 'arguments': { 'type': 'hmat-cache', 'node-id': 0, 'size': 10240,"
+        " 'level': 0, 'assoc': \"direct\", 'policy': \"write-back\","
+        " 'line': 8 } }")));
+    g_assert(!qmp_rsp_is_err(qtest_qmp(qs, "{ 'execute': 'set-numa-node',"
+        " 'arguments': { 'type': 'hmat-cache', 'node-id': 1, 'size': 10240,"
+        " 'level': 1, 'assoc': \"direct\", 'policy': \"write-back\","
+        " 'line': 8 } }")));
+
+    /* let machine initialization to complete and run */
+    g_assert(!qmp_rsp_is_err(qtest_qmp(qs,
+                                       "{ 'execute': 'x-exit-preconfig' }")));
+    qtest_qmp_eventwait(qs, "RESUME");
+
+    qtest_quit(qs);
+}
+
+static void pc_hmat_off_cfg(const void *data)
+{
+    QTestState *qs = qtest_initf("%s -nodefaults --preconfig "
+                     "-smp 2,sockets=2 "
+                     "-m 128M,slots=2,maxmem=1G "
+                     "-object memory-backend-ram,size=64M,id=m0 "
+                     "-object memory-backend-ram,size=64M,id=m1 "
+                     "-numa node,nodeid=0,memdev=m0",
+                     data ? (char *)data : "");
+
+    /*
+     * Fail: Enable HMAT with -machine hmat=on
+     * before using any of hmat specific options
+     */
+    g_assert(qmp_rsp_is_err(qtest_qmp(qs, "{ 'execute': 'set-numa-node',"
+        " 'arguments': { 'type': 'node', 'nodeid': 1, 'memdev': \"m1\","
+        " 'initiator': 0 } }")));
+    g_assert(!qmp_rsp_is_err(qtest_qmp(qs, "{ 'execute': 'set-numa-node',"
+        " 'arguments': { 'type': 'node', 'nodeid': 1, 'memdev': \"m1\" } }")));
+    g_assert(qmp_rsp_is_err(qtest_qmp(qs, "{ 'execute': 'set-numa-node',"
+        " 'arguments': { 'type': 'hmat-lb', 'initiator': 0, 'target': 0,"
+        " 'hierarchy': \"memory\", 'data-type': \"access-latency\","
+        " 'latency': 1 } }")));
+    g_assert(qmp_rsp_is_err(qtest_qmp(qs, "{ 'execute': 'set-numa-node',"
+        " 'arguments': { 'type': 'hmat-cache', 'node-id': 0, 'size': 10240,"
+        " 'level': 1, 'assoc': \"direct\", 'policy': \"write-back\","
+        " 'line': 8 } }")));
+
+    /* let machine initialization to complete and run */
+    g_assert(!qmp_rsp_is_err(qtest_qmp(qs,
+                                       "{ 'execute': 'x-exit-preconfig' }")));
+    qtest_qmp_eventwait(qs, "RESUME");
+
+    qtest_quit(qs);
+}
+
+static void pc_hmat_erange_cfg(const void *data)
+{
+    QTestState *qs = qtest_initf("%s -nodefaults --preconfig -machine hmat=on "
+                     "-smp 2,sockets=2 "
+                     "-m 128M,slots=2,maxmem=1G "
+                     "-object memory-backend-ram,size=64M,id=m0 "
+                     "-object memory-backend-ram,size=64M,id=m1 "
+                     "-numa node,nodeid=0,memdev=m0 "
+                     "-numa node,nodeid=1,memdev=m1,initiator=0 "
+                     "-numa cpu,node-id=0,socket-id=0 "
+                     "-numa cpu,node-id=0,socket-id=1",
+                     data ? (char *)data : "");
+
+    /* Can't store the compressed latency */
+    g_assert(!qmp_rsp_is_err(qtest_qmp(qs, "{ 'execute': 'set-numa-node',"
+        " 'arguments': { 'type': 'hmat-lb', 'initiator': 0, 'target': 0,"
+        " 'hierarchy': \"memory\", 'data-type': \"access-latency\","
+        " 'latency': 1 } }")));    /* 1 ns */
+    g_assert(qmp_rsp_is_err(qtest_qmp(qs, "{ 'execute': 'set-numa-node',"
+        " 'arguments': { 'type': 'hmat-lb', 'initiator': 0, 'target': 0,"
+        " 'hierarchy': \"memory\", 'data-type': \"access-latency\","
+        " 'latency': 65535 } }")));    /* 65535 ns */
+
+    /* Test the 0 input (bandwidth not provided) */
+    g_assert(!qmp_rsp_is_err(qtest_qmp(qs, "{ 'execute': 'set-numa-node',"
+        " 'arguments': { 'type': 'hmat-lb', 'initiator': 0, 'target': 0,"
+        " 'hierarchy': \"memory\", 'data-type': \"access-bandwidth\","
+        " 'bandwidth': 0 } }")));    /* 0 MB/s */
+    /* Fail: bandwidth should be provided before memory side cache attributes */
+    g_assert(qmp_rsp_is_err(qtest_qmp(qs, "{ 'execute': 'set-numa-node',"
+        " 'arguments': { 'type': 'hmat-cache', 'node-id': 0, 'size': 10240,"
+        " 'level': 1, 'assoc': \"direct\", 'policy': \"write-back\","
+        " 'line': 8 } }")));
+
+    /* Can't store the compressed bandwidth */
+    g_assert(qmp_rsp_is_err(qtest_qmp(qs, "{ 'execute': 'set-numa-node',"
+        " 'arguments': { 'type': 'hmat-lb', 'initiator': 0, 'target': 1,"
+        " 'hierarchy': \"memory\", 'data-type': \"access-bandwidth\","
+        " 'bandwidth': 68718428160 } }")));    /* 65535 MB/s */
+
+    /* let machine initialization to complete and run */
+    g_assert(!qmp_rsp_is_err(qtest_qmp(qs,
+                                       "{ 'execute': 'x-exit-preconfig' }")));
+    qtest_qmp_eventwait(qs, "RESUME");
+
+    qtest_quit(qs);
+}
+
 int main(int argc, char **argv)
 {
     const char *args = NULL;
@@ -346,6 +556,9 @@  int main(int argc, char **argv)
     if (!strcmp(arch, "i386") || !strcmp(arch, "x86_64")) {
         qtest_add_data_func("/numa/pc/cpu/explicit", args, pc_numa_cpu);
         qtest_add_data_func("/numa/pc/dynamic/cpu", args, pc_dynamic_cpu_cfg);
+        qtest_add_data_func("/numa/pc/hmat/build", args, pc_hmat_build_cfg);
+        qtest_add_data_func("/numa/pc/hmat/off", args, pc_hmat_off_cfg);
+        qtest_add_data_func("/numa/pc/hmat/erange", args, pc_hmat_erange_cfg);
     }
 
     if (!strcmp(arch, "ppc64")) {