Re: [patch 2/2] x86: mce: Implement cmci poll mode for intel machines

[Chen Gong]

ä 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/