Re: [PATCH v5 4/6] powerpc/pseries: Consolidate different NUMA distance update code paths
On Tue, Jul 27, 2021 at 09:02:33AM +0530, Aneesh Kumar K.V wrote:
> David Gibson writes:
>
> > On Thu, Jul 22, 2021 at 12:37:46PM +0530, Aneesh Kumar K.V wrote:
> >> David Gibson writes:
> >>
> >> > On Mon, Jun 28, 2021 at 08:41:15PM +0530, Aneesh Kumar K.V wrote:
>
>
>
> >
> >> >
> >> >> + nid = of_read_number(&aa.arrays[index], 1);
> >> >> +
> >> >> + if (nid == 0x || nid >= nr_node_ids)
> >> >> + nid = default_nid;
> >> >> + if (nid > 0 && affinity_form == FORM1_AFFINITY) {
> >> >> + int i;
> >> >> + const __be32 *associativity;
> >> >> +
> >> >> + index = lmb->aa_index * aa.array_sz;
> >> >> + associativity = &aa.arrays[index];
> >> >> + /*
> >> >> +* lookup array associativity entries have
> >> >> different format
> >> >> +* There is no length of the array as the first
> >> >> element.
> >> >
> >> > The difference it very small, and this is not a hot path. Couldn't
> >> > you reduce a chunk of code by prepending aa.array_sz, then re-using
> >> > __initialize_form1_numa_distance. Or even making
> >> > __initialize_form1_numa_distance() take the length as a parameter.
> >>
> >> The changes are small but confusing w.r.t how we look at the
> >> associativity-lookup-arrays. The way we interpret associativity array
> >> and associativity lookup array using primary_domain_index is different.
> >> Hence the '-1' in the node lookup here.
> >
> > They're really not, though. It's exactly the same interpretation of
> > the associativity array itself - it's just that one of them has the
> > array prepended with a (redundant) length. So you can make
> > __initialize_form1_numa_distance() work on the "bare" associativity
> > array, with a given length. Here you call it with aa.array_sz as the
> > length, and in the other place you call it with prop[0] as the length.
> >
> >>
> >>index = lmb->aa_index * aa.array_sz + primary_domain_index - 1;
> >>nid = of_read_number(&aa.arrays[index], 1);
> >>
> >>
> >> >
> >> >> +*/
> >> >> + for (i = 0; i < max_associativity_domain_index;
> >> >> i++) {
> >> >> + const __be32 *entry;
> >> >> +
> >> >> + entry =
> >> >> &associativity[be32_to_cpu(distance_ref_points[i]) - 1];
> >> >
> >> > Does anywhere verify that distance_ref_points[i] <= aa.array_size for
> >> > every i?
> >>
> >> We do check for
> >>
> >>if (primary_domain_index <= aa.array_sz &&
> >
> > Right, but that doesn't check the other distance_ref_points entries.
> > Not that there's any reason to have extra entries with Form2, but we
> > still don't want stray array accesses.
>
> This is how the change looks. I am not convinced this makes it simpler.
It's not, but that's because the lookup_array_assoc flag is not needed...
> I will add that as the last patch and we can drop that if we find that
> not helpful?
>
> modified arch/powerpc/mm/numa.c
> @@ -171,20 +171,31 @@ static void unmap_cpu_from_node(unsigned long cpu)
> }
> #endif /* CONFIG_HOTPLUG_CPU || CONFIG_PPC_SPLPAR */
>
> -/*
> - * Returns nid in the range [0..nr_node_ids], or -1 if no useful NUMA
> - * info is found.
> - */
> -static int associativity_to_nid(const __be32 *associativity)
> +static int __associativity_to_nid(const __be32 *associativity,
> + bool lookup_array_assoc,
> + int max_array_index)
> {
> int nid = NUMA_NO_NODE;
> + int index;
>
> if (!numa_enabled)
> goto out;
> + /*
> + * ibm,associativity-lookup-array doesn't have element
> + * count at the start of the associativity. Hence
> + * decrement the primary_domain_index when used with
> + * lookup-array associativity.
> + */
> + if (lookup_array_assoc)
> + index = primary_domain_index - 1;
> + else {
> + index = primary_domain_index;
> + max_array_index = of_read_number(associativity, 1);
> + }
> + if (index > max_array_index)
> + goto out;
So, the associativity-array-with-length is exactly a length, followed
by an associativity-array-without-length. What I was suggesting is
you make this function only take an
associativity-array-without-length, with the length passed separately.
Where you want to use it on an associativity-array-with-length, stored
in __be32 *awl, you just invoke it as:
associativity_to_nid(awl + 1, of_read_number(awl, 1));
> - if (of_read_number(associativity, 1) >= primary_domain_index)
> - nid = of_read_number(&associativity[primary_domain_index], 1);
> -
> + nid = of_read_number(&associativity[index], 1);
> /* POWER4 LPAR uses 0x as invalid node */
> if (nid == 0x
Re: [PATCH v5 4/6] powerpc/pseries: Consolidate different NUMA distance update code paths
David Gibson writes:
> On Thu, Jul 22, 2021 at 12:37:46PM +0530, Aneesh Kumar K.V wrote:
>> David Gibson writes:
>>
>> > On Mon, Jun 28, 2021 at 08:41:15PM +0530, Aneesh Kumar K.V wrote:
>
>> >
>> >> + nid = of_read_number(&aa.arrays[index], 1);
>> >> +
>> >> + if (nid == 0x || nid >= nr_node_ids)
>> >> + nid = default_nid;
>> >> + if (nid > 0 && affinity_form == FORM1_AFFINITY) {
>> >> + int i;
>> >> + const __be32 *associativity;
>> >> +
>> >> + index = lmb->aa_index * aa.array_sz;
>> >> + associativity = &aa.arrays[index];
>> >> + /*
>> >> + * lookup array associativity entries have different
>> >> format
>> >> + * There is no length of the array as the first element.
>> >
>> > The difference it very small, and this is not a hot path. Couldn't
>> > you reduce a chunk of code by prepending aa.array_sz, then re-using
>> > __initialize_form1_numa_distance. Or even making
>> > __initialize_form1_numa_distance() take the length as a parameter.
>>
>> The changes are small but confusing w.r.t how we look at the
>> associativity-lookup-arrays. The way we interpret associativity array
>> and associativity lookup array using primary_domain_index is different.
>> Hence the '-1' in the node lookup here.
>
> They're really not, though. It's exactly the same interpretation of
> the associativity array itself - it's just that one of them has the
> array prepended with a (redundant) length. So you can make
> __initialize_form1_numa_distance() work on the "bare" associativity
> array, with a given length. Here you call it with aa.array_sz as the
> length, and in the other place you call it with prop[0] as the length.
>
>>
>> index = lmb->aa_index * aa.array_sz + primary_domain_index - 1;
>> nid = of_read_number(&aa.arrays[index], 1);
>>
>>
>> >
>> >> + */
>> >> + for (i = 0; i < max_associativity_domain_index; i++) {
>> >> + const __be32 *entry;
>> >> +
>> >> + entry =
>> >> &associativity[be32_to_cpu(distance_ref_points[i]) - 1];
>> >
>> > Does anywhere verify that distance_ref_points[i] <= aa.array_size for
>> > every i?
>>
>> We do check for
>>
>> if (primary_domain_index <= aa.array_sz &&
>
> Right, but that doesn't check the other distance_ref_points entries.
> Not that there's any reason to have extra entries with Form2, but we
> still don't want stray array accesses.
This is how the change looks. I am not convinced this makes it simpler.
I will add that as the last patch and we can drop that if we find that
not helpful?
modified arch/powerpc/mm/numa.c
@@ -171,20 +171,31 @@ static void unmap_cpu_from_node(unsigned long cpu)
}
#endif /* CONFIG_HOTPLUG_CPU || CONFIG_PPC_SPLPAR */
-/*
- * Returns nid in the range [0..nr_node_ids], or -1 if no useful NUMA
- * info is found.
- */
-static int associativity_to_nid(const __be32 *associativity)
+static int __associativity_to_nid(const __be32 *associativity,
+ bool lookup_array_assoc,
+ int max_array_index)
{
int nid = NUMA_NO_NODE;
+ int index;
if (!numa_enabled)
goto out;
+ /*
+* ibm,associativity-lookup-array doesn't have element
+* count at the start of the associativity. Hence
+* decrement the primary_domain_index when used with
+* lookup-array associativity.
+*/
+ if (lookup_array_assoc)
+ index = primary_domain_index - 1;
+ else {
+ index = primary_domain_index;
+ max_array_index = of_read_number(associativity, 1);
+ }
+ if (index > max_array_index)
+ goto out;
- if (of_read_number(associativity, 1) >= primary_domain_index)
- nid = of_read_number(&associativity[primary_domain_index], 1);
-
+ nid = of_read_number(&associativity[index], 1);
/* POWER4 LPAR uses 0x as invalid node */
if (nid == 0x || nid >= nr_node_ids)
nid = NUMA_NO_NODE;
@@ -192,6 +203,15 @@ static int associativity_to_nid(const __be32
*associativity)
return nid;
}
+/*
+ * Returns nid in the range [0..nr_node_ids], or -1 if no useful NUMA
+ * info is found.
+ */
+static inline int associativity_to_nid(const __be32 *associativity)
+{
+ return __associativity_to_nid(associativity, false, 0);
+}
+
static int __cpu_form2_relative_distance(__be32 *cpu1_assoc, __be32
*cpu2_assoc)
{
int dist;
@@ -295,19 +315,38 @@ int of_node_to_nid(struct device_node *device)
}
EXPORT_SYMBOL(of_node_to_nid);
-static void __initialize_form1_numa_distance(const __be32 *associativity)
+static void __initialize_form1_numa_distance(const __be32 *associativity,
+bool lookup_arra
Re: [PATCH v5 4/6] powerpc/pseries: Consolidate different NUMA distance update code paths
On Thu, Jul 22, 2021 at 12:37:46PM +0530, Aneesh Kumar K.V wrote:
> David Gibson writes:
>
> > On Mon, Jun 28, 2021 at 08:41:15PM +0530, Aneesh Kumar K.V wrote:
> >> The associativity details of the newly added resourced are collected from
> >> the hypervisor via "ibm,configure-connector" rtas call. Update the numa
> >> distance details of the newly added numa node after the above call.
> >>
> >> Instead of updating NUMA distance every time we lookup a node id
> >> from the associativity property, add helpers that can be used
> >> during boot which does this only once. Also remove the distance
> >> update from node id lookup helpers.
> >>
> >> Signed-off-by: Aneesh Kumar K.V
> >> ---
> >> arch/powerpc/mm/numa.c| 173 +-
> >> arch/powerpc/platforms/pseries/hotplug-cpu.c | 2 +
> >> .../platforms/pseries/hotplug-memory.c| 2 +
> >> arch/powerpc/platforms/pseries/pseries.h | 1 +
> >> 4 files changed, 132 insertions(+), 46 deletions(-)
> >>
> >> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
> >> index 0ec16999beef..7b142f79d600 100644
> >> --- a/arch/powerpc/mm/numa.c
> >> +++ b/arch/powerpc/mm/numa.c
> >> @@ -208,22 +208,6 @@ int __node_distance(int a, int b)
> >> }
> >> EXPORT_SYMBOL(__node_distance);
> >>
> >> -static void initialize_distance_lookup_table(int nid,
> >> - const __be32 *associativity)
> >> -{
> >> - int i;
> >> -
> >> - if (affinity_form != FORM1_AFFINITY)
> >> - return;
> >> -
> >> - for (i = 0; i < max_associativity_domain_index; i++) {
> >> - const __be32 *entry;
> >> -
> >> - entry = &associativity[be32_to_cpu(distance_ref_points[i]) - 1];
> >> - distance_lookup_table[nid][i] = of_read_number(entry, 1);
> >> - }
> >> -}
> >> -
> >> /*
> >> * Returns nid in the range [0..nr_node_ids], or -1 if no useful NUMA
> >> * info is found.
> >> @@ -241,15 +225,6 @@ static int associativity_to_nid(const __be32
> >> *associativity)
> >>/* POWER4 LPAR uses 0x as invalid node */
> >>if (nid == 0x || nid >= nr_node_ids)
> >>nid = NUMA_NO_NODE;
> >> -
> >> - if (nid > 0 &&
> >> - of_read_number(associativity, 1) >=
> >> max_associativity_domain_index) {
> >> - /*
> >> - * Skip the length field and send start of associativity array
> >> - */
> >> - initialize_distance_lookup_table(nid, associativity + 1);
> >> - }
> >> -
> >> out:
> >>return nid;
> >> }
> >> @@ -287,6 +262,49 @@ int of_node_to_nid(struct device_node *device)
> >> }
> >> EXPORT_SYMBOL(of_node_to_nid);
> >>
> >> +static void __initialize_form1_numa_distance(const __be32 *associativity)
> >> +{
> >> + int i, nid;
> >> +
> >> + if (affinity_form != FORM1_AFFINITY)
> >
> > Since this shouldn't be called on a !form1 system, this could be a
> > WARN_ON().
>
> The way we call functions currently, instead of doing
>
> if (affinity_form == FORM1_AFFINITY)
> __initialize_form1_numa_distance()
>
> We avoid doing the if check in multiple places. For example
> parse_numa_properties will fetch the associativity array to find the
> details of online node and set it online. We use the same code path to
> initialize distance.
>
> if (__vphn_get_associativity(i, vphn_assoc) == 0) {
> nid = associativity_to_nid(vphn_assoc);
> __initialize_form1_numa_distance(vphn_assoc);
> } else {
>
> cpu = of_get_cpu_node(i, NULL);
> BUG_ON(!cpu);
>
> associativity = of_get_associativity(cpu);
> if (associativity) {
> nid = associativity_to_nid(associativity);
> __initialize_form1_numa_distance(associativity);
> }
>
> We avoid the the if (affinity_form == FORM1_AFFINITY) check there by
> moving the check inside __initialize_form1_numa_distance().
Oh.. ok. The only caller I spotted was already doing a test against
affinity_form.
> >> + return;
> >> +
> >> + if (of_read_number(associativity, 1) >= primary_domain_index) {
> >> + nid = of_read_number(&associativity[primary_domain_index], 1);
> >
> > This computes the nid from the assoc array independently of
> > associativity_to_nid, which doesn't seem like a good idea. Wouldn't
> > it be better to call assocaitivity_to_nid(), then make the next bit
> > conditional on nid !== NUMA_NO_NODE?
>
> @@ -302,9 +302,8 @@ static void __initialize_form1_numa_distance(const __be32
> *associativity)
> if (affinity_form != FORM1_AFFINITY)
> return;
>
> - if (of_read_number(associativity, 1) >= primary_domain_index) {
> - nid = of_read_number(&associativity[primary_domain_index], 1);
> -
> + nid = associativity_to_nid(associativity);
> + if (nid != NUMA_NO_NODE) {
> for (i = 0; i <
Re: [PATCH v5 4/6] powerpc/pseries: Consolidate different NUMA distance update code paths
David Gibson writes:
> On Mon, Jun 28, 2021 at 08:41:15PM +0530, Aneesh Kumar K.V wrote:
>> The associativity details of the newly added resourced are collected from
>> the hypervisor via "ibm,configure-connector" rtas call. Update the numa
>> distance details of the newly added numa node after the above call.
>>
>> Instead of updating NUMA distance every time we lookup a node id
>> from the associativity property, add helpers that can be used
>> during boot which does this only once. Also remove the distance
>> update from node id lookup helpers.
>>
>> Signed-off-by: Aneesh Kumar K.V
>> ---
>> arch/powerpc/mm/numa.c| 173 +-
>> arch/powerpc/platforms/pseries/hotplug-cpu.c | 2 +
>> .../platforms/pseries/hotplug-memory.c| 2 +
>> arch/powerpc/platforms/pseries/pseries.h | 1 +
>> 4 files changed, 132 insertions(+), 46 deletions(-)
>>
>> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
>> index 0ec16999beef..7b142f79d600 100644
>> --- a/arch/powerpc/mm/numa.c
>> +++ b/arch/powerpc/mm/numa.c
>> @@ -208,22 +208,6 @@ int __node_distance(int a, int b)
>> }
>> EXPORT_SYMBOL(__node_distance);
>>
>> -static void initialize_distance_lookup_table(int nid,
>> -const __be32 *associativity)
>> -{
>> -int i;
>> -
>> -if (affinity_form != FORM1_AFFINITY)
>> -return;
>> -
>> -for (i = 0; i < max_associativity_domain_index; i++) {
>> -const __be32 *entry;
>> -
>> -entry = &associativity[be32_to_cpu(distance_ref_points[i]) - 1];
>> -distance_lookup_table[nid][i] = of_read_number(entry, 1);
>> -}
>> -}
>> -
>> /*
>> * Returns nid in the range [0..nr_node_ids], or -1 if no useful NUMA
>> * info is found.
>> @@ -241,15 +225,6 @@ static int associativity_to_nid(const __be32
>> *associativity)
>> /* POWER4 LPAR uses 0x as invalid node */
>> if (nid == 0x || nid >= nr_node_ids)
>> nid = NUMA_NO_NODE;
>> -
>> -if (nid > 0 &&
>> -of_read_number(associativity, 1) >=
>> max_associativity_domain_index) {
>> -/*
>> - * Skip the length field and send start of associativity array
>> - */
>> -initialize_distance_lookup_table(nid, associativity + 1);
>> -}
>> -
>> out:
>> return nid;
>> }
>> @@ -287,6 +262,49 @@ int of_node_to_nid(struct device_node *device)
>> }
>> EXPORT_SYMBOL(of_node_to_nid);
>>
>> +static void __initialize_form1_numa_distance(const __be32 *associativity)
>> +{
>> +int i, nid;
>> +
>> +if (affinity_form != FORM1_AFFINITY)
>
> Since this shouldn't be called on a !form1 system, this could be a WARN_ON().
The way we call functions currently, instead of doing
if (affinity_form == FORM1_AFFINITY)
__initialize_form1_numa_distance()
We avoid doing the if check in multiple places. For example
parse_numa_properties will fetch the associativity array to find the
details of online node and set it online. We use the same code path to
initialize distance.
if (__vphn_get_associativity(i, vphn_assoc) == 0) {
nid = associativity_to_nid(vphn_assoc);
__initialize_form1_numa_distance(vphn_assoc);
} else {
cpu = of_get_cpu_node(i, NULL);
BUG_ON(!cpu);
associativity = of_get_associativity(cpu);
if (associativity) {
nid = associativity_to_nid(associativity);
__initialize_form1_numa_distance(associativity);
}
We avoid the the if (affinity_form == FORM1_AFFINITY) check there by
moving the check inside __initialize_form1_numa_distance().
>
>> +return;
>> +
>> +if (of_read_number(associativity, 1) >= primary_domain_index) {
>> +nid = of_read_number(&associativity[primary_domain_index], 1);
>
> This computes the nid from the assoc array independently of
> associativity_to_nid, which doesn't seem like a good idea. Wouldn't
> it be better to call assocaitivity_to_nid(), then make the next bit
> conditional on nid !== NUMA_NO_NODE?
@@ -302,9 +302,8 @@ static void __initialize_form1_numa_distance(const __be32
*associativity)
if (affinity_form != FORM1_AFFINITY)
return;
- if (of_read_number(associativity, 1) >= primary_domain_index) {
- nid = of_read_number(&associativity[primary_domain_index], 1);
-
+ nid = associativity_to_nid(associativity);
+ if (nid != NUMA_NO_NODE) {
for (i = 0; i < distance_ref_points_depth; i++) {
const __be32 *entry;
>
>> +
>> +for (i = 0; i < max_associativity_domain_index; i++) {
>> +const __be32 *entry;
>> +
>> +entry =
>> &associativity[be32_to_cpu(distance_ref_points[i])];
>> +
Re: [PATCH v5 4/6] powerpc/pseries: Consolidate different NUMA distance update code paths
On Mon, Jun 28, 2021 at 08:41:15PM +0530, Aneesh Kumar K.V wrote:
> The associativity details of the newly added resourced are collected from
> the hypervisor via "ibm,configure-connector" rtas call. Update the numa
> distance details of the newly added numa node after the above call.
>
> Instead of updating NUMA distance every time we lookup a node id
> from the associativity property, add helpers that can be used
> during boot which does this only once. Also remove the distance
> update from node id lookup helpers.
>
> Signed-off-by: Aneesh Kumar K.V
> ---
> arch/powerpc/mm/numa.c| 173 +-
> arch/powerpc/platforms/pseries/hotplug-cpu.c | 2 +
> .../platforms/pseries/hotplug-memory.c| 2 +
> arch/powerpc/platforms/pseries/pseries.h | 1 +
> 4 files changed, 132 insertions(+), 46 deletions(-)
>
> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
> index 0ec16999beef..7b142f79d600 100644
> --- a/arch/powerpc/mm/numa.c
> +++ b/arch/powerpc/mm/numa.c
> @@ -208,22 +208,6 @@ int __node_distance(int a, int b)
> }
> EXPORT_SYMBOL(__node_distance);
>
> -static void initialize_distance_lookup_table(int nid,
> - const __be32 *associativity)
> -{
> - int i;
> -
> - if (affinity_form != FORM1_AFFINITY)
> - return;
> -
> - for (i = 0; i < max_associativity_domain_index; i++) {
> - const __be32 *entry;
> -
> - entry = &associativity[be32_to_cpu(distance_ref_points[i]) - 1];
> - distance_lookup_table[nid][i] = of_read_number(entry, 1);
> - }
> -}
> -
> /*
> * Returns nid in the range [0..nr_node_ids], or -1 if no useful NUMA
> * info is found.
> @@ -241,15 +225,6 @@ static int associativity_to_nid(const __be32
> *associativity)
> /* POWER4 LPAR uses 0x as invalid node */
> if (nid == 0x || nid >= nr_node_ids)
> nid = NUMA_NO_NODE;
> -
> - if (nid > 0 &&
> - of_read_number(associativity, 1) >=
> max_associativity_domain_index) {
> - /*
> - * Skip the length field and send start of associativity array
> - */
> - initialize_distance_lookup_table(nid, associativity + 1);
> - }
> -
> out:
> return nid;
> }
> @@ -287,6 +262,49 @@ int of_node_to_nid(struct device_node *device)
> }
> EXPORT_SYMBOL(of_node_to_nid);
>
> +static void __initialize_form1_numa_distance(const __be32 *associativity)
> +{
> + int i, nid;
> +
> + if (affinity_form != FORM1_AFFINITY)
Since this shouldn't be called on a !form1 system, this could be a WARN_ON().
> + return;
> +
> + if (of_read_number(associativity, 1) >= primary_domain_index) {
> + nid = of_read_number(&associativity[primary_domain_index], 1);
This computes the nid from the assoc array independently of
associativity_to_nid, which doesn't seem like a good idea. Wouldn't
it be better to call assocaitivity_to_nid(), then make the next bit
conditional on nid !== NUMA_NO_NODE?
> +
> + for (i = 0; i < max_associativity_domain_index; i++) {
> + const __be32 *entry;
> +
> + entry =
> &associativity[be32_to_cpu(distance_ref_points[i])];
> + distance_lookup_table[nid][i] = of_read_number(entry,
> 1);
> + }
> + }
> +}
> +
> +static void initialize_form1_numa_distance(struct device_node *node)
> +{
> + const __be32 *associativity;
> +
> + associativity = of_get_associativity(node);
> + if (!associativity)
> + return;
> +
> + __initialize_form1_numa_distance(associativity);
> +}
> +
> +/*
> + * Used to update distance information w.r.t newly added node.
> + */
> +void update_numa_distance(struct device_node *node)
> +{
> + if (affinity_form == FORM0_AFFINITY)
> + return;
> + else if (affinity_form == FORM1_AFFINITY) {
> + initialize_form1_numa_distance(node);
> + return;
> + }
> +}
> +
> static int __init find_primary_domain_index(void)
> {
> int index;
> @@ -433,6 +451,48 @@ static int of_get_assoc_arrays(struct assoc_arrays *aa)
> return 0;
> }
>
> +static int get_nid_and_numa_distance(struct drmem_lmb *lmb)
> +{
> + struct assoc_arrays aa = { .arrays = NULL };
> + int default_nid = NUMA_NO_NODE;
> + int nid = default_nid;
> + int rc, index;
> +
> + if ((primary_domain_index < 0) || !numa_enabled)
Under what circumstances could you get primary_domain_index < 0?
> + return default_nid;
> +
> + rc = of_get_assoc_arrays(&aa);
> + if (rc)
> + return default_nid;
> +
> + if (primary_domain_index <= aa.array_sz &&
> + !(lmb->flags & DRCONF_MEM_AI_INVALID) && lmb->aa_index <
> aa.n_arrays) {
> + index = lmb->aa_index * aa.array_sz + primary_domain_index - 1;
Does anywhere verify that primary_domain_index <= aa.array_sz?
> +
Re: [PATCH v5 4/6] powerpc/pseries: Consolidate different NUMA distance update code paths
Hi "Aneesh,
I love your patch! Yet something to improve:
[auto build test ERROR on powerpc/next]
[also build test ERROR on v5.13 next-20210628]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/0day-ci/linux/commits/Aneesh-Kumar-K-V/Add-support-for-FORM2-associativity/20210628-231546
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-randconfig-r024-20210628 (attached as .config)
compiler: powerpc64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
#
https://github.com/0day-ci/linux/commit/fcbc8b19e99b1cf44fde904817f19616c6baecdb
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Aneesh-Kumar-K-V/Add-support-for-FORM2-associativity/20210628-231546
git checkout fcbc8b19e99b1cf44fde904817f19616c6baecdb
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross
ARCH=powerpc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All errors (new ones prefixed by >>):
arch/powerpc/mm/numa.c:298:6: warning: no previous prototype for
'update_numa_distance' [-Wmissing-prototypes]
298 | void update_numa_distance(struct device_node *node)
| ^~~~
arch/powerpc/mm/numa.c: In function 'parse_numa_properties':
>> arch/powerpc/mm/numa.c:809:7: error: implicit declaration of function
>> '__vphn_get_associativity'; did you mean 'of_get_associativity'?
>> [-Werror=implicit-function-declaration]
809 | if (__vphn_get_associativity(i, vphn_assoc) == 0) {
| ^~~~
| of_get_associativity
cc1: some warnings being treated as errors
vim +809 arch/powerpc/mm/numa.c
771
772 static int __init parse_numa_properties(void)
773 {
774 struct device_node *memory;
775 int default_nid = 0;
776 unsigned long i;
777 const __be32 *associativity;
778
779 if (numa_enabled == 0) {
780 printk(KERN_WARNING "NUMA disabled by user\n");
781 return -1;
782 }
783
784 primary_domain_index = find_primary_domain_index();
785
786 if (primary_domain_index < 0) {
787 /*
788 * if we fail to parse primary_domain_index from device
tree
789 * mark the numa disabled, boot with numa disabled.
790 */
791 numa_enabled = false;
792 return primary_domain_index;
793 }
794
795 dbg("NUMA associativity depth for CPU/Memory: %d\n",
primary_domain_index);
796
797 /*
798 * Even though we connect cpus to numa domains later in SMP
799 * init, we need to know the node ids now. This is because
800 * each node to be onlined must have NODE_DATA etc backing it.
801 */
802 for_each_present_cpu(i) {
803 __be32 vphn_assoc[VPHN_ASSOC_BUFSIZE];
804 struct device_node *cpu;
805 int nid = NUMA_NO_NODE;
806
807 memset(vphn_assoc, 0, VPHN_ASSOC_BUFSIZE *
sizeof(__be32));
808
> 809 if (__vphn_get_associativity(i, vphn_assoc) == 0) {
810 nid = associativity_to_nid(vphn_assoc);
811 __initialize_form1_numa_distance(vphn_assoc);
812 } else {
813
814 /*
815 * Don't fall back to default_nid yet -- we
will plug
816 * cpus into nodes once the memory scan has
discovered
817 * the topology.
818 */
819 cpu = of_get_cpu_node(i, NULL);
820 BUG_ON(!cpu);
821
822 associativity = of_get_associativity(cpu);
823 if (associativity) {
824 nid =
associativity_to_nid(associativity);
825
__initialize_form1_numa_distance(associativity);
826 }
827 of_node_put(cpu);
828 }
829
830 node_set_online(nid);
831 }
832
833 get_n_mem_cells(&n_mem_addr_cells, &n_mem_size_cells);
834
835 for_each_node_by_type(memory, "memory") {
836
Re: [PATCH v5 4/6] powerpc/pseries: Consolidate different NUMA distance update code paths
Hi "Aneesh,
I love your patch! Perhaps something to improve:
[auto build test WARNING on powerpc/next]
[also build test WARNING on v5.13 next-20210628]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/0day-ci/linux/commits/Aneesh-Kumar-K-V/Add-support-for-FORM2-associativity/20210628-231546
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-allyesconfig (attached as .config)
compiler: powerpc64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
#
https://github.com/0day-ci/linux/commit/fcbc8b19e99b1cf44fde904817f19616c6baecdb
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Aneesh-Kumar-K-V/Add-support-for-FORM2-associativity/20210628-231546
git checkout fcbc8b19e99b1cf44fde904817f19616c6baecdb
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross
ARCH=powerpc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All warnings (new ones prefixed by >>):
>> arch/powerpc/mm/numa.c:298:6: warning: no previous prototype for
>> 'update_numa_distance' [-Wmissing-prototypes]
298 | void update_numa_distance(struct device_node *node)
| ^~~~
vim +/update_numa_distance +298 arch/powerpc/mm/numa.c
294
295 /*
296 * Used to update distance information w.r.t newly added node.
297 */
> 298 void update_numa_distance(struct device_node *node)
299 {
300 if (affinity_form == FORM0_AFFINITY)
301 return;
302 else if (affinity_form == FORM1_AFFINITY) {
303 initialize_form1_numa_distance(node);
304 return;
305 }
306 }
307
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
.config.gz
Description: application/gzip
[PATCH v5 4/6] powerpc/pseries: Consolidate different NUMA distance update code paths
The associativity details of the newly added resourced are collected from
the hypervisor via "ibm,configure-connector" rtas call. Update the numa
distance details of the newly added numa node after the above call.
Instead of updating NUMA distance every time we lookup a node id
from the associativity property, add helpers that can be used
during boot which does this only once. Also remove the distance
update from node id lookup helpers.
Signed-off-by: Aneesh Kumar K.V
---
arch/powerpc/mm/numa.c| 173 +-
arch/powerpc/platforms/pseries/hotplug-cpu.c | 2 +
.../platforms/pseries/hotplug-memory.c| 2 +
arch/powerpc/platforms/pseries/pseries.h | 1 +
4 files changed, 132 insertions(+), 46 deletions(-)
diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
index 0ec16999beef..7b142f79d600 100644
--- a/arch/powerpc/mm/numa.c
+++ b/arch/powerpc/mm/numa.c
@@ -208,22 +208,6 @@ int __node_distance(int a, int b)
}
EXPORT_SYMBOL(__node_distance);
-static void initialize_distance_lookup_table(int nid,
- const __be32 *associativity)
-{
- int i;
-
- if (affinity_form != FORM1_AFFINITY)
- return;
-
- for (i = 0; i < max_associativity_domain_index; i++) {
- const __be32 *entry;
-
- entry = &associativity[be32_to_cpu(distance_ref_points[i]) - 1];
- distance_lookup_table[nid][i] = of_read_number(entry, 1);
- }
-}
-
/*
* Returns nid in the range [0..nr_node_ids], or -1 if no useful NUMA
* info is found.
@@ -241,15 +225,6 @@ static int associativity_to_nid(const __be32
*associativity)
/* POWER4 LPAR uses 0x as invalid node */
if (nid == 0x || nid >= nr_node_ids)
nid = NUMA_NO_NODE;
-
- if (nid > 0 &&
- of_read_number(associativity, 1) >=
max_associativity_domain_index) {
- /*
-* Skip the length field and send start of associativity array
-*/
- initialize_distance_lookup_table(nid, associativity + 1);
- }
-
out:
return nid;
}
@@ -287,6 +262,49 @@ int of_node_to_nid(struct device_node *device)
}
EXPORT_SYMBOL(of_node_to_nid);
+static void __initialize_form1_numa_distance(const __be32 *associativity)
+{
+ int i, nid;
+
+ if (affinity_form != FORM1_AFFINITY)
+ return;
+
+ if (of_read_number(associativity, 1) >= primary_domain_index) {
+ nid = of_read_number(&associativity[primary_domain_index], 1);
+
+ for (i = 0; i < max_associativity_domain_index; i++) {
+ const __be32 *entry;
+
+ entry =
&associativity[be32_to_cpu(distance_ref_points[i])];
+ distance_lookup_table[nid][i] = of_read_number(entry,
1);
+ }
+ }
+}
+
+static void initialize_form1_numa_distance(struct device_node *node)
+{
+ const __be32 *associativity;
+
+ associativity = of_get_associativity(node);
+ if (!associativity)
+ return;
+
+ __initialize_form1_numa_distance(associativity);
+}
+
+/*
+ * Used to update distance information w.r.t newly added node.
+ */
+void update_numa_distance(struct device_node *node)
+{
+ if (affinity_form == FORM0_AFFINITY)
+ return;
+ else if (affinity_form == FORM1_AFFINITY) {
+ initialize_form1_numa_distance(node);
+ return;
+ }
+}
+
static int __init find_primary_domain_index(void)
{
int index;
@@ -433,6 +451,48 @@ static int of_get_assoc_arrays(struct assoc_arrays *aa)
return 0;
}
+static int get_nid_and_numa_distance(struct drmem_lmb *lmb)
+{
+ struct assoc_arrays aa = { .arrays = NULL };
+ int default_nid = NUMA_NO_NODE;
+ int nid = default_nid;
+ int rc, index;
+
+ if ((primary_domain_index < 0) || !numa_enabled)
+ return default_nid;
+
+ rc = of_get_assoc_arrays(&aa);
+ if (rc)
+ return default_nid;
+
+ if (primary_domain_index <= aa.array_sz &&
+ !(lmb->flags & DRCONF_MEM_AI_INVALID) && lmb->aa_index <
aa.n_arrays) {
+ index = lmb->aa_index * aa.array_sz + primary_domain_index - 1;
+ nid = of_read_number(&aa.arrays[index], 1);
+
+ if (nid == 0x || nid >= nr_node_ids)
+ nid = default_nid;
+ if (nid > 0 && affinity_form == FORM1_AFFINITY) {
+ int i;
+ const __be32 *associativity;
+
+ index = lmb->aa_index * aa.array_sz;
+ associativity = &aa.arrays[index];
+ /*
+* lookup array associativity entries have different
format
+* There is no length of the array as the first element.
+*/
+ for (i = 0;
