On 18/08/2024 08:45, David Hildenbrand wrote: > Hi Hugh, > >> 2. I don't understand why there needs to be a new PG_partially_mapped >> flag, with all its attendant sets and tests and clears all over. Why >> can't deferred_split_scan() detect that case for itself, using the >> criteria from __folio_remove_rmap()? I see folio->_nr_pages_mapped >> is commented "Do not use outside of rmap and debug code", and >> folio_nr_pages_mapped() is currently only used from mm/debug.c; but >> using the info already maintained is preferable to adding a PG_flag >> (and perhaps more efficient - skips splitting when _nr_pages_mapped >> already fell to 0 and folio will soon be freed). > > No new users of _nr_pages_mapped if easily/cleanly avoidable, please. > > I'm currently cleaning up the final patches that introduce a new kernel config where we will stop maintaining the page->_mapcount for large folios (and consequently have to stop maintaining folio->_nr_pages_mapped). > > That's the main reasons for the comment -- at one point in my life I want to be done with that project ;) . > > folio->_nr_pages_mapped will still exist and be maintained without the new kernel config enabled. But in the new one, once we'll detect a partial mapping we'll have to flag the folio -- for example as done in this series. > > Having two ways of handling that, depending on the kernel config, will not make the code any better. > > But I agree that we should look into minimizing the usage of any new such flag: I'd have thought we only have to set the flag once, once we detect a partial mapping ... still have to review that patch more thoroughly. Yes, the flag is set only once, in deferred_split_folio once we detect a partial mapping.
