Seems like a nice feature. Could you please provide a git-formatted patch?

I gave it a cursory look and noticed a couple things. 

A few days ago I went through and made the summaries for the columns in the GSettings schema xml file match the column names exactly. This includes capitalization. Please update the patch to be consistent.

Another issue I find is the capitalization of cgroup. Generally it's all lowercase but that would not really fit here. You've chosen 'Cgroup' while I'd say 'CGroup' seems more appropriate. I'd prefer 'Control Group' but feel this requires too much space. So, let's use 'CGroup' unless there's a good reason against it.

Once the git-formatted patch arrives I'll look at it more closely.

(In reply to comment #1)
> Another issue I find is the capitalization of cgroup. Generally it's all
> lowercase but that would not really fit here. You've chosen 'Cgroup' while I'd
> say 'CGroup' seems more appropriate. I'd prefer 'Control Group' but feel this
> requires too much space. So, let's use 'CGroup' unless there's a good reason
> against it.
> 

I take this back. I've found others using 'Cgroup' and none using 'CGroup'. So, please leave as 'Cgroup'.

I talked to Lennart today and he said he might have some ideas about how this could be better implemented. I'm CCing him so that he can chime in on the discussion.

Created attachment 201173 [details] [review]
Add support for cpu and memory cgroups in process view

Ok, I'm attaching an updated patch with:

-updates in GSettings schema xml file to match the column names exactly
-optimization to only call g_strdup() when the cgroup name has changed

thanks.

I'd strongly advise not to merge the patch like this. Arbitrarily picking two of the controllers here sounds like a bad idea. Also, in most setups the control group paths in a number of the controllers will be the same for most processes, hence showing the same data for every controller is not particularly useful. And showing information if the control group path is / is probably  not a good idea either and should be suppressed.

My original intention was to extend gnome-system-monitor to show higher-level information only, for example instead of showing which cgroup a process is in, show to which systemd service or systemd session a process belongs. That information is also gathered from the low-level cgroup tree, but distilled into a more high-level value.

That said, I understand that there's interest to show cgroup information independent of systemd's cgroup management. So, to accommodate for that I'd still recommend to coalesce the cgroup information for all trees into a terse string: i.e. suppress showing info for controllers where the process is in the root cgroup, and coalesce information for all controllers where the process is in the same path. Example: if a process is in cgroup /a in "cpu" and also in /a in "memory", but in "/b" for "blkio", and in / in all others (including "cpuset") then create a single "Control Group" field for that with a value like "/a (CPU/Memory), /b (Block IO)". In most cases this logic will have the effect that either the empty string is shown of a process (if it is in the root cgroup), or that a single field is shown, since the fewest processes will be in different cgroups in the various controllers. 

In summary:

a) Only a single column for all cgroup hierarchies, not one for each

b) cover all controllers, not just "cpu" and "memory"

c) suppress information if a process is in the root cgroup

d) coalesce cgroup information for multiple hierarchies if a cgroup is in the same path in them

e) translate the raw kernel controller name into a pretty human readable string, and fall back to the raw kernel controller name if no human readable string is known for the cases where the kernel adds a new controller, which isn't known yet by g-s-m

ok. Chris, if you like Lennart's suggestion, I can re-post it using the format he has suggested?

