Re: [PATCH v14 1/6] sched/core: uclamp: Extend CPU's cgroup controller
From: Patrick Bellasi <hidden>
Date: 2019-09-02 06:41:16
Also in:
cgroups, linux-pm, lkml
On Fri, Aug 30, 2019 at 09:45:05 +0000, Peter Zijlstra wrote...
On Thu, Aug 22, 2019 at 02:28:06PM +0100, Patrick Bellasi wrote:quoted
+#define _POW10(exp) ((unsigned int)1e##exp) +#define POW10(exp) _POW10(exp)What is this magic? You're forcing a float literal into an integer. Surely that deserves a comment!
Yes, I'm introducing the two constants: UCLAMP_PERCENT_SHIFT, UCLAMP_PERCENT_SCALE similar to what we have for CAPACITY. Moreover, I need both 100*100 (for the scale) and 100 further down in the code for the: percent = div_u64_rem(percent, POW10(UCLAMP_PERCENT_SHIFT), &rem); used in cpu_uclamp_print(). That's why adding a compile time support to compute a 10^N is useful. C provides the "1eN" literal, I just convert it to integer and to do that at compile time I need a two level macros. What if I add this comment just above the macro definitions: /* * Integer 10^N with a given N exponent by casting to integer the literal "1eN" * C expression. Since there is no way to convert a macro argument (N) into a * character constant, use two levels of macros. */ is this clear enough?
quoted
+struct uclamp_request { +#define UCLAMP_PERCENT_SHIFT 2 +#define UCLAMP_PERCENT_SCALE (100 * POW10(UCLAMP_PERCENT_SHIFT)) + s64 percent; + u64 util; + int ret; +}; + +static inline struct uclamp_request +capacity_from_percent(char *buf) +{ + struct uclamp_request req = { + .percent = UCLAMP_PERCENT_SCALE, + .util = SCHED_CAPACITY_SCALE, + .ret = 0, + }; + + buf = strim(buf); + if (strncmp("max", buf, 4)) {That is either a bug, and you meant to write: strncmp(buf, "max", 3), or it is not, and then you could've written: strcmp(buf, "max")
I don't think it's a bug. The usage of 4 is intentional, to force a '\0' check while using strncmp(). Otherwise, strncmp(buf, "max", 3) would accept also strings starting by "max", which we don't want.
But as written it doesn't make sense.
The code is safe but I agree that strcmp() does just the same and it does not generate confusion. That's actually a pretty good example on how it's not always better to use strncmp() instead of strcmp(). Cheers, Patrick