Re: [PATCH rfc 3/4] mm: filemap: move __lruvec_stat_mod_folio() out of filemap_set_pte_range()

[Kefeng Wang <wangkefeng.wang@huawei.com>]

On 2024/5/8 17:33, David Hildenbrand wrote:
> On 07.05.24 15:12, Kefeng Wang wrote:
>>
>>
>> On 2024/5/7 19:11, David Hildenbrand wrote:
>>> On 29.04.24 09:24, Kefeng Wang wrote:
>>>> Adding __folio_add_file_rmap_ptes() which don't update lruvec stat, it
>>>> is used in filemap_set_pte_range(), with it, lruvec stat updating is
>>>> moved into the caller, no functional changes.
>>>>
>>>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>>>> ---
>>>>    include/linux/rmap.h |  2 ++
>>>>    mm/filemap.c         | 27 ++++++++++++++++++---------
>>>>    mm/rmap.c            | 16 ++++++++++++++++
>>>>    3 files changed, 36 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/include/linux/rmap.h b/include/linux/rmap.h
>>>> index 7229b9baf20d..43014ddd06f9 100644
>>>> --- a/include/linux/rmap.h
>>>> +++ b/include/linux/rmap.h
>>>> @@ -242,6 +242,8 @@ void folio_add_anon_rmap_pmd(struct folio *,
>>>> struct page *,
>>>>            struct vm_area_struct *, unsigned long address, rmap_t 
>>>> flags);
>>>>    void folio_add_new_anon_rmap(struct folio *, struct 
>>>> vm_area_struct *,
>>>>            unsigned long address);
>>>> +int __folio_add_file_rmap_ptes(struct folio *, struct page *, int
>>>> nr_pages,
>>>> +        struct vm_area_struct *);
>>>>    void folio_add_file_rmap_ptes(struct folio *, struct page *, int
>>>> nr_pages,
>>>>            struct vm_area_struct *);
>>>>    #define folio_add_file_rmap_pte(folio, page, vma) \
>>>> diff --git a/mm/filemap.c b/mm/filemap.c
>>>> index 7019692daddd..3966b6616d02 100644
>>>> --- a/mm/filemap.c
>>>> +++ b/mm/filemap.c
>>>> @@ -3501,14 +3501,15 @@ static struct folio
>>>> *next_uptodate_folio(struct xa_state *xas,
>>>>    static void filemap_set_pte_range(struct vm_fault *vmf, struct folio
>>>> *folio,
>>>>                struct page *page, unsigned int nr, unsigned long addr,
>>>> -            unsigned long *rss)
>>>> +            unsigned long *rss, int *nr_mapped)
>>>>    {
>>>>        struct vm_area_struct *vma = vmf->vma;
>>>>        pte_t entry;
>>>>        entry = prepare_range_pte_entry(vmf, false, folio, page, nr, 
>>>> addr);
>>>> -    folio_add_file_rmap_ptes(folio, page, nr, vma);
>>>> +    *nr_mapped += __folio_add_file_rmap_ptes(folio, page, nr, vma);
>>>> +
>>>>        set_ptes(vma->vm_mm, addr, vmf->pte, entry, nr);
>>>>        /* no need to invalidate: a not-present page won't be cached */
>>>> @@ -3525,7 +3526,8 @@ static void filemap_set_pte_range(struct
>>>> vm_fault *vmf, struct folio *folio,
>>>>    static vm_fault_t filemap_map_folio_range(struct vm_fault *vmf,
>>>>                struct folio *folio, unsigned long start,
>>>>                unsigned long addr, unsigned int nr_pages,
>>>> -            unsigned long *rss, unsigned int *mmap_miss)
>>>> +            unsigned long *rss, int *nr_mapped,
>>>> +            unsigned int *mmap_miss)
>>>>    {
>>>>        vm_fault_t ret = 0;
>>>>        struct page *page = folio_page(folio, start);
>>>> @@ -3558,7 +3560,8 @@ static vm_fault_t filemap_map_folio_range(struct
>>>> vm_fault *vmf,
>>>>            continue;
>>>>    skip:
>>>>            if (count) {
>>>> -            filemap_set_pte_range(vmf, folio, page, count, addr, rss);
>>>> +            filemap_set_pte_range(vmf, folio, page, count, addr,
>>>> +                          rss, nr_mapped);
>>>>                if (in_range(vmf->address, addr, count * PAGE_SIZE))
>>>>                    ret = VM_FAULT_NOPAGE;
>>>>            }
>>>> @@ -3571,7 +3574,8 @@ static vm_fault_t filemap_map_folio_range(struct
>>>> vm_fault *vmf,
>>>>        } while (--nr_pages > 0);
>>>>        if (count) {
>>>> -        filemap_set_pte_range(vmf, folio, page, count, addr, rss);
>>>> +        filemap_set_pte_range(vmf, folio, page, count, addr, rss,
>>>> +                      nr_mapped);
>>>>            if (in_range(vmf->address, addr, count * PAGE_SIZE))
>>>>                ret = VM_FAULT_NOPAGE;
>>>>        }
>>>> @@ -3583,7 +3587,7 @@ static vm_fault_t filemap_map_folio_range(struct
>>>> vm_fault *vmf,
>>>>    static vm_fault_t filemap_map_order0_folio(struct vm_fault *vmf,
>>>>            struct folio *folio, unsigned long addr,
>>>> -        unsigned long *rss, unsigned int *mmap_miss)
>>>> +        unsigned long *rss, int *nr_mapped, unsigned int *mmap_miss)
>>>>    {
>>>>        vm_fault_t ret = 0;
>>>>        struct page *page = &folio->page;
>>>> @@ -3606,7 +3610,7 @@ static vm_fault_t
>>>> filemap_map_order0_folio(struct vm_fault *vmf,
>>>>        if (vmf->address == addr)
>>>>            ret = VM_FAULT_NOPAGE;
>>>> -    filemap_set_pte_range(vmf, folio, page, 1, addr, rss);
>>>> +    filemap_set_pte_range(vmf, folio, page, 1, addr, rss, nr_mapped);
>>>>        return ret;
>>>>    }
>>>> @@ -3646,6 +3650,7 @@ vm_fault_t filemap_map_pages(struct vm_fault 
>>>> *vmf,
>>>>        folio_type = mm_counter_file(folio);
>>>>        do {
>>>>            unsigned long end;
>>>> +        int nr_mapped = 0;
>>>>            addr += (xas.xa_index - last_pgoff) << PAGE_SHIFT;
>>>>            vmf->pte += xas.xa_index - last_pgoff;
>>>> @@ -3655,11 +3660,15 @@ vm_fault_t filemap_map_pages(struct vm_fault
>>>> *vmf,
>>>>            if (!folio_test_large(folio))
>>>>                ret |= filemap_map_order0_folio(vmf,
>>>> -                    folio, addr, &rss, &mmap_miss);
>>>> +                    folio, addr, &rss, &nr_mapped,
>>>> +                    &mmap_miss);
>>>>            else
>>>>                ret |= filemap_map_folio_range(vmf, folio,
>>>>                        xas.xa_index - folio->index, addr,
>>>> -                    nr_pages, &rss, &mmap_miss);
>>>> +                    nr_pages, &rss, &nr_mapped,
>>>> +                    &mmap_miss);
>>>> +
>>>> +        __lruvec_stat_mod_folio(folio, NR_FILE_MAPPED, nr_mapped);
>>>>            folio_unlock(folio);
>>>>            folio_put(folio);
>>>> diff --git a/mm/rmap.c b/mm/rmap.c
>>>> index 2608c40dffad..55face4024f2 100644
>>>> --- a/mm/rmap.c
>>>> +++ b/mm/rmap.c
>>>> @@ -1452,6 +1452,22 @@ static __always_inline void
>>>> __folio_add_file_rmap(struct folio *folio,
>>>>            mlock_vma_folio(folio, vma);
>>>>    }
>>>> +int __folio_add_file_rmap_ptes(struct folio *folio, struct page *page,
>>>> +        int nr_pages, struct vm_area_struct *vma)
>>>> +{
>>>> +    int nr, nr_pmdmapped = 0;
>>>> +
>>>> +    VM_WARN_ON_FOLIO(folio_test_anon(folio), folio);
>>>> +
>>>> +    nr = __folio_add_rmap(folio, page, nr_pages, RMAP_LEVEL_PTE,
>>>> +                  &nr_pmdmapped);
>>>> +
>>>> +    /* See comments in folio_add_anon_rmap_*() */
>>>> +    if (!folio_test_large(folio))
>>>> +        mlock_vma_folio(folio, vma);
>>>> +
>>>> +    return nr;
>>>> +}
>>>
>>> I'm not really a fan :/ It does make the code more complicated, and it
>>> will be harder to extend if we decide to ever account differently (e.g.,
>>> NR_SHMEM_MAPPED, additional tracking for mTHP etc).
>>
>> If more different accounts, this may lead to bad scalability.
> 
> We already do it for PMD mappings.
> 
>>>
>>> With large folios we'll be naturally batching already here, and I do
>>
>> Yes, it is batched with large folios，but our fs is ext4/tmpfs, there
>> are not support large folio or still upstreaming.
> 
> Okay, so that will be sorted out sooner or later.
> 
>>
>>> wonder, if this is really worth for performance, or if we could find
>>> another way of batching (let the caller activate batching and drain
>>> afterwards) without exposing these details to the caller.
>>
>> It does reduce latency when batch lruvec stat updating without large
>> folio, but I can't find better way, or let's wait for the large folio
>> support on ext4/tmpfs, I also Cced memcg maintainers in patch4 to see if
>> there are any other ideas.
> 
> I'm not convinced this benefit here is worth making the code more 
> complicated.
> 
> Maybe we can find another way to optimize this batching in rmap code 
> without having to leak these details to the callers.
> 
> For example, we could pass an optional batching structure to all rmap 
> add/rel functions that would collect these stat updates. Then we could 
> have one function to flush it and update the counters combined.
> 
> Such batching could be beneficial also for page unmapping/zapping where 
> we might unmap various different folios in one go.

It sounds better and clearer, I will try it and see the results, thanks 
for your advise!

>