From: Minchan Kim <minchan@kernel.org>
To: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
linux-mm@kvack.org, linux-kernel@vger.kernel.org,
Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Subject: Re: [PATCH v6 11/12] zsmalloc: page migration support
Date: Tue, 24 May 2016 15:28:01 +0900 [thread overview]
Message-ID: <20160524062801.GB29094@bbox> (raw)
In-Reply-To: <20160524052824.GA496@swordfish>
On Tue, May 24, 2016 at 02:28:24PM +0900, Sergey Senozhatsky wrote:
> On (05/20/16 23:23), Minchan Kim wrote:
> [..]
> > +static int get_zspage_isolation(struct zspage *zspage)
> > +{
> > + return zspage->isolated;
> > +}
> > +
>
> may be is_zspage_isolated()?
Now, it would be better. I will change it.
>
> [..]
> > @@ -502,23 +556,19 @@ static int get_size_class_index(int size)
> > static inline void zs_stat_inc(struct size_class *class,
> > enum zs_stat_type type, unsigned long cnt)
> > {
> > - if (type < NR_ZS_STAT_TYPE)
> > - class->stats.objs[type] += cnt;
> > + class->stats.objs[type] += cnt;
> > }
> >
> > static inline void zs_stat_dec(struct size_class *class,
> > enum zs_stat_type type, unsigned long cnt)
> > {
> > - if (type < NR_ZS_STAT_TYPE)
> > - class->stats.objs[type] -= cnt;
> > + class->stats.objs[type] -= cnt;
> > }
> >
> > static inline unsigned long zs_stat_get(struct size_class *class,
> > enum zs_stat_type type)
> > {
> > - if (type < NR_ZS_STAT_TYPE)
> > - return class->stats.objs[type];
> > - return 0;
> > + return class->stats.objs[type];
> > }
>
> hmm... the ordering of STAT types and those if-conditions were here for
> a reason:
>
> commit 6fe5186f0c7c18a8beb6d96c21e2390df7a12375
> Author: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
> Date: Fri Nov 6 16:29:38 2015 -0800
>
> zsmalloc: reduce size_class memory usage
>
> Each `struct size_class' contains `struct zs_size_stat': an array of
> NR_ZS_STAT_TYPE `unsigned long'. For zsmalloc built with no
> CONFIG_ZSMALLOC_STAT this results in a waste of `2 * sizeof(unsigned
> long)' per-class.
>
> The patch removes unneeded `struct zs_size_stat' members by redefining
> NR_ZS_STAT_TYPE (max stat idx in array).
>
> Since both NR_ZS_STAT_TYPE and zs_stat_type are compile time constants,
> GCC can eliminate zs_stat_inc()/zs_stat_dec() calls that use zs_stat_type
> larger than NR_ZS_STAT_TYPE: CLASS_ALMOST_EMPTY and CLASS_ALMOST_FULL at
> the moment.
>
> ./scripts/bloat-o-meter mm/zsmalloc.o.old mm/zsmalloc.o.new
> add/remove: 0/0 grow/shrink: 0/3 up/down: 0/-39 (-39)
> function old new delta
> fix_fullness_group 97 94 -3
> insert_zspage 100 86 -14
> remove_zspage 141 119 -22
>
> To summarize:
> a) each class now uses less memory
> b) we avoid a number of dec/inc stats (a minor optimization,
> but still).
>
> The gain will increase once we introduce additional stats.
>
> so it helped to eliminate instructions at compile time from a very hot
> path for !CONFIG_ZSMALLOC_STAT builds (which is 99% of the builds I think,
> I doubt anyone apart from us is using ZSMALLOC_STAT).
Most important point to me is that it makes code *simple* at the cost of
addtional wasting memory. Now, every zspage lives in *a* list so we don't
need to check zspage groupness to use list_empty of zspage.
I'm not sure how you feel it makes code simple a lot.
However, while I implement page migration logic, the check with condition
that zspage's groupness is either almost_empty and almost_full is really
bogus and tricky to me so I should debug several time to find what's
wrong.
Compared to old, zsmalloc is complicated day by day so I want to weight
on *simple* for easy maintainance.
One more note:
Now, ZS_EMPTY is used as pool. Look at find_get_zspage. So adding
"empty" column in ZSMALLOC_STAT might be worth but I wanted to handle it
as another topic.
So if you don't feel strong the saving is really huge, I want to
go with this. And if we are adding more wasted memory in future,
let's handle it then.
About CONFIG_ZSMALLOC_STAT, It might be off-topic. Frankly speaking,
I have guided production team to enable it because when I profile the
overhead caused by ZSMALLOC_STAT, there is no performance lost
in real workload. However, the stat gives more detailed useful
information.
>
>
> [..]
> > +static int get_first_obj_offset(struct size_class *class,
> > + struct page *first_page, struct page *page)
> > {
> > - return page->next;
> > + int pos, bound;
> > + int page_idx = 0;
> > + int ofs = 0;
> > + struct page *cursor = first_page;
> > +
> > + if (first_page == page)
> > + goto out;
> > +
> > + while (page != cursor) {
> > + page_idx++;
> > + cursor = get_next_page(cursor);
> > + }
> > +
> > + bound = PAGE_SIZE * page_idx;
>
> 'bound' not used.
-_-;;
>
>
> > + pos = (((class->objs_per_zspage * class->size) *
> > + page_idx / class->pages_per_zspage) / class->size
> > + ) * class->size;
>
>
> something went wrong with the indentation here :)
>
> so... it's
>
> (((class->objs_per_zspage * class->size) * page_idx / class->pages_per_zspage) / class->size ) * class->size;
>
> the last ' / class->size ) * class->size' can be dropped, I think.
You prove I didn't learn math.
Will drop it.
>
> [..]
> > + pos += class->size;
> > + }
> > +
> > + /*
> > + * Here, any user cannot access all objects in the zspage so let's move.
> "no one can access any object" ?
>
> [..]
> > + spin_lock(&class->lock);
> > + dec_zspage_isolation(zspage);
> > + if (!get_zspage_isolation(zspage)) {
> > + fg = putback_zspage(class, zspage);
> > + /*
> > + * Due to page_lock, we cannot free zspage immediately
> > + * so let's defer.
> > + */
> > + if (fg == ZS_EMPTY)
> > + schedule_work(&pool->free_work);
>
> hm... zsmalloc is getting sooo complex now.
>
> `system_wq' -- can we have problems here when the system is getting
> low on memory and workers are getting increasingly busy trying to
> allocate the memory for some other purposes?
>
> _theoretically_ zsmalloc can stack a number of ready-to-release zspages,
> which won't be accessible to zsmalloc, nor will they be released. how likely
> is this? hm, can zsmalloc take zspages from that deferred release list when
> it wants to allocate a new zspage?
Done.
>
> do you also want to kick the deferred page release from the shrinker
> callback, for example?
Yeb, it can be. I will do it at next revision. :)
Thanks!
>
> -ss
next prev parent reply other threads:[~2016-05-24 6:27 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-05-20 14:23 [PATCH v6 00/12] Support non-lru page migration Minchan Kim
2016-05-20 14:23 ` [PATCH v6 01/12] mm: use put_page to free page instead of putback_lru_page Minchan Kim
2016-05-20 14:23 ` [PATCH v6 02/12] mm: migrate: support non-lru movable page migration Minchan Kim
2016-05-27 14:26 ` Vlastimil Babka
2016-05-30 1:33 ` Minchan Kim
2016-05-30 9:01 ` Vlastimil Babka
2016-05-30 1:39 ` PATCH v6v2 " Minchan Kim
2016-05-30 9:36 ` Vlastimil Babka
2016-05-30 16:25 ` Minchan Kim
2016-05-31 7:51 ` Vlastimil Babka
2016-05-31 0:01 ` [PATCH v6v3 " Minchan Kim
2016-05-31 7:52 ` Vlastimil Babka
2016-05-31 23:05 ` Minchan Kim
2016-06-13 9:38 ` Anshuman Khandual
2016-06-15 2:32 ` Minchan Kim
2016-06-15 6:45 ` Anshuman Khandual
2016-06-16 0:26 ` Minchan Kim
2016-06-16 3:42 ` Anshuman Khandual
2016-06-16 5:37 ` Minchan Kim
2016-06-27 5:51 ` Anshuman Khandual
2016-06-28 6:39 ` Minchan Kim
2016-06-30 5:56 ` Anshuman Khandual
2016-06-30 6:18 ` Minchan Kim
2016-05-20 14:23 ` [PATCH v6 03/12] mm: balloon: use general non-lru movable page feature Minchan Kim
2016-05-30 12:16 ` Vlastimil Babka
2016-05-20 14:23 ` [PATCH v6 04/12] zsmalloc: keep max_object in size_class Minchan Kim
2016-05-20 14:23 ` [PATCH v6 05/12] zsmalloc: use bit_spin_lock Minchan Kim
2016-05-20 14:23 ` [PATCH v6 06/12] zsmalloc: use accessor Minchan Kim
2016-05-20 14:23 ` [PATCH v6 07/12] zsmalloc: factor page chain functionality out Minchan Kim
2016-05-20 14:23 ` [PATCH v6 08/12] zsmalloc: introduce zspage structure Minchan Kim
2016-05-20 14:23 ` [PATCH v6 09/12] zsmalloc: separate free_zspage from putback_zspage Minchan Kim
2016-05-20 14:23 ` [PATCH v6 10/12] zsmalloc: use freeobj for index Minchan Kim
2016-05-20 14:23 ` [PATCH v6 11/12] zsmalloc: page migration support Minchan Kim
2016-05-24 5:28 ` Sergey Senozhatsky
2016-05-24 6:28 ` Minchan Kim [this message]
2016-05-24 8:05 ` Sergey Senozhatsky
2016-05-24 8:17 ` Minchan Kim
2016-05-25 5:14 ` Minchan Kim
2016-05-25 15:23 ` Sergey Senozhatsky
2016-05-26 0:32 ` Minchan Kim
2016-05-26 0:59 ` Sergey Senozhatsky
2016-05-26 4:37 ` Minchan Kim
2016-05-26 21:50 ` [PATCH v6r2 " Minchan Kim
2016-05-20 14:23 ` [PATCH v6 12/12] zram: use __GFP_MOVABLE for memory allocation Minchan Kim
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160524062801.GB29094@bbox \
--to=minchan@kernel.org \
--cc=akpm@linux-foundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=sergey.senozhatsky.work@gmail.com \
--cc=sergey.senozhatsky@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).