| Message ID | 20210219123909.13130-1-chenwandun@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/hugetlb: suppress wrong warning info when alloc gigantic page | expand |
On 2/19/21 4:39 AM, Chen Wandun wrote: > If hugetlb_cma is enabled, it will skip boot time allocation > when allocating gigantic page, that doesn't means allocation > failure, so suppress this warning info. > Normally the addition of warning messages is discouraged. However, in this case the additional message provides value. Why? Prior to the commit cf11e85fc08c, one could have a kernel command line that contains: hugepagesz=1G hugepages=16 This would allocate 16 1G pages at boot time. After the commit, someone could specify a command line containing: hugepagesz=1G hugepages=16 hugetlb_cma=16G In this case, 16G of CMA will be reserved for 1G huge page allocations after boot time. The parameter 'hugepages=16' is ignored, and the warning message is logged. The warning message should only be logged when the kernel parameter 'hugepages=' is ignored. IMO, it make sense to log a warning if ignoring a user specified parameter. The user should not be attempting boot time allocation and CMA reservation for 1G pages. I do not think we should drop the warning as the it tells the user thay have specified two incompatible allocation options.
On 19.02.21 20:14, Mike Kravetz wrote: > On 2/19/21 4:39 AM, Chen Wandun wrote: >> If hugetlb_cma is enabled, it will skip boot time allocation >> when allocating gigantic page, that doesn't means allocation >> failure, so suppress this warning info. >> > > Normally the addition of warning messages is discouraged. However, in > this case the additional message provides value. Why? > > Prior to the commit cf11e85fc08c, one could have a kernel command line > that contains: > > hugepagesz=1G hugepages=16 > > This would allocate 16 1G pages at boot time. > > After the commit, someone could specify a command line containing: > > hugepagesz=1G hugepages=16 hugetlb_cma=16G > > In this case, 16G of CMA will be reserved for 1G huge page allocations > after boot time. The parameter 'hugepages=16' is ignored, and the warning > message is logged. The warning message should only be logged when the > kernel parameter 'hugepages=' is ignored. > > IMO, it make sense to log a warning if ignoring a user specified parameter. > The user should not be attempting boot time allocation and CMA reservation > for 1G pages. > > I do not think we should drop the warning as the it tells the user thay > have specified two incompatible allocation options. > I agree. It has value.
On 3/4/21 1:35 AM, David Hildenbrand wrote: > On 19.02.21 20:14, Mike Kravetz wrote: >> On 2/19/21 4:39 AM, Chen Wandun wrote: >>> If hugetlb_cma is enabled, it will skip boot time allocation >>> when allocating gigantic page, that doesn't means allocation >>> failure, so suppress this warning info. >>> >> >> Normally the addition of warning messages is discouraged. However, in >> this case the additional message provides value. Why? >> >> Prior to the commit cf11e85fc08c, one could have a kernel command line >> that contains: >> >> hugepagesz=1G hugepages=16 >> >> This would allocate 16 1G pages at boot time. >> >> After the commit, someone could specify a command line containing: >> >> hugepagesz=1G hugepages=16 hugetlb_cma=16G >> >> In this case, 16G of CMA will be reserved for 1G huge page allocations >> after boot time. The parameter 'hugepages=16' is ignored, and the warning >> message is logged. The warning message should only be logged when the >> kernel parameter 'hugepages=' is ignored. >> >> IMO, it make sense to log a warning if ignoring a user specified parameter. >> The user should not be attempting boot time allocation and CMA reservation >> for 1G pages. >> >> I do not think we should drop the warning as the it tells the user thay >> have specified two incompatible allocation options. >> > > I agree. It has value. > Hi David, Sorry my above reply was too quick as I did not take a close look at the code/patch. See, https://lore.kernel.org/mm-commits/YDAbeDsG7GhV6s6B@carbon.dhcp.thefacebook.com/ This patch is actually in Andrew's tree.
On 04.03.21 18:20, Mike Kravetz wrote: > On 3/4/21 1:35 AM, David Hildenbrand wrote: >> On 19.02.21 20:14, Mike Kravetz wrote: >>> On 2/19/21 4:39 AM, Chen Wandun wrote: >>>> If hugetlb_cma is enabled, it will skip boot time allocation >>>> when allocating gigantic page, that doesn't means allocation >>>> failure, so suppress this warning info. >>>> >>> >>> Normally the addition of warning messages is discouraged. However, in >>> this case the additional message provides value. Why? >>> >>> Prior to the commit cf11e85fc08c, one could have a kernel command line >>> that contains: >>> >>> hugepagesz=1G hugepages=16 >>> >>> This would allocate 16 1G pages at boot time. >>> >>> After the commit, someone could specify a command line containing: >>> >>> hugepagesz=1G hugepages=16 hugetlb_cma=16G >>> >>> In this case, 16G of CMA will be reserved for 1G huge page allocations >>> after boot time. The parameter 'hugepages=16' is ignored, and the warning >>> message is logged. The warning message should only be logged when the >>> kernel parameter 'hugepages=' is ignored. >>> >>> IMO, it make sense to log a warning if ignoring a user specified parameter. >>> The user should not be attempting boot time allocation and CMA reservation >>> for 1G pages. >>> >>> I do not think we should drop the warning as the it tells the user thay >>> have specified two incompatible allocation options. >>> >> >> I agree. It has value. >> > > Hi David, > > Sorry my above reply was too quick as I did not take a close look at > the code/patch. See, > > https://lore.kernel.org/mm-commits/YDAbeDsG7GhV6s6B@carbon.dhcp.thefacebook.com/ > > This patch is actually in Andrew's tree. > Oh, I missed that discussion - thanks for the pointer!
diff --git a/mm/hugetlb.c b/mm/hugetlb.c index b6992297aa16..98a49cb9250c 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -2465,7 +2465,7 @@ static void __init hugetlb_hstate_alloc_pages(struct hstate *h) if (hstate_is_gigantic(h)) { if (hugetlb_cma_size) { pr_warn_once("HugeTLB: hugetlb_cma is enabled, skip boot time allocation\n"); - break; + goto free; } if (!alloc_bootmem_huge_page(h)) break; @@ -2483,7 +2483,7 @@ static void __init hugetlb_hstate_alloc_pages(struct hstate *h) h->max_huge_pages, buf, i); h->max_huge_pages = i; } - +free: kfree(node_alloc_noretry); }
If hugetlb_cma is enabled, it will skip boot time allocation when allocating gigantic page, that doesn't means allocation failure, so suppress this warning info. Fixes: cf11e85fc08c ("mm: hugetlb: optionally allocate gigantic hugepages using cma") Signed-off-by: Chen Wandun <chenwandun@huawei.com> --- mm/hugetlb.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)