Messages in this thread | | | Subject | Re: [PATCH -v4] x86: MCE: Re-implement MCE log ring buffer as per-CPU ring buffer | From | Huang Ying <> | Date | Fri, 05 Jun 2009 16:08:04 +0800 |
| |
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
| |