Re: [PATCH 3/3] ia64: Call migration code on correctable errors v2

Pekka J Enberg
2008-05-02T16:57:44Z

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