Re: [PATCH] mm/page_alloc: remove the static for local variable node_order
On Mon 04-01-21 15:23:57, Andrew Morton wrote: > On Wed, 30 Dec 2020 12:42:33 + 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. > > 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
Re: [PATCH] mm/page_alloc: remove the static for local variable node_order
On Mon, Jan 04, 2021 at 03:23:57PM -0800, Andrew Morton wrote: > On Wed, 30 Dec 2020 12:42:33 + 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. > > 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
On Wed, 30 Dec 2020 12:42:33 + 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. 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
On Wed 30-12-20 21:41:30, Muchun Song wrote: > On Wed, Dec 30, 2020 at 8:45 PM 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. > > 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
On Wed, Dec 30, 2020 at 8:45 PM 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. 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
Hi, Matthew: On Wed, Dec 30, 2020 at 12:42:33PM +, 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
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.
