Re: [PATCH -v2] x86: MCE: Re-implement MCE log ring buffer asper-CPU ring buffer

[Huang Ying]

On Tue, 2009-04-28 at 18:21 +0800, Andi Kleen wrote:
> Huang Ying <ying.huang@xxxxxxxxx> writes:
> >
> > ChangeLog:
> >
> > v2:
> >
> > - Use alloc_percpu() to allocate per_cpu mcelog buffer
> 
> Sorry, why didn't you just use DEFINE_PER_CPU ? That should work
> as well and will be shorter.

OK. I can do that.

> Another thing I noticed. the "MACHINECHECK" signature was originally
> for crash dump tools to find the log. If you change the format
> you should change it to MACHINECHEC2 or so.

Oh, thanks, I will change this.

> > +	size_t usize_limit;
> > +
> > +	/* Too large user buffer size may cause system not response */
> > +	usize_limit = num_possible_cpus() * MCE_LOG_LEN * sizeof(struct mce);
> > +	if (usize > usize_limit)
> > +		usize = usize_limit;
> 
> Did you ever track down what happens here? I still find it worrying

Maybe the comment is a little confusing. What we do is limit the size of
user buffer size to prevent system from not response. If the user buffer
size is limited, the time taken by mce_read() can be reasonable, and
will not cause system not to response.

But maybe num_possible_cpus() * MCE_LOG_LEN is not a good upper limit,
because possible cpus can be fairly large. Maybe we can add another
limit, something as follow:

#define MCE_READ_RECORD_LIMIT	512

/*
 * Too large user buffer size may cause system not response, so constrain
 * the effective size of user buffer
 */
usize_limit = num_possible_cpus() * MCE_LOG_LEN * sizeof(struct mce);
if (usize_limit > MCE_RECOARD_LIMIT * sizeof(struct mce))
	usize_limit = MCE_RECOARD_LIMIT * sizeof(struct mce);
if (usize > usize_limit)
	usize = usize_limit;

Best Regards,
Huang Ying

Attachment: signature.asc
Description: This is a digitally signed message part