Re: Dirty page tracking patch

[Arjan van de Ven <arjan@infradead.org>]

On Mon, 2005-07-11 at 09:16 -0400, Kimball Murray wrote:
> Hello to all.
> 
> 	On behalf of Stratus Technologies (www.stratus.com) I'd like to
> present a patch to the i386 kernel code that will allow developers to track
> dirty memory pages.  Stratus uses this technique to facilitate bringing
> separate cpu and memory module "nodes" into lockstep with each other, with
> a minimum of OS down time. This feature could also be used to provide inputs
> into memory management algorithms, or to support hot-plug memory dimm modules
> for specially developed hardware.
> 

is the stratus code entirely open source/GPL ? (I assume it is since you
EXPORT_SYMBOL_GPL and also use other similar stuff). If so.. could you
post an URL to that? It's customary to do so when you post interface
patches for review so that the users of the interfaces can be seen, and
thus the interface better reviewed.


Also this patch is just plain weird/really corner case...

+#define mm_track(ptep)

you have to make that a do { ; } while (0) define as per kernel
convention/need

also you now make set_pte() and co more than trivial assignments, please
convert them to inlines so that typechecking is performed and no double
evaluation of arguments is done!
(this is a real problem for code that would do set_pte(pte++, value) in
a loop or so)


-       if (!pte_dirty(*ptep))
-               return 0;
-       return test_and_clear_bit(_PAGE_BIT_DIRTY, &ptep->pte_low);
+       mm_track(ptep);
+               return (test_and_clear_bit(_PAGE_BIT_DIRTY,
&ptep->pte_low) |
+               test_and_clear_bit(_PAGE_BIT_SOFTDIRTY,
&ptep->pte_low));
 }


are you sure you're not introducing a race condition there?
and if you're sure, why do you need 2 atomic ops in sequence?