Re: [PATCH v2 1/3] MIPS: add a common mips_cyc2ns()

[David Daney <ddaney@caviumnetworks.com>]

On 04/07/2010 09:05 AM, Wu Zhangin wrote:
> From: Wu Zhangjin<wuzhangjin@gmail.com>
>
> Changes:
>
> v1 ->  v2:
>
>    o change the old mips_sched_clock() to mips_cyc2ns() and modify the
>    arguments to support 32bit.
>    o add 32bit support: use a smaller shift to avoid the quick overflow
>    of 64bit arithmatic and balance the overhead of the 128bit arithmatic
>    and the precision lost with the smaller shift.
>
> ----------------------
>
> Because the high resolution sched_clock() for r4k has the same overflow
> problem and solution mentioned in "MIPS: Octeon: Use non-overflowing
> arithmetic in sched_clock".
>
>      "With typical mult and shift values, the calculation for Octeon's
>      sched_clock overflows when using 64-bit arithmetic.  Use 128-bit
>      calculations instead."
>
> To reduce the duplication, This patch abstracts the solution into an
> inline funciton mips_cyc2ns() into arch/mips/include/asm/time.h from
> arch/mips/cavium-octeon/csrc-octeon.c.
>
> Two patches for Cavium and R4K will be sent out respectively to use this
> common function.
>
> Signed-off-by: Wu Zhangjin<wuzhangjin@gmail.com>
> ---
>   arch/mips/include/asm/time.h |   38 ++++++++++++++++++++++++++++++++++++++
>   1 files changed, 38 insertions(+), 0 deletions(-)
>
> diff --git a/arch/mips/include/asm/time.h b/arch/mips/include/asm/time.h
> index c7f1bfe..898f0e0 100644
> --- a/arch/mips/include/asm/time.h
> +++ b/arch/mips/include/asm/time.h
> @@ -96,4 +96,42 @@ static inline void clockevent_set_clock(struct clock_event_device *cd,
>   	clockevents_calc_mult_shift(cd, clock, 4);
>   }
>
> +static inline unsigned long long mips_cyc2ns(u64 cyc, u64 mult, u64 shift)
> +{
> +#ifdef CONFIG_32BIT
> +	/*
> +	 * To balance the overhead of 128bit-arithematic and the precision
> +	 * lost, we choose a smaller shift to avoid the quick overflow as the
> +	 * X86&  ARM does. please refer to arch/x86/kernel/tsc.c and
> +	 * arch/arm/plat-orion/time.c
> +	 */
> +	return (cyc * mult)>>  shift;

Have you tested that on a 32-bit kernel?  I think it may overflow for 
many cases.

David Daney