| Message ID | 20200725080749.70470-1-songmuchun@bytedance.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] mm/hugetlb: add mempolicy check in the reservation routine | expand |
On 7/25/20 1:07 AM, Muchun Song wrote: > In the reservation routine, we only check whether the cpuset meets > the memory allocation requirements. But we ignore the mempolicy of > MPOL_BIND case. If someone mmap hugetlb succeeds, but the subsequent > memory allocation may fail due to mempolicy restrictions and receives > the SIGBUS signal. This can be reproduced by the follow steps. > > 1) Compile the test case. > cd tools/testing/selftests/vm/ > gcc map_hugetlb.c -o map_hugetlb > > 2) Pre-allocate huge pages. Suppose there are 2 numa nodes in the > system. Each node will pre-allocate one huge page. > echo 2 > /proc/sys/vm/nr_hugepages > > 3) Run test case(mmap 4MB). We receive the SIGBUS signal. > numactl --membind=0 ./map_hugetlb 4 > > With this patch applied, the mmap will fail in the step 3) and throw > "mmap: Cannot allocate memory". > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > Reported-by: Jianchao Guo <guojianchao@bytedance.com> > Suggested-by: Michal Hocko <mhocko@kernel.org> Thank you Muchun and Michal, This patch forced me to once again look at the bad the interaction of hugetlb reservations and cpusets or mempolicy. This new code will help produce a quick failure as described in the commit message, and it does not make existing interactions any worse. Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>
On Sat, 25 Jul 2020 16:07:49 +0800 Muchun Song <songmuchun@bytedance.com> wrote: > In the reservation routine, we only check whether the cpuset meets > the memory allocation requirements. But we ignore the mempolicy of > MPOL_BIND case. If someone mmap hugetlb succeeds, but the subsequent > memory allocation may fail due to mempolicy restrictions and receives > the SIGBUS signal. This can be reproduced by the follow steps. > > 1) Compile the test case. > cd tools/testing/selftests/vm/ > gcc map_hugetlb.c -o map_hugetlb > > 2) Pre-allocate huge pages. Suppose there are 2 numa nodes in the > system. Each node will pre-allocate one huge page. > echo 2 > /proc/sys/vm/nr_hugepages > > 3) Run test case(mmap 4MB). We receive the SIGBUS signal. > numactl --membind=0 ./map_hugetlb 4 > > With this patch applied, the mmap will fail in the step 3) and throw > "mmap: Cannot allocate memory". This doesn't compile with CONFIG_NUMA=n - ther eis no implementation of get_task_policy(). I think it needs more than a simple build fix - can we please rework the patch so that its impact (mainly code size) on non-NUMA machines is minimized?
On Tue, Jul 28, 2020 at 8:19 AM Andrew Morton <akpm@linux-foundation.org> wrote: > > On Sat, 25 Jul 2020 16:07:49 +0800 Muchun Song <songmuchun@bytedance.com> wrote: > > > In the reservation routine, we only check whether the cpuset meets > > the memory allocation requirements. But we ignore the mempolicy of > > MPOL_BIND case. If someone mmap hugetlb succeeds, but the subsequent > > memory allocation may fail due to mempolicy restrictions and receives > > the SIGBUS signal. This can be reproduced by the follow steps. > > > > 1) Compile the test case. > > cd tools/testing/selftests/vm/ > > gcc map_hugetlb.c -o map_hugetlb > > > > 2) Pre-allocate huge pages. Suppose there are 2 numa nodes in the > > system. Each node will pre-allocate one huge page. > > echo 2 > /proc/sys/vm/nr_hugepages > > > > 3) Run test case(mmap 4MB). We receive the SIGBUS signal. > > numactl --membind=0 ./map_hugetlb 4 > > > > With this patch applied, the mmap will fail in the step 3) and throw > > "mmap: Cannot allocate memory". > > This doesn't compile with CONFIG_NUMA=n - ther eis no implementation of > get_task_policy(). > > I think it needs more than a simple build fix - can we please rework > the patch so that its impact (mainly code size) on non-NUMA machines is > minimized? > OK. I will do that, thanks.
On 7/27/20 5:19 PM, Andrew Morton wrote: > On Sat, 25 Jul 2020 16:07:49 +0800 Muchun Song <songmuchun@bytedance.com> wrote: > >> In the reservation routine, we only check whether the cpuset meets >> the memory allocation requirements. But we ignore the mempolicy of >> MPOL_BIND case. If someone mmap hugetlb succeeds, but the subsequent >> memory allocation may fail due to mempolicy restrictions and receives >> the SIGBUS signal. This can be reproduced by the follow steps. >> >> 1) Compile the test case. >> cd tools/testing/selftests/vm/ >> gcc map_hugetlb.c -o map_hugetlb >> >> 2) Pre-allocate huge pages. Suppose there are 2 numa nodes in the >> system. Each node will pre-allocate one huge page. >> echo 2 > /proc/sys/vm/nr_hugepages >> >> 3) Run test case(mmap 4MB). We receive the SIGBUS signal. >> numactl --membind=0 ./map_hugetlb 4 >> >> With this patch applied, the mmap will fail in the step 3) and throw >> "mmap: Cannot allocate memory". > > This doesn't compile with CONFIG_NUMA=n - ther eis no implementation of > get_task_policy(). > > I think it needs more than a simple build fix - can we please rework > the patch so that its impact (mainly code size) on non-NUMA machines is > minimized? I'll let Muchun see if there is a more elegant fix. However, a relatively simple build fix such as: diff --git a/mm/hugetlb.c b/mm/hugetlb.c index 8069ca47c18c..4bfbddfee0d3 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -3455,12 +3455,14 @@ static unsigned int allowed_mems_nr(struct hstate *h) { int node; unsigned int nr = 0; - struct mempolicy *mpol = get_task_policy(current); - nodemask_t *mpol_allowed; + nodemask_t *mpol_allowed = NULL; unsigned int *array = h->free_huge_pages_node; +#ifdef CONFIG_NUMA + struct mempolicy *mpol = get_task_policy(current); gfp_t gfp_mask = htlb_alloc_mask(h); mpol_allowed = policy_nodemask(gfp_mask, mpol); +#endif for_each_node_mask(node, cpuset_current_mems_allowed) { if (!mpol_allowed || Does not have much of an impact on code size. Here are the non-numa versions of the routine before Muchun's patch and after. Dump of assembler code for function cpuset_mems_nr: 0xffffffff8126a3a0 <+0>: callq 0xffffffff81060f80 <__fentry__> 0xffffffff8126a3a5 <+5>: xor %eax,%eax 0xffffffff8126a3a7 <+7>: mov %gs:0x17bc0,%rdx 0xffffffff8126a3b0 <+16>: testb $0x1,0x778(%rdx) 0xffffffff8126a3b7 <+23>: jne 0xffffffff8126a3ba <cpuset_mems_nr+26> 0xffffffff8126a3b9 <+25>: retq 0xffffffff8126a3ba <+26>: mov (%rdi),%eax 0xffffffff8126a3bc <+28>: retq End of assembler dump. Dump of assembler code for function allowed_mems_nr: 0xffffffff8126a3a0 <+0>: callq 0xffffffff81060f80 <__fentry__> 0xffffffff8126a3a5 <+5>: xor %eax,%eax 0xffffffff8126a3a7 <+7>: mov %gs:0x17bc0,%rdx 0xffffffff8126a3b0 <+16>: testb $0x1,0x778(%rdx) 0xffffffff8126a3b7 <+23>: jne 0xffffffff8126a3ba <allowed_mems_nr+26> 0xffffffff8126a3b9 <+25>: retq 0xffffffff8126a3ba <+26>: mov 0x6c(%rdi),%eax 0xffffffff8126a3bd <+29>: retq End of assembler dump.
diff --git a/include/linux/mempolicy.h b/include/linux/mempolicy.h index ea9c15b60a96..6b9640f1c990 100644 --- a/include/linux/mempolicy.h +++ b/include/linux/mempolicy.h @@ -152,6 +152,7 @@ extern int huge_node(struct vm_area_struct *vma, extern bool init_nodemask_of_mempolicy(nodemask_t *mask); extern bool mempolicy_nodemask_intersects(struct task_struct *tsk, const nodemask_t *mask); +extern nodemask_t *policy_nodemask(gfp_t gfp, struct mempolicy *policy); extern unsigned int mempolicy_slab_node(void); extern enum zone_type policy_zone; diff --git a/mm/hugetlb.c b/mm/hugetlb.c index 589c330df4db..6f8e79e76676 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -3463,13 +3463,22 @@ static int __init default_hugepagesz_setup(char *s) } __setup("default_hugepagesz=", default_hugepagesz_setup); -static unsigned int cpuset_mems_nr(unsigned int *array) +static unsigned int allowed_mems_nr(struct hstate *h) { int node; unsigned int nr = 0; + struct mempolicy *mpol = get_task_policy(current); + nodemask_t *mpol_allowed; + unsigned int *array = h->free_huge_pages_node; + gfp_t gfp_mask = htlb_alloc_mask(h); + + mpol_allowed = policy_nodemask(gfp_mask, mpol); - for_each_node_mask(node, cpuset_current_mems_allowed) - nr += array[node]; + for_each_node_mask(node, cpuset_current_mems_allowed) { + if (!mpol_allowed || + (mpol_allowed && node_isset(node, *mpol_allowed))) + nr += array[node]; + } return nr; } @@ -3648,12 +3657,18 @@ static int hugetlb_acct_memory(struct hstate *h, long delta) * we fall back to check against current free page availability as * a best attempt and hopefully to minimize the impact of changing * semantics that cpuset has. + * + * Apart from cpuset, we also have memory policy mechanism that + * also determines from which node the kernel will allocate memory + * in a NUMA system. So similar to cpuset, we also should consider + * the memory policy of the current task. Similar to the description + * above. */ if (delta > 0) { if (gather_surplus_pages(h, delta) < 0) goto out; - if (delta > cpuset_mems_nr(h->free_huge_pages_node)) { + if (delta > allowed_mems_nr(h)) { return_unused_surplus_pages(h, delta); goto out; } diff --git a/mm/mempolicy.c b/mm/mempolicy.c index 93fcfc1f2fa2..fce14c3f4f38 100644 --- a/mm/mempolicy.c +++ b/mm/mempolicy.c @@ -1873,7 +1873,7 @@ static int apply_policy_zone(struct mempolicy *policy, enum zone_type zone) * Return a nodemask representing a mempolicy for filtering nodes for * page allocation */ -static nodemask_t *policy_nodemask(gfp_t gfp, struct mempolicy *policy) +nodemask_t *policy_nodemask(gfp_t gfp, struct mempolicy *policy) { /* Lower zones don't get a nodemask applied for MPOL_BIND */ if (unlikely(policy->mode == MPOL_BIND) &&
In the reservation routine, we only check whether the cpuset meets the memory allocation requirements. But we ignore the mempolicy of MPOL_BIND case. If someone mmap hugetlb succeeds, but the subsequent memory allocation may fail due to mempolicy restrictions and receives the SIGBUS signal. This can be reproduced by the follow steps. 1) Compile the test case. cd tools/testing/selftests/vm/ gcc map_hugetlb.c -o map_hugetlb 2) Pre-allocate huge pages. Suppose there are 2 numa nodes in the system. Each node will pre-allocate one huge page. echo 2 > /proc/sys/vm/nr_hugepages 3) Run test case(mmap 4MB). We receive the SIGBUS signal. numactl --membind=0 ./map_hugetlb 4 With this patch applied, the mmap will fail in the step 3) and throw "mmap: Cannot allocate memory". Signed-off-by: Muchun Song <songmuchun@bytedance.com> Reported-by: Jianchao Guo <guojianchao@bytedance.com> Suggested-by: Michal Hocko <mhocko@kernel.org> --- chagelog in v3: 1) Do not allocate nodemask on the stack. 2) Update comment. changelog in v2: 1) Reuse policy_nodemask(). include/linux/mempolicy.h | 1 + mm/hugetlb.c | 23 +++++++++++++++++++---- mm/mempolicy.c | 2 +- 3 files changed, 21 insertions(+), 5 deletions(-)