Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

cgroups: Add support to dump fake attach events #135

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

cgroups: Add support to dump fake attach events #135

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions devlib/bin/scripts/shutils.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,15 @@ cgroups_tasks_in() {
exit 0
}

cgroup_trace_attach_task() {
DST_ROOT=${1}
DST_PATH=${2}
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

done
}

################################################################################
# Main Function Dispatcher
Expand All @@ -198,6 +207,9 @@ cpufreq_get_all_governors)
cpufreq_trace_all_frequencies)
cpufreq_trace_all_frequencies $*
;;
cgroup_trace_attach_task)
cgroup_trace_attach_task $*
;;
cpuidle_wake_all_cpus)
cpuidle_wake_all_cpus $*
;;
Expand Down
5 changes: 5 additions & 0 deletions devlib/module/cgroups.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

exec_cmd = "cgroup_trace_attach_task {} {} {}".format(self.controller.hid, self.directory, self.tasks_file)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can save some columns by splitting before .format

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

self.target._execute_util(exec_cmd)

def add_task(self, tid):
self.target.write_value(self.tasks_file, tid, verify=False)

Expand Down