+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
