Re: [RFC PATCH 0/2] perf_events: add support for per-cpu per-cgroupmonitoring (v3)

[Stephane Eranian]

Peter,

On Tue, Sep 21, 2010 at 11:43 AM, Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> On Tue, 2010-09-21 at 11:38 +0200, Peter Zijlstra wrote:
>> On Thu, 2010-09-09 at 15:05 +0200, Stephane Eranian wrote:
>> > The cgroup to monitor is designated by passing a file descriptor opened
>> > on a new per-cgroup file in the cgroup filesystem (perf_event.perf). The
>> > option must be activated by setting perf_event_attr.cgroup=1 and passing
>> > a valid file descriptor in perf_event_attr.cgroup_fd. Those are the only
>> > two ABI extensions.
>>
>> > +++ b/include/linux/perf_event.h
>> > @@ -215,8 +215,9 @@ struct perf_event_attr {
>> > Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â*/
>> >                 precise_ip   : Â2, /* skid constraint    */
>> >                 mmap_data   Â: Â1, /* non-exec mmap data  Â*/
>> > +                cgroup     : Â1, /* cgroup aggregation  Â*/
>> >
>> > - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â __reserved_1 Â : 46;
>> > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â __reserved_1 Â : 45;
>> >
>> > Â Â Â Â union {
>> > Â Â Â Â Â Â Â Â __u32 Â Â Â Â Â wakeup_events; Â Â/* wakeup every n events */
>> > @@ -226,6 +227,8 @@ struct perf_event_attr {
>> > Â Â Â Â __u32 Â Â Â Â Â Â Â Â Â bp_type;
>> > Â Â Â Â __u64 Â Â Â Â Â Â Â Â Â bp_addr;
>> > Â Â Â Â __u64 Â Â Â Â Â Â Â Â Â bp_len;
>> > +
>> > +    int           cgroup_fd;
>> > Â};
>> >
>> > Â/*
>>
>> I'm not sure I like this much.. so we attach to {pid,cpu}, for nodes we
>> can use cpu_to_node(cpu), which would suggest to use
>> cgroup_of_task(pid), except that a task can be part of multiple cgroups,
>> so its not unique.
>>
>> One thing we could do is pass this cgroup identifier in the pid field
>> and use PERF_FLAG_CGROUP or something. Currently the syscall signature
>> uses pid_t, but I think we can safely change that to int.
>>
>> You create a special new file in the cgroup stuff, I'm not sure about
>> that either, but its not something I feel too strongly about, why
>> wouldn't a fd of any file or even directory of that cgroup work? Do the
>> cgroup people have an opinion?
>
> Ahh, I just read more of the patch, and you create a full perf cgroup,
> in which case cgroup_of_task(pid) will work, simply pick the perf
> cgroup's tasks.
>
> No need to actually create that file, open it and pass fds around, just
> pick a task from that cgroup and attach to the cgroup through that.
>
If I understand, you are proposing that we use the pid argument to the
syscall to designate the cgroup. A task belongs to only one cgroup at
a time. Thus with the pid you can identify a cgroup. No need for an
entry in cgroup_fs and therefore no need for cgroup_fd in perf_event_attr.
We would still need a flag somewhere to indicate that we don't want
per-thread mode but per-cpu per-cgroup. It could be a new field in the
bitfield in perf_event_attr. In fact, I already have such a field.

The main issue I see with this is that it relies on having at least one
task in the cgroup when you start the measurement. That is certainly
not always the case.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/