[PATCH] MM: handle THP in swap_*page_fs() - count_vm_events()

[PATCH] MM: handle THP in swap_*page_fs() - count_vm_events()

[NeilBrown]

We need to use count_swpout_vm_event() for sio_write_complete() and
sio_read_complete(), to get correct counting.

This patch should be squased into
    MM: handle THP in swap_*page_fs()

Reported-by: Miaohe Lin <linmiaohe@huawei.com>
Signed-off-by: NeilBrown <neilb@suse.de>
---
 mm/page_io.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/mm/page_io.c b/mm/page_io.c
index d636a3531cad..3e2e9029ce50 100644
--- a/mm/page_io.c
+++ b/mm/page_io.c
@@ -280,8 +280,10 @@ static void sio_write_complete(struct kiocb *iocb, long ret)
 			set_page_dirty(page);
 			ClearPageReclaim(page);
 		}
-	} else
-		count_vm_events(PSWPOUT, sio->pages);
+	} else {
+		for (p = 0; p < sio->pages; p++)
+			count_swpout_vm_event(sio->bvec[p].bv_page);
+	}
 
 	for (p = 0; p < sio->pages; p++)
 		end_page_writeback(sio->bvec[p].bv_page);
@@ -390,9 +392,9 @@ static void sio_read_complete(struct kiocb *iocb, long ret)
 			struct page *page = sio->bvec[p].bv_page;
 
 			SetPageUptodate(page);
+			count_swpout_vm_event(page);
 			unlock_page(page);
 		}
-		count_vm_events(PSWPIN, sio->pages);
 	} else {
 		for (p = 0; p < sio->pages; p++) {
 			struct page *page = sio->bvec[p].bv_page;
-- 
2.36.0

Re: [PATCH] MM: handle THP in swap_*page_fs() - count_vm_events()

[Matthew Wilcox]

On Mon, May 02, 2022 at 02:57:46PM +1000, NeilBrown wrote:
> @@ -390,9 +392,9 @@ static void sio_read_complete(struct kiocb *iocb, long ret)
>  			struct page *page = sio->bvec[p].bv_page;
>  
>  			SetPageUptodate(page);
> +			count_swpout_vm_event(page);
>  			unlock_page(page);
>  		}
> -		count_vm_events(PSWPIN, sio->pages);

Surely that should be count_swpIN_vm_event?

Re: [PATCH] MM: handle THP in swap_*page_fs() - count_vm_events()

[NeilBrown]

On Mon, 02 May 2022, Matthew Wilcox wrote:
> On Mon, May 02, 2022 at 02:57:46PM +1000, NeilBrown wrote:
> > @@ -390,9 +392,9 @@ static void sio_read_complete(struct kiocb *iocb, long ret)
> >  			struct page *page = sio->bvec[p].bv_page;
> >  
> >  			SetPageUptodate(page);
> > +			count_swpout_vm_event(page);
> >  			unlock_page(page);
> >  		}
> > -		count_vm_events(PSWPIN, sio->pages);
> 
> Surely that should be count_swpIN_vm_event?
> 
I'm not having a good day....

Certainly shouldn't be swpout.  There isn't a count_swpin_vm_event().

swap_readpage() only counts once for each page no matter how big it is.
While swap_writepage() counts one for each PAGE_SIZE written.

And we have THP_SWPOUT but not THP_SWPIN

And I cannot find where any of these counters are documents, so I cannot
say what is "correct".

Well.... arch/s390/appldata/appldata_mem.c says
	u64 pswpin;		/* pages swapped in  */
	u64 pswpout;		/* pages swapped out */

but that isn't exactly unambiguous, and is for s390 which doesn't
support THP_SWAP

Ho hum.  I guess I put that back as it was.

Thanks for the review!!!

NeilBrown

[PATCH v2] MM: handle THP in swap_*page_fs() - count_vm_events()

[NeilBrown]

We need to use count_swpout_vm_event() for sio_write_complete() to get
correct counting.

