perf header: Refactor pipe mode end marker handling

In non-pipe/data mode the header has a 256-bit bitmap representing
whether a feature is enabled or not. In pipe mode features are written
out in perf_event__synthesize_features as PERF_RECORD_HEADER_FEATURE
events with a special zero sized marker for the last feature. If a new
feature is added the last feature marker event appears as that feature
from old pipe mode perf data. As the event is zero sized it will fail
to be processed and generally terminate perf.

Add a last_feat variable to the header that in non-pipe/data mode is
just HEADER_LAST_FEATURE. In pipe mode compute the last_feat by
handling zero sized feature events, assuming they are the marker and
updating last_feat accordingly. Potentially a feature event could be
zero sized and so still process the feature event, just ignore the
error if it fails.

As perf_event__process_feature can properly handle pipe mode data,
migrate users to it except for report that still wants to group events
and stop header printing with the last feature marker. Make
perf_event__process_feature non-fatal in the case of a newer feature
than this version of perf's HEADER_LAST_FEATURE, which was the
behavior all users wanted.

Signed-off-by: Ian Rogers <irogers@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>

diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index 686ad08561d66924e037103ab14e04f59d33e731..530348b6981b73bed9ba91f5445e8ebe7644e2ef 100644 (file)
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -313,15 +313,6 @@ out_put:
        return ret;
 }
 
-static int process_feature_event(const struct perf_tool *tool __maybe_unused,
-                                struct perf_session *session,
-                                union perf_event *event)
-{
-       if (event->feat.feat_id < HEADER_LAST_FEATURE)
-               return perf_event__process_feature(session, event);
-       return 0;
-}
-
 static int hist_entry__stdio_annotate(struct hist_entry *he,
                                    struct evsel *evsel,
                                    struct perf_annotate *ann)
@@ -875,7 +866,7 @@ int cmd_annotate(int argc, const char **argv)
        annotate.tool.id_index  = perf_event__process_id_index;
        annotate.tool.auxtrace_info     = perf_event__process_auxtrace_info;
        annotate.tool.auxtrace  = perf_event__process_auxtrace;
