Re: Re-implement MCE log ring buffer as per-CPU ring buffer

[Huang Ying <ying.huang@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3447 bytes --]

On Wed, 2009-04-22 at 19:11 +0800, Andi Kleen wrote:
> Huang Ying wrote:
> 
> Basic concept is good and it fixes some long standing problems.
> 
> But some details need to be improved I think:
> 
> > +/*
> > + * Initialize MCE per-CPU log buffer
> > + */
> > +static void mce_log_init(void)
> > +{
> > +	int cpu;
> > +	struct mce_log_cpu *mcelog_cpu;
> > +
> > +	if (mcelog.log_cpus)
> > +		return;
> > +	mcelog.log_cpus = kmalloc(num_possible_cpus() * sizeof(mcelog_cpu),
> > +				  GFP_KERNEL);
> > +	for_each_possible_cpu(cpu) {
> > +		mcelog_cpu = &per_cpu(mcelog_cpus, cpu);
> > +		mcelog.log_cpus[cpu] = mcelog_cpu;
> > +	}
> 
> I don't understand why the separate allocation. Can't you just put
> the whole log buffer directly into the per cpu data?
> 
> This would also make the initialization cleaner because you would need to only
> initialize state for the currently initialized CPU.

The original intention to use mcelog.log_cpus is to make it easier to
find per-cpu buffers from memory image produced by kdump. But is seems
that something as follow is just sufficient.

mcelog.log_cpus = alloc_percpu(struct mce_log_cpu);

Is this what you suggested?

> 
> > +			while (!mcelog_cpu->entry[i].finished) {
> > +				rdtscll(now);
> > +				if (now - start > WRITER_TIMEOUT_CYC) {
> > +					memset(mcelog_cpu->entry + i, 0,
> >  					       sizeof(struct mce));
> > +					head = mcelog_cpu->head;
> >  					goto timeout;
> 
> This timeout should be reported somehow, perhaps with a printk.
> Also it's problematic to hardcode cycles here, i think it would
> be better to use a similar scheme as the Monarch timeout
> with a ndelay() and a spinunit. That is guaranteed to stay
> the same even as CPU frequencies change.

Yes. I will change this.

> > +static ssize_t mce_read(struct file *filp, char __user *inubuf, size_t usize,
> > +			loff_t *off)
> > +{
> > +	char __user *ubuf = inubuf;
> > +	struct mce_log_cpu *mcelog_cpu;
> > +	int cpu, new_mce, err = 0;
> > +	static DEFINE_MUTEX(mce_read_mutex);
> > +	size_t usize_limit;
> > +
> > +	/* Too large user buffer size may cause system not response */
> 
> Did you understand why that happens? It's worrying.

The loop in mce_read() does not release the CPU in non-preempt kernel.
If the user buffer size is too large, it may cause the CPU to be
soft-lock-upped or not response. One way to solve the issue is to
constrain the size of user buffer, another way may be insert a
schedule() in the loop. 

> > +static ssize_t show_log_flags(struct sys_device *s,
> > +			      struct sysdev_attribute *attr,
> > +			      char *buf)
> > +{
> > +	int cpu = s->id;
> > +	struct mce_log_cpu *mcelog_cpu = &per_cpu(mcelog_cpus, cpu);
> > +	unsigned flags;
> > +	do {
> > +		flags = mcelog_cpu->flags;
> > +	} while (cmpxchg((unsigned *)&mcelog_cpu->flags, flags, 0) != flags);
> > +	return sprintf(buf, "0x%x\n", flags);
> > +}
> > +
> >  static SYSDEV_ATTR(trigger, 0644, show_trigger, set_trigger);
> >  static SYSDEV_INT_ATTR(tolerant, 0644, tolerant);
> >  ACCESSOR(check_interval,check_interval,mce_restart())
> > +static SYSDEV_ATTR(log_flags, 0644, show_log_flags, NULL);
> 
> Do we really need that interface? It seems rather obscure.
> mcelog should get these flags anyways. Better to drop it.

For overflow flag, it seems not very useful. I will drop this.

Best Regards,
Huang Ying


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]