Re: [PATCH v5 6/7] zsmalloc: account the number of compacted pages

Re: [PATCH v5 6/7] zsmalloc: account the number of compacted pages

[Minchan Kim]

On Mon, Jul 06, 2015 at 09:17:49PM +0900, Sergey Senozhatsky wrote:
> Compaction returns back to zram the number of migrated objects,
> which is quite uninformative -- we have objects of different
> sizes so user space cannot obtain any valuable data from that
> number. Change compaction to operate in terms of pages and
> return back to compaction issuer the number of pages that
> were freed during compaction. So from now on `num_compacted'
> column in zram<id>/mm_stat represents more meaningful value:
> the number of freed (compacted) pages.

Fair enough.
 
The main reason I introduced num_migrated is to investigate
the effieciency of compaction. ie, num_freed / num_migrated.
However, I didn't put num_freed at that time so I can't get 
my goal with only num_migrated.
 
We could put new knob num_compacted as well as num_migrated
but I don't think we need it now. Zram's compaction would be
much efficient compared to VM's compaction because we don't
have any non-movable objects in zspages and we can account
exact number of free slots.

So, I want to change name from num_migrated to num_compacted
and maintain only it which is more useful for admin, you said.

It's not far since we introduced num_migrated so I don't think
change name wouldn't be a big problem for userspace,(I hope).

> 
> Return first_page's fullness_group from putback_zspage(),
> so we now for sure know that putback_zspage() has issued
> free_zspage() and we must update compaction stats.
> 
> Update documentation.
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
>  Documentation/blockdev/zram.txt |  3 ++-
>  mm/zsmalloc.c                   | 27 +++++++++++++++++----------
>  2 files changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/Documentation/blockdev/zram.txt b/Documentation/blockdev/zram.txt
> index c4de576..71f4744 100644
> --- a/Documentation/blockdev/zram.txt
> +++ b/Documentation/blockdev/zram.txt
> @@ -144,7 +144,8 @@ mem_used_max      RW    the maximum amount memory zram have consumed to
>                          store compressed data
>  mem_limit         RW    the maximum amount of memory ZRAM can use to store
>                          the compressed data
> -num_migrated      RO    the number of objects migrated migrated by compaction
> +num_migrated      RO    the number of pages freed during compaction
> +                        (available only via zram<id>/mm_stat node)
>  compact           WO    trigger memory compaction
>  
>  WARNING
> diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
> index e0f508a..a761733 100644
> --- a/mm/zsmalloc.c
> +++ b/mm/zsmalloc.c
> @@ -245,7 +245,7 @@ struct zs_pool {
>  	/* Allocation flags used when growing pool */
>  	gfp_t			flags;
>  	atomic_long_t		pages_allocated;
> -	/* How many objects were migrated */
> +	/* How many pages were migrated (freed) */
>  	unsigned long		num_migrated;
>  
>  #ifdef CONFIG_ZSMALLOC_STAT
> @@ -1596,8 +1596,6 @@ struct zs_compact_control {
>  	 /* Starting object index within @s_page which used for live object
>  	  * in the subpage. */
>  	int index;
> -	/* How many of objects were migrated */
> -	int nr_migrated;
>  };
>  
>  static int migrate_zspage(struct zs_pool *pool, struct size_class *class,
> @@ -1634,7 +1632,6 @@ static int migrate_zspage(struct zs_pool *pool, struct size_class *class,
>  		record_obj(handle, free_obj);
>  		unpin_tag(handle);
>  		obj_free(pool, class, used_obj);
> -		cc->nr_migrated++;
>  	}
>  
>  	/* Remember last position in this iteration */
> @@ -1660,8 +1657,17 @@ static struct page *isolate_target_page(struct size_class *class)
>  	return page;
>  }
>  
> -static void putback_zspage(struct zs_pool *pool, struct size_class *class,
> -				struct page *first_page)
> +/*
> + * putback_zspage - add @first_page into right class's fullness list
> + * @pool: target pool
> + * @class: destination class
> + * @first_page: target page
> + *
> + * Return @fist_page's fullness_group
> + */
> +static enum fullness_group putback_zspage(struct zs_pool *pool,
> +			struct size_class *class,
> +			struct page *first_page)
>  {
>  	enum fullness_group fullness;
>  
> @@ -1679,6 +1685,8 @@ static void putback_zspage(struct zs_pool *pool, struct size_class *class,
>  
>  		free_zspage(first_page);
>  	}
> +
> +	return fullness;
>  }
>  
>  static struct page *isolate_source_page(struct size_class *class)
> @@ -1720,7 +1728,6 @@ static void __zs_compact(struct zs_pool *pool, struct size_class *class)
>  	struct page *src_page;
>  	struct page *dst_page = NULL;
>  
> -	cc.nr_migrated = 0;
>  	spin_lock(&class->lock);
>  	while ((src_page = isolate_source_page(class))) {
>  
> @@ -1749,7 +1756,9 @@ static void __zs_compact(struct zs_pool *pool, struct size_class *class)
>  			break;
>  
>  		putback_zspage(pool, class, dst_page);
> -		putback_zspage(pool, class, src_page);
> +		if (putback_zspage(pool, class, src_page) == ZS_EMPTY)
> +			pool->num_migrated +=
> +				get_pages_per_zspage(class->size);
>  		spin_unlock(&class->lock);
>  		cond_resched();
>  		spin_lock(&class->lock);
> @@ -1758,8 +1767,6 @@ static void __zs_compact(struct zs_pool *pool, struct size_class *class)
>  	if (src_page)
>  		putback_zspage(pool, class, src_page);
>  
> -	pool->num_migrated += cc.nr_migrated;
> -
>  	spin_unlock(&class->lock);
>  }
>  
> -- 
> 2.5.0.rc0.3.g912bd49
> 

-- 
Kind regards,
Minchan Kim

Re: [PATCH v5 6/7] zsmalloc: account the number of compacted pages

[Sergey Senozhatsky]

On (07/06/15 22:22), Minchan Kim wrote:
> On Mon, Jul 06, 2015 at 09:17:49PM +0900, Sergey Senozhatsky wrote:
> > Compaction returns back to zram the number of migrated objects,
> > which is quite uninformative -- we have objects of different
> > sizes so user space cannot obtain any valuable data from that
> > number. Change compaction to operate in terms of pages and
> > return back to compaction issuer the number of pages that
> > were freed during compaction. So from now on `num_compacted'
> > column in zram<id>/mm_stat represents more meaningful value:
> > the number of freed (compacted) pages.
> 
> Fair enough.
>  
> The main reason I introduced num_migrated is to investigate
> the effieciency of compaction. ie, num_freed / num_migrated.
> However, I didn't put num_freed at that time so I can't get 
> my goal with only num_migrated.
>  
> We could put new knob num_compacted as well as num_migrated
> but I don't think we need it now. Zram's compaction would be
> much efficient compared to VM's compaction because we don't
> have any non-movable objects in zspages and we can account
> exact number of free slots.
> 
> So, I want to change name from num_migrated to num_compacted
> and maintain only it which is more useful for admin, you said.
> 
> It's not far since we introduced num_migrated so I don't think
> change name wouldn't be a big problem for userspace,(I hope).
> 

