Re: [PATCH-for-7.0 v4 08/11] tests/unit/test-smp-parse: Add 'smp-without-dies-valid' machine type

2021-11-17 Thread wangyanan (Y) 



On 2021/11/17 16:08, Philippe Mathieu-Daudé wrote:

Hi Yanan,

On 11/17/21 08:37, wangyanan (Y) wrote:

On 2021/11/15 22:58, Philippe Mathieu-Daudé wrote:

Keep the common TYPE_MACHINE class initialization in
machine_base_class_init(), make it abstract, and move
the non-common code to a new class: "smp-without-dies-valid".

Signed-off-by: Philippe Mathieu-Daudé 
---
   tests/unit/test-smp-parse.c | 19 +++
   1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index dfe7f1313b0..90a249fe8c8 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -478,13 +478,19 @@ static void machine_base_class_init(ObjectClass
*oc, void *data)
   {
   MachineClass *mc = MACHINE_CLASS(oc);
   +    mc->smp_props.prefer_sockets = true;
+
+    mc->name = g_strdup(SMP_MACHINE_NAME);
+}
+
+static void machine_without_dies_valid_class_init(ObjectClass *oc,
void *data)
+{
+    MachineClass *mc = MACHINE_CLASS(oc);
+
   mc->min_cpus = MIN_CPUS;
   mc->max_cpus = MAX_CPUS;
   -    mc->smp_props.prefer_sockets = true;
   mc->smp_props.dies_supported = false;
-
-    mc->name = g_strdup(SMP_MACHINE_NAME);
   }
     static void machine_without_dies_invalid_class_init(ObjectClass
*oc, void *data)
@@ -606,9 +612,14 @@ static const TypeInfo smp_machine_types[] = {
   {
   .name   = TYPE_MACHINE,
   .parent = TYPE_OBJECT,
+    .abstract   = true,
   .class_init = machine_base_class_init,
   .class_size = sizeof(MachineClass),
   .instance_size  = sizeof(MachineState),
+    }, {
+    .name   = MACHINE_TYPE_NAME("smp-without-dies-valid"),
+    .parent = TYPE_MACHINE,
+    .class_init = machine_without_dies_valid_class_init,
   }, {
   .name   =
MACHINE_TYPE_NAME("smp-without-dies-invalid"),
   .parent = TYPE_MACHINE,
@@ -629,7 +640,7 @@ int main(int argc, char *argv[])
   g_test_init(, , NULL);
     g_test_add_data_func("/test-smp-parse/generic/valid",
- TYPE_MACHINE,
+ MACHINE_TYPE_NAME("smp-without-dies-valid"),
    test_generic_valid);
   g_test_add_data_func("/test-smp-parse/generic/invalid",
    MACHINE_TYPE_NAME("smp-without-dies-invalid"),

After patch #7 and #8, we will have sub-tests as below. It looks nice,
but it will
probably be better to tweak "smp-without-dies-valid" to
"smp-generic-valid",
and "smp-without-dies-invalid" to "smp-generic-invalid", which will be more
consistent with the corresponding sub-test name.

It's Ok now as we only have dies currently besides generic
sockets/cores/threads,
but "smp-without-dies-xxx" will become a bit confusing when new topology
members are introduced and tested here.

OK I modified it and will respin once v6.2 is released.

Also test_with_dies() should be split in 2 tests: valid/invalid;
then smp_parse_test() should split tests further regarding the
socket preference. But I'll let that to you,

Sure, I can do this in an appropriate time. But IMHO, I don't see a
strong necessity to split test_with_dies for now, as the valid/invalid
scenarios share the same class properties. We can probably keep it
as is until we have to split it.

As for socket preference, I can foresee code duplication if we split
all the tests into two parts just regarding the socket preference.
Isn't it clear enough to use current smp_parse_test() for each
test data sample? Do we have other concern here?

I wanted to 1/ fix
our Windows CI and 2/ show you how to structure the tests.

Understood. The test architecture is indeed improved a lot.
Thanks for your education. 

Thanks,
Yanan

Regards,

Phil.

.

Re: [PATCH-for-7.0 v4 08/11] tests/unit/test-smp-parse: Add 'smp-without-dies-valid' machine type

2021-11-17 Thread Philippe Mathieu-Daudé 
Hi Yanan,

On 11/17/21 08:37, wangyanan (Y) wrote:
> On 2021/11/15 22:58, Philippe Mathieu-Daudé wrote:
>> Keep the common TYPE_MACHINE class initialization in
>> machine_base_class_init(), make it abstract, and move
>> the non-common code to a new class: "smp-without-dies-valid".
>>
>> Signed-off-by: Philippe Mathieu-Daudé 
>> ---
>>   tests/unit/test-smp-parse.c | 19 +++
>>   1 file changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
>> index dfe7f1313b0..90a249fe8c8 100644
>> --- a/tests/unit/test-smp-parse.c
>> +++ b/tests/unit/test-smp-parse.c
>> @@ -478,13 +478,19 @@ static void machine_base_class_init(ObjectClass
>> *oc, void *data)
>>   {
>>   MachineClass *mc = MACHINE_CLASS(oc);
>>   +    mc->smp_props.prefer_sockets = true;
>> +
>> +    mc->name = g_strdup(SMP_MACHINE_NAME);
>> +}
>> +
>> +static void machine_without_dies_valid_class_init(ObjectClass *oc,
>> void *data)
>> +{
>> +    MachineClass *mc = MACHINE_CLASS(oc);
>> +
>>   mc->min_cpus = MIN_CPUS;
>>   mc->max_cpus = MAX_CPUS;
>>   -    mc->smp_props.prefer_sockets = true;
>>   mc->smp_props.dies_supported = false;
>> -
>> -    mc->name = g_strdup(SMP_MACHINE_NAME);
>>   }
>>     static void machine_without_dies_invalid_class_init(ObjectClass
>> *oc, void *data)
>> @@ -606,9 +612,14 @@ static const TypeInfo smp_machine_types[] = {
>>   {
>>   .name   = TYPE_MACHINE,
>>   .parent = TYPE_OBJECT,
>> +    .abstract   = true,
>>   .class_init = machine_base_class_init,
>>   .class_size = sizeof(MachineClass),
>>   .instance_size  = sizeof(MachineState),
>> +    }, {
>> +    .name   = MACHINE_TYPE_NAME("smp-without-dies-valid"),
>> +    .parent = TYPE_MACHINE,
>> +    .class_init = machine_without_dies_valid_class_init,
>>   }, {
>>   .name   =
>> MACHINE_TYPE_NAME("smp-without-dies-invalid"),
>>   .parent = TYPE_MACHINE,
>> @@ -629,7 +640,7 @@ int main(int argc, char *argv[])
>>   g_test_init(, , NULL);
>>     g_test_add_data_func("/test-smp-parse/generic/valid",
>> - TYPE_MACHINE,
>> + MACHINE_TYPE_NAME("smp-without-dies-valid"),
>>    test_generic_valid);
>>   g_test_add_data_func("/test-smp-parse/generic/invalid",
>>    MACHINE_TYPE_NAME("smp-without-dies-invalid"),
> After patch #7 and #8, we will have sub-tests as below. It looks nice,
> but it will
> probably be better to tweak "smp-without-dies-valid" to
> "smp-generic-valid",
> and "smp-without-dies-invalid" to "smp-generic-invalid", which will be more
> consistent with the corresponding sub-test name.
> 
> It's Ok now as we only have dies currently besides generic
> sockets/cores/threads,
> but "smp-without-dies-xxx" will become a bit confusing when new topology
> members are introduced and tested here.

OK I modified it and will respin once v6.2 is released.

Also test_with_dies() should be split in 2 tests: valid/invalid;
then smp_parse_test() should split tests further regarding the
socket preference. But I'll let that to you, I wanted to 1/ fix
our Windows CI and 2/ show you how to structure the tests.

Regards,

Phil.

Re: [PATCH-for-7.0 v4 08/11] tests/unit/test-smp-parse: Add 'smp-without-dies-valid' machine type

2021-11-16 Thread wangyanan (Y) 

Hi Philippe,

On 2021/11/15 22:58, Philippe Mathieu-Daudé wrote:

Keep the common TYPE_MACHINE class initialization in
machine_base_class_init(), make it abstract, and move
the non-common code to a new class: "smp-without-dies-valid".

Signed-off-by: Philippe Mathieu-Daudé 
---
  tests/unit/test-smp-parse.c | 19 +++
  1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index dfe7f1313b0..90a249fe8c8 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -478,13 +478,19 @@ static void machine_base_class_init(ObjectClass *oc, void 
*data)
  {
  MachineClass *mc = MACHINE_CLASS(oc);
  
+mc->smp_props.prefer_sockets = true;

+
+mc->name = g_strdup(SMP_MACHINE_NAME);
+}
+
+static void machine_without_dies_valid_class_init(ObjectClass *oc, void *data)
+{
+MachineClass *mc = MACHINE_CLASS(oc);
+
  mc->min_cpus = MIN_CPUS;
  mc->max_cpus = MAX_CPUS;
  
-mc->smp_props.prefer_sockets = true;

  mc->smp_props.dies_supported = false;
-
-mc->name = g_strdup(SMP_MACHINE_NAME);
  }
  
  static void machine_without_dies_invalid_class_init(ObjectClass *oc, void *data)

@@ -606,9 +612,14 @@ static const TypeInfo smp_machine_types[] = {
  {
  .name   = TYPE_MACHINE,
  .parent = TYPE_OBJECT,
+.abstract   = true,
  .class_init = machine_base_class_init,
  .class_size = sizeof(MachineClass),
  .instance_size  = sizeof(MachineState),
+}, {
+.name   = MACHINE_TYPE_NAME("smp-without-dies-valid"),
+.parent = TYPE_MACHINE,
+.class_init = machine_without_dies_valid_class_init,
  }, {
  .name   = MACHINE_TYPE_NAME("smp-without-dies-invalid"),
  .parent = TYPE_MACHINE,
@@ -629,7 +640,7 @@ int main(int argc, char *argv[])
  g_test_init(, , NULL);
  
  g_test_add_data_func("/test-smp-parse/generic/valid",

- TYPE_MACHINE,
+ MACHINE_TYPE_NAME("smp-without-dies-valid"),
   test_generic_valid);
  g_test_add_data_func("/test-smp-parse/generic/invalid",
   MACHINE_TYPE_NAME("smp-without-dies-invalid"),
After patch #7 and #8, we will have sub-tests as below. It looks nice, 
but it will

probably be better to tweak "smp-without-dies-valid" to "smp-generic-valid",
and "smp-without-dies-invalid" to "smp-generic-invalid", which will be more
consistent with the corresponding sub-test name.

It's Ok now as we only have dies currently besides generic 
sockets/cores/threads,

but "smp-without-dies-xxx" will become a bit confusing when new topology
members are introduced and tested here.

int main(int argc, char *argv[])
{
module_call_init(MODULE_INIT_QOM);

    g_test_init(, , NULL);

g_test_add_data_func("/test-smp-parse/generic/valid",
MACHINE_TYPE_NAME("smp-without-dies-valid"),
test_generic_valid);
g_test_add_data_func("/test-smp-parse/generic/invalid",
MACHINE_TYPE_NAME("smp-without-dies-invalid"),
test_generic_invalid);
g_test_add_data_func("/test-smp-parse/with_dies",
MACHINE_TYPE_NAME("smp-with-dies"),
test_with_dies);

g_test_run();

    return 0;
}

Otherwise, #7 and #8 look nice. Thanks for your work!

Thanks,
Yanan

Re: [PATCH-for-7.0 v4 08/11] tests/unit/test-smp-parse: Add 'smp-without-dies-valid' machine type

2021-11-16 Thread Richard Henderson 

On 11/15/21 3:58 PM, Philippe Mathieu-Daudé wrote:

Keep the common TYPE_MACHINE class initialization in
machine_base_class_init(), make it abstract, and move
the non-common code to a new class: "smp-without-dies-valid".

Signed-off-by: Philippe Mathieu-Daudé
---
  tests/unit/test-smp-parse.c | 19 +++
  1 file changed, 15 insertions(+), 4 deletions(-)


Reviewed-by: Richard Henderson 

r~

[PATCH-for-7.0 v4 08/11] tests/unit/test-smp-parse: Add 'smp-without-dies-valid' machine type

2021-11-15 Thread Philippe Mathieu-Daudé 
Keep the common TYPE_MACHINE class initialization in
machine_base_class_init(), make it abstract, and move
the non-common code to a new class: "smp-without-dies-valid".

Signed-off-by: Philippe Mathieu-Daudé 
---
 tests/unit/test-smp-parse.c | 19 +++
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index dfe7f1313b0..90a249fe8c8 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -478,13 +478,19 @@ static void machine_base_class_init(ObjectClass *oc, void 
*data)
 {
 MachineClass *mc = MACHINE_CLASS(oc);
 
+mc->smp_props.prefer_sockets = true;
+
+mc->name = g_strdup(SMP_MACHINE_NAME);
+}
+
+static void machine_without_dies_valid_class_init(ObjectClass *oc, void *data)
+{
+MachineClass *mc = MACHINE_CLASS(oc);
+
 mc->min_cpus = MIN_CPUS;
 mc->max_cpus = MAX_CPUS;
 
-mc->smp_props.prefer_sockets = true;
 mc->smp_props.dies_supported = false;
-
-mc->name = g_strdup(SMP_MACHINE_NAME);
 }
 
 static void machine_without_dies_invalid_class_init(ObjectClass *oc, void 
*data)
@@ -606,9 +612,14 @@ static const TypeInfo smp_machine_types[] = {
 {
 .name   = TYPE_MACHINE,
 .parent = TYPE_OBJECT,
+.abstract   = true,
 .class_init = machine_base_class_init,
 .class_size = sizeof(MachineClass),
 .instance_size  = sizeof(MachineState),
+}, {
+.name   = MACHINE_TYPE_NAME("smp-without-dies-valid"),
+.parent = TYPE_MACHINE,
+.class_init = machine_without_dies_valid_class_init,
 }, {
 .name   = MACHINE_TYPE_NAME("smp-without-dies-invalid"),
 .parent = TYPE_MACHINE,
@@ -629,7 +640,7 @@ int main(int argc, char *argv[])
 g_test_init(, , NULL);
 
 g_test_add_data_func("/test-smp-parse/generic/valid",
- TYPE_MACHINE,
+ MACHINE_TYPE_NAME("smp-without-dies-valid"),
  test_generic_valid);
 g_test_add_data_func("/test-smp-parse/generic/invalid",
  MACHINE_TYPE_NAME("smp-without-dies-invalid"),
-- 
2.31.1