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

Huang Ying
2009-04-24T05:51:22Z

[-- 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 --]