| Message ID | 20210116170725.5245-1-dsmythies@telus.net |
|---|---|
| State | New |
| Headers | show |
| Series | tools/power/x86/turbostat: Fix TCC offset bit mask | expand |
Thanks for the close read, Doug. This field size actually varies from system to system, but the reality is that the offset is never that big, and so the smaller mask is sufficient. Finally, this may all be moot, because there is discussion that using the offset this way is simply erroneous. stay tuned. -Len On Sat, Jan 16, 2021 at 12:07 PM Doug Smythies <doug.smythies@gmail.com> wrote: > > The TCC offset mask is incorrect, resulting in > incorrect target temperature calculations, if > the offset is big enough to exceed the mask size. > > Signed-off-by: Doug Smythies <dsmythies@telus.net> > --- > tools/power/x86/turbostat/turbostat.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c > index 389ea5209a83..d7acdd4d16c4 100644 > --- a/tools/power/x86/turbostat/turbostat.c > +++ b/tools/power/x86/turbostat/turbostat.c > @@ -4823,7 +4823,7 @@ int read_tcc_activation_temp() > > target_c = (msr >> 16) & 0xFF; > > - offset_c = (msr >> 24) & 0xF; > + offset_c = (msr >> 24) & 0x3F; > > tcc = target_c - offset_c; > > -- > 2.25.1 > -- Len Brown, Intel Open Source Technology Center
Hi Len, thank you for your reply. On Thu, Mar 11, 2021 at 3:19 PM Len Brown <lenb@kernel.org> wrote: > > Thanks for the close read, Doug. > > This field size actually varies from system to system, > but the reality is that the offset is never that big, and so the > smaller mask is sufficient. Disagree. I want to use an offset of 26. > Finally, this may all be moot, because there is discussion that using > the offset this way is simply erroneous. Disagree. It works great. As far as I know/recall I was the only person that responded to Rui's thread "thermal/intel: introduce tcc cooling driver" [1] And, I spent quite a bit of time doing so. However, I agree the response seems different between the two systems under test, Rui's and mine. [1] https://marc.info/?l=linux-pm&m=161070345329806&w=2 > stay tuned. O.K. ... Doug > > -Len > > > On Sat, Jan 16, 2021 at 12:07 PM Doug Smythies <doug.smythies@gmail.com> wrote: > > > > The TCC offset mask is incorrect, resulting in > > incorrect target temperature calculations, if > > the offset is big enough to exceed the mask size. > > > > Signed-off-by: Doug Smythies <dsmythies@telus.net> > > --- > > tools/power/x86/turbostat/turbostat.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c > > index 389ea5209a83..d7acdd4d16c4 100644 > > --- a/tools/power/x86/turbostat/turbostat.c > > +++ b/tools/power/x86/turbostat/turbostat.c > > @@ -4823,7 +4823,7 @@ int read_tcc_activation_temp() > > > > target_c = (msr >> 16) & 0xFF; > > > > - offset_c = (msr >> 24) & 0xF; > > + offset_c = (msr >> 24) & 0x3F; > > > > tcc = target_c - offset_c; > > > > -- > > 2.25.1 > > > > > -- > Len Brown, Intel Open Source Technology Center
Doug, The offset works for control. However, it is erroneous to use it for reporting of the actual temperature, like I did in turbostat. Thus, I'm going to revert the patch that added it's use in turbostat for the Temperature column. thanks, -Len On Fri, Mar 12, 2021 at 1:26 AM Doug Smythies <dsmythies@telus.net> wrote: > > Hi Len, > > > thank you for your reply. > > On Thu, Mar 11, 2021 at 3:19 PM Len Brown <lenb@kernel.org> wrote: > > > > Thanks for the close read, Doug. > > > > This field size actually varies from system to system, > > but the reality is that the offset is never that big, and so the > > smaller mask is sufficient. > > Disagree. > > I want to use an offset of 26. > > > Finally, this may all be moot, because there is discussion that using > > the offset this way is simply erroneous. > > Disagree. > It works great. > As far as I know/recall I was the only person that responded to Rui's thread > "thermal/intel: introduce tcc cooling driver" [1] > And, I spent quite a bit of time doing so. > However, I agree the response seems different between the two systems > under test, Rui's and mine. > > [1] https://marc.info/?l=linux-pm&m=161070345329806&w=2 > > > stay tuned. > > O.K. > > ... Doug > > > > -Len > > > > > > On Sat, Jan 16, 2021 at 12:07 PM Doug Smythies <doug.smythies@gmail.com> wrote: > > > > > > The TCC offset mask is incorrect, resulting in > > > incorrect target temperature calculations, if > > > the offset is big enough to exceed the mask size. > > > > > > Signed-off-by: Doug Smythies <dsmythies@telus.net> > > > --- > > > tools/power/x86/turbostat/turbostat.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c > > > index 389ea5209a83..d7acdd4d16c4 100644 > > > --- a/tools/power/x86/turbostat/turbostat.c > > > +++ b/tools/power/x86/turbostat/turbostat.c > > > @@ -4823,7 +4823,7 @@ int read_tcc_activation_temp() > > > > > > target_c = (msr >> 16) & 0xFF; > > > > > > - offset_c = (msr >> 24) & 0xF; > > > + offset_c = (msr >> 24) & 0x3F; > > > > > > tcc = target_c - offset_c; > > > > > > -- > > > 2.25.1 > > > > > > > > > -- > > Len Brown, Intel Open Source Technology Center -- Len Brown, Intel Open Source Technology Center
On Fri, Mar 12, 2021 at 2:16 PM Len Brown <lenb@kernel.org> wrote: > > Doug, > The offset works for control. > > However, it is erroneous to use it for reporting of the actual > temperature, like I did in turbostat. Agreed. I have been running with a correction for that for a while, and as discussed on Rui's thread. But this bit mask correction patch is still needed isn't it? for this: cpu4: MSR_IA32_TEMPERATURE_TARGET: 0x1a64100d (90 C) (100 default - 10 offset) which should be this: cpu4: MSR_IA32_TEMPERATURE_TARGET: 0x1a64100d (74 C) (100 default - 26 offset) But yes, I do now see the field size is only 4 bits for some parts. ... Doug > Thus, I'm going to revert the patch that added it's use in turbostat > for the Temperature column. > > thanks, > -Len > > On Fri, Mar 12, 2021 at 1:26 AM Doug Smythies <dsmythies@telus.net> wrote: > > > > Hi Len, > > > > > > thank you for your reply. > > > > On Thu, Mar 11, 2021 at 3:19 PM Len Brown <lenb@kernel.org> wrote: > > > > > > Thanks for the close read, Doug. > > > > > > This field size actually varies from system to system, > > > but the reality is that the offset is never that big, and so the > > > smaller mask is sufficient. > > > > Disagree. > > > > I want to use an offset of 26. > > > > > Finally, this may all be moot, because there is discussion that using > > > the offset this way is simply erroneous. > > > > Disagree. > > It works great. > > As far as I know/recall I was the only person that responded to Rui's thread > > "thermal/intel: introduce tcc cooling driver" [1] > > And, I spent quite a bit of time doing so. > > However, I agree the response seems different between the two systems > > under test, Rui's and mine. > > > > [1] https://marc.info/?l=linux-pm&m=161070345329806&w=2 > > > > > stay tuned. > > > > O.K. > > > > ... Doug > > > > > > -Len > > > > > > > > > On Sat, Jan 16, 2021 at 12:07 PM Doug Smythies <doug.smythies@gmail.com> wrote: > > > > > > > > The TCC offset mask is incorrect, resulting in > > > > incorrect target temperature calculations, if > > > > the offset is big enough to exceed the mask size. > > > > > > > > Signed-off-by: Doug Smythies <dsmythies@telus.net> > > > > --- > > > > tools/power/x86/turbostat/turbostat.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c > > > > index 389ea5209a83..d7acdd4d16c4 100644 > > > > --- a/tools/power/x86/turbostat/turbostat.c > > > > +++ b/tools/power/x86/turbostat/turbostat.c > > > > @@ -4823,7 +4823,7 @@ int read_tcc_activation_temp() > > > > > > > > target_c = (msr >> 16) & 0xFF; > > > > > > > > - offset_c = (msr >> 24) & 0xF; > > > > + offset_c = (msr >> 24) & 0x3F; > > > > > > > > tcc = target_c - offset_c; > > > > > > > > -- > > > > 2.25.1 > > > > > > > > > > > > > -- > > > Len Brown, Intel Open Source Technology Center > > > > -- > Len Brown, Intel Open Source Technology Center
On Sat, 2021-03-13 at 07:16 -0800, Doug Smythies wrote: > On Fri, Mar 12, 2021 at 2:16 PM Len Brown <lenb@kernel.org> wrote: > > > > Doug, > > The offset works for control. > > > > However, it is erroneous to use it for reporting of the actual > > temperature, like I did in turbostat. > > Agreed. > I have been running with a correction for that for a while, > and as discussed on Rui's thread. > But this bit mask correction patch is still needed isn't it? > for this: > cpu4: MSR_IA32_TEMPERATURE_TARGET: 0x1a64100d (90 C) (100 default - > 10 offset) > which should be this: > cpu4: MSR_IA32_TEMPERATURE_TARGET: 0x1a64100d (74 C) (100 default - > 26 offset) > > But yes, I do now see the field size is only 4 bits for some parts. As this is CPU specific, and we don't know which is which for all the CPUs, so it seems that we can have a white list for the ones that we care and have been verified. For the others, by default, we only show the raw value and default TCC activation temperature, like cpu4: MSR_IA32_TEMPERATURE_TARGET: 0x1a64100d (100 default ) And this white list can be updated together with the one in the kernel tcc_offset_cooling driver. what do you think? thanks, rui > > ... Doug > > > Thus, I'm going to revert the patch that added it's use in > > turbostat > > for the Temperature column. > > > > thanks, > > -Len > > > > On Fri, Mar 12, 2021 at 1:26 AM Doug Smythies <dsmythies@telus.net> > > wrote: > > > > > > Hi Len, > > > > > > > > > thank you for your reply. > > > > > > On Thu, Mar 11, 2021 at 3:19 PM Len Brown <lenb@kernel.org> > > > wrote: > > > > > > > > Thanks for the close read, Doug. > > > > > > > > This field size actually varies from system to system, > > > > but the reality is that the offset is never that big, and so > > > > the > > > > smaller mask is sufficient. > > > > > > Disagree. > > > > > > I want to use an offset of 26. > > > > > > > Finally, this may all be moot, because there is discussion that > > > > using > > > > the offset this way is simply erroneous. > > > > > > Disagree. > > > It works great. > > > As far as I know/recall I was the only person that responded to > > > Rui's thread > > > "thermal/intel: introduce tcc cooling driver" [1] > > > And, I spent quite a bit of time doing so. > > > However, I agree the response seems different between the two > > > systems > > > under test, Rui's and mine. > > > > > > [1] https://marc.info/?l=linux-pm&m=161070345329806&w=2 > > > > > > > stay tuned. > > > > > > O.K. > > > > > > ... Doug > > > > > > > > -Len > > > > > > > > > > > > On Sat, Jan 16, 2021 at 12:07 PM Doug Smythies < > > > > doug.smythies@gmail.com> wrote: > > > > > > > > > > The TCC offset mask is incorrect, resulting in > > > > > incorrect target temperature calculations, if > > > > > the offset is big enough to exceed the mask size. > > > > > > > > > > Signed-off-by: Doug Smythies <dsmythies@telus.net> > > > > > --- > > > > > tools/power/x86/turbostat/turbostat.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/tools/power/x86/turbostat/turbostat.c > > > > > b/tools/power/x86/turbostat/turbostat.c > > > > > index 389ea5209a83..d7acdd4d16c4 100644 > > > > > --- a/tools/power/x86/turbostat/turbostat.c > > > > > +++ b/tools/power/x86/turbostat/turbostat.c > > > > > @@ -4823,7 +4823,7 @@ int read_tcc_activation_temp() > > > > > > > > > > target_c = (msr >> 16) & 0xFF; > > > > > > > > > > - offset_c = (msr >> 24) & 0xF; > > > > > + offset_c = (msr >> 24) & 0x3F; > > > > > > > > > > tcc = target_c - offset_c; > > > > > > > > > > -- > > > > > 2.25.1 > > > > > > > > > > > > > > > > > -- > > > > Len Brown, Intel Open Source Technology Center > > > > > > > > -- > > Len Brown, Intel Open Source Technology Center
diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c index 389ea5209a83..d7acdd4d16c4 100644 --- a/tools/power/x86/turbostat/turbostat.c +++ b/tools/power/x86/turbostat/turbostat.c @@ -4823,7 +4823,7 @@ int read_tcc_activation_temp() target_c = (msr >> 16) & 0xFF; - offset_c = (msr >> 24) & 0xF; + offset_c = (msr >> 24) & 0x3F; tcc = target_c - offset_c;
The TCC offset mask is incorrect, resulting in incorrect target temperature calculations, if the offset is big enough to exceed the mask size. Signed-off-by: Doug Smythies <dsmythies@telus.net> --- tools/power/x86/turbostat/turbostat.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)