| Message ID | 20230206153049.770556-3-fengwei.yin@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | remove parameter 'compound' of add_file_rmap | expand |
On Mon, Feb 06, 2023 at 11:30:49PM +0800, Yin Fengwei wrote: > Remove parameter 'compound' from folio_add_file_rmap_range(). > > The parameter nr_pages is checked whether same as folio page > numbers to know whether it's entire folio operation. We can't do this yet. Even if a folio is large enough to be PMD mapped, it may have been mapped askew (or its PMD mapping may have been split). We still need the caller to tell us whether it's been mapped with a PMD or with PTEs.
On 2/7/2023 12:50 AM, Matthew Wilcox wrote: > On Mon, Feb 06, 2023 at 11:30:49PM +0800, Yin Fengwei wrote: >> Remove parameter 'compound' from folio_add_file_rmap_range(). >> >> The parameter nr_pages is checked whether same as folio page >> numbers to know whether it's entire folio operation. > > We can't do this yet. Even if a folio is large enough to be PMD mapped, > it may have been mapped askew (or its PMD mapping may have been split). > We still need the caller to tell us whether it's been mapped with > a PMD or with PTEs. OK. My understand is that the info about PMD mapped is only for statistic update. Maybe move the PMD mapped statistic to caller. Another thing I am not sure whether it's worthy: What about maintaining total mapcount for folio? So we don't need to query each page mapcount to know it. So we can use "total_mapoucnt > mapped" to know whether the folio has at least one page mapped more than once. The payback is that we need update total mapcount when map/unmap the folio. Regards Yin, Fengwei >
On Tue, Feb 07, 2023 at 10:44:10AM +0800, Yin, Fengwei wrote: > On 2/7/2023 12:50 AM, Matthew Wilcox wrote: > > On Mon, Feb 06, 2023 at 11:30:49PM +0800, Yin Fengwei wrote: > >> Remove parameter 'compound' from folio_add_file_rmap_range(). > >> > >> The parameter nr_pages is checked whether same as folio page > >> numbers to know whether it's entire folio operation. > > > > We can't do this yet. Even if a folio is large enough to be PMD mapped, > > it may have been mapped askew (or its PMD mapping may have been split). > > We still need the caller to tell us whether it's been mapped with > > a PMD or with PTEs. > OK. My understand is that the info about PMD mapped is only for > statistic update. Maybe move the PMD mapped statistic to caller. Alas, no. That 'compound' parameter really means "to be mapped by a PMD". And we need to change all of add_file, add_anon and remove at the same time, otherwise we don't know which counter to inc/dec in page_remove_rmap(). > Another thing I am not sure whether it's worthy: > What about maintaining total mapcount for folio? So we don't need to > query each page mapcount to know it. So we can use "total_mapoucnt > > mapped" to know whether the folio has at least one page mapped more > than once. > > The payback is that we need update total mapcount when map/unmap > the folio. Right, there are lots of mapcounts we _could_ maintain. The important thing to understand is who wants to know what. As I see it, there are three important things we need to know: 1. Is any page in this folio mapped? (everywhere that calls folio_mapped() or page_mapped()) 2. Is any page in this folio mapped more than once? (some madvise calls, migration) 3. How many refcounts does the mapcount account for? (splitting, compaction) Some of the things we use mapcount for today I consider to be unimportant. For example, shrink_folio_list() checks to see if there are any PMD mappings and will split it if there are not. This doesn't fit my vision of how the VM should work; I believe we should swap the entire folio out as a single unit. So I don't really want to think about maintaining total_mapcount, I want the concept of total_mapcount to go away and just have a single mapcount for each folio, instead of this complex mismash of entire_mapcount, mapcount and total_mapcount.
On 2/7/2023 1:03 PM, Matthew Wilcox wrote: > On Tue, Feb 07, 2023 at 10:44:10AM +0800, Yin, Fengwei wrote: >> On 2/7/2023 12:50 AM, Matthew Wilcox wrote: >>> On Mon, Feb 06, 2023 at 11:30:49PM +0800, Yin Fengwei wrote: >>>> Remove parameter 'compound' from folio_add_file_rmap_range(). >>>> >>>> The parameter nr_pages is checked whether same as folio page >>>> numbers to know whether it's entire folio operation. >>> >>> We can't do this yet. Even if a folio is large enough to be PMD mapped, >>> it may have been mapped askew (or its PMD mapping may have been split). >>> We still need the caller to tell us whether it's been mapped with >>> a PMD or with PTEs. >> OK. My understand is that the info about PMD mapped is only for >> statistic update. Maybe move the PMD mapped statistic to caller. > > Alas, no. That 'compound' parameter really means "to be mapped by a > PMD". And we need to change all of add_file, add_anon and remove > at the same time, otherwise we don't know which counter to inc/dec > in page_remove_rmap(). OK. I thought that "compound" could mean "mapping the entire folio" when I worked on this patch. Thanks a lot for clarification. Regards Yin, Fengwei > >> Another thing I am not sure whether it's worthy: >> What about maintaining total mapcount for folio? So we don't need to >> query each page mapcount to know it. So we can use "total_mapoucnt > >> mapped" to know whether the folio has at least one page mapped more >> than once. >> >> The payback is that we need update total mapcount when map/unmap >> the folio. > > Right, there are lots of mapcounts we _could_ maintain. The important > thing to understand is who wants to know what. As I see it, there are > three important things we need to know: > > 1. Is any page in this folio mapped? > (everywhere that calls folio_mapped() or page_mapped()) > > 2. Is any page in this folio mapped more than once? > (some madvise calls, migration) > > 3. How many refcounts does the mapcount account for? > (splitting, compaction) > > Some of the things we use mapcount for today I consider to be unimportant. > For example, shrink_folio_list() checks to see if there are any PMD > mappings and will split it if there are not. This doesn't fit my vision > of how the VM should work; I believe we should swap the entire folio > out as a single unit. > > So I don't really want to think about maintaining total_mapcount, > I want the concept of total_mapcount to go away and just have a single > mapcount for each folio, instead of this complex mismash of > entire_mapcount, mapcount and total_mapcount.
diff --git a/include/linux/rmap.h b/include/linux/rmap.h index 974124b41fee..00e2a229bb24 100644 --- a/include/linux/rmap.h +++ b/include/linux/rmap.h @@ -199,7 +199,7 @@ void folio_add_new_anon_rmap(struct folio *, struct vm_area_struct *, void page_add_file_rmap(struct page *, struct vm_area_struct *, bool compound); void folio_add_file_rmap_range(struct folio *, unsigned long start, - unsigned int nr_pages, struct vm_area_struct *, bool compound); + unsigned int nr_pages, struct vm_area_struct *); void page_remove_rmap(struct page *, struct vm_area_struct *, bool compound); diff --git a/mm/memory.c b/mm/memory.c index 51f8bd91d9f0..fff1de9ac233 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -4270,7 +4270,7 @@ void do_set_pte_range(struct vm_fault *vmf, struct folio *folio, pte_t entry; if (!cow) { - folio_add_file_rmap_range(folio, start, nr, vma, false); + folio_add_file_rmap_range(folio, start, nr, vma); add_mm_counter(vma->vm_mm, mm_counter_file(page), nr); } else { /* diff --git a/mm/rmap.c b/mm/rmap.c index 3ab67b33094b..23cb6983bfe7 100644 --- a/mm/rmap.c +++ b/mm/rmap.c @@ -1308,24 +1308,23 @@ void folio_add_new_anon_rmap(struct folio *folio, struct vm_area_struct *vma, * @start: The first page number in folio * @nr_pages: The number of pages which will be mapped * @vma: the vm area in which the mapping is added - * @compound: charge the page as compound or small page * * The page range of folio is defined by [first_page, first_page + nr_pages) * * The caller needs to hold the pte lock. */ void folio_add_file_rmap_range(struct folio *folio, unsigned long start, - unsigned int nr_pages, struct vm_area_struct *vma, - bool compound) + unsigned int nr_pages, struct vm_area_struct *vma) { atomic_t *mapped = &folio->_nr_pages_mapped; unsigned int nr_pmdmapped = 0, first; int nr = 0; + bool entire_map = folio_test_large(folio) && + (nr_pages == folio_nr_pages(folio)); - VM_WARN_ON_FOLIO(compound && !folio_test_pmd_mappable(folio), folio); /* Is page being mapped by PTE? Is this its first map to be added? */ - if (likely(!compound)) { + if (likely(!entire_map)) { struct page *page = folio_page(folio, start); nr_pages = min_t(unsigned int, nr_pages, @@ -1341,15 +1340,13 @@ void folio_add_file_rmap_range(struct folio *folio, unsigned long start, if (first) nr++; } while (page++, --nr_pages > 0); - } else if (folio_test_pmd_mappable(folio)) { - /* That test is redundant: it's for safety or to optimize out */ + } else { first = atomic_inc_and_test(&folio->_entire_mapcount); if (first) { nr = atomic_add_return_relaxed(COMPOUND_MAPPED, mapped); if (likely(nr < COMPOUND_MAPPED + COMPOUND_MAPPED)) { - nr_pmdmapped = folio_nr_pages(folio); - nr = nr_pmdmapped - (nr & FOLIO_PAGES_MAPPED); + nr = nr_pages - (nr & FOLIO_PAGES_MAPPED); /* Raced ahead of a remove and another add? */ if (unlikely(nr < 0)) nr = 0; @@ -1358,6 +1355,9 @@ void folio_add_file_rmap_range(struct folio *folio, unsigned long start, nr = 0; } } + + if (folio_test_pmd_mappable(folio)) + nr_pmdmapped = nr_pages; } if (nr_pmdmapped) @@ -1366,7 +1366,7 @@ void folio_add_file_rmap_range(struct folio *folio, unsigned long start, if (nr) __lruvec_stat_mod_folio(folio, NR_FILE_MAPPED, nr); - mlock_vma_folio(folio, vma, compound); + mlock_vma_folio(folio, vma, entire_map); } /** @@ -1390,8 +1390,8 @@ void page_add_file_rmap(struct page *page, struct vm_area_struct *vma, else nr_pages = folio_nr_pages(folio); - folio_add_file_rmap_range(folio, folio_page_idx(folio, page), - nr_pages, vma, compound); + folio_add_file_rmap_range(folio, + folio_page_idx(folio, page), nr_pages, vma); } static void folio_remove_entire_rmap(struct folio *folio, @@ -1448,15 +1448,30 @@ void page_remove_rmap(struct page *page, struct vm_area_struct *vma, /* Is page being unmapped by PTE? Is this its last map to be removed? */ if (likely(!compound)) { - last = atomic_add_negative(-1, &page->_mapcount); - nr = last; - if (last && folio_test_large(folio)) { - nr = atomic_dec_return_relaxed(mapped); - nr = (nr < COMPOUND_MAPPED); + if ((atomic_read(&page->_mapcount) == -1) && + folio_test_large(folio)) { + int i, nr_pages = folio_nr_pages(folio); + + for (i = 0; i < nr_pages; i++) { + struct page *pg = folio_page(folio, i); + + if (pg != page) + atomic_inc(&pg->_mapcount); + } + folio_remove_entire_rmap(folio, &nr, + &nr_pmdmapped); + + nr++; + } else { + last = atomic_add_negative(-1, &page->_mapcount); + nr = last; + if (last && folio_test_large(folio)) { + nr = atomic_dec_return_relaxed(mapped); + nr = (nr < COMPOUND_MAPPED); + } } } else if (folio_test_pmd_mappable(folio)) { /* That test is redundant: it's for safety or to optimize out */ - folio_remove_entire_rmap(folio, &nr, &nr_pmdmapped); }
Remove parameter 'compound' from folio_add_file_rmap_range(). The parameter nr_pages is checked whether same as folio page numbers to know whether it's entire folio operation. Convert the folio _entire_mapcount to page mapcount to handle the case the folio is added as entire folio but removed by removing each page. Signed-off-by: Yin Fengwei <fengwei.yin@intel.com> --- include/linux/rmap.h | 2 +- mm/memory.c | 2 +- mm/rmap.c | 51 ++++++++++++++++++++++++++++---------------- 3 files changed, 35 insertions(+), 20 deletions(-)