cgroups: Add support to dump fake attach events #135

joelagnel · 2017-06-20T23:03:44Z

With this its possible for users to dump the initial cgroup state into
the traces with something like the following:

cgroup = te.target.cgroups.controllers['cpuset'].cgroup('/foreground')
cgroup.trace_cgroup_tasks()

Signed-off-by: Joel Fernandes [email protected]

joelagnel · 2017-06-20T23:03:53Z

CC @derkling @bjackman

bjackman · 2017-06-21T14:14:31Z

The shutil and trace_cgroup_tasks look good to me.

I don't understand why create_existing_cgroups is added? I might be being slow here...

Also could you add a docstring explaining what trace_existing_cgroups does?

joelagnel · 2017-06-21T16:01:26Z

I think it would work without this too. But it made sense to me to pre create the cgroup. Now that you mention this I am thinking I'll drop it and just build it later.

With this its possible for users to dump the initial cgroup state into the traces with something like the following: cgroup = te.target.cgroups.controllers['cpuset'].cgroup('/foreground') cgroup.trace_cgroup_tasks() Change-Id: I1f3741ff4fdc052651a48b44a0489f17c217961c Signed-off-by: Joel Fernandes <[email protected]>

joelagnel · 2017-06-27T03:58:21Z

@derkling please merge this?

derkling

We need to make this API a bit more generic, since the beginning... some comments inline.

derkling · 2017-06-27T08:31:57Z

devlib/bin/scripts/shutils.in

+        TASKS_FILE=${3}
+
+        cat $TASKS_FILE | while read PID; do
+            echo "cgroup_attach_task_devlib: dst_root=$DST_ROOT dst_path=$DST_PATH pid=$PID" > /sys/kernel/debug/tracing/trace_marker


Maybe this format is less aggressive on the 80 cols limit?

EVENT= "cgroup_attach_task_devlib: dst_root=$DST_ROOT dst_path=$DST_PATH pid=$PID" cat $TASKS_FILE | while read PID; do echo $EVENT >/sys/kernel/debug/tracing/trace_marker done

Moreover, this is not the same format of the original (i.e. kernel generated) event... but I guess we cannot do otherwise since we don't know the source groups.

Yes I believe, only the extra fields are dropped but the existing fields have the same format, so that's not an issue

Agreed with comment on 80 line limit and will fix, thanks

derkling · 2017-06-27T08:34:23Z

devlib/module/cgroups.py

@@ -287,6 +287,11 @@ def get_tasks(self):
        logging.debug('Tasks: %s', task_ids)
        return map(int, task_ids)

+    # Used to insert fake cgroup attach events to know existing cgroup assignments
+    def trace_cgroup_tasks(self):


We are already into the CGroup class, thous I would suggest to rename this to be just CGroup::trace_tasks

Moreover, I think we should also add a method to the CgroupsModule class, which allows to trace the cgroups for all the CGroups of all the currently mounted controllers.
I think this will be a quite common use case and we don't want to have this code in the clients of the CGroups API. Possibly, this new call should also allow to specify a "mask" of cgroups names to report. Something like:

class CgroupModule(Module): def trace_cgroups(self, controllers=None, cgroups=None): if controllers is None: controllers = [ss.name for ss in self.controllers] for controller in self.controllers: if controller not in controllers: continue controller.trace_cgroups(cgroups)

Which ultimately requires an additional method in the Contoller class doing almost the same but filtering on cgroups names to call your method above just on listed cgroups (or all if None).

If you don't have time, I can provide such a patch in the next few days...

Honestly I think this can be a wrapper on top of my patch. I do believe my patch is clean enough but all that's missing is a wrapper to take controller and group parameters. IMO the core dumping into trace logic should be as low as possible (infact right fully in the CGroup class since we're dumping tasks per CGroup). Also I tried implementing it the other way - make the CGroup controller or CgroupModule do the dumping into trace and it complicates things because self.tasks_file is available in CGroup class and can just be directly accessed and passed along to the shutil. So I still like my approach to this is clean.

If you still want to do this differently, I am Ok with that. But I would like it to be as simple as my approach. Maybe you can just add a wrapper to this patch in CgroupModule if your concern is the interface?

