-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44379/#review130504
-----------------------------------------------------------


Ship it!




Thanks for the comment!

I've added a commit description and made some minor adjustments that I'll 
commit with your change:

```
commit a5c81d4077400892cd3a5c306143f16903aac62c
Author: fan du <fan...@intel.com>
Date:   Mon Apr 25 13:50:50 2016 -0700

    Fixed the 'perf' parsing logic.

    Previously the 'perf' parsing logic used the kernel version to
    determine the token ordering. However, this approach breaks
    when distributions backport perf parsing changes onto older
    kernel versions. This updates the parsing logic to understand
    all existing formats.

    Co-authored with haosdent.

    Review: https://reviews.apache.org/r/44379/

diff --git a/src/linux/perf.cpp b/src/linux/perf.cpp
index 749e676..2364ab5 100644
--- a/src/linux/perf.cpp
+++ b/src/linux/perf.cpp
@@ -343,28 +343,24 @@ struct Sample
     // because the unit field can be empty.
     vector<string> tokens = strings::split(line, PERF_DELIMITER);

-    if (version >= Version(4, 0, 0)) {
-      // Optional running time and ratio were introduced in Linux v4.0,
-      // which make the format either:
-      //   value,unit,event,cgroup
-      //   value,unit,event,cgroup,running,ratio
-      if ((tokens.size() == 4) || (tokens.size() == 6)) {
-        return Sample({tokens[0], internal::normalize(tokens[2]), tokens[3]});
-      }
-    } else if (version >= Version(3, 13, 0)) {
-      // Unit was added in Linux v3.13, making the format:
-      //   value,unit,event,cgroup
-      if (tokens.size() == 4) {
-        return Sample({tokens[0], internal::normalize(tokens[2]), tokens[3]});
-      }
-    } else {
-      // Expected format for Linux kernel <= 3.12 is:
-      //   value,event,cgroup
-      if (tokens.size() == 3) {
-        return Sample({tokens[0], internal::normalize(tokens[1]), tokens[2]});
-      }
+    // The following formats are possible:
+    //   (1) value,event,cgroup (since Linux v2.6.39)
+    //   (2) value,unit,event,cgroup (since Linux v3.14)
+    //   (3) value,unit,event,cgroup,running,ratio (since Linux v4.1)
+    //
+    // Note that we do not use the kernel version when parsing
+    // because OS vendors often backport perf tool functionality
+    // into older kernel versions.
+
+    if (tokens.size() == 3) {
+      return Sample({tokens[0], internal::normalize(tokens[1]), tokens[2]});
+    }
+
+    if (tokens.size() == 4 || tokens.size() == 6) {
+      return Sample({tokens[0], internal::normalize(tokens[2]), tokens[3]});
     }

+    // Bail out if the format is not recognized.
     return Error("Unexpected number of fields");
   }
 };
```

- Ben Mahler


On April 18, 2016, 5:41 a.m., fan du wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44379/
> -----------------------------------------------------------
> 
> (Updated April 18, 2016, 5:41 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-4705
>     https://issues.apache.org/jira/browse/MESOS-4705
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Co-authored with haosdent.
> 
> 
> Diffs
> -----
> 
>   src/linux/perf.cpp 749e676aaf2ce639dd976f2b23e323300c6114c5 
> 
> Diff: https://reviews.apache.org/r/44379/diff/
> 
> 
> Testing
> -------
> 
> 1. {Found and Test} with Serenity, ema filter could get perf event statistics 
> correctly as expected.
> 2. ./bin/mesos-tests.sh --gtest_filter=PerfEventIsolatorTest* 
> --log_dir=/tmp/mesos/
> 
> 
> Thanks,
> 
> fan du
> 
>

Reply via email to