Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1360399 > unrolled thread

[PATCH] zram: revive swap_slot_free_notify

Back to article view | Back to linux.kernel

Contents

  [PATCH] zram: revive swap_slot_free_notify Minchan Kim <minchan@kernel.org> - 2016-03-18 09:00 +0100
    Re: [PATCH] zram: revive swap_slot_free_notify Andrew Morton <akpm@linux-foundation.org> - 2016-03-21 21:40 +0100
      Re: [PATCH] zram: revive swap_slot_free_notify Minchan Kim <minchan@kernel.org> - 2016-03-22 01:00 +0100
    Re: [PATCH] zram: revive swap_slot_free_notify Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-03-22 06:10 +0100
      Re: [PATCH] zram: revive swap_slot_free_notify Minchan Kim <minchan@kernel.org> - 2016-03-22 09:00 +0100
        Re: [PATCH] zram: revive swap_slot_free_notify Joonsoo Kim <js1304@gmail.com> - 2016-03-22 09:30 +0100
          Re: [PATCH] zram: revive swap_slot_free_notify Minchan Kim <minchan@kernel.org> - 2016-03-22 15:10 +0100
            Re: [PATCH] zram: revive swap_slot_free_notify Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-03-23 05:50 +0100

#1360399 — [PATCH] zram: revive swap_slot_free_notify

<b430e9d1c6d4> "remove compressed copy from zram in-memory"
applied swap_slot_free_notify call in *end_swap_bio_read* to
remove duplicated memory between zram and memory.

However, with introducing rw_page in zram <8c7f01025f7b>
"zram: implement rw_page operation of zram", it became void
because rw_page doesn't need bio.

This patch restores the function for rw_page.

Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 mm/page_io.c | 93 ++++++++++++++++++++++++++++++++----------------------------
 1 file changed, 50 insertions(+), 43 deletions(-)

diff --git a/mm/page_io.c b/mm/page_io.c
index ff74e512f029..18aac7819cc9 100644
--- a/mm/page_io.c
+++ b/mm/page_io.c
@@ -66,6 +66,54 @@ void end_swap_bio_write(struct bio *bio)
 	bio_put(bio);
 }
 
+static void swap_slot_free_notify(struct page *page)
+{
+	struct swap_info_struct *sis;
+	struct gendisk *disk;
+
+	/*
+	 * There is no guarantee that the page is in swap cache - the software
+	 * suspend code (at least) uses end_swap_bio_read() against a non-
+	 * swapcache page.  So we must check PG_swapcache before proceeding with
+	 * this optimization.
+	 */
+	if (unlikely(!PageSwapCache(page)))
+		return;
+
+	sis = page_swap_info(page);
+	if (!(sis->flags & SWP_BLKDEV))
+		return;
+
+	/*
+	 * The swap subsystem performs lazy swap slot freeing,
+	 * expecting that the page will be swapped out again.
+	 * So we can avoid an unnecessary write if the page
+	 * isn't redirtied.
+	 * This is good for real swap storage because we can
+	 * reduce unnecessary I/O and enhance wear-leveling
+	 * if an SSD is used as the as swap device.
+	 * But if in-memory swap device (eg zram) is used,
+	 * this causes a duplicated copy between uncompressed
+	 * data in VM-owned memory and compressed data in
+	 * zram-owned memory.  So let's free zram-owned memory
+	 * and make the VM-owned decompressed page *dirty*,
+	 * so the page should be swapped out somewhere again if
+	 * we again wish to reclaim it.
+	 */
+	disk = sis->bdev->bd_disk;
+	if (disk->fops->swap_slot_free_notify) {
+		swp_entry_t entry;
+		unsigned long offset;
+
+		entry.val = page_private(page);
+		offset = swp_offset(entry);
+
+		SetPageDirty(page);
+		disk->fops->swap_slot_free_notify(sis->bdev,
+				offset);
+	}
+}
+
 static void end_swap_bio_read(struct bio *bio)
 {
 	struct page *page = bio->bi_io_vec[0].bv_page;
@@ -81,49 +129,7 @@ static void end_swap_bio_read(struct bio *bio)
 	}
 
 	SetPageUptodate(page);
-
-	/*
-	 * There is no guarantee that the page is in swap cache - the software
-	 * suspend code (at least) uses end_swap_bio_read() against a non-
-	 * swapcache page.  So we must check PG_swapcache before proceeding with
-	 * this optimization.
-	 */
-	if (likely(PageSwapCache(page))) {
-		struct swap_info_struct *sis;
-
-		sis = page_swap_info(page);
-		if (sis->flags & SWP_BLKDEV) {
-			/*
-			 * The swap subsystem performs lazy swap slot freeing,
-			 * expecting that the page will be swapped out again.
-			 * So we can avoid an unnecessary write if the page
-			 * isn't redirtied.
-			 * This is good for real swap storage because we can
-			 * reduce unnecessary I/O and enhance wear-leveling
-			 * if an SSD is used as the as swap device.
-			 * But if in-memory swap device (eg zram) is used,
-			 * this causes a duplicated copy between uncompressed
-			 * data in VM-owned memory and compressed data in
-			 * zram-owned memory.  So let's free zram-owned memory
-			 * and make the VM-owned decompressed page *dirty*,
-			 * so the page should be swapped out somewhere again if
-			 * we again wish to reclaim it.
-			 */
-			struct gendisk *disk = sis->bdev->bd_disk;
-			if (disk->fops->swap_slot_free_notify) {
-				swp_entry_t entry;
-				unsigned long offset;
-
-				entry.val = page_private(page);
-				offset = swp_offset(entry);
-
-				SetPageDirty(page);
-				disk->fops->swap_slot_free_notify(sis->bdev,
-						offset);
-			}
-		}
-	}
-
+	swap_slot_free_notify(page);
 out:
 	unlock_page(page);
 	bio_put(bio);
