Message ID | 20210921194347.52347-1-danielhb413@gmail.com |
---|---|
State | New |
Headers | show |
Series | spapr_numa.c: fixes in spapr_numa_FORM2_write_rtas_tables() | expand |
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))); > } > > /*
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))); > } > > /*
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))); >> } >> >> /* > > >
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))); >>> } >>> >>> /* >> >> >>
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
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))); > } > > /* >
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))); >> } >> /* >> >
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))); > > } > > > > /* > > >
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.
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))); } /*
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(-)