Received-SPF: pass (google.com: domain of linux-kernel+bounces-4033-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <e5b69854-2bb3-4df4-92cc-3101e9691615@arm.com>
Date: Mon, 18 Dec 2023 16:07:27 +0000
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v1 13/39] mm/rmap: factor out adding folio mappings into
 __folio_add_rmap()
Content-Language: en-GB
To: David Hildenbrand <david@redhat.com>, linux-kernel@vger.kernel.org
Cc: linux-mm@kvack.org, Andrew Morton <akpm@linux-foundation.org>,
 "Matthew Wilcox (Oracle)" <willy@infradead.org>,
 Hugh Dickins <hughd@google.com>, Yin Fengwei <fengwei.yin@intel.com>,
 Mike Kravetz <mike.kravetz@oracle.com>, Muchun Song <muchun.song@linux.dev>,
 Peter Xu <peterx@redhat.com>
References: <20231211155652.131054-1-david@redhat.com>
 <20231211155652.131054-14-david@redhat.com>
From: Ryan Roberts <ryan.roberts@arm.com>
In-Reply-To: <20231211155652.131054-14-david@redhat.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 11/12/2023 15:56, David Hildenbrand wrote:
> Let's factor it out to prepare for reuse as we convert
> page_add_anon_rmap() to folio_add_anon_rmap_[pte|ptes|pmd]().
> 
> Make the compiler always special-case on the granularity by using
> __always_inline.
> 
> Reviewed-by: Yin Fengwei <fengwei.yin@intel.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  mm/rmap.c | 81 ++++++++++++++++++++++++++++++-------------------------
>  1 file changed, 45 insertions(+), 36 deletions(-)
> 
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 2ff2f11275e5..c5761986a411 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1157,6 +1157,49 @@ int folio_total_mapcount(struct folio *folio)
>  	return mapcount;
>  }
>  
> +static __always_inline unsigned int __folio_add_rmap(struct folio *folio,
> +		struct page *page, int nr_pages, enum rmap_mode mode,
> +		unsigned int *nr_pmdmapped)
> +{
> +	atomic_t *mapped = &folio->_nr_pages_mapped;
> +	int first, nr = 0;
> +
> +	__folio_rmap_sanity_checks(folio, page, nr_pages, mode);
> +
> +	/* Is page being mapped by PTE? Is this its first map to be added? */

I suspect this comment is left over from the old version? It sounds a bit odd in
its new context.

> +	switch (mode) {
> +	case RMAP_MODE_PTE:
> +		do {
> +			first = atomic_inc_and_test(&page->_mapcount);
> +			if (first && folio_test_large(folio)) {
> +				first = atomic_inc_return_relaxed(mapped);
> +				first = (first < COMPOUND_MAPPED);
> +			}
> +
> +			if (first)
> +				nr++;
> +		} while (page++, --nr_pages > 0);
> +		break;
> +	case RMAP_MODE_PMD:
> +		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);
> +				/* Raced ahead of a remove and another add? */
> +				if (unlikely(nr < 0))
> +					nr = 0;
> +			} else {
> +				/* Raced ahead of a remove of COMPOUND_MAPPED */
> +				nr = 0;
> +			}
> +		}
> +		break;
> +	}
> +	return nr;
> +}
> +
>  /**
>   * folio_move_anon_rmap - move a folio to our anon_vma
>   * @folio:	The folio to move to our anon_vma
> @@ -1380,45 +1423,11 @@ static __always_inline void __folio_add_file_rmap(struct folio *folio,
>  		struct page *page, int nr_pages, struct vm_area_struct *vma,
>  		enum rmap_mode mode)
>  {
> -	atomic_t *mapped = &folio->_nr_pages_mapped;
> -	unsigned int nr_pmdmapped = 0, first;
> -	int nr = 0;
> +	unsigned int nr, nr_pmdmapped = 0;

You're still being inconsistent with signed/unsigned here. Is there a reason
these can't be signed like nr_pages in the interface?

>  
>  	VM_WARN_ON_FOLIO(folio_test_anon(folio), folio);
> -	__folio_rmap_sanity_checks(folio, page, nr_pages, mode);
> -
> -	/* Is page being mapped by PTE? Is this its first map to be added? */
> -	switch (mode) {
> -	case RMAP_MODE_PTE:
> -		do {
> -			first = atomic_inc_and_test(&page->_mapcount);
> -			if (first && folio_test_large(folio)) {
> -				first = atomic_inc_return_relaxed(mapped);
> -				first = (first < COMPOUND_MAPPED);
> -			}
> -
> -			if (first)
> -				nr++;
> -		} while (page++, --nr_pages > 0);
> -		break;
> -	case RMAP_MODE_PMD:
> -		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);
> -				/* Raced ahead of a remove and another add? */
> -				if (unlikely(nr < 0))
> -					nr = 0;
> -			} else {
> -				/* Raced ahead of a remove of COMPOUND_MAPPED */
> -				nr = 0;
> -			}
> -		}
> -		break;
> -	}
>  
> +	nr = __folio_add_rmap(folio, page, nr_pages, mode, &nr_pmdmapped);
>  	if (nr_pmdmapped)
>  		__lruvec_stat_mod_folio(folio, folio_test_swapbacked(folio) ?
>  			NR_SHMEM_PMDMAPPED : NR_FILE_PMDMAPPED, nr_pmdmapped);