Re: [PATCH v4 00/14] Introduce Copy-On-Write to Page Table

[Chih-En Lin <shiyn.lin@gmail.com>]

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