| Message ID | 20220408025947.1619-2-richard.weiyang@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] mm/page_alloc: add same penalty is enough to get round-robin order | expand |
On 08.04.22 04:59, Wei Yang wrote: > Since we just increase a constance of 1 to node penalty, it is not > necessary to multiply MAX_NODE_LOAD for preference. > > This patch also remove the definition. > > [vbabka@suse.cz: suggests] > > Signed-off-by: Wei Yang <richard.weiyang@gmail.com> > CC: Vlastimil Babka <vbabka@suse.cz> > CC: David Hildenbrand <david@redhat.com> > CC: Oscar Salvador <osalvador@suse.de> > --- > mm/page_alloc.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 86b6573fbeb5..ca6a127bbc26 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -6170,7 +6170,6 @@ int numa_zonelist_order_handler(struct ctl_table *table, int write, > } > > > -#define MAX_NODE_LOAD (nr_online_nodes) > static int node_load[MAX_NUMNODES]; > > /** > @@ -6217,7 +6216,7 @@ int find_next_best_node(int node, nodemask_t *used_node_mask) > val += PENALTY_FOR_NODE_WITH_CPUS; > > /* Slight preference for less loaded node */ > - val *= (MAX_NODE_LOAD*MAX_NUMNODES); > + val *= MAX_NUMNODES; > val += node_load[n]; > > if (val < min_val) { I feel like this should be squashed into the previous patch. It has the same effect of making this code independent of nr_online_nodes. And I had to scratch my head a couple of times in patch #1 why the change in patch #1 is fine with thus remaining in place. Having that said, I consider this code highly unnecessary over-complicated at first sight. Removing some of the magic most certainly is very welcome. This semantics of the global variable node_load[] remains mostly mysterious for me.
On Fri, Apr 08, 2022 at 10:09:48AM +0200, David Hildenbrand wrote: >On 08.04.22 04:59, Wei Yang wrote: >> Since we just increase a constance of 1 to node penalty, it is not >> necessary to multiply MAX_NODE_LOAD for preference. >> >> This patch also remove the definition. >> >> [vbabka@suse.cz: suggests] >> >> Signed-off-by: Wei Yang <richard.weiyang@gmail.com> >> CC: Vlastimil Babka <vbabka@suse.cz> >> CC: David Hildenbrand <david@redhat.com> >> CC: Oscar Salvador <osalvador@suse.de> >> --- >> mm/page_alloc.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/mm/page_alloc.c b/mm/page_alloc.c >> index 86b6573fbeb5..ca6a127bbc26 100644 >> --- a/mm/page_alloc.c >> +++ b/mm/page_alloc.c >> @@ -6170,7 +6170,6 @@ int numa_zonelist_order_handler(struct ctl_table *table, int write, >> } >> >> >> -#define MAX_NODE_LOAD (nr_online_nodes) >> static int node_load[MAX_NUMNODES]; >> >> /** >> @@ -6217,7 +6216,7 @@ int find_next_best_node(int node, nodemask_t *used_node_mask) >> val += PENALTY_FOR_NODE_WITH_CPUS; >> >> /* Slight preference for less loaded node */ >> - val *= (MAX_NODE_LOAD*MAX_NUMNODES); >> + val *= MAX_NUMNODES; >> val += node_load[n]; >> >> if (val < min_val) { > >I feel like this should be squashed into the previous patch. It has the >same effect of making this code independent of nr_online_nodes. And I >had to scratch my head a couple of times in patch #1 why the change in >patch #1 is fine with thus remaining in place. > > >Having that said, I consider this code highly unnecessary >over-complicated at first sight. Removing some of the magic most >certainly is very welcome. > >This semantics of the global variable node_load[] remains mostly >mysterious for me. > So the suggestion is a v3 with #1 and #2 squashed? >-- >Thanks, > >David / dhildenb
On 4/9/22 01:07, Wei Yang wrote: > On Fri, Apr 08, 2022 at 10:09:48AM +0200, David Hildenbrand wrote: >>On 08.04.22 04:59, Wei Yang wrote: >>> Since we just increase a constance of 1 to node penalty, it is not >>> necessary to multiply MAX_NODE_LOAD for preference. >>> >>> This patch also remove the definition. >>> >>> [vbabka@suse.cz: suggests] >>> >>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com> >>> CC: Vlastimil Babka <vbabka@suse.cz> >>> CC: David Hildenbrand <david@redhat.com> >>> CC: Oscar Salvador <osalvador@suse.de> >>> --- >>> mm/page_alloc.c | 3 +-- >>> 1 file changed, 1 insertion(+), 2 deletions(-) >>> >>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c >>> index 86b6573fbeb5..ca6a127bbc26 100644 >>> --- a/mm/page_alloc.c >>> +++ b/mm/page_alloc.c >>> @@ -6170,7 +6170,6 @@ int numa_zonelist_order_handler(struct ctl_table *table, int write, >>> } >>> >>> >>> -#define MAX_NODE_LOAD (nr_online_nodes) >>> static int node_load[MAX_NUMNODES]; >>> >>> /** >>> @@ -6217,7 +6216,7 @@ int find_next_best_node(int node, nodemask_t *used_node_mask) >>> val += PENALTY_FOR_NODE_WITH_CPUS; >>> >>> /* Slight preference for less loaded node */ >>> - val *= (MAX_NODE_LOAD*MAX_NUMNODES); >>> + val *= MAX_NUMNODES; >>> val += node_load[n]; >>> >>> if (val < min_val) { >> >>I feel like this should be squashed into the previous patch. It has the >>same effect of making this code independent of nr_online_nodes. And I >>had to scratch my head a couple of times in patch #1 why the change in >>patch #1 is fine with thus remaining in place. >> >> >>Having that said, I consider this code highly unnecessary >>over-complicated at first sight. Removing some of the magic most >>certainly is very welcome. >> >>This semantics of the global variable node_load[] remains mostly >>mysterious for me. Looks like after this patch(es), it would be "how many times was this node picked as the first fallback out of nodes with the same distance"? >> > > So the suggestion is a v3 with #1 and #2 squashed? Yes, and I agree with the suggestion. >>-- >>Thanks, >> >>David / dhildenb >
On Mon, Apr 11, 2022 at 12:52:33PM +0200, Vlastimil Babka wrote: >On 4/9/22 01:07, Wei Yang wrote: >> On Fri, Apr 08, 2022 at 10:09:48AM +0200, David Hildenbrand wrote: >>>On 08.04.22 04:59, Wei Yang wrote: >>>> Since we just increase a constance of 1 to node penalty, it is not >>>> necessary to multiply MAX_NODE_LOAD for preference. >>>> >>>> This patch also remove the definition. >>>> >>>> [vbabka@suse.cz: suggests] >>>> >>>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com> >>>> CC: Vlastimil Babka <vbabka@suse.cz> >>>> CC: David Hildenbrand <david@redhat.com> >>>> CC: Oscar Salvador <osalvador@suse.de> >>>> --- >>>> mm/page_alloc.c | 3 +-- >>>> 1 file changed, 1 insertion(+), 2 deletions(-) >>>> >>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c >>>> index 86b6573fbeb5..ca6a127bbc26 100644 >>>> --- a/mm/page_alloc.c >>>> +++ b/mm/page_alloc.c >>>> @@ -6170,7 +6170,6 @@ int numa_zonelist_order_handler(struct ctl_table *table, int write, >>>> } >>>> >>>> >>>> -#define MAX_NODE_LOAD (nr_online_nodes) >>>> static int node_load[MAX_NUMNODES]; >>>> >>>> /** >>>> @@ -6217,7 +6216,7 @@ int find_next_best_node(int node, nodemask_t *used_node_mask) >>>> val += PENALTY_FOR_NODE_WITH_CPUS; >>>> >>>> /* Slight preference for less loaded node */ >>>> - val *= (MAX_NODE_LOAD*MAX_NUMNODES); >>>> + val *= MAX_NUMNODES; >>>> val += node_load[n]; >>>> >>>> if (val < min_val) { >>> >>>I feel like this should be squashed into the previous patch. It has the >>>same effect of making this code independent of nr_online_nodes. And I >>>had to scratch my head a couple of times in patch #1 why the change in >>>patch #1 is fine with thus remaining in place. >>> >>> >>>Having that said, I consider this code highly unnecessary >>>over-complicated at first sight. Removing some of the magic most >>>certainly is very welcome. >>> >>>This semantics of the global variable node_load[] remains mostly >>>mysterious for me. > >Looks like after this patch(es), it would be "how many times was this node >picked as the first fallback out of nodes with the same distance"? > >>> >> >> So the suggestion is a v3 with #1 and #2 squashed? > >Yes, and I agree with the suggestion. No problem. > >>>-- >>>Thanks, >>> >>>David / dhildenb >>
On 11.04.22 12:52, Vlastimil Babka wrote: > On 4/9/22 01:07, Wei Yang wrote: >> On Fri, Apr 08, 2022 at 10:09:48AM +0200, David Hildenbrand wrote: >>> On 08.04.22 04:59, Wei Yang wrote: >>>> Since we just increase a constance of 1 to node penalty, it is not >>>> necessary to multiply MAX_NODE_LOAD for preference. >>>> >>>> This patch also remove the definition. >>>> >>>> [vbabka@suse.cz: suggests] >>>> >>>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com> >>>> CC: Vlastimil Babka <vbabka@suse.cz> >>>> CC: David Hildenbrand <david@redhat.com> >>>> CC: Oscar Salvador <osalvador@suse.de> >>>> --- >>>> mm/page_alloc.c | 3 +-- >>>> 1 file changed, 1 insertion(+), 2 deletions(-) >>>> >>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c >>>> index 86b6573fbeb5..ca6a127bbc26 100644 >>>> --- a/mm/page_alloc.c >>>> +++ b/mm/page_alloc.c >>>> @@ -6170,7 +6170,6 @@ int numa_zonelist_order_handler(struct ctl_table *table, int write, >>>> } >>>> >>>> >>>> -#define MAX_NODE_LOAD (nr_online_nodes) >>>> static int node_load[MAX_NUMNODES]; >>>> >>>> /** >>>> @@ -6217,7 +6216,7 @@ int find_next_best_node(int node, nodemask_t *used_node_mask) >>>> val += PENALTY_FOR_NODE_WITH_CPUS; >>>> >>>> /* Slight preference for less loaded node */ >>>> - val *= (MAX_NODE_LOAD*MAX_NUMNODES); >>>> + val *= MAX_NUMNODES; >>>> val += node_load[n]; >>>> >>>> if (val < min_val) { >>> >>> I feel like this should be squashed into the previous patch. It has the >>> same effect of making this code independent of nr_online_nodes. And I >>> had to scratch my head a couple of times in patch #1 why the change in >>> patch #1 is fine with thus remaining in place. >>> >>> >>> Having that said, I consider this code highly unnecessary >>> over-complicated at first sight. Removing some of the magic most >>> certainly is very welcome. >>> >>> This semantics of the global variable node_load[] remains mostly >>> mysterious for me. > > Looks like after this patch(es), it would be "how many times was this node > picked as the first fallback out of nodes with the same distance"? Yeah, that makes sense. I'd appreciate if we'd just stop using a global variable here and pass it as a parameter. But that's obviously an independent cleanup.
diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 86b6573fbeb5..ca6a127bbc26 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -6170,7 +6170,6 @@ int numa_zonelist_order_handler(struct ctl_table *table, int write, } -#define MAX_NODE_LOAD (nr_online_nodes) static int node_load[MAX_NUMNODES]; /** @@ -6217,7 +6216,7 @@ int find_next_best_node(int node, nodemask_t *used_node_mask) val += PENALTY_FOR_NODE_WITH_CPUS; /* Slight preference for less loaded node */ - val *= (MAX_NODE_LOAD*MAX_NUMNODES); + val *= MAX_NUMNODES; val += node_load[n]; if (val < min_val) {
Since we just increase a constance of 1 to node penalty, it is not necessary to multiply MAX_NODE_LOAD for preference. This patch also remove the definition. [vbabka@suse.cz: suggests] Signed-off-by: Wei Yang <richard.weiyang@gmail.com> CC: Vlastimil Babka <vbabka@suse.cz> CC: David Hildenbrand <david@redhat.com> CC: Oscar Salvador <osalvador@suse.de> --- mm/page_alloc.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)