diff mbox series

[11/11] mm/vmscan: Allow arbitrary sized pages to be paged out

Message ID 20200908195539.25896-12-willy@infradead.org
State New
Headers show
Series Remove assumptions of THP size | expand

Commit Message

Matthew Wilcox Sept. 8, 2020, 7:55 p.m. UTC
Remove the assumption that a compound page has HPAGE_PMD_NR pins from
the page cache.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Cc: Huang Ying <ying.huang@intel.com>
---
 mm/vmscan.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Kirill A. Shutemov Sept. 9, 2020, 2:55 p.m. UTC | #1
On Tue, Sep 08, 2020 at 08:55:38PM +0100, Matthew Wilcox (Oracle) wrote:
> Remove the assumption that a compound page has HPAGE_PMD_NR pins from
> the page cache.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> Cc: Huang Ying <ying.huang@intel.com>

Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
SeongJae Park Sept. 15, 2020, 7:40 a.m. UTC | #2
On Tue,  8 Sep 2020 20:55:38 +0100 "Matthew Wilcox (Oracle)" <willy@infradead.org> wrote:

> Remove the assumption that a compound page has HPAGE_PMD_NR pins from
> the page cache.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> Cc: Huang Ying <ying.huang@intel.com>
> ---
>  mm/vmscan.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 99e1796eb833..91b787fff71a 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -722,8 +722,7 @@ static inline int is_page_cache_freeable(struct page *page)
>  	 * that isolated the page, the page cache and optional buffer
>  	 * heads at page->private.
>  	 */
> -	int page_cache_pins = PageTransHuge(page) && PageSwapCache(page) ?
> -		HPAGE_PMD_NR : 1;
> +	int page_cache_pins = thp_nr_pages(page);

Is it ok to remove the PageSwapCache() check?


Thanks,
SeongJae Park
Matthew Wilcox Sept. 15, 2020, 12:52 p.m. UTC | #3
On Tue, Sep 15, 2020 at 09:40:45AM +0200, SeongJae Park wrote:
> On Tue,  8 Sep 2020 20:55:38 +0100 "Matthew Wilcox (Oracle)" <willy@infradead.org> wrote:
> > Remove the assumption that a compound page has HPAGE_PMD_NR pins from
> > the page cache.
> > 
> > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> > Cc: Huang Ying <ying.huang@intel.com>

> > -	int page_cache_pins = PageTransHuge(page) && PageSwapCache(page) ?
> > -		HPAGE_PMD_NR : 1;
> > +	int page_cache_pins = thp_nr_pages(page);
> 
> Is it ok to remove the PageSwapCache() check?

I think so?  My understanding is that it was added in commit bd4c82c22c36
to catch shmem pages, but there was really no reason to only do this for
shmem pages.
Huang\, Ying Sept. 16, 2020, 1:40 a.m. UTC | #4
Matthew Wilcox <willy@infradead.org> writes:

> On Tue, Sep 15, 2020 at 09:40:45AM +0200, SeongJae Park wrote:
>> On Tue,  8 Sep 2020 20:55:38 +0100 "Matthew Wilcox (Oracle)" <willy@infradead.org> wrote:
>> > Remove the assumption that a compound page has HPAGE_PMD_NR pins from
>> > the page cache.
>> > 
>> > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
>> > Cc: Huang Ying <ying.huang@intel.com>
>
>> > -	int page_cache_pins = PageTransHuge(page) && PageSwapCache(page) ?
>> > -		HPAGE_PMD_NR : 1;
>> > +	int page_cache_pins = thp_nr_pages(page);
>> 
>> Is it ok to remove the PageSwapCache() check?
>
> I think so?  My understanding is that it was added in commit bd4c82c22c36
> to catch shmem pages, but there was really no reason to only do this for
> shmem pages.

The original implementation is to write out Anonymous THP (not shmem).
The code should work after the changing, because now any THP except
normal Anonymous THP in swap cache will be split during reclaiming
already.

Acked-by: "Huang, Ying" <ying.huang@intel.com>

Best Regards,
Huang, Ying
SeongJae Park Sept. 16, 2020, 6:09 a.m. UTC | #5
On Wed, 16 Sep 2020 09:40:10 +0800 "Huang\, Ying" <ying.huang@intel.com> wrote:

