linux-kernel - Re: [PATCH] clocksource_cyc2ns: avoid overflowing 64 bits

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 16 Nov 2016 11:40:46 -0800
From:   John Stultz <john.stultz@...aro.org>
To:     Chris Metcalf <cmetcalf@...lanox.com>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Salman Qazi <sqazi@...gle.com>, Paul Turner <pjt@...gle.com>,
        Tony Lindgren <tony@...mide.com>,
        Steven Miao <realmz6@...il.com>,
        lkml <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] clocksource_cyc2ns: avoid overflowing 64 bits

On Wed, Nov 16, 2016 at 11:30 AM, Chris Metcalf <cmetcalf@...lanox.com> wrote:
> On 11/16/2016 1:04 PM, John Stultz wrote:
>>
>> On Wed, Nov 16, 2016 at 8:57 AM, Chris Metcalf <cmetcalf@...lanox.com>
>> 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