Re: [PATCH -v4] x86: MCE: Re-implement MCE log ring buffer as per-CPU ring buffer

Huang Ying
2009-06-05T08:27:43Z

On Fri, 2009-06-05 at 15:01 +0800, Hidetoshi Seto wrote:
> Huang Ying wrote:
> > Re-implement MCE log ring buffer as per-CPU ring buffer for better
> > scalability. Basic design is as follow:
>
> That would be great. This should be in .31, I think.
>
> Some minor points:
>
> > struct mce_log {
> > - char signature[12]; /* "MACHINECHECK" */
> > + char signature[12]; /* "MACHINECHEC2" */
> > unsigned len; /* = MCE_LOG_LEN */
> > - unsigned next;
> > unsigned flags;
> > unsigned pad0;
> > - struct mce entry[MCE_LOG_LEN];
> > + struct mce_log_cpu *mcelog_cpus;
> > };
>
> What is this *mcelog_cpus to be used for?
> It seems it will point one of per-CPU buffers (maybe cpu#0's buffer)
> if I have read the following mce_log_init() correctly.

It is mainly used by something like kdump, which can search
"MACHINECHEC2", and analyze mce_log. mcelog_cpus can help kdump find the
real mcelog storage.

> > @@ -642,6 +669,16 @@ static int mce_cap_init(void)
> > return 0;
> > }
> >
> > +/*
> > + * Initialize MCE per-CPU log buffer
> > + */
> > +static __cpuinit void mce_log_init(void)
> > +{
> > + if (mcelog.mcelog_cpus)
> > + return;
> > + mcelog.mcelog_cpus = &per_cpu_var(mce_log_cpus);
> > +}
> > +
> > static void mce_init(void)
> > {
> > mce_banks_t all_banks;
>
>
> Next,
>
> > +static int mce_empty_cpu(struct mce_log_cpu *mcelog_cpu)
> > +{
> > + int head, tail;
> > + head = mcelog_cpu->head;
> > + tail = mcelog_cpu->tail;
> > + return head == tail;
> > +}
>
> Are there any race condition?
> Why we cannot have "return (mcelog_cpu->head == mcelog_cpu->tail);"?

Yes, it is better, I will change this.

> Last,
>
> > +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);
> > +
> (snip)
> > + return err ? : ubuf - inubuf;
> > }
>
> It would be better to put "static DEFINE_MUTEX(mce_read_mutex)" to outside of
> the function.
>
> And it looks work, but I'd prefer:
> return err ? err : ubuf - inubuf;

OK, I will change this.

> Overall it looks good.
> Unfortunately there were updates on tip/mce3 so I could not apply this
> patch on the top of the branch. I'd appreciate it if you could rebase
> this patch again.

Yes. I will re-base it.

Best Regards,
Huang Ying