Re: [PATCH] clocksource_cyc2ns: avoid overflowing 64 bits

[John Stultz]

On Wed, Nov 16, 2016 at 11:30 AM, Chris Metcalf <cmetcalf@xxxxxxxxxxxx> wrote:
> On 11/16/2016 1:04 PM, John Stultz wrote:
>>
>> On Wed, Nov 16, 2016 at 8:57 AM, Chris Metcalf <cmetcalf@xxxxxxxxxxxx>
>> wrote:
>>>
>>>   include/linux/clocksource.h | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
>>> index 08398182f56e..b2a022acf232 100644
>>> --- a/include/linux/clocksource.h
>>> +++ b/include/linux/clocksource.h
>>> @@ -175,7 +175,7 @@ static inline u32 clocksource_hz2mult(u32 hz, u32
>>> shift_constant)
>>>    */
>>>   static inline s64 clocksource_cyc2ns(cycle_t cycles, u32 mult, u32
>>> shift)
>>>   {
>>> -       return ((u64) cycles * mult) >> shift;
>>> +       return mult_frac(cycles, mult, 1ULL << shift);
>>>   }
>>
>>
>> So clocksource_cyc2ns() was never intended to be used with
>> indefinitely large cycle values, and it looks like tile and blackfin
>> are abusing the interface (the omap usage provide cycle deltas rather
>> then just the current counter value).
>
>
> Well, the interface does just say "convert clocksource cycles to
> nanoseconds". :-)

Right, and I can understand the confusion, but its not being used with
a struct clocksource. Its just being used to convert get_cycles().

> If you think it's more important that it be a little faster, we should
> adjust the
> documentation to say it is only appropriate for delta-cycles, not absolute
> cycles.
> I've appended a commit that does this if you'd like to take it.

That's fair. Thanks for sending that, II'll queue that in my tree here
in a moment.

>> I'd suggest instead to move tile/blackfin to using the generic
>> sched_clock implementation that most of the architectures use, or
>> special case the code in the arch specific sched_clock
>> implementations(as x86 does) instead of modifying the common interface
>> to better handle a use case its not intended for.
>
>
> Yes, since tile has a full 64-bit cycle counter, the best thing is to just
> directly
> open-code the mult_frac() in tile's sched_clock().  I'll push that change.
> Steven Miao, I assume you should do the same for blackfin.

thanks
-john