Hello,

Yes, num_migrated rename patch was on my table.
I was thinking about two variants:

struct zs_pool
  atomic_long_t		pages_allocated;
  unsigned long		num_compacted;

or

struct zs_pool
  atomic_long_t		pages_allocated;
  unsigned long		pages_compacted;

the latter looks even better I think. But I didn't come up with a
sane API name to get these stats-- zs_get_pages_compacted() is a bit
misleading. So I decided to keep num_compacted to make the patch set
smaller.

Hm, exporting a new `struct zs_pool_stat' to zram is probably a good
way to go.

	-ss

Re: [PATCH v5 5/7] zsmalloc/zram: store compaction stats in zspool

[Sergey Senozhatsky]

On (07/06/15 22:27), Minchan Kim wrote:
> > `zs_compact_control' accounts the number of migrated objects but
> > it has a limited lifespan -- we lose it as soon as zs_compaction()
> > returns back to zram. It was fine, because (a) zram had it's own
> > counter of migrated objects and (b) only zram could trigger
> > compaction. However, this does not work for automatic pool
> > compaction (not issued by zram). To account objects migrated
> > during auto-compaction (issued by the shrinker) we need to store
> > this number in zs_pool.
> > 
> > A new zsmalloc zs_get_num_migrated() symbol exports zs_pool's
> > ->num_migrated counter, so we better start using it, rather than
> > continue keeping zram's own `num_migrated' copy in zram_stats.
> 
> If we introduce like this API we should make new another API when
> we want to introduce new stats. So I don't think it's a good idea.
> How about this?
> 
>         void zsmalloc_stats(struct zsmalloc_stats *stats);
> 
> So, we could return any upcoming stats without new API introduce.
> 