Note that THP swap in (if it ever happens) is current accounted 1 for
each page, whether HUGE or normal.  This is different from swap-out
accounting.

This patch should be squashed into
    MM: handle THP in swap_*page_fs()

Reported-by: Miaohe Lin <linmiaohe@huawei.com>
Signed-off-by: NeilBrown <neilb@suse.de>
---
 mm/page_io.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/mm/page_io.c b/mm/page_io.c
index d636a3531cad..1b8075ef3418 100644
--- a/mm/page_io.c
+++ b/mm/page_io.c
@@ -280,8 +280,10 @@ static void sio_write_complete(struct kiocb *iocb, long ret)
 			set_page_dirty(page);
 			ClearPageReclaim(page);
 		}
-	} else
-		count_vm_events(PSWPOUT, sio->pages);
+	} else {
+		for (p = 0; p < sio->pages; p++)
+			count_swpout_vm_event(sio->bvec[p].bv_page);
+	}
 
 	for (p = 0; p < sio->pages; p++)
 		end_page_writeback(sio->bvec[p].bv_page);
-- 
2.36.0

Re: [PATCH] MM: handle THP in swap_*page_fs() - count_vm_events()

[Matthew Wilcox]

On Mon, May 02, 2022 at 03:28:49PM +1000, NeilBrown wrote:
> On Mon, 02 May 2022, Matthew Wilcox wrote:
> > On Mon, May 02, 2022 at 02:57:46PM +1000, NeilBrown wrote:
> > > @@ -390,9 +392,9 @@ static void sio_read_complete(struct kiocb *iocb, long ret)
> > >  			struct page *page = sio->bvec[p].bv_page;
> > >  
> > >  			SetPageUptodate(page);
> > > +			count_swpout_vm_event(page);
> > >  			unlock_page(page);
> > >  		}
> > > -		count_vm_events(PSWPIN, sio->pages);
> > 
> > Surely that should be count_swpIN_vm_event?
> > 
> I'm not having a good day....
> 
> Certainly shouldn't be swpout.  There isn't a count_swpin_vm_event().
> 
> swap_readpage() only counts once for each page no matter how big it is.
> While swap_writepage() counts one for each PAGE_SIZE written.
> 
> And we have THP_SWPOUT but not THP_SWPIN

_If_ I understand the swap-in patch correctly (at least as invoked by
shmem), it won't attempt to swap in an entire THP.  Even if it swapped
out an order-9 page, it will bring in order-0 pages from swap, and then
rely on khugepaged to reassemble them.

Someone who actually understands the swap code should check that my
explanation here is correct.

Re: [PATCH] MM: handle THP in swap_*page_fs() - count_vm_events()

[Yang Shi]

On Sun, May 1, 2022 at 10:32 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Mon, May 02, 2022 at 03:28:49PM +1000, NeilBrown wrote:
> > On Mon, 02 May 2022, Matthew Wilcox wrote:
> > > On Mon, May 02, 2022 at 02:57:46PM +1000, NeilBrown wrote:
> > > > @@ -390,9 +392,9 @@ static void sio_read_complete(struct kiocb *iocb, long ret)
> > > >                   struct page *page = sio->bvec[p].bv_page;
> > > >
> > > >                   SetPageUptodate(page);
> > > > +                 count_swpout_vm_event(page);
> > > >                   unlock_page(page);
> > > >           }
> > > > -         count_vm_events(PSWPIN, sio->pages);
> > >
> > > Surely that should be count_swpIN_vm_event?
> > >
> > I'm not having a good day....
> >
> > Certainly shouldn't be swpout.  There isn't a count_swpin_vm_event().
> >
> > swap_readpage() only counts once for each page no matter how big it is.
> > While swap_writepage() counts one for each PAGE_SIZE written.
> >
> > And we have THP_SWPOUT but not THP_SWPIN
>
> _If_ I understand the swap-in patch correctly (at least as invoked by
> shmem), it won't attempt to swap in an entire THP.  Even if it swapped
> out an order-9 page, it will bring in order-0 pages from swap, and then
> rely on khugepaged to reassemble them.

