Re: [PATCH 14/15] MIPS: csrc-sb1250: Implement read_sched_clock

[Deng-Cheng Zhu <dengcheng.zhu@imgtec.com>]

Thank you Maciej for the review!


On 03/06/2015 04:47 AM, Maciej W. Rozycki wrote:
> On Wed, 4 Mar 2015, Deng-Cheng Zhu wrote:
>
>> Use sb1250 hpt for sched_clock source. This implementation will give high
>> resolution cputime accounting.
>>
>> Signed-off-by: Deng-Cheng Zhu <dengcheng.zhu@imgtec.com>
>> ---
>>   arch/mips/kernel/csrc-sb1250.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/arch/mips/kernel/csrc-sb1250.c b/arch/mips/kernel/csrc-sb1250.c
>> index df84836..6546fff 100644
>> --- a/arch/mips/kernel/csrc-sb1250.c
>> +++ b/arch/mips/kernel/csrc-sb1250.c
>> @@ -12,6 +12,7 @@
>>    * GNU General Public License for more details.
>>    */
>>   #include <linux/clocksource.h>
>> +#include <linux/sched_clock.h>
>>   
>>   #include <asm/addrspace.h>
>>   #include <asm/io.h>
>> @@ -46,6 +47,11 @@ struct clocksource bcm1250_clocksource = {
>>   	.flags	= CLOCK_SOURCE_IS_CONTINUOUS,
>>   };
>>   
>> +static u64 notrace sb1250_read_sched_clock(void)
>> +{
>> +	return sb1250_hpt_read(NULL);
>> +}
>> +
>   I think you're abusing the API of `sb1250_hpt_read' here, by relying on
> the implementation not using its `cs' argument.
>
>   I think it would make sense to reverse the implementation, by calling
> `sb1250_read_sched_clock' from `sb1250_hpt_read' instead.  Or perhaps
> better yet use a static inline helper for both, so as to avoid the extra
> tail call and the associated performance hit.
>
>    Maciej

Good point. Will do in v2.


Deng-Cheng