Date:   Wed, 15 Feb 2023 03:06:39 +0800
From:   Chih-En Lin <shiyn.lin@...il.com>
To:     David Hildenbrand <david@...hat.com>
Cc:     Pasha Tatashin <pasha.tatashin@...een.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Qi Zheng <zhengqi.arch@...edance.com>,
        "Matthew Wilcox (Oracle)" <willy@...radead.org>,
        Christophe Leroy <christophe.leroy@...roup.eu>,
        John Hubbard <jhubbard@...dia.com>,
        Nadav Amit <namit@...are.com>, Barry Song <baohua@...nel.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Yang Shi <shy828301@...il.com>, Peter Xu <peterx@...hat.com>,
        Vlastimil Babka <vbabka@...e.cz>,
        Zach O'Keefe <zokeefe@...gle.com>,
        Yun Zhou <yun.zhou@...driver.com>,
        Hugh Dickins <hughd@...gle.com>,
        Suren Baghdasaryan <surenb@...gle.com>,
        Yu Zhao <yuzhao@...gle.com>, Juergen Gross <jgross@...e.com>,
        Tong Tiangen <tongtiangen@...wei.com>,
        Liu Shixin <liushixin2@...wei.com>,
        Anshuman Khandual <anshuman.khandual@....com>,
        Li kunyu <kunyu@...china.com>,
        Minchan Kim <minchan@...nel.org>,
        Miaohe Lin <linmiaohe@...wei.com>,
        Gautam Menghani <gautammenghani201@...il.com>,
        Catalin Marinas <catalin.marinas@....com>,
        Mark Brown <broonie@...nel.org>, Will Deacon <will@...nel.org>,
        Vincenzo Frascino <Vincenzo.Frascino@....com>,
        Thomas Gleixner <tglx@...utronix.de>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Andy Lutomirski <luto@...nel.org>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        "Liam R. Howlett" <Liam.Howlett@...cle.com>,
        Fenghua Yu <fenghua.yu@...el.com>,
        Andrei Vagin <avagin@...il.com>,
        Barret Rhoden <brho@...gle.com>,
        Michal Hocko <mhocko@...e.com>,
        "Jason A. Donenfeld" <Jason@...c4.com>,
        Alexey Gladkov <legion@...nel.org>,
        linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-mm@...ck.org, linux-trace-kernel@...r.kernel.org,
        linux-perf-users@...r.kernel.org,
        Dinglan Peng <peng301@...due.edu>,
        Pedro Fonseca <pfonseca@...due.edu>,
        Jim Huang <jserv@...s.ncku.edu.tw>,
        Huichun Feng <foxhoundsk.tw@...il.com>
Subject: Re: [PATCH v4 00/14] Introduce Copy-On-Write to Page Table

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

Powered by blists - more mailing lists