Totally correct. The try_to_unmap() called by vmscan would split PMD
to PTEs then install swap entries for each PTE but keep the huge page
unsplit.

BTW, there were patches adding THP swapin support, but they were never merged.

>
> Someone who actually understands the swap code should check that my
> explanation here is correct.
>

Re: [PATCH v2] MM: handle THP in swap_*page_fs() - count_vm_events()

[Miaohe Lin]

On 2022/5/2 13:31, NeilBrown wrote:
> 
> We need to use count_swpout_vm_event() for sio_write_complete() to get
> correct counting.
> 
> Note that THP swap in (if it ever happens) is current accounted 1 for
> each page, whether HUGE or normal.  This is different from swap-out
> accounting.

Agree, there is no THP swap-in now.

> 
> This patch should be squashed into
>     MM: handle THP in swap_*page_fs()
> 

This patch looks good to me. Thanks!

Reviewed-by: Miaohe Lin <linmiaohe@huawei.com>

> Reported-by: Miaohe Lin <linmiaohe@huawei.com>
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  mm/page_io.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/page_io.c b/mm/page_io.c
> index d636a3531cad..1b8075ef3418 100644
> --- a/mm/page_io.c
> +++ b/mm/page_io.c
> @@ -280,8 +280,10 @@ static void sio_write_complete(struct kiocb *iocb, long ret)
>  			set_page_dirty(page);
>  			ClearPageReclaim(page);
>  		}
> -	} else
> -		count_vm_events(PSWPOUT, sio->pages);
> +	} else {
> +		for (p = 0; p < sio->pages; p++)
> +			count_swpout_vm_event(sio->bvec[p].bv_page);
> +	}
>  
>  	for (p = 0; p < sio->pages; p++)
>  		end_page_writeback(sio->bvec[p].bv_page);
> 

Re: [PATCH] MM: handle THP in swap_*page_fs() - count_vm_events()

[Miaohe Lin]

On 2022/5/7 1:26, Yang Shi wrote:
> On Sun, May 1, 2022 at 10:32 PM Matthew Wilcox <willy@infradead.org> wrote:
>>
>> On Mon, May 02, 2022 at 03:28:49PM +1000, NeilBrown wrote:
>>> On Mon, 02 May 2022, Matthew Wilcox wrote:
>>>> On Mon, May 02, 2022 at 02:57:46PM +1000, NeilBrown wrote:
>>>>> @@ -390,9 +392,9 @@ static void sio_read_complete(struct kiocb *iocb, long ret)
>>>>>                   struct page *page = sio->bvec[p].bv_page;
>>>>>
>>>>>                   SetPageUptodate(page);
>>>>> +                 count_swpout_vm_event(page);
>>>>>                   unlock_page(page);
>>>>>           }
>>>>> -         count_vm_events(PSWPIN, sio->pages);
>>>>
>>>> Surely that should be count_swpIN_vm_event?
>>>>
>>> I'm not having a good day....
>>>
>>> Certainly shouldn't be swpout.  There isn't a count_swpin_vm_event().
>>>
>>> swap_readpage() only counts once for each page no matter how big it is.
>>> While swap_writepage() counts one for each PAGE_SIZE written.
>>>
>>> And we have THP_SWPOUT but not THP_SWPIN
>>
>> _If_ I understand the swap-in patch correctly (at least as invoked by
>> shmem), it won't attempt to swap in an entire THP.  Even if it swapped
>> out an order-9 page, it will bring in order-0 pages from swap, and then
>> rely on khugepaged to reassemble them.
> 
> Totally correct. The try_to_unmap() called by vmscan would split PMD
> to PTEs then install swap entries for each PTE but keep the huge page
> unsplit.
> 
> BTW, there were patches adding THP swapin support, but they were never merged.

Could you please tell me where the THP swapin patches are ? It would be really helpful
if you can kindly figure that out for me! :)

Thanks a lot!

> 
>>
>> Someone who actually understands the swap code should check that my
>> explanation here is correct.
>>
> .
>