| Message ID | 20240417160842.76665-8-ryncsn@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/swap: optimize swap cache search space | expand |
On Thu, Apr 18, 2024 at 4:12 AM Kairui Song <ryncsn@gmail.com> wrote: > > From: Kairui Song <kasong@tencent.com> > > When applied on swap cache pages, page_index / page_file_offset was used > to retrieve the swap cache index or swap file offset of a page, and they > have their folio equivalence version: folio_index / folio_file_pos. > > We have eliminated all users for page_index / page_file_offset, everything > is using folio_index / folio_file_pos now, so remove the old helpers. > > Then convert the implementation of folio_index / folio_file_pos to > to use folio natively. > > After this commit, all users that might encounter mixed usage of swap > cache and page cache will only use following two helpers: > > folio_index (calls __folio_swap_cache_index) > folio_file_pos (calls __folio_swap_file_pos) > > The offset in swap file and index in swap cache is still basically the > same thing at this moment, but will be different in following commits. > > Signed-off-by: Kairui Song <kasong@tencent.com> Hi Kairui, thanks ! I also find it rather odd that folio_file_page() is utilized for both swp and file. mm/memory.c <<do_swap_page>> page = folio_file_page(folio, swp_offset(entry)); mm/swap_state.c <<swapin_readahead>> return folio_file_page(folio, swp_offset(entry)); mm/swapfile.c <<unuse_pte>> page = folio_file_page(folio, swp_offset(entry)); Do you believe it's worthwhile to tidy up? > --- > include/linux/mm.h | 13 ------------- > include/linux/pagemap.h | 19 +++++++++---------- > mm/swapfile.c | 13 +++++++++---- > 3 files changed, 18 insertions(+), 27 deletions(-) > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index 0436b919f1c7..797480e76c9c 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -2245,19 +2245,6 @@ static inline void *folio_address(const struct folio *folio) > return page_address(&folio->page); > } > > -extern pgoff_t __page_file_index(struct page *page); > - > -/* > - * Return the pagecache index of the passed page. Regular pagecache pages > - * use ->index whereas swapcache pages use swp_offset(->private) > - */ > -static inline pgoff_t page_index(struct page *page) > -{ > - if (unlikely(PageSwapCache(page))) > - return __page_file_index(page); > - return page->index; > -} > - > /* > * Return true only if the page has been allocated with > * ALLOC_NO_WATERMARKS and the low watermark was not > diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h > index 2df35e65557d..313f3144406e 100644 > --- a/include/linux/pagemap.h > +++ b/include/linux/pagemap.h > @@ -780,7 +780,7 @@ static inline struct page *grab_cache_page_nowait(struct address_space *mapping, > mapping_gfp_mask(mapping)); > } > > -#define swapcache_index(folio) __page_file_index(&(folio)->page) > +extern pgoff_t __folio_swap_cache_index(struct folio *folio); > > /** > * folio_index - File index of a folio. > @@ -795,9 +795,9 @@ static inline struct page *grab_cache_page_nowait(struct address_space *mapping, > */ > static inline pgoff_t folio_index(struct folio *folio) > { > - if (unlikely(folio_test_swapcache(folio))) > - return swapcache_index(folio); > - return folio->index; > + if (unlikely(folio_test_swapcache(folio))) > + return __folio_swap_cache_index(folio); > + return folio->index; > } > > /** > @@ -920,11 +920,6 @@ static inline loff_t page_offset(struct page *page) > return ((loff_t)page->index) << PAGE_SHIFT; > } > > -static inline loff_t page_file_offset(struct page *page) > -{ > - return ((loff_t)page_index(page)) << PAGE_SHIFT; > -} > - > /** > * folio_pos - Returns the byte position of this folio in its file. > * @folio: The folio. > @@ -934,6 +929,8 @@ static inline loff_t folio_pos(struct folio *folio) > return page_offset(&folio->page); > } > > +extern loff_t __folio_swap_file_pos(struct folio *folio); > + > /** > * folio_file_pos - Returns the byte position of this folio in its file. > * @folio: The folio. > @@ -943,7 +940,9 @@ static inline loff_t folio_pos(struct folio *folio) > */ > static inline loff_t folio_file_pos(struct folio *folio) > { > - return page_file_offset(&folio->page); > + if (unlikely(folio_test_swapcache(folio))) > + return __folio_swap_file_pos(folio); > + return ((loff_t)folio->index << PAGE_SHIFT); > } > > /* > diff --git a/mm/swapfile.c b/mm/swapfile.c > index 4919423cce76..0c36a5c2400f 100644 > --- a/mm/swapfile.c > +++ b/mm/swapfile.c > @@ -3419,12 +3419,17 @@ struct address_space *swapcache_mapping(struct folio *folio) > } > EXPORT_SYMBOL_GPL(swapcache_mapping); > > -pgoff_t __page_file_index(struct page *page) > +pgoff_t __folio_swap_cache_index(struct folio *folio) > { > - swp_entry_t swap = page_swap_entry(page); > - return swp_offset(swap); > + return swp_offset(folio->swap); > } > -EXPORT_SYMBOL_GPL(__page_file_index); > +EXPORT_SYMBOL_GPL(__folio_swap_cache_index); > + > +loff_t __folio_swap_file_pos(struct folio *folio) > +{ > + return swap_file_pos(folio->swap); > +} > +EXPORT_SYMBOL_GPL(__folio_swap_file_pos); > > /* > * add_swap_count_continuation - called when a swap count is duplicated > -- > 2.44.0 > Thanks Barry
On Thu, Apr 18, 2024 at 9:55 AM Barry Song <21cnbao@gmail.com> wrote: > > On Thu, Apr 18, 2024 at 4:12 AM Kairui Song <ryncsn@gmail.com> wrote: > > > > From: Kairui Song <kasong@tencent.com> > > > > When applied on swap cache pages, page_index / page_file_offset was used > > to retrieve the swap cache index or swap file offset of a page, and they > > have their folio equivalence version: folio_index / folio_file_pos. > > > > We have eliminated all users for page_index / page_file_offset, everything > > is using folio_index / folio_file_pos now, so remove the old helpers. > > > > Then convert the implementation of folio_index / folio_file_pos to > > to use folio natively. > > > > After this commit, all users that might encounter mixed usage of swap > > cache and page cache will only use following two helpers: > > > > folio_index (calls __folio_swap_cache_index) > > folio_file_pos (calls __folio_swap_file_pos) > > > > The offset in swap file and index in swap cache is still basically the > > same thing at this moment, but will be different in following commits. > > > > Signed-off-by: Kairui Song <kasong@tencent.com> > > Hi Kairui, thanks ! > > I also find it rather odd that folio_file_page() is utilized for both > swp and file. > > mm/memory.c <<do_swap_page>> > page = folio_file_page(folio, swp_offset(entry)); > mm/swap_state.c <<swapin_readahead>> > return folio_file_page(folio, swp_offset(entry)); > mm/swapfile.c <<unuse_pte>> > page = folio_file_page(folio, swp_offset(entry)); > > Do you believe it's worthwhile to tidy up? > Hi Barry, I'm not sure about this. Using folio_file_page doesn't look too bad, and it will be gone once we convert them to always use folio, this shouldn't take too long.
On Thu, Apr 18, 2024 at 01:55:28PM +1200, Barry Song wrote: > I also find it rather odd that folio_file_page() is utilized for both > swp and file. > > mm/memory.c <<do_swap_page>> > page = folio_file_page(folio, swp_offset(entry)); > mm/swap_state.c <<swapin_readahead>> > return folio_file_page(folio, swp_offset(entry)); > mm/swapfile.c <<unuse_pte>> > page = folio_file_page(folio, swp_offset(entry)); > > Do you believe it's worthwhile to tidy up? Why do you find it odd? What would you propose instead?
On Thu, Apr 18, 2024 at 3:31 PM Matthew Wilcox <willy@infradead.org> wrote: > > On Thu, Apr 18, 2024 at 01:55:28PM +1200, Barry Song wrote: > > I also find it rather odd that folio_file_page() is utilized for both > > swp and file. > > > > mm/memory.c <<do_swap_page>> > > page = folio_file_page(folio, swp_offset(entry)); > > mm/swap_state.c <<swapin_readahead>> > > return folio_file_page(folio, swp_offset(entry)); > > mm/swapfile.c <<unuse_pte>> > > page = folio_file_page(folio, swp_offset(entry)); > > > > Do you believe it's worthwhile to tidy up? > > Why do you find it odd? What would you propose instead? i'd prefer something like folio_swap_page(folio, entry);
On Thu, Apr 18, 2024 at 2:42 PM Kairui Song <ryncsn@gmail.com> wrote: > > On Thu, Apr 18, 2024 at 9:55 AM Barry Song <21cnbao@gmail.com> wrote: > > > > On Thu, Apr 18, 2024 at 4:12 AM Kairui Song <ryncsn@gmail.com> wrote: > > > > > > From: Kairui Song <kasong@tencent.com> > > > > > > When applied on swap cache pages, page_index / page_file_offset was used > > > to retrieve the swap cache index or swap file offset of a page, and they > > > have their folio equivalence version: folio_index / folio_file_pos. > > > > > > We have eliminated all users for page_index / page_file_offset, everything > > > is using folio_index / folio_file_pos now, so remove the old helpers. > > > > > > Then convert the implementation of folio_index / folio_file_pos to > > > to use folio natively. > > > > > > After this commit, all users that might encounter mixed usage of swap > > > cache and page cache will only use following two helpers: > > > > > > folio_index (calls __folio_swap_cache_index) > > > folio_file_pos (calls __folio_swap_file_pos) > > > > > > The offset in swap file and index in swap cache is still basically the > > > same thing at this moment, but will be different in following commits. > > > > > > Signed-off-by: Kairui Song <kasong@tencent.com> > > > > Hi Kairui, thanks ! > > > > I also find it rather odd that folio_file_page() is utilized for both > > swp and file. > > > > mm/memory.c <<do_swap_page>> > > page = folio_file_page(folio, swp_offset(entry)); > > mm/swap_state.c <<swapin_readahead>> > > return folio_file_page(folio, swp_offset(entry)); > > mm/swapfile.c <<unuse_pte>> > > page = folio_file_page(folio, swp_offset(entry)); > > > > Do you believe it's worthwhile to tidy up? > > > > Hi Barry, > > I'm not sure about this. Using folio_file_page doesn't look too bad, > and it will be gone once we convert them to always use folio, this > shouldn't take too long. HI Kairui, I am not quite sure this is going to be quite soon. our swap-in large folios refault still have corner cases which can't map large folios even we hit large folios in swapcache [1]. personally, i feel do_swap_page() isn't handling file, and those pages are anon but not file. so a separate helper taking folio and entry as arguments seem more readable as Chris even wants to remove the assumption large folios have been to swapped to contiguous swap offsets. anyway, it is just me :-） [1] https://lore.kernel.org/linux-mm/20240409082631.187483-1-21cnbao@gmail.com/
diff --git a/include/linux/mm.h b/include/linux/mm.h index 0436b919f1c7..797480e76c9c 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -2245,19 +2245,6 @@ static inline void *folio_address(const struct folio *folio) return page_address(&folio->page); } -extern pgoff_t __page_file_index(struct page *page); - -/* - * Return the pagecache index of the passed page. Regular pagecache pages - * use ->index whereas swapcache pages use swp_offset(->private) - */ -static inline pgoff_t page_index(struct page *page) -{ - if (unlikely(PageSwapCache(page))) - return __page_file_index(page); - return page->index; -} - /* * Return true only if the page has been allocated with * ALLOC_NO_WATERMARKS and the low watermark was not diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h index 2df35e65557d..313f3144406e 100644 --- a/include/linux/pagemap.h +++ b/include/linux/pagemap.h @@ -780,7 +780,7 @@ static inline struct page *grab_cache_page_nowait(struct address_space *mapping, mapping_gfp_mask(mapping)); } -#define swapcache_index(folio) __page_file_index(&(folio)->page) +extern pgoff_t __folio_swap_cache_index(struct folio *folio); /** * folio_index - File index of a folio. @@ -795,9 +795,9 @@ static inline struct page *grab_cache_page_nowait(struct address_space *mapping, */ static inline pgoff_t folio_index(struct folio *folio) { - if (unlikely(folio_test_swapcache(folio))) - return swapcache_index(folio); - return folio->index; + if (unlikely(folio_test_swapcache(folio))) + return __folio_swap_cache_index(folio); + return folio->index; } /** @@ -920,11 +920,6 @@ static inline loff_t page_offset(struct page *page) return ((loff_t)page->index) << PAGE_SHIFT; } -static inline loff_t page_file_offset(struct page *page) -{ - return ((loff_t)page_index(page)) << PAGE_SHIFT; -} - /** * folio_pos - Returns the byte position of this folio in its file. * @folio: The folio. @@ -934,6 +929,8 @@ static inline loff_t folio_pos(struct folio *folio) return page_offset(&folio->page); } +extern loff_t __folio_swap_file_pos(struct folio *folio); + /** * folio_file_pos - Returns the byte position of this folio in its file. * @folio: The folio. @@ -943,7 +940,9 @@ static inline loff_t folio_pos(struct folio *folio) */ static inline loff_t folio_file_pos(struct folio *folio) { - return page_file_offset(&folio->page); + if (unlikely(folio_test_swapcache(folio))) + return __folio_swap_file_pos(folio); + return ((loff_t)folio->index << PAGE_SHIFT); } /* diff --git a/mm/swapfile.c b/mm/swapfile.c index 4919423cce76..0c36a5c2400f 100644 --- a/mm/swapfile.c +++ b/mm/swapfile.c @@ -3419,12 +3419,17 @@ struct address_space *swapcache_mapping(struct folio *folio) } EXPORT_SYMBOL_GPL(swapcache_mapping); -pgoff_t __page_file_index(struct page *page) +pgoff_t __folio_swap_cache_index(struct folio *folio) { - swp_entry_t swap = page_swap_entry(page); - return swp_offset(swap); + return swp_offset(folio->swap); } -EXPORT_SYMBOL_GPL(__page_file_index); +EXPORT_SYMBOL_GPL(__folio_swap_cache_index); + +loff_t __folio_swap_file_pos(struct folio *folio) +{ + return swap_file_pos(folio->swap); +} +EXPORT_SYMBOL_GPL(__folio_swap_file_pos); /* * add_swap_count_continuation - called when a swap count is duplicated