@@ -347,6 +353,7 @@ int swap_readpage(struct page *page)
 
 	ret = bdev_read_page(sis->bdev, swap_page_sector(page), page);
 	if (!ret) {
+		swap_slot_free_notify(page);
 		count_vm_event(PSWPIN);
 		return 0;
 	}
-- 
1.9.1

[toc] | [next] | [standalone]

#1362139

On Fri, 18 Mar 2016 16:58:31 +0900 Minchan Kim <minchan@kernel.org> wrote:

> <b430e9d1c6d4> "remove compressed copy from zram in-memory"
> applied swap_slot_free_notify call in *end_swap_bio_read* to
> remove duplicated memory between zram and memory.
> 
> However, with introducing rw_page in zram <8c7f01025f7b>
> "zram: implement rw_page operation of zram", it became void
> because rw_page doesn't need bio.
> 
> This patch restores the function for rw_page.

This is a bit mysterious.  What is the actual runtime effect of the
patch?  I assume that 8c7f01025f7b caused duplication of memory and
that the only problem is additional resource consumption?  If so, what
are the observable effects?  etcetera, please.

[toc] | [prev] | [next] | [standalone]

#1362229

Hello Andrew,

On Mon, Mar 21, 2016 at 01:30:17PM -0700, Andrew Morton wrote:
> On Fri, 18 Mar 2016 16:58:31 +0900 Minchan Kim <minchan@kernel.org> wrote:
> 
> > <b430e9d1c6d4> "remove compressed copy from zram in-memory"
> > applied swap_slot_free_notify call in *end_swap_bio_read* to
> > remove duplicated memory between zram and memory.
> > 
> > However, with introducing rw_page in zram <8c7f01025f7b>
> > "zram: implement rw_page operation of zram", it became void
> > because rw_page doesn't need bio.
> > 
> > This patch restores the function for rw_page.
> 
> This is a bit mysterious.  What is the actual runtime effect of the
> patch?  I assume that 8c7f01025f7b caused duplication of memory and
> that the only problem is additional resource consumption?  If so, what
> are the observable effects?  etcetera, please.
> 

As you already pointed out, only problem is memory duplication.
In b430e9d1c6d4 description, I wrote down how it saves memory(e.g,
about 19M in kernel build workload).
Memory footprint is really important in embedded platforms which
have small memory, for example, 512M) recently because it could
start to kill processes if memory footprint exceeds some threshold
by LMK or some similar memory management modules.

[toc] | [prev] | [next] | [standalone]

#1362327

On Fri, Mar 18, 2016 at 04:58:31PM +0900, Minchan Kim wrote:
> <b430e9d1c6d4> "remove compressed copy from zram in-memory"
> applied swap_slot_free_notify call in *end_swap_bio_read* to
> remove duplicated memory between zram and memory.
> 
> However, with introducing rw_page in zram <8c7f01025f7b>
> "zram: implement rw_page operation of zram", it became void
> because rw_page doesn't need bio.
> 
> This patch restores the function for rw_page.
> 
> Signed-off-by: Minchan Kim <minchan@kernel.org>
> ---
>  mm/page_io.c | 93 ++++++++++++++++++++++++++++++++----------------------------
>  1 file changed, 50 insertions(+), 43 deletions(-)
> 
> diff --git a/mm/page_io.c b/mm/page_io.c
> index ff74e512f029..18aac7819cc9 100644
> --- a/mm/page_io.c
> +++ b/mm/page_io.c
> @@ -66,6 +66,54 @@ void end_swap_bio_write(struct bio *bio)
>  	bio_put(bio);
>  }
>  
> +static void swap_slot_free_notify(struct page *page)
> +{
> +	struct swap_info_struct *sis;
> +	struct gendisk *disk;
> +
> +	/*
> +	 * There is no guarantee that the page is in swap cache - the software
> +	 * suspend code (at least) uses end_swap_bio_read() against a non-
> +	 * swapcache page.  So we must check PG_swapcache before proceeding with
> +	 * this optimization.
> +	 */
> +	if (unlikely(!PageSwapCache(page)))
> +		return;
> +
> +	sis = page_swap_info(page);
> +	if (!(sis->flags & SWP_BLKDEV))
> +		return;
> +
> +	/*
> +	 * The swap subsystem performs lazy swap slot freeing,
> +	 * expecting that the page will be swapped out again.
> +	 * So we can avoid an unnecessary write if the page
> +	 * isn't redirtied.
> +	 * This is good for real swap storage because we can
> +	 * reduce unnecessary I/O and enhance wear-leveling
> +	 * if an SSD is used as the as swap device.
> +	 * But if in-memory swap device (eg zram) is used,
> +	 * this causes a duplicated copy between uncompressed
> +	 * data in VM-owned memory and compressed data in
> +	 * zram-owned memory.  So let's free zram-owned memory
> +	 * and make the VM-owned decompressed page *dirty*,
> +	 * so the page should be swapped out somewhere again if
> +	 * we again wish to reclaim it.
> +	 */
> +	disk = sis->bdev->bd_disk;
> +	if (disk->fops->swap_slot_free_notify) {
> +		swp_entry_t entry;
> +		unsigned long offset;
> +
> +		entry.val = page_private(page);
> +		offset = swp_offset(entry);
> +
> +		SetPageDirty(page);
> +		disk->fops->swap_slot_free_notify(sis->bdev,
> +				offset);
> +	}
> +}
> +
>  static void end_swap_bio_read(struct bio *bio)
>  {
>  	struct page *page = bio->bi_io_vec[0].bv_page;
> @@ -81,49 +129,7 @@ static void end_swap_bio_read(struct bio *bio)
>  	}
>  
>  	SetPageUptodate(page);
> -
> -	/*
> -	 * There is no guarantee that the page is in swap cache - the software
> -	 * suspend code (at least) uses end_swap_bio_read() against a non-
> -	 * swapcache page.  So we must check PG_swapcache before proceeding with
> -	 * this optimization.
> -	 */
> -	if (likely(PageSwapCache(page))) {
> -		struct swap_info_struct *sis;
> -
> -		sis = page_swap_info(page);
> -		if (sis->flags & SWP_BLKDEV) {
> -			/*
> -			 * The swap subsystem performs lazy swap slot freeing,
> -			 * expecting that the page will be swapped out again.
> -			 * So we can avoid an unnecessary write if the page
> -			 * isn't redirtied.
> -			 * This is good for real swap storage because we can
> -			 * reduce unnecessary I/O and enhance wear-leveling
> -			 * if an SSD is used as the as swap device.
> -			 * But if in-memory swap device (eg zram) is used,
> -			 * this causes a duplicated copy between uncompressed
> -			 * data in VM-owned memory and compressed data in
> -			 * zram-owned memory.  So let's free zram-owned memory
> -			 * and make the VM-owned decompressed page *dirty*,
> -			 * so the page should be swapped out somewhere again if
> -			 * we again wish to reclaim it.
> -			 */
> -			struct gendisk *disk = sis->bdev->bd_disk;
> -			if (disk->fops->swap_slot_free_notify) {
> -				swp_entry_t entry;
> -				unsigned long offset;
> -
> -				entry.val = page_private(page);
> -				offset = swp_offset(entry);
> -
> -				SetPageDirty(page);
> -				disk->fops->swap_slot_free_notify(sis->bdev,
> -						offset);
> -			}
> -		}
> -	}
> -
> +	swap_slot_free_notify(page);
>  out:
>  	unlock_page(page);
>  	bio_put(bio);
> @@ -347,6 +353,7 @@ int swap_readpage(struct page *page)
>  
>  	ret = bdev_read_page(sis->bdev, swap_page_sector(page), page);
>  	if (!ret) {
> +		swap_slot_free_notify(page);
>  		count_vm_event(PSWPIN);
>  		return 0;
>  	}

