Re: Dirty page tracking patch

[Arjan van de Ven]

> >
> >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.

ok so your entire kernel side stack is GPL? Good.
> >
> >+#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.

then you didn't answer my question about why the lock; atomic bit is
needed ;)


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/