[PATCH] mm/page_alloc: remove the static for local variable node_order

[PATCH] mm/page_alloc: remove the static for local variable node_order

[Hui Su]

local variable node_order do not need the static here.

Signed-off-by: Hui Su <sh_def@163.com>
---
 mm/page_alloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index bdbec4c98173..45e049ccf117 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -5864,7 +5864,7 @@ static void build_thisnode_zonelists(pg_data_t *pgdat)
 
 static void build_zonelists(pg_data_t *pgdat)
 {
-	static int node_order[MAX_NUMNODES];
+	int node_order[MAX_NUMNODES];
 	int node, load, nr_nodes = 0;
 	nodemask_t used_mask = NODE_MASK_NONE;
 	int local_node, prev_node;
-- 
2.25.1

Re: [PATCH] mm/page_alloc: remove the static for local variable node_order

[Matthew Wilcox]

On Wed, Dec 30, 2020 at 07:40:14PM +0800, Hui Su wrote:
> local variable node_order do not need the static here.

It bloody well does.  It can be up to 2^10 entries on x86 (and larger
on others) That's 4kB which you've now moved onto the stack.

Please, learn more about what you're doing.  I suggest sending patches
to drivers/staging; that will help you learn how to submit patches to
linux.

Re: [PATCH] mm/page_alloc: remove the static for local variable node_order

[Hui Su]

Hi, Matthew:

On Wed, Dec 30, 2020 at 12:42:33PM +0000, Matthew Wilcox wrote:
> On Wed, Dec 30, 2020 at 07:40:14PM +0800, Hui Su wrote:
> > local variable node_order do not need the static here.
> 
> It bloody well does.  It can be up to 2^10 entries on x86 (and larger
> on others) That's 4kB which you've now moved onto the stack.
> 
Thanks for your explanation, this change will put too much onto the stack
in some cases which i didn't consider.

Please ignore this change.

> Please, learn more about what you're doing.  I suggest sending patches
> to drivers/staging; that will help you learn how to submit patches to
> linux.

Re: [PATCH] mm/page_alloc: remove the static for local variable node_order

[Muchun Song]

On Wed, Dec 30, 2020 at 8:45 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Wed, Dec 30, 2020 at 07:40:14PM +0800, Hui Su wrote:
> > local variable node_order do not need the static here.
>
> It bloody well does.  It can be up to 2^10 entries on x86 (and larger
> on others) That's 4kB which you've now moved onto the stack.

This is not the first time I have seen similar changes. So what
do you think about adding a comment here to avoid another one
do this in the feature?

>
> Please, learn more about what you're doing.  I suggest sending patches
> to drivers/staging; that will help you learn how to submit patches to
> linux.

Re: [PATCH] mm/page_alloc: remove the static for local variable node_order

[Muchun Song]

On Wed, Dec 30, 2020 at 8:45 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Wed, Dec 30, 2020 at 07:40:14PM +0800, Hui Su wrote:
> > local variable node_order do not need the static here.
>
> It bloody well does.  It can be up to 2^10 entries on x86 (and larger
> on others) That's 4kB which you've now moved onto the stack.

This is not the first time I have seen similar changes. So what
do you think about adding a comment here to avoid another one
do this in the feature?

>
> Please, learn more about what you're doing.  I suggest sending patches
> to drivers/staging; that will help you learn how to submit patches to
> linux.

Re: [PATCH] mm/page_alloc: remove the static for local variable node_order

[Michal Hocko]

On Wed 30-12-20 21:41:30, Muchun Song wrote:
> On Wed, Dec 30, 2020 at 8:45 PM Matthew Wilcox <willy@infradead.org> wrote:
> >
> > On Wed, Dec 30, 2020 at 07:40:14PM +0800, Hui Su wrote:
> > > local variable node_order do not need the static here.
> >
> > It bloody well does.  It can be up to 2^10 entries on x86 (and larger
> > on others) That's 4kB which you've now moved onto the stack.
> 
> This is not the first time I have seen similar changes. So what
> do you think about adding a comment here to avoid another one
> do this in the feature?

Well, this is not an unusual technieque to reduce the stack space. I am
not really sure we really need to put an explicit comment about that.  I
would appreciate much more if patch submitters took an extra step when
creating seemingly trivial patches and either consult the history of the
respective code or look for a similar pattern elsewhere before sending
them.

I do agree with Willy that mm code is usually not a great place to
search for trivial patches. First of all most people tend to be pretty
busy with other reviewes and the code has grown rather delicate and
tricky so each review is non trivial.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm/page_alloc: remove the static for local variable node_order

[Andrew Morton]

On Wed, 30 Dec 2020 12:42:33 +0000 Matthew Wilcox <willy@infradead.org> wrote:

> On Wed, Dec 30, 2020 at 07:40:14PM +0800, Hui Su wrote:
> > local variable node_order do not need the static here.
> 
> It bloody well does.  It can be up to 2^10 entries on x86 (and larger
> on others) That's 4kB which you've now moved onto the stack.

That being said, could we kmalloc the scratch area in
__build_all_zonelists()?  And maybe remove that static spinlock?

(what blocks node and cpu hotplug in there??)

Re: [PATCH] mm/page_alloc: remove the static for local variable node_order

[Matthew Wilcox]

On Mon, Jan 04, 2021 at 03:23:57PM -0800, Andrew Morton wrote:
> On Wed, 30 Dec 2020 12:42:33 +0000 Matthew Wilcox <willy@infradead.org> wrote:
> 
> > On Wed, Dec 30, 2020 at 07:40:14PM +0800, Hui Su wrote:
> > > local variable node_order do not need the static here.
> > 
> > It bloody well does.  It can be up to 2^10 entries on x86 (and larger
> > on others) That's 4kB which you've now moved onto the stack.
> 
> That being said, could we kmalloc the scratch area in
> __build_all_zonelists()?  And maybe remove that static spinlock?
> 
> (what blocks node and cpu hotplug in there??)

if we don't have the zonelists built yet, can slab possibly be operational?

Re: [PATCH] mm/page_alloc: remove the static for local variable node_order

[Michal Hocko]

On Mon 04-01-21 15:23:57, Andrew Morton wrote:
> On Wed, 30 Dec 2020 12:42:33 +0000 Matthew Wilcox <willy@infradead.org> wrote:
> 
> > On Wed, Dec 30, 2020 at 07:40:14PM +0800, Hui Su wrote:
> > > local variable node_order do not need the static here.
> > 
> > It bloody well does.  It can be up to 2^10 entries on x86 (and larger
> > on others) That's 4kB which you've now moved onto the stack.
> 
> That being said, could we kmalloc the scratch area in
> __build_all_zonelists()?  And maybe remove that static spinlock?

I am not sure we can (e.g. early init code) but even if we could, what
would be an advantage. This code is called very seldom with a very
shallow stacks so using the stack allocation sounds like the easiest
thing to do.

> (what blocks node and cpu hotplug in there??)

Memory hotplug is excluded by the caller when it matters (e.g. no
locking for the early init).
-- 
Michal Hocko
SUSE Labs