* [bug report] x86, sched: Bail out of frequency invariance if turbo frequency is unknown
@ 2022-05-05 7:05 Dan Carpenter
2022-05-13 8:15 ` Giovanni Gherdovich
2022-05-13 13:17 ` Giovanni Gherdovich
0 siblings, 2 replies; 5+ messages in thread
From: Dan Carpenter @ 2022-05-05 7:05 UTC (permalink / raw)
To: ggherdovich; +Cc: kernel-janitors
Hello Giovanni Gherdovich,
The patch 51beea8862a3: "x86, sched: Bail out of frequency invariance
if turbo frequency is unknown" from May 31, 2020, leads to the
following Smatch static checker warning:
arch/x86/kernel/cpu/aperfmperf.c:274 intel_set_max_freq_ratio()
error: uninitialized symbol 'turbo_freq'.
arch/x86/kernel/cpu/aperfmperf.c
242 static bool __init intel_set_max_freq_ratio(void)
243 {
244 u64 base_freq, turbo_freq;
245 u64 turbo_ratio;
246
247 if (slv_set_max_freq_ratio(&base_freq, &turbo_freq))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Imagine this fails.
248 goto out;
249
250 if (x86_match_cpu(has_glm_turbo_ratio_limits) &&
251 skx_set_max_freq_ratio(&base_freq, &turbo_freq, 1))
252 goto out;
253
254 if (x86_match_cpu(has_knl_turbo_ratio_limits) &&
255 knl_set_max_freq_ratio(&base_freq, &turbo_freq, 1))
256 goto out;
257
258 if (x86_match_cpu(has_skx_turbo_ratio_limits) &&
259 skx_set_max_freq_ratio(&base_freq, &turbo_freq, 4))
260 goto out;
261
262 if (core_set_max_freq_ratio(&base_freq, &turbo_freq))
263 goto out;
264
265 return false;
266
267 out:
268 /*
269 * Some hypervisors advertise X86_FEATURE_APERFMPERF
270 * but then fill all MSR's with zeroes.
271 * Some CPUs have turbo boost but don't declare any turbo ratio
272 * in MSR_TURBO_RATIO_LIMIT.
273 */
--> 274 if (!base_freq || !turbo_freq) {
^^^^^^^^^^^^^^^^^^^^^^^^^
Uninitialized. Although I notice that base_freq is also unintialized
and that predates your patch... So I should probably send this bug
report to someone else... Sorry?
275 pr_debug("Couldn't determine cpu base or turbo frequency, necessary for scale-invariant accounting.\n");
276 return false;
277 }
278
279 turbo_ratio = div_u64(turbo_freq * SCHED_CAPACITY_SCALE, base_freq);
280 if (!turbo_ratio) {
281 pr_debug("Non-zero turbo and base frequencies led to a 0 ratio.\n");
282 return false;
283 }
284
285 arch_turbo_freq_ratio = turbo_ratio;
286 arch_set_max_freq_ratio(turbo_disabled());
287
288 return true;
289 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] x86, sched: Bail out of frequency invariance if turbo frequency is unknown
2022-05-05 7:05 [bug report] x86, sched: Bail out of frequency invariance if turbo frequency is unknown Dan Carpenter
@ 2022-05-13 8:15 ` Giovanni Gherdovich
2022-05-13 13:17 ` Giovanni Gherdovich
1 sibling, 0 replies; 5+ messages in thread
From: Giovanni Gherdovich @ 2022-05-13 8:15 UTC (permalink / raw)
To: Dan Carpenter; +Cc: kernel-janitors
On Thu, 2022-05-05 at 10:05 +0300, Dan Carpenter wrote:
> Hello Giovanni Gherdovich,
>
> The patch 51beea8862a3: "x86, sched: Bail out of frequency invariance
> if turbo frequency is unknown" from May 31, 2020, leads to the
> following Smatch static checker warning:
>
> arch/x86/kernel/cpu/aperfmperf.c:274 intel_set_max_freq_ratio()
> error: uninitialized symbol 'turbo_freq'.
>
> arch/x86/kernel/cpu/aperfmperf.c
> 242 static bool __init intel_set_max_freq_ratio(void)
> 243 {
> 244 u64 base_freq, turbo_freq;
> 245 u64 turbo_ratio;
> 246
> 247 if (slv_set_max_freq_ratio(&base_freq, &turbo_freq))
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> Imagine this fails.
>
> 248 goto out;
> 249
> 250 if (x86_match_cpu(has_glm_turbo_ratio_limits) &&
> 251 skx_set_max_freq_ratio(&base_freq, &turbo_freq, 1))
> 252 goto out;
> 253
> 254 if (x86_match_cpu(has_knl_turbo_ratio_limits) &&
> 255 knl_set_max_freq_ratio(&base_freq, &turbo_freq, 1))
> 256 goto out;
> 257
> 258 if (x86_match_cpu(has_skx_turbo_ratio_limits) &&
> 259 skx_set_max_freq_ratio(&base_freq, &turbo_freq, 4))
> 260 goto out;
> 261
> 262 if (core_set_max_freq_ratio(&base_freq, &turbo_freq))
> 263 goto out;
> 264
> 265 return false;
> 266
> 267 out:
> 268 /*
> 269 * Some hypervisors advertise X86_FEATURE_APERFMPERF
> 270 * but then fill all MSR's with zeroes.
> 271 * Some CPUs have turbo boost but don't declare any turbo ratio
> 272 * in MSR_TURBO_RATIO_LIMIT.
> 273 */
> --> 274 if (!base_freq || !turbo_freq) {
> ^^^^^^^^^^^^^^^^^^^^^^^^^
> Uninitialized. Although I notice that base_freq is also unintialized
> and that predates your patch... So I should probably send this bug
> report to someone else... Sorry?
>
Hello Dan,
I'm the right person for this report, as I think I wrote both the buggy
patch and the buggy code that predates it. I'm taking a moment to check
what's going on, as that really look like a dumb mistake.
Thanks for the report, I'll follow up.
Giovanni
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] x86, sched: Bail out of frequency invariance if turbo frequency is unknown
2022-05-05 7:05 [bug report] x86, sched: Bail out of frequency invariance if turbo frequency is unknown Dan Carpenter
2022-05-13 8:15 ` Giovanni Gherdovich
@ 2022-05-13 13:17 ` Giovanni Gherdovich
2022-05-13 14:05 ` Dan Carpenter
1 sibling, 1 reply; 5+ messages in thread
From: Giovanni Gherdovich @ 2022-05-13 13:17 UTC (permalink / raw)
To: Dan Carpenter; +Cc: kernel-janitors
On Thu, 2022-05-05 at 10:05 +0300, Dan Carpenter wrote:
> Hello Giovanni Gherdovich,
>
> The patch 51beea8862a3: "x86, sched: Bail out of frequency invariance
> if turbo frequency is unknown" from May 31, 2020, leads to the
> following Smatch static checker warning:
>
> arch/x86/kernel/cpu/aperfmperf.c:274 intel_set_max_freq_ratio()
> error: uninitialized symbol 'turbo_freq'.
>
> arch/x86/kernel/cpu/aperfmperf.c
> 242 static bool __init intel_set_max_freq_ratio(void)
> 243 {
> 244 u64 base_freq, turbo_freq;
> 245 u64 turbo_ratio;
> 246
> 247 if (slv_set_max_freq_ratio(&base_freq, &turbo_freq))
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> Imagine this fails.
>
> 248 goto out;
I think this is a false positive of the static checker; I'll send the patch
to initialize the variables anyway as it looks better.
When slv_set_max_freq_ratio() fails, it returns false, and we move on to the
next "if" statement. "goto out" is executed only if slv_set_max_freq_ratio()
succeeds (returns true), and writes data in its input parameters.
I give you that the code looks wrong, because apparently when people (and
static analyzers) read that, they think "if (problem) goto error;" but this
function is written like "if (it_works) goto the_end;".
When I received your report I thought the code was wrong, too.
You already sent me this report two years ago,
https://www.spinics.net/lists/linux-kernel-janitors/msg51372.html
and I didn't reply (I should have).
> 249
> 250 if (x86_match_cpu(has_glm_turbo_ratio_limits) &&
> 251 skx_set_max_freq_ratio(&base_freq, &turbo_freq, 1))
> 252 goto out;
> 253
> 254 if (x86_match_cpu(has_knl_turbo_ratio_limits) &&
> 255 knl_set_max_freq_ratio(&base_freq, &turbo_freq, 1))
> 256 goto out;
> 257
> 258 if (x86_match_cpu(has_skx_turbo_ratio_limits) &&
> 259 skx_set_max_freq_ratio(&base_freq, &turbo_freq, 4))
> 260 goto out;
> 261
> 262 if (core_set_max_freq_ratio(&base_freq, &turbo_freq))
> 263 goto out;
> 264
> 265 return false;
The "return false" above is what happens if none of the "if" matched, and this
is when base_freq and turbo_freq are uninitialized.
> 266
> 267 out:
> 268 /*
> 269 * Some hypervisors advertise X86_FEATURE_APERFMPERF
> 270 * but then fill all MSR's with zeroes.
> 271 * Some CPUs have turbo boost but don't declare any turbo ratio
> 272 * in MSR_TURBO_RATIO_LIMIT.
> 273 */
> --> 274 if (!base_freq || !turbo_freq) {
> ^^^^^^^^^^^^^^^^^^^^^^^^^
> Uninitialized.
I know it looks sketchy (and makes you think I forgot the initialization), but
the condition here is checking for when a *_set_max_freq_ratio() above
matched, but received zeroes from reading the corresponding MSR.
Anyways, patch incoming to make this look better, but I don't think it's a
"fixes: [...]".
> Although I notice that base_freq is also unintialized
> and that predates your patch... So I should probably send this bug
> report to someone else... Sorry?
>
> 275 pr_debug("Couldn't determine cpu base or turbo frequency, necessary for scale-invariant accounting.\n");
> 276 return false;
> 277 }
> 278
> 279 turbo_ratio = div_u64(turbo_freq * SCHED_CAPACITY_SCALE, base_freq);
> 280 if (!turbo_ratio) {
> 281 pr_debug("Non-zero turbo and base frequencies led to a 0 ratio.\n");
> 282 return false;
> 283 }
> 284
> 285 arch_turbo_freq_ratio = turbo_ratio;
> 286 arch_set_max_freq_ratio(turbo_disabled());
> 287
> 288 return true;
> 289 }
>
> regards,
> dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] x86, sched: Bail out of frequency invariance if turbo frequency is unknown
2022-05-13 13:17 ` Giovanni Gherdovich
@ 2022-05-13 14:05 ` Dan Carpenter
2022-05-13 14:40 ` Giovanni Gherdovich
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2022-05-13 14:05 UTC (permalink / raw)
To: Giovanni Gherdovich; +Cc: kernel-janitors
On Fri, May 13, 2022 at 03:17:24PM +0200, Giovanni Gherdovich wrote:
> On Thu, 2022-05-05 at 10:05 +0300, Dan Carpenter wrote:
> > Hello Giovanni Gherdovich,
> >
> > The patch 51beea8862a3: "x86, sched: Bail out of frequency invariance
> > if turbo frequency is unknown" from May 31, 2020, leads to the
> > following Smatch static checker warning:
> >
> > arch/x86/kernel/cpu/aperfmperf.c:274 intel_set_max_freq_ratio()
> > error: uninitialized symbol 'turbo_freq'.
> >
> > arch/x86/kernel/cpu/aperfmperf.c
> > 242 static bool __init intel_set_max_freq_ratio(void)
> > 243 {
> > 244 u64 base_freq, turbo_freq;
> > 245 u64 turbo_ratio;
> > 246
> > 247 if (slv_set_max_freq_ratio(&base_freq, &turbo_freq))
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >
> > Imagine this fails.
> >
> > 248 goto out;
>
> I think this is a false positive of the static checker; I'll send the patch
> to initialize the variables anyway as it looks better.
>
> When slv_set_max_freq_ratio() fails, it returns false, and we move on to the
> next "if" statement. "goto out" is executed only if slv_set_max_freq_ratio()
> succeeds (returns true), and writes data in its input parameters.
>
> I give you that the code looks wrong, because apparently when people (and
> static analyzers) read that, they think "if (problem) goto error;" but this
> function is written like "if (it_works) goto the_end;".
I read this wrong. But looking at it closer, I think the static analyzer is
actually correct (definitely correct-ish for sure). Smatch is complaining about
knl_set_max_freq_ratio().
149 static bool __init knl_set_max_freq_ratio(u64 *base_freq, u64 *turbo_freq,
150 int num_delta_fratio)
151 {
152 int fratio, delta_fratio, found;
153 int err, i;
154 u64 msr;
155
156 err = rdmsrl_safe(MSR_PLATFORM_INFO, base_freq);
157 if (err)
158 return false;
159
160 *base_freq = (*base_freq >> 8) & 0xFF; /* max P state */
161
162 err = rdmsrl_safe(MSR_TURBO_RATIO_LIMIT, &msr);
163 if (err)
164 return false;
165
166 fratio = (msr >> 8) & 0xFF;
167 i = 16;
168 found = 0;
169 do {
170 if (found >= num_delta_fratio) {
171 *turbo_freq = fratio;
172 return true;
173 }
174
175 delta_fratio = (msr >> (i + 5)) & 0x7;
176
177 if (delta_fratio) {
178 found += 1;
179 fratio -= delta_fratio;
180 }
181
182 i += 8;
183 } while (i < 64);
184
185 return true;
If we reach this "return true" then turbo_freq is not set. This may not be
reachable in real life. Should it be return false?
186 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] x86, sched: Bail out of frequency invariance if turbo frequency is unknown
2022-05-13 14:05 ` Dan Carpenter
@ 2022-05-13 14:40 ` Giovanni Gherdovich
0 siblings, 0 replies; 5+ messages in thread
From: Giovanni Gherdovich @ 2022-05-13 14:40 UTC (permalink / raw)
To: Dan Carpenter; +Cc: kernel-janitors
On Fri, 2022-05-13 at 17:05 +0300, Dan Carpenter wrote:
> On Fri, May 13, 2022 at 03:17:24PM +0200, Giovanni Gherdovich wrote:
> > On Thu, 2022-05-05 at 10:05 +0300, Dan Carpenter wrote:
> > > Hello Giovanni Gherdovich,
> > >
> > > The patch 51beea8862a3: "x86, sched: Bail out of frequency invariance
> > > if turbo frequency is unknown" from May 31, 2020, leads to the
> > > following Smatch static checker warning:
> > >
> > > arch/x86/kernel/cpu/aperfmperf.c:274 intel_set_max_freq_ratio()
> > > error: uninitialized symbol 'turbo_freq'.
> > >
> > > arch/x86/kernel/cpu/aperfmperf.c
> > > 242 static bool __init intel_set_max_freq_ratio(void)
> > > 243 {
> > > 244 u64 base_freq, turbo_freq;
> > > 245 u64 turbo_ratio;
> > > 246
> > > 247 if (slv_set_max_freq_ratio(&base_freq, &turbo_freq))
> > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > >
> > > Imagine this fails.
> > >
> > > 248 goto out;
> >
> > I think this is a false positive of the static checker; I'll send the patch
> > to initialize the variables anyway as it looks better.
> >
> > When slv_set_max_freq_ratio() fails, it returns false, and we move on to the
> > next "if" statement. "goto out" is executed only if slv_set_max_freq_ratio()
> > succeeds (returns true), and writes data in its input parameters.
> >
> > I give you that the code looks wrong, because apparently when people (and
> > static analyzers) read that, they think "if (problem) goto error;" but this
> > function is written like "if (it_works) goto the_end;".
>
> I read this wrong. But looking at it closer, I think the static analyzer is
> actually correct (definitely correct-ish for sure). Smatch is complaining about
> knl_set_max_freq_ratio().
>
> 149 static bool __init knl_set_max_freq_ratio(u64 *base_freq, u64 *turbo_freq,
> 150 int num_delta_fratio)
> 151 {
> 152 int fratio, delta_fratio, found;
> 153 int err, i;
> 154 u64 msr;
> 155
> 156 err = rdmsrl_safe(MSR_PLATFORM_INFO, base_freq);
> 157 if (err)
> 158 return false;
> 159
> 160 *base_freq = (*base_freq >> 8) & 0xFF; /* max P state */
> 161
> 162 err = rdmsrl_safe(MSR_TURBO_RATIO_LIMIT, &msr);
> 163 if (err)
> 164 return false;
> 165
> 166 fratio = (msr >> 8) & 0xFF;
> 167 i = 16;
> 168 found = 0;
> 169 do {
> 170 if (found >= num_delta_fratio) {
> 171 *turbo_freq = fratio;
> 172 return true;
> 173 }
> 174
> 175 delta_fratio = (msr >> (i + 5)) & 0x7;
> 176
> 177 if (delta_fratio) {
> 178 found += 1;
> 179 fratio -= delta_fratio;
> 180 }
> 181
> 182 i += 8;
> 183 } while (i < 64);
> 184
> 185 return true;
>
> If we reach this "return true" then turbo_freq is not set. This may not be
> reachable in real life. Should it be return false?
>
That's right, it's a bug. It should return false at the end.
I'll send a fix, thanks.
Giovanni
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-05-13 14:44 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-05-05 7:05 [bug report] x86, sched: Bail out of frequency invariance if turbo frequency is unknown Dan Carpenter
2022-05-13 8:15 ` Giovanni Gherdovich
2022-05-13 13:17 ` Giovanni Gherdovich
2022-05-13 14:05 ` Dan Carpenter
2022-05-13 14:40 ` Giovanni Gherdovich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.