Re: [PATCH v2] devfreq: Add tracepoint for frequency changes
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 > >>> --- > >>> (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.
Re: [PATCH v2] devfreq: Add tracepoint for frequency changes
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
>>> ---
>>> (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
>>> #include
>>>
>>> +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(>dev))
>>> + __field(unsigned long, freq)
>>> + ),
>>> +
>>> + TP_fast_assign(
>>> + __assign_str(dev_name, dev_name(>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
Re: [PATCH v2] devfreq: Add tracepoint for frequency changes
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
> > ---
> > (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
> > #include
> >
> > +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(>dev))
> > + __field(unsigned long, freq)
> > + ),
> > +
> > + TP_fast_assign(
> > + __assign_str(dev_name, dev_name(>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
Re: [PATCH v2] devfreq: Add tracepoint for frequency changes
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
> ---
> (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
> #include
>
> +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(>dev))
> + __field(unsigned long, freq)
> + ),
> +
> + TP_fast_assign(
> + __assign_str(dev_name, dev_name(>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
Re: [PATCH v2] devfreq: Add tracepoint for frequency changes
On Thu, 19 Sep 2019 10:44:23 -0700 Matthias Kaehlcke wrote: > Add a tracepoint for frequency changes of devfreq devices and > use it. > > Signed-off-by: Matthias Kaehlcke >From the tracing point of view: Reviewed-by: Steven Rostedt (VMware) -- Steve
