diff mbox series

tools/power/x86/turbostat: Fix TCC offset bit mask

Message ID 20210116170725.5245-1-dsmythies@telus.net
State New
Headers show
Series tools/power/x86/turbostat: Fix TCC offset bit mask | expand

Commit Message

Doug Smythies Jan. 16, 2021, 5:07 p.m. UTC
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(-)

Comments

Len Brown March 11, 2021, 11:18 p.m. UTC | #1
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
Doug Smythies March 12, 2021, 6:26 a.m. UTC | #2
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 March 12, 2021, 10:15 p.m. UTC | #3
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
Doug Smythies March 13, 2021, 3:16 p.m. UTC | #4
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
Zhang Rui April 11, 2021, 2:09 p.m. UTC | #5
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 mbox series

Patch

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;