Re: [RFP-V2 0/3] Make mmu_notifier_invalidate_range_start able to sleep.

[Andrea Arcangeli <aarcange@redhat.com>]

On Tue, Feb 02, 2010 at 06:48:33PM -0600, Robin Holt wrote:
> In the _invalidate_page case, it is called by the kernel from sites where
> the kernel is relying upon the reference count to eliminate the page from
> use while maintaining the page's data as clean and ready to be released.
> If the page is marked as dirty, etc. then the kernel will "do the right
> thing" with the page to maintain data consistency.
>
> The _invalidate_range_start/end pairs are used in places where the
> caller's address space is being modified.  If we allow the attachers
> to continue to use the old pages from the old mapping even for a short
> time after the process has started to use the new pages, there would be
> silent data corruption.

Just to show how fragile your assumption is, your code is already
generating mm corruption in fork and in ksm... the set_pte_at_notify
invalidate_page has to run immediately and be effective immediately
despite being called with the PT lock hold.

> A difference is the kernel's expectations.  The truncate case is the one

do_wp_page and ksm ->invalidate_page or ->change_pte have expectations
different than what the ones you expect it to have.

It's the first time I hear about the semantics of invalidate_page and
range_start/end being different. The only reason why there are two
different calls is that start/end wants to run a single tlb flush for
many ptes teardown. While this isn't doable with the ptep_clear_flush
or set_pte_at versions beacuse there is a single pte to invalidate and
not multiple. So for the latter we simply have a single call and
there's no need of a start/stop range.

> place where the kernel's expectation for _invalidate_range_start/end
> more closely matches those of _invalidate_page.  When I was babbling
> about a new version of the patch, it basically adds that concept to the
> _invalidate_range_start callout as a parameter.  Essentially changing
> the bool atomic into a flag indicating the kernel does not expect this
> step to be complete prior finishing this callout.

I don't get it sorry.

> I don't like that second patch which is why I have not posted it.
> It relies upon the fuzzy quantity of an "adequate" period of time between
> when the file is truncated down before it may be extended again to ensure
> data consistency.  Shrink and extend too quickly and problems will ensue.

Not sure I get it, it's all i_mutex serialized so i_size can't change
under the code under discussion, I don't see the shrink and extend too
quick.

> > can't schedule inside the anon_vma->lock and uses the range calls to
> > be safer (then maybe we can require the mmu notifier users to check
> > PageTransHuge against the pages and handle the invalidate through
> > ->invalidate_page or we can add ->invalidate_transhuge_page.
> 
> I don't think that is a problem.  I don't think the GRU has any issues
> at all.  I believe that the invalidate even of a standard page size will
> eliminate the entire TLB.  Jack was going to verify that the last time
> I talked with him.  If it behaved any differently, I would be surprised
> as it would be inconsistent with nearly every other TLB out there.

GRU sure is fine as it has no sleepability requirement. My
pmdp_clear_flush_notify calls invalidate_range_start/end to flush the
entire mmu range in case the secondary mmu couldn't map the hugepage
in a single hugetlb (like kvm does).

> XPMEM will currently not work, but I believe I can get it to work quite
> easily as I can walk the segment's PFN table without acquiring any
> sleeping locks and decide to expand the page size for any invalidation
> within that range.  With that, at the time of the callout, I can schedule
> an invalidation of the appropriate size.

Schedule or/and do it later isn't safe in khugepaged, ksm, do_wp_page
etc.. all running under PT lock. It also isn't ok for
pmdp_clear_flush_notify calling the
mmu_notifier_invalidate_range_start/end(atomic=0), it's unacceptable
to fail and not flush sptes as I have to migrate a writable page in
khugepaged and data writes will be lost if you don't really stop the
remote dma before pmdp_clear_flush_notify returns!

> As for the transparent huge page patches, I have just skimmed them
> lightly as I really don't have much time to understand your intention.
> I do think I am agreeing with Christoph Lameter that using the migration
> style mechanism is probably better in that it handles the invalidate_page
> callouts, follows the page reference counts, and allows my asynchronous
> invalidation expectation to persist.

I think your asynchronous invalidation expectation are invalid
regardless of khugepaged and pmdp_clear_flush_notify.

> If I read it correctly, your patch would now require invalidate_page
> to be complete and have reference counts pushed back down upon return.
> I probably missed a key portion of that patch and would welcome the
> chance to be informed.

The problem aren't the reference counts. The problem is that the
writes coming from remote must not go to a page regardless of its page
count, this isn't about khugepaged only. The reads coming from remote
also must not read from local memory after invalidate or do_wp_page
will break as the writes through the pte will go to the copied page
while the remote mapping still reads from the old page.

It's a pure accident it works ok in swap (because swap won't alter the
pte->pnf mapping after the invalidate and it won't actually lose the
swap entry until the refcount is only the one for the lru). Every
other place where invalidate_page is called and the pte->pfn mapping
is altered (and it's not only marked nonpresent and remapped on the
same pfn in case of swapcache fault) requires synchronous updating of
secondary sptes in invalidate_page too.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>