Hm, agree. Do you prefer me to fold this into this patch set or to do as
a separate work later?


P.S.

Sorry. Seems that my git send-email has some problems, so group-reply
in mutt does not work as expected.


	-ss

Re: [PATCH v5 6/7] zsmalloc: account the number of compacted pages

[Minchan Kim]

On Mon, Jul 06, 2015 at 10:48:50PM +0900, Sergey Senozhatsky wrote:
> On (07/06/15 22:22), Minchan Kim wrote:
> > On Mon, Jul 06, 2015 at 09:17:49PM +0900, Sergey Senozhatsky wrote:
> > > Compaction returns back to zram the number of migrated objects,
> > > which is quite uninformative -- we have objects of different
> > > sizes so user space cannot obtain any valuable data from that
> > > number. Change compaction to operate in terms of pages and
> > > return back to compaction issuer the number of pages that
> > > were freed during compaction. So from now on `num_compacted'
> > > column in zram<id>/mm_stat represents more meaningful value:
> > > the number of freed (compacted) pages.
> > 
> > Fair enough.
> >  
> > The main reason I introduced num_migrated is to investigate
> > the effieciency of compaction. ie, num_freed / num_migrated.
> > However, I didn't put num_freed at that time so I can't get 
> > my goal with only num_migrated.
> >  
> > We could put new knob num_compacted as well as num_migrated
> > but I don't think we need it now. Zram's compaction would be
> > much efficient compared to VM's compaction because we don't
> > have any non-movable objects in zspages and we can account
> > exact number of free slots.
> > 
> > So, I want to change name from num_migrated to num_compacted
> > and maintain only it which is more useful for admin, you said.
> > 
> > It's not far since we introduced num_migrated so I don't think
> > change name wouldn't be a big problem for userspace,(I hope).
> > 
> 
> Hello,
> 
> Yes, num_migrated rename patch was on my table.
> I was thinking about two variants:
> 
> struct zs_pool
>   atomic_long_t		pages_allocated;
>   unsigned long		num_compacted;
> 
> or
> 
> struct zs_pool
>   atomic_long_t		pages_allocated;
>   unsigned long		pages_compacted;
> 
> the latter looks even better I think. But I didn't come up with a

Yeb.

> sane API name to get these stats-- zs_get_pages_compacted() is a bit
> misleading. So I decided to keep num_compacted to make the patch set
> smaller.
> 
> Hm, exporting a new `struct zs_pool_stat' to zram is probably a good
> way to go.

Agreed.

> 
> 	-ss

-- 
Kind regards,
Minchan Kim

Re: [PATCH v5 5/7] zsmalloc/zram: store compaction stats in zspool

[Minchan Kim]

On Mon, Jul 06, 2015 at 10:56:46PM +0900, Sergey Senozhatsky wrote:
> On (07/06/15 22:27), Minchan Kim wrote:
> > > `zs_compact_control' accounts the number of migrated objects but
> > > it has a limited lifespan -- we lose it as soon as zs_compaction()
> > > returns back to zram. It was fine, because (a) zram had it's own
> > > counter of migrated objects and (b) only zram could trigger
> > > compaction. However, this does not work for automatic pool
> > > compaction (not issued by zram). To account objects migrated
> > > during auto-compaction (issued by the shrinker) we need to store
> > > this number in zs_pool.
> > > 
> > > A new zsmalloc zs_get_num_migrated() symbol exports zs_pool's
> > > ->num_migrated counter, so we better start using it, rather than
> > > continue keeping zram's own `num_migrated' copy in zram_stats.
> > 
> > If we introduce like this API we should make new another API when
> > we want to introduce new stats. So I don't think it's a good idea.
> > How about this?
> > 
> >         void zsmalloc_stats(struct zsmalloc_stats *stats);
> > 
> > So, we could return any upcoming stats without new API introduce.
> > 
> 
> Hm, agree. Do you prefer me to fold this into this patch set or to do as
> a separate work later?

Let's fold it so your next patch can use it for getting num_compacted.

> 
> 
> P.S.
> 
> Sorry. Seems that my git send-email has some problems, so group-reply
> in mutt does not work as expected.
> 
> 
> 	-ss

-- 
Kind regards,
Minchan Kim