Re: [PATCH v4 2/2] tests/unit: Add an unit test for smp parsing

[wangyanan (Y)]

On 2021/11/11 17:14, Philippe Mathieu-Daudé wrote:
> On 10/28/21 17:09, Philippe Mathieu-Daudé wrote:
> > From: Yanan Wang <wangyanan55@huawei.com>
> >
> > Now that we have a generic parser smp_parse(), let's add an unit
> > test for the code. All possible valid/invalid SMP configurations
> > that the user can specify are covered.
> >
> > Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> > Reviewed-by: Andrew Jones <drjones@redhat.com>
> > Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > Message-Id: <20211026034659.22040-3-wangyanan55@huawei.com>
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > ---
> >   tests/unit/test-smp-parse.c | 594 ++++++++++++++++++++++++++++++++++++
> >   MAINTAINERS                 |   1 +
> >   tests/unit/meson.build      |   1 +
> >   3 files changed, 596 insertions(+)
> >   create mode 100644 tests/unit/test-smp-parse.c
> > +static struct SMPTestData data_generic_valid[] = {
> > +    {
> > +        /* config: no configuration provided
> > +         * expect: cpus=1,sockets=1,cores=1,threads=1,maxcpus=1 */
> [1]
>
> > +        .config = SMP_CONFIG_GENERIC(F, 0, F, 0, F, 0, F, 0, F, 0),
> > +        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(1, 1, 1, 1, 1),
> > +        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(1, 1, 1, 1, 1),
> > +    }, {
> > +static void test_generic(void)
> > +{
> > +    Object *obj = object_new(TYPE_MACHINE);
> > +    MachineState *ms = MACHINE(obj);
> > +    MachineClass *mc = MACHINE_GET_CLASS(obj);
> Watch out, while you create a machine instance in each
> test, there is only one machine class registered (see
> type_register_static(&smp_machine_info) below in [2]),
> ...
Yes, I noticed this. So on the top of each sub-test function, the properties
of the single machine class is re-initialized by smp_machine_class_init(mc).
See [*] below.
> > +    SMPTestData *data = &(SMPTestData){0};
> > +    int i;
> > +
> > +    smp_machine_class_init(mc);
[*]
> > +
> > +    for (i = 0; i < ARRAY_SIZE(data_generic_valid); i++) {
> > +        *data = data_generic_valid[i];
> > +        unsupported_params_init(mc, data);
> > +
> > +        smp_parse_test(ms, data, true);
> > +
> > +        /* Unsupported parameters can be provided with their values as 1 */
> > +        data->config.has_dies = true;
> > +        data->config.dies = 1;
> > +        smp_parse_test(ms, data, true);
> > +    }
> > +
> > +    /* Reset the supported min CPUs and max CPUs */
> > +    mc->min_cpus = 2;
> > +    mc->max_cpus = 511;
> ... and here you are modifying the single machine class state, ...
>
> > +
> > +    for (i = 0; i < ARRAY_SIZE(data_generic_invalid); i++) {
> > +        *data = data_generic_invalid[i];
> > +        unsupported_params_init(mc, data);
> > +
> > +        smp_parse_test(ms, data, false);
> > +    }
> > +
> > +    object_unref(obj);
> > +}
> > +
> > +static void test_with_dies(void)
> > +{
> > +    Object *obj = object_new(TYPE_MACHINE);
> > +    MachineState *ms = MACHINE(obj);
> > +    MachineClass *mc = MACHINE_GET_CLASS(obj);
> ... so here the machine class state is inconsistent, ...
>
> > +    SMPTestData *data = &(SMPTestData){0};
> > +    unsigned int num_dies = 2;
> > +    int i;
> > +
> > +    smp_machine_class_init(mc);
And here [*].
> > +    mc->smp_props.dies_supported = true;
> > +
> > +    for (i = 0; i < ARRAY_SIZE(data_generic_valid); i++) {
> > +        *data = data_generic_valid[i];
> > +        unsupported_params_init(mc, data);
> > +
> > +        /* when dies parameter is omitted, it will be set as 1 */
> > +        data->expect_prefer_sockets.dies = 1;
> > +        data->expect_prefer_cores.dies = 1;
> > +
> > +        smp_parse_test(ms, data, true);
> ... in particular the first test [1] is tested with mc->min_cpus = 2.
>
> I wonder why you are not getting:
>
> Output error report: Invalid SMP CPUs 1. The min CPUs supported by
> machine '(null)' is 2
>
> for [1].
So as I have explained above, we won't get an output error report like 
this here. :)

Thanks,
Yanan
> > +
> > +        /* when dies parameter is specified */
> > +        data->config.has_dies = true;
> > +        data->config.dies = num_dies;
> > +        if (data->config.has_cpus) {
> > +            data->config.cpus *= num_dies;
> > +        }
> > +        if (data->config.has_maxcpus) {
> > +            data->config.maxcpus *= num_dies;
> > +        }
> > +
> > +        data->expect_prefer_sockets.dies = num_dies;
> > +        data->expect_prefer_sockets.cpus *= num_dies;
> > +        data->expect_prefer_sockets.max_cpus *= num_dies;
> > +        data->expect_prefer_cores.dies = num_dies;
> > +        data->expect_prefer_cores.cpus *= num_dies;
> > +        data->expect_prefer_cores.max_cpus *= num_dies;
> > +
> > +        smp_parse_test(ms, data, true);
> > +    }
> > +
> > +    for (i = 0; i < ARRAY_SIZE(data_with_dies_invalid); i++) {
> > +        *data = data_with_dies_invalid[i];
> > +        unsupported_params_init(mc, data);
> > +
> > +        smp_parse_test(ms, data, false);
> > +    }
> > +
> > +    object_unref(obj);
> > +}
> > +
> > +int main(int argc, char *argv[])
> > +{
> > +    g_test_init(&argc, &argv, NULL);
> > +
> > +    module_call_init(MODULE_INIT_QOM);
> > +    type_register_static(&smp_machine_info);
> [2]
>
> .