Re: [RFC PATCH 3/5] rmap: add page_add_file_rmap_range()

[Matthew Wilcox <willy@infradead.org>]

On Mon, Jan 30, 2023 at 08:55:02PM +0800, Yin Fengwei wrote:
> +++ b/include/linux/rmap.h
> @@ -198,6 +198,8 @@ void folio_add_new_anon_rmap(struct folio *, struct vm_area_struct *,
>  		unsigned long address);
>  void page_add_file_rmap(struct page *, struct vm_area_struct *,
>  		bool compound);
> +void page_add_file_rmap_range(struct folio *, struct page *, int len,
> +	struct vm_area_struct *, bool compound);

Again, two tabs please.

> -/**
> - * page_add_file_rmap - add pte mapping to a file page
> - * @page:	the page to add the mapping to
> - * @vma:	the vm area in which the mapping is added
> - * @compound:	charge the page as compound or small page
> - *
> - * The caller needs to hold the pte lock.
> - */

We really should have kernel-doc for this function.

> -void page_add_file_rmap(struct page *page, struct vm_area_struct *vma,
> -		bool compound)
> +void page_add_file_rmap_range(struct folio *folio, struct page *page, int len,
> +	struct vm_area_struct *vma, bool compound)

Indentation

>  {
> -	struct folio *folio = page_folio(page);
>  	atomic_t *mapped = &folio->_nr_pages_mapped;
> -	int nr = 0, nr_pmdmapped = 0;
> +	int nr = 0, nr_pmdmapped = 0, nr_pages = folio_nr_pages(folio);

Should be unsigned.

Also the 'len' parameter should be called nr_pages, and then do
something like:

	if (nr_pages > folio_nr_pages(folio))
		nr_pages = folio_nr_pages(folio);

>  	bool first;
>  
> -	VM_BUG_ON_PAGE(compound && !PageTransHuge(page), page);
> -
> -	/* Is page being mapped by PTE? Is this its first map to be added? */
> -	if (likely(!compound)) {
> -		first = atomic_inc_and_test(&page->_mapcount);
> -		nr = first;
> -		if (first && folio_test_large(folio)) {
> -			nr = atomic_inc_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 */
> -
> +	if (compound) {

You've dropped the 'unlikely' marker here.  Also I'm not sure why you
switched the order of these two cases around; makes it harder to review.

>  		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_pmdmapped = nr_pages;
>  				nr = nr_pmdmapped - (nr & FOLIO_PAGES_MAPPED);
>  				/* Raced ahead of a remove and another add? */
>  				if (unlikely(nr < 0))
> @@ -1344,6 +1323,20 @@ void page_add_file_rmap(struct page *page, struct vm_area_struct *vma,
>  				nr = 0;
>  			}
>  		}
> +	} else {
> +		int i = 0, new_mapped = 0, count;
> +
> +		count = min_t(int, len, nr_pages - folio_page_idx(folio, page));
> +		do {
> +			first = atomic_inc_and_test(&page->_mapcount);
> +			new_mapped = first;
> +			if (first && folio_test_large(folio)) {
> +				new_mapped = atomic_inc_return_relaxed(mapped);
> +				new_mapped = (new_mapped < COMPOUND_MAPPED);
> +			}
> +			if  (new_mapped)

Space between if and '('.

I'm not quite sure why you want to pass 'page' in rather than the
offset of the page within the folio, but perhaps that will become
obvious as I read along.