lkml.org 
[lkml]   [2008]   [May]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 3/3] ia64: Call migration code on correctable errors v2
    Hi Russ,

    On Fri, 2 May 2008, Russ Anderson wrote:
    > > I think sparse and checkpatch would have caught most of these but here goes:
    >
    > I did run checkpatch.pl. The two warnings were a false positive and
    > the other I let slide. I'll make the rest of your suggested changes.
    >
    > WARNING: consider using strict_strtoul in preference to simple_strtoul
    > #340: FILE: arch/ia64/kernel/cpe_migrate.c:301:
    > + opt = simple_strtoul(optstr, NULL, 0);

    Why do you think this is a false positive? We just converted SLUB over to
    use strict_stroul() as suggested by Andrew.

    > > > + if (cpe_paddr[cpe_head] == 0) {
    > > > + cpe_paddr[cpe_head] = paddr;
    > > > + cpe_node[cpe_head] = node;
    > > > +
    > > > + if (++cpe_head >= CE_HISTORY_LENGTH)
    > > > + cpe_head = 0;
    > > > + }
    > > > +
    > > > + if (!work_scheduled) {
    > > > + work_scheduled = 1;
    > > > + schedule_work(&cpe_enable_work);
    > >
    > > So you must not schedule cpe_enable_work if it's already in progress. Why?
    >
    > If there is already a worker thread scheduled, it will process all
    > the addresses on the queue, including new entries. So all ce_setup_migrate()
    > needs to do is add the new entry to the queue. The CPE interrupt can come in faster
    > than the worker thread can migrate the pages. Scheduling another worker
    > thread on each CPE interrupt when there is already one scheduled/running
    > would be overkill.

    Okay. Maybe a kthread would be cleaner here then (which sleeps when the
    buffer is empty)? I didn't notice any locking for cpe_paddr and cpe_node.
    Why is that?

    > > > + proc_badpage = create_proc_entry(BADRAM_BASENAME, 0644, NULL);
    > >
    > > Why is this file not in sysfs?
    >
    > /proc seemed like a appropriate place. Where would you suggest in sysfs?

    Oh, /proc is for _process specific_ files although historically it has
    been (ab)used for other files as well. I think something like
    /sys/kernel/badram would be appropriate. Christoph, Greg?

    Pekka


    \
     
     \ /
      Last update: 2008-05-02 19:01    [W:2.290 / U:0.724 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site