| Message ID | 20201230114014.GA1934427@ubuntu-A520I-AC (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/page_alloc: remove the static for local variable node_order | expand |
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.
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.
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.
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.
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??)
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?
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).
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;
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(-)