| Message ID | 20240423170339.54131-8-ryncsn@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/swap: optimize swap cache search space | expand |
Kairui Song <ryncsn@gmail.com> writes: > From: Kairui Song <kasong@tencent.com> > > There are four helpers for retrieving the page index within address > space, or offset within mapped file: > > - page_index > - page_file_offset > - folio_index (equivalence of page_index) > - folio_file_pos (equivalence of page_file_offset) > > And they are only needed for mixed usage of swap cache & page cache (eg. > migration, huge memory split). Else users can just use folio->index or > folio_pos. > > This commit drops page_index and page_file_offset as we have eliminated > all users, and converts folio_index and folio_file_pos (they were simply > wrappers of page_index and page_file_offset, and implemented in a not > very clean way) to use folio internally. > > After this commit, there will be only two helpers for users that may > encounter mixed usage of swap cache and page cache: > > - folio_index (calls __folio_swap_cache_index for swap cache folio) > - folio_file_pos (calls __folio_swap_dev_pos for swap cache folio) > > The index in swap cache and offset in swap device are still basically > the same thing, but will be different in following commits. > > Signed-off-by: Kairui Song <kasong@tencent.com> > --- > 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..a7d025571ee6 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_dev_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_dev_pos(folio); > + return ((loff_t)folio->index << PAGE_SHIFT); This still looks confusing for me. The function returns the byte position of the folio in its file. But we returns the swap device position of the folio. Tried to search folio_file_pos() usage. The 2 usage in page_io.c is swap specific, we can use swap_dev_pos() directly. There are also other file system users (NFS and AFS) of folio_file_pos(), I don't know why they need to work with swap cache. Cced file system maintainers for help. > } > > /* > diff --git a/mm/swapfile.c b/mm/swapfile.c > index 4919423cce76..2387f5e131d7 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_dev_pos(struct folio *folio) > +{ > + return swap_dev_pos(folio->swap); > +} > +EXPORT_SYMBOL_GPL(__folio_swap_dev_pos); > > /* > * add_swap_count_continuation - called when a swap count is duplicated -- Best Regards, Huang, Ying
On Wed, Apr 24, 2024 at 10:19 AM Huang, Ying <ying.huang@intel.com> wrote: > > Kairui Song <ryncsn@gmail.com> writes: > > > From: Kairui Song <kasong@tencent.com> > > > > There are four helpers for retrieving the page index within address > > space, or offset within mapped file: > > > > - page_index > > - page_file_offset > > - folio_index (equivalence of page_index) > > - folio_file_pos (equivalence of page_file_offset) > > > > And they are only needed for mixed usage of swap cache & page cache (eg. > > migration, huge memory split). Else users can just use folio->index or > > folio_pos. > > > > This commit drops page_index and page_file_offset as we have eliminated > > all users, and converts folio_index and folio_file_pos (they were simply > > wrappers of page_index and page_file_offset, and implemented in a not > > very clean way) to use folio internally. > > > > After this commit, there will be only two helpers for users that may > > encounter mixed usage of swap cache and page cache: > > > > - folio_index (calls __folio_swap_cache_index for swap cache folio) > > - folio_file_pos (calls __folio_swap_dev_pos for swap cache folio) > > > > The index in swap cache and offset in swap device are still basically > > the same thing, but will be different in following commits. > > > > Signed-off-by: Kairui Song <kasong@tencent.com> > > --- > > 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..a7d025571ee6 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_dev_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_dev_pos(folio); > > + return ((loff_t)folio->index << PAGE_SHIFT); > > This still looks confusing for me. The function returns the byte > position of the folio in its file. But we returns the swap device > position of the folio. Thanks for the comment. This doesn't look too confusing to me, __folio_swap_dev_pos -> swap_dev_pos also returns the byte position of the folio in swap device. If we agree swap device is kind of equivalent to the page cache file here, it shouldn't be too hard to understand. > > Tried to search folio_file_pos() usage. The 2 usage in page_io.c is > swap specific, we can use swap_dev_pos() directly. The 2 usage in page_io.c is already converted to use swap_dev_pos directly in this series (patch 6/8). > > There are also other file system users (NFS and AFS) of > folio_file_pos(), I don't know why they need to work with swap > cache. Cced file system maintainers for help. > Thanks, I'm not very sure if we can just drop folio_file_pos and convert all users to use folio_pos directly. Swap cache mapping shouldn't be exposed to fs, but I'm not confident enough that this is a safe move. It looks OK to do so just by examining NFS code, but let's wait for feedback from FS people.
On Wed, Apr 24, 2024 at 10:17:04AM +0800, Huang, Ying wrote: > Kairui Song <ryncsn@gmail.com> writes: > > 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_dev_pos(folio); > > + return ((loff_t)folio->index << PAGE_SHIFT); > > This still looks confusing for me. The function returns the byte > position of the folio in its file. But we returns the swap device > position of the folio. > > Tried to search folio_file_pos() usage. The 2 usage in page_io.c is > swap specific, we can use swap_dev_pos() directly. > > There are also other file system users (NFS and AFS) of > folio_file_pos(), I don't know why they need to work with swap > cache. Cced file system maintainers for help. Time for a history lesson! In d56b4ddf7781 (2012) we introduced page_file_index() and page_file_mapping() to support swap-over-NFS. Writes to the swapfile went through ->direct_IO but reads went through ->readpage. So NFS was changed to remove direct references to page->mapping and page->index because those aren't right for anon pages (or shmem pages being swapped out). In e1209d3a7a67 (2022), we stopped using ->readpage in favour of using ->swap_rw. Now we don't need to use page_file_*(); we get the swap_file and ki_pos directly in the swap_iocb. But there are still relics in NFS that nobody has dared rip out. And there are all the copy-and-pasted filesystems that use page_file_* because they don't know any better. We should delete page_file_*() and folio_file_*(). They shouldn't be needed any more.
Hi, Matthew, Matthew Wilcox <willy@infradead.org> writes: > On Wed, Apr 24, 2024 at 10:17:04AM +0800, Huang, Ying wrote: >> Kairui Song <ryncsn@gmail.com> writes: >> > 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_dev_pos(folio); >> > + return ((loff_t)folio->index << PAGE_SHIFT); >> >> This still looks confusing for me. The function returns the byte >> position of the folio in its file. But we returns the swap device >> position of the folio. >> >> Tried to search folio_file_pos() usage. The 2 usage in page_io.c is >> swap specific, we can use swap_dev_pos() directly. >> >> There are also other file system users (NFS and AFS) of >> folio_file_pos(), I don't know why they need to work with swap >> cache. Cced file system maintainers for help. > > Time for a history lesson! > > In d56b4ddf7781 (2012) we introduced page_file_index() and > page_file_mapping() to support swap-over-NFS. Writes to the swapfile went > through ->direct_IO but reads went through ->readpage. So NFS was changed > to remove direct references to page->mapping and page->index because > those aren't right for anon pages (or shmem pages being swapped out). > > In e1209d3a7a67 (2022), we stopped using ->readpage in favour of using > ->swap_rw. Now we don't need to use page_file_*(); we get the swap_file > and ki_pos directly in the swap_iocb. But there are still relics in NFS > that nobody has dared rip out. And there are all the copy-and-pasted > filesystems that use page_file_* because they don't know any better. > > We should delete page_file_*() and folio_file_*(). They shouldn't be > needed any more. Thanks a lot for your detailed explanation! Yes, this will simplify the semantics and improve the readability of the corresponding code. -- Best Regards, Huang, Ying
On Wed, Apr 24, 2024 at 12:06 PM Matthew Wilcox <willy@infradead.org> wrote: > > On Wed, Apr 24, 2024 at 10:17:04AM +0800, Huang, Ying wrote: > > Kairui Song <ryncsn@gmail.com> writes: > > > 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_dev_pos(folio); > > > + return ((loff_t)folio->index << PAGE_SHIFT); > > > > This still looks confusing for me. The function returns the byte > > position of the folio in its file. But we returns the swap device > > position of the folio. > > > > Tried to search folio_file_pos() usage. The 2 usage in page_io.c is > > swap specific, we can use swap_dev_pos() directly. > > > > There are also other file system users (NFS and AFS) of > > folio_file_pos(), I don't know why they need to work with swap > > cache. Cced file system maintainers for help. > > Time for a history lesson! > > In d56b4ddf7781 (2012) we introduced page_file_index() and > page_file_mapping() to support swap-over-NFS. Writes to the swapfile went > through ->direct_IO but reads went through ->readpage. So NFS was changed > to remove direct references to page->mapping and page->index because > those aren't right for anon pages (or shmem pages being swapped out). > > In e1209d3a7a67 (2022), we stopped using ->readpage in favour of using > ->swap_rw. Now we don't need to use page_file_*(); we get the swap_file > and ki_pos directly in the swap_iocb. But there are still relics in NFS > that nobody has dared rip out. And there are all the copy-and-pasted > filesystems that use page_file_* because they don't know any better. > > We should delete page_file_*() and folio_file_*(). They shouldn't be > needed any more. Thanks for the explanation! I'll update the series, and just delete paeg_file_offset and folio_file_pos with more auditing, to make the code cleaner. Should I add a suggest-by for the removal?
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..a7d025571ee6 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_dev_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_dev_pos(folio); + return ((loff_t)folio->index << PAGE_SHIFT); } /* diff --git a/mm/swapfile.c b/mm/swapfile.c index 4919423cce76..2387f5e131d7 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_dev_pos(struct folio *folio) +{ + return swap_dev_pos(folio->swap); +} +EXPORT_SYMBOL_GPL(__folio_swap_dev_pos); /* * add_swap_count_continuation - called when a swap count is duplicated