On 13.04.22 14:48, Matthew Wilcox wrote: > On Wed, Apr 13, 2022 at 02:28:38PM +0200, David Hildenbrand wrote: >> On 13.04.22 14:26, Matthew Wilcox wrote: >>> On Tue, Apr 12, 2022 at 11:37:09AM +0200, David Hildenbrand wrote: >>>> On 12.04.22 10:47, Vlastimil Babka wrote: >>>>> There's a VM_BUG_ON_PAGE(PageTransCompound(page), page); later in a >>>>> !compound branch. Since compound is now determined by the same check, could >>>>> be deleted. >>>> >>>> Yes, eventually we could get rid of both VM_BUG_ON_PAGE() on both >>>> branches and add a single VM_BUG_ON_PAGE(PageTail(page), page) check on >>>> the compound branch. (we could also make sure that we're not given a >>>> hugetlb page) >>> >>> As a rule of thumb, if you find yourself wanting to add >>> VM_BUG_ON_PAGE(PageTail(page), page), you probably want to change the >>> interface to take a folio. >> >> Yeah, I had the same in mind. Might be a reasonable addon on top -- >> although it would stick out in the rmap code a bit because most >> functions deal with both, folios and subpages. > > I have the start of a series which starts looking at the fault path > to see where it makes sense to use folios and where it makes sense to > use pages. > > We're (generally) faulting on a PTE, so we need the precise page to > be returned in vmf->page. However vmf->cow_page can/should be a > folio (because it's definitely not a tail page). That trickles > down into copy_present_page() (new_page and prealloc both become folios) > and so page_add_new_anon_rmap() then looks like a good target to > take a folio. > > The finish_fault() -> do_set_pte() -> page_add_new_anon_rmap() looks > like the only kind of strange place where we don't necessarily have a > folio (all the others we just allocated it). > That's an interesting point. In this patch I'm assuming that we don't have a compound page here (see below). Which makes sense, because as the interface states "Same as page_add_anon_rmap but must only be called on *new* pages.". At least to me it would be weird to allocate a new compound page to then pass a subpage to do_set_pte() page_add_new_anon_rmap(). And in fact, inside page_add_new_anon_rmap(compound=false) we have /* Anon THP always mapped first with PMD */ VM_BUG_ON_PAGE(PageTransCompound(page), page); which makes sure that we cannot have a compound page here, but in fact a folio. So unless I am missing something, do_set_pte() should in fact have a folio here unless BUG? -- Thanks, David / dhildenb
