2021-01-16 17:11:23

by Doug Smythies

Subject: [PATCH] tools/power/x86/turbostat: Fix TCC offset bit mask

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 <[email protected]>
---
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

2021-03-11 23:22:53

by Len Brown

Subject: Re: [PATCH] tools/power/x86/turbostat: Fix TCC offset bit mask

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 <[email protected]> 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 <[email protected]>
> ---
> 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

2021-03-12 11:39:36

by Doug Smythies

Subject: Re: [PATCH] tools/power/x86/turbostat: Fix TCC offset bit mask

Hi Len,


thank you for your reply.

On Thu, Mar 11, 2021 at 3:19 PM Len Brown <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > ---
> > 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

2021-03-12 22:17:43

by Len Brown

Subject: Re: [PATCH] tools/power/x86/turbostat: Fix TCC offset bit mask

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 <[email protected]> wrote:
>
> Hi Len,
>
>
> thank you for your reply.
>
> On Thu, Mar 11, 2021 at 3:19 PM Len Brown <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > ---
> > > 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

2021-03-13 15:20:32

by Doug Smythies

Subject: Re: [PATCH] tools/power/x86/turbostat: Fix TCC offset bit mask

On Fri, Mar 12, 2021 at 2:16 PM Len Brown <[email protected]> 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 <[email protected]> wrote:
> >
> > Hi Len,
> >
> >
> > thank you for your reply.
> >
> > On Thu, Mar 11, 2021 at 3:19 PM Len Brown <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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

2021-04-11 16:52:16

by Zhang, Rui

Subject: Re: [PATCH] tools/power/x86/turbostat: Fix TCC offset bit mask

On Sat, 2021-03-13 at 07:16 -0800, Doug Smythies wrote:
> On Fri, Mar 12, 2021 at 2:16 PM Len Brown <[email protected]> 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 <[email protected]>
> > wrote:
> > >
> > > Hi Len,
> > >
> > >
> > > thank you for your reply.
> > >
> > > On Thu, Mar 11, 2021 at 3:19 PM Len Brown <[email protected]>
> > > 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 <
> > > > [email protected]> 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 <[email protected]>
> > > > > ---
> > > > > 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