Re: [PATCH v2] devfreq: Add tracepoint for frequency changes

[Chanwoo Choi]

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@xxxxxxxxxxxx>
>>> ---
>>> (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