linux-kernel - Re: [PATCH 2/2] memcg: move memcg_update_cache_size to slab

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 22 Sep 2014 16:49:10 +0200
From:	Michal Hocko <mhocko@...e.cz>
To:	Vladimir Davydov <vdavydov@...allels.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org, linux-mm@...ck.org,
	Johannes Weiner <hannes@...xchg.org>,
	Christoph Lameter <cl@...ux.com>
Subject: Re: [PATCH 2/2] memcg: move memcg_update_cache_size to slab_common.c

On Mon 22-09-14 18:20:35, Vladimir Davydov wrote:
> On Mon, Sep 22, 2014 at 04:07:34PM +0200, Michal Hocko wrote:
> > On Thu 18-09-14 19:50:20, Vladimir Davydov wrote:
> > > The only reason why this function lives in memcontrol.c is that it
> > > depends on memcg_caches_array_size. However, we can pass the new array
> > > size immediately to it instead of new_id+1 so that it will be free of
> > > any memcontrol.c dependencies.
> > > 
> > > So let's move this function to slab_common.c and make it static.
> > 
> > Why?
> 
> Jumping from memcontrol.c to slab_common.c and then back to memcontrol.c
> while updating per-memcg caches looks ugly IMO. We can do the update on
> the slab's side.

Then put this into the patch description. I am always kind of lost when
following all those slab <-> memcg jumps so I definitely do not have
anything against cleaning this up. But please be explicit about your
motivation about the change and put it into the changelog. Things might
be obvious for you when you are deeply familiar with the code but the
poor reviewer has to build up the whole thing from scratch usually.

> > besides that the patch does more code reshuffling which should be
> > documented. I have got lost a bit to be honest.
> 
> It just makes it sane :-) Currently we walk over all slab caches each
> time new kmemcg is created even if memcg_limited_groups_array_size
> doesn't grow and we've actually nothing to do. So it moves cache id
> allocation stuff to a separate function (memcg_alloc_cache_id) and
> places the check there so that memcg_update_all_caches is only called
> when it's really necessary.
> 
> I'm sorry if it confuses you. I thought the patch isn't big and rather
> easy to understand :-/ Next time will split better.

This would be worth a separate patch then and explain all of this.

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