| Message ID | 20220509062703.64249-4-songmuchun@bytedance.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | add hugetlb_optimize_vmemmap sysctl | expand |
On 09.05.22 08:27, Muchun Song wrote: > Use kstrtobool rather than open coding "on" and "off" parsing in > mm/hugetlb_vmemmap.c, which is more powerful to handle all kinds > of parameters like 'Yy1Nn0' or [oO][NnFf] for "on" and "off". > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com> > --- > Documentation/admin-guide/kernel-parameters.txt | 6 +++--- > mm/hugetlb_vmemmap.c | 10 +++++----- > 2 files changed, 8 insertions(+), 8 deletions(-) > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > index 308da668bbb1..43b8385073ad 100644 > --- a/Documentation/admin-guide/kernel-parameters.txt > +++ b/Documentation/admin-guide/kernel-parameters.txt > @@ -1703,10 +1703,10 @@ > enabled. > Allows heavy hugetlb users to free up some more > memory (7 * PAGE_SIZE for each 2MB hugetlb page). > - Format: { on | off (default) } > + Format: { [oO][Nn]/Y/y/1 | [oO][Ff]/N/n/0 (default) } Really? Can we make the syntax even harder to parse for human beings?! :) Not to mention that it's partially wrong? What about "oFf" ? That would have to be [oO][Ff][Ff] Honestly, "on | off" is good enough. That "oN" and friends work is just a "nice to have" IMHO. No need to over-complicate this description. > > - on: enable the feature > - off: disable the feature > + [oO][Nn]/Y/y/1: enable the feature > + [oO][Ff]/N/n/0: disable the feature > > Built with CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP_DEFAULT_ON=y, > the default is on. > diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c > index 6254bb2d4ae5..cc4ec752ec16 100644 > --- a/mm/hugetlb_vmemmap.c > +++ b/mm/hugetlb_vmemmap.c > @@ -28,15 +28,15 @@ EXPORT_SYMBOL(hugetlb_optimize_vmemmap_key); > > static int __init hugetlb_vmemmap_early_param(char *buf) > { > - if (!buf) > + bool enable; > + > + if (kstrtobool(buf, &enable)) > return -EINVAL; > > - if (!strcmp(buf, "on")) > + if (enable) > static_branch_enable(&hugetlb_optimize_vmemmap_key); > - else if (!strcmp(buf, "off")) > - static_branch_disable(&hugetlb_optimize_vmemmap_key); > else > - return -EINVAL; > + static_branch_disable(&hugetlb_optimize_vmemmap_key); > > return 0; > } Apart from that Acked-by: David Hildenbrand <david@redhat.com>
On Thu, May 12, 2022 at 09:41:22AM +0200, David Hildenbrand wrote: > On 09.05.22 08:27, Muchun Song wrote: > > Use kstrtobool rather than open coding "on" and "off" parsing in > > mm/hugetlb_vmemmap.c, which is more powerful to handle all kinds > > of parameters like 'Yy1Nn0' or [oO][NnFf] for "on" and "off". > > > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > > Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com> > > --- > > Documentation/admin-guide/kernel-parameters.txt | 6 +++--- > > mm/hugetlb_vmemmap.c | 10 +++++----- > > 2 files changed, 8 insertions(+), 8 deletions(-) > > > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > > index 308da668bbb1..43b8385073ad 100644 > > --- a/Documentation/admin-guide/kernel-parameters.txt > > +++ b/Documentation/admin-guide/kernel-parameters.txt > > @@ -1703,10 +1703,10 @@ > > enabled. > > Allows heavy hugetlb users to free up some more > > memory (7 * PAGE_SIZE for each 2MB hugetlb page). > > - Format: { on | off (default) } > > + Format: { [oO][Nn]/Y/y/1 | [oO][Ff]/N/n/0 (default) } > > Really? Can we make the syntax even harder to parse for human beings?! :) > > Not to mention that it's partially wrong? What about "oFf" ? That would > have to be [oO][Ff][Ff] > > Honestly, "on | off" is good enough. That "oN" and friends work is just > a "nice to have" IMHO. No need to over-complicate this description. Got it. How about also telling users "on/1 | off/0"? Because 0 and 1 are also widely used to disable/enable a knob. > > > > - on: enable the feature > > - off: disable the feature > > + [oO][Nn]/Y/y/1: enable the feature > > + [oO][Ff]/N/n/0: disable the feature > > > > Built with CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP_DEFAULT_ON=y, > > the default is on. > > diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c > > index 6254bb2d4ae5..cc4ec752ec16 100644 > > --- a/mm/hugetlb_vmemmap.c > > +++ b/mm/hugetlb_vmemmap.c > > @@ -28,15 +28,15 @@ EXPORT_SYMBOL(hugetlb_optimize_vmemmap_key); > > > > static int __init hugetlb_vmemmap_early_param(char *buf) > > { > > - if (!buf) > > + bool enable; > > + > > + if (kstrtobool(buf, &enable)) > > return -EINVAL; > > > > - if (!strcmp(buf, "on")) > > + if (enable) > > static_branch_enable(&hugetlb_optimize_vmemmap_key); > > - else if (!strcmp(buf, "off")) > > - static_branch_disable(&hugetlb_optimize_vmemmap_key); > > else > > - return -EINVAL; > > + static_branch_disable(&hugetlb_optimize_vmemmap_key); > > > > return 0; > > } > > > Apart from that > > Acked-by: David Hildenbrand <david@redhat.com> > Thanks. > -- > Thanks, > > David / dhildenb > >
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index 308da668bbb1..43b8385073ad 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -1703,10 +1703,10 @@ enabled. Allows heavy hugetlb users to free up some more memory (7 * PAGE_SIZE for each 2MB hugetlb page). - Format: { on | off (default) } + Format: { [oO][Nn]/Y/y/1 | [oO][Ff]/N/n/0 (default) } - on: enable the feature - off: disable the feature + [oO][Nn]/Y/y/1: enable the feature + [oO][Ff]/N/n/0: disable the feature Built with CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP_DEFAULT_ON=y, the default is on. diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c index 6254bb2d4ae5..cc4ec752ec16 100644 --- a/mm/hugetlb_vmemmap.c +++ b/mm/hugetlb_vmemmap.c @@ -28,15 +28,15 @@ EXPORT_SYMBOL(hugetlb_optimize_vmemmap_key); static int __init hugetlb_vmemmap_early_param(char *buf) { - if (!buf) + bool enable; + + if (kstrtobool(buf, &enable)) return -EINVAL; - if (!strcmp(buf, "on")) + if (enable) static_branch_enable(&hugetlb_optimize_vmemmap_key); - else if (!strcmp(buf, "off")) - static_branch_disable(&hugetlb_optimize_vmemmap_key); else - return -EINVAL; + static_branch_disable(&hugetlb_optimize_vmemmap_key); return 0; }