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

[David Hildenbrand <david@redhat.com>]

>>> +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