mm: annotate refault stalls from swap_readpage

PATCH In review Minchan Kim 4 participants 5 messages Oct 10, 2019 – Oct 11, 2019

Subsystems (2)

Memory management MM swap

lore

Share & tools

lore

Minchan Kim

author

2019-10-10 15:21 UTC #0 lore raw

PATCH 1/1 mm: annotate refault stalls from swap_readpage

From: Minchan Kim <[email protected]>

If block device supports rw_page operation, it doesn't submit bio so annotation in submit_bio for refault stall doesn't work. It happens with zram in android, especially swap read path which could consume CPU cycle for decompress. It is also a problem for zswap which uses frontswap.

Annotate swap_readpage() to account the synchronous IO overhead to prevent underreport memory pressure.

Cc Johannes Weiner <[email protected]>

Signed-off-by Minchan Kim <[email protected]>

---
 mm/page_io.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

mm/page_io.c +8 -2

index 24ee600f9131..20147473d4a5 100644
@@ -22,6 +22,7 @@
 #include <linux/writeback.h>
 #include <linux/frontswap.h>
 #include <linux/blkdev.h>
+#include <linux/psi.h>
 #include <linux/uio.h>
 #include <linux/sched/task.h>
 #include <asm/pgtable.h>

@@ -354,10 +355,14 @@ int swap_readpage(struct page *page, bool synchronous)
 	struct swap_info_struct *sis = page_swap_info(page);
 	blk_qc_t qc;
 	struct gendisk *disk;
+	unsigned long pflags;
 
 	VM_BUG_ON_PAGE(!PageSwapCache(page) && !synchronous, page);
 	VM_BUG_ON_PAGE(!PageLocked(page), page);
 	VM_BUG_ON_PAGE(PageUptodate(page), page);
+
+	psi_memstall_enter(&pflags);
+
 	if (frontswap_load(page) == 0) {
 		SetPageUptodate(page);
 		unlock_page(page);

@@ -371,7 +376,7 @@ int swap_readpage(struct page *page, bool synchronous)
 		ret = mapping->a_ops->readpage(swap_file, page);
 		if (!ret)
 			count_vm_event(PSWPIN);
-		return ret;
+		goto out;
 	}
 
 	ret = bdev_read_page(sis->bdev, swap_page_sector(page), page);

@@ -382,7 +387,7 @@ int swap_readpage(struct page *page, bool synchronous)
 		}
 
 		count_vm_event(PSWPIN);
-		return 0;
+		goto out;
 	}
 
 	ret = 0;

@@ -418,6 +423,7 @@ int swap_readpage(struct page *page, bool synchronous)
 	bio_put(bio);
 
 out:
+	psi_memstall_leave(&pflags);
 	return ret;
 }

Signature · 1 line

2.23.0.581.g78d2f28ef7-goog

Johannes Weiner

Acked-by on v1 1/1

2019-10-10 19:17 UTC #1 lore raw

Quoted 8 lines · Minchan Kim

From: Minchan Kim <[email protected]>
If block device supports rw_page operation, it doesn't submit bio so annotation in submit_bio for refault stall doesn't work. It happens with zram in android, especially swap read path which could consume CPU cycle for decompress. It is also a problem for zswap which uses frontswap.

Quoted 5 lines

Annotate swap_readpage() to account the synchronous IO overhead to prevent underreport memory pressure.
Cc Johannes Weiner <[email protected]>
Signed-off-by Minchan Kim <[email protected]>

Acked-by Johannes Weiner <[email protected]>

Can you please add a comment to the caller? Lifted from submit_bio():

/*
* Count submission time as memory stall. When the device is
* congested, or the submitting cgroup IO-throttled,
* submission can be a significant part of overall IO time. */

Minchan Kim

author Acked-by Johannes Weiner

2019-10-10 22:11 UTC #2 lore raw

Quoted 9 lines · Johannes Weiner

Quoted 8 lines · Minchan Kim
From: Minchan Kim <[email protected]>
If block device supports rw_page operation, it doesn't submit bio so annotation in submit_bio for refault stall doesn't work. It happens with zram in android, especially swap read path which could consume CPU cycle for decompress. It is also a problem for zswap which uses frontswap.

Quoted 7 lines

Quoted 5 lines
Annotate swap_readpage() to account the synchronous IO overhead to prevent underreport memory pressure.
Cc Johannes Weiner <[email protected]>
Signed-off-by Minchan Kim <[email protected]>
Acked-by Johannes Weiner <[email protected]>

