* [PATCH v2] devfreq: Add tracepoint for frequency changes
@ 2019-09-19 17:44 ` Matthias Kaehlcke
2019-09-19 19:27 ` Steven Rostedt
2019-09-20 1:15 ` Chanwoo Choi
0 siblings, 2 replies; 6+ messages in thread
From: Matthias Kaehlcke @ 2019-09-19 17:44 UTC (permalink / raw)
To: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Steven Rostedt, Ingo Molnar
Cc: linux-pm, linux-kernel, Douglas Anderson, Matthias Kaehlcke
Add a tracepoint for frequency changes of devfreq devices and
use it.
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
(sending v2 without much delay wrt v1, since the change in devfreq
probably isn't controversial, and I'll be offline a few days)
Changes in v2:
- included trace_devfreq_frequency_enabled() in the condition
to avoid unnecessary evaluation when the trace point is
disabled
---
drivers/devfreq/devfreq.c | 3 +++
include/trace/events/devfreq.h | 18 ++++++++++++++++++
2 files changed, 21 insertions(+)
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index ab22bf8a12d6..e9f04dcafb01 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -317,6 +317,9 @@ static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq,
devfreq->previous_freq = new_freq;
+ if (trace_devfreq_frequency_enabled() && new_freq != cur_freq)
+ trace_devfreq_frequency(devfreq, new_freq);
+
if (devfreq->suspend_freq)
devfreq->resume_freq = cur_freq;
diff --git a/include/trace/events/devfreq.h b/include/trace/events/devfreq.h
index cf5b8772175d..a62d32fe3c33 100644
--- a/include/trace/events/devfreq.h
+++ b/include/trace/events/devfreq.h
@@ -8,6 +8,24 @@
#include <linux/devfreq.h>
#include <linux/tracepoint.h>
+TRACE_EVENT(devfreq_frequency,
+ TP_PROTO(struct devfreq *devfreq, unsigned long freq),
+
+ TP_ARGS(devfreq, freq),
+
+ TP_STRUCT__entry(
+ __string(dev_name, dev_name(&devfreq->dev))
+ __field(unsigned long, freq)
+ ),
+
+ TP_fast_assign(
+ __assign_str(dev_name, dev_name(&devfreq->dev));
+ __entry->freq = freq;
+ ),
+
+ TP_printk("dev_name=%s freq=%lu", __get_str(dev_name), __entry->freq)
+);
+
TRACE_EVENT(devfreq_monitor,
TP_PROTO(struct devfreq *devfreq),
--
2.23.0.237.gc6a4ce50a0-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] devfreq: Add tracepoint for frequency changes
2019-09-19 17:44 ` [PATCH v2] devfreq: Add tracepoint for frequency changes Matthias Kaehlcke
@ 2019-09-19 19:27 ` Steven Rostedt
2019-09-20 1:15 ` Chanwoo Choi
1 sibling, 0 replies; 6+ messages in thread
From: Steven Rostedt @ 2019-09-19 19:27 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Ingo Molnar, linux-pm,
linux-kernel, Douglas Anderson
On Thu, 19 Sep 2019 10:44:23 -0700
Matthias Kaehlcke <mka@chromium.org> wrote:
> Add a tracepoint for frequency changes of devfreq devices and
> use it.
>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
From the tracing point of view:
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
-- Steve
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] devfreq: Add tracepoint for frequency changes
2019-09-19 17:44 ` [PATCH v2] devfreq: Add tracepoint for frequency changes Matthias Kaehlcke
2019-09-19 19:27 ` Steven Rostedt
@ 2019-09-20 1:15 ` Chanwoo Choi
2019-09-24 19:37 ` Matthias Kaehlcke
1 sibling, 1 reply; 6+ messages in thread
From: Chanwoo Choi @ 2019-09-20 1:15 UTC (permalink / raw)
To: Matthias Kaehlcke, MyungJoo Ham, Kyungmin Park, Steven Rostedt,
Ingo Molnar
Cc: linux-pm, linux-kernel, Douglas Anderson
Hi,
On 19. 9. 20. 오전 2:44, Matthias Kaehlcke wrote:
> Add a tracepoint for frequency changes of devfreq devices and
> use it.
>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> (sending v2 without much delay wrt v1, since the change in devfreq
> probably isn't controversial, and I'll be offline a few days)
>
> Changes in v2:
> - included trace_devfreq_frequency_enabled() in the condition
> to avoid unnecessary evaluation when the trace point is
> disabled
> ---
> drivers/devfreq/devfreq.c | 3 +++
> include/trace/events/devfreq.h | 18 ++++++++++++++++++
> 2 files changed, 21 insertions(+)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index ab22bf8a12d6..e9f04dcafb01 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -317,6 +317,9 @@ static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq,
>
> devfreq->previous_freq = new_freq;
>
> + if (trace_devfreq_frequency_enabled() && new_freq != cur_freq)
> + trace_devfreq_frequency(devfreq, new_freq);
You can change as following without 'new_freq' variable
because devfreq->previous_freq is the new frequency.
trace_devfreq_frequency(devfreq);
> +
> if (devfreq->suspend_freq)
> devfreq->resume_freq = cur_freq;
>
> diff --git a/include/trace/events/devfreq.h b/include/trace/events/devfreq.h
> index cf5b8772175d..a62d32fe3c33 100644
> --- a/include/trace/events/devfreq.h
> +++ b/include/trace/events/devfreq.h
> @@ -8,6 +8,24 @@
> #include <linux/devfreq.h>
> #include <linux/tracepoint.h>
>
> +TRACE_EVENT(devfreq_frequency,
> + TP_PROTO(struct devfreq *devfreq, unsigned long freq),
'unsigned long freq' parameter is not necessary.
> +
> + TP_ARGS(devfreq, freq),
> +
> + TP_STRUCT__entry(
> + __string(dev_name, dev_name(&devfreq->dev))
> + __field(unsigned long, freq)
> + ),
> +
> + TP_fast_assign(
> + __assign_str(dev_name, dev_name(&devfreq->dev));
> + __entry->freq = freq;
Initialize the new frequency with 'devfreq->previous_freq' as following:
__entry->freq = devfreq->previous_freq;
> + ),
> +
> + TP_printk("dev_name=%s freq=%lu", __get_str(dev_name), __entry->freq)
> +);
> +
> TRACE_EVENT(devfreq_monitor,
> TP_PROTO(struct devfreq *devfreq),
>
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] devfreq: Add tracepoint for frequency changes
2019-09-20 1:15 ` Chanwoo Choi
@ 2019-09-24 19:37 ` Matthias Kaehlcke
2019-09-25 1:56 ` Chanwoo Choi
0 siblings, 1 reply; 6+ messages in thread
From: Matthias Kaehlcke @ 2019-09-24 19:37 UTC (permalink / raw)
To: Chanwoo Choi
Cc: MyungJoo Ham, Kyungmin Park, Steven Rostedt, Ingo Molnar,
linux-pm, linux-kernel, Douglas Anderson
On Fri, Sep 20, 2019 at 10:15:57AM +0900, Chanwoo Choi wrote:
> Hi,
sorry for the delayed response, you message got buried in my
mailbox.
> On 19. 9. 20. 오전 2:44, Matthias Kaehlcke wrote:
> > Add a tracepoint for frequency changes of devfreq devices and
> > use it.
> >
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> > (sending v2 without much delay wrt v1, since the change in devfreq
> > probably isn't controversial, and I'll be offline a few days)
> >
> > Changes in v2:
> > - included trace_devfreq_frequency_enabled() in the condition
> > to avoid unnecessary evaluation when the trace point is
> > disabled
> > ---
> > drivers/devfreq/devfreq.c | 3 +++
> > include/trace/events/devfreq.h | 18 ++++++++++++++++++
> > 2 files changed, 21 insertions(+)
> >
> > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> > index ab22bf8a12d6..e9f04dcafb01 100644
> > --- a/drivers/devfreq/devfreq.c
> > +++ b/drivers/devfreq/devfreq.c
> > @@ -317,6 +317,9 @@ static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq,
> >
> > devfreq->previous_freq = new_freq;
> >
> > + if (trace_devfreq_frequency_enabled() && new_freq != cur_freq)
> > + trace_devfreq_frequency(devfreq, new_freq);
>
> You can change as following without 'new_freq' variable
> because devfreq->previous_freq is the new frequency.
> trace_devfreq_frequency(devfreq);
In general that sounds good.
devfreq essentially uses df->previous_freq as df->cur_freq, I think
most code using it would be clearer if we renamed it accordingly.
I'll send a separate patch for this.
> > +
> > if (devfreq->suspend_freq)
> > devfreq->resume_freq = cur_freq;
> >
> > diff --git a/include/trace/events/devfreq.h b/include/trace/events/devfreq.h
> > index cf5b8772175d..a62d32fe3c33 100644
> > --- a/include/trace/events/devfreq.h
> > +++ b/include/trace/events/devfreq.h
> > @@ -8,6 +8,24 @@
> > #include <linux/devfreq.h>
> > #include <linux/tracepoint.h>
> >
> > +TRACE_EVENT(devfreq_frequency,
> > + TP_PROTO(struct devfreq *devfreq, unsigned long freq),
>
> 'unsigned long freq' parameter is not necessary.
>
> > +
> > + TP_ARGS(devfreq, freq),
> > +
> > + TP_STRUCT__entry(
> > + __string(dev_name, dev_name(&devfreq->dev))
> > + __field(unsigned long, freq)
> > + ),
> > +
> > + TP_fast_assign(
> > + __assign_str(dev_name, dev_name(&devfreq->dev));
> > + __entry->freq = freq;
>
> Initialize the new frequency with 'devfreq->previous_freq' as following:
>
> __entry->freq = devfreq->previous_freq;
>
> > + ),
> > +
> > + TP_printk("dev_name=%s freq=%lu", __get_str(dev_name), __entry->freq)
> > +);
> > +
> > TRACE_EVENT(devfreq_monitor,
> > TP_PROTO(struct devfreq *devfreq),
> >
> >
>
>
> --
> Best Regards,
> Chanwoo Choi
> Samsung Electronics
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] devfreq: Add tracepoint for frequency changes
2019-09-24 19:37 ` Matthias Kaehlcke
@ 2019-09-25 1:56 ` Chanwoo Choi
2019-09-25 17:45 ` Matthias Kaehlcke
0 siblings, 1 reply; 6+ messages in thread
From: Chanwoo Choi @ 2019-09-25 1:56 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: MyungJoo Ham, Kyungmin Park, Steven Rostedt, Ingo Molnar,
linux-pm, linux-kernel, Douglas Anderson
On 19. 9. 25. 오전 4:37, Matthias Kaehlcke wrote:
> On Fri, Sep 20, 2019 at 10:15:57AM +0900, Chanwoo Choi wrote:
>> Hi,
>
> sorry for the delayed response, you message got buried in my
> mailbox.
>
>> On 19. 9. 20. 오전 2:44, Matthias Kaehlcke wrote:
>>> Add a tracepoint for frequency changes of devfreq devices and
>>> use it.
>>>
>>> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
>>> ---
>>> (sending v2 without much delay wrt v1, since the change in devfreq
>>> probably isn't controversial, and I'll be offline a few days)
>>>
>>> Changes in v2:
>>> - included trace_devfreq_frequency_enabled() in the condition
>>> to avoid unnecessary evaluation when the trace point is
>>> disabled
>>> ---
>>> drivers/devfreq/devfreq.c | 3 +++
>>> include/trace/events/devfreq.h | 18 ++++++++++++++++++
>>> 2 files changed, 21 insertions(+)
>>>
>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>> index ab22bf8a12d6..e9f04dcafb01 100644
>>> --- a/drivers/devfreq/devfreq.c
>>> +++ b/drivers/devfreq/devfreq.c
>>> @@ -317,6 +317,9 @@ static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq,
>>>
>>> devfreq->previous_freq = new_freq;
>>>
>>> + if (trace_devfreq_frequency_enabled() && new_freq != cur_freq)
>>> + trace_devfreq_frequency(devfreq, new_freq);
>>
>> You can change as following without 'new_freq' variable
>> because devfreq->previous_freq is the new frequency.
>> trace_devfreq_frequency(devfreq);
>
> In general that sounds good.
>
> devfreq essentially uses df->previous_freq as df->cur_freq, I think
> most code using it would be clearer if we renamed it accordingly.
> I'll send a separate patch for this.
Actually, according to reference time of the 'df->previous_freq',
'previous_freq' is proper or 'cur_freq is proper.
But, In the comment of 'struct devfreq',
it means the configured time as following:
* @previous_freq: previously configured frequency value.
I think that it it not big problem to keep the name.
>
>>> +
>>> if (devfreq->suspend_freq)
>>> devfreq->resume_freq = cur_freq;
>>>
>>> diff --git a/include/trace/events/devfreq.h b/include/trace/events/devfreq.h
>>> index cf5b8772175d..a62d32fe3c33 100644
>>> --- a/include/trace/events/devfreq.h
>>> +++ b/include/trace/events/devfreq.h
>>> @@ -8,6 +8,24 @@
>>> #include <linux/devfreq.h>
>>> #include <linux/tracepoint.h>
>>>
>>> +TRACE_EVENT(devfreq_frequency,
>>> + TP_PROTO(struct devfreq *devfreq, unsigned long freq),
>>
>> 'unsigned long freq' parameter is not necessary.
>>
>>> +
>>> + TP_ARGS(devfreq, freq),
>>> +
>>> + TP_STRUCT__entry(
>>> + __string(dev_name, dev_name(&devfreq->dev))
>>> + __field(unsigned long, freq)
>>> + ),
>>> +
>>> + TP_fast_assign(
>>> + __assign_str(dev_name, dev_name(&devfreq->dev));
>>> + __entry->freq = freq;
>>
>> Initialize the new frequency with 'devfreq->previous_freq' as following:
>>
>> __entry->freq = devfreq->previous_freq;
>>
>>> + ),
>>> +
>>> + TP_printk("dev_name=%s freq=%lu", __get_str(dev_name), __entry->freq)
>>> +);
>>> +
>>> TRACE_EVENT(devfreq_monitor,
>>> TP_PROTO(struct devfreq *devfreq),
>>>
>>>
>>
>>
>> --
>> Best Regards,
>> Chanwoo Choi
>> Samsung Electronics
>
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] devfreq: Add tracepoint for frequency changes
2019-09-25 1:56 ` Chanwoo Choi
@ 2019-09-25 17:45 ` Matthias Kaehlcke
0 siblings, 0 replies; 6+ messages in thread
From: Matthias Kaehlcke @ 2019-09-25 17:45 UTC (permalink / raw)
To: Chanwoo Choi
Cc: MyungJoo Ham, Kyungmin Park, Steven Rostedt, Ingo Molnar,
linux-pm, linux-kernel, Douglas Anderson
On Wed, Sep 25, 2019 at 10:56:15AM +0900, Chanwoo Choi wrote:
> On 19. 9. 25. 오전 4:37, Matthias Kaehlcke wrote:
> > On Fri, Sep 20, 2019 at 10:15:57AM +0900, Chanwoo Choi wrote:
> >> Hi,
> >
> > sorry for the delayed response, you message got buried in my
> > mailbox.
> >
> >> On 19. 9. 20. 오전 2:44, Matthias Kaehlcke wrote:
> >>> Add a tracepoint for frequency changes of devfreq devices and
> >>> use it.
> >>>
> >>> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> >>> ---
> >>> (sending v2 without much delay wrt v1, since the change in devfreq
> >>> probably isn't controversial, and I'll be offline a few days)
> >>>
> >>> Changes in v2:
> >>> - included trace_devfreq_frequency_enabled() in the condition
> >>> to avoid unnecessary evaluation when the trace point is
> >>> disabled
> >>> ---
> >>> drivers/devfreq/devfreq.c | 3 +++
> >>> include/trace/events/devfreq.h | 18 ++++++++++++++++++
> >>> 2 files changed, 21 insertions(+)
> >>>
> >>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> >>> index ab22bf8a12d6..e9f04dcafb01 100644
> >>> --- a/drivers/devfreq/devfreq.c
> >>> +++ b/drivers/devfreq/devfreq.c
> >>> @@ -317,6 +317,9 @@ static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq,
> >>>
> >>> devfreq->previous_freq = new_freq;
> >>>
> >>> + if (trace_devfreq_frequency_enabled() && new_freq != cur_freq)
> >>> + trace_devfreq_frequency(devfreq, new_freq);
> >>
> >> You can change as following without 'new_freq' variable
> >> because devfreq->previous_freq is the new frequency.
> >> trace_devfreq_frequency(devfreq);
> >
> > In general that sounds good.
> >
> > devfreq essentially uses df->previous_freq as df->cur_freq, I think
> > most code using it would be clearer if we renamed it accordingly.
> > I'll send a separate patch for this.
>
> Actually, according to reference time of the 'df->previous_freq',
> 'previous_freq' is proper or 'cur_freq is proper.
> But, In the comment of 'struct devfreq',
> it means the configured time as following:
> * @previous_freq: previously configured frequency value.
sure, I wouldn't expect the comment of a variable/field called 'previous_freq'
say that it's the current frequency.
> I think that it it not big problem to keep the name.
It's indeed not a big problem, because the code works either way, something
like 'cur_freq' would just be less confusing.
These are the functions that use 'previous_freq' and how they use it:
devfreq_set_target
devfreq_monitor_suspend
cur_freq_show
target_freq_show
trans_stat_show
devfreq_passive_notifier_call
devfreq_userspace_func
cur freq
devfreq_update_status
prev freq
More than 85% use the variable as current frequency, which seems like a
good argument to give it the proper name, instead of having folks wonder
why the previous frequency is used.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-09-25 17:45 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20190919174436epcas4p17bf0528950813d3326237fcc56fd9b21@epcas4p1.samsung.com>
2019-09-19 17:44 ` [PATCH v2] devfreq: Add tracepoint for frequency changes Matthias Kaehlcke
2019-09-19 19:27 ` Steven Rostedt
2019-09-20 1:15 ` Chanwoo Choi
2019-09-24 19:37 ` Matthias Kaehlcke
2019-09-25 1:56 ` Chanwoo Choi
2019-09-25 17:45 ` Matthias Kaehlcke
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).