lkml.org 
[lkml]   [2009]   [Jun]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH -v4] x86: MCE: Re-implement MCE log ring buffer as per-CPU ring buffer
    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.

    > @@ -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);"?


    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;


    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.


    Thanks,
    H.Seto



    \
     
     \ /
      Last update: 2009-06-05 09:05    [W:0.028 / U:0.492 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site