Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v14 058/138] mm/swap: Add folio_mark_accessed()
To:     "Matthew Wilcox (Oracle)" <willy@infradead.org>,
        linux-kernel@vger.kernel.org
Cc:     linux-mm@kvack.org, linux-fsdevel@vger.kernel.org
References: <20210715033704.692967-1-willy@infradead.org>
 <20210715033704.692967-59-willy@infradead.org>
From:   Vlastimil Babka <vbabka@suse.cz>
Message-ID: <114cd25f-8c75-9a7a-f46d-60c31685a055@suse.cz>
Date:   Wed, 11 Aug 2021 16:50:35 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.12.0
MIME-Version: 1.0
In-Reply-To: <20210715033704.692967-59-willy@infradead.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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@infradead.org>

Acked-by: Vlastimil Babka <vbabka@suse.cz>

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
>