Re: [PATCH v14 058/138] mm/swap: Add folio_mark_accessed()

[Vlastimil Babka]

On 7/15/21 5:35 AM, Matthew Wilcox (Oracle) wrote:
> Convert mark_page_accessed() to folio_mark_accessed().  It already
> operated on the entire compound page, but now we can avoid calling
> compound_head quite so many times.  Shrinks the function from 424 bytes
> to 295 bytes (shrinking by 129 bytes).  The compatibility wrapper is 30
> bytes, plus the 8 bytes for the exported symbol means the kernel shrinks
> by 91 bytes.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@xxxxxxxxxxxxx>

Acked-by: Vlastimil Babka <vbabka@xxxxxxx>

Question below:

> @@ -430,36 +430,34 @@ static void __lru_cache_activate_page(struct page *page)
>   * When a newly allocated page is not yet visible, so safe for non-atomic ops,
>   * __SetPageReferenced(page) may be substituted for mark_page_accessed(page).
>   */

The other patches converting whole functions rewrote also comments to be about
folios, but not this one?

> -void mark_page_accessed(struct page *page)
> +void folio_mark_accessed(struct folio *folio)
>  {
> -	page = compound_head(page);
> -
> -	if (!PageReferenced(page)) {
> -		SetPageReferenced(page);
> -	} else if (PageUnevictable(page)) {
> +	if (!folio_test_referenced(folio)) {
> +		folio_set_referenced(folio);
> +	} else if (folio_test_unevictable(folio)) {
>  		/*
>  		 * Unevictable pages are on the "LRU_UNEVICTABLE" list. But,
>  		 * this list is never rotated or maintained, so marking an
>  		 * evictable page accessed has no effect.
>  		 */

These comments too?

> -	} else if (!PageActive(page)) {
> +	} else if (!folio_test_active(folio)) {
>  		/*
>  		 * If the page is on the LRU, queue it for activation via
>  		 * lru_pvecs.activate_page. Otherwise, assume the page is on a
>  		 * pagevec, mark it active and it'll be moved to the active
>  		 * LRU on the next drain.
>  		 */
> -		if (PageLRU(page))
> -			folio_activate(page_folio(page));
> +		if (folio_test_lru(folio))
> +			folio_activate(folio);
>  		else
> -			__lru_cache_activate_page(page);
> -		ClearPageReferenced(page);
> -		workingset_activation(page_folio(page));
> +			__lru_cache_activate_folio(folio);
> +		folio_clear_referenced(folio);
> +		workingset_activation(folio);
>  	}
> -	if (page_is_idle(page))
> -		clear_page_idle(page);
> +	if (folio_test_idle(folio))
> +		folio_clear_idle(folio);
>  }
> -EXPORT_SYMBOL(mark_page_accessed);
> +EXPORT_SYMBOL(folio_mark_accessed);
>  
>  /**
>   * lru_cache_add - add a page to a page list
>