Messages in this thread | | | Date | Tue, 05 Jun 2012 19:47:20 +0800 | From | Chen Gong <> | Subject | Re: [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/
| |