-       annotate.tool.feature   = process_feature_event;
+       annotate.tool.feature   = perf_event__process_feature;
        annotate.tool.ordering_requires_timestamps = true;
 
        annotate.session = perf_session__new(&data, &annotate.tool);

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 343c0ada5ea1bcc9a921b21983ae70e763c3a057..95c0bdba6b11652cdfa95631fc82a2782dc7feef 100644 (file)
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -245,25 +245,20 @@ static int process_feature_event(const struct perf_tool *tool,
                                 union perf_event *event)
 {
        struct report *rep = container_of(tool, struct report, tool);
+       int ret = perf_event__process_feature(tool, session, event);
 
-       if (event->feat.feat_id < HEADER_LAST_FEATURE)
-               return perf_event__process_feature(session, event);
+       if (ret == 0 && event->header.size == sizeof(struct perf_record_header_feature) &&
+           (int)event->feat.feat_id >= session->header.last_feat) {
+               /*
+                * (feat_id = HEADER_LAST_FEATURE) is the end marker which means
+                * all features are received.
+                */
+               if (rep->header_only)
+                       session_done = 1;
 
-       if (event->feat.feat_id != HEADER_LAST_FEATURE) {
-               pr_err("failed: wrong feature ID: %" PRI_lu64 "\n",
-                      event->feat.feat_id);
-               return -1;
-       } else if (rep->header_only) {
-               session_done = 1;
+               setup_forced_leader(rep, session->evlist);
        }
-
-       /*
-        * (feat_id = HEADER_LAST_FEATURE) is the end marker which
-        * means all features are received, now we can force the
-        * group if needed.
-        */
-       setup_forced_leader(rep, session->evlist);
-       return 0;
+       return ret;
 }
 
 static int process_sample_event(const struct perf_tool *tool,

diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index b005b23f9d8cd273dd3ef2620cad38a707d34f85..622130d3aed4a4e5103d0da660df80596287ecd9 100644 (file)
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -3944,15 +3944,6 @@ int process_cpu_map_event(const struct perf_tool *tool,
        return set_maps(script);
 }
 
-static int process_feature_event(const struct perf_tool *tool __maybe_unused,
-                                struct perf_session *session,
-                                union perf_event *event)
-{
-       if (event->feat.feat_id < HEADER_LAST_FEATURE)
-               return perf_event__process_feature(session, event);
-       return 0;
-}
-
 static int perf_script__process_auxtrace_info(const struct perf_tool *tool,
                                              struct perf_session *session,
                                              union perf_event *event)
@@ -4427,7 +4418,7 @@ script_found:
 #ifdef HAVE_LIBTRACEEVENT
        script.tool.tracing_data         = perf_event__process_tracing_data;
 #endif
-       script.tool.feature              = process_feature_event;
+       script.tool.feature              = perf_event__process_feature;
        script.tool.build_id             = perf_event__process_build_id;
        script.tool.id_index             = perf_event__process_id_index;
        script.tool.auxtrace_info        = perf_script__process_auxtrace_info;

diff --git a/tools/perf/util/data-convert-bt.c b/tools/perf/util/data-convert-bt.c
index ba1c8e48d4952e4a4bd2551e380a9c420cd25a99..665bf8eea24b19d03faf0d7096321f0164847eb3 100644 (file)
--- a/tools/perf/util/data-convert-bt.c
+++ b/tools/perf/util/data-convert-bt.c
@@ -1412,13 +1412,10 @@ static int process_feature_event(const struct perf_tool *tool,
        struct convert *c = container_of(tool, struct convert, tool);
        struct ctf_writer *cw = &c->writer;
        struct perf_record_header_feature *fe = &event->feat;
+       int ret = perf_event__process_feature(tool, session, event);
 
-       if (event->feat.feat_id < HEADER_LAST_FEATURE) {
-               int ret = perf_event__process_feature(session, event);
-
-               if (ret)
-                       return ret;
-       }
+       if (ret)
+               return ret;
 
        switch (fe->feat_id) {
        case HEADER_HOSTNAME:

diff --git a/tools/perf/util/data-convert-json.c b/tools/perf/util/data-convert-json.c
index 6a626322476a28d0a0511c7d7525b23f2771c069..4b1b2f7bed2516913ef1f76cf963fa095d639ab9 100644 (file)
--- a/tools/perf/util/data-convert-json.c
+++ b/tools/perf/util/data-convert-json.c
@@ -326,16 +326,6 @@ static void output_headers(struct perf_session *session, struct convert_json *c)
        output_json_format(out, false, 2, "]");
 }
 
-static int process_feature_event(const struct perf_tool *tool __maybe_unused,
-                                struct perf_session *session,
-                                union perf_event *event)
-{
-       if (event->feat.feat_id < HEADER_LAST_FEATURE)
-               return perf_event__process_feature(session, event);
-
-       return 0;
-}
-
 int bt_convert__perf2json(const char *input_name, const char *output_name,
                struct perf_data_convert_opts *opts __maybe_unused)
 {
@@ -375,7 +365,7 @@ int bt_convert__perf2json(const char *input_name, const char *output_name,
        c.tool.auxtrace       = perf_event__process_auxtrace;
        c.tool.event_update   = perf_event__process_event_update;
        c.tool.attr           = perf_event__process_attr;
-       c.tool.feature        = process_feature_event;
+       c.tool.feature        = perf_event__process_feature;
        c.tool.ordering_requires_timestamps = true;
 
        if (opts->all) {

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 9f1fe35a6b8a428ff390cee9c6cd5dc5e0b17261..ad7d09a481bb35e92f9e8213ef6d45e6d1493f39 100644 (file)
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -3819,11 +3819,11 @@ static int perf_file_section__fprintf_info(struct perf_file_section *section,
        struct feat_fd ff;
 
        if (lseek(fd, section->offset, SEEK_SET) == (off_t)-1) {
-               pr_debug("Failed to lseek to %" PRIu64 " offset for feature "
-                               "%d, continuing...\n", section->offset, feat);
+               pr_debug("Failed to lseek to %" PRIu64 " offset for feature %s (%d), continuing...\n",
+                        section->offset, header_feat__name(feat), feat);
                return 0;
        }
-       if (feat >= HEADER_LAST_FEATURE) {
+       if (feat >= ph->last_feat) {
                pr_warning("unknown feature %d\n", feat);
                return 0;
        }
@@ -3875,7 +3875,7 @@ int perf_header__fprintf_info(struct perf_session *session, FILE *fp, bool full)
                return 0;
 
        fprintf(fp, "# missing features: ");
-       for_each_clear_bit(bit, header->adds_features, HEADER_LAST_FEATURE) {
+       for_each_clear_bit(bit, header->adds_features, header->last_feat) {
                if (bit)
                        fprintf(fp, "%s ", feat_ops[bit].name);
        }
@@ -4205,7 +4205,7 @@ int perf_header__process_sections(struct perf_header *header, int fd,
        if (err < 0)
                goto out_free;
 
-       for_each_set_bit(feat, header->adds_features, HEADER_LAST_FEATURE) {
+       for_each_set_bit(feat, header->adds_features, header->last_feat) {
                err = process(sec++, header, feat, fd, data);
                if (err < 0)
                        goto out_free;
@@ -4420,6 +4420,7 @@ int perf_file_header__read(struct perf_file_header *header,
        ph->data_offset  = header->data.offset;
        ph->data_size    = header->data.size;
        ph->feat_offset  = header->data.offset + header->data.size;
+       ph->last_feat    = HEADER_LAST_FEATURE;
        return 0;
 }
 
@@ -4435,8 +4436,8 @@ static int perf_file_section__process(struct perf_file_section *section,
        };
 
        if (lseek(fd, section->offset, SEEK_SET) == (off_t)-1) {
-               pr_debug("Failed to lseek to %" PRIu64 " offset for feature "
-                         "%d, continuing...\n", section->offset, feat);
+               pr_debug("Failed to lseek to %" PRIu64 " offset for feature %s (%d), continuing...\n",
+                        section->offset, header_feat__name(feat), feat);
                return 0;
        }
 
@@ -4469,6 +4470,8 @@ static int perf_file_header__read_pipe(struct perf_pipe_file_header *header,
        if (ph->needs_swap)
                header->size = bswap_64(header->size);
 
+       /* The last feature is written out as a 0 sized event and will update this value. */
+       ph->last_feat = 0;
        return 0;
 }
 
@@ -4701,31 +4704,68 @@ out_delete_evlist:
        return -ENOMEM;
 }
 
-int perf_event__process_feature(struct perf_session *session,
+int perf_event__process_feature(const struct perf_tool *tool __maybe_unused,
+                               struct perf_session *session,
                                union perf_event *event)
 {
        struct feat_fd ff = { .fd = 0 };
        struct perf_record_header_feature *fe = (struct perf_record_header_feature *)event;
+       struct perf_header *header = &session->header;
        int type = fe->header.type;
-       u64 feat = fe->feat_id;
+       int feat = (int)fe->feat_id;
        int ret = 0;
        bool print = dump_trace;
+       bool last_feature_mark = false;
 
        if (type < 0 || type >= PERF_RECORD_HEADER_MAX) {
                pr_warning("invalid record type %d in pipe-mode\n", type);
                return 0;
        }
-       if (feat == HEADER_RESERVED || feat >= HEADER_LAST_FEATURE) {
-               pr_warning("invalid record type %d in pipe-mode\n", type);
+       if (feat == HEADER_RESERVED) {
+               pr_warning("invalid reserved record type in pipe-mode\n");
+               return -1;
+       }
+       if (feat < 0 || feat == INT_MAX) {
+               pr_warning("invalid value for feature type %x\n", feat);
+               return -1;
+       }
+       if (feat >= header->last_feat) {
+               if (event->header.size == sizeof(*fe)) {
+                       /*
+                        * Either an unexpected zero size feature or the
+                        * HEADER_LAST_FEATURE mark.
+                        */
+                       if (feat > header->last_feat)
+                               header->last_feat = min(feat, HEADER_LAST_FEATURE);
+                       last_feature_mark = true;
+               } else {
+                       /*
+                        * A feature but beyond what is known as in
+                        * bounds. Assume the last feature is 1 beyond this
+                        * feature.
+                        */
+                       session->header.last_feat = min(feat + 1, HEADER_LAST_FEATURE);
+               }
+       }
+       if (feat >= HEADER_LAST_FEATURE) {
+               if (!last_feature_mark) {
+                       pr_warning("unknown feature %d for data file version (%s) in this version of perf (%s)\n",
+                                  feat, header->env.version, perf_version_string);
+               }
+               return 0;
+       }
+       if (event->header.size < sizeof(*fe)) {
+               pr_warning("feature header size too small\n");
                return -1;
        }
-
        ff.buf  = (void *)fe->data;
        ff.size = event->header.size - sizeof(*fe);
-       ff.ph = &session->header;
+       ff.ph = header;
 
        if (feat_ops[feat].process && feat_ops[feat].process(&ff, NULL)) {
-               ret = -1;
+               // Processing failed, ignore when this is the last feature mark.
+               if (!last_feature_mark)
+                       ret = -1;
                goto out;
        }
 

diff --git a/tools/perf/util/header.h b/tools/perf/util/header.h
index ca22030a143435f2c87084c77e152dc6b9bdf819..41ce663d93ff8ab2007512d3472af65c1863243a 100644 (file)
--- a/tools/perf/util/header.h
+++ b/tools/perf/util/header.h
@@ -109,6 +109,7 @@ struct perf_header {
        u64                             data_size;
        u64                             feat_offset;
        DECLARE_BITMAP(adds_features, HEADER_FEAT_BITS);
+       int                             last_feat;
        struct perf_env         env;
 };
 
@@ -172,7 +173,8 @@ int perf_header__process_sections(struct perf_header *header, int fd,
 
 int perf_header__fprintf_info(struct perf_session *s, FILE *fp, bool full);
 
-int perf_event__process_feature(struct perf_session *session,
+int perf_event__process_feature(const struct perf_tool *tool,
+                               struct perf_session *session,
                                union perf_event *event);
 int perf_event__process_attr(const struct perf_tool *tool, union perf_event *event,
                             struct evlist **pevlist);

diff --git a/tools/perf/util/intel-tpebs.c b/tools/perf/util/intel-tpebs.c
index 2af5455488b2d82b528f20bdceacff1c3ed58ffc..8b615dc94e9ea34aabc74dc412ab594637a414b1 100644 (file)
--- a/tools/perf/util/intel-tpebs.c
+++ b/tools/perf/util/intel-tpebs.c
@@ -216,15 +216,6 @@ static int process_sample_event(const struct perf_tool *tool __maybe_unused,
        return 0;
 }
 
-static int process_feature_event(const struct perf_tool *tool __maybe_unused,
-                                struct perf_session *session,
-                                union perf_event *event)
-{
-       if (event->feat.feat_id < HEADER_LAST_FEATURE)
-               return perf_event__process_feature(session, event);
-       return 0;
-}
-
 static void *__sample_reader(void *arg __maybe_unused)
 {
        struct perf_session *session;
@@ -237,7 +228,7 @@ static void *__sample_reader(void *arg __maybe_unused)
 
        perf_tool__init(&tool, /*ordered_events=*/false);
        tool.sample = process_sample_event;
-       tool.feature = process_feature_event;
+       tool.feature = perf_event__process_feature;
        tool.attr = perf_event__process_attr;
 
        session = perf_session__new(&data, &tool);