(In reply to comment #6)
> ok. Chris, if you like Lennart's suggestion, I can re-post it using the format
> he has suggested?

Yes, Lennart's *much* more knowledgeable about cgroups than I. So, if this seems clear to you then let's do as Lennart suggests. Also, since we'll have only one column, please use 'Control Group' in the column header.

Created attachment 201256 [details] [review]
Path to implement Lennart's single column for cgroups idea

Hi,

Ok, here's a patch to implement this idea. I need to do some more testing on it...but it seems to basically work.

Thanks,

-Jason

(In reply to comment #8)
> Created an attachment (id=201256) [details] [review]
> Path to implement Lennart's single column for cgroups idea
> 
> Hi,
> 
> Ok, here's a patch to implement this idea. I need to do some more testing on
> it...but it seems to basically work.
> 

I'm applying this so that we can get some feedback from tomorrow's development release. I had to make a small change to get it to apply and cleaned up 2 whitespace errors. Please watch those.

I'm considering this the first iteration and am not closing this till you've done your testing.

Cheers

great. Thanks.

I've got a newer version now, that deals with the case where a controllers are co-mounted, and also optimizes the case where the cgroup has not changed. Should I post a diff on top of the latest tree? Or a replacement patch?

Thanks,

-Jason

(In reply to comment #10)
> great. Thanks.
> 
<snip>
> Should I post a diff on top of the latest tree? Or a replacement patch?

A git-formatted patch based off master would be great.

Thanks.

The CGROUPS related functionality has been committed to the git repo, see http://git.gnome.org/browse/gnome-system-monitor/commit/?id=2d33adcbc4347c112d57082956b4e199ff7132db. Did the commit fix the problem? If yes, the bug should be set as resolved.

Robert, we are still working on this. Leave this open for now.

Created attachment 202020 [details] [review]
only update the cgroup path when its changed

Hi,

This patch only allocates a new string for the cgroup when its changed. It also recognizes more complex cgroup setups, such as co-mounted cgroups.

Thanks,

-Jason

Hi,

Any thoughts on this patch?

Also, I'm wondering if there is interest in further cgroup support. What I'm thinking is potentially a new cgroup tab. In that tab one could select which controller they are interested in - memory, cpu, etc., via a toggle box, perhaps at the top. Once the desired controller is selected, the relevant cgoups are displayed in a table form much like the process tab, where the first column in the cgroup name. The columns of the table would then have the relevant data for the particular cgroup. So, for instance, for the cpu controller, it would display cpu usage. Thus, one could sort by the cpu usage column and see which cgroup is using the most cpu resources.

Perhaps, others have better ideas?

Thanks,

-Jason

Review of attachment 202020 [details] [review]:

Looks fine.

Review of attachment 201256 [details] [review]:

Forgot to mark this as committed. Doing it now.

Review of attachment 201256 [details] [review]:

Forgot to mark this as committed. Doing it now.

(In reply to comment #15)
> Hi,
> 
> Any thoughts on this patch?
> 

Committed.

> Also, I'm wondering if there is interest in further cgroup support. What I'm
> thinking is potentially a new cgroup tab. In that tab one could select which
> controller they are interested in - memory, cpu, etc., via a toggle box,
> perhaps at the top. Once the desired controller is selected, the relevant
> cgoups are displayed in a table form much like the process tab, where the first
> column in the cgroup name. The columns of the table would then have the
> relevant data for the particular cgroup. So, for instance, for the cpu
> controller, it would display cpu usage. Thus, one could sort by the cpu usage
> column and see which cgroup is using the most cpu resources.

I'm generally very reluctant to add new tabs. Sounds like this might be better in a stand-alone app. But as I've said before, I'm not really fluent c-groups so perhaps I'm wrong.

Lennart, 

Any thoughts on gsm cgroup tab vs. separate gui? I've outlined a pretty basic interface in comment #15.

Thanks,

-Jason

Lennart still appears to be working through his email backlog following his vacation but I talked to him the other day and he was also not in favor of having a cgourps tab in g-s-m.

I'm closing this. Please file another bug if you feel you'd like to add anything else.

Thanks.

I am not convinced that it makes sense giving cgroups an entire tab of their own. I mean, cgroups should be exposed and all, but I have my doubts they should get their own tab. I think whatever we show in g-s-m needs to be distilled to the relevant essence, and I don't think showing the full cgroup hierarchies over all controllers would be helpful to achieve that. We shouldn't lose focus here, g-s-m is a general system monitor, and I don't think cgroups are too central for the system to deserve an entire tab of their own.

I think more detailed cgroup explorer tools might be something for a separate project.

That said, I think there might be a couple of higher-level cgroup-related things we might still want to expose in g-s-m. For example, a tab showing the login session with sd_pid_get_session() and the session user (i.e. the owner of a session, which is not influenced by sudo and stuff) with sd_pid_get_owner_uid().

http://0pointer.de/public/systemd-man/sd_pid_get_session.html

Oh, and it might even make sense showing seat information, too, via sd_session_get_seat(sd_pid_get_session()).

http://0pointer.de/public/systemd-man/sd_session_is_active.html

(those three calls are basically just parsed cgroup information, which is why i suggest this as higher-level cgroup-based info)

It is pretty hard to discuss this in a meaningful way without background
information on what extra information and knobs cgroups give us. Also, having a
screenshot of the proposed tab (as well as the new column in the process list)
might help getting an idea what we are talking about here...

I second Matthias' comment. I also think this discussion would be better in its own bug report.