Re: [PATCH v6 3/7] hw/acpi/aml-build: Improve scalability of PPTT generation

[Andrew Jones]

On Mon, Jan 03, 2022 at 04:46:32PM +0800, Yanan Wang wrote:
> Currently we generate a PPTT table of n-level processor hierarchy
> with n-level loops in build_pptt(). It works fine as now there are
> only three CPU topology parameters. But the code may become less
> scalable with the processor hierarchy levels increasing.
> 
> This patch only improves the scalability of build_pptt by reducing
> the loops, and intends to make no functional change.
> 
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>  hw/acpi/aml-build.c | 50 +++++++++++++++++++++++++++++----------------
>  1 file changed, 32 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index b3b3310df3..be3851be36 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -2001,7 +2001,10 @@ static void build_processor_hierarchy_node(GArray 
> *tbl, uint32_t flags,
>  void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms,
>                  const char *oem_id, const char *oem_table_id)
>  {
> -    int pptt_start = table_data->len;
> +    GQueue *list = g_queue_new();
> +    guint pptt_start = table_data->len;
> +    guint father_offset;

"parent_offset" would be more conventional.

> +    guint length, i;
>      int uid = 0;
>      int socket;
>      AcpiTable table = { .sig = "PPTT", .rev = 2,
> @@ -2010,9 +2013,8 @@ void build_pptt(GArray *table_data, BIOSLinker *linker, 
> MachineState *ms,
>      acpi_table_begin(&table, table_data);
>  
>      for (socket = 0; socket < ms->smp.sockets; socket++) {
> -        uint32_t socket_offset = table_data->len - pptt_start;
> -        int core;
> -
> +        g_queue_push_tail(list,
> +            GUINT_TO_POINTER(table_data->len - pptt_start));
>          build_processor_hierarchy_node(
>              table_data,
>              /*
> @@ -2021,35 +2023,47 @@ void build_pptt(GArray *table_data, BIOSLinker 
> *linker, MachineState *ms,
>               */
>              (1 << 0),
>              0, socket, NULL, 0);
> +    }
>  
> -        for (core = 0; core < ms->smp.cores; core++) {
> -            uint32_t core_offset = table_data->len - pptt_start;
> -            int thread;
> +    length = g_queue_get_length(list);
> +    for (i = 0; i < length; i++) {
> +        int core;
>  
> +        father_offset = GPOINTER_TO_UINT(g_queue_pop_head(list));
> +        for (core = 0; core < ms->smp.cores; core++) {
>              if (ms->smp.threads > 1) {
> +                g_queue_push_tail(list,
> +                    GUINT_TO_POINTER(table_data->len - pptt_start));
>                  build_processor_hierarchy_node(
>                      table_data,
>                      (0 << 0), /* not a physical package */
> -                    socket_offset, core, NULL, 0);
> -
> -                for (thread = 0; thread < ms->smp.threads; thread++) {
> -                    build_processor_hierarchy_node(
> -                        table_data,
> -                        (1 << 1) | /* ACPI Processor ID valid */
> -                        (1 << 2) | /* Processor is a Thread */
> -                        (1 << 3),  /* Node is a Leaf */
> -                        core_offset, uid++, NULL, 0);
> -                }
> +                    father_offset, core, NULL, 0);
>              } else {
>                  build_processor_hierarchy_node(
>                      table_data,
>                      (1 << 1) | /* ACPI Processor ID valid */
>                      (1 << 3),  /* Node is a Leaf */
> -                    socket_offset, uid++, NULL, 0);
> +                    father_offset, uid++, NULL, 0);
>              }
>          }
>      }
>  
> +    length = g_queue_get_length(list);
> +    for (i = 0; i < length; i++) {
> +        int thread;
> +
> +        father_offset = GPOINTER_TO_UINT(g_queue_pop_head(list));
> +        for (thread = 0; thread < ms->smp.threads; thread++) {
> +            build_processor_hierarchy_node(
> +                table_data,
> +                (1 << 1) | /* ACPI Processor ID valid */
> +                (1 << 2) | /* Processor is a Thread */
> +                (1 << 3),  /* Node is a Leaf */
> +                father_offset, uid++, NULL, 0);
> +        }
> +    }
> +
> +    g_queue_free(list);
>      acpi_table_end(linker, &table);
>  }

This patch actually increases the number of loops, since we need to visit
higher hierarchical nodes twice (once to enqueue and once to dequeue). We
do reduce code indentation and it looks like we could more easily skip
hierarchy levels we don't want, though. While my impulse is to say we
should just keep this simple and add another nested loop for clusters, I
guess I'm OK with this too.

Reviewed-by: Andrew Jones <drjones@redhat.com>

Thanks,
drew