Re: [git pull] timer fix

[Ray Lee <ray-lk@madrabbit.org>]

On Thu, Feb 5, 2009 at 1:58 AM, Pavel Emelyanov <xemul@openvz.org> wrote:
> Kirill Korotaev wrote:
>> ACK, it works. Why not save another 4 bytes of .bss then by changing
>> hept_t1_cmp to u32? ;-)
>
> Hm... .bss you say? :) The bloat-o-meter results would be:
>
> For the original fix with casting hpet_t1_cmp to u32 inside the loop
> add/remove: 0/0 grow/shrink: 1/0 up/down: 2/0 (2)
> function                                     old     new   delta
> hpet_rtc_interrupt                           741     743      +2
>
> For the fix with casting the substitution result to s32
> add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-2 (-2)
> function                                     old     new   delta
> hpet_rtc_interrupt                           741     739      -2
>
> For the proposed by Kirill type change of the hpet_t1_cmp
> add/remove: 0/0 grow/shrink: 0/3 up/down: 0/-6 (-6)
> function                                     old     new   delta
> hpet_rtc_timer_init                          186     185      -1
> hpet_rtc_interrupt                           741     740      -1
> hpet_t1_cmp                                    8       4      -4
>
> That's the fix:
>
> diff --git a/arch/x86/kernel/hpet.c b/arch/x86/kernel/hpet.c
> index 64d5ad0..dfbbb94 100644
> --- a/arch/x86/kernel/hpet.c
> +++ b/arch/x86/kernel/hpet.c
> @@ -897,7 +897,7 @@ static unsigned long hpet_rtc_flags;
>  static int hpet_prev_update_sec;
>  static struct rtc_time hpet_alarm_time;
>  static unsigned long hpet_pie_count;
> -static unsigned long hpet_t1_cmp;
> +static u32 hpet_t1_cmp;
>  static unsigned long hpet_default_delta;
>  static unsigned long hpet_pie_delta;
>  static unsigned long hpet_pie_limit;
>
> Reported-by: Kirill Korotaev <dev@openvz.org>
> Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
>
> This one also works. Should I re-send this patch in a proper way?

Ugh. This version seems too subtle for a new reader coming into this
code. I'd prefer the fix be kept in the comparison site, where people
already expect wraparound issues.

If you don't have time to add the helper, I'd really rather see the
version with the explicit cast acting as documentation, rather than a
test that magically works due to type issues.

Dunno, maybe it's just me.