Thanks, Johannes!

Quoted 2 lines

Can you please add a comment to the caller? Lifted from submit_bio():

Sure, I added a little about zram.

Quoted 6 lines

/*
* Count submission time as memory stall. When the device is
* congested, or the submitting cgroup IO-throttled,
* submission can be a significant part of overall IO time. */

From a8ae7cbc2d3f050aca810fd68285d45cb933b825 Mon Sep 17 00:00:00 2001
From: Minchan Kim <[email protected]>
Date: Thu, 10 Oct 2019 08:09:06 -0700
Subject: [PATCH v2] mm: annotate refault stalls from swap_readpage

Annotate swap_readpage() to account the synchronous IO overhead to prevent underreport memory pressure.

Acked-by Johannes Weiner <[email protected]>

Signed-off-by Minchan Kim <[email protected]>

---
 mm/page_io.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

mm/page_io.c +14 -2

index 24ee600f9131..18f1f8e1d27f 100644
@@ -22,6 +22,7 @@
 #include <linux/writeback.h>
 #include <linux/frontswap.h>
 #include <linux/blkdev.h>
+#include <linux/psi.h>
 #include <linux/uio.h>
 #include <linux/sched/task.h>
 #include <asm/pgtable.h>

@@ -354,10 +355,20 @@ int swap_readpage(struct page *page, bool synchronous)
 	struct swap_info_struct *sis = page_swap_info(page);
 	blk_qc_t qc;
 	struct gendisk *disk;
+	unsigned long pflags;
 
 	VM_BUG_ON_PAGE(!PageSwapCache(page) && !synchronous, page);
 	VM_BUG_ON_PAGE(!PageLocked(page), page);
 	VM_BUG_ON_PAGE(PageUptodate(page), page);
+
+	/*
+	 * Count submission time as memory stall. When the device is
+	 * congested, the submitting cgroup IO-throttled, or backing
+	 * device works synchronously(e.g., zram), submission can be
+	 * a significant part of overall IO time.
+	 */
+	psi_memstall_enter(&pflags);
+
 	if (frontswap_load(page) == 0) {
 		SetPageUptodate(page);
 		unlock_page(page);

@@ -371,7 +382,7 @@ int swap_readpage(struct page *page, bool synchronous)
 		ret = mapping->a_ops->readpage(swap_file, page);
 		if (!ret)
 			count_vm_event(PSWPIN);
-		return ret;
+		goto out;
 	}
 
 	ret = bdev_read_page(sis->bdev, swap_page_sector(page), page);

@@ -382,7 +393,7 @@ int swap_readpage(struct page *page, bool synchronous)
 		}
 
 		count_vm_event(PSWPIN);
-		return 0;
+		goto out;
 	}
 
 	ret = 0;

@@ -418,6 +429,7 @@ int swap_readpage(struct page *page, bool synchronous)
 	bio_put(bio);
 
 out:
+	psi_memstall_leave(&pflags);
 	return ret;
 }

Signature · 1 line

2.23.0.581.g78d2f28ef7-goog

Shakeel Butt

Reviewed-by

2019-10-11 17:32 UTC #3 lore raw

Quoted 45 lines · Minchan Kim

Quoted 9 lines · Johannes Weiner
Quoted 8 lines · Minchan Kim
From: Minchan Kim <[email protected]>
If block device supports rw_page operation, it doesn't submit bio so annotation in submit_bio for refault stall doesn't work. It happens with zram in android, especially swap read path which could consume CPU cycle for decompress. It is also a problem for zswap which uses frontswap.
Quoted 7 lines
Quoted 5 lines
Annotate swap_readpage() to account the synchronous IO overhead to prevent underreport memory pressure.
Cc Johannes Weiner <[email protected]>
Signed-off-by Minchan Kim <[email protected]>
Acked-by Johannes Weiner <[email protected]>
Thanks, Johannes!
Quoted 2 lines
Can you please add a comment to the caller? Lifted from submit_bio():
Sure, I added a little about zram.
Quoted 6 lines
/*
* Count submission time as memory stall. When the device is
* congested, or the submitting cgroup IO-throttled,
* submission can be a significant part of overall IO time. */
From a8ae7cbc2d3f050aca810fd68285d45cb933b825 Mon Sep 17 00:00:00 2001
From: Minchan Kim <[email protected]>
Date: Thu, 10 Oct 2019 08:09:06 -0700
Subject: [PATCH v2] mm: annotate refault stalls from swap_readpage
If block device supports rw_page operation, it doesn't submit bio so annotation in submit_bio for refault stall doesn't work. It happens with zram in android, especially swap read path which could consume CPU cycle for decompress. It is also a problem for zswap which uses frontswap.

