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

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

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