| Message ID | 20200724100306.33457-1-songmuchun@bytedance.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] mm/hugetlb: add mempolicy check in the reservation routine | expand |
On Fri 24-07-20 18:03:06, 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". > > Reported-by: Jianchao Guo <guojianchao@bytedance.com> > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > --- > > changelog in v2: > 1) Reuse policy_nodemask(). > > include/linux/mempolicy.h | 1 + > mm/hugetlb.c | 19 ++++++++++++++++--- > mm/mempolicy.c | 2 +- > 3 files changed, 18 insertions(+), 4 deletions(-) > > 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..a753fe8591b4 100644 > --- a/mm/hugetlb.c > +++ b/mm/hugetlb.c > @@ -3463,12 +3463,25 @@ 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, *mems_allowed, nodemask; > + unsigned int *array = h->free_huge_pages_node; > + gfp_t gfp_mask = htlb_alloc_mask(h); > + > + mpol_allowed = policy_nodemask(gfp_mask, mpol); > + if (mpol_allowed) { > + nodes_and(nodemask, cpuset_current_mems_allowed, > + *mpol_allowed); > + mems_allowed = &nodemask; > + } else { > + mems_allowed = &cpuset_current_mems_allowed; > + } I believe you can simplify this and use a similar pattern as the page allocator. Something like for_each_node_mask(node, mpol_allowed) { if (node_isset(node, &cpuset_current_mems_allowed)) nr += array[node]; } There shouldn't be any need to allocate a potentially large nodemask on the stack.
On Fri, Jul 24, 2020 at 7:34 PM Michal Hocko <mhocko@kernel.org> wrote: > > On Fri 24-07-20 18:03:06, 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". > > > > Reported-by: Jianchao Guo <guojianchao@bytedance.com> > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > > --- > > > > changelog in v2: > > 1) Reuse policy_nodemask(). > > > > include/linux/mempolicy.h | 1 + > > mm/hugetlb.c | 19 ++++++++++++++++--- > > mm/mempolicy.c | 2 +- > > 3 files changed, 18 insertions(+), 4 deletions(-) > > > > 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..a753fe8591b4 100644 > > --- a/mm/hugetlb.c > > +++ b/mm/hugetlb.c > > @@ -3463,12 +3463,25 @@ 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, *mems_allowed, nodemask; > > + unsigned int *array = h->free_huge_pages_node; > > + gfp_t gfp_mask = htlb_alloc_mask(h); > > + > > + mpol_allowed = policy_nodemask(gfp_mask, mpol); > > + if (mpol_allowed) { > > + nodes_and(nodemask, cpuset_current_mems_allowed, > > + *mpol_allowed); > > + mems_allowed = &nodemask; > > + } else { > > + mems_allowed = &cpuset_current_mems_allowed; > > + } > > I believe you can simplify this and use a similar pattern as the page > allocator. Something like > > for_each_node_mask(node, mpol_allowed) { > if (node_isset(node, &cpuset_current_mems_allowed)) > nr += array[node]; > } > > There shouldn't be any need to allocate a potentially large nodemask on > the stack. An unsigned long can satisfy 64 nodes. So I think that nodemask is using little stack memory. Right? > -- > Michal Hocko > SUSE Labs
On Fri 24-07-20 21:56:29, Muchun Song wrote: > On Fri, Jul 24, 2020 at 7:34 PM Michal Hocko <mhocko@kernel.org> wrote: [...] > > I believe you can simplify this and use a similar pattern as the page > > allocator. Something like > > > > for_each_node_mask(node, mpol_allowed) { > > if (node_isset(node, &cpuset_current_mems_allowed)) > > nr += array[node]; > > } > > > > There shouldn't be any need to allocate a potentially large nodemask on > > the stack. > > An unsigned long can satisfy 64 nodes. So I think that nodemask is using > little stack memory. Right? CONFIG_NODES_SHIFT=10 is not something surprising. E.g. SLES is using that default for a long time because those kernels tend to run on very large machines.
On 7/24/20 3:03 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". > > Reported-by: Jianchao Guo <guojianchao@bytedance.com> > Signed-off-by: Muchun Song <songmuchun@bytedance.com> Michal has already provides some good suggestions. > @@ -3653,7 +3666,7 @@ static int hugetlb_acct_memory(struct hstate *h, long delta) > 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; > } There is a big comment before this code in hugetlb_acct_memory. The comment only talks about cpusets. We should probably update that to include mempolicy as well. It could be as simple as s/cpuset/cpuset or mempolicy/.
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..a753fe8591b4 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -3463,12 +3463,25 @@ 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, *mems_allowed, nodemask; + unsigned int *array = h->free_huge_pages_node; + gfp_t gfp_mask = htlb_alloc_mask(h); + + mpol_allowed = policy_nodemask(gfp_mask, mpol); + if (mpol_allowed) { + nodes_and(nodemask, cpuset_current_mems_allowed, + *mpol_allowed); + mems_allowed = &nodemask; + } else { + mems_allowed = &cpuset_current_mems_allowed; + } - for_each_node_mask(node, cpuset_current_mems_allowed) + for_each_node_mask(node, *mems_allowed) nr += array[node]; return nr; @@ -3653,7 +3666,7 @@ static int hugetlb_acct_memory(struct hstate *h, long delta) 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". Reported-by: Jianchao Guo <guojianchao@bytedance.com> Signed-off-by: Muchun Song <songmuchun@bytedance.com> --- changelog in v2: 1) Reuse policy_nodemask(). include/linux/mempolicy.h | 1 + mm/hugetlb.c | 19 ++++++++++++++++--- mm/mempolicy.c | 2 +- 3 files changed, 18 insertions(+), 4 deletions(-)