| Message ID | 1478141499-13825-3-git-send-email-shijie.huang@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Huang,
[auto build test ERROR on mmotm/master]
[also build test ERROR on v4.9-rc3 next-20161028]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Huang-Shijie/mm-hugetlb-rename-some-allocation-functions/20161103-105611
base: git://git.cmpxchg.org/linux-mmotm.git master
config: x86_64-randconfig-x011-201644 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All error/warnings (new ones prefixed by >>):
mm/hugetlb.c: In function '__alloc_huge_page':
>> mm/hugetlb.c:1623:10: error: implicit declaration of function 'alloc_gigantic_page' [-Werror=implicit-function-declaration]
page = alloc_gigantic_page(nid, huge_page_order(h));
^~~~~~~~~~~~~~~~~~~
>> mm/hugetlb.c:1623:8: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
page = alloc_gigantic_page(nid, huge_page_order(h));
^
cc1: some warnings being treated as errors
vim +/alloc_gigantic_page +1623 mm/hugetlb.c
1617 h->nr_huge_pages++;
1618 h->surplus_huge_pages++;
1619 }
1620 spin_unlock(&hugetlb_lock);
1621
1622 if (hstate_is_gigantic(h)) {
> 1623 page = alloc_gigantic_page(nid, huge_page_order(h));
1624 if (page)
1625 prep_compound_gigantic_page(page, huge_page_order(h));
1626 } else {
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Thu, 3 Nov 2016 10:51:38 +0800 Huang Shijie <shijie.huang@arm.com> wrote: > When testing the gigantic page whose order is too large for the buddy > allocator, the libhugetlbfs test case "counter.sh" will fail. > > The failure is caused by: > 1) kernel fails to allocate a gigantic page for the surplus case. > And the gather_surplus_pages() will return NULL in the end. > > 2) The condition checks for "over-commit" is wrong. > > This patch adds code to allocate the gigantic page in the > __alloc_huge_page(). After this patch, gather_surplus_pages() > can return a gigantic page for the surplus case. > > This patch also changes the condition checks for: > return_unused_surplus_pages() > nr_overcommit_hugepages_store() > > After this patch, the counter.sh can pass for the gigantic page. > > Acked-by: Steve Capper <steve.capper@arm.com> > Signed-off-by: Huang Shijie <shijie.huang@arm.com> > --- > mm/hugetlb.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > index 0bf4444..2b67aff 100644 > --- a/mm/hugetlb.c > +++ b/mm/hugetlb.c > @@ -1574,7 +1574,7 @@ static struct page *__alloc_huge_page(struct hstate *h, > struct page *page; > unsigned int r_nid; > > - if (hstate_is_gigantic(h)) > + if (hstate_is_gigantic(h) && !gigantic_page_supported()) > return NULL; Is it really possible to stumble over gigantic pages w/o having gigantic_page_supported()? Also, I've just tried this on s390 and counter.sh still fails after these patches, and it should fail on all archs as long as you use the gigantic hugepage size as default hugepage size. This is because you only changed nr_overcommit_hugepages_store(), which handles nr_overcommit_hugepages in sysfs, and missed hugetlb_overcommit_handler() which handles /proc/sys/vm/nr_overcommit_hugepages for the default sized hugepages. However, changing hugetlb_overcommit_handler() in a similar way produces a lockdep warning, see below, and counters.sh now results in FAIL mmap failed: Cannot allocate memory So I guess this needs more thinking (or just a proper annotation, as suggested, didn't really look into it): [ 129.595054] INFO: trying to register non-static key. [ 129.595060] the code is fine but needs lockdep annotation. [ 129.595062] turning off the locking correctness validator. [ 129.595066] CPU: 4 PID: 1108 Comm: counters Not tainted 4.9.0-rc3-00261-g577f12c-dirty #12 [ 129.595067] Hardware name: IBM 2964 N96 704 (LPAR) [ 129.595069] Stack: [ 129.595070] 00000003b4833688 00000003b4833718 0000000000000003 0000000000000000 [ 129.595075] 00000003b48337b8 00000003b4833730 00000003b4833730 0000000000000020 [ 129.595078] 0000000000000000 0000000000000020 000000000000000a 000000000000000a [ 129.595082] 000000000000000c 00000003b4833780 0000000000000000 00000003b4830000 [ 129.595086] 0000000000000000 0000000000112d90 00000003b4833718 00000003b4833770 [ 129.595089] Call Trace: [ 129.595095] ([<0000000000112c6a>] show_trace+0x8a/0xe0) [ 129.595098] [<0000000000112d40>] show_stack+0x80/0xd8 [ 129.595103] [<0000000000744eec>] dump_stack+0x9c/0xe0 [ 129.595106] [<00000000001b0760>] register_lock_class+0x1a8/0x530 [ 129.595109] [<00000000001b59fa>] __lock_acquire+0x10a/0x7f0 [ 129.595110] [<00000000001b69b8>] lock_acquire+0x2e0/0x330 [ 129.595115] [<0000000000a44920>] _raw_spin_lock_irqsave+0x70/0xb8 [ 129.595118] [<000000000031cdce>] alloc_gigantic_page+0x8e/0x2c8 [ 129.595120] [<000000000031e95a>] __alloc_huge_page+0xea/0x4d8 [ 129.595122] [<000000000031f4c6>] hugetlb_acct_memory+0xa6/0x418 [ 129.595125] [<0000000000323b32>] hugetlb_reserve_pages+0x132/0x240 [ 129.595152] [<000000000048be62>] hugetlbfs_file_mmap+0xd2/0x130 [ 129.595155] [<0000000000303918>] mmap_region+0x368/0x6e0 [ 129.595157] [<0000000000303fb8>] do_mmap+0x328/0x400 [ 129.595160] [<00000000002dc1aa>] vm_mmap_pgoff+0x9a/0xe8 [ 129.595162] [<00000000003016dc>] SyS_mmap_pgoff+0x23c/0x288 [ 129.595164] [<00000000003017b6>] SyS_old_mmap+0x8e/0xb0 [ 129.595166] [<0000000000a45b06>] system_call+0xd6/0x270 [ 129.595167] INFO: lockdep is turned off.
On Mon, Nov 07, 2016 at 04:25:04PM +0100, Gerald Schaefer wrote: > On Thu, 3 Nov 2016 10:51:38 +0800 > Huang Shijie <shijie.huang@arm.com> wrote: > > > When testing the gigantic page whose order is too large for the buddy > > allocator, the libhugetlbfs test case "counter.sh" will fail. > > > > The failure is caused by: > > 1) kernel fails to allocate a gigantic page for the surplus case. > > And the gather_surplus_pages() will return NULL in the end. > > > > 2) The condition checks for "over-commit" is wrong. > > > > This patch adds code to allocate the gigantic page in the > > __alloc_huge_page(). After this patch, gather_surplus_pages() > > can return a gigantic page for the surplus case. > > > > This patch also changes the condition checks for: > > return_unused_surplus_pages() > > nr_overcommit_hugepages_store() > > > > After this patch, the counter.sh can pass for the gigantic page. > > > > Acked-by: Steve Capper <steve.capper@arm.com> > > Signed-off-by: Huang Shijie <shijie.huang@arm.com> > > --- > > mm/hugetlb.c | 15 ++++++++++----- > > 1 file changed, 10 insertions(+), 5 deletions(-) > > > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > > index 0bf4444..2b67aff 100644 > > --- a/mm/hugetlb.c > > +++ b/mm/hugetlb.c > > @@ -1574,7 +1574,7 @@ static struct page *__alloc_huge_page(struct hstate *h, > > struct page *page; > > unsigned int r_nid; > > > > - if (hstate_is_gigantic(h)) > > + if (hstate_is_gigantic(h) && !gigantic_page_supported()) > > return NULL; > > Is it really possible to stumble over gigantic pages w/o having > gigantic_page_supported()? > > Also, I've just tried this on s390 and counter.sh still fails after these > patches, and it should fail on all archs as long as you use the gigantic I guess the failure you met is caused by the libhugetlbfs itself, there are several bugs in the libhugetlbfs. I have a patch set for the libhugetlbfs too. I will send it as soon as possible. > hugepage size as default hugepage size. This is because you only changed > nr_overcommit_hugepages_store(), which handles nr_overcommit_hugepages > in sysfs, and missed hugetlb_overcommit_handler() which handles > /proc/sys/vm/nr_overcommit_hugepages for the default sized hugepages. This is wrong. :) I did have an extra patch to fix the hugetlb_overcommit_handler(). but the counters.sh does not use the /proc/sys/vm/nr_overcommit_hugepages. Please grep it in the code. Thanks Huang Shijie
On Tue, Nov 08, 2016 at 10:19:30AM +0800, Huang Shijie wrote: > On Mon, Nov 07, 2016 at 04:25:04PM +0100, Gerald Schaefer wrote: > > On Thu, 3 Nov 2016 10:51:38 +0800 > > Huang Shijie <shijie.huang@arm.com> wrote: > > > > > When testing the gigantic page whose order is too large for the buddy > > > allocator, the libhugetlbfs test case "counter.sh" will fail. > > > > > > The failure is caused by: > > > 1) kernel fails to allocate a gigantic page for the surplus case. > > > And the gather_surplus_pages() will return NULL in the end. > > > > > > 2) The condition checks for "over-commit" is wrong. > > > > > > This patch adds code to allocate the gigantic page in the > > > __alloc_huge_page(). After this patch, gather_surplus_pages() > > > can return a gigantic page for the surplus case. > > > > > > This patch also changes the condition checks for: > > > return_unused_surplus_pages() > > > nr_overcommit_hugepages_store() > > > > > > After this patch, the counter.sh can pass for the gigantic page. > > > > > > Acked-by: Steve Capper <steve.capper@arm.com> > > > Signed-off-by: Huang Shijie <shijie.huang@arm.com> > > > --- > > > mm/hugetlb.c | 15 ++++++++++----- > > > 1 file changed, 10 insertions(+), 5 deletions(-) > > > > > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > > > index 0bf4444..2b67aff 100644 > > > --- a/mm/hugetlb.c > > > +++ b/mm/hugetlb.c > > > @@ -1574,7 +1574,7 @@ static struct page *__alloc_huge_page(struct hstate *h, > > > struct page *page; > > > unsigned int r_nid; > > > > > > - if (hstate_is_gigantic(h)) > > > + if (hstate_is_gigantic(h) && !gigantic_page_supported()) > > > return NULL; > > > > Is it really possible to stumble over gigantic pages w/o having > > gigantic_page_supported()? > > > > Also, I've just tried this on s390 and counter.sh still fails after these > > patches, and it should fail on all archs as long as you use the gigantic > I guess the failure you met is caused by the libhugetlbfs itself, there are > several bugs in the libhugetlbfs. I have a patch set for the > libhugetlbfs too. I will send it as soon as possible. > > > hugepage size as default hugepage size. This is because you only changed > > nr_overcommit_hugepages_store(), which handles nr_overcommit_hugepages > > in sysfs, and missed hugetlb_overcommit_handler() which handles > > /proc/sys/vm/nr_overcommit_hugepages for the default sized hugepages. > This is wrong. :) Sorry, I was wrong :). The counters test does call the /proc/sys/vm/nr_overcommit_hugepages. But in the arm64, it does not trigger a fail for the counters test. In an other word, I did not change the hugetlb_overcommit_handler(), the counters.sh also can pass in arm64. I will look at the lockdep issue. Thanks Huang Shijie
Hi Gerald, On Tue, Nov 08, 2016 at 03:08:52PM +0800, Huang Shijie wrote: > On Tue, Nov 08, 2016 at 10:19:30AM +0800, Huang Shijie wrote: > > On Mon, Nov 07, 2016 at 04:25:04PM +0100, Gerald Schaefer wrote: > > > On Thu, 3 Nov 2016 10:51:38 +0800 > > > Huang Shijie <shijie.huang@arm.com> wrote: > > > > > > > When testing the gigantic page whose order is too large for the buddy > > > > allocator, the libhugetlbfs test case "counter.sh" will fail. > > > > > > > > The failure is caused by: > > > > 1) kernel fails to allocate a gigantic page for the surplus case. > > > > And the gather_surplus_pages() will return NULL in the end. > > > > > > > > 2) The condition checks for "over-commit" is wrong. > > > > > > > > This patch adds code to allocate the gigantic page in the > > > > __alloc_huge_page(). After this patch, gather_surplus_pages() > > > > can return a gigantic page for the surplus case. > > > > > > > > This patch also changes the condition checks for: > > > > return_unused_surplus_pages() > > > > nr_overcommit_hugepages_store() > > > > > > > > After this patch, the counter.sh can pass for the gigantic page. > > > > > > > > Acked-by: Steve Capper <steve.capper@arm.com> > > > > Signed-off-by: Huang Shijie <shijie.huang@arm.com> > > > > --- > > > > mm/hugetlb.c | 15 ++++++++++----- > > > > 1 file changed, 10 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > > > > index 0bf4444..2b67aff 100644 > > > > --- a/mm/hugetlb.c > > > > +++ b/mm/hugetlb.c > > > > @@ -1574,7 +1574,7 @@ static struct page *__alloc_huge_page(struct hstate *h, > > > > struct page *page; > > > > unsigned int r_nid; > > > > > > > > - if (hstate_is_gigantic(h)) > > > > + if (hstate_is_gigantic(h) && !gigantic_page_supported()) > > > > return NULL; > > > > > > Is it really possible to stumble over gigantic pages w/o having > > > gigantic_page_supported()? > > > > > > Also, I've just tried this on s390 and counter.sh still fails after these > > > patches, and it should fail on all archs as long as you use the gigantic > > I guess the failure you met is caused by the libhugetlbfs itself, there are > > several bugs in the libhugetlbfs. I have a patch set for the > > libhugetlbfs too. I will send it as soon as possible. > > > > > hugepage size as default hugepage size. This is because you only changed > > > nr_overcommit_hugepages_store(), which handles nr_overcommit_hugepages > > > in sysfs, and missed hugetlb_overcommit_handler() which handles > > > /proc/sys/vm/nr_overcommit_hugepages for the default sized hugepages. > > This is wrong. :) > Sorry, I was wrong :). The counters test does call the /proc/sys/vm/nr_overcommit_hugepages. > But in the arm64, it does not trigger a fail for the counters test. > In an other word, I did not change the hugetlb_overcommit_handler(), > the counters.sh also can pass in arm64. After I add the "default_hugepagesz=32M" to the kernel cmdlin, I can reproduce this issue. Thanks for point this. > > I will look at the lockdep issue. I tested the new patch (will be sent out later) on the arm64 platform, and I did not meet the lockdep issue when I enabled the lockdep. The following is my config: CONFIG_LOCKD=y CONFIG_LOCKD_V4=y CONFIG_LOCKUP_DETECTOR=y # CONFIG_BOOTPARAM_SOFTLOCKUP_PANIC is not set CONFIG_BOOTPARAM_SOFTLOCKUP_PANIC_VALUE=0 CONFIG_DEBUG_SPINLOCK=y CONFIG_DEBUG_LOCK_ALLOC=y CONFIG_PROVE_LOCKING=y CONFIG_LOCKDEP=y CONFIG_LOCK_STAT=y CONFIG_DEBUG_LOCKDEP=y CONFIG_DEBUG_LOCKING_API_SELFTESTS=y So do I miss something? Thanks Huang Shijie
On Tue, 8 Nov 2016 17:17:28 +0800 Huang Shijie <shijie.huang@arm.com> wrote: > > I will look at the lockdep issue. > I tested the new patch (will be sent out later) on the arm64 platform, > and I did not meet the lockdep issue when I enabled the lockdep. > The following is my config: > > CONFIG_LOCKD=y > CONFIG_LOCKD_V4=y > CONFIG_LOCKUP_DETECTOR=y > # CONFIG_BOOTPARAM_SOFTLOCKUP_PANIC is not set > CONFIG_BOOTPARAM_SOFTLOCKUP_PANIC_VALUE=0 > CONFIG_DEBUG_SPINLOCK=y > CONFIG_DEBUG_LOCK_ALLOC=y > CONFIG_PROVE_LOCKING=y > CONFIG_LOCKDEP=y > CONFIG_LOCK_STAT=y > CONFIG_DEBUG_LOCKDEP=y > CONFIG_DEBUG_LOCKING_API_SELFTESTS=y > > So do I miss something? Those options should be OK. Meanwhile I looked into this a little more, and the problematic line/lock is spin_lock_irqsave(&z->lock, flags) at the top of alloc_gigantic_page(). From the lockdep trace we see that it is triggered by an mmap(), and then hugetlb_acct_memory() -> __alloc_huge_page() -> alloc_gigantic_page(). However, in between those functions (inside gather_surplus_pages()) a NUMA_NO_NODE node id comes into play. And this finally results in alloc_gigantic_page() being called with NUMA_NO_NODE as nid (which is -1), and NODE_DATA(nid)->node_zones will then reach into Nirvana. So, I guess the problem is a missing NUMA_NO_NODE check in alloc_gigantic_page(), similar to the one in __hugetlb_alloc_buddy_huge_page(). And somehow this was not a problem before the gigantic surplus change.
On Tue, Nov 08, 2016 at 08:27:42PM +0100, Gerald Schaefer wrote: > On Tue, 8 Nov 2016 17:17:28 +0800 > Huang Shijie <shijie.huang@arm.com> wrote: > > > > I will look at the lockdep issue. > > I tested the new patch (will be sent out later) on the arm64 platform, > > and I did not meet the lockdep issue when I enabled the lockdep. > > The following is my config: > > > > CONFIG_LOCKD=y > > CONFIG_LOCKD_V4=y > > CONFIG_LOCKUP_DETECTOR=y > > # CONFIG_BOOTPARAM_SOFTLOCKUP_PANIC is not set > > CONFIG_BOOTPARAM_SOFTLOCKUP_PANIC_VALUE=0 > > CONFIG_DEBUG_SPINLOCK=y > > CONFIG_DEBUG_LOCK_ALLOC=y > > CONFIG_PROVE_LOCKING=y > > CONFIG_LOCKDEP=y > > CONFIG_LOCK_STAT=y > > CONFIG_DEBUG_LOCKDEP=y > > CONFIG_DEBUG_LOCKING_API_SELFTESTS=y > > > > So do I miss something? > > Those options should be OK. Meanwhile I looked into this a little more, > and the problematic line/lock is spin_lock_irqsave(&z->lock, flags) at > the top of alloc_gigantic_page(). From the lockdep trace we see that > it is triggered by an mmap(), and then hugetlb_acct_memory() -> > __alloc_huge_page() -> alloc_gigantic_page(). > > However, in between those functions (inside gather_surplus_pages()) > a NUMA_NO_NODE node id comes into play. And this finally results in > alloc_gigantic_page() being called with NUMA_NO_NODE as nid (which is > -1), and NODE_DATA(nid)->node_zones will then reach into Nirvana. Thanks for pointing this. I sent out the new patch just now. Could you please try it again? I added a NUMA_NO_NODE check in the alloc_gigantic_page(); thanks Huang Shijie
diff --git a/mm/hugetlb.c b/mm/hugetlb.c index 0bf4444..2b67aff 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -1574,7 +1574,7 @@ static struct page *__alloc_huge_page(struct hstate *h, struct page *page; unsigned int r_nid; - if (hstate_is_gigantic(h)) + if (hstate_is_gigantic(h) && !gigantic_page_supported()) return NULL; /* @@ -1619,7 +1619,13 @@ static struct page *__alloc_huge_page(struct hstate *h, } spin_unlock(&hugetlb_lock); - page = __hugetlb_alloc_buddy_huge_page(h, vma, addr, nid); + if (hstate_is_gigantic(h)) { + page = alloc_gigantic_page(nid, huge_page_order(h)); + if (page) + prep_compound_gigantic_page(page, huge_page_order(h)); + } else { + page = __hugetlb_alloc_buddy_huge_page(h, vma, addr, nid); + } spin_lock(&hugetlb_lock); if (page) { @@ -1786,8 +1792,7 @@ static void return_unused_surplus_pages(struct hstate *h, /* Uncommit the reservation */ h->resv_huge_pages -= unused_resv_pages; - /* Cannot return gigantic pages currently */ - if (hstate_is_gigantic(h)) + if (hstate_is_gigantic(h) && !gigantic_page_supported()) return; nr_pages = min(unused_resv_pages, h->surplus_huge_pages); @@ -2439,7 +2444,7 @@ static ssize_t nr_overcommit_hugepages_store(struct kobject *kobj, unsigned long input; struct hstate *h = kobj_to_hstate(kobj, NULL); - if (hstate_is_gigantic(h)) + if (hstate_is_gigantic(h) && !gigantic_page_supported()) return -EINVAL; err = kstrtoul(buf, 10, &input);
When testing the gigantic page whose order is too large for the buddy allocator, the libhugetlbfs test case "counter.sh" will fail. The failure is caused by: 1) kernel fails to allocate a gigantic page for the surplus case. And the gather_surplus_pages() will return NULL in the end. 2) The condition checks for "over-commit" is wrong. This patch adds code to allocate the gigantic page in the __alloc_huge_page(). After this patch, gather_surplus_pages() can return a gigantic page for the surplus case. This patch also changes the condition checks for: return_unused_surplus_pages() nr_overcommit_hugepages_store() After this patch, the counter.sh can pass for the gigantic page. Acked-by: Steve Capper <steve.capper@arm.com> Signed-off-by: Huang Shijie <shijie.huang@arm.com> --- mm/hugetlb.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)