On Tue 06-08-13 12:15:09, Tejun Heo wrote:

Hello, Michal.

On Tue, Aug 06, 2013 at 05:58:04PM +0200, Michal Hocko wrote:

quoted

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?

How is it specific to memcg? The fact only memcg uses the interface
doesn't imply it is memcg specific.

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. 

There are other ways to achieve the same. E.g. not ack new usage of
register callback users. We have done similar with other things like
use_hierarchy...

It also helps cleaning up cftype in general.  I'm not sure what you're
objecting to here.

The cleanup is removing 2 callbacks with a cost of moving non-memcg
specific code inside memcg. That is what I am objecting to.
 

quoted

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. 

There are pros and cons for both approaches and it should be discussed
in a separate thread with a code to back all the claims.

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.

quoted

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.

quoted

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

Was the above sentence necessary?

quoted

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?

I will not repeat myself. We seem to disagree on where the code belongs.
As I've said I will not ack this code, try to find somebody else who
think it is a good idea. I do not see any added value.
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>