LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Vladimir Davydov <vdavydov.dev@gmail.com>
To: Kirill Tkhai <ktkhai@virtuozzo.com>
Cc: akpm@linux-foundation.org, shakeelb@google.com,
	viro@zeniv.linux.org.uk, hannes@cmpxchg.org, mhocko@kernel.org,
	tglx@linutronix.de, pombredanne@nexb.com,
	stummala@codeaurora.org, gregkh@linuxfoundation.org,
	sfr@canb.auug.org.au, guro@fb.com, mka@chromium.org,
	penguin-kernel@I-love.SAKURA.ne.jp, chris@chris-wilson.co.uk,
	longman@redhat.com, minchan@kernel.org, hillf.zj@alibaba-inc.com,
	ying.huang@intel.com, mgorman@techsingularity.net, jbacik@fb.com,
	linux@roeck-us.net, linux-kernel@vger.kernel.org,
	linux-mm@kvack.org, willy@infradead.org, lirongqing@baidu.com,
	aryabinin@virtuozzo.com
Subject: Re: [PATCH v2 04/12] mm: Assign memcg-aware shrinkers bitmap to memcg
Date: Tue, 24 Apr 2018 15:15:16 +0300	[thread overview]
Message-ID: <20180424121516.ihn6lewpidc34ayl@esperanza> (raw)
In-Reply-To: <7bf5372d-7d9d-abee-27dd-5044da5ec489@virtuozzo.com>

On Tue, Apr 24, 2018 at 02:38:51PM +0300, Kirill Tkhai wrote:
> On 24.04.2018 14:28, Vladimir Davydov wrote:
> > On Mon, Apr 23, 2018 at 01:54:50PM +0300, Kirill Tkhai wrote:
> >>>> @@ -1200,6 +1206,8 @@ extern int memcg_nr_cache_ids;
> >>>>  void memcg_get_cache_ids(void);
> >>>>  void memcg_put_cache_ids(void);
> >>>>  
> >>>> +extern int shrinkers_max_nr;
> >>>> +
> >>>
> >>> memcg_shrinker_id_max?
> >>
> >> memcg_shrinker_id_max sounds like an includive value, doesn't it?
> >> While shrinker->id < shrinker_max_nr.
> >>
> >> Let's better use memcg_shrinker_nr_max.
> > 
> > or memcg_nr_shrinker_ids (to match memcg_nr_cache_ids), not sure...
> > 
> > Come to think of it, this variable is kinda awkward: it is defined in
> > vmscan.c but declared in memcontrol.h; it is used by vmscan.c for max
> > shrinker id and by memcontrol.c for shrinker map capacity. Just a raw
> > idea: what about splitting it in two: one is private to vmscan.c, used
> > as max id, say we call it shrinker_id_max; the other is defined in
> > memcontrol.c and is used for shrinker map capacity, say we call it
> > memcg_shrinker_map_capacity. What do you think?
> 
> I don't much like a duplication of the single variable...

Well, it's not really a duplication. For example, shrinker_id_max could
decrease when a shrinker is unregistered while shrinker_map_capacity can
only grow exponentially.

> Are there real problems, if it defined in memcontrol.{c,h} and use in
> both of the places?

The code is more difficult to follow when variables are shared like that
IMHO. I suggest you try it and see how it looks. May be, it will only
get worse and we'll have to revert to what we have now. Difficult to say
without seeing the code.

