The current code does not protect against swapoff of the underlying
swap device, so this is a bug fix as well as a worthwhile reduction in
code complexity.
Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
---
mm/memcontrol.c | 25 ++-----------------------
1 file changed, 2 insertions(+), 23 deletions(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index b807952b4d43..4075f214a841 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -5539,35 +5539,14 @@ static struct page *mc_handle_swap_pte(struct vm_area_struct *vma,
static struct page *mc_handle_file_pte(struct vm_area_struct *vma,
unsigned long addr, pte_t ptent, swp_entry_t *entry)
{
- struct page *page = NULL;
- struct address_space *mapping;
- pgoff_t pgoff;
-
if (!vma->vm_file) /* anonymous vma */
return NULL;
if (!(mc.flags & MOVE_FILE))
return NULL;
- mapping = vma->vm_file->f_mapping;
- pgoff = linear_page_index(vma, addr);
-
/* page is moved even if it's not RSS of this task(page-faulted). */
-#ifdef CONFIG_SWAP
- /* shmem/tmpfs may report page out on swap: account for that too. */
- if (shmem_mapping(mapping)) {
- page = find_get_entry(mapping, pgoff);
- if (xa_is_value(page)) {
- swp_entry_t swp = radix_to_swp_entry(page);
- *entry = swp;
- page = find_get_page(swap_address_space(swp),
- swp_offset(swp));
- }
- } else
- page = find_get_page(mapping, pgoff);
-#else
- page = find_get_page(mapping, pgoff);
-#endif
- return page;
+ return find_get_swap_page(vma->vm_file->f_mapping,
+ linear_page_index(vma, addr));
}
/**
--
2.28.0
On Wed, Aug 19, 2020 at 07:48:44PM +0100, Matthew Wilcox (Oracle) wrote:
> The current code does not protect against swapoff of the underlying
> swap device, so this is a bug fix as well as a worthwhile reduction in
> code complexity.
>
> Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
> ---
> mm/memcontrol.c | 25 ++-----------------------
> 1 file changed, 2 insertions(+), 23 deletions(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index b807952b4d43..4075f214a841 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -5539,35 +5539,14 @@ static struct page *mc_handle_swap_pte(struct vm_area_struct *vma,
> static struct page *mc_handle_file_pte(struct vm_area_struct *vma,
> unsigned long addr, pte_t ptent, swp_entry_t *entry)
> {
> - struct page *page = NULL;
> - struct address_space *mapping;
> - pgoff_t pgoff;
> -
> if (!vma->vm_file) /* anonymous vma */
> return NULL;
> if (!(mc.flags & MOVE_FILE))
> return NULL;
>
> - mapping = vma->vm_file->f_mapping;
> - pgoff = linear_page_index(vma, addr);
> -
> /* page is moved even if it's not RSS of this task(page-faulted). */
> -#ifdef CONFIG_SWAP
> - /* shmem/tmpfs may report page out on swap: account for that too. */
> - if (shmem_mapping(mapping)) {
> - page = find_get_entry(mapping, pgoff);
> - if (xa_is_value(page)) {
> - swp_entry_t swp = radix_to_swp_entry(page);
> - *entry = swp;
> - page = find_get_page(swap_address_space(swp),
> - swp_offset(swp));
> - }
> - } else
> - page = find_get_page(mapping, pgoff);
> -#else
> - page = find_get_page(mapping, pgoff);
> -#endif
> - return page;
> + return find_get_swap_page(vma->vm_file->f_mapping,
> + linear_page_index(vma, addr));
The refactor makes sense to me, but the name is confusing. We're not
looking for a swap page, we're primarily looking for a file page in
the page cache mapping that's handed in. Only in the special case
where it's a shmem mapping and there is a swap entry do we consult the
auxiliary swap cache.
How about find_get_page_or_swapcache()? find_get_page_shmemswap()?
Maybe you have a better idea. It's a fairly specialized operation that
isn't widely used, so a longer name isn't a bad thing IMO.
On Wed, Aug 26, 2020 at 10:20:02AM -0400, Johannes Weiner wrote:
> On Wed, Aug 19, 2020 at 07:48:44PM +0100, Matthew Wilcox (Oracle) wrote:
> > + return find_get_swap_page(vma->vm_file->f_mapping,
> > + linear_page_index(vma, addr));
>
> The refactor makes sense to me, but the name is confusing. We're not
> looking for a swap page, we're primarily looking for a file page in
> the page cache mapping that's handed in. Only in the special case
> where it's a shmem mapping and there is a swap entry do we consult the
> auxiliary swap cache.
>
> How about find_get_page_or_swapcache()? find_get_page_shmemswap()?
> Maybe you have a better idea. It's a fairly specialized operation that
> isn't widely used, so a longer name isn't a bad thing IMO.
Yeah, I had trouble with the naming here too.
get_page_even_from_swap()
find_get_shmem_page()
or maybe refactor the whole thing:
struct page *page = find_get_entry(mapping, index);
page = find_swap_page(mapping, page);
struct page *find_swap_page(struct address_space *mapping, struct page *page)
{
swp_entry_t swp;
struct swap_info_struct *si;
if (!xa_is_value(page))
return page;
if (!shmem_mapping(mapping))
return NULL;
...
}
On Wed, Aug 26, 2020 at 03:54:14PM +0100, Matthew Wilcox wrote:
> On Wed, Aug 26, 2020 at 10:20:02AM -0400, Johannes Weiner wrote:
> > On Wed, Aug 19, 2020 at 07:48:44PM +0100, Matthew Wilcox (Oracle) wrote:
> > > + return find_get_swap_page(vma->vm_file->f_mapping,
> > > + linear_page_index(vma, addr));
> >
> > The refactor makes sense to me, but the name is confusing. We're not
> > looking for a swap page, we're primarily looking for a file page in
> > the page cache mapping that's handed in. Only in the special case
> > where it's a shmem mapping and there is a swap entry do we consult the
> > auxiliary swap cache.
> >
> > How about find_get_page_or_swapcache()? find_get_page_shmemswap()?
> > Maybe you have a better idea. It's a fairly specialized operation that
> > isn't widely used, so a longer name isn't a bad thing IMO.
>
> Yeah, I had trouble with the naming here too.
>
> get_page_even_from_swap()
> find_get_shmem_page()
>
> or maybe refactor the whole thing:
>
> struct page *page = find_get_entry(mapping, index);
> page = find_swap_page(mapping, page);
>
> struct page *find_swap_page(struct address_space *mapping, struct page *page)
> {
> swp_entry_t swp;
> struct swap_info_struct *si;
>
> if (!xa_is_value(page))
> return page;
> if (!shmem_mapping(mapping))
> return NULL;
>
> ...
> }
Yeah, I like the idea of two lookups if we can't find a good name for
the operation that combines them. I'd just bubble the control flow
that links them up to the callsite - that still seems plenty compact
for two callsites, and keeps all the shmem magic in shmem code:
page = find_get_entry(mapping, index);
if (xa_is_value(page))
if (shmem_mapping(mapping))
page = lookup_shmem_swap_cache(page);
else
page = NULL;
So close to making radix_to_swp_entry() & co. private to shmem.c, too
- if it weren't for force_shm_swapin_readahead(). Ah well.
On Wed, Aug 26, 2020 at 10:20:02AM -0400, Johannes Weiner wrote:
> The refactor makes sense to me, but the name is confusing. We're not
> looking for a swap page, we're primarily looking for a file page in
> the page cache mapping that's handed in. Only in the special case
> where it's a shmem mapping and there is a swap entry do we consult the
> auxiliary swap cache.
>
> How about find_get_page_or_swapcache()? find_get_page_shmemswap()?
> Maybe you have a better idea. It's a fairly specialized operation that
> isn't widely used, so a longer name isn't a bad thing IMO.
Got it. find_get_incore_page(). I was going to go with inmem, but that
it matches mincore sold me on it.
/**
* find_get_incore_page - Find and get a page from the page or swap caches.
* @mapping: The address_space to search.
* @index: The page cache index.
*
* This differs from find_get_page() in that it will also look for the
* page in the swap cache.
*
* Return: The found page or %NULL.
*/
I was focusing too much on what the function did, not why it was doing it.
On Thu, Aug 27, 2020 at 01:59:41PM +0100, Matthew Wilcox wrote:
> On Wed, Aug 26, 2020 at 10:20:02AM -0400, Johannes Weiner wrote:
> > The refactor makes sense to me, but the name is confusing. We're not
> > looking for a swap page, we're primarily looking for a file page in
> > the page cache mapping that's handed in. Only in the special case
> > where it's a shmem mapping and there is a swap entry do we consult the
> > auxiliary swap cache.
> >
> > How about find_get_page_or_swapcache()? find_get_page_shmemswap()?
> > Maybe you have a better idea. It's a fairly specialized operation that
> > isn't widely used, so a longer name isn't a bad thing IMO.
>
> Got it. find_get_incore_page(). I was going to go with inmem, but that
> it matches mincore sold me on it.
>
> /**
> * find_get_incore_page - Find and get a page from the page or swap caches.
> * @mapping: The address_space to search.
> * @index: The page cache index.
> *
> * This differs from find_get_page() in that it will also look for the
> * page in the swap cache.
> *
> * Return: The found page or %NULL.
> */
Nice work, that's perfect.
> I was focusing too much on what the function did, not why it was doing it.
Me too.