lkml.org 
[lkml]   [2012]   [Jun]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [patch 2/2] x86: mce: Implement cmci poll mode for intel machines
    于 2012/6/5 4:01, Thomas Gleixner 写道:
    > On Mon, 4 Jun 2012, Chen Gong wrote:
    >>> +/*
    >>> + * Ensure that the timer is firing in @interval from now.
    >>> + */
    >>> +void mce_timer_kick(unsigned long interval)
    >>> +{
    >>> + struct timer_list *t = &__get_cpu_var(mce_timer);
    >>> + unsigned long when = jiffies + interval;
    >>> + unsigned long iv = __this_cpu_read(mce_next_interval);
    >>> +
    >>> + if (time_before(when, t->expires) && timer_pending(t)) {
    >>> + mod_timer(t, when);
    >>> + } else if (!mce_next_interval) {
    >>
    >> Why using mce_next_interval, it is a per_cpu var, should be non-NULL
    >> definitely, right? How about using *iv* here?
    >
    > iv is the thing to use. No idea why I typoed mce_next_interval into
    > that.
    >
    >>> + t->expires = round_jiffies(jiffies + iv);
    >>> + add_timer_on(t, smp_processor_id());
    >>> + }
    >>> + if (interval < iv)
    >>> + __this_cpu_write(mce_next_interval, iv);
    >>> }
    >>
    >> This code should be __this_cpu_write(mce_next_interval, interval);?
    >
    > Duh, yes.
    >
    > Thanks,
    >
    > tglx
    >
    Hi, Thomas

    Besides above issues, I still have some other questions as below:

    > static void mce_timer_fn(unsigned long data)
    > {
    > ...
    > + /* Might have become 0 after CMCI storm subsided */
    > + if (iv) {
    > + t->expires = jiffies + iv;
    > + add_timer_on(t, smp_processor_id());
    > + }
    > +}

    I've found under some conditions, *t* is pending on the timer tree, so
    add_timer_on will crash the whole system. Furthermore, if this timer
    function triggers "WARN_ON(smp_processor_id() != data);", this timer
    will be added on the other CPU, which means it loses the chance to
    decrement *cmci_storm_on_cpus* to zero to reenable the CMCI. Maybe
    this situation happens seldomly, but once it happens, CMCI will never
    be actived again after it is disabled.


    > +void mce_timer_kick(unsigned long interval)
    > +{
    > + struct timer_list *t = &__get_cpu_var(mce_timer);
    > + unsigned long when = jiffies + interval;
    > + unsigned long iv = __this_cpu_read(mce_next_interval);
    > +
    > + if (time_before(when, t->expires) && timer_pending(t)) {
    > + mod_timer(t, when);
    > + } else if (!mce_next_interval) {
    > + t->expires = round_jiffies(jiffies + iv);
    > + add_timer_on(t, smp_processor_id());

    I've changed "else if (!mce_next_interval)" to "else if (iv)", but
    I still think it is not right. Imaging *when* is after t->expires and
    this timer is pending on the timer tree, so it will hit *else if*
    if iv is not zero(common situations), again, add_timer_on will trigger
    BUG_ON because this timer is pending.


    > static void intel_threshold_interrupt(void)
    > {
    > + if (cmci_storm_detect())
    > + return;
    > machine_check_poll(MCP_TIMESTAMP, &__get_cpu_var(mce_banks_owned));
    > mce_notify_irq();
    > }

    I think cmci_storm_detect should be placed in the machine_check_poll,
    not out of it. Because machine_check_poll it the core execution logic
    for CMCI handling, in the meanwhile, poll timer and mce-inject module
    call machine_check_poll at any time. If poll timer or mce-inject run
    too quickly, the CMCI handler has trouble. Whereas, if
    cmci_storm_detect is in the machine_check_poll, this kind of possibility
    can be avoid.
    --
    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/

    \
     
     \ /
      Last update: 2012-06-05 14:21    [W:4.032 / U:0.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site