Hello,

You need to check PageUpdate() or something because bdev_read_page()
can be asynchronous.

BTW, something like as swap_slot_free_notify() which invalidate
backend of storage can also be possible for frontswap when
frontswap_load() succeed. Isn't it?

Thanks.

[toc] | [prev] | [next] | [standalone]

#1362400

On Tue, Mar 22, 2016 at 02:08:59PM +0900, Joonsoo Kim wrote:
> On Fri, Mar 18, 2016 at 04:58:31PM +0900, Minchan Kim wrote:
> > <b430e9d1c6d4> "remove compressed copy from zram in-memory"
> > applied swap_slot_free_notify call in *end_swap_bio_read* to
> > remove duplicated memory between zram and memory.
> > 
> > However, with introducing rw_page in zram <8c7f01025f7b>
> > "zram: implement rw_page operation of zram", it became void
> > because rw_page doesn't need bio.
> > 
> > This patch restores the function for rw_page.
> > 
> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> > ---
> >  mm/page_io.c | 93 ++++++++++++++++++++++++++++++++----------------------------
> >  1 file changed, 50 insertions(+), 43 deletions(-)
> > 
> > diff --git a/mm/page_io.c b/mm/page_io.c
> > index ff74e512f029..18aac7819cc9 100644
> > --- a/mm/page_io.c
> > +++ b/mm/page_io.c
> > @@ -66,6 +66,54 @@ void end_swap_bio_write(struct bio *bio)
> >  	bio_put(bio);
> >  }
> >  
> > +static void swap_slot_free_notify(struct page *page)
> > +{
> > +	struct swap_info_struct *sis;
> > +	struct gendisk *disk;
> > +
> > +	/*
> > +	 * There is no guarantee that the page is in swap cache - the software
> > +	 * suspend code (at least) uses end_swap_bio_read() against a non-
> > +	 * swapcache page.  So we must check PG_swapcache before proceeding with
> > +	 * this optimization.
> > +	 */
> > +	if (unlikely(!PageSwapCache(page)))
> > +		return;
> > +
> > +	sis = page_swap_info(page);
> > +	if (!(sis->flags & SWP_BLKDEV))
> > +		return;
> > +
> > +	/*
> > +	 * The swap subsystem performs lazy swap slot freeing,
> > +	 * expecting that the page will be swapped out again.
> > +	 * So we can avoid an unnecessary write if the page
> > +	 * isn't redirtied.
> > +	 * This is good for real swap storage because we can
> > +	 * reduce unnecessary I/O and enhance wear-leveling
> > +	 * if an SSD is used as the as swap device.
> > +	 * But if in-memory swap device (eg zram) is used,
> > +	 * this causes a duplicated copy between uncompressed
> > +	 * data in VM-owned memory and compressed data in
> > +	 * zram-owned memory.  So let's free zram-owned memory
> > +	 * and make the VM-owned decompressed page *dirty*,
> > +	 * so the page should be swapped out somewhere again if
> > +	 * we again wish to reclaim it.
> > +	 */
> > +	disk = sis->bdev->bd_disk;
> > +	if (disk->fops->swap_slot_free_notify) {
> > +		swp_entry_t entry;
> > +		unsigned long offset;
> > +
> > +		entry.val = page_private(page);
> > +		offset = swp_offset(entry);
> > +
> > +		SetPageDirty(page);
> > +		disk->fops->swap_slot_free_notify(sis->bdev,
> > +				offset);
> > +	}
> > +}
> > +
> >  static void end_swap_bio_read(struct bio *bio)
> >  {
> >  	struct page *page = bio->bi_io_vec[0].bv_page;
> > @@ -81,49 +129,7 @@ static void end_swap_bio_read(struct bio *bio)
> >  	}
> >  
> >  	SetPageUptodate(page);
> > -
> > -	/*
> > -	 * There is no guarantee that the page is in swap cache - the software
> > -	 * suspend code (at least) uses end_swap_bio_read() against a non-
> > -	 * swapcache page.  So we must check PG_swapcache before proceeding with
> > -	 * this optimization.
> > -	 */
> > -	if (likely(PageSwapCache(page))) {
> > -		struct swap_info_struct *sis;
> > -
> > -		sis = page_swap_info(page);
> > -		if (sis->flags & SWP_BLKDEV) {
> > -			/*
> > -			 * The swap subsystem performs lazy swap slot freeing,
> > -			 * expecting that the page will be swapped out again.
> > -			 * So we can avoid an unnecessary write if the page
> > -			 * isn't redirtied.
> > -			 * This is good for real swap storage because we can
> > -			 * reduce unnecessary I/O and enhance wear-leveling
> > -			 * if an SSD is used as the as swap device.
> > -			 * But if in-memory swap device (eg zram) is used,
> > -			 * this causes a duplicated copy between uncompressed
> > -			 * data in VM-owned memory and compressed data in
> > -			 * zram-owned memory.  So let's free zram-owned memory
> > -			 * and make the VM-owned decompressed page *dirty*,
> > -			 * so the page should be swapped out somewhere again if
> > -			 * we again wish to reclaim it.
> > -			 */
> > -			struct gendisk *disk = sis->bdev->bd_disk;
> > -			if (disk->fops->swap_slot_free_notify) {
> > -				swp_entry_t entry;
> > -				unsigned long offset;
> > -
> > -				entry.val = page_private(page);
> > -				offset = swp_offset(entry);
> > -
> > -				SetPageDirty(page);
> > -				disk->fops->swap_slot_free_notify(sis->bdev,
> > -						offset);
> > -			}
> > -		}
> > -	}
> > -
> > +	swap_slot_free_notify(page);
> >  out:
> >  	unlock_page(page);
> >  	bio_put(bio);
> > @@ -347,6 +353,7 @@ int swap_readpage(struct page *page)
> >  
> >  	ret = bdev_read_page(sis->bdev, swap_page_sector(page), page);
> >  	if (!ret) {
> > +		swap_slot_free_notify(page);
> >  		count_vm_event(PSWPIN);
> >  		return 0;
> >  	}
> 
> Hello,

