Arjan van de Ven wrote:

>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.
>
I don't have access to a Stratus web server where I could post the code, 
since I'm currently sitting at a Redhat desk :)  So if you'll indulge 
me, I'll attach the harvest code to this note.  The harvest code is one 
component of a GPL'd kernel module.  The harvest component is the only 
one that interacts with the dirty page tracking patch.  Other parts of 
the module do PCI-hotplug related things that are specific to our platform.

For the purposes of looking at how Stratus uses the harvest and page 
tracking, have a look at the harvest and process_vector functions, both 
of which are called from the brownout_cycle function.  The rest of the 
code in the file is mostly scaffolding to support cpu rendezvous 
operations, and to make call-outs to routines that copy memory pages.

>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)
>
thanks,  I'll make those changes and re-build/re-test/re-post.

>-       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?
>
I think we're OK there. That code only appears in the sys_msync path
(sync_page_range), after the page table lock is taken.  The only code
that moves hardware dirty bits to software dirty bits is in the harvest
itself (attached).  And that code runs with all other cpus parked.

-kimball