From: Daniel Lezcano <daniel.lezcano@linaro.org>
To: Zhang Rui <rui.zhang@intel.com>, Junwen Wu <wudaemon@163.com>,
rafael@kernel.org, amitk@kernel.org
Cc: linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
Viresh Kumar <viresh.kumar@linaro.org>
Subject: Re: [PATCH v1] thermal/core: change mm alloc method to avoid kernel warning
Date: Tue, 19 Apr 2022 19:56:35 +0200 [thread overview]
Message-ID: <ddbbc1db-2dd3-0c4d-26c0-0992867d35be@linaro.org> (raw)
In-Reply-To: <df7e04d86dd64dc85125d536434d93bab3d6314d.camel@intel.com>
On 19/04/2022 15:54, Zhang Rui wrote:
> CC Viresh.
>
> On Tue, 2022-04-19 at 11:14 +0200, Daniel Lezcano wrote:
>> On 19/04/2022 10:48, Zhang Rui wrote:
>>> On Sun, 2022-04-17 at 12:56 +0000, Junwen Wu wrote:
>>>> Very high cooling device max state value makes cooling device
>>>> stats
>>>> buffer allocation fails,like below.Using kzvalloc instead of
>>>> kzalloc
>>>> can avoid this issue.
>>>
>>> When a cooling device has big max_state, this patch can get ride of
>>> the
>>> warning here, but still we end up with the read failure of the
>>> trans_table in sysfs because it is larger than PAGE_SIZE.
>>>
>>> $ cat /sys/class/thermal/cooling_device8/stats/trans_table
>>> cat: /sys/class/thermal/cooling_device8/stats/trans_table: File too
>>> large
>>>
>>> IMO, unless we can fix both places, I'd suggest we skip allocating
>>> and
>>> creating the broken trans_table attr. Like a prototype patch below
>>
>> Why not create a thermal debugfs with real useful information and
>> get
>> rid of this broken code ?
>
> The idea looks good to me.
What about doing a percentile approach of the state indexes changes
instead of a raw matrix full of zeros ? So we show the most significant
transitions, perhaps something like:
99%: 7->6 6->7
98%: 6->5 5->6
95%: 5->4 4->5
90%: 7->5 5->7
80%: 6->4 4->6
70%: 7->1 7->2
50%: ... ...
total: 123456 124573
And another statistics file containing some timings information like the
total duration in mitigation, and the duration in the most significant
states above?
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
WARNING: multiple messages have this Message-ID (diff)
From: Junwen Wu <wudaemon@163.com>
To: daniel.lezcano@linaro.org, Zhang Rui <rui.zhang@intel.com>,
Junwen Wu <wudaemon@163.com>,
rafael@kernel.org, amitk@kernel.org
Cc: linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
viresh.kumar@linaro.org
Subject: Re: [PATCH v1] thermal/core: change mm alloc method to avoid kernel warning
Date: Sun, 8 May 2022 15:07:50 +0000 [thread overview]
Message-ID: <ddbbc1db-2dd3-0c4d-26c0-0992867d35be@linaro.org> (raw) (raw)
Message-ID: <20220508150750.-AVnbE_5eME_5X0oa1Pqfwp4UnKGjlkoeSu8r_-4xdg@z> (raw)
In-Reply-To: <df7e04d86dd64dc85125d536434d93bab3d6314d.camel@intel.com>
From: Daniel Lezcano <daniel.lezcano@linaro.org>
On 19/04/2022 15:54, Zhang Rui wrote:
> CC Viresh.
>
> On Tue, 2022-04-19 at 11:14 +0200, Daniel Lezcano wrote:
>> On 19/04/2022 10:48, Zhang Rui wrote:
>>> large
>>>
>>> IMO, unless we can fix both places, I'd suggest we skip allocating
>>> and
>>> creating the broken trans_table attr. Like a prototype patch below
>>
>> Why not create a thermal debugfs with real useful information and
>> get
>> rid of this broken code ?
>
> The idea looks good to me.
>What about doing a percentile approach of the state indexes changes
>instead of a raw matrix full of zeros ? So we show the most significant
>transitions, perhaps something like:
>
>99%: 7->6 6->7
>98%: 6->5 5->6
>95%: 5->4 4->5
>90%: 7->5 5->7
>80%: 6->4 4->6
>70%: 7->1 7->2
>50%: ... ...
>total: 123456 124573
>And another statistics file containing some timings information like the
>total duration in mitigation, and the duration in the most significant
>states above?
Viresh, Zhang Rui, Daniel,sorry for the delay indeed ,the trans_table is always full of zero,
I introduce 'show_state' node(tunnable by user,default set as max_states/2) ,thus only show show_state'th trans count
to the max trans count change stats. in this way trans_table_show's buffer always less than PAGE_SIZE
I create a patch v2
like this:
/sys/class/thermal/cooling_device0/stats # cat trans_table
From : Index_change
state 0: ->1( 1) ->2( 2) ->7( 1)
state 1: ->0( 1) ->2( 1)
state 2: ->0( 2) ->1( 1)
here is the patch:
From 64a7fefd008cb890a4a9ea4efd0dd388ac536ad5 Mon Sep 17 00:00:00 2001
From: Junwen Wu <wudaemon@163.com>
Date: Sun, 8 May 2022 14:50:14 +0000
Subject: [PATCH v2] thermal/core: Make trans_table tunnable to avoid some
needless zero output
Very high cooling device max state value make trans_table node prompt File too large.
we introduce show_state node, tunnable by user,thus trans_table only show show_state'th
trans count to the max trans count, in this way trans_table_show's buffer is
always less than PAGE_SIZE and shows the important changes.
Signed-off-by: Junwen Wu <wudaemon@163.com>
---
V1 -> V2: avoid some needless zero output
drivers/thermal/thermal_sysfs.c | 136 +++++++++++++++++++++++---------
1 file changed, 99 insertions(+), 37 deletions(-)
diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index f154bada2906..1496088a1638 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -656,6 +656,7 @@ struct cooling_dev_stats {
spinlock_t lock;
unsigned int total_trans;
unsigned long state;
+ unsigned long show_state;
unsigned long max_states;
ktime_t last_time;
ktime_t *time_in_state;
@@ -752,60 +753,119 @@ reset_store(struct device *dev, struct device_attribute *attr, const char *buf,
return count;
}
-static ssize_t trans_table_show(struct device *dev,
- struct device_attribute *attr, char *buf)
+static ssize_t
+show_state_store(struct device *dev, struct device_attribute *attr, const char *buf,
+ size_t count)
{
- struct thermal_cooling_device *cdev = to_cooling_device(dev);
- struct cooling_dev_stats *stats = cdev->stats;
- ssize_t len = 0;
- int i, j;
+ struct thermal_cooling_device *cdev = to_cooling_device(dev);
+ struct cooling_dev_stats *stats = cdev->stats;
+ unsigned long state;
+ ssize_t ret;
- len += snprintf(buf + len, PAGE_SIZE - len, " From : To\n");
- len += snprintf(buf + len, PAGE_SIZE - len, " : ");
- for (i = 0; i < stats->max_states; i++) {
- if (len >= PAGE_SIZE)
- break;
- len += snprintf(buf + len, PAGE_SIZE - len, "state%2u ", i);
- }
- if (len >= PAGE_SIZE)
- return PAGE_SIZE;
+ spin_lock(&stats->lock);
- len += snprintf(buf + len, PAGE_SIZE - len, "\n");
+ ret = kstrtoul(buf, 10, &state);
+ if (ret || (state > stats->max_states))
+ goto unlock;
- for (i = 0; i < stats->max_states; i++) {
- if (len >= PAGE_SIZE)
- break;
+ stats->show_state = state;
+unlock:
+ spin_unlock(&stats->lock);
+ return count;
+}
- len += snprintf(buf + len, PAGE_SIZE - len, "state%2u:", i);
+static ssize_t
+show_state_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+ struct thermal_cooling_device *cdev = to_cooling_device(dev);
+ struct cooling_dev_stats *stats = cdev->stats;
+
+ return sprintf(buf, "%lu\n", stats->show_state);
+}
+
+static int find_show_state( int *nums, int numsSize, int k, unsigned int *max_value)
+{
+ int i, min = INT_MAX, max = 0;
+ for( i = 0; i < numsSize; ++i )
+ {
+ min = nums[i] < min ? nums[i] : min;
+ max = nums[i] > max ? nums[i] : max;
+ }
+ int l = min, r = max, mid, cnt = 0;
+ while( l < r )
+ {
+ mid = r - (r - l) / 2;
+ for( i = 0; i < numsSize; ++i )
+ {
+ if( nums[i] >= mid )
+ ++cnt;
+ }
+ if( cnt < k )
+ {
+ r = mid - 1;
+ cnt = 0;
+ }
+ else
+ {
+ l = mid;
+ cnt = 0;
+ }
+ }
+ *max_value = max;
+ return l;
+}
- for (j = 0; j < stats->max_states; j++) {
- if (len >= PAGE_SIZE)
- break;
- len += snprintf(buf + len, PAGE_SIZE - len, "%8u ",
- stats->trans_table[i * stats->max_states + j]);
- }
- if (len >= PAGE_SIZE)
- break;
- len += snprintf(buf + len, PAGE_SIZE - len, "\n");
- }
- if (len >= PAGE_SIZE) {
- pr_warn_once("Thermal transition table exceeds PAGE_SIZE. Disabling\n");
- return -EFBIG;
- }
- return len;
+
+static ssize_t trans_table_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct thermal_cooling_device *cdev = to_cooling_device(dev);
+ struct cooling_dev_stats *stats = cdev->stats;
+ ssize_t len = 0;
+ int i, j;
+ unsigned int show_state_value = 0;
+ unsigned int max_state_value = 0;
+
+ len += snprintf(buf + len, PAGE_SIZE - len, " From : Index_change\n");
+ for (i = 0; i < stats->max_states; i++) {
+ show_state_value = find_show_state(&stats->trans_table[i * stats->max_states], stats->max_states, stats->show_state, &max_state_value);
+ if (max_state_value) {
+ len += snprintf(buf + len, PAGE_SIZE - len, "state%2u:", i);
+ }
+ else {
+ continue;
+ }
+
+ for (j = 0; j < stats->max_states; j++) {
+ if (stats->trans_table[i * stats->max_states + j] && (show_state_value <= stats->trans_table[i * stats->max_states + j])) {
+ len += snprintf(buf + len, PAGE_SIZE - len, " ->%u(%u)",j, stats->trans_table[i * stats->max_states + j]);
+ }
+ }
+ if (len >= PAGE_SIZE)
+ break;
+ len += snprintf(buf + len, PAGE_SIZE - len, "\n");
+ }
+
+ if (len >= PAGE_SIZE) {
+ pr_warn_once("Thermal transition table exceeds PAGE_SIZE. Disabling\n");
+ return -EFBIG;
+ }
+ return len;
}
static DEVICE_ATTR_RO(total_trans);
static DEVICE_ATTR_RO(time_in_state_ms);
static DEVICE_ATTR_WO(reset);
static DEVICE_ATTR_RO(trans_table);
+static DEVICE_ATTR_RW(show_state);
static struct attribute *cooling_device_stats_attrs[] = {
&dev_attr_total_trans.attr,
&dev_attr_time_in_state_ms.attr,
&dev_attr_reset.attr,
&dev_attr_trans_table.attr,
+ &dev_attr_show_state.attr,
NULL
};
@@ -829,7 +889,7 @@ static void cooling_device_stats_setup(struct thermal_cooling_device *cdev)
var += sizeof(*stats->time_in_state) * states;
var += sizeof(*stats->trans_table) * states * states;
- stats = kzalloc(var, GFP_KERNEL);
+ stats = kvzalloc(var, GFP_KERNEL);
if (!stats)
return;
@@ -838,6 +898,8 @@ static void cooling_device_stats_setup(struct thermal_cooling_device *cdev)
cdev->stats = stats;
stats->last_time = ktime_get();
stats->max_states = states;
+ /* default set show_state = max_states/2 */
+ stats->show_state = states / 2;
spin_lock_init(&stats->lock);
@@ -848,7 +910,7 @@ static void cooling_device_stats_setup(struct thermal_cooling_device *cdev)
static void cooling_device_stats_destroy(struct thermal_cooling_device *cdev)
{
- kfree(cdev->stats);
+ kvfree(cdev->stats);
cdev->stats = NULL;
}
--
2.25.1
next prev parent reply other threads:[~2022-04-19 17:56 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-17 12:56 [PATCH v1] thermal/core: change mm alloc method to avoid kernel warning Junwen Wu
2022-04-19 8:48 ` Zhang Rui
2022-04-19 9:14 ` Daniel Lezcano
2022-04-19 13:54 ` Zhang Rui
2022-04-19 17:56 ` Daniel Lezcano [this message]
2022-05-08 15:07 ` Junwen Wu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ddbbc1db-2dd3-0c4d-26c0-0992867d35be@linaro.org \
--to=daniel.lezcano@linaro.org \
--cc=amitk@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=rafael@kernel.org \
--cc=rui.zhang@intel.com \
--cc=viresh.kumar@linaro.org \
--cc=wudaemon@163.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox