[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20180521175640.twrlrqkg7bxoqowa@esperanza>
Date: Mon, 21 May 2018 20:56:40 +0300
From: Vladimir Davydov <vdavydov.dev@...il.com>
To: Kirill Tkhai <ktkhai@...tuozzo.com>
Cc: akpm@...ux-foundation.org, shakeelb@...gle.com,
viro@...iv.linux.org.uk, hannes@...xchg.org, mhocko@...nel.org,
tglx@...utronix.de, pombredanne@...b.com, stummala@...eaurora.org,
gregkh@...uxfoundation.org, sfr@...b.auug.org.au, guro@...com,
mka@...omium.org, penguin-kernel@...ove.SAKURA.ne.jp,
chris@...is-wilson.co.uk, longman@...hat.com, minchan@...nel.org,
ying.huang@...el.com, mgorman@...hsingularity.net, jbacik@...com,
linux@...ck-us.net, linux-kernel@...r.kernel.org,
linux-mm@...ck.org, willy@...radead.org, lirongqing@...du.com,
aryabinin@...tuozzo.com
Subject: Re: [PATCH v6 14/17] mm: Iterate only over charged shrinkers during
memcg shrink_slab()
On Mon, May 21, 2018 at 12:17:07PM +0300, Kirill Tkhai wrote:
> >> +static unsigned long shrink_slab_memcg(gfp_t gfp_mask, int nid,
> >> + struct mem_cgroup *memcg, int priority)
> >> +{
> >> + struct memcg_shrinker_map *map;
> >> + unsigned long freed = 0;
> >> + int ret, i;
> >> +
> >> + if (!memcg_kmem_enabled() || !mem_cgroup_online(memcg))
> >> + return 0;
> >> +
> >> + if (!down_read_trylock(&shrinker_rwsem))
> >> + return 0;
> >> +
> >> + /*
> >> + * 1) Caller passes only alive memcg, so map can't be NULL.
> >> + * 2) shrinker_rwsem protects from maps expanding.
> >> + */
> >> + map = rcu_dereference_protected(memcg->nodeinfo[nid]->shrinker_map,
> >> + true);
> >> + BUG_ON(!map);
> >> +
> >> + for_each_set_bit(i, map->map, memcg_shrinker_nr_max) {
> >> + struct shrink_control sc = {
> >> + .gfp_mask = gfp_mask,
> >> + .nid = nid,
> >> + .memcg = memcg,
> >> + };
> >> + struct shrinker *shrinker;
> >> +
> >> + shrinker = idr_find(&shrinker_idr, i);
> >> + if (unlikely(!shrinker)) {
> >
> > Nit: I don't think 'unlikely' is required here as this is definitely not
> > a hot path.
>
> In case of big machines with many containers and overcommit, shrink_slab()
> in general is very hot path. See the patchset description. There are configurations,
> when only shrink_slab() is executing and occupies cpu for 100%, it's the reason
> of this patchset is made for.
>
> Here is the place we are absolutely sure shrinker is NULL in case if race with parallel
> registering, so I don't see anything wrong to give compiler some information about branch
> prediction.
OK. If you're confident this 'unlikely' is useful, let's leave it as is.
Powered by blists - more mailing lists