[PATCH] intel_powerclamp: Fix cstate counter detection.

[PATCH] intel_powerclamp: Fix cstate counter detection.

[Yuxuan Shui]

Having all zero cstate count doesn't necesserily mean the cstate
counter is no functional.

Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>
---
 drivers/thermal/intel_powerclamp.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/thermal/intel_powerclamp.c b/drivers/thermal/intel_powerclamp.c
index b40b37c..e302769 100644
--- a/drivers/thermal/intel_powerclamp.c
+++ b/drivers/thermal/intel_powerclamp.c
@@ -206,6 +206,15 @@ static void find_target_mwait(void)
 
 }
 
+static bool has_pkg_state_counter(void)
+{
+	u64 tmp;
+	return !rdmsrl_safe(MSR_PKG_C2_RESIDENCY, &tmp) ||
+	       !rdmsrl_safe(MSR_PKG_C3_RESIDENCY, &tmp) ||
+	       !rdmsrl_safe(MSR_PKG_C6_RESIDENCY, &tmp) ||
+	       !rdmsrl_safe(MSR_PKG_C7_RESIDENCY, &tmp);
+}
+
 static u64 pkg_state_counter(void)
 {
 	u64 val;
@@ -500,7 +509,7 @@ static int start_power_clamp(void)
 	struct task_struct *thread;
 
 	/* check if pkg cstate counter is completely 0, abort in this case */
-	if (!pkg_state_counter()) {
+	if (!has_pkg_state_counter()) {
 		pr_err("pkg cstate counter not functional, abort\n");
 		return -EINVAL;
 	}
-- 
1.8.4.2

Re: [PATCH] intel_powerclamp: Fix cstate counter detection.

[Arjan van de Ven]

On 11/17/2013 11:06 PM, Yuxuan Shui wrote:
> Having all zero cstate count doesn't necesserily mean the cstate
> counter is no functional.
>

... but it does mean that powerclamp will be non-functional

but you had a reason to make this patch.
Can you expand a little bit on what you were seeing that made you
decide this patch was needed ?

Re: [PATCH] intel_powerclamp: Fix cstate counter detection.

[Yuxuan Shui]

There are possibilities that the system is busy from boot so that it
doesn't enter C-state at all, right? In that situation powerclamp
won't work.

Also, pkg_state_counter is used to calculate a cstate ratio, and I
can't find any reason why powerclamp will be non-funtional when that
ratio is zero.

On Tue, Nov 19, 2013 at 12:22 AM, Arjan van de Ven
<arjan@linux.intel.com> wrote:
> On 11/17/2013 11:06 PM, Yuxuan Shui wrote:
>>
>> Having all zero cstate count doesn't necesserily mean the cstate
>> counter is no functional.
>>
>
> ... but it does mean that powerclamp will be non-functional
>
> but you had a reason to make this patch.
> Can you expand a little bit on what you were seeing that made you
> decide this patch was needed ?
>

Re: [PATCH] intel_powerclamp: Fix cstate counter detection.

[Yuxuan Shui]

On Tue, Nov 19, 2013 at 10:25 PM, Arjan van de Ven
<arjan@linux.intel.com> wrote:
> On 11/18/2013 9:29 PM, Yuxuan Shui wrote:
>>
>> On Tue, Nov 19, 2013 at 12:22 AM, Arjan van de Ven
>> <arjan@linux.intel.com> wrote:
>>>
>>> On 11/17/2013 11:06 PM, Yuxuan Shui wrote:
>>>>
>>>>
>>>> Having all zero cstate count doesn't necesserily mean the cstate
>>>> counter is no functional.
>>>>
>>>
>>> ... but it does mean that powerclamp will be non-functional
>>>
>>> but you had a reason to make this patch.
>>> Can you expand a little bit on what you were seeing that made you
>>> decide this patch was needed ?
>>>
>> There are possibilities that the system is busy from boot so that it
>> doesn't enter C-state at all, right? In that situation powerclamp
>> won't work.
>
>
>
> that is... extremely unlikely. Have you seen that happen?

Sure I've seen this, that's why I wrote this patch in the first place.
It's my (rather old) laptop. After boot powertop show zero percent in
C3 and C6, and powerclamp complaining pkg cstate counter is not
functional.

>
>>
>> Also, pkg_state_counter is used to calculate a cstate ratio, and I
>> can't find any reason why powerclamp will be non-funtional when that
>> ratio is zero.
>
>
> if the counters we use are zero.. we can't use them in our control loop

I skimmed through the code. pkg_state_counter is called twice (except
that once in start_power_clamp). Once in poll_pkg_cstate to calculate
pkg_cstate_ratio_cur which is used for sysfs. The other time is in
powerclamp_adjust_controls, which is used to calculate a ratio which
is stored in a global variable current_ratio.

The current_ratio is also used twice. Once in the same function, to
check if we have done enough idle injection. The other is in
adjust_compensation, where it's used to calculate a delta. In neither
case current_ratio being zero will matter.

Also, I'm using this patch myself, and it seems to be totally functional.

So I failed to see why the counter can't be zero. If I made any
mistakes, can you point them out?

>
> again, what were you actually seeing?

Re: [PATCH] intel_powerclamp: Fix cstate counter detection.

[Jacob Pan]

On Wed, 20 Nov 2013 02:09:12 +0800
Yuxuan Shui <yshuiv7@gmail.com> wrote:

> On Tue, Nov 19, 2013 at 10:25 PM, Arjan van de Ven
> <arjan@linux.intel.com> wrote:
> > On 11/18/2013 9:29 PM, Yuxuan Shui wrote:
> >>
> >> On Tue, Nov 19, 2013 at 12:22 AM, Arjan van de Ven
> >> <arjan@linux.intel.com> wrote:
> >>>
> >>> On 11/17/2013 11:06 PM, Yuxuan Shui wrote:
> >>>>
> >>>>
> >>>> Having all zero cstate count doesn't necesserily mean the cstate
> >>>> counter is no functional.
> >>>>
> >>>
> >>> ... but it does mean that powerclamp will be non-functional
> >>>
> >>> but you had a reason to make this patch.
> >>> Can you expand a little bit on what you were seeing that made you
> >>> decide this patch was needed ?
> >>>
> >> There are possibilities that the system is busy from boot so that
> >> it doesn't enter C-state at all, right? In that situation
> >> powerclamp won't work.
> >
> >
> >
> > that is... extremely unlikely. Have you seen that happen?
> 
> Sure I've seen this, that's why I wrote this patch in the first place.
> It's my (rather old) laptop. After boot powertop show zero percent in
> C3 and C6, and powerclamp complaining pkg cstate counter is not
> functional.
> 
I see, this is indeed a corner case where the sanity check by powerclamp
driver is too strict and refused to load. I am OK with your patch and
perhaps add a sanity check later while idle injection is in action?

> >
> >>
> >> Also, pkg_state_counter is used to calculate a cstate ratio, and I
> >> can't find any reason why powerclamp will be non-funtional when
> >> that ratio is zero.
> >
> >
> > if the counters we use are zero.. we can't use them in our control
> > loop
> 
> I skimmed through the code. pkg_state_counter is called twice (except
> that once in start_power_clamp). Once in poll_pkg_cstate to calculate
> pkg_cstate_ratio_cur which is used for sysfs. The other time is in
> powerclamp_adjust_controls, which is used to calculate a ratio which
> is stored in a global variable current_ratio.
> 
> The current_ratio is also used twice. Once in the same function, to
> check if we have done enough idle injection. The other is in
> adjust_compensation, where it's used to calculate a delta. In neither
> case current_ratio being zero will matter.
> 
> Also, I'm using this patch myself, and it seems to be totally
> functional.
> 
> So I failed to see why the counter can't be zero. If I made any
> mistakes, can you point them out?
> 

> >
> > again, what were you actually seeing?
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Jacob Pan]

Re: [PATCH] intel_powerclamp: Fix cstate counter detection.

[Zhang Rui]

On Tue, 2013-11-19 at 10:38 -0800, Jacob Pan wrote:
> On Wed, 20 Nov 2013 02:09:12 +0800
> Yuxuan Shui <yshuiv7@gmail.com> wrote:
> 
> > On Tue, Nov 19, 2013 at 10:25 PM, Arjan van de Ven
> > <arjan@linux.intel.com> wrote:
> > > On 11/18/2013 9:29 PM, Yuxuan Shui wrote:
> > >>
> > >> On Tue, Nov 19, 2013 at 12:22 AM, Arjan van de Ven
> > >> <arjan@linux.intel.com> wrote:
> > >>>
> > >>> On 11/17/2013 11:06 PM, Yuxuan Shui wrote:
> > >>>>
> > >>>>
> > >>>> Having all zero cstate count doesn't necesserily mean the cstate
> > >>>> counter is no functional.
> > >>>>
> > >>>
> > >>> ... but it does mean that powerclamp will be non-functional
> > >>>
> > >>> but you had a reason to make this patch.
> > >>> Can you expand a little bit on what you were seeing that made you
> > >>> decide this patch was needed ?
> > >>>
> > >> There are possibilities that the system is busy from boot so that
> > >> it doesn't enter C-state at all, right? In that situation
> > >> powerclamp won't work.
> > >
> > >
> > >
> > > that is... extremely unlikely. Have you seen that happen?
> > 
> > Sure I've seen this, that's why I wrote this patch in the first place.
> > It's my (rather old) laptop. After boot powertop show zero percent in
> > C3 and C6, and powerclamp complaining pkg cstate counter is not
> > functional.
> > 
> I see, this is indeed a corner case where the sanity check by powerclamp
> driver is too strict and refused to load. I am OK with your patch and
> perhaps add a sanity check later while idle injection is in action?
> 
so do you want me to include this patch for 3.14?

thanks,
rui

> > >
> > >>
> > >> Also, pkg_state_counter is used to calculate a cstate ratio, and I
> > >> can't find any reason why powerclamp will be non-funtional when
> > >> that ratio is zero.
> > >
> > >
> > > if the counters we use are zero.. we can't use them in our control
> > > loop
> > 
> > I skimmed through the code. pkg_state_counter is called twice (except
> > that once in start_power_clamp). Once in poll_pkg_cstate to calculate
> > pkg_cstate_ratio_cur which is used for sysfs. The other time is in
> > powerclamp_adjust_controls, which is used to calculate a ratio which
> > is stored in a global variable current_ratio.
> > 
> > The current_ratio is also used twice. Once in the same function, to
> > check if we have done enough idle injection. The other is in
> > adjust_compensation, where it's used to calculate a delta. In neither
> > case current_ratio being zero will matter.
> > 
> > Also, I'm using this patch myself, and it seems to be totally
> > functional.
> > 
> > So I failed to see why the counter can't be zero. If I made any
> > mistakes, can you point them out?
> > 
> 
> > >
> > > again, what were you actually seeing?
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> [Jacob Pan]
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] intel_powerclamp: Fix cstate counter detection.