>  
> >>>> +int expand_shrinker_maps(int old_nr, int nr)
> >>>> +{
> >>>> +	int id, size, old_size, node, ret;
> >>>> +	struct mem_cgroup *memcg;
> >>>> +
> >>>> +	old_size = old_nr / BITS_PER_BYTE;
> >>>> +	size = nr / BITS_PER_BYTE;
> >>>> +
> >>>> +	down_write(&shrinkers_max_nr_rwsem);
> >>>> +	for_each_node(node) {
> >>>
> >>> Iterating over cgroups first, numa nodes second seems like a better idea
> >>> to me. I think you should fold for_each_node in memcg_expand_maps.
> >>>
> >>>> +		idr_for_each_entry(&mem_cgroup_idr, memcg, id) {
> >>>
> >>> Iterating over mem_cgroup_idr looks strange. Why don't you use
> >>> for_each_mem_cgroup?
> >>
> >> We want to allocate shrinkers maps in mem_cgroup_css_alloc(), since
> >> mem_cgroup_css_online() mustn't fail (it's a requirement of currently
> >> existing design of memcg_cgroup::id).
> >>
> >> A new memcg is added to parent's list between two of these calls:
> >>
> >> css_create()
> >>   ss->css_alloc()
> >>   list_add_tail_rcu(&css->sibling, &parent_css->children)
> >>   ss->css_online()
> >>
> >> for_each_mem_cgroup() does not see allocated, but not linked children.
> > 
> > Why don't we move shrinker map allocation to css_online then?
> 
> Because the design of memcg_cgroup::id prohibits mem_cgroup_css_online() to fail.
> This function can't fail.

I fail to understand why it is so. Could you please elaborate?

> 
> I don't think it will be good to dive into reworking of this stuff for this patchset,
> which is really already big. Also, it will be assymmetric to allocate one part of
> data in css_alloc(), while another data in css_free(). This breaks cgroup design,
> which specially introduces this two function to differ allocation and onlining.
> Also, I've just move the allocation to alloc_mem_cgroup_per_node_info() like it was
> suggested in comments to v1...

Yeah, but (ab)using mem_cgroup_idr for iterating over all allocated
memory cgroups looks rather dubious to me...

  reply	other threads:[~2018-04-24 12:15 UTC|newest]

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-17 18:52 [PATCH v2 00/12] Improve shrink_slab() scalability (old complexity was O(n^2), new is O(n)) Kirill Tkhai
2018-04-17 18:53 ` [PATCH v2 01/12] mm: Assign id to every memcg-aware shrinker Kirill Tkhai
2018-04-18 14:14   ` Tetsuo Handa
2018-04-18 14:27     ` Kirill Tkhai
2018-04-18 14:32       ` Tetsuo Handa
2018-04-18 15:02         ` Kirill Tkhai
2018-04-22 17:16   ` Vladimir Davydov
2018-04-17 18:53 ` [PATCH v2 02/12] memcg: Refactoring in mem_cgroup_alloc() Kirill Tkhai
2018-04-17 18:53 ` [PATCH v2 03/12] memcg: Refactoring in alloc_mem_cgroup_per_node_info() Kirill Tkhai
2018-04-17 18:53 ` [PATCH v2 04/12] mm: Assign memcg-aware shrinkers bitmap to memcg Kirill Tkhai
2018-04-18 12:55   ` kbuild test robot
2018-04-18 13:05     ` Kirill Tkhai
2018-04-22 17:59   ` Vladimir Davydov
2018-04-23 10:54     ` Kirill Tkhai
2018-04-24 11:28       ` Vladimir Davydov
2018-04-24 11:38         ` Kirill Tkhai
2018-04-24 12:15           ` Vladimir Davydov [this message]
2018-04-24 12:24             ` Kirill Tkhai
2018-04-28 15:08               ` Vladimir Davydov
2018-05-03 11:15                 ` Kirill Tkhai
2018-04-24 12:13         ` Kirill Tkhai
2018-04-23 11:02     ` Kirill Tkhai
2018-04-23 11:06     ` Kirill Tkhai
2018-04-24 11:08       ` Vladimir Davydov
2018-04-17 18:53 ` [PATCH v2 05/12] fs: Propagate shrinker::id to list_lru Kirill Tkhai
2018-04-22 18:03   ` Vladimir Davydov
2018-04-17 18:53 ` [PATCH v2 06/12] list_lru: Add memcg argument to list_lru_from_kmem() Kirill Tkhai
2018-04-17 18:54 ` [PATCH v2 07/12] list_lru: Pass dst_memcg argument to memcg_drain_list_lru_node() Kirill Tkhai
2018-04-17 18:54 ` [PATCH v2 08/12] list_lru: Pass lru " Kirill Tkhai
2018-04-17 18:54 ` [PATCH v2 09/12] mm: Set bit in memcg shrinker bitmap on first list_lru item apearance Kirill Tkhai
2018-04-17 18:54 ` [PATCH v2 10/12] mm: Iterate only over charged shrinkers during memcg shrink_slab() Kirill Tkhai
2018-04-22 18:19   ` Vladimir Davydov
2018-04-17 18:54 ` [PATCH v2 11/12] mm: Add SHRINK_EMPTY shrinker methods return value Kirill Tkhai
2018-04-17 18:54 ` [PATCH v2 12/12] mm: Clear shrinker bit if there are no objects related to memcg Kirill Tkhai
2018-04-22 18:21   ` Vladimir Davydov
2018-04-23 10:01     ` Kirill Tkhai
2018-04-24 10:56       ` Vladimir Davydov

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=20180424121516.ihn6lewpidc34ayl@esperanza \
    --to=vdavydov.dev@gmail.com \
    --cc=akpm@linux-foundation.org \
    --cc=aryabinin@virtuozzo.com \
    --cc=chris@chris-wilson.co.uk \
    --cc=gregkh@linuxfoundation.org \
    --cc=guro@fb.com \
    --cc=hannes@cmpxchg.org \
    --cc=hillf.zj@alibaba-inc.com \
    --cc=jbacik@fb.com \
    --cc=ktkhai@virtuozzo.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=linux@roeck-us.net \
    --cc=lirongqing@baidu.com \
    --cc=longman@redhat.com \
    --cc=mgorman@techsingularity.net \
    --cc=mhocko@kernel.org \
    --cc=minchan@kernel.org \
    --cc=mka@chromium.org \
    --cc=penguin-kernel@I-love.SAKURA.ne.jp \
    --cc=pombredanne@nexb.com \
    --cc=sfr@canb.auug.org.au \
    --cc=shakeelb@google.com \
    --cc=stummala@codeaurora.org \
    --cc=tglx@linutronix.de \
    --cc=viro@zeniv.linux.org.uk \
    --cc=willy@infradead.org \
    --cc=ying.huang@intel.com \
    --subject='Re: [PATCH v2 04/12] mm: Assign memcg-aware shrinkers bitmap to memcg' \
    /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

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).