On Fri, Nov 1, 2024 at 10:39 AM David Hildenbrand <david@xxxxxxxxxx> wrote: > > On 31.10.24 22:12, Barry Song wrote: > > On Fri, Nov 1, 2024 at 3:20 AM Maíra Canal <mcanal@xxxxxxxxxx> wrote: > >> > >> On 31/10/24 10:33, David Hildenbrand wrote: > >>> On 31.10.24 14:24, Maíra Canal wrote: > >>>> Hi David, > >>>> > >>>> On 31/10/24 09:57, David Hildenbrand wrote: > >>>>> On 31.10.24 13:51, Maíra Canal wrote: > >>>>>> Hi David, > >>>>>> > >>>>>> On 31/10/24 09:37, David Hildenbrand wrote: > >>>>>>> On 30.10.24 13:58, Maíra Canal wrote: > >>>>>>>> Add the ``thp_shmem=`` kernel command line to allow specifying the > >>>>>>>> default policy of each supported shmem hugepage size. The kernel > >>>>>>>> parameter > >>>>>>>> accepts the following format: > >>>>>>>> > >>>>>>>> thp_shmem=<size>[KMG],<size>[KMG]:<policy>;<size>[KMG]- > >>>>>>>> <size>[KMG]:<policy> > >>>>>>>> > >>>>>>>> For example, > >>>>>>>> > >>>>>>>> thp_shmem=16K-64K:always;128K,512K:inherit;256K:advise;1M-2M:never;4M-8M:within_size > >>>>>>>> > >>>>>>>> By configuring the default policy of several shmem hugepages, the > >>>>>>>> user > >>>>>>>> can take advantage of mTHP before it's been configured through sysfs. > >>>>>>>> > >>>>>>>> Signed-off-by: Maíra Canal <mcanal@xxxxxxxxxx> > >>>>>>>> --- > >>>>>>>> .../admin-guide/kernel-parameters.txt | 10 ++ > >>>>>>>> Documentation/admin-guide/mm/transhuge.rst | 17 +++ > >>>>>>>> mm/shmem.c | 109 +++++++++++++ > >>>>>>>> ++++- > >>>>>>>> 3 files changed, 135 insertions(+), 1 deletion(-) > >>>>>>>> > >>>>>> > >>>>>> [...] > >>>>>> > >>>>>>>> diff --git a/mm/shmem.c b/mm/shmem.c > >>>>>>>> index dfcc88ec6e34..c2299fa0b345 100644 > >>>>>>>> --- a/mm/shmem.c > >>>>>>>> +++ b/mm/shmem.c > >>>>>>>> @@ -136,6 +136,7 @@ static unsigned long huge_shmem_orders_always > >>>>>>>> __read_mostly; > >>>>>>>> static unsigned long huge_shmem_orders_madvise __read_mostly; > >>>>>>>> static unsigned long huge_shmem_orders_inherit __read_mostly; > >>>>>>>> static unsigned long huge_shmem_orders_within_size __read_mostly; > >>>>>>>> +static bool shmem_orders_configured __initdata; > >>>>>>>> #endif > >>>>>>>> #ifdef CONFIG_TMPFS > >>>>>>>> @@ -5027,7 +5028,8 @@ void __init shmem_init(void) > >>>>>>>> * Default to setting PMD-sized THP to inherit the global > >>>>>>>> setting and > >>>>>>>> * disable all other multi-size THPs. > >>>>>>>> */ > >>>>>>>> - huge_shmem_orders_inherit = BIT(HPAGE_PMD_ORDER); > >>>>>>>> + if (!shmem_orders_configured) > >>>>>>>> + huge_shmem_orders_inherit = BIT(HPAGE_PMD_ORDER); > >>>>>>>> #endif > >>>>>>>> return; > >>>>>>>> @@ -5180,6 +5182,26 @@ struct kobj_attribute > >>>>>>>> thpsize_shmem_enabled_attr = > >>>>>>>> #if defined(CONFIG_TRANSPARENT_HUGEPAGE) > >>>>>>>> +static inline int get_order_from_str(const char *size_str) > >>>>>>>> +{ > >>>>>>>> + unsigned long size; > >>>>>>>> + char *endptr; > >>>>>>>> + int order; > >>>>>>>> + > >>>>>>>> + size = memparse(size_str, &endptr); > >>>>>>>> + > >>>>>>>> + if (!is_power_of_2(size)) > >>>>>>>> + goto err; > >>>>>>>> + order = get_order(size); > >>>>>>>> + if (BIT(order) & ~THP_ORDERS_ALL_FILE_DEFAULT) > >>>>>>>> + goto err; > >>>>>>>> + > >>>>>>>> + return order; > >>>>>>>> +err: > >>>>>>>> + pr_err("invalid size %s in thp_shmem boot parameter\n", > >>>>>>>> size_str); > >>>>>>>> + return -EINVAL; > >>>>>>>> +} > >>>>>>> > >>>>>>> Hm, mostly copy and paste. You could reuse existing > >>>>>>> get_order_from_str() > >>>>>>> simply by passing in the supported orders and moving error > >>>>>>> reporting to > >>>>>>> the caller. > >>>>>>> > >>>>>> > >>>>>> Can I use functions from mm/huge_memory.c here? > >>>>> > >>>>> Yes, that's the idea. > >>>>> > >>>> > >>>> Unfortunately, it isn't possible without adding the function to a > >>>> header. > >>> > >>> Well ... sure, what's the problem with that? > >> > >> David & Barry, how do you feel about adding `get_order_from_str()` to > >> lib/cmdline.c? > > > > I'd vote to leave it as is. If, at some point, the controls for shared memory > > and anonymous memory are moved to a file, that would be the right time > > to call the same get_order_from_str() for both. > > > This is too trivial to warrant being an exposed API in huge_memory.h > > or cmdline. > > I ... don't quite agree. cmdline.c is probably a bit excessive and I > wouldn't suggest that at this point. > > This seems like a reasonable helper function to have as inline in > mm/internal.h. For get_order_from_str() specifically, I agree that it's fine to keep it in internal.h, as it could be considered a general need. However, anything larger than that in parse bootcmd seems too trivial and not general enough to qualify as an API. > > ... unless I am missing something important and the obvious code > duplication is warranted. > > -- > Cheers, > > David / dhildenb > Thanks barry