> Matthew Wilcox <willy@infradead.org> writes:
> 
> > On Tue, Sep 15, 2020 at 09:40:45AM +0200, SeongJae Park wrote:
> >> On Tue,  8 Sep 2020 20:55:38 +0100 "Matthew Wilcox (Oracle)" <willy@infradead.org> wrote:
> >> > Remove the assumption that a compound page has HPAGE_PMD_NR pins from
> >> > the page cache.
> >> > 
> >> > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> >> > Cc: Huang Ying <ying.huang@intel.com>
> >
> >> > -	int page_cache_pins = PageTransHuge(page) && PageSwapCache(page) ?
> >> > -		HPAGE_PMD_NR : 1;
> >> > +	int page_cache_pins = thp_nr_pages(page);
> >> 
> >> Is it ok to remove the PageSwapCache() check?
> >
> > I think so?  My understanding is that it was added in commit bd4c82c22c36
> > to catch shmem pages, but there was really no reason to only do this for
> > shmem pages.
> 
> The original implementation is to write out Anonymous THP (not shmem).
> The code should work after the changing, because now any THP except
> normal Anonymous THP in swap cache will be split during reclaiming
> already.

Thanks for the kind explanation :)

> 
> Acked-by: "Huang, Ying" <ying.huang@intel.com>

Reviewed-by: SeongJae Park <sjpark@amazon.de>


Thanks,
SeongJae Park
Matthew Wilcox Sept. 30, 2020, 12:13 p.m. UTC | #6
On Wed, Sep 16, 2020 at 09:40:10AM +0800, Huang, Ying wrote:
> Matthew Wilcox <willy@infradead.org> writes:
> > On Tue, Sep 15, 2020 at 09:40:45AM +0200, SeongJae Park wrote:
> >> On Tue,  8 Sep 2020 20:55:38 +0100 "Matthew Wilcox (Oracle)" <willy@infradead.org> wrote:
> >> > Remove the assumption that a compound page has HPAGE_PMD_NR pins from
> >> > the page cache.
> >> > 
> >> > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> >> > Cc: Huang Ying <ying.huang@intel.com>
> >
> >> > -	int page_cache_pins = PageTransHuge(page) && PageSwapCache(page) ?
> >> > -		HPAGE_PMD_NR : 1;
> >> > +	int page_cache_pins = thp_nr_pages(page);
> >> 
> >> Is it ok to remove the PageSwapCache() check?
> >
> > I think so?  My understanding is that it was added in commit bd4c82c22c36
> > to catch shmem pages, but there was really no reason to only do this for
> > shmem pages.
> 
> The original implementation is to write out Anonymous THP (not shmem).
> The code should work after the changing, because now any THP except
> normal Anonymous THP in swap cache will be split during reclaiming
> already.

Actually, that's a problem I just hit.  Simple to reproduce:

git clone git://git.infradead.org/users/willy/pagecache.git
build it, boot it:
mkdir /mnt/scratch; mkfs.xfs /dev/sdb; mount /dev/sdb /mnt/scratch/; dd if=/dev/zero of=/mnt/scratch/bigfile count=2000 bs=1M; umount /mnt/scratch/; mount /dev/sdb /mnt/scratch/; cat /mnt/scratch/bigfile >/dev/null

(the virtual machine i'm using only has 2GB of memory so this forces
vmscan to happen).  Anyway, we quickly run into OOM and get this kind
of report:

 active_anon:307 inactive_anon:4137 isolated_anon:0
  active_file:0 inactive_file:436964 isolated_file:192
  unevictable:0 dirty:0 writeback:0
  slab_reclaimable:3774 slab_unreclaimable:4132
  mapped:40 shmem:320 pagetables:167 bounce:0
  free:24315 free_pcp:0 free_cma:0

A little debugging shows split_huge_page_to_list() is failing because
the page still has page_private set, so the refcount is one higher
than expected.  This patch makes the problem go away:

@@ -1271,10 +1271,6 @@ static unsigned int shrink_page_list(struct list_head *page_list,
                                /* Adding to swap updated mapping */
                                mapping = page_mapping(page);
                        }
-               } else if (unlikely(PageTransHuge(page))) {
-                       /* Split file THP */
-                       if (split_huge_page_to_list(page, page_list))
-                               goto keep_locked;
                }
 
                /*

but I'm not sure what that's going to do to tmpfs/swap.  Could you guide
me here?
diff mbox series

Patch

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 99e1796eb833..91b787fff71a 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -722,8 +722,7 @@  static inline int is_page_cache_freeable(struct page *page)
 	 * that isolated the page, the page cache and optional buffer
 	 * heads at page->private.
 	 */
-	int page_cache_pins = PageTransHuge(page) && PageSwapCache(page) ?
-		HPAGE_PMD_NR : 1;
+	int page_cache_pins = thp_nr_pages(page);
 	return page_count(page) - page_has_private(page) == 1 + page_cache_pins;
 }