| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <9cfbdb8a-1326-e7e3-3a65-c3f8c45eaf19@samsung.com>
Date: Wed, 8 Jan 2020 20:22:33 +0900
From: Chanwoo Choi <cw00.choi@...sung.com>
To: Dmitry Osipenko <digetx@...il.com>, linux-pm@...r.kernel.org,
linux-kernel@...r.kernel.org
Cc: leonard.crestez@....com, lukasz.luba@....com, a.swigon@...sung.com,
m.szyprowski@...sung.com, enric.balletbo@...labora.com,
hl@...k-chips.com, bjorn.andersson@...aro.org,
jcrouse@...eaurora.org, chanwoo@...nel.org,
myungjoo.ham@...sung.com, kyungmin.park@...sung.com
Subject: Re: [PATCH 2/2] PM / devfreq: Add devfreq_transitions debugfs file
On 1/8/20 6:56 AM, Dmitry Osipenko wrote:
> 07.01.2020 12:05, Chanwoo Choi пишет:
>> Add new devfreq_transitions debugfs file to track the frequency transitions
>> of all devfreq devices for the simple profiling as following:
>> - /sys/kernel/debug/devfreq/devfreq_transitions
>>
>> And the user can decide the storage size (CONFIG_NR_DEVFREQ_TRANSITIONS)
>> in Kconfig in order to save the transition history.
>>
>> [Detailed description of each field of 'devfreq_transitions' debugfs file]
>> - time_ms : Change time of frequency transition. (unit: millisecond)
>> - dev_name : Device name of h/w.
>> - dev : Device name made by devfreq core.
>> - parent_dev : If devfreq device uses the passive governor,
>> show parent devfreq device name.
>> - load_% : If devfreq device uses the simple_ondemand governor,
>> load is used by governor whene deciding the new frequency.
>> (unit: percentage)
>> - old_freq_hz : Frequency before changing. (unit: hz)
>> - new_freq_hz : Frequency after changed. (unit: hz)
>>
>> [For example on Exynos5422-based Odroid-XU3 board]
>> $ cat /sys/kernel/debug/devfreq/devfreq_transitions
>> time_ms dev_name dev parent_dev load_% old_freq_hz new_freq_hz
>> ---------- ------------------------------ ---------- ---------- ---------- ------------ ------------
>> 14600 soc:bus_noc devfreq2 devfreq1 0 100000000 67000000
>> 14600 soc:bus_fsys_apb devfreq3 devfreq1 0 200000000 100000000
>> 14600 soc:bus_fsys devfreq4 devfreq1 0 200000000 100000000
>> 14600 soc:bus_fsys2 devfreq5 devfreq1 0 150000000 75000000
>> 14602 soc:bus_mfc devfreq6 devfreq1 0 222000000 96000000
>> 14602 soc:bus_gen devfreq7 devfreq1 0 267000000 89000000
>> 14602 soc:bus_g2d devfreq9 devfreq1 0 300000000 84000000
>> 14602 soc:bus_g2d_acp devfreq10 devfreq1 0 267000000 67000000
>> 14602 soc:bus_jpeg devfreq11 devfreq1 0 300000000 75000000
>> 14602 soc:bus_jpeg_apb devfreq12 devfreq1 0 167000000 84000000
>> 14603 soc:bus_disp1_fimd devfreq13 devfreq1 0 200000000 120000000
>> 14603 soc:bus_disp1 devfreq14 devfreq1 0 300000000 120000000
>> 14606 soc:bus_gscl_scaler devfreq15 devfreq1 0 300000000 150000000
>> 14606 soc:bus_mscl devfreq16 devfreq1 0 333000000 84000000
>> 14608 soc:bus_wcore devfreq1 9 333000000 84000000
>> 14783 10c20000.memory-controller devfreq0 35 825000000 633000000
>> 15873 soc:bus_wcore devfreq1 41 84000000 400000000
>> 15873 soc:bus_noc devfreq2 devfreq1 0 67000000 100000000
>> [snip]
>>
>> Signed-off-by: Chanwoo Choi <cw00.choi@...sung.com>
>> ---
>> drivers/devfreq/Kconfig | 13 +++
>> drivers/devfreq/devfreq.c | 126 +++++++++++++++++++++++++++++
>> drivers/devfreq/governor.h | 3 +
>> drivers/devfreq/governor_passive.c | 2 +
>> include/linux/devfreq.h | 1 +
>> 5 files changed, 145 insertions(+)
>>
>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>> index 0b1df12e0f21..84936eec0ef9 100644
>> --- a/drivers/devfreq/Kconfig
>> +++ b/drivers/devfreq/Kconfig
>> @@ -74,6 +74,19 @@ config DEVFREQ_GOV_PASSIVE
>> through sysfs entries. The passive governor recommends that
>> devfreq device uses the OPP table to get the frequency/voltage.
>>
>> +comment "DEVFREQ Debugging"
>> +
>> +config NR_DEVFREQ_TRANSITIONS
>> + int "Maximum storage size to save DEVFREQ Transitions (10-1000)"
>> + depends on DEBUG_FS
>> + range 10 1000
>> + default "100"
>> + help
>> + Show the frequency transitions of all devfreq devices via
>> + '/sys/kernel/debug/devfreq/devfreq_transitions' for the simple
>> + profiling. It needs to decide the storage size to save transition
>> + history of all devfreq devices.
>> +
>> comment "DEVFREQ Drivers"
>>
>> config ARM_EXYNOS_BUS_DEVFREQ
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index c7f5e4e06420..7abaae06fa65 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -268,6 +268,57 @@ int devfreq_update_status(struct devfreq *devfreq, unsigned long freq)
>> }
>> EXPORT_SYMBOL(devfreq_update_status);
>>
>> +/**
>> + * devfreq_update_transitions() - Update frequency transitions for debugfs file
>> + * @devfreq: the devfreq instance
>> + * @old_freq: the previous frequency before changing the frequency
>> + * @new_freq: the new frequency after frequency is changed
>> + */
>> +struct devfreq_transitions {
>> + struct devfreq *devfreq;
>> + struct devfreq_freqs freqs;
>> + unsigned long load;
>> +} debugfs_transitions[CONFIG_NR_DEVFREQ_TRANSITIONS];
>> +
>> +static spinlock_t devfreq_debugfs_lock;
>> +static int debugfs_transitions_index;
>> +
>> +void devfreq_update_transitions(struct devfreq *devfreq,
>> + unsigned long old_freq, unsigned long new_freq,
>> + unsigned long busy_time, unsigned long total_time)
>> +{
>> + unsigned long load;
>> + int i;
>> +
>> + if (!devfreq_debugfs || !devfreq || (old_freq == new_freq))
>> + return;
>> +
>> + spin_lock_nested(&devfreq_debugfs_lock, SINGLE_DEPTH_NESTING);
>> +
>> + i = debugfs_transitions_index;
>> +
>> + /*
>> + * Calculate the load and if load is larger than 100,
>> + * initialize to 100 because the unit of load is percentage.
>> + */
>> + load = (total_time == 0 ? 0 : (100 * busy_time) / total_time);
>> + if (load > 100)
>> + load = 100;
>> +
>> + debugfs_transitions[i].devfreq = devfreq;
>> + debugfs_transitions[i].freqs.time = ktime_to_ms(ktime_get());
>> + debugfs_transitions[i].freqs.old = old_freq;
>> + debugfs_transitions[i].freqs.new = new_freq;
>> + debugfs_transitions[i].load = load;
>> +
>> + if (++i == CONFIG_NR_DEVFREQ_TRANSITIONS)
>> + i = 0;
>> + debugfs_transitions_index = i;
>> +
>> + spin_unlock(&devfreq_debugfs_lock);
>> +}
>> +EXPORT_SYMBOL(devfreq_update_transitions);
>> +
>> /**
>> * find_devfreq_governor() - Find devfreq governor from name
>> * @name: name of the governor
>> @@ -401,6 +452,10 @@ static int set_target(struct devfreq *devfreq,
>> return err;
>> }
>>
>> + devfreq_update_transitions(devfreq, cur_freq, new_freq,
>> + devfreq->last_status.busy_time,
>> + devfreq->last_status.total_time);
>> +
>> freqs.new = new_freq;
>> notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
>>
>> @@ -1787,6 +1842,72 @@ static int devfreq_summary_show(struct seq_file *s, void *data)
>> }
>> DEFINE_SHOW_ATTRIBUTE(devfreq_summary);
>>
>> +/**
>> + * devfreq_transitions_show() - Show the frequency transitions of the registered
>> + * devfreq devices via 'devfreq_transitions' debugfs file.
>> + */
>> +static int devfreq_transitions_show(struct seq_file *s, void *data)
>> +{
>> + struct devfreq *devfreq = NULL;
>> + struct devfreq *p_devfreq = NULL;
>> + struct devfreq_freqs *freqs = NULL;
>> + unsigned long load;
>> + int i = debugfs_transitions_index;
>> + int count;
>> +
>> + seq_printf(s, "%-10s %-30s %-10s %-10s %-6s %-12s %-12s\n",
>> + "time_ms",
>> + "dev_name",
>> + "dev",
>> + "parent_dev",
>> + "load_%",
>> + "old_freq_hz",
>> + "new_freq_hz");
>> + seq_printf(s, "%-10s %-30s %-10s %-10s %-6s %-12s %-12s\n",
>> + "----------",
>> + "------------------------------",
>> + "----------",
>> + "----------",
>> + "----------",
>> + "------------",
>> + "------------");
>
> Isn't this needed here?
>
> mutex_lock(&devfreq_list_lock);
It doesn't touch the devfreq instance of devfreq_list.
So, it is not necessary locked of devfreq_list_lock.
>
>> + spin_lock(&devfreq_debugfs_lock);
>> + for (count = 0; count < CONFIG_NR_DEVFREQ_TRANSITIONS; count++) {
>> + devfreq = debugfs_transitions[i].devfreq;
>> + freqs = &debugfs_transitions[i].freqs;
>> + load = debugfs_transitions[i].load;
>> +
>> + i = (CONFIG_NR_DEVFREQ_TRANSITIONS == ++i) ? 0 : i;
>> + if (!devfreq)
>> + continue;
>
> I suppose debugfs_transitions[i].devfreq should be set to NULL when
> devfreq device is removed, but I don't see it happening anywhere in this
> patch.
When debugfs_transitions[] array is not fully filled out
by devfreq_update_transitions(), debugfs_transitions[i].devfreq is NULL.
In this case, if user execute 'cat /sys/kernel/debug/devfreq/devfreq_transitions',
devfreq_transitions_show() need to check the debugfs_transitions[i].devfreq
is NULL or not.
After filled out the debugfs_transitions[] array,
actually, 'if(!devfreq)' is not necessary. Maybe, this style is inefficient
It need to rework. I'll think again.
>
>> +#if IS_ENABLED(CONFIG_DEVFREQ_GOV_PASSIVE)
>> + if (!strncmp(devfreq->governor_name,
>> + DEVFREQ_GOV_PASSIVE, DEVFREQ_NAME_LEN)) {
>
> This could be:
>
> if (IS_ENABLED(CONFIG_DEVFREQ_GOV_PASSIVE) &&
> !strncmp(devfreq->governor_name,
> DEVFREQ_GOV_PASSIVE, DEVFREQ_NAME_LEN)) {
Good. It is more clear. I'll edit it according to your comment.
>
>> + struct devfreq_passive_data *data = devfreq->data;
>> +
>> + if (data)
>> + p_devfreq = data->parent;
>
> const char *devname = "";
>
> ...
>
> if (data)
> devname = dev_name(data->parent);
>
'devname' word is too general. It is difficult to know
this name is for parent devfreq device. So, I prefer
to keep the origin style.
>> + } else {
>> + p_devfreq = NULL;
>> + }
>> +#endif
>> + seq_printf(s, "%-10lld %-30s %-10s %-10s %-6ld %-12ld %-12ld\n",
>> + freqs->time,
>> + dev_name(devfreq->dev.parent),
>> + dev_name(&devfreq->dev),
>> + p_devfreq ? dev_name(&p_devfreq->dev) : "",
>> + load,
>> + freqs->old,
>> + freqs->new);
>> + }
>> + spin_unlock(&devfreq_debugfs_lock);
>> +
>> + return 0;
>> +}
>> +DEFINE_SHOW_ATTRIBUTE(devfreq_transitions);
>> +
>> static int __init devfreq_init(void)
>> {
>> devfreq_class = class_create(THIS_MODULE, "devfreq");
>> @@ -1808,9 +1929,14 @@ static int __init devfreq_init(void)
>> devfreq_debugfs = NULL;
>> pr_warn("%s: couldn't create debugfs dir\n", __FILE__);
>> } else {
>> + spin_lock_init(&devfreq_debugfs_lock);
>> +
>> debugfs_create_file("devfreq_summary", 0444,
>> devfreq_debugfs, NULL,
>> &devfreq_summary_fops);
>> + debugfs_create_file("devfreq_transitions", 0444,
>> + devfreq_debugfs, NULL,
>> + &devfreq_transitions_fops);
>> }
>>
>> return 0;
>> diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
>> index dc7533ccc3db..01eecfdaf2d6 100644
>> --- a/drivers/devfreq/governor.h
>> +++ b/drivers/devfreq/governor.h
>> @@ -68,6 +68,9 @@ extern int devfreq_add_governor(struct devfreq_governor *governor);
>> extern int devfreq_remove_governor(struct devfreq_governor *governor);
>>
>> extern int devfreq_update_status(struct devfreq *devfreq, unsigned long freq);
>> +extern void devfreq_update_transitions(struct devfreq *devfreq,
>> + unsigned long old_freq, unsigned long new_freq,
>> + unsigned long busy_time, unsigned long total_time);
>>
>> static inline int devfreq_update_stats(struct devfreq *df)
>> {
>> diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
>> index be6eeab9c814..05fa654239f5 100644
>> --- a/drivers/devfreq/governor_passive.c
>> +++ b/drivers/devfreq/governor_passive.c
>> @@ -109,6 +109,8 @@ static int update_devfreq_passive(struct devfreq *devfreq, unsigned long freq)
>> if (ret < 0)
>> goto out;
>>
>> + devfreq_update_transitions(devfreq, devfreq->previous_freq, freq, 0, 0);
>> +
>> if (devfreq->profile->freq_table
>> && (devfreq_update_status(devfreq, freq)))
>> dev_err(&devfreq->dev,
>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
>> index 49cdb2378030..933692e5d867 100644
>> --- a/include/linux/devfreq.h
>> +++ b/include/linux/devfreq.h
>> @@ -196,6 +196,7 @@ struct devfreq {
>> };
>>
>> struct devfreq_freqs {
>> + s64 time;
>> unsigned long old;
>> unsigned long new;
>> };
>>
>
>
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
Powered by blists - more mailing lists