Re: [PATCH 7/8] mm/khugepaged: hoist more code into collapse_isolate_folio()

[Nico Pache (Red Hat)]

> Checking expected refcounts or dirtyness has no need for the i_pages
> xarray lock.
> 
> Signed-off-by: Pedro Falcato <pfalcato@xxxxxxx>
>
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 429e2c5833d0..a09e4e4e1943 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -2382,13 +2382,48 @@ static enum scan_result collapse_isolate_folio(struct collapse_file_state *state
>  
>  	if (!filemap_release_folio(folio, GFP_KERNEL)) {
>  		result = SCAN_PAGE_HAS_PRIVATE;
> -		folio_putback_lru(folio);
> -		goto out_unlock;
> +		goto out_putback;
>  	}
>  
>  	if (folio_mapped(folio))
>  		try_to_unmap(folio, TTU_IGNORE_MLOCK | TTU_BATCH_FLUSH);
> +	/*
> +	 * We control 2 + nr_pages references to the folio:
> +	 *  - we hold a pin on it;
> +	 *  - nr_pages reference from page cache;
> +	 *  - one from lru_isolate_folio;
> +	 * If those are the only references, then any new usage
> +	 * of the folio will have to fetch it from the page
> +	 * cache. That requires locking the folio to handle
> +	 * truncate, so any new usage will be blocked until we
> +	 * unlock folio after collapse/during rollback.
> +	 */
> +	if (folio_ref_count(folio) != 2 + folio_nr_pages(folio)) {
> +		result = SCAN_PAGE_COUNT;
> +		goto out_putback;
> +	}
> +
> +	/*
> +	 * At this point, the folio is locked and unmapped. If the PTE
> +	 * was dirty, try_to_unmap() has transferred the dirty bit to
> +	 * the folio and we must not collapse it into a clean
> +	 * file-backed folio.
> +	 *
> +	 * If the folio is clean here, no one can write it until we
> +	 * drop the folio lock. A write through a stale TLB entry came
> +	 * from a clean PTE and must fault because the PTE has been
> +	 * cleared; the fault path has to take the folio lock before
> +	 * installing a writable mapping. Buffered write paths also
> +	 * have to take the folio lock before modifying file contents
> +	 * without a mapping, typically via write_begin_get_folio().
> +	 */
> +	if (!state->is_shmem && folio_test_dirty(folio)) {
> +		result = SCAN_PAGE_DIRTY_OR_WRITEBACK;
> +		goto out_putback;
> +	}
>  	return SCAN_SUCCEED;
> +out_putback:
> +	folio_putback_lru(folio);
>  out_unlock:
>  	folio_unlock(folio);
>  	folio_put(folio);
> @@ -2513,55 +2548,11 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr,
>  		xas_lock_irq(&xas);
>  		VM_BUG_ON_FOLIO(folio != xa_load(xas.xa, index), folio);
>  
> -		/*
> -		 * We control 2 + nr_pages references to the folio:
> -		 *  - we hold a pin on it;
> -		 *  - nr_pages reference from page cache;
> -		 *  - one from lru_isolate_folio;
> -		 * If those are the only references, then any new usage
> -		 * of the folio will have to fetch it from the page
> -		 * cache. That requires locking the folio to handle
> -		 * truncate, so any new usage will be blocked until we
> -		 * unlock folio after collapse/during rollback.
> -		 */
> -		if (folio_ref_count(folio) != 2 + folio_nr_pages(folio)) {
> -			result = SCAN_PAGE_COUNT;
> -			xas_unlock_irq(&xas);
> -			folio_putback_lru(folio);
> -			goto out_unlock;
> -		}
> -
> -		/*
> -		 * At this point, the folio is locked and unmapped. If the PTE
> -		 * was dirty, try_to_unmap() has transferred the dirty bit to
> -		 * the folio and we must not collapse it into a clean
> -		 * file-backed folio.
> -		 *
> -		 * If the folio is clean here, no one can write it until we
> -		 * drop the folio lock. A write through a stale TLB entry came
> -		 * from a clean PTE and must fault because the PTE has been
> -		 * cleared; the fault path has to take the folio lock before
> -		 * installing a writable mapping. Buffered write paths also
> -		 * have to take the folio lock before modifying file contents
> -		 * without a mapping, typically via write_begin_get_folio().
> -		 */
> -		if (!is_shmem && folio_test_dirty(folio)) {
> -			result = SCAN_PAGE_DIRTY_OR_WRITEBACK;
> -			xas_unlock_irq(&xas);
> -			folio_putback_lru(folio);
> -			goto out_unlock;
> -		}
> -
>  		/*
>  		 * Accumulate the folios that are being collapsed.
>  		 */
>  		list_add_tail(&folio->lru, &pagelist);
>  		index += folio_nr_pages(folio);

This can/should also be hoisted up if you follow my recommended code flow.
This requires also passing pagelist (and index, as lorenzo already suggested) 
into the helper struct but thats fine.

> -		continue;
> -out_unlock:
> -		folio_unlock(folio);
> -		folio_put(folio);
> -		goto xa_unlocked;
>  	}
>  
>  xa_locked:

my recommendations also allow you to group all these gotos into the bottom of
the prepare stage which drastically cleanups the amount of gotos in the
collapse_file() function. 

with my code recommendations the loop becomes.

for (index = start; index < end;) {
...
-out_unlock:
-               folio_unlock(folio);
-               folio_put(folio);
-               goto xa_unlocked;
+               result = collapse_file_check_folio(folio, mapping, &xas,
+                                                  file, &index, start,
+                                                  end, is_shmem, &nr_none,
+                                                  &pagelist);
+               if (result != SCAN_SUCCEED)
+                       break;
        }
 
-xa_locked:
-       xas_unlock_irq(&xas);
-xa_unlocked:
+       if (result == SCAN_SUCCEED)
+               xas_unlock_irq(&xas);

        /*
         * If collapse is successful, flush must be done now before copying.
         * If collapse is unsuccessful, does flush actually need to be done?
         * Do it anyway, to clear the state.
         */
        try_to_unmap_flush();

        if (result == SCAN_SUCCEED && nr_none &&
            !shmem_charge(mapping->host, nr_none))
                result = SCAN_FAIL;
        if (result != SCAN_SUCCEED) {
                nr_none = 0;
                goto rollback;
        }

	//Copy stage

Now we have no weird goto labels in collapse_file and rely on results to
control the follow.

If you think my approach is already useable perhaps you can just take my
commit I pointed to you that does all the major code moving with no major
refactoring and add your further improvements on top.

let me know what you think!

Cheers,
-- Nico

-- 
Nico Pache (Red Hat) <nico.pache@xxxxxxxxx>