Quoted 5 lines

Annotate swap_readpage() to account the synchronous IO overhead to prevent underreport memory pressure.
Acked-by Johannes Weiner <[email protected]>
Signed-off-by Minchan Kim <[email protected]>

Reviewed-by Shakeel Butt <[email protected]>

Quoted 66 lines

---
 mm/page_io.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

mm/page_io.c +14 -3

index 24ee600f9131..18f1f8e1d27f 100644
@@ -22,6 +22,7 @@
 #include <linux/writeback.h>
 #include <linux/frontswap.h>
 #include <linux/blkdev.h>
+#include <linux/psi.h>
 #include <linux/uio.h>
 #include <linux/sched/task.h>
 #include <asm/pgtable.h>

@@ -354,10 +355,20 @@ int swap_readpage(struct page *page, bool synchronous)
        struct swap_info_struct *sis = page_swap_info(page);
        blk_qc_t qc;
        struct gendisk *disk;
+       unsigned long pflags;
 
        VM_BUG_ON_PAGE(!PageSwapCache(page) && !synchronous, page);
        VM_BUG_ON_PAGE(!PageLocked(page), page);
        VM_BUG_ON_PAGE(PageUptodate(page), page);
+
+       /*
+        * Count submission time as memory stall. When the device is
+        * congested, the submitting cgroup IO-throttled, or backing
+        * device works synchronously(e.g., zram), submission can be
+        * a significant part of overall IO time.
+        */
+       psi_memstall_enter(&pflags);
+
        if (frontswap_load(page) == 0) {
                SetPageUptodate(page);
                unlock_page(page);

@@ -371,7 +382,7 @@ int swap_readpage(struct page *page, bool synchronous)
                ret = mapping->a_ops->readpage(swap_file, page);
                if (!ret)
                        count_vm_event(PSWPIN);
-               return ret;
+               goto out;
        }
 
        ret = bdev_read_page(sis->bdev, swap_page_sector(page), page);

@@ -382,7 +393,7 @@ int swap_readpage(struct page *page, bool synchronous)
                }
 
                count_vm_event(PSWPIN);
-               return 0;
+               goto out;
        }
 
        ret = 0;

@@ -418,6 +429,7 @@ int swap_readpage(struct page *page, bool synchronous)
        bio_put(bio);
 
 out:
+       psi_memstall_leave(&pflags);
        return ret;
 }
 
--

2.23.0.581.g78d2f28ef7-goog

Andrew Morton

maintainer

2019-10-11 21:32 UTC #4 lore raw

In reply to Johannes Weiner

Quoted 17 lines · On Thu, 10 Oct 2019 15:17:47 -0400 Johannes Weiner <[email protected]> wrote:

Quoted 8 lines · Minchan Kim
From: Minchan Kim <[email protected]>
If block device supports rw_page operation, it doesn't submit bio so annotation in submit_bio for refault stall doesn't work. It happens with zram in android, especially swap read path which could consume CPU cycle for decompress. It is also a problem for zswap which uses frontswap.
Quoted 5 lines
Annotate swap_readpage() to account the synchronous IO overhead to prevent underreport memory pressure.
Cc Johannes Weiner <[email protected]>
Signed-off-by Minchan Kim <[email protected]>
Acked-by Johannes Weiner <[email protected]>

Quoted 7 lines

Can you please add a comment to the caller? Lifted from submit_bio():
/*
* Count submission time as memory stall. When the device is
* congested, or the submitting cgroup IO-throttled,
* submission can be a significant part of overall IO time. */

This?

mm/page_io.c renamed from mm/page_io.c~mm-annotate-refault-stalls-from-swap_readpage-fix +5 -0

@@ -361,6 +361,11 @@ int swap_readpage(struct page *page, boo
 	VM_BUG_ON_PAGE(!PageLocked(page), page);
 	VM_BUG_ON_PAGE(PageUptodate(page), page);
 
+	/*
+	 * Count submission time as memory stall. When the device is congested,
+	 * or the submitting cgroup IO-throttled, submission can be a
+	 * significant part of overall IO time.
+	 */
 	psi_memstall_enter(&pflags);
 
 	if (frontswap_load(page) == 0) {