Re: [PATCH v4] mm: Override mTHP "enabled" defaults at kernel cmdline

[David Hildenbrand]

> > > +static inline int get_order_from_str(const char *size_str)
> > > +{
> > > +     unsigned long size;
> > > +     char *endptr;
> > > +     int order;
> > > +
> > > +     size = memparse(size_str, &endptr);
> >
> > Do we have to also test if is_power_of_2(), and refuse if not? For
> > example, what if someone would pass 3K, would the existing check catch it?
>
> no, the existing check can't catch it.
>
> I passed thp_anon=15K-64K:always, then I got 16K enabled:
>
> / # cat /sys/kernel/mm/transparent_hugepage/hugepages-16kB/enabled
> [always] inherit madvise never

Okay, so we should document then that start/end of the range must be 
valid THP sizes.

> I can actually check that by:
>
> 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 >> PAGE_SHIFT))

No need for the shift.

if (!is_power_of_2(size))

Is likely even more correct if someone would manage to pass something 
stupid like

16385 (16K + 1)

> 		goto err;
> 	order = get_order(size);
> 	if ((1 << order) & ~THP_ORDERS_ALL_ANON)
> 		goto err;
>
> 	return order;
> err:
> 	pr_err("invalid size %s in thp_anon boot parameter\n", size_str);
> 	return -EINVAL;
> }
>
> > > +     order = fls(size >> PAGE_SHIFT) - 1;
> >
> > Is this a fancy way of writing
> >
> > order = log2(size >> PAGE_SHIFT);
> >
> > ? :)
>
> I think ilog2 is implemented by fls ?

Yes, so we should have used that instead. But get_order()
is even better.

> > Anyhow, if get_order() wraps that, all good.
>
> I guess it doesn't check power of 2?
>
> > > +     if ((1 << order) & ~THP_ORDERS_ALL_ANON) {
> > > +             pr_err("invalid size %s(order %d) in thp_anon boot parameter\n",
> > > +                     size_str, order);
> > > +             return -EINVAL;
> > > +     }
> > > +
> > > +     return order;
> > > +}
> >
> > Apart from that, nothing jumped at me.
>
> Please take a look at the new get_order_from_str() before I
> send v5 :-)

Besides the shift for is_power_of_2(), LGTM, thanks!

--
Cheers,

David / dhildenb