Hey Joonsoo,

> 
> You need to check PageUpdate() or something because bdev_read_page()
> can be asynchronous.

I considered it but decided not to add the check :(.
Because I couldn't justify what benfit we can have with the check.

The swap_slot_free_notify is tightly coupled with zram for several
years and zram have been worked synchronously. So if bdev_read_page
returns 0, it means we already have read the page successfully.
Even, when I looked up other rw_page user, it seems there is no async
rw_page users at the moment.

If there is someone want to use *async* rw_page && *swap_slot_free_noity*
in future, we could add the check easily. But I hope anyone never use
swap_slot_free_notify any more which is mess. :(

> 
> BTW, something like as swap_slot_free_notify() which invalidate
> backend of storage can also be possible for frontswap when
> frontswap_load() succeed. Isn't it?

frontswap_tmem_exclusive_gets_enabled?

> 
> Thanks.

[toc] | [prev] | [next] | [standalone]

#1362419

2016-03-22 17:00 GMT+09:00 Minchan Kim <minchan@kernel.org>:
> On Tue, Mar 22, 2016 at 02:08:59PM +0900, Joonsoo Kim wrote:
>> On Fri, Mar 18, 2016 at 04:58:31PM +0900, Minchan Kim wrote:
>> > <b430e9d1c6d4> "remove compressed copy from zram in-memory"
>> > applied swap_slot_free_notify call in *end_swap_bio_read* to
>> > remove duplicated memory between zram and memory.
>> >
>> > However, with introducing rw_page in zram <8c7f01025f7b>
>> > "zram: implement rw_page operation of zram", it became void
>> > because rw_page doesn't need bio.
>> >
>> > This patch restores the function for rw_page.
>> >
>> > Signed-off-by: Minchan Kim <minchan@kernel.org>
>> > ---
>> >  mm/page_io.c | 93 ++++++++++++++++++++++++++++++++----------------------------
>> >  1 file changed, 50 insertions(+), 43 deletions(-)
>> >
>> > diff --git a/mm/page_io.c b/mm/page_io.c
>> > index ff74e512f029..18aac7819cc9 100644
>> > --- a/mm/page_io.c
>> > +++ b/mm/page_io.c
>> > @@ -66,6 +66,54 @@ void end_swap_bio_write(struct bio *bio)
>> >     bio_put(bio);
>> >  }
>> >
>> > +static void swap_slot_free_notify(struct page *page)
>> > +{
>> > +   struct swap_info_struct *sis;
>> > +   struct gendisk *disk;
>> > +
>> > +   /*
>> > +    * There is no guarantee that the page is in swap cache - the software
>> > +    * suspend code (at least) uses end_swap_bio_read() against a non-
>> > +    * swapcache page.  So we must check PG_swapcache before proceeding with
>> > +    * this optimization.
>> > +    */
>> > +   if (unlikely(!PageSwapCache(page)))
>> > +           return;
>> > +
>> > +   sis = page_swap_info(page);
>> > +   if (!(sis->flags & SWP_BLKDEV))
>> > +           return;
>> > +
>> > +   /*
>> > +    * The swap subsystem performs lazy swap slot freeing,
>> > +    * expecting that the page will be swapped out again.
>> > +    * So we can avoid an unnecessary write if the page
>> > +    * isn't redirtied.
>> > +    * This is good for real swap storage because we can
>> > +    * reduce unnecessary I/O and enhance wear-leveling
>> > +    * if an SSD is used as the as swap device.
>> > +    * But if in-memory swap device (eg zram) is used,
>> > +    * this causes a duplicated copy between uncompressed
>> > +    * data in VM-owned memory and compressed data in
>> > +    * zram-owned memory.  So let's free zram-owned memory
>> > +    * and make the VM-owned decompressed page *dirty*,
>> > +    * so the page should be swapped out somewhere again if
>> > +    * we again wish to reclaim it.
>> > +    */
>> > +   disk = sis->bdev->bd_disk;
>> > +   if (disk->fops->swap_slot_free_notify) {
>> > +           swp_entry_t entry;
>> > +           unsigned long offset;
>> > +
>> > +           entry.val = page_private(page);
>> > +           offset = swp_offset(entry);
>> > +
>> > +           SetPageDirty(page);
>> > +           disk->fops->swap_slot_free_notify(sis->bdev,
>> > +                           offset);
>> > +   }
>> > +}
>> > +
>> >  static void end_swap_bio_read(struct bio *bio)
>> >  {
>> >     struct page *page = bio->bi_io_vec[0].bv_page;
>> > @@ -81,49 +129,7 @@ static void end_swap_bio_read(struct bio *bio)
>> >     }
>> >
>> >     SetPageUptodate(page);
>> > -
>> > -   /*
>> > -    * There is no guarantee that the page is in swap cache - the software
>> > -    * suspend code (at least) uses end_swap_bio_read() against a non-
>> > -    * swapcache page.  So we must check PG_swapcache before proceeding with
>> > -    * this optimization.
>> > -    */
>> > -   if (likely(PageSwapCache(page))) {
>> > -           struct swap_info_struct *sis;
>> > -
>> > -           sis = page_swap_info(page);
>> > -           if (sis->flags & SWP_BLKDEV) {
>> > -                   /*
>> > -                    * The swap subsystem performs lazy swap slot freeing,
>> > -                    * expecting that the page will be swapped out again.
>> > -                    * So we can avoid an unnecessary write if the page
>> > -                    * isn't redirtied.
>> > -                    * This is good for real swap storage because we can
>> > -                    * reduce unnecessary I/O and enhance wear-leveling
>> > -                    * if an SSD is used as the as swap device.
>> > -                    * But if in-memory swap device (eg zram) is used,
>> > -                    * this causes a duplicated copy between uncompressed
>> > -                    * data in VM-owned memory and compressed data in
>> > -                    * zram-owned memory.  So let's free zram-owned memory
>> > -                    * and make the VM-owned decompressed page *dirty*,
>> > -                    * so the page should be swapped out somewhere again if
>> > -                    * we again wish to reclaim it.
>> > -                    */
>> > -                   struct gendisk *disk = sis->bdev->bd_disk;
>> > -                   if (disk->fops->swap_slot_free_notify) {
>> > -                           swp_entry_t entry;
>> > -                           unsigned long offset;
>> > -
>> > -                           entry.val = page_private(page);
>> > -                           offset = swp_offset(entry);
>> > -
>> > -                           SetPageDirty(page);
>> > -                           disk->fops->swap_slot_free_notify(sis->bdev,
>> > -                                           offset);
>> > -                   }
>> > -           }
>> > -   }
>> > -
>> > +   swap_slot_free_notify(page);
>> >  out:
>> >     unlock_page(page);
>> >     bio_put(bio);
>> > @@ -347,6 +353,7 @@ int swap_readpage(struct page *page)
>> >
>> >     ret = bdev_read_page(sis->bdev, swap_page_sector(page), page);
>> >     if (!ret) {
>> > +           swap_slot_free_notify(page);
>> >             count_vm_event(PSWPIN);
>> >             return 0;
>> >     }
>>
>> Hello,
>
> Hey Joonsoo,
>
>>
>> You need to check PageUpdate() or something because bdev_read_page()
>> can be asynchronous.
>
> I considered it but decided not to add the check :(.
> Because I couldn't justify what benfit we can have with the check.
> The swap_slot_free_notify is tightly coupled with zram for several
> years and zram have been worked synchronously. So if bdev_read_page
> returns 0, it means we already have read the page successfully.
> Even, when I looked up other rw_page user, it seems there is no async
> rw_page users at the moment.

Yes, I also looked up other rw_page users and found that
there is no async rw_page now.

> If there is someone want to use *async* rw_page && *swap_slot_free_noity*
> in future, we could add the check easily. But I hope anyone never use
> swap_slot_free_notify any more which is mess. :(

But, I think that we should add the check. If someone want it, how does
he/she know about it? Even, if someone makes zram to read/write
asynchronously, we can miss it easily. This is error-prone practice.

>>
>> BTW, something like as swap_slot_free_notify() which invalidate
>> backend of storage can also be possible for frontswap when
>> frontswap_load() succeed. Isn't it?
>
> frontswap_tmem_exclusive_gets_enabled?

Wow... yes. that's what I try to find.
Do you know the reason why zswap doesn't enable it?

Thanks.

[toc] | [prev] | [next] | [standalone]

#1362760

On Tue, Mar 22, 2016 at 05:20:08PM +0900, Joonsoo Kim wrote:
> 2016-03-22 17:00 GMT+09:00 Minchan Kim <minchan@kernel.org>:
> > On Tue, Mar 22, 2016 at 02:08:59PM +0900, Joonsoo Kim wrote:
> >> On Fri, Mar 18, 2016 at 04:58:31PM +0900, Minchan Kim wrote:
> >> > <b430e9d1c6d4> "remove compressed copy from zram in-memory"
> >> > applied swap_slot_free_notify call in *end_swap_bio_read* to
> >> > remove duplicated memory between zram and memory.
> >> >
> >> > However, with introducing rw_page in zram <8c7f01025f7b>
> >> > "zram: implement rw_page operation of zram", it became void
> >> > because rw_page doesn't need bio.
> >> >
> >> > This patch restores the function for rw_page.
> >> >
> >> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> >> > ---
> >> >  mm/page_io.c | 93 ++++++++++++++++++++++++++++++++----------------------------
> >> >  1 file changed, 50 insertions(+), 43 deletions(-)
> >> >
> >> > diff --git a/mm/page_io.c b/mm/page_io.c
> >> > index ff74e512f029..18aac7819cc9 100644
> >> > --- a/mm/page_io.c
> >> > +++ b/mm/page_io.c
> >> > @@ -66,6 +66,54 @@ void end_swap_bio_write(struct bio *bio)
> >> >     bio_put(bio);
> >> >  }
> >> >
> >> > +static void swap_slot_free_notify(struct page *page)
> >> > +{
> >> > +   struct swap_info_struct *sis;
> >> > +   struct gendisk *disk;
> >> > +
> >> > +   /*
> >> > +    * There is no guarantee that the page is in swap cache - the software
> >> > +    * suspend code (at least) uses end_swap_bio_read() against a non-
> >> > +    * swapcache page.  So we must check PG_swapcache before proceeding with
> >> > +    * this optimization.
> >> > +    */
> >> > +   if (unlikely(!PageSwapCache(page)))
> >> > +           return;
> >> > +
> >> > +   sis = page_swap_info(page);
> >> > +   if (!(sis->flags & SWP_BLKDEV))
> >> > +           return;
> >> > +
> >> > +   /*
> >> > +    * The swap subsystem performs lazy swap slot freeing,
> >> > +    * expecting that the page will be swapped out again.
> >> > +    * So we can avoid an unnecessary write if the page
> >> > +    * isn't redirtied.
> >> > +    * This is good for real swap storage because we can
> >> > +    * reduce unnecessary I/O and enhance wear-leveling
> >> > +    * if an SSD is used as the as swap device.
> >> > +    * But if in-memory swap device (eg zram) is used,
> >> > +    * this causes a duplicated copy between uncompressed
> >> > +    * data in VM-owned memory and compressed data in
> >> > +    * zram-owned memory.  So let's free zram-owned memory
> >> > +    * and make the VM-owned decompressed page *dirty*,
> >> > +    * so the page should be swapped out somewhere again if
> >> > +    * we again wish to reclaim it.
> >> > +    */
> >> > +   disk = sis->bdev->bd_disk;
> >> > +   if (disk->fops->swap_slot_free_notify) {
> >> > +           swp_entry_t entry;
> >> > +           unsigned long offset;
> >> > +
> >> > +           entry.val = page_private(page);
> >> > +           offset = swp_offset(entry);
> >> > +
> >> > +           SetPageDirty(page);
> >> > +           disk->fops->swap_slot_free_notify(sis->bdev,
> >> > +                           offset);
> >> > +   }
> >> > +}
> >> > +
> >> >  static void end_swap_bio_read(struct bio *bio)
> >> >  {
> >> >     struct page *page = bio->bi_io_vec[0].bv_page;
> >> > @@ -81,49 +129,7 @@ static void end_swap_bio_read(struct bio *bio)
> >> >     }
> >> >
> >> >     SetPageUptodate(page);
> >> > -
> >> > -   /*
> >> > -    * There is no guarantee that the page is in swap cache - the software
> >> > -    * suspend code (at least) uses end_swap_bio_read() against a non-
> >> > -    * swapcache page.  So we must check PG_swapcache before proceeding with
> >> > -    * this optimization.
> >> > -    */
> >> > -   if (likely(PageSwapCache(page))) {
> >> > -           struct swap_info_struct *sis;
> >> > -
> >> > -           sis = page_swap_info(page);
> >> > -           if (sis->flags & SWP_BLKDEV) {
> >> > -                   /*
> >> > -                    * The swap subsystem performs lazy swap slot freeing,
> >> > -                    * expecting that the page will be swapped out again.
> >> > -                    * So we can avoid an unnecessary write if the page
> >> > -                    * isn't redirtied.
> >> > -                    * This is good for real swap storage because we can
> >> > -                    * reduce unnecessary I/O and enhance wear-leveling
> >> > -                    * if an SSD is used as the as swap device.
> >> > -                    * But if in-memory swap device (eg zram) is used,
> >> > -                    * this causes a duplicated copy between uncompressed
> >> > -                    * data in VM-owned memory and compressed data in
> >> > -                    * zram-owned memory.  So let's free zram-owned memory
> >> > -                    * and make the VM-owned decompressed page *dirty*,
> >> > -                    * so the page should be swapped out somewhere again if
> >> > -                    * we again wish to reclaim it.
> >> > -                    */
> >> > -                   struct gendisk *disk = sis->bdev->bd_disk;
> >> > -                   if (disk->fops->swap_slot_free_notify) {
> >> > -                           swp_entry_t entry;
> >> > -                           unsigned long offset;
> >> > -
> >> > -                           entry.val = page_private(page);
> >> > -                           offset = swp_offset(entry);
> >> > -
> >> > -                           SetPageDirty(page);
> >> > -                           disk->fops->swap_slot_free_notify(sis->bdev,
> >> > -                                           offset);
> >> > -                   }
> >> > -           }
> >> > -   }
> >> > -
> >> > +   swap_slot_free_notify(page);
> >> >  out:
> >> >     unlock_page(page);
> >> >     bio_put(bio);
> >> > @@ -347,6 +353,7 @@ int swap_readpage(struct page *page)
> >> >
> >> >     ret = bdev_read_page(sis->bdev, swap_page_sector(page), page);
> >> >     if (!ret) {
> >> > +           swap_slot_free_notify(page);
> >> >             count_vm_event(PSWPIN);
> >> >             return 0;
> >> >     }
> >>
> >> Hello,
> >
> > Hey Joonsoo,
> >
> >>
> >> You need to check PageUpdate() or something because bdev_read_page()
> >> can be asynchronous.
> >
> > I considered it but decided not to add the check :(.
> > Because I couldn't justify what benfit we can have with the check.
> > The swap_slot_free_notify is tightly coupled with zram for several
> > years and zram have been worked synchronously. So if bdev_read_page
> > returns 0, it means we already have read the page successfully.
> > Even, when I looked up other rw_page user, it seems there is no async
> > rw_page users at the moment.
> 
> Yes, I also looked up other rw_page users and found that
> there is no async rw_page now.
> 
> > If there is someone want to use *async* rw_page && *swap_slot_free_noity*
> > in future, we could add the check easily. But I hope anyone never use
> > swap_slot_free_notify any more which is mess. :(
> 
> But, I think that we should add the check. If someone want it, how does
> he/she know about it? Even, if someone makes zram to read/write
> asynchronously, we can miss it easily. This is error-prone practice.

Okay, I don't have strong against it.
If we really want to catch such case, let's add WARN_ON_ONCE.

diff --git a/mm/page_io.c b/mm/page_io.c
index 18aac7819cc9..6592893d16ca 100644
--- a/mm/page_io.c
+++ b/mm/page_io.c
@@ -353,6 +353,7 @@ int swap_readpage(struct page *page)
 
 	ret = bdev_read_page(sis->bdev, swap_page_sector(page), page);
 	if (!ret) {
+		WARN_ON_ONCE(!PageUptodate(page));
 		swap_slot_free_notify(page);
 		count_vm_event(PSWPIN);
 		return 0;

> 
> >>
> >> BTW, something like as swap_slot_free_notify() which invalidate
> >> backend of storage can also be possible for frontswap when
> >> frontswap_load() succeed. Isn't it?
> >
> > frontswap_tmem_exclusive_gets_enabled?
> 
> Wow... yes. that's what I try to find.
> Do you know the reason why zswap doesn't enable it?

Hmm, I couldn't remember. Maybe, it's not zswap stuff but frontswap stuff
so I guess zswap user can enable it via frontswap interface if he want.

> 
> Thanks.

[toc] | [prev] | [next] | [standalone]

#1363196

On Tue, Mar 22, 2016 at 11:06:29PM +0900, Minchan Kim wrote:
> On Tue, Mar 22, 2016 at 05:20:08PM +0900, Joonsoo Kim wrote:
> > 2016-03-22 17:00 GMT+09:00 Minchan Kim <minchan@kernel.org>:
> > > On Tue, Mar 22, 2016 at 02:08:59PM +0900, Joonsoo Kim wrote:
> > >> On Fri, Mar 18, 2016 at 04:58:31PM +0900, Minchan Kim wrote:
> > >> > <b430e9d1c6d4> "remove compressed copy from zram in-memory"
> > >> > applied swap_slot_free_notify call in *end_swap_bio_read* to
> > >> > remove duplicated memory between zram and memory.
> > >> >
> > >> > However, with introducing rw_page in zram <8c7f01025f7b>
> > >> > "zram: implement rw_page operation of zram", it became void
> > >> > because rw_page doesn't need bio.
> > >> >
> > >> > This patch restores the function for rw_page.
> > >> >
> > >> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> > >> > ---
> > >> >  mm/page_io.c | 93 ++++++++++++++++++++++++++++++++----------------------------
> > >> >  1 file changed, 50 insertions(+), 43 deletions(-)
> > >> >
> > >> > diff --git a/mm/page_io.c b/mm/page_io.c
> > >> > index ff74e512f029..18aac7819cc9 100644
> > >> > --- a/mm/page_io.c
> > >> > +++ b/mm/page_io.c
> > >> > @@ -66,6 +66,54 @@ void end_swap_bio_write(struct bio *bio)
> > >> >     bio_put(bio);
> > >> >  }
> > >> >
> > >> > +static void swap_slot_free_notify(struct page *page)
> > >> > +{
> > >> > +   struct swap_info_struct *sis;
> > >> > +   struct gendisk *disk;
> > >> > +
> > >> > +   /*
> > >> > +    * There is no guarantee that the page is in swap cache - the software
> > >> > +    * suspend code (at least) uses end_swap_bio_read() against a non-
> > >> > +    * swapcache page.  So we must check PG_swapcache before proceeding with
> > >> > +    * this optimization.
> > >> > +    */
> > >> > +   if (unlikely(!PageSwapCache(page)))
> > >> > +           return;
> > >> > +
> > >> > +   sis = page_swap_info(page);
> > >> > +   if (!(sis->flags & SWP_BLKDEV))
> > >> > +           return;
> > >> > +
> > >> > +   /*
> > >> > +    * The swap subsystem performs lazy swap slot freeing,
> > >> > +    * expecting that the page will be swapped out again.
> > >> > +    * So we can avoid an unnecessary write if the page
> > >> > +    * isn't redirtied.
> > >> > +    * This is good for real swap storage because we can
> > >> > +    * reduce unnecessary I/O and enhance wear-leveling
> > >> > +    * if an SSD is used as the as swap device.
> > >> > +    * But if in-memory swap device (eg zram) is used,
> > >> > +    * this causes a duplicated copy between uncompressed
> > >> > +    * data in VM-owned memory and compressed data in
> > >> > +    * zram-owned memory.  So let's free zram-owned memory
> > >> > +    * and make the VM-owned decompressed page *dirty*,
> > >> > +    * so the page should be swapped out somewhere again if
> > >> > +    * we again wish to reclaim it.
> > >> > +    */
> > >> > +   disk = sis->bdev->bd_disk;
> > >> > +   if (disk->fops->swap_slot_free_notify) {
> > >> > +           swp_entry_t entry;
> > >> > +           unsigned long offset;
> > >> > +
> > >> > +           entry.val = page_private(page);
> > >> > +           offset = swp_offset(entry);
> > >> > +
> > >> > +           SetPageDirty(page);
> > >> > +           disk->fops->swap_slot_free_notify(sis->bdev,
> > >> > +                           offset);
> > >> > +   }
> > >> > +}
> > >> > +
> > >> >  static void end_swap_bio_read(struct bio *bio)
> > >> >  {
> > >> >     struct page *page = bio->bi_io_vec[0].bv_page;
> > >> > @@ -81,49 +129,7 @@ static void end_swap_bio_read(struct bio *bio)
> > >> >     }
> > >> >
> > >> >     SetPageUptodate(page);
> > >> > -
> > >> > -   /*
> > >> > -    * There is no guarantee that the page is in swap cache - the software
> > >> > -    * suspend code (at least) uses end_swap_bio_read() against a non-
> > >> > -    * swapcache page.  So we must check PG_swapcache before proceeding with
> > >> > -    * this optimization.
> > >> > -    */
> > >> > -   if (likely(PageSwapCache(page))) {
> > >> > -           struct swap_info_struct *sis;
> > >> > -
> > >> > -           sis = page_swap_info(page);
> > >> > -           if (sis->flags & SWP_BLKDEV) {
> > >> > -                   /*
> > >> > -                    * The swap subsystem performs lazy swap slot freeing,
> > >> > -                    * expecting that the page will be swapped out again.
> > >> > -                    * So we can avoid an unnecessary write if the page
> > >> > -                    * isn't redirtied.
> > >> > -                    * This is good for real swap storage because we can
> > >> > -                    * reduce unnecessary I/O and enhance wear-leveling
> > >> > -                    * if an SSD is used as the as swap device.
> > >> > -                    * But if in-memory swap device (eg zram) is used,
> > >> > -                    * this causes a duplicated copy between uncompressed
> > >> > -                    * data in VM-owned memory and compressed data in
> > >> > -                    * zram-owned memory.  So let's free zram-owned memory
> > >> > -                    * and make the VM-owned decompressed page *dirty*,
> > >> > -                    * so the page should be swapped out somewhere again if
> > >> > -                    * we again wish to reclaim it.
> > >> > -                    */
> > >> > -                   struct gendisk *disk = sis->bdev->bd_disk;
> > >> > -                   if (disk->fops->swap_slot_free_notify) {
> > >> > -                           swp_entry_t entry;
> > >> > -                           unsigned long offset;
> > >> > -
> > >> > -                           entry.val = page_private(page);
> > >> > -                           offset = swp_offset(entry);
> > >> > -
> > >> > -                           SetPageDirty(page);
> > >> > -                           disk->fops->swap_slot_free_notify(sis->bdev,
> > >> > -                                           offset);
> > >> > -                   }
> > >> > -           }
> > >> > -   }
> > >> > -
> > >> > +   swap_slot_free_notify(page);
> > >> >  out:
> > >> >     unlock_page(page);
> > >> >     bio_put(bio);
> > >> > @@ -347,6 +353,7 @@ int swap_readpage(struct page *page)
> > >> >
> > >> >     ret = bdev_read_page(sis->bdev, swap_page_sector(page), page);
> > >> >     if (!ret) {
> > >> > +           swap_slot_free_notify(page);
> > >> >             count_vm_event(PSWPIN);
> > >> >             return 0;
> > >> >     }
> > >>
> > >> Hello,
> > >
> > > Hey Joonsoo,
> > >
> > >>
> > >> You need to check PageUpdate() or something because bdev_read_page()
> > >> can be asynchronous.
> > >
> > > I considered it but decided not to add the check :(.
> > > Because I couldn't justify what benfit we can have with the check.
> > > The swap_slot_free_notify is tightly coupled with zram for several
> > > years and zram have been worked synchronously. So if bdev_read_page
> > > returns 0, it means we already have read the page successfully.
> > > Even, when I looked up other rw_page user, it seems there is no async
> > > rw_page users at the moment.
> > 
> > Yes, I also looked up other rw_page users and found that
> > there is no async rw_page now.
> > 
> > > If there is someone want to use *async* rw_page && *swap_slot_free_noity*
> > > in future, we could add the check easily. But I hope anyone never use
> > > swap_slot_free_notify any more which is mess. :(
> > 
> > But, I think that we should add the check. If someone want it, how does
> > he/she know about it? Even, if someone makes zram to read/write
> > asynchronously, we can miss it easily. This is error-prone practice.
> 
> Okay, I don't have strong against it.
> If we really want to catch such case, let's add WARN_ON_ONCE.

I'm okay with it. But, please add code comment why WARN_ON_ONCE() is
added here.

Then,
Acked-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>

Thanks.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web