On 03/25/2016 08:22 PM, Andrew Morton wrote:
On Fri, 25 Mar 2016 14:56:04 +0800 Xishi Qiu <qiuxi...@huawei.com> wrote:
It is incorrect to use next_node to find a target node, it will
return MAX_NUMNODES or invalid node. This will lead to crash in
buddy system allocation.
...
--- a/mm/page_isolation.c
+++ b/mm/page_isolation.c
@@ -289,11 +289,11 @@ struct page *alloc_migrate_target(struct page *page,
unsigned long private,
* now as a simple work-around, we use the next node for destination.
*/
if (PageHuge(page)) {
- nodemask_t src = nodemask_of_node(page_to_nid(page));
- nodemask_t dst;
- nodes_complement(dst, src);
+ int node = next_online_node(page_to_nid(page));
+ if (node == MAX_NUMNODES)
+ node = first_online_node;
return alloc_huge_page_node(page_hstate(compound_head(page)),
- next_node(page_to_nid(page), dst));
+ node);
}
if (PageHighMem(page))
Indeed. Can you tell us more about this circumstances under which the
kernel will crash? I need to decide which kernel version(s) need the
patch, but the changelog doesn't contain the info needed to make this
decision (it should).
next_node() isn't a very useful interface, really. Just about every
caller does this:
node = next_node(node, XXX);
if (node == MAX_NUMNODES)
node = first_node(XXX);
so how about we write a function which does that, and stop open-coding
the same thing everywhere?
Good idea.
And I think your fix could then use such a function:
int node = that_new_function(page_to_nid(page), node_online_map);
Also, mm/mempolicy.c:offset_il_node() worries me:
do {
nid = next_node(nid, pol->v.nodes);
c++;
} while (c <= target);
Can't `nid' hit MAX_NUMNODES?
AFAICS it can. interleave_nid() uses this and the nid is then used e.g.
in node_zonelist() where it's used for NODE_DATA(nid). That's quite
scary. It also predates git. Why don't we see crashes or KASAN finding this?
And can someone please explain mem_cgroup_select_victim_node() to me?
How can we hit the "node = numa_node_id()" path? Only if
memcg->scan_nodes is empty? is that even valid? The comment seems to
have not much to do with the code?
I understand the comment that it's valid to be empty and the comment
lists reasons why that can happen (with somewhat broken language). Note
that I didn't verify these reasons:
- we call this when hitting memcg limit, not when adding pages to LRU,
as adding to LRU means it would contain the given LRU's node
- adding to unevictable LRU means it's not added to scan_nodes (probably
because scanning unevictable lru would be useless)
- for other reasons (which?) it might have pages not on LRU and it's so
small there are no other pages that would be on LRU
mpol_rebind_nodemask() is similar.
Something like this?
From: Andrew Morton <a...@linux-foundation.org>
Subject: include/linux/nodemask.h: create next_node_in() helper
Lots of code does
node = next_node(node, XXX);
if (node == MAX_NUMNODES)
node = first_node(XXX);
so create next_node_in() to do this and use it in various places.
Cc: Xishi Qiu <qiuxi...@huawei.com>
Cc: Vlastimil Babka <vba...@suse.cz>
Acked-by: Vlastimil Babka <vba...@suse.cz>
Patch doesn't address offset_il_node() which is good, because if it's
indeed buggy, it's serious and needs a non-cleanup patch.
