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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 2 May 2008 19:57:44 +0300 (EEST)
From:	Pekka J Enberg <penberg@...helsinki.fi>
To:	Russ Anderson <rja@....com>
cc:	linux-kernel@...r.kernel.org, linux-ia64@...r.kernel.org,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Tony Luck <tony.luck@...el.com>,
	Christoph Lameter <clameter@....com>, gregkh@...e.de
Subject: Re: [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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/