| Message ID | 20200318220634.32100-5-mike.kravetz@oracle.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Clean up hugetlb boot command line processing | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (8a445cbcb9f5090cb07ec6cbb89a8a1fc99a0ff7) |
| snowpatch_ozlabs/build-ppc64le | success | Build succeeded |
| snowpatch_ozlabs/build-ppc64be | success | Build succeeded |
| snowpatch_ozlabs/build-ppc64e | success | Build succeeded |
| snowpatch_ozlabs/build-pmac32 | success | Build succeeded |
| snowpatch_ozlabs/checkpatch | warning | total: 0 errors, 0 warnings, 3 checks, 183 lines checked |
| snowpatch_ozlabs/needsstable | success | Patch has no Fixes tags |
Hi Mike, On 3/18/20 3:06 PM, Mike Kravetz wrote: > With all hugetlb page processing done in a single file clean up code. > - Make code match desired semantics > - Update documentation with semantics > - Make all warnings and errors messages start with 'HugeTLB:'. > - Consistently name command line parsing routines. > - Add comments to code > - Describe some of the subtle interactions > - Describe semantics of command line arguments > > Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com> > --- > Documentation/admin-guide/mm/hugetlbpage.rst | 26 +++++++ > mm/hugetlb.c | 78 +++++++++++++++----- > 2 files changed, 87 insertions(+), 17 deletions(-) > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > index cc85b4f156ca..2b9bf01db2b6 100644 > --- a/mm/hugetlb.c > +++ b/mm/hugetlb.c > @@ -3214,8 +3238,15 @@ static int __init hugetlb_nrpages_setup(char *s) > > return 1; > } > -__setup("hugepages=", hugetlb_nrpages_setup); > +__setup("hugepages=", hugepages_setup); > > +/* > + * hugepagesz command line processing > + * A specific huge page size can only be specified once with hugepagesz. > + * hugepagesz is followed by hugepages on the commnad line. The global typo: command > + * variable 'parsed_valid_hugepagesz' is used to determine if prior > + * hugepagesz argument was valid. > + */ > static int __init hugepagesz_setup(char *s) > { > unsigned long long size; Does any of this need to be updated? (from Documentation/admin-guide/kernel-parameters.txt) hugepagesz= [HW,IA-64,PPC,X86-64] The size of the HugeTLB pages. On x86-64 and powerpc, this option can be specified multiple times interleaved with hugepages= to reserve huge pages of different sizes. Valid pages sizes on x86-64 are 2M (when the CPU supports "pse") and 1G (when the CPU supports the "pdpe1gb" cpuinfo flag).
On 3/18/20 5:20 PM, Randy Dunlap wrote: > Hi Mike, > > On 3/18/20 3:06 PM, Mike Kravetz wrote: >> With all hugetlb page processing done in a single file clean up code. >> - Make code match desired semantics >> - Update documentation with semantics >> - Make all warnings and errors messages start with 'HugeTLB:'. >> - Consistently name command line parsing routines. >> - Add comments to code >> - Describe some of the subtle interactions >> - Describe semantics of command line arguments >> >> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com> >> --- >> Documentation/admin-guide/mm/hugetlbpage.rst | 26 +++++++ >> mm/hugetlb.c | 78 +++++++++++++++----- >> 2 files changed, 87 insertions(+), 17 deletions(-) > > >> diff --git a/mm/hugetlb.c b/mm/hugetlb.c >> index cc85b4f156ca..2b9bf01db2b6 100644 >> --- a/mm/hugetlb.c >> +++ b/mm/hugetlb.c > >> @@ -3214,8 +3238,15 @@ static int __init hugetlb_nrpages_setup(char *s) >> >> return 1; >> } >> -__setup("hugepages=", hugetlb_nrpages_setup); >> +__setup("hugepages=", hugepages_setup); >> >> +/* >> + * hugepagesz command line processing >> + * A specific huge page size can only be specified once with hugepagesz. >> + * hugepagesz is followed by hugepages on the commnad line. The global > > typo: command Thanks > >> + * variable 'parsed_valid_hugepagesz' is used to determine if prior >> + * hugepagesz argument was valid. >> + */ >> static int __init hugepagesz_setup(char *s) >> { >> unsigned long long size; > > > Does any of this need to be updated? (from Documentation/admin-guide/kernel-parameters.txt) > > hugepagesz= [HW,IA-64,PPC,X86-64] The size of the HugeTLB pages. > On x86-64 and powerpc, this option can be specified > multiple times interleaved with hugepages= to reserve > huge pages of different sizes. Valid pages sizes on > x86-64 are 2M (when the CPU supports "pse") and 1G > (when the CPU supports the "pdpe1gb" cpuinfo flag). > No functional changes should be expected/seen as a result of these patches. So the documentation here is basically OK. However, it is out of date as more architectures are supported. In addition, the statement "this option can be specified multiple times interleaved with hugepages= to reserve huge pages of different sizes." may need a little clarification. As mentioned elsewhere, hugepagesz= can only be specified once per huge page size. I'll make some updates in v2.
On Wed, Mar 18, 2020 at 3:07 PM Mike Kravetz <mike.kravetz@oracle.com> wrote: > > With all hugetlb page processing done in a single file clean up code. Now that all hugepage page processing is done in a single file, clean up the code. > - Make code match desired semantics > - Update documentation with semantics > - Make all warnings and errors messages start with 'HugeTLB:'. > - Consistently name command line parsing routines. > - Add comments to code > - Describe some of the subtle interactions > - Describe semantics of command line arguments > > Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com> > --- > Documentation/admin-guide/mm/hugetlbpage.rst | 26 +++++++ > mm/hugetlb.c | 78 +++++++++++++++----- > 2 files changed, 87 insertions(+), 17 deletions(-) > > diff --git a/Documentation/admin-guide/mm/hugetlbpage.rst b/Documentation/admin-guide/mm/hugetlbpage.rst > index 1cc0bc78d10e..afc8888f33c3 100644 > --- a/Documentation/admin-guide/mm/hugetlbpage.rst > +++ b/Documentation/admin-guide/mm/hugetlbpage.rst > @@ -100,6 +100,32 @@ with a huge page size selection parameter "hugepagesz=<size>". <size> must > be specified in bytes with optional scale suffix [kKmMgG]. The default huge > page size may be selected with the "default_hugepagesz=<size>" boot parameter. > > +Hugetlb boot command line parameter semantics > +hugepagesz - Specify a huge page size. Used in conjunction with hugepages > + parameter to preallocate a number of huge pages of the specified > + size. Hence, hugepagesz and hugepages are typically specified in > + pairs such as: > + hugepagesz=2M hugepages=512 > + hugepagesz can only be specified once on the command line for a > + specific huge page size. Valid huge page sizes are architecture > + dependent. > +hugepages - Specify the number of huge pages to preallocate. This typically > + follows a valid hugepagesz parameter. However, if hugepages is the > + first or only hugetlb command line parameter it specifies the number > + of huge pages of default size to allocate. The number of huge pages > + of default size specified in this manner can be overwritten by a > + hugepagesz,hugepages parameter pair for the default size. > + For example, on an architecture with 2M default huge page size: > + hugepages=256 hugepagesz=2M hugepages=512 > + will result in 512 2M huge pages being allocated. If a hugepages > + parameter is preceded by an invalid hugepagesz parameter, it will > + be ignored. > +default_hugepagesz - Specify the default huge page size. This parameter can > + only be specified on the command line. No other hugetlb command line > + parameter is associated with default_hugepagesz. Therefore, it can > + appear anywhere on the command line. Valid default huge page size is > + architecture dependent. Maybe specify what happens/should happen in a case like: hugepages=100 default_hugepagesz=1G Does that allocate 100 2MB pages or 100 1G pages? Assuming the default size is 2MB. Also, regarding Randy's comment. It may be nice to keep these docs in one place only, so we don't have to maintain 2 docs in sync. > + > When multiple huge page sizes are supported, ``/proc/sys/vm/nr_hugepages`` > indicates the current number of pre-allocated huge pages of the default size. > Thus, one can use the following command to dynamically allocate/deallocate > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > index cc85b4f156ca..2b9bf01db2b6 100644 > --- a/mm/hugetlb.c > +++ b/mm/hugetlb.c > @@ -2954,7 +2954,7 @@ static void __init hugetlb_sysfs_init(void) > err = hugetlb_sysfs_add_hstate(h, hugepages_kobj, > hstate_kobjs, &hstate_attr_group); > if (err) > - pr_err("Hugetlb: Unable to add hstate %s", h->name); > + pr_err("HugeTLB: Unable to add hstate %s", h->name); > } > } > > @@ -3058,7 +3058,7 @@ static void hugetlb_register_node(struct node *node) > nhs->hstate_kobjs, > &per_node_hstate_attr_group); > if (err) { > - pr_err("Hugetlb: Unable to add hstate %s for node %d\n", > + pr_err("HugeTLB: Unable to add hstate %s for node %d\n", > h->name, node->dev.id); > hugetlb_unregister_node(node); > break; > @@ -3109,19 +3109,35 @@ static int __init hugetlb_init(void) > if (!hugepages_supported()) > return 0; > > - if (!size_to_hstate(default_hstate_size)) { > - if (default_hstate_size != 0) { > - pr_err("HugeTLB: unsupported default_hugepagesz %lu. Reverting to %lu\n", > - default_hstate_size, HPAGE_SIZE); > - } > - > + /* > + * Make sure HPAGE_SIZE (HUGETLB_PAGE_ORDER) hstate exists. Some > + * architectures depend on setup being done here. > + * > + * If a valid default huge page size was specified on the command line, > + * add associated hstate if necessary. If not, set default_hstate_size > + * to default size. default_hstate_idx is used at runtime to identify > + * the default huge page size/hstate. > + */ > + hugetlb_add_hstate(HUGETLB_PAGE_ORDER); > + if (default_hstate_size) > + hugetlb_add_hstate(ilog2(default_hstate_size) - PAGE_SHIFT); > + else > default_hstate_size = HPAGE_SIZE; > - hugetlb_add_hstate(HUGETLB_PAGE_ORDER); > - } > default_hstate_idx = hstate_index(size_to_hstate(default_hstate_size)); > + > + /* > + * default_hstate_max_huge_pages != 0 indicates a count (hugepages=) > + * specified before a size (hugepagesz=). Use this count for the > + * default huge page size, unless a specific value was specified for > + * this size in a hugepagesz/hugepages pair. > + */ > if (default_hstate_max_huge_pages) { > if (!default_hstate.max_huge_pages) > - default_hstate.max_huge_pages = default_hstate_max_huge_pages; > + default_hstate.max_huge_pages = > + default_hstate_max_huge_pages; > + else > + pr_warn("HugeTLB: First hugepages=%lu kB ignored\n", > + default_hstate_max_huge_pages); > } > > hugetlb_init_hstates(); > @@ -3174,20 +3190,27 @@ void __init hugetlb_add_hstate(unsigned int order) > parsed_hstate = h; > } > > -static int __init hugetlb_nrpages_setup(char *s) > +/* > + * hugepages command line processing > + * hugepages must normally follows a valid hugepagsz specification. If not, 'hugepages must' or 'hugepages normally follows' > + * ignore the hugepages value. hugepages can also be the first huge page > + * command line option in which case it specifies the number of huge pages > + * for the default size. > + */ > +static int __init hugepages_setup(char *s) > { > unsigned long *mhp; > static unsigned long *last_mhp; > > if (!parsed_valid_hugepagesz) { > - pr_warn("hugepages = %s preceded by " > + pr_warn("HugeTLB: hugepages = %s preceded by " > "an unsupported hugepagesz, ignoring\n", s); > parsed_valid_hugepagesz = true; > return 1; > } > /* > - * !hugetlb_max_hstate means we haven't parsed a hugepagesz= parameter yet, > - * so this hugepages= parameter goes to the "default hstate". > + * !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; We don't set parsed_valid_hugepagesz to false at the end of this function, shouldn't we? Parsing a hugepages= value should 'consume' a previously defined hugepagesz= value, so that this is invalid IIUC: hugepagesz=x hugepages=z hugepages=y > @@ -3195,7 +3218,8 @@ static int __init hugetlb_nrpages_setup(char *s) > mhp = &parsed_hstate->max_huge_pages; > > if (mhp == last_mhp) { > - pr_warn("hugepages= specified twice without interleaving hugepagesz=, ignoring\n"); > + pr_warn("HugeTLB: hugepages= specified twice without interleaving hugepagesz=, ignoring hugepages=%s\n", > + s); > return 1; > } > > @@ -3214,8 +3238,15 @@ static int __init hugetlb_nrpages_setup(char *s) > > return 1; > } > -__setup("hugepages=", hugetlb_nrpages_setup); > +__setup("hugepages=", hugepages_setup); > > +/* > + * hugepagesz command line processing > + * A specific huge page size can only be specified once with hugepagesz. > + * hugepagesz is followed by hugepages on the commnad line. The global > + * variable 'parsed_valid_hugepagesz' is used to determine if prior > + * hugepagesz argument was valid. > + */ > static int __init hugepagesz_setup(char *s) > { > unsigned long long size; > @@ -3230,16 +3261,23 @@ static int __init hugepagesz_setup(char *s) > } > > if (size_to_hstate(size)) { > + parsed_valid_hugepagesz = false; > pr_warn("HugeTLB: hugepagesz %s specified twice, ignoring\n", > saved_s); > return 0; > } > > + parsed_valid_hugepagesz = true; > hugetlb_add_hstate(ilog2(size) - PAGE_SHIFT); > return 1; > } > __setup("hugepagesz=", hugepagesz_setup); > > +/* > + * default_hugepagesz command line input > + * Only one instance of default_hugepagesz allowed on command line. Do not > + * add hstate here as that will confuse hugepagesz/hugepages processing. > + */ > static int __init default_hugepagesz_setup(char *s) > { > unsigned long long size; > @@ -3252,6 +3290,12 @@ static int __init default_hugepagesz_setup(char *s) > return 0; > } > > + if (default_hstate_size) { > + pr_err("HugeTLB: default_hugepagesz previously specified, ignoring %s\n", > + saved_s); > + return 0; > + } > + > default_hstate_size = size; > return 1; > } > -- > 2.24.1 > >
On 2020/3/24 8:43, Mina Almasry wrote: > On Wed, Mar 18, 2020 at 3:07 PM Mike Kravetz <mike.kravetz@oracle.com> wrote: >> >> With all hugetlb page processing done in a single file clean up code. > > Now that all hugepage page processing is done in a single file, clean > up the code. > >> - Make code match desired semantics >> - Update documentation with semantics >> - Make all warnings and errors messages start with 'HugeTLB:'. >> - Consistently name command line parsing routines. >> - Add comments to code >> - Describe some of the subtle interactions >> - Describe semantics of command line arguments >> >> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com> >> --- >> Documentation/admin-guide/mm/hugetlbpage.rst | 26 +++++++ >> mm/hugetlb.c | 78 +++++++++++++++----- >> 2 files changed, 87 insertions(+), 17 deletions(-) >> >> diff --git a/Documentation/admin-guide/mm/hugetlbpage.rst b/Documentation/admin-guide/mm/hugetlbpage.rst >> index 1cc0bc78d10e..afc8888f33c3 100644 >> --- a/Documentation/admin-guide/mm/hugetlbpage.rst >> +++ b/Documentation/admin-guide/mm/hugetlbpage.rst >> @@ -100,6 +100,32 @@ with a huge page size selection parameter "hugepagesz=<size>". <size> must >> be specified in bytes with optional scale suffix [kKmMgG]. The default huge >> page size may be selected with the "default_hugepagesz=<size>" boot parameter. >> >> +Hugetlb boot command line parameter semantics >> +hugepagesz - Specify a huge page size. Used in conjunction with hugepages >> + parameter to preallocate a number of huge pages of the specified >> + size. Hence, hugepagesz and hugepages are typically specified in >> + pairs such as: >> + hugepagesz=2M hugepages=512 >> + hugepagesz can only be specified once on the command line for a >> + specific huge page size. Valid huge page sizes are architecture >> + dependent. >> +hugepages - Specify the number of huge pages to preallocate. This typically >> + follows a valid hugepagesz parameter. However, if hugepages is the >> + first or only hugetlb command line parameter it specifies the number >> + of huge pages of default size to allocate. The number of huge pages >> + of default size specified in this manner can be overwritten by a >> + hugepagesz,hugepages parameter pair for the default size. >> + For example, on an architecture with 2M default huge page size: >> + hugepages=256 hugepagesz=2M hugepages=512 >> + will result in 512 2M huge pages being allocated. If a hugepages >> + parameter is preceded by an invalid hugepagesz parameter, it will >> + be ignored. >> +default_hugepagesz - Specify the default huge page size. This parameter can >> + only be specified on the command line. No other hugetlb command line >> + parameter is associated with default_hugepagesz. Therefore, it can >> + appear anywhere on the command line. Valid default huge page size is >> + architecture dependent. > > Maybe specify what happens/should happen in a case like: > > hugepages=100 default_hugepagesz=1G > > Does that allocate 100 2MB pages or 100 1G pages? Assuming the default > size is 2MB. > > Also, regarding Randy's comment. It may be nice to keep these docs in > one place only, so we don't have to maintain 2 docs in sync. > > >> + >> When multiple huge page sizes are supported, ``/proc/sys/vm/nr_hugepages`` >> indicates the current number of pre-allocated huge pages of the default size. >> Thus, one can use the following command to dynamically allocate/deallocate >> diff --git a/mm/hugetlb.c b/mm/hugetlb.c >> index cc85b4f156ca..2b9bf01db2b6 100644 >> --- a/mm/hugetlb.c >> +++ b/mm/hugetlb.c >> @@ -2954,7 +2954,7 @@ static void __init hugetlb_sysfs_init(void) >> err = hugetlb_sysfs_add_hstate(h, hugepages_kobj, >> hstate_kobjs, &hstate_attr_group); >> if (err) >> - pr_err("Hugetlb: Unable to add hstate %s", h->name); >> + pr_err("HugeTLB: Unable to add hstate %s", h->name); >> } >> } >> >> @@ -3058,7 +3058,7 @@ static void hugetlb_register_node(struct node *node) >> nhs->hstate_kobjs, >> &per_node_hstate_attr_group); >> if (err) { >> - pr_err("Hugetlb: Unable to add hstate %s for node %d\n", >> + pr_err("HugeTLB: Unable to add hstate %s for node %d\n", >> h->name, node->dev.id); >> hugetlb_unregister_node(node); >> break; >> @@ -3109,19 +3109,35 @@ static int __init hugetlb_init(void) >> if (!hugepages_supported()) >> return 0; >> >> - if (!size_to_hstate(default_hstate_size)) { >> - if (default_hstate_size != 0) { >> - pr_err("HugeTLB: unsupported default_hugepagesz %lu. Reverting to %lu\n", >> - default_hstate_size, HPAGE_SIZE); >> - } >> - >> + /* >> + * Make sure HPAGE_SIZE (HUGETLB_PAGE_ORDER) hstate exists. Some >> + * architectures depend on setup being done here. >> + * >> + * If a valid default huge page size was specified on the command line, >> + * add associated hstate if necessary. If not, set default_hstate_size >> + * to default size. default_hstate_idx is used at runtime to identify >> + * the default huge page size/hstate. >> + */ >> + hugetlb_add_hstate(HUGETLB_PAGE_ORDER); >> + if (default_hstate_size) >> + hugetlb_add_hstate(ilog2(default_hstate_size) - PAGE_SHIFT); >> + else >> default_hstate_size = HPAGE_SIZE; >> - hugetlb_add_hstate(HUGETLB_PAGE_ORDER); >> - } >> default_hstate_idx = hstate_index(size_to_hstate(default_hstate_size)); >> + >> + /* >> + * default_hstate_max_huge_pages != 0 indicates a count (hugepages=) >> + * specified before a size (hugepagesz=). Use this count for the >> + * default huge page size, unless a specific value was specified for >> + * this size in a hugepagesz/hugepages pair. >> + */ >> if (default_hstate_max_huge_pages) { >> if (!default_hstate.max_huge_pages) >> - default_hstate.max_huge_pages = default_hstate_max_huge_pages; >> + default_hstate.max_huge_pages = >> + default_hstate_max_huge_pages; >> + else >> + pr_warn("HugeTLB: First hugepages=%lu kB ignored\n", >> + default_hstate_max_huge_pages); >> } >> >> hugetlb_init_hstates(); >> @@ -3174,20 +3190,27 @@ void __init hugetlb_add_hstate(unsigned int order) >> parsed_hstate = h; >> } >> >> -static int __init hugetlb_nrpages_setup(char *s) >> +/* >> + * hugepages command line processing >> + * hugepages must normally follows a valid hugepagsz specification. If not, > > 'hugepages must' or 'hugepages normally follows' >> + * ignore the hugepages value. hugepages can also be the first huge page >> + * command line option in which case it specifies the number of huge pages >> + * for the default size. >> + */ >> +static int __init hugepages_setup(char *s) >> { >> unsigned long *mhp; >> static unsigned long *last_mhp; >> >> if (!parsed_valid_hugepagesz) { >> - pr_warn("hugepages = %s preceded by " >> + pr_warn("HugeTLB: hugepages = %s preceded by " >> "an unsupported hugepagesz, ignoring\n", s); >> parsed_valid_hugepagesz = true; >> return 1; >> } >> /* >> - * !hugetlb_max_hstate means we haven't parsed a hugepagesz= parameter yet, >> - * so this hugepages= parameter goes to the "default hstate". >> + * !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; > > We don't set parsed_valid_hugepagesz to false at the end of this > function, shouldn't we? Parsing a hugepages= value should 'consume' a > previously defined hugepagesz= value, so that this is invalid IIUC: > > hugepagesz=x hugepages=z hugepages=y > In this case, we'll get: "HugeTLB: hugepages= specified twice without interleaving hugepagesz=, ignoring hugepages=y" >> @@ -3195,7 +3218,8 @@ static int __init hugetlb_nrpages_setup(char *s) >> mhp = &parsed_hstate->max_huge_pages; >> >> if (mhp == last_mhp) { >> - pr_warn("hugepages= specified twice without interleaving hugepagesz=, ignoring\n"); >> + pr_warn("HugeTLB: hugepages= specified twice without interleaving hugepagesz=, ignoring hugepages=%s\n", >> + s); >> return 1; >> } >> >> @@ -3214,8 +3238,15 @@ static int __init hugetlb_nrpages_setup(char *s) >> >> return 1; >> } >> -__setup("hugepages=", hugetlb_nrpages_setup); >> +__setup("hugepages=", hugepages_setup); >> >> +/* >> + * hugepagesz command line processing >> + * A specific huge page size can only be specified once with hugepagesz. >> + * hugepagesz is followed by hugepages on the commnad line. The global >> + * variable 'parsed_valid_hugepagesz' is used to determine if prior >> + * hugepagesz argument was valid. >> + */ >> static int __init hugepagesz_setup(char *s) >> { >> unsigned long long size; >> @@ -3230,16 +3261,23 @@ static int __init hugepagesz_setup(char *s) >> } >> >> if (size_to_hstate(size)) { >> + parsed_valid_hugepagesz = false; >> pr_warn("HugeTLB: hugepagesz %s specified twice, ignoring\n", >> saved_s); >> return 0; >> } >> >> + parsed_valid_hugepagesz = true; >> hugetlb_add_hstate(ilog2(size) - PAGE_SHIFT); >> return 1; >> } >> __setup("hugepagesz=", hugepagesz_setup); >> >> +/* >> + * default_hugepagesz command line input >> + * Only one instance of default_hugepagesz allowed on command line. Do not >> + * add hstate here as that will confuse hugepagesz/hugepages processing. >> + */ >> static int __init default_hugepagesz_setup(char *s) >> { >> unsigned long long size; >> @@ -3252,6 +3290,12 @@ static int __init default_hugepagesz_setup(char *s) >> return 0; >> } >> >> + if (default_hstate_size) { >> + pr_err("HugeTLB: default_hugepagesz previously specified, ignoring %s\n", >> + saved_s); >> + return 0; >> + } >> + >> default_hstate_size = size; >> return 1; >> } >> -- >> 2.24.1 >> >> > . > --- Regards, Longpeng(Mike)
On 3/23/20 8:47 PM, Longpeng (Mike, Cloud Infrastructure Service Product Dept.) wrote: > > > On 2020/3/24 8:43, Mina Almasry wrote: >> On Wed, Mar 18, 2020 at 3:07 PM Mike Kravetz <mike.kravetz@oracle.com> wrote: >>> +default_hugepagesz - Specify the default huge page size. This parameter can >>> + only be specified on the command line. No other hugetlb command line >>> + parameter is associated with default_hugepagesz. Therefore, it can >>> + appear anywhere on the command line. Valid default huge page size is >>> + architecture dependent. >> >> Maybe specify what happens/should happen in a case like: >> >> hugepages=100 default_hugepagesz=1G >> >> Does that allocate 100 2MB pages or 100 1G pages? Assuming the default >> size is 2MB. That will allocate 100 1G pages as 1G is the default. However, if the command line reads: hugepages=100 default_hugepagesz=1G hugepages=200 You will get this warning, HugeTLB: First hugepages=104857600 kB ignored >> >> Also, regarding Randy's comment. It may be nice to keep these docs in >> one place only, so we don't have to maintain 2 docs in sync. Let me think about that a bit. We should probably expand the kernel-parameters doc. Or, we should at least make it more clear. This doc also talks about the command line parameters and in general goes into more detail. However, more people read kernel-parameters doc. >>> + >>> When multiple huge page sizes are supported, ``/proc/sys/vm/nr_hugepages`` >>> indicates the current number of pre-allocated huge pages of the default size. >>> Thus, one can use the following command to dynamically allocate/deallocate >>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c >>> index cc85b4f156ca..2b9bf01db2b6 100644 >>> --- a/mm/hugetlb.c >>> +++ b/mm/hugetlb.c <snip> >>> -static int __init hugetlb_nrpages_setup(char *s) >>> +/* >>> + * hugepages command line processing >>> + * hugepages must normally follows a valid hugepagsz specification. If not, >> >> 'hugepages must' or 'hugepages normally follows' >>> + * ignore the hugepages value. hugepages can also be the first huge page >>> + * command line option in which case it specifies the number of huge pages >>> + * for the default size. >>> + */ >>> +static int __init hugepages_setup(char *s) >>> { >>> unsigned long *mhp; >>> static unsigned long *last_mhp; >>> >>> if (!parsed_valid_hugepagesz) { >>> - pr_warn("hugepages = %s preceded by " >>> + pr_warn("HugeTLB: hugepages = %s preceded by " >>> "an unsupported hugepagesz, ignoring\n", s); >>> parsed_valid_hugepagesz = true; >>> return 1; >>> } >>> /* >>> - * !hugetlb_max_hstate means we haven't parsed a hugepagesz= parameter yet, >>> - * so this hugepages= parameter goes to the "default hstate". >>> + * !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; >> >> We don't set parsed_valid_hugepagesz to false at the end of this >> function, shouldn't we? Parsing a hugepages= value should 'consume' a >> previously defined hugepagesz= value, so that this is invalid IIUC: >> >> hugepagesz=x hugepages=z hugepages=y >> > In this case, we'll get: > "HugeTLB: hugepages= specified twice without interleaving hugepagesz=, ignoring > hugepages=y" > Thanks Longpeng (Mike), I believe that is the desired message in this situation. The code uses saved values of mhp (max hstate pointer) to catch this condition. Setting parsed_valid_hugepagesz to false would result in the message: HugeTLB: hugepages=y preceded by an unsupported hugepagesz, ignoring Thanks for all your comments I will incorporate in v2 and send later this week.
diff --git a/Documentation/admin-guide/mm/hugetlbpage.rst b/Documentation/admin-guide/mm/hugetlbpage.rst index 1cc0bc78d10e..afc8888f33c3 100644 --- a/Documentation/admin-guide/mm/hugetlbpage.rst +++ b/Documentation/admin-guide/mm/hugetlbpage.rst @@ -100,6 +100,32 @@ with a huge page size selection parameter "hugepagesz=<size>". <size> must be specified in bytes with optional scale suffix [kKmMgG]. The default huge page size may be selected with the "default_hugepagesz=<size>" boot parameter. +Hugetlb boot command line parameter semantics +hugepagesz - Specify a huge page size. Used in conjunction with hugepages + parameter to preallocate a number of huge pages of the specified + size. Hence, hugepagesz and hugepages are typically specified in + pairs such as: + hugepagesz=2M hugepages=512 + hugepagesz can only be specified once on the command line for a + specific huge page size. Valid huge page sizes are architecture + dependent. +hugepages - Specify the number of huge pages to preallocate. This typically + follows a valid hugepagesz parameter. However, if hugepages is the + first or only hugetlb command line parameter it specifies the number + of huge pages of default size to allocate. The number of huge pages + of default size specified in this manner can be overwritten by a + hugepagesz,hugepages parameter pair for the default size. + For example, on an architecture with 2M default huge page size: + hugepages=256 hugepagesz=2M hugepages=512 + will result in 512 2M huge pages being allocated. If a hugepages + parameter is preceded by an invalid hugepagesz parameter, it will + be ignored. +default_hugepagesz - Specify the default huge page size. This parameter can + only be specified on the command line. No other hugetlb command line + parameter is associated with default_hugepagesz. Therefore, it can + appear anywhere on the command line. Valid default huge page size is + architecture dependent. + When multiple huge page sizes are supported, ``/proc/sys/vm/nr_hugepages`` indicates the current number of pre-allocated huge pages of the default size. Thus, one can use the following command to dynamically allocate/deallocate diff --git a/mm/hugetlb.c b/mm/hugetlb.c index cc85b4f156ca..2b9bf01db2b6 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -2954,7 +2954,7 @@ static void __init hugetlb_sysfs_init(void) err = hugetlb_sysfs_add_hstate(h, hugepages_kobj, hstate_kobjs, &hstate_attr_group); if (err) - pr_err("Hugetlb: Unable to add hstate %s", h->name); + pr_err("HugeTLB: Unable to add hstate %s", h->name); } } @@ -3058,7 +3058,7 @@ static void hugetlb_register_node(struct node *node) nhs->hstate_kobjs, &per_node_hstate_attr_group); if (err) { - pr_err("Hugetlb: Unable to add hstate %s for node %d\n", + pr_err("HugeTLB: Unable to add hstate %s for node %d\n", h->name, node->dev.id); hugetlb_unregister_node(node); break; @@ -3109,19 +3109,35 @@ static int __init hugetlb_init(void) if (!hugepages_supported()) return 0; - if (!size_to_hstate(default_hstate_size)) { - if (default_hstate_size != 0) { - pr_err("HugeTLB: unsupported default_hugepagesz %lu. Reverting to %lu\n", - default_hstate_size, HPAGE_SIZE); - } - + /* + * Make sure HPAGE_SIZE (HUGETLB_PAGE_ORDER) hstate exists. Some + * architectures depend on setup being done here. + * + * If a valid default huge page size was specified on the command line, + * add associated hstate if necessary. If not, set default_hstate_size + * to default size. default_hstate_idx is used at runtime to identify + * the default huge page size/hstate. + */ + hugetlb_add_hstate(HUGETLB_PAGE_ORDER); + if (default_hstate_size) + hugetlb_add_hstate(ilog2(default_hstate_size) - PAGE_SHIFT); + else default_hstate_size = HPAGE_SIZE; - hugetlb_add_hstate(HUGETLB_PAGE_ORDER); - } default_hstate_idx = hstate_index(size_to_hstate(default_hstate_size)); + + /* + * default_hstate_max_huge_pages != 0 indicates a count (hugepages=) + * specified before a size (hugepagesz=). Use this count for the + * default huge page size, unless a specific value was specified for + * this size in a hugepagesz/hugepages pair. + */ if (default_hstate_max_huge_pages) { if (!default_hstate.max_huge_pages) - default_hstate.max_huge_pages = default_hstate_max_huge_pages; + default_hstate.max_huge_pages = + default_hstate_max_huge_pages; + else + pr_warn("HugeTLB: First hugepages=%lu kB ignored\n", + default_hstate_max_huge_pages); } hugetlb_init_hstates(); @@ -3174,20 +3190,27 @@ void __init hugetlb_add_hstate(unsigned int order) parsed_hstate = h; } -static int __init hugetlb_nrpages_setup(char *s) +/* + * hugepages command line processing + * hugepages must normally follows a valid hugepagsz specification. If not, + * ignore the hugepages value. hugepages can also be the first huge page + * command line option in which case it specifies the number of huge pages + * for the default size. + */ +static int __init hugepages_setup(char *s) { unsigned long *mhp; static unsigned long *last_mhp; if (!parsed_valid_hugepagesz) { - pr_warn("hugepages = %s preceded by " + pr_warn("HugeTLB: hugepages = %s preceded by " "an unsupported hugepagesz, ignoring\n", s); parsed_valid_hugepagesz = true; return 1; } /* - * !hugetlb_max_hstate means we haven't parsed a hugepagesz= parameter yet, - * so this hugepages= parameter goes to the "default hstate". + * !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; @@ -3195,7 +3218,8 @@ static int __init hugetlb_nrpages_setup(char *s) mhp = &parsed_hstate->max_huge_pages; if (mhp == last_mhp) { - pr_warn("hugepages= specified twice without interleaving hugepagesz=, ignoring\n"); + pr_warn("HugeTLB: hugepages= specified twice without interleaving hugepagesz=, ignoring hugepages=%s\n", + s); return 1; } @@ -3214,8 +3238,15 @@ static int __init hugetlb_nrpages_setup(char *s) return 1; } -__setup("hugepages=", hugetlb_nrpages_setup); +__setup("hugepages=", hugepages_setup); +/* + * hugepagesz command line processing + * A specific huge page size can only be specified once with hugepagesz. + * hugepagesz is followed by hugepages on the commnad line. The global + * variable 'parsed_valid_hugepagesz' is used to determine if prior + * hugepagesz argument was valid. + */ static int __init hugepagesz_setup(char *s) { unsigned long long size; @@ -3230,16 +3261,23 @@ static int __init hugepagesz_setup(char *s) } if (size_to_hstate(size)) { + parsed_valid_hugepagesz = false; pr_warn("HugeTLB: hugepagesz %s specified twice, ignoring\n", saved_s); return 0; } + parsed_valid_hugepagesz = true; hugetlb_add_hstate(ilog2(size) - PAGE_SHIFT); return 1; } __setup("hugepagesz=", hugepagesz_setup); +/* + * default_hugepagesz command line input + * Only one instance of default_hugepagesz allowed on command line. Do not + * add hstate here as that will confuse hugepagesz/hugepages processing. + */ static int __init default_hugepagesz_setup(char *s) { unsigned long long size; @@ -3252,6 +3290,12 @@ static int __init default_hugepagesz_setup(char *s) return 0; } + if (default_hstate_size) { + pr_err("HugeTLB: default_hugepagesz previously specified, ignoring %s\n", + saved_s); + return 0; + } + default_hstate_size = size; return 1; }
With all hugetlb page processing done in a single file clean up code. - Make code match desired semantics - Update documentation with semantics - Make all warnings and errors messages start with 'HugeTLB:'. - Consistently name command line parsing routines. - Add comments to code - Describe some of the subtle interactions - Describe semantics of command line arguments Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com> --- Documentation/admin-guide/mm/hugetlbpage.rst | 26 +++++++ mm/hugetlb.c | 78 +++++++++++++++----- 2 files changed, 87 insertions(+), 17 deletions(-)