* Re: [PATCH] spapr_numa.c: fixes in spapr_numa_FORM2_write_rtas_tables()
2021-09-21 19:43 [PATCH] spapr_numa.c: fixes in spapr_numa_FORM2_write_rtas_tables() Daniel Henrique Barboza
@ 2021-09-22 3:08 ` David Gibson
2021-09-22 8:26 ` Greg Kurz
2021-09-22 11:17 ` Philippe Mathieu-Daudé
2 siblings, 0 replies; 10+ messages in thread
From: David Gibson @ 2021-09-22 3:08 UTC (permalink / raw)
To: Daniel Henrique Barboza; +Cc: qemu-ppc, qemu-devel, groug
[-- Attachment #1: Type: text/plain, Size: 4851 bytes --]
On Tue, Sep 21, 2021 at 04:43:47PM -0300, Daniel Henrique Barboza wrote:
> This patch has a handful of modifications for the recent added
> FORM2 support:
>
> - there is no particular reason for both 'lookup_index_table' and
> 'distance_table' to be allocated in the heap, since their sizes are
> known right at the start of the function. Use static allocation in
> them to spare a couple of g_new0() calls;
>
> - to not allocate more than the necessary size in 'distance_table'. At
> this moment the array is oversized due to allocating uint32_t for all
> elements, when most of them fits in an uint8_t;
>
> - create a NUMA_LOCAL_DISTANCE macro to avoid hardcoding the local
> distance value.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Applied to ppc-fot-6.2, thanks.
> ---
> hw/ppc/spapr_numa.c | 35 +++++++++++++++++++----------------
> 1 file changed, 19 insertions(+), 16 deletions(-)
>
> diff --git a/hw/ppc/spapr_numa.c b/hw/ppc/spapr_numa.c
> index 58d5dc7084..039a0439c6 100644
> --- a/hw/ppc/spapr_numa.c
> +++ b/hw/ppc/spapr_numa.c
> @@ -19,6 +19,9 @@
> /* Moved from hw/ppc/spapr_pci_nvlink2.c */
> #define SPAPR_GPU_NUMA_ID (cpu_to_be32(1))
>
> +/* Macro to avoid hardcoding the local distance value */
> +#define NUMA_LOCAL_DISTANCE 10
> +
> /*
> * Retrieves max_dist_ref_points of the current NUMA affinity.
> */
> @@ -500,17 +503,21 @@ static void spapr_numa_FORM2_write_rtas_tables(SpaprMachineState *spapr,
> MachineState *ms = MACHINE(spapr);
> NodeInfo *numa_info = ms->numa_state->nodes;
> int nb_numa_nodes = ms->numa_state->num_nodes;
> + /* Lookup index table has an extra uint32_t with its length */
> + uint32_t lookup_index_table[nb_numa_nodes + 1];
> int distance_table_entries = nb_numa_nodes * nb_numa_nodes;
> - g_autofree uint32_t *lookup_index_table = NULL;
> - g_autofree uint32_t *distance_table = NULL;
> - int src, dst, i, distance_table_size;
> - uint8_t *node_distances;
> + /*
> + * Distance table is an uint8_t array with a leading uint32_t
> + * containing its length.
> + */
> + uint8_t distance_table[distance_table_entries + 4];
> + uint32_t *distance_table_length;
> + int src, dst, i;
>
> /*
> * ibm,numa-lookup-index-table: array with length and a
> * list of NUMA ids present in the guest.
> */
> - lookup_index_table = g_new0(uint32_t, nb_numa_nodes + 1);
> lookup_index_table[0] = cpu_to_be32(nb_numa_nodes);
>
> for (i = 0; i < nb_numa_nodes; i++) {
> @@ -518,8 +525,7 @@ static void spapr_numa_FORM2_write_rtas_tables(SpaprMachineState *spapr,
> }
>
> _FDT(fdt_setprop(fdt, rtas, "ibm,numa-lookup-index-table",
> - lookup_index_table,
> - (nb_numa_nodes + 1) * sizeof(uint32_t)));
> + lookup_index_table, sizeof(lookup_index_table)));
>
> /*
> * ibm,numa-distance-table: contains all node distances. First
> @@ -531,11 +537,10 @@ static void spapr_numa_FORM2_write_rtas_tables(SpaprMachineState *spapr,
> * array because NUMA ids can be sparse (node 0 is the first,
> * node 8 is the second ...).
> */
> - distance_table = g_new0(uint32_t, distance_table_entries + 1);
> - distance_table[0] = cpu_to_be32(distance_table_entries);
> + distance_table_length = (uint32_t *)distance_table;
> + distance_table_length[0] = cpu_to_be32(distance_table_entries);
>
> - node_distances = (uint8_t *)&distance_table[1];
> - i = 0;
> + i = 4;
>
> for (src = 0; src < nb_numa_nodes; src++) {
> for (dst = 0; dst < nb_numa_nodes; dst++) {
> @@ -546,18 +551,16 @@ static void spapr_numa_FORM2_write_rtas_tables(SpaprMachineState *spapr,
> * adding the numa_info to retrieve distance info from.
> */
> if (src == dst) {
> - node_distances[i++] = 10;
> + distance_table[i++] = NUMA_LOCAL_DISTANCE;
> continue;
> }
>
> - node_distances[i++] = numa_info[src].distance[dst];
> + distance_table[i++] = numa_info[src].distance[dst];
> }
> }
>
> - distance_table_size = distance_table_entries * sizeof(uint8_t) +
> - sizeof(uint32_t);
> _FDT(fdt_setprop(fdt, rtas, "ibm,numa-distance-table",
> - distance_table, distance_table_size));
> + distance_table, sizeof(distance_table)));
> }
>
> /*
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] spapr_numa.c: fixes in spapr_numa_FORM2_write_rtas_tables()
2021-09-21 19:43 [PATCH] spapr_numa.c: fixes in spapr_numa_FORM2_write_rtas_tables() Daniel Henrique Barboza
2021-09-22 3:08 ` David Gibson
@ 2021-09-22 8:26 ` Greg Kurz
2021-09-22 9:51 ` BALATON Zoltan
2021-09-22 11:17 ` Philippe Mathieu-Daudé
2 siblings, 1 reply; 10+ messages in thread
From: Greg Kurz @ 2021-09-22 8:26 UTC (permalink / raw)
To: Daniel Henrique Barboza; +Cc: qemu-ppc, qemu-devel, david
On Tue, 21 Sep 2021 16:43:47 -0300
Daniel Henrique Barboza <danielhb413@gmail.com> wrote:
> This patch has a handful of modifications for the recent added
> FORM2 support:
>
> - there is no particular reason for both 'lookup_index_table' and
> 'distance_table' to be allocated in the heap, since their sizes are
> known right at the start of the function. Use static allocation in
> them to spare a couple of g_new0() calls;
>
> - to not allocate more than the necessary size in 'distance_table'. At
> this moment the array is oversized due to allocating uint32_t for all
> elements, when most of them fits in an uint8_t;
>
> - create a NUMA_LOCAL_DISTANCE macro to avoid hardcoding the local
> distance value.
>
Not needed. A notion of minimal distance, which is obviously
synonymous to local, already exists in the "sysemu/numa.h"
header :
#define NUMA_DISTANCE_MIN 10
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
> hw/ppc/spapr_numa.c | 35 +++++++++++++++++++----------------
> 1 file changed, 19 insertions(+), 16 deletions(-)
>
> diff --git a/hw/ppc/spapr_numa.c b/hw/ppc/spapr_numa.c
> index 58d5dc7084..039a0439c6 100644
> --- a/hw/ppc/spapr_numa.c
> +++ b/hw/ppc/spapr_numa.c
> @@ -19,6 +19,9 @@
> /* Moved from hw/ppc/spapr_pci_nvlink2.c */
> #define SPAPR_GPU_NUMA_ID (cpu_to_be32(1))
>
> +/* Macro to avoid hardcoding the local distance value */
> +#define NUMA_LOCAL_DISTANCE 10
> +
> /*
> * Retrieves max_dist_ref_points of the current NUMA affinity.
> */
> @@ -500,17 +503,21 @@ static void spapr_numa_FORM2_write_rtas_tables(SpaprMachineState *spapr,
> MachineState *ms = MACHINE(spapr);
> NodeInfo *numa_info = ms->numa_state->nodes;
> int nb_numa_nodes = ms->numa_state->num_nodes;
> + /* Lookup index table has an extra uint32_t with its length */
> + uint32_t lookup_index_table[nb_numa_nodes + 1];
> int distance_table_entries = nb_numa_nodes * nb_numa_nodes;
> - g_autofree uint32_t *lookup_index_table = NULL;
> - g_autofree uint32_t *distance_table = NULL;
> - int src, dst, i, distance_table_size;
> - uint8_t *node_distances;
> + /*
> + * Distance table is an uint8_t array with a leading uint32_t
> + * containing its length.
> + */
> + uint8_t distance_table[distance_table_entries + 4];
> + uint32_t *distance_table_length;
> + int src, dst, i;
>
> /*
> * ibm,numa-lookup-index-table: array with length and a
> * list of NUMA ids present in the guest.
> */
> - lookup_index_table = g_new0(uint32_t, nb_numa_nodes + 1);
> lookup_index_table[0] = cpu_to_be32(nb_numa_nodes);
>
> for (i = 0; i < nb_numa_nodes; i++) {
> @@ -518,8 +525,7 @@ static void spapr_numa_FORM2_write_rtas_tables(SpaprMachineState *spapr,
> }
>
> _FDT(fdt_setprop(fdt, rtas, "ibm,numa-lookup-index-table",
> - lookup_index_table,
> - (nb_numa_nodes + 1) * sizeof(uint32_t)));
> + lookup_index_table, sizeof(lookup_index_table)));
>
> /*
> * ibm,numa-distance-table: contains all node distances. First
> @@ -531,11 +537,10 @@ static void spapr_numa_FORM2_write_rtas_tables(SpaprMachineState *spapr,
> * array because NUMA ids can be sparse (node 0 is the first,
> * node 8 is the second ...).
> */
> - distance_table = g_new0(uint32_t, distance_table_entries + 1);
> - distance_table[0] = cpu_to_be32(distance_table_entries);
> + distance_table_length = (uint32_t *)distance_table;
> + distance_table_length[0] = cpu_to_be32(distance_table_entries);
>
> - node_distances = (uint8_t *)&distance_table[1];
> - i = 0;
> + i = 4;
>
A comment reminding why we're doing that wouldn't hurt, e.g.
/* Skip the array size (uint32_t) */
With these fixed, especially using NUMA_DISTANCE_MIN, you
can add:
Reviewed-by: Greg Kurz <groug@kaod.org>
> for (src = 0; src < nb_numa_nodes; src++) {
> for (dst = 0; dst < nb_numa_nodes; dst++) {
> @@ -546,18 +551,16 @@ static void spapr_numa_FORM2_write_rtas_tables(SpaprMachineState *spapr,
> * adding the numa_info to retrieve distance info from.
> */
> if (src == dst) {
> - node_distances[i++] = 10;
> + distance_table[i++] = NUMA_LOCAL_DISTANCE;
> continue;
> }
>
> - node_distances[i++] = numa_info[src].distance[dst];
> + distance_table[i++] = numa_info[src].distance[dst];
> }
> }
>
> - distance_table_size = distance_table_entries * sizeof(uint8_t) +
> - sizeof(uint32_t);
> _FDT(fdt_setprop(fdt, rtas, "ibm,numa-distance-table",
> - distance_table, distance_table_size));
> + distance_table, sizeof(distance_table)));
> }
>
> /*
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] spapr_numa.c: fixes in spapr_numa_FORM2_write_rtas_tables()
2021-09-22 8:26 ` Greg Kurz
@ 2021-09-22 9:51 ` BALATON Zoltan
2021-09-22 11:00 ` Daniel Henrique Barboza
0 siblings, 1 reply; 10+ messages in thread
From: BALATON Zoltan @ 2021-09-22 9:51 UTC (permalink / raw)
To: Greg Kurz; +Cc: Daniel Henrique Barboza, qemu-ppc, qemu-devel, david
On Wed, 22 Sep 2021, Greg Kurz wrote:
> On Tue, 21 Sep 2021 16:43:47 -0300
> Daniel Henrique Barboza <danielhb413@gmail.com> wrote:
>
>> This patch has a handful of modifications for the recent added
>> FORM2 support:
>>
>> - there is no particular reason for both 'lookup_index_table' and
>> 'distance_table' to be allocated in the heap, since their sizes are
>> known right at the start of the function. Use static allocation in
>> them to spare a couple of g_new0() calls;
>>
>> - to not allocate more than the necessary size in 'distance_table'. At
>> this moment the array is oversized due to allocating uint32_t for all
>> elements, when most of them fits in an uint8_t;
>>
>> - create a NUMA_LOCAL_DISTANCE macro to avoid hardcoding the local
>> distance value.
>>
>
> Not needed. A notion of minimal distance, which is obviously
> synonymous to local, already exists in the "sysemu/numa.h"
> header :
>
> #define NUMA_DISTANCE_MIN 10
>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>> hw/ppc/spapr_numa.c | 35 +++++++++++++++++++----------------
>> 1 file changed, 19 insertions(+), 16 deletions(-)
>>
>> diff --git a/hw/ppc/spapr_numa.c b/hw/ppc/spapr_numa.c
>> index 58d5dc7084..039a0439c6 100644
>> --- a/hw/ppc/spapr_numa.c
>> +++ b/hw/ppc/spapr_numa.c
>> @@ -19,6 +19,9 @@
>> /* Moved from hw/ppc/spapr_pci_nvlink2.c */
>> #define SPAPR_GPU_NUMA_ID (cpu_to_be32(1))
>>
>> +/* Macro to avoid hardcoding the local distance value */
>> +#define NUMA_LOCAL_DISTANCE 10
>> +
>> /*
>> * Retrieves max_dist_ref_points of the current NUMA affinity.
>> */
>> @@ -500,17 +503,21 @@ static void spapr_numa_FORM2_write_rtas_tables(SpaprMachineState *spapr,
>> MachineState *ms = MACHINE(spapr);
>> NodeInfo *numa_info = ms->numa_state->nodes;
>> int nb_numa_nodes = ms->numa_state->num_nodes;
>> + /* Lookup index table has an extra uint32_t with its length */
>> + uint32_t lookup_index_table[nb_numa_nodes + 1];
>> int distance_table_entries = nb_numa_nodes * nb_numa_nodes;
>> - g_autofree uint32_t *lookup_index_table = NULL;
>> - g_autofree uint32_t *distance_table = NULL;
>> - int src, dst, i, distance_table_size;
>> - uint8_t *node_distances;
>> + /*
>> + * Distance table is an uint8_t array with a leading uint32_t
>> + * containing its length.
>> + */
>> + uint8_t distance_table[distance_table_entries + 4];
>> + uint32_t *distance_table_length;
>> + int src, dst, i;
>>
>> /*
>> * ibm,numa-lookup-index-table: array with length and a
>> * list of NUMA ids present in the guest.
>> */
>> - lookup_index_table = g_new0(uint32_t, nb_numa_nodes + 1);
>> lookup_index_table[0] = cpu_to_be32(nb_numa_nodes);
>>
>> for (i = 0; i < nb_numa_nodes; i++) {
>> @@ -518,8 +525,7 @@ static void spapr_numa_FORM2_write_rtas_tables(SpaprMachineState *spapr,
>> }
>>
>> _FDT(fdt_setprop(fdt, rtas, "ibm,numa-lookup-index-table",
>> - lookup_index_table,
>> - (nb_numa_nodes + 1) * sizeof(uint32_t)));
>> + lookup_index_table, sizeof(lookup_index_table)));
>>
>> /*
>> * ibm,numa-distance-table: contains all node distances. First
>> @@ -531,11 +537,10 @@ static void spapr_numa_FORM2_write_rtas_tables(SpaprMachineState *spapr,
>> * array because NUMA ids can be sparse (node 0 is the first,
>> * node 8 is the second ...).
>> */
>> - distance_table = g_new0(uint32_t, distance_table_entries + 1);
>> - distance_table[0] = cpu_to_be32(distance_table_entries);
>> + distance_table_length = (uint32_t *)distance_table;
>> + distance_table_length[0] = cpu_to_be32(distance_table_entries);
>>
>> - node_distances = (uint8_t *)&distance_table[1];
>> - i = 0;
>> + i = 4;
>>
>
> A comment reminding why we're doing that wouldn't hurt, e.g.
>
> /* Skip the array size (uint32_t) */
Then maybe instead of (or in addition to) a comment you could write
sizeof(uint32_t) or sizeof(distance_rable[0]) instead of constant 4 to
make this more explicit.
Regards,
BALATON Zoltan
> With these fixed, especially using NUMA_DISTANCE_MIN, you
> can add:
>
> Reviewed-by: Greg Kurz <groug@kaod.org>
>
>> for (src = 0; src < nb_numa_nodes; src++) {
>> for (dst = 0; dst < nb_numa_nodes; dst++) {
>> @@ -546,18 +551,16 @@ static void spapr_numa_FORM2_write_rtas_tables(SpaprMachineState *spapr,
>> * adding the numa_info to retrieve distance info from.
>> */
>> if (src == dst) {
>> - node_distances[i++] = 10;
>> + distance_table[i++] = NUMA_LOCAL_DISTANCE;
>> continue;
>> }
>>
>> - node_distances[i++] = numa_info[src].distance[dst];
>> + distance_table[i++] = numa_info[src].distance[dst];
>> }
>> }
>>
>> - distance_table_size = distance_table_entries * sizeof(uint8_t) +
>> - sizeof(uint32_t);
>> _FDT(fdt_setprop(fdt, rtas, "ibm,numa-distance-table",
>> - distance_table, distance_table_size));
>> + distance_table, sizeof(distance_table)));
>> }
>>
>> /*
>
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] spapr_numa.c: fixes in spapr_numa_FORM2_write_rtas_tables()
2021-09-22 9:51 ` BALATON Zoltan
@ 2021-09-22 11:00 ` Daniel Henrique Barboza
2021-09-22 11:10 ` BALATON Zoltan
0 siblings, 1 reply; 10+ messages in thread
From: Daniel Henrique Barboza @ 2021-09-22 11:00 UTC (permalink / raw)
To: BALATON Zoltan, Greg Kurz; +Cc: qemu-ppc, qemu-devel, david
On 9/22/21 06:51, BALATON Zoltan wrote:
> On Wed, 22 Sep 2021, Greg Kurz wrote:
>> On Tue, 21 Sep 2021 16:43:47 -0300
>> Daniel Henrique Barboza <danielhb413@gmail.com> wrote:
>>
>>> This patch has a handful of modifications for the recent added
>>> FORM2 support:
>>>
>>> - there is no particular reason for both 'lookup_index_table' and
>>> 'distance_table' to be allocated in the heap, since their sizes are
>>> known right at the start of the function. Use static allocation in
>>> them to spare a couple of g_new0() calls;
>>>
>>> - to not allocate more than the necessary size in 'distance_table'. At
>>> this moment the array is oversized due to allocating uint32_t for all
>>> elements, when most of them fits in an uint8_t;
>>>
>>> - create a NUMA_LOCAL_DISTANCE macro to avoid hardcoding the local
>>> distance value.
>>>
>>
>> Not needed. A notion of minimal distance, which is obviously
>> synonymous to local, already exists in the "sysemu/numa.h"
>> header :
>>
>> #define NUMA_DISTANCE_MIN 10
>>
>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>> ---
>>> hw/ppc/spapr_numa.c | 35 +++++++++++++++++++----------------
>>> 1 file changed, 19 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/hw/ppc/spapr_numa.c b/hw/ppc/spapr_numa.c
>>> index 58d5dc7084..039a0439c6 100644
>>> --- a/hw/ppc/spapr_numa.c
>>> +++ b/hw/ppc/spapr_numa.c
>>> @@ -19,6 +19,9 @@
>>> /* Moved from hw/ppc/spapr_pci_nvlink2.c */
>>> #define SPAPR_GPU_NUMA_ID (cpu_to_be32(1))
>>>
>>> +/* Macro to avoid hardcoding the local distance value */
>>> +#define NUMA_LOCAL_DISTANCE 10
>>> +
>>> /*
>>> * Retrieves max_dist_ref_points of the current NUMA affinity.
>>> */
>>> @@ -500,17 +503,21 @@ static void spapr_numa_FORM2_write_rtas_tables(SpaprMachineState *spapr,
>>> MachineState *ms = MACHINE(spapr);
>>> NodeInfo *numa_info = ms->numa_state->nodes;
>>> int nb_numa_nodes = ms->numa_state->num_nodes;
>>> + /* Lookup index table has an extra uint32_t with its length */
>>> + uint32_t lookup_index_table[nb_numa_nodes + 1];
>>> int distance_table_entries = nb_numa_nodes * nb_numa_nodes;
>>> - g_autofree uint32_t *lookup_index_table = NULL;
>>> - g_autofree uint32_t *distance_table = NULL;
>>> - int src, dst, i, distance_table_size;
>>> - uint8_t *node_distances;
>>> + /*
>>> + * Distance table is an uint8_t array with a leading uint32_t
>>> + * containing its length.
>>> + */
>>> + uint8_t distance_table[distance_table_entries + 4];
>>> + uint32_t *distance_table_length;
>>> + int src, dst, i;
>>>
>>> /*
>>> * ibm,numa-lookup-index-table: array with length and a
>>> * list of NUMA ids present in the guest.
>>> */
>>> - lookup_index_table = g_new0(uint32_t, nb_numa_nodes + 1);
>>> lookup_index_table[0] = cpu_to_be32(nb_numa_nodes);
>>>
>>> for (i = 0; i < nb_numa_nodes; i++) {
>>> @@ -518,8 +525,7 @@ static void spapr_numa_FORM2_write_rtas_tables(SpaprMachineState *spapr,
>>> }
>>>
>>> _FDT(fdt_setprop(fdt, rtas, "ibm,numa-lookup-index-table",
>>> - lookup_index_table,
>>> - (nb_numa_nodes + 1) * sizeof(uint32_t)));
>>> + lookup_index_table, sizeof(lookup_index_table)));
>>>
>>> /*
>>> * ibm,numa-distance-table: contains all node distances. First
>>> @@ -531,11 +537,10 @@ static void spapr_numa_FORM2_write_rtas_tables(SpaprMachineState *spapr,
>>> * array because NUMA ids can be sparse (node 0 is the first,
>>> * node 8 is the second ...).
>>> */
>>> - distance_table = g_new0(uint32_t, distance_table_entries + 1);
>>> - distance_table[0] = cpu_to_be32(distance_table_entries);
>>> + distance_table_length = (uint32_t *)distance_table;
>>> + distance_table_length[0] = cpu_to_be32(distance_table_entries);
>>>
>>> - node_distances = (uint8_t *)&distance_table[1];
>>> - i = 0;
>>> + i = 4;
>>>
>>
>> A comment reminding why we're doing that wouldn't hurt, e.g.
>>
>> /* Skip the array size (uint32_t) */
>
> Then maybe instead of (or in addition to) a comment you could write sizeof(uint32_t) or sizeof(distance_rable[0]) instead of constant 4 to make this more explicit.
distance_table is an uint8_t array. sizeof(distance_table[0]) would return 1.
Doing i = sizeof(uint32_t) demands the reader to realize "this works because it is
skipping an uint32_t in an uint8_t array and sizeof(uint8_t) is 1".
I think it's clearer to just be explicit in the comment:
/* First 4 uint8_t contains the uint32_t array length */
Thanks,
Daniel
>
> Regards,
> BALATON Zoltan
>
>> With these fixed, especially using NUMA_DISTANCE_MIN, you
>> can add:
>>
>> Reviewed-by: Greg Kurz <groug@kaod.org>
>>
>>> for (src = 0; src < nb_numa_nodes; src++) {
>>> for (dst = 0; dst < nb_numa_nodes; dst++) {
>>> @@ -546,18 +551,16 @@ static void spapr_numa_FORM2_write_rtas_tables(SpaprMachineState *spapr,
>>> * adding the numa_info to retrieve distance info from.
>>> */
>>> if (src == dst) {
>>> - node_distances[i++] = 10;
>>> + distance_table[i++] = NUMA_LOCAL_DISTANCE;
>>> continue;
>>> }
>>>
>>> - node_distances[i++] = numa_info[src].distance[dst];
>>> + distance_table[i++] = numa_info[src].distance[dst];
>>> }
>>> }
>>>
>>> - distance_table_size = distance_table_entries * sizeof(uint8_t) +
>>> - sizeof(uint32_t);
>>> _FDT(fdt_setprop(fdt, rtas, "ibm,numa-distance-table",
>>> - distance_table, distance_table_size));
>>> + distance_table, sizeof(distance_table)));
>>> }
>>>
>>> /*
>>
>>
>>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] spapr_numa.c: fixes in spapr_numa_FORM2_write_rtas_tables()
2021-09-22 11:00 ` Daniel Henrique Barboza
@ 2021-09-22 11:10 ` BALATON Zoltan
0 siblings, 0 replies; 10+ messages in thread
From: BALATON Zoltan @ 2021-09-22 11:10 UTC (permalink / raw)
To: Daniel Henrique Barboza; +Cc: david, qemu-ppc, Greg Kurz, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 5527 bytes --]
On Wed, 22 Sep 2021, Daniel Henrique Barboza wrote:
> On 9/22/21 06:51, BALATON Zoltan wrote:
>> On Wed, 22 Sep 2021, Greg Kurz wrote:
>>> On Tue, 21 Sep 2021 16:43:47 -0300
>>> Daniel Henrique Barboza <danielhb413@gmail.com> wrote:
>>>
>>>> This patch has a handful of modifications for the recent added
>>>> FORM2 support:
>>>>
>>>> - there is no particular reason for both 'lookup_index_table' and
>>>> 'distance_table' to be allocated in the heap, since their sizes are
>>>> known right at the start of the function. Use static allocation in
>>>> them to spare a couple of g_new0() calls;
>>>>
>>>> - to not allocate more than the necessary size in 'distance_table'. At
>>>> this moment the array is oversized due to allocating uint32_t for all
>>>> elements, when most of them fits in an uint8_t;
>>>>
>>>> - create a NUMA_LOCAL_DISTANCE macro to avoid hardcoding the local
>>>> distance value.
>>>>
>>>
>>> Not needed. A notion of minimal distance, which is obviously
>>> synonymous to local, already exists in the "sysemu/numa.h"
>>> header :
>>>
>>> #define NUMA_DISTANCE_MIN 10
>>>
>>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>>> ---
>>>> hw/ppc/spapr_numa.c | 35 +++++++++++++++++++----------------
>>>> 1 file changed, 19 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/hw/ppc/spapr_numa.c b/hw/ppc/spapr_numa.c
>>>> index 58d5dc7084..039a0439c6 100644
>>>> --- a/hw/ppc/spapr_numa.c
>>>> +++ b/hw/ppc/spapr_numa.c
>>>> @@ -19,6 +19,9 @@
>>>> /* Moved from hw/ppc/spapr_pci_nvlink2.c */
>>>> #define SPAPR_GPU_NUMA_ID (cpu_to_be32(1))
>>>>
>>>> +/* Macro to avoid hardcoding the local distance value */
>>>> +#define NUMA_LOCAL_DISTANCE 10
>>>> +
>>>> /*
>>>> * Retrieves max_dist_ref_points of the current NUMA affinity.
>>>> */
>>>> @@ -500,17 +503,21 @@ static void
>>>> spapr_numa_FORM2_write_rtas_tables(SpaprMachineState *spapr,
>>>> MachineState *ms = MACHINE(spapr);
>>>> NodeInfo *numa_info = ms->numa_state->nodes;
>>>> int nb_numa_nodes = ms->numa_state->num_nodes;
>>>> + /* Lookup index table has an extra uint32_t with its length */
>>>> + uint32_t lookup_index_table[nb_numa_nodes + 1];
>>>> int distance_table_entries = nb_numa_nodes * nb_numa_nodes;
>>>> - g_autofree uint32_t *lookup_index_table = NULL;
>>>> - g_autofree uint32_t *distance_table = NULL;
>>>> - int src, dst, i, distance_table_size;
>>>> - uint8_t *node_distances;
>>>> + /*
>>>> + * Distance table is an uint8_t array with a leading uint32_t
>>>> + * containing its length.
>>>> + */
>>>> + uint8_t distance_table[distance_table_entries + 4];
>>>> + uint32_t *distance_table_length;
>>>> + int src, dst, i;
>>>>
>>>> /*
>>>> * ibm,numa-lookup-index-table: array with length and a
>>>> * list of NUMA ids present in the guest.
>>>> */
>>>> - lookup_index_table = g_new0(uint32_t, nb_numa_nodes + 1);
>>>> lookup_index_table[0] = cpu_to_be32(nb_numa_nodes);
>>>>
>>>> for (i = 0; i < nb_numa_nodes; i++) {
>>>> @@ -518,8 +525,7 @@ static void
>>>> spapr_numa_FORM2_write_rtas_tables(SpaprMachineState *spapr,
>>>> }
>>>>
>>>> _FDT(fdt_setprop(fdt, rtas, "ibm,numa-lookup-index-table",
>>>> - lookup_index_table,
>>>> - (nb_numa_nodes + 1) * sizeof(uint32_t)));
>>>> + lookup_index_table, sizeof(lookup_index_table)));
>>>>
>>>> /*
>>>> * ibm,numa-distance-table: contains all node distances. First
>>>> @@ -531,11 +537,10 @@ static void
>>>> spapr_numa_FORM2_write_rtas_tables(SpaprMachineState *spapr,
>>>> * array because NUMA ids can be sparse (node 0 is the first,
>>>> * node 8 is the second ...).
>>>> */
>>>> - distance_table = g_new0(uint32_t, distance_table_entries + 1);
>>>> - distance_table[0] = cpu_to_be32(distance_table_entries);
>>>> + distance_table_length = (uint32_t *)distance_table;
>>>> + distance_table_length[0] = cpu_to_be32(distance_table_entries);
>>>>
>>>> - node_distances = (uint8_t *)&distance_table[1];
>>>> - i = 0;
>>>> + i = 4;
>>>>
>>>
>>> A comment reminding why we're doing that wouldn't hurt, e.g.
>>>
>>> /* Skip the array size (uint32_t) */
>>
>> Then maybe instead of (or in addition to) a comment you could write
>> sizeof(uint32_t) or sizeof(distance_rable[0]) instead of constant 4 to make
>> this more explicit.
>
> distance_table is an uint8_t array. sizeof(distance_table[0]) would return 1.
Yes, sorry I was looking at uint32_t *distance_table_length; instead of
the array definition.
> Doing i = sizeof(uint32_t) demands the reader to realize "this works because
> it is
> skipping an uint32_t in an uint8_t array and sizeof(uint8_t) is 1".
>
> I think it's clearer to just be explicit in the comment:
>
>
> /* First 4 uint8_t contains the uint32_t array length */
That explains it better (although a bit confusing, if you need the length
why don't you have a struct with the length and the array instead of
sroring it in the array when that's a different type, unless this is some
data structure defined that way, I don't know what this is all about). But
I don't really care, adding this comment is fine.
Regards,
BALATON Zoltan
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] spapr_numa.c: fixes in spapr_numa_FORM2_write_rtas_tables()
2021-09-21 19:43 [PATCH] spapr_numa.c: fixes in spapr_numa_FORM2_write_rtas_tables() Daniel Henrique Barboza
2021-09-22 3:08 ` David Gibson
2021-09-22 8:26 ` Greg Kurz
@ 2021-09-22 11:17 ` Philippe Mathieu-Daudé
2021-09-22 11:50 ` Daniel Henrique Barboza
2021-09-22 11:52 ` Greg Kurz
2 siblings, 2 replies; 10+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-09-22 11:17 UTC (permalink / raw)
To: Daniel Henrique Barboza, qemu-devel; +Cc: qemu-ppc, groug, david
On 9/21/21 21:43, Daniel Henrique Barboza wrote:
> This patch has a handful of modifications for the recent added
> FORM2 support:
>
> - there is no particular reason for both 'lookup_index_table' and
> 'distance_table' to be allocated in the heap, since their sizes are
> known right at the start of the function. Use static allocation in
> them to spare a couple of g_new0() calls;
>
> - to not allocate more than the necessary size in 'distance_table'. At
> this moment the array is oversized due to allocating uint32_t for all
> elements, when most of them fits in an uint8_t;
>
> - create a NUMA_LOCAL_DISTANCE macro to avoid hardcoding the local
> distance value.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
> hw/ppc/spapr_numa.c | 35 +++++++++++++++++++----------------
> 1 file changed, 19 insertions(+), 16 deletions(-)
>
> diff --git a/hw/ppc/spapr_numa.c b/hw/ppc/spapr_numa.c
> index 58d5dc7084..039a0439c6 100644
> --- a/hw/ppc/spapr_numa.c
> +++ b/hw/ppc/spapr_numa.c
> @@ -19,6 +19,9 @@
> /* Moved from hw/ppc/spapr_pci_nvlink2.c */
> #define SPAPR_GPU_NUMA_ID (cpu_to_be32(1))
>
> +/* Macro to avoid hardcoding the local distance value */
> +#define NUMA_LOCAL_DISTANCE 10
> +
> /*
> * Retrieves max_dist_ref_points of the current NUMA affinity.
> */
> @@ -500,17 +503,21 @@ static void spapr_numa_FORM2_write_rtas_tables(SpaprMachineState *spapr,
> MachineState *ms = MACHINE(spapr);
> NodeInfo *numa_info = ms->numa_state->nodes;
> int nb_numa_nodes = ms->numa_state->num_nodes;
> + /* Lookup index table has an extra uint32_t with its length */
> + uint32_t lookup_index_table[nb_numa_nodes + 1];
> int distance_table_entries = nb_numa_nodes * nb_numa_nodes;
> - g_autofree uint32_t *lookup_index_table = NULL;
> - g_autofree uint32_t *distance_table = NULL;
> - int src, dst, i, distance_table_size;
> - uint8_t *node_distances;
This should have be of ptrdiff_t type.
> + /*
> + * Distance table is an uint8_t array with a leading uint32_t
> + * containing its length.
> + */
> + uint8_t distance_table[distance_table_entries + 4];
The previous code seems better by using the heap, now we have
to worry about stack overflow...
> + uint32_t *distance_table_length;
Please drop, ...
> + int src, dst, i;
>
> /*
> * ibm,numa-lookup-index-table: array with length and a
> * list of NUMA ids present in the guest.
> */
> - lookup_index_table = g_new0(uint32_t, nb_numa_nodes + 1);
> lookup_index_table[0] = cpu_to_be32(nb_numa_nodes);
>
> for (i = 0; i < nb_numa_nodes; i++) {
> @@ -518,8 +525,7 @@ static void spapr_numa_FORM2_write_rtas_tables(SpaprMachineState *spapr,
> }
>
> _FDT(fdt_setprop(fdt, rtas, "ibm,numa-lookup-index-table",
> - lookup_index_table,
> - (nb_numa_nodes + 1) * sizeof(uint32_t)));
> + lookup_index_table, sizeof(lookup_index_table)));
>
> /*
> * ibm,numa-distance-table: contains all node distances. First
> @@ -531,11 +537,10 @@ static void spapr_numa_FORM2_write_rtas_tables(SpaprMachineState *spapr,
> * array because NUMA ids can be sparse (node 0 is the first,
> * node 8 is the second ...).
> */
> - distance_table = g_new0(uint32_t, distance_table_entries + 1);
> - distance_table[0] = cpu_to_be32(distance_table_entries);
> + distance_table_length = (uint32_t *)distance_table;
> + distance_table_length[0] = cpu_to_be32(distance_table_entries);
... and use instead:
stl_be_p(distance_table, distance_table_entries);
> - node_distances = (uint8_t *)&distance_table[1];
> - i = 0;
> + i = 4;
>
> for (src = 0; src < nb_numa_nodes; src++) {
> for (dst = 0; dst < nb_numa_nodes; dst++) {
> @@ -546,18 +551,16 @@ static void spapr_numa_FORM2_write_rtas_tables(SpaprMachineState *spapr,
> * adding the numa_info to retrieve distance info from.
> */
> if (src == dst) {
> - node_distances[i++] = 10;
> + distance_table[i++] = NUMA_LOCAL_DISTANCE;
> continue;
> }
>
> - node_distances[i++] = numa_info[src].distance[dst];
> + distance_table[i++] = numa_info[src].distance[dst];
> }
> }
>
> - distance_table_size = distance_table_entries * sizeof(uint8_t) +
> - sizeof(uint32_t);
> _FDT(fdt_setprop(fdt, rtas, "ibm,numa-distance-table",
> - distance_table, distance_table_size));
> + distance_table, sizeof(distance_table)));
> }
>
> /*
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] spapr_numa.c: fixes in spapr_numa_FORM2_write_rtas_tables()
2021-09-22 11:17 ` Philippe Mathieu-Daudé
@ 2021-09-22 11:50 ` Daniel Henrique Barboza
2021-09-22 11:52 ` Greg Kurz
1 sibling, 0 replies; 10+ messages in thread
From: Daniel Henrique Barboza @ 2021-09-22 11:50 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel; +Cc: qemu-ppc, groug, david
On 9/22/21 08:17, Philippe Mathieu-Daudé wrote:
> On 9/21/21 21:43, Daniel Henrique Barboza wrote:
>> This patch has a handful of modifications for the recent added
>> FORM2 support:
>>
>> - there is no particular reason for both 'lookup_index_table' and
>> 'distance_table' to be allocated in the heap, since their sizes are
>> known right at the start of the function. Use static allocation in
>> them to spare a couple of g_new0() calls;
>>
>> - to not allocate more than the necessary size in 'distance_table'. At
>> this moment the array is oversized due to allocating uint32_t for all
>> elements, when most of them fits in an uint8_t;
>>
>> - create a NUMA_LOCAL_DISTANCE macro to avoid hardcoding the local
>> distance value.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>> hw/ppc/spapr_numa.c | 35 +++++++++++++++++++----------------
>> 1 file changed, 19 insertions(+), 16 deletions(-)
>>
>> diff --git a/hw/ppc/spapr_numa.c b/hw/ppc/spapr_numa.c
>> index 58d5dc7084..039a0439c6 100644
>> --- a/hw/ppc/spapr_numa.c
>> +++ b/hw/ppc/spapr_numa.c
>> @@ -19,6 +19,9 @@
>> /* Moved from hw/ppc/spapr_pci_nvlink2.c */
>> #define SPAPR_GPU_NUMA_ID (cpu_to_be32(1))
>> +/* Macro to avoid hardcoding the local distance value */
>> +#define NUMA_LOCAL_DISTANCE 10
>> +
>> /*
>> * Retrieves max_dist_ref_points of the current NUMA affinity.
>> */
>> @@ -500,17 +503,21 @@ static void spapr_numa_FORM2_write_rtas_tables(SpaprMachineState *spapr,
>> MachineState *ms = MACHINE(spapr);
>> NodeInfo *numa_info = ms->numa_state->nodes;
>> int nb_numa_nodes = ms->numa_state->num_nodes;
>> + /* Lookup index table has an extra uint32_t with its length */
>> + uint32_t lookup_index_table[nb_numa_nodes + 1];
>> int distance_table_entries = nb_numa_nodes * nb_numa_nodes;
>> - g_autofree uint32_t *lookup_index_table = NULL;
>> - g_autofree uint32_t *distance_table = NULL;
>> - int src, dst, i, distance_table_size;
>> - uint8_t *node_distances;
>
> This should have be of ptrdiff_t type.
>
>> + /*
>> + * Distance table is an uint8_t array with a leading uint32_t
>> + * containing its length.
>> + */
>> + uint8_t distance_table[distance_table_entries + 4];
>
> The previous code seems better by using the heap, now we have
> to worry about stack overflow...
Fair point.
Since no one asked for this change in previous reviews I guess it's fine to keep
using the heap.
Daniel
>
>> + uint32_t *distance_table_length;
>
> Please drop, ...
>
>> + int src, dst, i;
>> /*
>> * ibm,numa-lookup-index-table: array with length and a
>> * list of NUMA ids present in the guest.
>> */
>> - lookup_index_table = g_new0(uint32_t, nb_numa_nodes + 1);
>> lookup_index_table[0] = cpu_to_be32(nb_numa_nodes);
>> for (i = 0; i < nb_numa_nodes; i++) {
>> @@ -518,8 +525,7 @@ static void spapr_numa_FORM2_write_rtas_tables(SpaprMachineState *spapr,
>> }
>> _FDT(fdt_setprop(fdt, rtas, "ibm,numa-lookup-index-table",
>> - lookup_index_table,
>> - (nb_numa_nodes + 1) * sizeof(uint32_t)));
>> + lookup_index_table, sizeof(lookup_index_table)));
>> /*
>> * ibm,numa-distance-table: contains all node distances. First
>> @@ -531,11 +537,10 @@ static void spapr_numa_FORM2_write_rtas_tables(SpaprMachineState *spapr,
>> * array because NUMA ids can be sparse (node 0 is the first,
>> * node 8 is the second ...).
>> */
>> - distance_table = g_new0(uint32_t, distance_table_entries + 1);
>> - distance_table[0] = cpu_to_be32(distance_table_entries);
>> + distance_table_length = (uint32_t *)distance_table;
>> + distance_table_length[0] = cpu_to_be32(distance_table_entries);
>
> ... and use instead:
>
> stl_be_p(distance_table, distance_table_entries);
>
>> - node_distances = (uint8_t *)&distance_table[1];
>> - i = 0;
>> + i = 4;
>> for (src = 0; src < nb_numa_nodes; src++) {
>> for (dst = 0; dst < nb_numa_nodes; dst++) {
>> @@ -546,18 +551,16 @@ static void spapr_numa_FORM2_write_rtas_tables(SpaprMachineState *spapr,
>> * adding the numa_info to retrieve distance info from.
>> */
>> if (src == dst) {
>> - node_distances[i++] = 10;
>> + distance_table[i++] = NUMA_LOCAL_DISTANCE;
>> continue;
>> }
>> - node_distances[i++] = numa_info[src].distance[dst];
>> + distance_table[i++] = numa_info[src].distance[dst];
>> }
>> }
>> - distance_table_size = distance_table_entries * sizeof(uint8_t) +
>> - sizeof(uint32_t);
>> _FDT(fdt_setprop(fdt, rtas, "ibm,numa-distance-table",
>> - distance_table, distance_table_size));
>> + distance_table, sizeof(distance_table)));
>> }
>> /*
>>
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] spapr_numa.c: fixes in spapr_numa_FORM2_write_rtas_tables()
2021-09-22 11:17 ` Philippe Mathieu-Daudé
2021-09-22 11:50 ` Daniel Henrique Barboza
@ 2021-09-22 11:52 ` Greg Kurz
2021-09-22 12:35 ` Philippe Mathieu-Daudé
1 sibling, 1 reply; 10+ messages in thread
From: Greg Kurz @ 2021-09-22 11:52 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Daniel Henrique Barboza, qemu-ppc, qemu-devel, david
On Wed, 22 Sep 2021 13:17:32 +0200
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> On 9/21/21 21:43, Daniel Henrique Barboza wrote:
> > This patch has a handful of modifications for the recent added
> > FORM2 support:
> >
> > - there is no particular reason for both 'lookup_index_table' and
> > 'distance_table' to be allocated in the heap, since their sizes are
> > known right at the start of the function. Use static allocation in
> > them to spare a couple of g_new0() calls;
> >
> > - to not allocate more than the necessary size in 'distance_table'. At
> > this moment the array is oversized due to allocating uint32_t for all
> > elements, when most of them fits in an uint8_t;
> >
> > - create a NUMA_LOCAL_DISTANCE macro to avoid hardcoding the local
> > distance value.
> >
> > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> > ---
> > hw/ppc/spapr_numa.c | 35 +++++++++++++++++++----------------
> > 1 file changed, 19 insertions(+), 16 deletions(-)
> >
> > diff --git a/hw/ppc/spapr_numa.c b/hw/ppc/spapr_numa.c
> > index 58d5dc7084..039a0439c6 100644
> > --- a/hw/ppc/spapr_numa.c
> > +++ b/hw/ppc/spapr_numa.c
> > @@ -19,6 +19,9 @@
> > /* Moved from hw/ppc/spapr_pci_nvlink2.c */
> > #define SPAPR_GPU_NUMA_ID (cpu_to_be32(1))
> >
> > +/* Macro to avoid hardcoding the local distance value */
> > +#define NUMA_LOCAL_DISTANCE 10
> > +
> > /*
> > * Retrieves max_dist_ref_points of the current NUMA affinity.
> > */
> > @@ -500,17 +503,21 @@ static void spapr_numa_FORM2_write_rtas_tables(SpaprMachineState *spapr,
> > MachineState *ms = MACHINE(spapr);
> > NodeInfo *numa_info = ms->numa_state->nodes;
> > int nb_numa_nodes = ms->numa_state->num_nodes;
> > + /* Lookup index table has an extra uint32_t with its length */
> > + uint32_t lookup_index_table[nb_numa_nodes + 1];
> > int distance_table_entries = nb_numa_nodes * nb_numa_nodes;
> > - g_autofree uint32_t *lookup_index_table = NULL;
> > - g_autofree uint32_t *distance_table = NULL;
> > - int src, dst, i, distance_table_size;
> > - uint8_t *node_distances;
>
> This should have be of ptrdiff_t type.
>
Why ? I don't see pointer subtraction in the code.
> > + /*
> > + * Distance table is an uint8_t array with a leading uint32_t
> > + * containing its length.
> > + */
> > + uint8_t distance_table[distance_table_entries + 4];
>
> The previous code seems better by using the heap, now we have
> to worry about stack overflow...
>
Indeed the size of this array could be up to 16k + 4. I guess
Philippe's point make sense. David's request was to use uint8_t
instead of uin32t_t, not to drop g_new0(). Please revert to
using the heap.
lookup_index_table[] can _only_ grow up to 516 bytes, which is
less problematic, but it doesn't hurt either to allocate it
on the heap. Not changing that would make this patch simpler.
> > + uint32_t *distance_table_length;
>
> Please drop, ...
>
> > + int src, dst, i;
> >
> > /*
> > * ibm,numa-lookup-index-table: array with length and a
> > * list of NUMA ids present in the guest.
> > */
> > - lookup_index_table = g_new0(uint32_t, nb_numa_nodes + 1);
> > lookup_index_table[0] = cpu_to_be32(nb_numa_nodes);
> >
> > for (i = 0; i < nb_numa_nodes; i++) {
> > @@ -518,8 +525,7 @@ static void spapr_numa_FORM2_write_rtas_tables(SpaprMachineState *spapr,
> > }
> >
> > _FDT(fdt_setprop(fdt, rtas, "ibm,numa-lookup-index-table",
> > - lookup_index_table,
> > - (nb_numa_nodes + 1) * sizeof(uint32_t)));
> > + lookup_index_table, sizeof(lookup_index_table)));
> >
> > /*
> > * ibm,numa-distance-table: contains all node distances. First
> > @@ -531,11 +537,10 @@ static void spapr_numa_FORM2_write_rtas_tables(SpaprMachineState *spapr,
> > * array because NUMA ids can be sparse (node 0 is the first,
> > * node 8 is the second ...).
> > */
> > - distance_table = g_new0(uint32_t, distance_table_entries + 1);
> > - distance_table[0] = cpu_to_be32(distance_table_entries);
> > + distance_table_length = (uint32_t *)distance_table;
> > + distance_table_length[0] = cpu_to_be32(distance_table_entries);
>
> ... and use instead:
>
> stl_be_p(distance_table, distance_table_entries);
>
+1
> > - node_distances = (uint8_t *)&distance_table[1];
> > - i = 0;
> > + i = 4;
> >
> > for (src = 0; src < nb_numa_nodes; src++) {
> > for (dst = 0; dst < nb_numa_nodes; dst++) {
> > @@ -546,18 +551,16 @@ static void spapr_numa_FORM2_write_rtas_tables(SpaprMachineState *spapr,
> > * adding the numa_info to retrieve distance info from.
> > */
> > if (src == dst) {
> > - node_distances[i++] = 10;
> > + distance_table[i++] = NUMA_LOCAL_DISTANCE;
> > continue;
> > }
> >
> > - node_distances[i++] = numa_info[src].distance[dst];
> > + distance_table[i++] = numa_info[src].distance[dst];
> > }
> > }
> >
> > - distance_table_size = distance_table_entries * sizeof(uint8_t) +
> > - sizeof(uint32_t);
> > _FDT(fdt_setprop(fdt, rtas, "ibm,numa-distance-table",
> > - distance_table, distance_table_size));
> > + distance_table, sizeof(distance_table)));
> > }
> >
> > /*
> >
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] spapr_numa.c: fixes in spapr_numa_FORM2_write_rtas_tables()
2021-09-22 11:52 ` Greg Kurz
@ 2021-09-22 12:35 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 10+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-09-22 12:35 UTC (permalink / raw)
To: Greg Kurz; +Cc: Daniel Henrique Barboza, qemu-ppc, qemu-devel, david
On 9/22/21 13:52, Greg Kurz wrote:
> On Wed, 22 Sep 2021 13:17:32 +0200
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
>> On 9/21/21 21:43, Daniel Henrique Barboza wrote:
>>> This patch has a handful of modifications for the recent added
>>> FORM2 support:
>>>
>>> - there is no particular reason for both 'lookup_index_table' and
>>> 'distance_table' to be allocated in the heap, since their sizes are
>>> known right at the start of the function. Use static allocation in
>>> them to spare a couple of g_new0() calls;
>>>
>>> - to not allocate more than the necessary size in 'distance_table'. At
>>> this moment the array is oversized due to allocating uint32_t for all
>>> elements, when most of them fits in an uint8_t;
>>>
>>> - create a NUMA_LOCAL_DISTANCE macro to avoid hardcoding the local
>>> distance value.
>>>
>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>> ---
>>> hw/ppc/spapr_numa.c | 35 +++++++++++++++++++----------------
>>> 1 file changed, 19 insertions(+), 16 deletions(-)
>>> /*
>>> * Retrieves max_dist_ref_points of the current NUMA affinity.
>>> */
>>> @@ -500,17 +503,21 @@ static void spapr_numa_FORM2_write_rtas_tables(SpaprMachineState *spapr,
>>> MachineState *ms = MACHINE(spapr);
>>> NodeInfo *numa_info = ms->numa_state->nodes;
>>> int nb_numa_nodes = ms->numa_state->num_nodes;
>>> + /* Lookup index table has an extra uint32_t with its length */
>>> + uint32_t lookup_index_table[nb_numa_nodes + 1];
>>> int distance_table_entries = nb_numa_nodes * nb_numa_nodes;
>>> - g_autofree uint32_t *lookup_index_table = NULL;
>>> - g_autofree uint32_t *distance_table = NULL;
>>> - int src, dst, i, distance_table_size;
>>> - uint8_t *node_distances;
>>
>> This should have be of ptrdiff_t type.
>>
>
> Why ? I don't see pointer subtraction in the code.
Oops, you are right.
^ permalink raw reply [flat|nested] 10+ messages in thread