[jacob pan]

On Thu, 02 Jan 2014 11:03:32 +0800
Zhang Rui <rui.zhang@intel.com> wrote:

> On Tue, 2013-11-19 at 10:38 -0800, Jacob Pan wrote:
> > On Wed, 20 Nov 2013 02:09:12 +0800
> > Yuxuan Shui <yshuiv7@gmail.com> wrote:
> > 
> > > On Tue, Nov 19, 2013 at 10:25 PM, Arjan van de Ven
> > > <arjan@linux.intel.com> wrote:
> > > > On 11/18/2013 9:29 PM, Yuxuan Shui wrote:
> > > >>
> > > >> On Tue, Nov 19, 2013 at 12:22 AM, Arjan van de Ven
> > > >> <arjan@linux.intel.com> wrote:
> > > >>>
> > > >>> On 11/17/2013 11:06 PM, Yuxuan Shui wrote:
> > > >>>>
> > > >>>>
> > > >>>> Having all zero cstate count doesn't necesserily mean the
> > > >>>> cstate counter is no functional.
> > > >>>>
> > > >>>
> > > >>> ... but it does mean that powerclamp will be non-functional
> > > >>>
> > > >>> but you had a reason to make this patch.
> > > >>> Can you expand a little bit on what you were seeing that made
> > > >>> you decide this patch was needed ?
> > > >>>
> > > >> There are possibilities that the system is busy from boot so
> > > >> that it doesn't enter C-state at all, right? In that situation
> > > >> powerclamp won't work.
> > > >
> > > >
> > > >
> > > > that is... extremely unlikely. Have you seen that happen?
> > > 
> > > Sure I've seen this, that's why I wrote this patch in the first
> > > place. It's my (rather old) laptop. After boot powertop show zero
> > > percent in C3 and C6, and powerclamp complaining pkg cstate
> > > counter is not functional.
> > > 
> > I see, this is indeed a corner case where the sanity check by
> > powerclamp driver is too strict and refused to load. I am OK with
> > your patch and perhaps add a sanity check later while idle
> > injection is in action?
> > 
> so do you want me to include this patch for 3.14?
> 
yes.

