On Fri, 30 Jun 2023 08:28:54 -0700 (PDT)
Hugh Dickins <hughd@xxxxxxxxxx> wrote:
> On Fri, 30 Jun 2023, Claudio Imbrenda wrote:
> > On Tue, 20 Jun 2023 00:51:19 -0700 (PDT)
> > Hugh Dickins <hughd@xxxxxxxxxx> wrote:
> >
> > [...]
> >
> > > +void pte_free_defer(struct mm_struct *mm, pgtable_t pgtable)
> > > +{
> > > + unsigned int bit, mask;
> > > + struct page *page;
> > > +
> > > + page = virt_to_page(pgtable);
> > > + if (mm_alloc_pgste(mm)) {
> > > + call_rcu(&page->rcu_head, pte_free_pgste);
> >
> > so is this now going to be used to free page tables
> > instead of page_table_free_rcu?
>
> No.
>
> All pte_free_defer() is being used for (in this series; and any future
> use beyond this series will have to undertake its own evaluations) is
> for the case of removing an empty page table, which used to map a group
> of PTE mappings of a file, in order to make way for one PMD mapping of
> the huge page which those scattered pages have now been gathered into.
>
> You're worried by that mm_alloc_pgste() block: it's something I didn't
actually no, but thanks for bringing it up :D
> have at all in my first draft, then I thought that perhaps the pgste
> case might be able to come this way, so it seemed stupid to leave out
> the handling for it.
>
> I hope that you're implying that should be dead code here? Perhaps,
> that the pgste case corresponds to the case in s390 where THPs are
> absolutely forbidden? That would be good news for us.
>
> Gerald, in his version of this block, added a comment asking:
> /*
> * TODO: Do we need gmap_unlink(mm, pgtable, addr), like in
> * page_table_free_rcu()?
> * If yes -> need addr parameter here, like in pte_free_tlb().
> */
> Do you have the answer to that? Neither of us could work it out.
this is the thing I'm worried about; removing a page table that was
used to map a guest will leave dangling pointers in the gmap that will
cause memory corruption (I actually ran into that problem myself for
another patchseries).
gmap_unlink() is needed to clean up the pointers before they become
dangling (and also potentially do some TLB purging as needed)
the point here is: we need that only for page_table_free_rcu(); all
other users of page_table_free() cannot act on guest page tables
(because we don't allow THP for KVM guests). and that is why
page_table_free() does not do gmap_unlink() currently.
>
> >
> > or will it be used instead of page_table_free?
>
> Not always; but yes, this case of removing a page table used
> page_table_free() before; but now, with the lighter locking, needs
> to keep the page table valid until the RCU grace period expires.
so if I understand correctly your code will, sometimes, under some
circumstances, replace what page_table_free() does, but it will never
replace page_table_free_rcu()?
because in that case there would be no issues
>
> >
> > this is actually quite important for KVM on s390
>
> None of us are wanting to break KVM on s390: your guidance appreciated!
>
> Thanks,
> Hugh
