cgroup: make cgroup_event specific to memcg

kernel/cgroup.c +1 -2

+struct cgroup *__cgroup_from_dentry(struct dentry *dentry, struct cftype **cftp)
 {
-	if (file_inode(file)->i_fop != &cgroup_file_operations)
-		return ERR_PTR(-EINVAL);

static int cgroup_create_file(struct dentry *dentry, umode_t mode,

kernel/cgroup.c +1 -1

 				struct super_block *sb)

@@ -3953,7 +3956,7 @@ static int cgroup_write_notify_on_release(struct cgroup_subsys_state *css,
  *
  * That's why we hold a reference before dput() and drop it right after.
  */
-static void cgroup_dput(struct cgroup *cgrp)
+void __cgroup_dput(struct cgroup *cgrp)
 {
 	struct super_block *sb = cgrp->root->sb;
 

Quoted 9 lines

kernel/cgroup.c +5 -3

+struct cgroup *__cgroup_from_dentry(struct dentry *dentry, struct cftype **cftp)
 {
-	if (file_inode(file)->i_fop != &cgroup_file_operations)
-		return ERR_PTR(-EINVAL);
-	return __d_cft(file->f_dentry);
+	if (!dentry->d_inode ||
+	    dentry->d_inode->i_op != &cgroup_file_inode_operations)
+		return NULL;
+

kernel/cgroup.c static struct dentry *cgroup_lookup(struct inode *dir, struct dentry *dentry, un · line 2675

Quoted 5 lines

kernel/cgroup.c +4 -0

+	if (cftp)
+		*cftp = __d_cft(dentry);
+	return __d_cgrp(dentry->d_parent);
 }
+EXPORT_SYMBOL_GPL(__cgroup_from_dentry);

As we don't expect new users, why export this symbol? memcg can't be built as a module.

cgroup_event is way over-designed and tries to build a generic flexible event mechanism into cgroup - fully customizable event specification for each user of the interface. This is utterly unnecessary and overboard especially in the light of the planned unified hierarchy as there's gonna be single agent. Simply generating events at fixed points, or if that's too restrictive, configureable cadence or single set of configureable points should be enough.

Thankfully, memcg is the only user and gets to keep it. Replacing it with something simpler on sane_behavior is strongly recommended.

This patch moves cgroup_event and "cgroup.event_control" implementation to mm/memcontrol.c. Clearing of events on cgroup destruction is moved from cgroup_destroy_locked() to mem_cgroup_css_offline(), which shouldn't make any noticeable difference.

Note that "cgroup.event_control" will now exist only on the hierarchy with memcg attached to it. While this change is visible to userland, it is unlikely to be noticeable as the file has never been meaningful outside memcg.

Signed-off-by Tejun Heo <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Michal Hocko <[email protected]>

Cc Balbir Singh <[email protected]>

---
 kernel/cgroup.c | 237 -------------------------------------------------------
 mm/memcontrol.c | 238 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 238 insertions(+), 237 deletions(-)

kernel/cgroup.c +0 -0

 #include <linux/flex_array.h> /* used in cgroup_attach_task */
 #include <linux/kthread.h>

@@ -154,36 +153,6 @@ struct css_id {
 	unsigned short stack[0]; /* Array of Length (depth+1) */
 };
 

Quoted 7 lines

static struct cftype mem_cgroup_files[] = {

mm/memcontrol.c +0 -0

 	{
 		.name = "usage_in_bytes",

@@ -5973,6 +6192,12 @@ static struct cftype mem_cgroup_files[] = {
 		.read_u64 = mem_cgroup_hierarchy_read,
 	},
 	{

mm/memcontrol.c static struct cftype mem_cgroup_files[] = { · line 6199

Quoted 5 lines

mm/memcontrol.c +5 -0

+		.name = "cgroup.event_control",
+		.write_string = cgroup_write_event_control,
+		.flags = CFTYPE_NO_PREFIX,
+		.mode = S_IWUGO,
+	},

One of the misdesign of cgroup eventfd is, cgroup.event_control is totally redunant...

cgroup_event is way over-designed and tries to build a generic flexible event mechanism into cgroup - fully customizable event specification for each user of the interface. This is utterly unnecessary and overboard especially in the light of the planned unified hierarchy as there's gonna be single agent. Simply generating

events at fixed points, or if that's too restrictive, configureable cadence or single set of configureable points should be enough.

Thankfully, memcg is the only user and gets to keep it. Replacing it with something simpler on sane_behavior is strongly recommended.

This patch moves cgroup_event and "cgroup.event_control" implementation to mm/memcontrol.c. Clearing of events on cgroup destruction is moved from cgroup_destroy_locked() to mem_cgroup_css_offline(), which shouldn't make any noticeable difference.

Note that "cgroup.event_control" will now exist only on the hierarchy with memcg attached to it. While this change is visible to userland, it is unlikely to be noticeable as the file has never been meaningful outside memcg.

Signed-off-by Tejun Heo <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Michal Hocko <[email protected]>

Cc Balbir Singh <[email protected]>

---
 kernel/cgroup.c | 237 -------------------------------------------------------
 mm/memcontrol.c | 238 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 238 insertions(+), 237 deletions(-)

mm/memcontrol.c +110 -0

+/*
+ * cgroup_event represents events which userspace want to receive.
+ */
+struct cgroup_event {
+       /*
+        * css which the event belongs to.
+        */
+       struct cgroup_subsys_state *css;
+       /*
+        * Control file which the event associated.
+        */
+       struct cftype *cft;
+       /*
+        * eventfd to signal userspace about the event.
+        */
+       struct eventfd_ctx *eventfd;
+       /*
+        * Each of these stored in a list by the cgroup.
+        */
+       struct list_head list;
+       /*
+        * All fields below needed to unregister event when
+        * userspace closes eventfd.
+        */
+       poll_table pt;
+       wait_queue_head_t *wqh;
+       wait_queue_t wait;
+       struct work_struct remove;
+};
+
 static void mem_cgroup_threshold(struct mem_cgroup *memcg);
 static void mem_cgroup_oom_notify(struct mem_cgroup *memcg);

@@ -5926,6 +5956,194 @@ static void kmem_cgroup_css_offline(struct mem_cgroup *memcg)
 }
 #endif
 
+/*
+ * Unregister event and free resources.
+ *
+ * Gets called from workqueue.
+ */
+static void cgroup_event_remove(struct work_struct *work)
+{
+       struct cgroup_event *event = container_of(work, struct cgroup_event,
+                       remove);
+       struct cgroup_subsys_state *css = event->css;
+       struct cgroup *cgrp = css->cgroup;
+
+       remove_wait_queue(event->wqh, &event->wait);
+
+       event->cft->unregister_event(css, event->cft, event->eventfd);
+
+       /* Notify userspace the event is going away. */
+       eventfd_signal(event->eventfd, 1);
+
+       eventfd_ctx_put(event->eventfd);
+       kfree(event);
+       __cgroup_dput(cgrp);
+}
+
+/*
+ * Gets called on POLLHUP on eventfd when user closes it.
+ *
+ * Called with wqh->lock held and interrupts disabled.
+ */
+static int cgroup_event_wake(wait_queue_t *wait, unsigned mode,
+               int sync, void *key)
+{
+       struct cgroup_event *event = container_of(wait,
+                       struct cgroup_event, wait);
+       struct cgroup *cgrp = event->css->cgroup;
+       unsigned long flags = (unsigned long)key;
+
+       if (flags & POLLHUP) {
+               /*
+                * If the event has been detached at cgroup removal, we
+                * can simply return knowing the other side will cleanup
+                * for us.
+                *
+                * We can't race against event freeing since the other
+                * side will require wqh->lock via remove_wait_queue(),
+                * which we hold.
+                */
+               spin_lock(&cgrp->event_list_lock);
+               if (!list_empty(&event->list)) {
+                       list_del_init(&event->list);
+                       /*
+                        * We are in atomic context, but cgroup_event_remove()
+                        * may sleep, so we have to call it in workqueue.
+                        */
+                       schedule_work(&event->remove);
+               }
+               spin_unlock(&cgrp->event_list_lock);
+       }
+
+       return 0;
+}
+
+static void cgroup_event_ptable_queue_proc(struct file *file,
+               wait_queue_head_t *wqh, poll_table *pt)
+{
+       struct cgroup_event *event = container_of(pt,
+                       struct cgroup_event, pt);
+
+       event->wqh = wqh;
+       add_wait_queue(wqh, &event->wait);
+}
+
+/*
+ * Parse input and register new memcg event handler.
+ *
+ * Input must be in format '<event_fd> <control_fd> <args>'.
+ * Interpretation of args is defined by control file implementation.
+ */
+static int cgroup_write_event_control(struct cgroup_subsys_state *css,
+                                     struct cftype *cft, const char *buffer)

mm/memcontrol.c +90 -0

+       buffer = endp + 1;
+
+       event = kzalloc(sizeof(*event), GFP_KERNEL);
+       if (!event)
+               return -ENOMEM;
+       event->css = css;
+       INIT_LIST_HEAD(&event->list);
+       init_poll_funcptr(&event->pt, cgroup_event_ptable_queue_proc);
+       init_waitqueue_func_entry(&event->wait, cgroup_event_wake);
+       INIT_WORK(&event->remove, cgroup_event_remove);
+
+       efile = eventfd_fget(efd);
+       if (IS_ERR(efile)) {
+               ret = PTR_ERR(efile);
+               goto out_kfree;
+       }
+
+       event->eventfd = eventfd_ctx_fileget(efile);
+       if (IS_ERR(event->eventfd)) {
+               ret = PTR_ERR(event->eventfd);
+               goto out_put_efile;
+       }
+
+       cfile = fget(cfd);
+       if (!cfile) {
+               ret = -EBADF;
+               goto out_put_eventfd;
+       }
+
+       /* the process need read permission on control file */
+       /* AV: shouldn't we check that it's been opened for read instead? */
+       ret = inode_permission(file_inode(cfile), MAY_READ);
+       if (ret < 0)
+               goto out_put_cfile;
+
+       cgrp_cfile = __cgroup_from_dentry(cfile->f_dentry, &event->cft);
+       if (!cgrp_cfile) {
+               ret = -EINVAL;
+               goto out_put_cfile;
+       }
+
+       /*
+        * The file to be monitored must be in the same cgroup as
+        * cgroup.event_control is.
+        */
+       if (cgrp_cfile != cgrp) {
+               ret = -EINVAL;
+               goto out_put_cfile;
+       }
+
+       if (!event->cft->register_event || !event->cft->unregister_event) {
+               ret = -EINVAL;
+               goto out_put_cfile;
+       }
+
+       ret = event->cft->register_event(css, event->cft,
+                       event->eventfd, buffer);
+       if (ret)
+               goto out_put_cfile;
+
+       efile->f_op->poll(efile, &event->pt);
+
+       /*
+        * Events should be removed after rmdir of cgroup directory, but before
+        * destroying subsystem state objects. Let's take reference to cgroup
+        * directory dentry to do that.
+        */
+       dget(cgrp->dentry);
+
+       spin_lock(&cgrp->event_list_lock);
+       list_add(&event->list, &cgrp->event_list);
+       spin_unlock(&cgrp->event_list_lock);
+
+       fput(cfile);
+       fput(efile);
+
+       return 0;
+
+out_put_cfile:
+       fput(cfile);
+out_put_eventfd:
+       eventfd_ctx_put(event->eventfd);
+out_put_efile:
+       fput(efile);
+out_kfree:
+       kfree(event);
+
+       return ret;
+}
+
 static struct cftype mem_cgroup_files[] = {
        {
                .name = "usage_in_bytes",

mm/memcontrol.c +13 -1

+       struct cgroup_event *event, *tmp;
+
+       /*
+        * Unregister events and notify userspace.
+        * Notify userspace about cgroup removing only after rmdir of cgroup
+        * directory to avoid race between userspace and kernelspace.
+        */
+       spin_lock(&cgrp->event_list_lock);
+       list_for_each_entry_safe(event, tmp, &cgrp->event_list, list) {
+               list_del_init(&event->list);
+               schedule_work(&event->remove);
+       }
+       spin_unlock(&cgrp->event_list_lock);
 
        kmem_cgroup_css_offline(memcg);
 
--

1.8.3.1

[off-topic] Has the unified hierarchy been agreed upon? I did not follow that thread

Quoted 2 lines

events at fixed points, or if that's too restrictive, configureable cadence or single set of configureable points should be enough.

Nit-pick: typo on the spelling of configurable

Tejun, I think the framework was designed to be flexible. Do you see cgroup subsystems never using this?

Quoted 6 lines

mm/memcontrol.c +6 -0

+static int cgroup_write_event_control(struct cgroup_subsys_state *css,
+                                     struct cftype *cft, const char *buffer)
+{
+       struct cgroup *cgrp = css->cgroup;
+       struct cgroup_event *event;
+       struct cgroup *cgrp_cfile;

mm/memcontrol.c static void kmem_cgroup_css_offline(struct mem_cgroup *memcg) · line 6048

Quoted 6 lines

mm/memcontrol.c +6 -0

+       unsigned int efd, cfd;
+       struct file *efile;
+       struct file *cfile;
+       char *endp;
+       int ret;
+

Can we assert that buffer is NOT NULL here?

Hello, Balbir.

Quoted 2 lines · Balbir Singh

[off-topic] Has the unified hierarchy been agreed upon? I did not follow that thread

I consider it agreed upon enough. There of course are objections but I feel fairly comfortable with the amount of existing consensus and considering the current state of cgroup in general, especially the API leaks, I don't think we have many other choices. The devil is always in the details but unless we meet a major technical barrier, I'm pretty sure it's happening.

Quoted 4 lines

Quoted 2 lines

events at fixed points, or if that's too restrictive, configureable cadence or single set of configureable points should be enough.

Nit-pick: typo on the spelling of configurable

Will update.

Quoted 2 lines

Tejun, I think the framework was designed to be flexible. Do you see cgroup subsystems never using this?

I can't be a hundred percent sure that we won't need events which are configureable per-listener but it's highly unlikely given that we're moving onto single agent model and the nature of event delivery - spurious events are unlikely to be noticeable unless the frequency is very high. In general, as anything, aiming for extremes isn't a healthy design practice. Maximum flexibility sounds good in isolation but nothing is free and it entails unneeded complexity both in implementation and usage. Note that even for memcg, both oom and vmpressure don't benefit in any way from all the added complexity at all. The only other place that I can see event being useful at the moment is freezer state change notification and that also would only require unconditional file modified notification.

Quoted 6 lines

Quoted 6 lines

mm/memcontrol.c +6 -0

+static int cgroup_write_event_control(struct cgroup_subsys_state *css,
+                                     struct cftype *cft, const char *buffer)
+{
+       struct cgroup *cgrp = css->cgroup;
+       struct cgroup_event *event;
+       struct cgroup *cgrp_cfile;

Quoted 8 lines

mm/memcontrol.c static void kmem_cgroup_css_offline(struct mem_cgroup *memcg) · line 6048

Quoted 6 lines

mm/memcontrol.c +6 -0

+       unsigned int efd, cfd;
+       struct file *efile;
+       struct file *cfile;
+       char *endp;
+       int ret;
+

Can we assert that buffer is NOT NULL here?

The patch moves the code as-is as things become difficult to review otherwise. After the patchset, it belongs to memcg, please feel free to modify as memcg people see fit.

cgroup_event is only available in memcg now. Let's brand it that way. While at it, add a comment encouraging deprecation of the feature and remove the respective section from cgroup documentation.

This patch is cosmetic.

Signed-off-by Tejun Heo <[email protected]>

---
 Documentation/cgroups/cgroups.txt | 19 -------------
 mm/memcontrol.c                   | 57 +++++++++++++++++++++++++--------------
 2 files changed, 37 insertions(+), 39 deletions(-)

Documentation/cgroups/cgroups.txt +0 -0

index 638bf17..ca5aee9 100644

Documentation/cgroups/cgroups.txt +0 -10

-2.4 Notification API
---------------------
-
-There is mechanism which allows to get notifications about changing
-status of a cgroup.
-
-To register a new notification handler you need to:
- - create a file descriptor for event notification using eventfd(2);
- - open a control file to be monitored (e.g. memory.usage_in_bytes);
- - write "<event_fd> <control_fd> <args>" to cgroup.event_control.

Hello,

Like many other things in cgroup, cgroup_event is way too flexible and complex - it strives to provide completely flexible event monitoring facility in cgroup proper which allows any number of users to monitor custom events. This is overboard, to say the least,

and I strongly think that cgroup should not any new usages of this facility and preferably deprecate the existing usages if at all possible.

Could you be more specific about what is so "overboard" about this interface? I am not familiar with internals much, so I cannot judge the complexity part, but I thought that eventfd was intended for this kind of kernel->userspace notifications.

So you think that vmpressure, oom notification or thresholds are an abuse of this interface? What would you consider a reasonable replacement for those notifications? Or do you think that controller shouldn't be signaling any conditions to the userspace at all?

Hello, Michal.

Quoted 4 lines · Michal Hocko

Could you be more specific about what is so "overboard" about this interface? I am not familiar with internals much, so I cannot judge the complexity part, but I thought that eventfd was intended for this kind of kernel->userspace notifications.

It's just way over-engineered like many other things in cgroup, most likely misguided by the appearance that cgroup could be delegated and accessed by multiple actors concurrently.

The most clear example would be the vmpressure event. When it could have just called fsnotify_modify() unconditionally when the state changes, now it involves parsing, dynamic list of events and so on without actually adding any benefits.

For the usage ones, configurability makes some sense but even then just giving it a single array of event points of limited size would be sufficient.

It's just way over-done.

Quoted 4 lines

So you think that vmpressure, oom notification or thresholds are an abuse of this interface? What would you consider a reasonable replacement for those notifications? Or do you think that controller shouldn't be signaling any conditions to the userspace at all?

I don't think the ability to generate events are an abuse, just that the facility itself is way over-engineered. Just generate a file changed event unconditionally for vmpressure and oom and maybe implement configureable cadence or single set of threshold array for threshold events. These are things which can and should be done in a a few tens of lines of code with far simpler interface.

I keep hearing that over and over. And I also keep hearing that there are users who do not like many simplifications because they are breaking their usecases. Users are those who matter to me. Hey some of them are even sane...

Besides that, is fsnotify really an interface to be used under memory pressure? I might be wrong but from a quick look fsnotify depends on GFP_KERNEL allocation which would be no-go for oom_control at least.

How does the reclaim context gets to struct file to notify? I am pretty sure we would get to more and more questions when digging further.

I am all for simplifications, but removing interfaces just because you feel they are "over-done" is not a way to go IMHO. In this particular

case you are removing an interface from cgroup core which has users, and will have to support them for very long time. "It is just memcg so move it there" is not a way that different subsystems should work together and I am _not_ going to ack such a move. All the flexibility that you are so complaining about is hidden from the cgroup core in register callbacks and the rest is only the core infrastructure (registration and unregistration).

And btw. a common notification interface at least makes things consistent and prevents controllers to invent their one purpose solutions.

So I am really skeptical about this patch set. It doesn't give anything. It just moves a code which you do not like out of your sight hoping that something will change.

There were mistakes done in the past. And some interfaces are really too flexible but that doesn't mean we should be militant about everything.

Quoted 3 lines

For the usage ones, configurability makes some sense but even then just giving it a single array of event points of limited size would be sufficient.

This would be a question for users. I am not one of those so I cannot tell you but I certainly cannot claim that something more coarse would be sufficient either.

Quoted 7 lines

It's just way over-done.

Quoted 4 lines

So you think that vmpressure, oom notification or thresholds are an abuse of this interface? What would you consider a reasonable replacement for those notifications? Or do you think that controller shouldn't be signaling any conditions to the userspace at all?

Quoted 6 lines

I don't think the ability to generate events are an abuse, just that the facility itself is way over-engineered. Just generate a file changed event unconditionally for vmpressure and oom and maybe implement configureable cadence or single set of threshold array for threshold events. These are things which can and should be done in a a few tens of lines of code with far simpler interface.

These are strong words without any justification.

Hey, Michal.

On Mon, Aug 05, 2013 at 09:16:41PM +0200, Michal Hocko wrote:

Quoted 3 lines

Besides that, is fsnotify really an interface to be used under memory pressure? I might be wrong but from a quick look fsnotify depends on GFP_KERNEL allocation which would be no-go for oom_control at least.

Yeap, I'm pretty sure you're wrong. I didn't follow the development but this is wrapper to combine inotify and dnotify and kernel's main FS event mechanism and none of the two mechanisms required memory allocation during delivery.

Quoted 2 lines

How does the reclaim context gets to struct file to notify? I am pretty sure we would get to more and more questions when digging further.

The file is optional and probably just to extract path for dnotify, I think. Not sure about namespace implications but I'll build cgroup level interface for it so that controllers won't have to deal with it directly. ie. there'll be css_notify_file(css, cfe) interface.

Quoted 2 lines

I am all for simplifications, but removing interfaces just because you feel they are "over-done" is not a way to go IMHO. In this particular

We can keep it around but it's a pretty good time to gradually move towards something saner as we're essentially doing v2 of the interface. Note that there's no reason for the interface to disappear immediately. You can keep it and there won't be any problem for it to co-exist with simpler mechanism.

Quoted 7 lines

case you are removing an interface from cgroup core which has users, and will have to support them for very long time. "It is just memcg so move it there" is not a way that different subsystems should work together and I am _not_ going to ack such a move. All the flexibility that you are so complaining about is hidden from the cgroup core in register callbacks and the rest is only the core infrastructure (registration and unregistration).

memcg is the only user and will stay that way. If you wanna keep it around, be my guest.

Also, cftype is planned to be simplified so that
it just provides seq_file interface and the types become helper routines like a normal file callback interface. There'll be nothing tying the event file handling to cgroup core and it surely won't be used by anyone else. memcg is and will be the only user. It's the natural place for the code.

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.

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.

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

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

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

Hello, Michal.

Quoted 3 lines · Michal Hocko

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.

Quoted 6 lines

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.

Quoted 1 line

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 2 lines

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

Was the above sentence necessary?

Quoted 2 lines

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?

How is it specific to memcg? The fact only memcg uses the interface doesn't imply it is 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...

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

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.

Hello, Michal.

Quoted 2 lines · Michal Hocko

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

I don't follow. It's only for memcg. That is *by definition* memcg specific. It's the verbatim meaning of the word.

Now, I do
understand that it can be a concern the implementation details as-is could be a bit too invasive into cgroup core to be moved to memcg, but that's something we can work on, right?

Can you at least agree that the feature is nmemcg specific and it'd be better to be located in memcg if possible? That really isn't not much to ask and is a logical thing to do.

Quoted 3 lines

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

Yes, but those are all inferior to actually moving the code where it belongs. Those makes the code harder to follow and people misunderstand and waste time working on stuff (either in the core or controllers) which eventually end up getting nacked. Why do that when we can easily do better? What's the rationale behind that?

Quoted 2 lines

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

I don't really get your "non-memcg" specific code assertion when it is by definition memcg-specific. What are you talking about?

Quoted 3 lines

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.

Nacking is part of your authority as maintainer but you should still provide plausible rationale for that.

Are you saying that even if the
code is restructured so that it's not invasive into cgroup core, you are still gonna disagree with it because it's still somehow not memcg-specifc?

Please don't repeat yourself but do explain your rationale. That's part of your duty as a maintainer too.

I would rather see it not changed unless it really is a big win in the cgroup core. So far I do not see anything like that (just look at __cgroup_from_dentry which needs to be exported to allow for the move).

You reduce the amount of code in cgroup.c, alright, but the code doesn't go away really. It just moves out of your sight and moves the same burden on somebody else without providing a new generic interface.

If somebody needs a notification interface (and there is no one available right now) then you cannot prevent from such a pointless work anyway...

cgroup_event_* don't sound memcg specific at all. They are playing with cgroup dentry reference counting and do a generic functionality which memcg doesn't need to know about.

I wouldn't object to having non-cgroup internals playing variant. I just do not think it makes sense to invest time to something that should go away long term.

Quoted 2 lines

If somebody needs a notification interface (and there is no one available right now) then you cannot prevent from such a pointless work anyway...

I'm gonna add one for freezer state transitions. It'll be simple "this file changed" thing and will probably apply that to at least oom and vmpressure. I'm relatively confident that it's gonna be pretty simple and that's gonna be the cgroup event mechanism.

I would like to see this happen. I have a feeling that we're deprecating features a bit aggressively without providing alternatives.