Re: [PATCH 2/8] mm: Use find_get_swap_page in memcontrol
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.
Re: [PATCH 2/8] mm: Use find_get_swap_page in memcontrol
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.
Re: [PATCH 2/8] mm: Use find_get_swap_page in memcontrol
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.
Re: [PATCH 2/8] mm: Use find_get_swap_page in memcontrol
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;
...
}
Re: [PATCH 2/8] mm: Use find_get_swap_page in memcontrol
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)
> ---
> 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.