My concern is that, since we are adding a new API, is should be properly implemented to be fully usable... while this one is a "partial implementation".

I guess your code which uses this API is already doing most of the things I'm asking for here, at least looping on all cgroups of a controller. Since this is a quite common use case, I would like we merge the API once we provide these minimal set of functionalities covered.

To summarise, what you did is simple, clean and ok... but a bit limited in scope to require additional client code. Let's try to factor in this client code in the new API.

If you do not have time to provide such an extension it's ok for me to add some patches on top of this PR before merging it.

Yes, I think if you can add some PRs on top of this with your idea, that will be the quickest. I think the high level implementation can be simple wrapper to this lower level functionality. Thanks @derkling

derkling · 2017-06-27T08:46:34Z

devlib/module/cgroups.py

@@ -287,6 +287,11 @@ def get_tasks(self):
        logging.debug('Tasks: %s', task_ids)
        return map(int, task_ids)

+    # Used to insert fake cgroup attach events to know existing cgroup assignments
+    def trace_cgroup_tasks(self):
+        exec_cmd = "cgroup_trace_attach_task {} {} {}".format(self.controller.hid, self.directory, self.tasks_file)


you can save some columns by splitting before .format

derkling · 2017-07-13T15:42:03Z

Hi @joelagnel, you can find here:
derkling:review-joel-for-cgroup-attach
a series with few patches I've added on top of the one of this PR.

Modifications:

fixup! cgroups: Add support to dump fake attach events
This should be squased into your own... it brings few updates on your original patch
cgroups: add high level API for controller/cgroups tracing
Introduces the high-level API I've suggested in one of my previous comments
Revert "ssh: Combine exit code and main command"
This reverts an optimisation by @bjackman , I would not add it to your PR, since it's not affecting
Android targets... but I needed it to test on my Juno/Linux target.

Please give a try to these new bits... especially the high-level API.

setrofim · 2017-09-11T12:33:09Z

@derkling @joelagnel @bjackman what is the state of this PR? Is this still needed? Are there more changes coming?

joelagnel · 2017-09-12T23:34:58Z

These are needed. I will squash Patrick's fixup patch into mine and submit it, sorry about the delay. This PR was already in our tree and is being used for some time without the need for the high level API.

…

On Sep 11, 2017 8:33 AM, "setrofim" ***@***.***> wrote: @derkling <https://github.com/derkling> @joelagnel <https://github.com/joelagnel> @bjackman <https://github.com/bjackman> what is the state of this PR? Is this still needed? Are there more changes coming? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#135 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACSVAJPe401Xk6RVPdc4Ipi4b43Uzb6ks5shSiGgaJpZM4OAQq1> .

derkling suggested changes Jun 27, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cgroups: Add support to dump fake attach events #135

cgroups: Add support to dump fake attach events #135

joelagnel commented Jun 20, 2017 •

edited

Loading

joelagnel commented Jun 20, 2017

bjackman commented Jun 21, 2017

joelagnel commented Jun 21, 2017

joelagnel commented Jun 27, 2017

derkling left a comment

derkling Jun 27, 2017

joelagnel Jun 27, 2017

joelagnel Jun 27, 2017

derkling Jun 27, 2017

derkling Jun 27, 2017

joelagnel Jun 27, 2017

derkling Jun 28, 2017

joelagnel Jun 28, 2017

derkling Jun 27, 2017

joelagnel Jun 27, 2017

derkling commented Jul 13, 2017

setrofim commented Sep 11, 2017

joelagnel commented Sep 12, 2017 via email

cgroups: Add support to dump fake attach events #135

Are you sure you want to change the base?

cgroups: Add support to dump fake attach events #135

Conversation

joelagnel commented Jun 20, 2017 • edited Loading

joelagnel commented Jun 20, 2017

bjackman commented Jun 21, 2017

joelagnel commented Jun 21, 2017

joelagnel commented Jun 27, 2017

derkling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derkling commented Jul 13, 2017

setrofim commented Sep 11, 2017

joelagnel commented Sep 12, 2017 via email

joelagnel commented Jun 20, 2017 •

edited

Loading