On Tue, Feb 14, 2023 at 06:59:50PM +0100, David Hildenbrand wrote: > On 14.02.23 18:54, Chih-En Lin wrote: > > > > > > > > > (2) break_cow_pte() can fail, which means that we can fail some > > > > > operations (possibly silently halfway through) now. For example, > > > > > looking at your change_pte_range() change, I suspect it's wrong. > > > > > > > > Maybe I should add WARN_ON() and skip the failed COW PTE. > > > > > > One way or the other we'll have to handle it. WARN_ON() sounds wrong for > > > handling OOM situations (e.g., if only that cgroup is OOM). > > > > Or we should do the same thing like you mentioned: > > " > > For example, __split_huge_pmd() is currently not able to report a > > failure. I assume that we could sleep in there. And if we're not able to > > allocate any memory in there (with sleeping), maybe the process should > > be zapped either way by the OOM killer. > > " > > > > But instead of zapping the process, we just skip the failed COW PTE. > > I don't think the user will expect their process to be killed by > > changing the protection. > > The process is consuming more memory than it is capable of consuming. The > process most probably would have died earlier without the PTE optimization. > > But yeah, it all gets tricky ... > > > > > > > > > > > > (3) handle_cow_pte_fault() looks quite complicated and needs quite some > > > > > double-checking: we temporarily clear the PMD, to reset it > > > > > afterwards. I am not sure if that is correct. For example, what > > > > > stops another page fault stumbling over that pmd_none() and > > > > > allocating an empty page table? Maybe there are some locking details > > > > > missing or they are very subtle such that we better document them. I > > > > > recall that THP played quite some tricks to make such cases work ... > > > > > > > > I think that holding mmap_write_lock may be enough (I added > > > > mmap_assert_write_locked() in the fault function btw). But, I might > > > > be wrong. I will look at the THP stuff to see how they work. Thanks. > > > > > > > > > > Ehm, but page faults don't hold the mmap lock writable? And so are other > > > callers, like MADV_DONTNEED or MADV_FREE. > > > > > > handle_pte_fault()->handle_pte_fault()->mmap_assert_write_locked() should > > > bail out. > > > > > > Either I am missing something or you didn't test with lockdep enabled :) > > > > You're right. I thought I enabled the lockdep. > > And, why do I have the page fault will handle the mmap lock writable in my mind. > > The page fault holds the mmap lock readable instead of writable. > > ;-) > > > > I should check/test all the locks again. > > Thanks. > > Note that we have other ways of traversing page tables, especially, using > the rmap which does not hold the mmap lock. Not sure if there are similar > issues when suddenly finding no page table where there logically should be > one. Or when a page table gets replaced and modified, while rmap code still > walks the shared copy. Hm. It seems like I should take carefully for the page table entry in page fault with rmap. ;) While the rmap code walks the page table, it will hold the pt lock. So, maybe I should hold the old (shared) PTE table's lock in handle_cow_pte_fault() all the time. Thanks, Chih-En Lin