Thanks,

Jacob
> thanks,
> rui
> 
> > > >
> > > >>
> > > >> Also, pkg_state_counter is used to calculate a cstate ratio,
> > > >> and I can't find any reason why powerclamp will be
> > > >> non-funtional when that ratio is zero.
> > > >
> > > >
> > > > if the counters we use are zero.. we can't use them in our
> > > > control loop
> > > 
> > > I skimmed through the code. pkg_state_counter is called twice
> > > (except that once in start_power_clamp). Once in poll_pkg_cstate
> > > to calculate pkg_cstate_ratio_cur which is used for sysfs. The
> > > other time is in powerclamp_adjust_controls, which is used to
> > > calculate a ratio which is stored in a global variable
> > > current_ratio.
> > > 
> > > The current_ratio is also used twice. Once in the same function,
> > > to check if we have done enough idle injection. The other is in
> > > adjust_compensation, where it's used to calculate a delta. In
> > > neither case current_ratio being zero will matter.
> > > 
> > > Also, I'm using this patch myself, and it seems to be totally
> > > functional.
> > > 
> > > So I failed to see why the counter can't be zero. If I made any
> > > mistakes, can you point them out?
> > > 
> > 
> > > >
> > > > again, what were you actually seeing?
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe
> > > linux-pm" in the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> > [Jacob Pan]
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pm"
> > in the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
>