Re: [RFC][PATCHv3 7/7] zsmalloc: register a shrinker to trigger auto-compaction

[Sergey Senozhatsky]

Hello,

thanks for review.

On (06/29/15 16:07), Minchan Kim wrote:
[..]
> >  			if (!migrate_zspage(pool, class, &cc))
> > -				break;
> > +				goto out;
> 
> It should retry with another target_page instead of going out.

yep.

[..]
> > +static unsigned long zs_shrinker_scan(struct shrinker *shrinker,
> > +		struct shrink_control *sc)
> > +{
[..]
> 
> Returns migrated object count.
> 
[..]
> > +static unsigned long zs_shrinker_count(struct shrinker *shrinker,
> > +		struct shrink_control *sc)
> > +{
[..]
> 
> But it returns wasted_obj / max_obj_per_zspage?
> 

Good catch.
So,  __zs_compact() and zs_shrinker_count() are ok. Returning
"wasted_obj / max_obj_per_zspage" from zs_can_compact() makes
sense there. The only place is zs_shrinker_scan()->zs_compact().

Hm, I can think of:

(a) We can change zs_compact() to return the total number of
freed zspages. That will not really change a user visible
interface. We export (fwiw) the number of compacted objects
in mm_stat. Basically, this is internal zsmalloc() counter and
no user space program can ever do anything with that data. From
that prospective we will just replace one senseless number with
another (equally meaningless) one.


(b) replace zs_compact() call in zs_shrinker_scan() with a class loop

1764         int i;
1765         unsigned long nr_migrated = 0;
1766         struct size_class *class;
1767
1768         for (i = zs_size_classes - 1; i >= 0; i--) {
1769                 class = pool->size_class[i];
1770                 if (!class)
1771                         continue;
1772                 if (class->index != i)
1773                         continue;
1774                 nr_migrated += __zs_compact(pool, class);
1775         }
1776
1777         return nr_migrated;

But on every iteration increment nr_migrated with
		"nr_migrated += just_migrated / max_obj_per_zspage"

(which will be unnecessary if zs_compact() will return the number of freed
zspages).

So, (b) is mostly fine, except that we already have several pool->size_class
loops, with same `if (!class)' and `if (class->index...)' checks; and it
asks for some sort of refactoring or... a tricky for_each_class() define.


In both cases, however, we don't tell anything valuable to user space.
Thus,

(c) Return from zs_compact() the number of pages (PAGE_SIZE) freed.
And change compaction to operate in terms of pages (PAGE_SIZE). At
least mm_stat::compacted will turn into something useful for user
space.

	-ss
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/