From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752732AbdHNWK0 (ORCPT ); Mon, 14 Aug 2017 18:10:26 -0400 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:47245 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752533AbdHNWKY (ORCPT ); Mon, 14 Aug 2017 18:10:24 -0400 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Date: Mon, 14 Aug 2017 19:12:28 -0300 From: Victor Aoqui To: Mike Kravetz Cc: linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, aneesh.kumar@linux.vnet.ibm.com, mpe@ellerman.id.au, khandual@linux.vnet.ibm.com, victora@br.ibm.com, mauricfo@linux.vnet.ibm.com Subject: Re: [PATCH v3] powerpc/mm: Implemented default_hugepagesz verification for powerpc In-Reply-To: <797c6885-06ec-4c5a-ecbe-82f259ab0343@oracle.com> References: <20170724235202.5675-1-victora@linux.vnet.ibm.com> <797c6885-06ec-4c5a-ecbe-82f259ab0343@oracle.com> User-Agent: Roundcube Webmail/1.0.1 X-TM-AS-GCONF: 00 x-cbid: 17081422-0028-0000-0000-0000082FA33B X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00007545; HX=3.00000241; KW=3.00000007; PH=3.00000004; SC=3.00000220; SDB=6.00902368; UDB=6.00451939; IPR=6.00682583; BA=6.00005531; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00016695; XFM=3.00000015; UTC=2017-08-14 22:10:22 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17081422-0029-0000-0000-0000371FC57C Message-Id: X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-08-14_19:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1706020000 definitions=main-1708140364 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Em 2017-08-04 15:17, Mike Kravetz escreveu: > On 07/24/2017 04:52 PM, Victor Aoqui wrote: >> Implemented default hugepage size verification (default_hugepagesz=) >> in order to allow allocation of defined number of pages (hugepages=) >> only for supported hugepage sizes. >> >> Signed-off-by: Victor Aoqui >> --- >> v2: >> >> - Renamed default_hugepage_setup_sz function to >> hugetlb_default_size_setup; >> - Added powerpc string to error message. >> >> v3: >> >> - Renamed hugetlb_default_size_setup() to hugepage_default_setup_sz(); >> - Implemented hugetlb_bad_default_size(); >> - Reimplemented hugepage_setup_sz() to just parse default_hugepagesz= >> and >> check if it's a supported size; >> - Added verification of default_hugepagesz= value on >> hugetlb_nrpages_setup() >> before allocating hugepages. >> >> arch/powerpc/mm/hugetlbpage.c | 15 +++++++++++++++ >> include/linux/hugetlb.h | 1 + >> mm/hugetlb.c | 17 +++++++++++++++-- >> 3 files changed, 31 insertions(+), 2 deletions(-) >> >> diff --git a/arch/powerpc/mm/hugetlbpage.c >> b/arch/powerpc/mm/hugetlbpage.c >> index e1bf5ca..5990381 100644 >> --- a/arch/powerpc/mm/hugetlbpage.c >> +++ b/arch/powerpc/mm/hugetlbpage.c >> @@ -780,6 +780,21 @@ static int __init hugepage_setup_sz(char *str) >> } >> __setup("hugepagesz=", hugepage_setup_sz); >> >> +static int __init hugepage_default_setup_sz(char *str) >> +{ >> + unsigned long long size; >> + >> + size = memparse(str, &str); >> + >> + if (add_huge_page_size(size) != 0) { >> + hugetlb_bad_default_size(); >> + pr_err("Invalid ppc default huge page size specified(%llu)\n", >> size); >> + } >> + >> + return 1; >> +} >> +__setup("default_hugepagesz=", hugepage_default_setup_sz); >> + >> struct kmem_cache *hugepte_cache; >> static int __init hugetlbpage_init(void) >> { >> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h >> index 0ed8e41..2927200 100644 >> --- a/include/linux/hugetlb.h >> +++ b/include/linux/hugetlb.h >> @@ -361,6 +361,7 @@ int huge_add_to_page_cache(struct page *page, >> struct address_space *mapping, >> int __init alloc_bootmem_huge_page(struct hstate *h); >> >> void __init hugetlb_bad_size(void); >> +void __init hugetlb_bad_default_size(void); >> void __init hugetlb_add_hstate(unsigned order); >> struct hstate *size_to_hstate(unsigned long size); >> >> diff --git a/mm/hugetlb.c b/mm/hugetlb.c >> index bc48ee7..3c24266 100644 >> --- a/mm/hugetlb.c >> +++ b/mm/hugetlb.c >> @@ -54,6 +54,7 @@ >> static unsigned long __initdata default_hstate_max_huge_pages; >> static unsigned long __initdata default_hstate_size; >> static bool __initdata parsed_valid_hugepagesz = true; >> +static bool __initdata parsed_valid_default_hugepagesz = true; >> >> /* >> * Protects updates to hugepage_freelists, hugepage_activelist, >> nr_huge_pages, >> @@ -2804,6 +2805,12 @@ void __init hugetlb_bad_size(void) >> parsed_valid_hugepagesz = false; >> } >> >> +/* Should be called on processing a default_hugepagesz=... option */ >> +void __init hugetlb_bad_default_size(void) >> +{ >> + parsed_valid_default_hugepagesz = false; >> +} >> + >> void __init hugetlb_add_hstate(unsigned int order) >> { >> struct hstate *h; >> @@ -2846,8 +2853,14 @@ static int __init hugetlb_nrpages_setup(char >> *s) >> * !hugetlb_max_hstate means we haven't parsed a hugepagesz= >> parameter yet, >> * so this hugepages= parameter goes to the "default hstate". >> */ >> - else if (!hugetlb_max_hstate) >> - mhp = &default_hstate_max_huge_pages; >> + else if (!hugetlb_max_hstate) { >> + if (!parsed_valid_default_hugepagesz) { >> + pr_warn("hugepages = %s cannot be allocated for " >> + "unsupported default_hugepagesz, ignoring\n", s); >> + parsed_valid_default_hugepagesz = true; >> + } else >> + mhp = &default_hstate_max_huge_pages; >> + } >> else >> mhp = &parsed_hstate->max_huge_pages; >> >> > > My compiler tells me, > > mm/hugetlb.c: In function ‘hugetlb_nrpages_setup’: > mm/hugetlb.c:2873:8: warning: ‘mhp’ may be used uninitialized in > this function [-Wmaybe-uninitialized] > > You have added a way of getting out of that big if/else if statement > without > setting mhp. mhp will be examined later in the code, so this is indeed > a bug. > > Like Aneesh, I am not sure if there is great benefit in this patch. > > You added this change in functionality only for powerpc. IMO, it would > be > best if behavior was consistent in all architectures. So, if we change > it > for powerpc we may want to change everywhere. Hi Mike, Yes, the patch mentioned by Aneesh solves the issue. Thanks -- Victor Aoqui