linux-kernel - Re: [PATCHSET cgroup/for-3.12] cgroup: make cgroup

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]

Message-ID: <20130806161509.GB10779@mtj.dyndns.org>
Date:	Tue, 6 Aug 2013 12:15:09 -0400
From:	Tejun Heo <tj@...nel.org>
To:	Michal Hocko <mhocko@...e.cz>
Cc:	lizefan@...wei.com, hannes@...xchg.org, bsingharora@...il.com,
	kamezawa.hiroyu@...fujitsu.com, cgroups@...r.kernel.org,
	linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCHSET cgroup/for-3.12] cgroup: make cgroup_event specific to
 memcg

Hello, Michal.

On Tue, Aug 06, 2013 at 05:58:04PM +0200, Michal Hocko wrote:
> I am objecting to moving the generic part of that code into memcg. The
> memcg part and the additional complexity (all the parsing and conditions
> for signalling) is already in the memcg code.

But how is it generic if it's specific to memcg?  The practical
purpose here is making it clear that the interface is only used by
memcg and preventing any new usages from sprining up and the best way
to achieve that is making the code actually memcg-specific.  It also
helps cleaning up cftype in general.  I'm not sure what you're
objecting to here.

> Such an interface would be really welcome but I would also ask how
> it would implement/allow context passing. E.g. how do we know which
> treshold has been reached? How do we find out the vmpressure level? Is
> the consumer supposed to do an additional action after it gets
> notification?
> Etc.

Yeap, exactly and that's how it should have been from the beginning.
Attaching information to notification itself isn't a particularly good
design (anyone remembers rtsig?) if there's polling mechanism to
report the current state.  It essentially amounts to duplicate
delivery mechanisms for the same information, which you usually don't
want.  Here, the inconvenience / performance implications are
negligible or even net-positive.  Plain file modified notification is
way more familiar / conventional and the overhead of an extra read
call, which is highly unlikely to be relevant given the expected
frequency of the events we're talking about, is small compared to the
action of event delivery and context switch.

> Really that natural? So memcg should touch internals like cgroup dentry

Functionally, it is completely specific to memcg at this point.  It's
the only user and will stay the only user.

> reference counting. You seem have forgotten all the hassles with
> cgroup_mutex, haven't you?

Was the above sentence necessary?

> No that part doesn't belong to memcg! You can discourage from new usage
> of this interface of course.

Oh, if you're objecting to the details of the implementation, we of
course can clean it up.  It should conceptually and functionally be
part of memcg and that is the guiding line we follow.  Implementations
follow the concepts and functions, not the other way around.  The
refcnt of course can be replaced with memcg css refcnting and we can
of course factor out dentry comparison in a prettier form.

Compare it to the other way around - having event callbacks in cftype
and clearing code embedded in cgroup core destruction path when both
of which are completely irrelevant to all other controllers.  Let's
clean up the implementation details and put things where they belong.
What's the excuse for not doing so when it's almost trivially doable?

Thanks.

-- 
tejun
--
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/