From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932355AbZHUOro (ORCPT ); Fri, 21 Aug 2009 10:47:44 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S932342AbZHUOrn (ORCPT ); Fri, 21 Aug 2009 10:47:43 -0400 Received: from mail-bw0-f219.google.com ([209.85.218.219]:34413 "EHLO mail-bw0-f219.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932339AbZHUOrl (ORCPT ); Fri, 21 Aug 2009 10:47:41 -0400 DomainKey-Signature: a=rsa-sha1; c=nofws; d=googlemail.com; s=gamma; h=mime-version:reply-to:in-reply-to:references:date:message-id :subject:from:to:cc:content-type:content-transfer-encoding; b=TpS+uZcfB+EXrkgoBL2nYYGbfWOio//Xs0tYL1THDbFDFdpyE80hr8SpER73GxGnlK LEhGxOMiRgbfLaZf0BrhYRWs9q0G5iNhfPMJbEfXymw/twyTMmD8/SyzPGI7s7IhsSUF W+2SYDWzsojw5QJAAjrl9QCiqTC0wl5rLuMYs= MIME-Version: 1.0 Reply-To: eranian@gmail.com In-Reply-To: <20090813103655.117411814@chello.nl> References: <20090813094752.433445710@chello.nl> <20090813103655.117411814@chello.nl> Date: Fri, 21 Aug 2009 16:47:41 +0200 Message-ID: <7c86c4470908210747u125dc6d6y76a9ca7f1af41160@mail.gmail.com> Subject: Re: [PATCH 1/4 -v2] perf: rework the whole read vs group stuff From: stephane eranian To: Peter Zijlstra Cc: Ingo Molnar , Paul Mackerras , Corey J Ashford , LKML , perfmon2-devel Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by alpha.home.local id n7LEmNb8005713 Hi, Well, I tried that and it brings more complexity than neededespecially with regards toextracting the ID for events when you're doing grouping. To extract the ID, one has to read out a struct as follows: * { u64 nr; * { u64 time_enabled; } && PERF_FORMAT_ENABLED * { u64 time_running; } && PERF_FORMAT_RUNNING * { u64 value; * { u64 id; } && PERF_FORMAT_ID * } cntr[nr]; * } && PERF_FORMAT_GROUP Supposedly, you should have to do this only once per group. Readingthis stuff using the group leader should yield the values of all the otherevents. This is not what I have observed. All events report the sameID as the leader. Not clear why. As I suggested in a previous message, I don't think all of this is necessary.If a tool was able to pass the ID to associate with an event, then therewould be no need for a read(). Furthermore, it would make it easier to pickan ID which suites the tool's data structure. For instance, if you create4 events in a group, the ID could be 0,1,2,3 and would most likely map toan index in the array used by the tool to manage the perf_counter structures.That would also make it easier in the critical path in the signal handler. Noneed to have a lookup table to map "random" ID to ID more relevantfor the tool. The ID does not need to be very wide. IDs are relevant only ifone uses group sampling. Therefore the ID needs to identify an event withina group. Could use a reserved field in perf_counter_attr or add an ioctl() toassign an ID. On Thu, Aug 13, 2009 at 11:47 AM, Peter Zijlstra wrote:> Replace PERF_SAMPLE_GROUP with PERF_SAMPLE_READ and introduce> PERF_FORMAT_GROUP to deal with group reads in a more generic way.>> This allows you to get group reads out of read() as well.>> Signed-off-by: Peter Zijlstra > --->  include/linux/perf_counter.h |   47 +++++-->  kernel/perf_counter.c        |  274 +++++++++++++++++++++++++++++++------------>  2 files changed, 238 insertions(+), 83 deletions(-)>> Index: linux-2.6/include/linux/perf_counter.h> ===================================================================> --- linux-2.6.orig/include/linux/perf_counter.h> +++ linux-2.6/include/linux/perf_counter.h> @@ -115,7 +115,7 @@ enum perf_counter_sample_format {>        PERF_SAMPLE_TID                         = 1U << 1,>        PERF_SAMPLE_TIME                        = 1U << 2,>        PERF_SAMPLE_ADDR                        = 1U << 3,> -       PERF_SAMPLE_GROUP                       = 1U << 4,> +       PERF_SAMPLE_READ                        = 1U << 4,>        PERF_SAMPLE_CALLCHAIN                   = 1U << 5,>        PERF_SAMPLE_ID                          = 1U << 6,>        PERF_SAMPLE_CPU                         = 1U << 7,> @@ -127,16 +127,32 @@ enum perf_counter_sample_format {>  };>>  /*> - * Bits that can be set in attr.read_format to request that> - * reads on the counter should return the indicated quantities,> - * in increasing order of bit value, after the counter value.> + * The format of the data returned by read() on a perf counter fd,> + * as specified by attr.read_format:> + *> + * struct read_format {> + *     { u64           value;> + *       { u64         time_enabled; } && PERF_FORMAT_ENABLED> + *       { u64         time_running; } && PERF_FORMAT_RUNNING> + *       { u64         id;           } && PERF_FORMAT_ID> + *     } && !PERF_FORMAT_GROUP> + *> + *     { u64           nr;> + *       { u64         time_enabled; } && PERF_FORMAT_ENABLED> + *       { u64         time_running; } && PERF_FORMAT_RUNNING> + *       { u64         value;> + *         { u64       id;           } && PERF_FORMAT_ID> + *       }             cntr[nr];> + *     } && PERF_FORMAT_GROUP> + * };>  */>  enum perf_counter_read_format {>        PERF_FORMAT_TOTAL_TIME_ENABLED          = 1U << 0,>        PERF_FORMAT_TOTAL_TIME_RUNNING          = 1U << 1,>        PERF_FORMAT_ID                          = 1U << 2,> +       PERF_FORMAT_GROUP                       = 1U << 3,>> -       PERF_FORMAT_MAX = 1U << 3,              /* non-ABI */> +       PERF_FORMAT_MAX = 1U << 4,              /* non-ABI */>  };>>  #define PERF_ATTR_SIZE_VER0    64      /* sizeof first published struct */> @@ -343,10 +359,8 @@ enum perf_event_type {>         * struct {>         *      struct perf_event_header        header;>         *      u32                             pid, tid;> -        *      u64                             value;> -        *      { u64           time_enabled;   } && PERF_FORMAT_ENABLED> -        *      { u64           time_running;   } && PERF_FORMAT_RUNNING> -        *      { u64           parent_id;      } && PERF_FORMAT_ID> +        *> +        *      struct read_format              values;>         * };>         */>        PERF_EVENT_READ                 = 8,> @@ -364,11 +378,22 @@ enum perf_event_type {>         *      { u32                   cpu, res; } && PERF_SAMPLE_CPU>         *      { u64                   period;   } && PERF_SAMPLE_PERIOD>         *> -        *      { u64                   nr;> -        *        { u64 id, val; }      cnt[nr];  } && PERF_SAMPLE_GROUP> +        *      { struct read_format    values;   } && PERF_SAMPLE_READ>         *>         *      { u64                   nr,>         *        u64                   ips[nr];  } && PERF_SAMPLE_CALLCHAIN> +        *> +        *      #> +        *      # The RAW record below is opaque data wrt the ABI> +        *      #> +        *      # That is, the ABI doesn't make any promises wrt to> +        *      # the stability of its content, it may vary depending> +        *      # on event, hardware, kernel version and phase of> +        *      # the moon.> +        *      #> +        *      # In other words, PERF_SAMPLE_RAW contents are not an ABI.> +        *      #> +        *>         *      { u32                   size;>         *        char                  data[size];}&& PERF_SAMPLE_RAW>         * };> Index: linux-2.6/kernel/perf_counter.c> ===================================================================> --- linux-2.6.orig/kernel/perf_counter.c> +++ linux-2.6/kernel/perf_counter.c> @@ -1692,7 +1692,32 @@ static int perf_release(struct inode *in>        return 0;>  }>> -static u64 perf_counter_read_tree(struct perf_counter *counter)> +static int perf_counter_read_size(struct perf_counter *counter)> +{> +       int entry = sizeof(u64); /* value */> +       int size = 0;> +       int nr = 1;> +> +       if (counter->attr.read_format & PERF_FORMAT_TOTAL_TIME_ENABLED)> +               size += sizeof(u64);> +> +       if (counter->attr.read_format & PERF_FORMAT_TOTAL_TIME_RUNNING)> +               size += sizeof(u64);> +> +       if (counter->attr.read_format & PERF_FORMAT_ID)> +               entry += sizeof(u64);> +> +       if (counter->attr.read_format & PERF_FORMAT_GROUP) {> +               nr += counter->group_leader->nr_siblings;> +               size += sizeof(u64);> +       }> +> +       size += entry * nr;> +> +       return size;> +}> +> +static u64 perf_counter_read_value(struct perf_counter *counter)>  {>        struct perf_counter *child;>        u64 total = 0;> @@ -1704,14 +1729,96 @@ static u64 perf_counter_read_tree(struct>        return total;>  }>> +static int perf_counter_read_entry(struct perf_counter *counter,> +                                  u64 read_format, char __user *buf)> +{> +       int n = 0, count = 0;> +       u64 values[2];> +> +       values[n++] = perf_counter_read_value(counter);> +       if (read_format & PERF_FORMAT_ID)> +               values[n++] = primary_counter_id(counter);> +> +       count = n * sizeof(u64);> +> +       if (copy_to_user(buf, values, count))> +               return -EFAULT;> +> +       return count;> +}> +> +static int perf_counter_read_group(struct perf_counter *counter,> +                                  u64 read_format, char __user *buf)> +{> +       struct perf_counter *leader = counter->group_leader, *sub;> +       int n = 0, size = 0, err = -EFAULT;> +       u64 values[3];> +> +       values[n++] = 1 + leader->nr_siblings;> +       if (read_format & PERF_FORMAT_TOTAL_TIME_ENABLED) {> +               values[n++] = leader->total_time_enabled +> +                       atomic64_read(&leader->child_total_time_enabled);> +       }> +       if (read_format & PERF_FORMAT_TOTAL_TIME_RUNNING) {> +               values[n++] = leader->total_time_running +> +                       atomic64_read(&leader->child_total_time_running);> +       }> +> +       size = n * sizeof(u64);> +> +       if (copy_to_user(buf, values, size))> +               return -EFAULT;> +> +       err = perf_counter_read_entry(leader, read_format, buf + size);> +       if (err < 0)> +               return err;> +> +       size += err;> +> +       list_for_each_entry(sub, &leader->sibling_list, list_entry) {> +               err = perf_counter_read_entry(counter, read_format,> +                               buf + size);> +               if (err < 0)> +                       return err;> +> +               size += err;> +       }> +> +       return size;> +}> +> +static int perf_counter_read_one(struct perf_counter *counter,> +                                u64 read_format, char __user *buf)> +{> +       u64 values[4];> +       int n = 0;> +> +       values[n++] = perf_counter_read_value(counter);> +       if (read_format & PERF_FORMAT_TOTAL_TIME_ENABLED) {> +               values[n++] = counter->total_time_enabled +> +                       atomic64_read(&counter->child_total_time_enabled);> +       }> +       if (read_format & PERF_FORMAT_TOTAL_TIME_RUNNING) {> +               values[n++] = counter->total_time_running +> +                       atomic64_read(&counter->child_total_time_running);> +       }> +       if (read_format & PERF_FORMAT_ID)> +               values[n++] = primary_counter_id(counter);> +> +       if (copy_to_user(buf, values, n * sizeof(u64)))> +               return -EFAULT;> +> +       return n * sizeof(u64);> +}> +>  /*>  * Read the performance counter - simple non blocking version for now>  */>  static ssize_t>  perf_read_hw(struct perf_counter *counter, char __user *buf, size_t count)>  {> -       u64 values[4];> -       int n;> +       u64 read_format = counter->attr.read_format;> +       int ret;>>        /*>         * Return end-of-file for a read on a counter that is in> @@ -1721,28 +1828,18 @@ perf_read_hw(struct perf_counter *counte>        if (counter->state == PERF_COUNTER_STATE_ERROR)>                return 0;>> +       if (count < perf_counter_read_size(counter))> +               return -ENOSPC;> +>        WARN_ON_ONCE(counter->ctx->parent_ctx);>        mutex_lock(&counter->child_mutex);> -       values[0] = perf_counter_read_tree(counter);> -       n = 1;> -       if (counter->attr.read_format & PERF_FORMAT_TOTAL_TIME_ENABLED)> -               values[n++] = counter->total_time_enabled +> -                       atomic64_read(&counter->child_total_time_enabled);> -       if (counter->attr.read_format & PERF_FORMAT_TOTAL_TIME_RUNNING)> -               values[n++] = counter->total_time_running +> -                       atomic64_read(&counter->child_total_time_running);> -       if (counter->attr.read_format & PERF_FORMAT_ID)> -               values[n++] = primary_counter_id(counter);> +       if (read_format & PERF_FORMAT_GROUP)> +               ret = perf_counter_read_group(counter, read_format, buf);> +       else> +               ret = perf_counter_read_one(counter, read_format, buf);>        mutex_unlock(&counter->child_mutex);>> -       if (count < n * sizeof(u64))> -               return -EINVAL;> -       count = n * sizeof(u64);> -> -       if (copy_to_user(buf, values, count))> -               return -EFAULT;> -> -       return count;> +       return ret;>  }>>  static ssize_t> @@ -2631,6 +2728,79 @@ static u32 perf_counter_tid(struct perf_>        return task_pid_nr_ns(p, counter->ns);>  }>> +static void perf_output_read_one(struct perf_output_handle *handle,> +                                struct perf_counter *counter)> +{> +       u64 read_format = counter->attr.read_format;> +       u64 values[4];> +       int n = 0;> +> +       values[n++] = atomic64_read(&counter->count);> +       if (read_format & PERF_FORMAT_TOTAL_TIME_ENABLED) {> +               values[n++] = counter->total_time_enabled +> +                       atomic64_read(&counter->child_total_time_enabled);> +       }> +       if (read_format & PERF_FORMAT_TOTAL_TIME_RUNNING) {> +               values[n++] = counter->total_time_running +> +                       atomic64_read(&counter->child_total_time_running);> +       }> +       if (read_format & PERF_FORMAT_ID)> +               values[n++] = primary_counter_id(counter);> +> +       perf_output_copy(handle, values, n * sizeof(u64));> +}> +> +/*> + * XXX PERF_FORMAT_GROUP vs inherited counters seems difficult.> + */> +static void perf_output_read_group(struct perf_output_handle *handle,> +                           struct perf_counter *counter)> +{> +       struct perf_counter *leader = counter->group_leader, *sub;> +       u64 read_format = counter->attr.read_format;> +       u64 values[5];> +       int n = 0;> +> +       values[n++] = 1 + leader->nr_siblings;> +> +       if (read_format & PERF_FORMAT_TOTAL_TIME_ENABLED)> +               values[n++] = leader->total_time_enabled;> +> +       if (read_format & PERF_FORMAT_TOTAL_TIME_RUNNING)> +               values[n++] = leader->total_time_running;> +> +       if (leader != counter)> +               leader->pmu->read(leader);> +> +       values[n++] = atomic64_read(&leader->count);> +       if (read_format & PERF_FORMAT_ID)> +               values[n++] = primary_counter_id(leader);> +> +       perf_output_copy(handle, values, n * sizeof(u64));> +> +       list_for_each_entry(sub, &leader->sibling_list, list_entry) {> +               n = 0;> +> +               if (sub != counter)> +                       sub->pmu->read(sub);> +> +               values[n++] = atomic64_read(&sub->count);> +               if (read_format & PERF_FORMAT_ID)> +                       values[n++] = primary_counter_id(sub);> +> +               perf_output_copy(handle, values, n * sizeof(u64));> +       }> +}> +> +static void perf_output_read(struct perf_output_handle *handle,> +                            struct perf_counter *counter)> +{> +       if (counter->attr.read_format & PERF_FORMAT_GROUP)> +               perf_output_read_group(handle, counter);> +       else> +               perf_output_read_one(handle, counter);> +}> +>  void perf_counter_output(struct perf_counter *counter, int nmi,>                                struct perf_sample_data *data)>  {> @@ -2642,10 +2812,6 @@ void perf_counter_output(struct perf_cou>        struct {>                u32 pid, tid;>        } tid_entry;> -       struct {> -               u64 id;> -               u64 counter;> -       } group_entry;>        struct perf_callchain_entry *callchain = NULL;>        int callchain_size = 0;>        u64 time;> @@ -2700,10 +2866,8 @@ void perf_counter_output(struct perf_cou>        if (sample_type & PERF_SAMPLE_PERIOD)>                header.size += sizeof(u64);>> -       if (sample_type & PERF_SAMPLE_GROUP) {> -               header.size += sizeof(u64) +> -                       counter->nr_siblings * sizeof(group_entry);> -       }> +       if (sample_type & PERF_SAMPLE_READ)> +               header.size += perf_counter_read_size(counter);>>        if (sample_type & PERF_SAMPLE_CALLCHAIN) {>                callchain = perf_callchain(data->regs);> @@ -2760,26 +2924,8 @@ void perf_counter_output(struct perf_cou>        if (sample_type & PERF_SAMPLE_PERIOD)>                perf_output_put(&handle, data->period);>> -       /*> -        * XXX PERF_SAMPLE_GROUP vs inherited counters seems difficult.> -        */> -       if (sample_type & PERF_SAMPLE_GROUP) {> -               struct perf_counter *leader, *sub;> -               u64 nr = counter->nr_siblings;> -> -               perf_output_put(&handle, nr);> -> -               leader = counter->group_leader;> -               list_for_each_entry(sub, &leader->sibling_list, list_entry) {> -                       if (sub != counter)> -                               sub->pmu->read(sub);> -> -                       group_entry.id = primary_counter_id(sub);> -                       group_entry.counter = atomic64_read(&sub->count);> -> -                       perf_output_put(&handle, group_entry);> -               }> -       }> +       if (sample_type & PERF_SAMPLE_READ)> +               perf_output_read(&handle, counter);>>        if (sample_type & PERF_SAMPLE_CALLCHAIN) {>                if (callchain)> @@ -2818,8 +2964,6 @@ struct perf_read_event {>>        u32                             pid;>        u32                             tid;> -       u64                             value;> -       u64                             format[3];>  };>>  static void> @@ -2831,34 +2975,20 @@ perf_counter_read_event(struct perf_coun>                .header = {>                        .type = PERF_EVENT_READ,>                        .misc = 0,> -                       .size = sizeof(event) - sizeof(event.format),> +                       .size = sizeof(event) + perf_counter_read_size(counter),>                },>                .pid = perf_counter_pid(counter, task),>                .tid = perf_counter_tid(counter, task),> -               .value = atomic64_read(&counter->count),>        };> -       int ret, i = 0;> -> -       if (counter->attr.read_format & PERF_FORMAT_TOTAL_TIME_ENABLED) {> -               event.header.size += sizeof(u64);> -               event.format[i++] = counter->total_time_enabled;> -       }> -> -       if (counter->attr.read_format & PERF_FORMAT_TOTAL_TIME_RUNNING) {> -               event.header.size += sizeof(u64);> -               event.format[i++] = counter->total_time_running;> -       }> -> -       if (counter->attr.read_format & PERF_FORMAT_ID) {> -               event.header.size += sizeof(u64);> -               event.format[i++] = primary_counter_id(counter);> -       }> +       int ret;>>        ret = perf_output_begin(&handle, counter, event.header.size, 0, 0);>        if (ret)>                return;>> -       perf_output_copy(&handle, &event, event.header.size);> +       perf_output_put(&handle, event);> +       perf_output_read(&handle, counter);> +>        perf_output_end(&handle);>  }>> @@ -3929,9 +4059,9 @@ perf_counter_alloc(struct perf_counter_a>        atomic64_set(&hwc->period_left, hwc->sample_period);>>        /*> -        * we currently do not support PERF_SAMPLE_GROUP on inherited counters> +        * we currently do not support PERF_FORMAT_GROUP on inherited counters>         */> -       if (attr->inherit && (attr->sample_type & PERF_SAMPLE_GROUP))> +       if (attr->inherit && (attr->read_format & PERF_FORMAT_GROUP))>                goto done;>>        switch (attr->type) {>> -->>{.n++%ݶw{.n+{G{ayʇڙ,jfhz_(階ݢj"mG?&~iOzv^m ?I