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

Vlastimil Babka
2021-08-11T14:50:39Z

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
>