[PATCH -tip] x86: apic LVTT - use APIC_DIVISOR on 64bit mode - Kernel

This is a discussion on [PATCH -tip] x86: apic LVTT - use APIC_DIVISOR on 64bit mode - Kernel ; We use APIC_TDR_DIV_16 while setting APIC_TDCR (divisor) it happened this moment is hidden from __setup_APIC_LVTT caller. So we better increment caller 'clocks' value to not change current behaviour and use APIC_DIVISOR (as already done in 32bit version). The main benefit ...

+ Reply to Thread
Results 1 to 6 of 6

Thread: [PATCH -tip] x86: apic LVTT - use APIC_DIVISOR on 64bit mode

  1. [PATCH -tip] x86: apic LVTT - use APIC_DIVISOR on 64bit mode

    We use APIC_TDR_DIV_16 while setting APIC_TDCR (divisor)
    it happened this moment is hidden from __setup_APIC_LVTT
    caller. So we better increment caller 'clocks' value to
    not change current behaviour and use APIC_DIVISOR
    (as already done in 32bit version).

    The main benefit - unified procedure code for 32/64bit modes.

    Signed-off-by: Cyrill Gorcunov
    CC: "Maciej W. Rozycki"
    ---

    Please review carefully. Any comments are welcome.
    I'm _not_ 100% sure if this patch is safe.

    Index: linux-2.6.git/arch/x86/kernel/apic_64.c
    ================================================== =================
    --- linux-2.6.git.orig/arch/x86/kernel/apic_64.c 2008-07-08 22:52:24.000000000 +0400
    +++ linux-2.6.git/arch/x86/kernel/apic_64.c 2008-07-08 23:00:24.000000000 +0400
    @@ -165,6 +165,9 @@ int lapic_get_maxlvt(void)
    return maxlvt;
    }

    +/* Clock divisor is set to 16 */
    +#define APIC_DIVISOR 16
    +
    /*
    * This function sets up the local APIC timer, with a timeout of
    * 'clocks' APIC bus clock. During calibration we actually call
    @@ -197,7 +200,7 @@ static void __setup_APIC_LVTT(unsigned i
    | APIC_TDR_DIV_16);

    if (!oneshot)
    - apic_write(APIC_TMICT, clocks);
    + apic_write(APIC_TMICT, clocks/APIC_DIVISOR);
    }

    /*
    @@ -329,7 +332,7 @@ static void __init calibrate_APIC_clock(
    *
    * No interrupt enable !
    */
    - __setup_APIC_LVTT(250000000, 0, 0);
    + __setup_APIC_LVTT(4000000000, 0, 0);

    apic_start = apic_read(APIC_TMCCT);
    #ifdef CONFIG_X86_PM_TIMER
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  2. Re: [PATCH -tip] x86: apic LVTT - use APIC_DIVISOR on 64bit mode


    * Cyrill Gorcunov wrote:

    > We use APIC_TDR_DIV_16 while setting APIC_TDCR (divisor) it happened
    > this moment is hidden from __setup_APIC_LVTT caller. So we better
    > increment caller 'clocks' value to not change current behaviour and
    > use APIC_DIVISOR (as already done in 32bit version).
    >
    > The main benefit - unified procedure code for 32/64bit modes.


    > Please review carefully. Any comments are welcome.
    > I'm _not_ 100% sure if this patch is safe.


    hm, how about the other caller to __setup_APIC_LVTT(),
    lapic_timer_setup():

    case CLOCK_EVT_MODE_ONESHOT:
    __setup_APIC_LVTT(calibration_result,
    mode != CLOCK_EVT_MODE_PERIODIC, 1);
    break;

    that will affect high-res timers. Wont calibration_result have the same
    value after your patch as well, but only a 16th of it will be used in
    __setup_APIC_LVTT(), causing 16 times shorter timeouts than intended?

    I.e. are we fixing a bug, are we changing nothing, or are we introducing
    a bug? :-)

    Ingo
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  3. Re: [PATCH -tip] x86: apic LVTT - use APIC_DIVISOR on 64bit mode

    [Ingo Molnar - Sat, Jul 12, 2008 at 08:05:04AM +0200]
    |
    | * Cyrill Gorcunov wrote:
    |
    | > We use APIC_TDR_DIV_16 while setting APIC_TDCR (divisor) it happened
    | > this moment is hidden from __setup_APIC_LVTT caller. So we better
    | > increment caller 'clocks' value to not change current behaviour and
    | > use APIC_DIVISOR (as already done in 32bit version).
    | >
    | > The main benefit - unified procedure code for 32/64bit modes.
    |
    | > Please review carefully. Any comments are welcome.
    | > I'm _not_ 100% sure if this patch is safe.
    |
    | hm, how about the other caller to __setup_APIC_LVTT(),
    | lapic_timer_setup():
    |
    | case CLOCK_EVT_MODE_ONESHOT:
    | __setup_APIC_LVTT(calibration_result,
    | mode != CLOCK_EVT_MODE_PERIODIC, 1);
    | break;
    |
    | that will affect high-res timers. Wont calibration_result have the same
    | value after your patch as well, but only a 16th of it will be used in
    | __setup_APIC_LVTT(), causing 16 times shorter timeouts than intended?
    |
    | I.e. are we fixing a bug, are we changing nothing, or are we introducing
    | a bug? :-)
    |
    | Ingo
    |

    well, the last I would say . That happened I sent first version of
    patch instead of modified second version. Thanks a lot Ingo for catching
    it! Oh my, sorry... Here is an updated version.
    ---

    We use APIC_TDR_DIV_16 while setting APIC_TDCR (divisor).
    It happened this moment is hidden from __setup_APIC_LVTT
    caller. So we better increment caller 'clocks' value to
    not change current behaviour and use APIC_DIVISOR
    (as already done in 32bit version).

    The main benefit - unified procedure code for 32/64bit modes.

    Signed-off-by: Cyrill Gorcunov
    CC: "Maciej W. Rozycki"
    ---

    Index: linux-2.6.git/arch/x86/kernel/apic_64.c
    ================================================== =================
    --- linux-2.6.git.orig/arch/x86/kernel/apic_64.c 2008-07-12 11:11:33.000000000 +0400
    +++ linux-2.6.git/arch/x86/kernel/apic_64.c 2008-07-12 11:14:22.000000000 +0400
    @@ -165,6 +165,9 @@ int lapic_get_maxlvt(void)
    return maxlvt;
    }

    +/* Clock divisor is set to 16 */
    +#define APIC_DIVISOR 16
    +
    /*
    * This function sets up the local APIC timer, with a timeout of
    * 'clocks' APIC bus clock. During calibration we actually call
    @@ -197,7 +200,7 @@ static void __setup_APIC_LVTT(unsigned i
    | APIC_TDR_DIV_16);

    if (!oneshot)
    - apic_write(APIC_TMICT, clocks);
    + apic_write(APIC_TMICT, clocks/APIC_DIVISOR);
    }

    /*
    @@ -329,7 +332,7 @@ static void __init calibrate_APIC_clock(
    *
    * No interrupt enable !
    */
    - __setup_APIC_LVTT(250000000, 0, 0);
    + __setup_APIC_LVTT(4000000000, 0, 0);

    apic_start = apic_read(APIC_TMCCT);
    #ifdef CONFIG_X86_PM_TIMER
    @@ -367,7 +370,7 @@ static void __init calibrate_APIC_clock(
    lapic_clockevent.min_delta_ns =
    clockevent_delta2ns(0xF, &lapic_clockevent);

    - calibration_result = result / HZ;
    + calibration_result = result * APIC_DIVISOR / HZ;
    }

    /*
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  4. Re: [PATCH -tip] x86: apic LVTT - use APIC_DIVISOR on 64bit mode


    * Cyrill Gorcunov wrote:

    > @@ -329,7 +332,7 @@ static void __init calibrate_APIC_clock(
    > *
    > * No interrupt enable !
    > */
    > - __setup_APIC_LVTT(250000000, 0, 0);
    > + __setup_APIC_LVTT(4000000000, 0, 0);


    note how close it is to 2^32. For this to be unifiable later on this
    needs to be UL i guess, and this:

    > - calibration_result = result / HZ;
    > + calibration_result = result * APIC_DIVISOR / HZ;


    might overflow 32 bits.

    Ingo
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  5. Re: [PATCH -tip] x86: apic LVTT - use APIC_DIVISOR on 64bit mode


    * Ingo Molnar wrote:

    >
    > * Cyrill Gorcunov wrote:
    >
    > > We use APIC_TDR_DIV_16 while setting APIC_TDCR (divisor) it happened
    > > this moment is hidden from __setup_APIC_LVTT caller. So we better
    > > increment caller 'clocks' value to not change current behaviour and
    > > use APIC_DIVISOR (as already done in 32bit version).
    > >
    > > The main benefit - unified procedure code for 32/64bit modes.

    >
    > > Please review carefully. Any comments are welcome.
    > > I'm _not_ 100% sure if this patch is safe.

    >
    > hm, how about the other caller to __setup_APIC_LVTT(),
    > lapic_timer_setup():
    >
    > case CLOCK_EVT_MODE_ONESHOT:
    > __setup_APIC_LVTT(calibration_result,
    > mode != CLOCK_EVT_MODE_PERIODIC, 1);
    > break;
    >
    > that will affect high-res timers. Wont calibration_result have the same
    > value after your patch as well, but only a 16th of it will be used in
    > __setup_APIC_LVTT(), causing 16 times shorter timeouts than intended?
    >
    > I.e. are we fixing a bug, are we changing nothing, or are we
    > introducing a bug? :-)


    i think we are introducing a bug

    Ingo
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  6. Re: [PATCH -tip] x86: apic LVTT - use APIC_DIVISOR on 64bit mode

    [Ingo Molnar - Sat, Jul 12, 2008 at 09:39:10AM +0200]
    |
    | * Cyrill Gorcunov wrote:
    |
    | > @@ -329,7 +332,7 @@ static void __init calibrate_APIC_clock(
    | > *
    | > * No interrupt enable !
    | > */
    | > - __setup_APIC_LVTT(250000000, 0, 0);
    | > + __setup_APIC_LVTT(4000000000, 0, 0);
    |
    | note how close it is to 2^32. For this to be unifiable later on this
    | needs to be UL i guess, and this:
    |
    | > - calibration_result = result / HZ;
    | > + calibration_result = result * APIC_DIVISOR / HZ;
    |
    | might overflow 32 bits.
    |
    | Ingo
    |

    hmm... I need more time for analisys, thanks. Drop this patch
    please

    - Cyrill -
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread