On Mon, Aug 21, 2023 at 03:29:01AM +0100, Matthew Wilcox wrote: > On Sun, Aug 20, 2023 at 09:13:55PM -0400, Peter Xu wrote: > > > https://lore.kernel.org/linux-mm/ZNp7yUgUrIpILnXu@xxxxxxxxxxxxxxxxxxxx/ > > > > https://lore.kernel.org/linux-mm/ZNqFv0AwkfDKExiw@x1n/#t > > > > Firstly, I've answered and you didn't follow that up. > > I didn't see it. I get a lot of email ... > > > > > More importantly, I think this is over-parametrisation. If you start to > > > > use extra fields in struct folio, just change the code in page_alloc.c > > > > directly. > > > > Change the hard-coded "2"s in different functions? Can you kindly explain > > why can't we just have a macro to help? > > Because it's unnecessary. You're putting in way too much effort here > for something that might happen once. > > > Setting tail mapping for tail 1/2 is even wrong, which part of this patch > > fixes: > > > > @@ -428,7 +428,8 @@ static inline void prep_compound_tail(struct page *head, int tail_idx) > > { > > struct page *p = head + tail_idx; > > > > - p->mapping = TAIL_MAPPING; > > + if (tail_idx > TAIL_MAPPING_REUSED_MAX) > > + p->mapping = TAIL_MAPPING; > > set_compound_head(p, head); > > set_page_private(p, 0); > > } > > I didn't see this. This is wrong. tail->mapping is only reused for > large rmappable pages. It's not reused for other compound pages. Just to make sure we're on the same page: I think it's not only _deferred_list (of tail page 2) that reused the mapping field (word offset 3), but also _nr_pages_mapped (of tail page 1)? > > If you really insist on cleaning this up, the special casing of tail pages > should move out of page_alloc entirely. folio_undo_large_rmappable() > should restore TAIL_MAPPING for all tail pages that were modified by > folio_prep_large_rmappable(). > > The other thing we should do is verify that the additional large-rmap > fields have the correct values in folio_undo_large_rmappable(). > > But let's look back to why TAIL_MAPPING was introduced. Commit > 1c290f642101e poisoned tail->mapping to catch users which forgot to > call compound_head(). So we can actually remove TAIL_MAPPING entirely > if we get rid of page->mapping. > > You probably think that's an unattainable goal; there are something like > 340 occurrences of the string 'page->mapping' in the kernel right now > (and there are probably instances of struct page named something other > than 'page'), but a lot of those are actually in comments, which would > be my fault for not fixing them during folio conversions. > > However, I have a small patch series which enables 'allnoconfig' to > build after renaming page->mapping to page->_mapping. Aside from fs/ > there are *very* few places which look at page->mapping [1]. I'll post > that patch series tomorrow. Assuming it's still not yet posted; I can wait and read it. If you plan to remove the whole TAIL_MAPPING in a few days then I agree this patch is not needed, but so far I don't know when it'll land and also why, before that it does sound like we can still keep this patch. Regarding the question on "why removing TAIL_MAPPING": poisoning an unused field is always helpful to me even if not referenced with "page->mapping". So I don't see an immediate benefit from removing the poisoning if it's already there; OTOH not sure whether poison more unused fields will be more helpful in general? > > I think with some serious work, we can land "remove page->mapping" > (which would include removing TAIL_MAPPING) by the end of the year. > And that work gets us closer to the goal of shrinking struct page. > > [1] device-dax, intel_th, mthca, cortina, fb_defio Thanks, -- Peter Xu
