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.

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

Fix tasks states #301

Merged
merged 3 commits into from
Feb 27, 2017
Merged

Fix tasks states #301

merged 3 commits into from
Feb 27, 2017

Conversation

derkling
Copy link
Contributor

No description provided.

Copy link
Contributor

@bjackman bjackman left a comment

Choose a reason for hiding this comment

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

I seem to be in a nitpicky mood today but looks useful.

@@ -644,6 +644,17 @@ def _init_platform(self):
# Adding topology information
self.platform['topology'] = self.topology.get_level("cluster")

# Adding kernel build information
kver = self.target.execute('uname -r').strip()
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should extend devlib's KernelVersion instead of having the regex and uname call in here? I don't mind though, up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again I don't want to possibly break some devlib APIs... however, here the main goal for the regexp is to report the major.minor version and the exact SHA1 within the platform.json.

Otherwise we should update devlib to build a namedtuple with all these values...

Copy link
Contributor

Choose a reason for hiding this comment

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

No need to change any existing APIs, the only function in KernelVersion right now is __str__/__repr__ so we could just add properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this new interface in:
ARM-software/devlib#90

... let's wait for this to be merged and than I'll add a refresh of devlib to this PR.

self.platform['kernel_version'] = match.group('ver')
self.platform['kernel_sha1'] = match.group('sha1')
self.platform['kernel_version_full'] = kver
self.platform['build'] = str(self.target.kernel_version)
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest either putting all the kernel version stuff under a sub-dict keyed with "kernel" (self.platform['kernel'] = {'build': foo, 'sha1': bar, ...}) or changing '"build"' to '"kernel_build"'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That make sense... again perhaps better a namedtuple returned by devlib...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

kver = float(self._trace.platform.get('kernel_version', '3.18'))
if kver > 4.7:
TASK_STATES[2048] = "n" # TASK_NEW
TASK_MAX_STATE = 2*sorted(TASK_STATES.keys())[-1]
Copy link
Contributor

Choose a reason for hiding this comment

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

sorted(x)[-1] can just be max(x)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

kver = self.target.execute('uname -r').strip()
match = KVERSION_RE.match(kver)
if match:
self.platform['kernel_version'] = match.group('ver')
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be handy to have this as a tuple of ints (like (3, 18) instead of "3.18"), then we would leave space for richer info (e.g. stable kernels like 4.9.10) in the future. This works nicely because tuples sort lexicographically, so in the later commit you could have if self.platform.get('kernel_version', (0,)) > (4, 7):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm using a dict because I need to dump it as a JSON... the new KernelVersion set of attributes reports separate attributes for version.major.minor... we can use these attributes to build whatever tuple we need at client side.

@derkling
Copy link
Contributor Author

I guess that, as long as ARM-software/devlib#90 gets merged, this PR can enter as well.

@derkling derkling mentioned this pull request Feb 20, 2017
@derkling derkling added this to the 17.02 milestone Feb 20, 2017
@derkling derkling force-pushed the fix-tasks-states branch 3 times, most recently from 1e34d90 to 7bc91d3 Compare February 20, 2017 16:14
kver = self.target.kernel_version
self.platform['kernel'] = {t: getattr(kver, t, '') \
for t in [
'release', 'version',
Copy link
Contributor

Choose a reason for hiding this comment

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

KernelVersion doesn't have release or version attributes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yeah, sorry!

@@ -707,6 +707,18 @@ def _init_platform(self):
# Adding topology information
self.platform['topology'] = self.topology.get_level("cluster")

# Adding kernel build information
kver = self.target.kernel_version
self.platform['kernel'] = {t: getattr(kver, t, '') \
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: don't need the \

@@ -977,4 +989,8 @@ def _feature(self, feature):
r'Bcast:(.*) '
)

KVERSION_RE = re.compile(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that we have the code in devlib... this has to be removed! :-/

}
TASK_MAX_STATE = 4096
kver = float(self._trace.platform.get('kernel_version', '3.18'))
if kver > 4.7:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If @bjackman adds the new cmp API to devlib.... this should be updated as well...

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't add the __cmp__ in the end. However this needs to change anyway because 'kernel_version' isn't a field any more. I put a couple of suggested fixup patches here: https://github.com/bjackman/lisa/commits/fix-tasks-states

These platform data can be useful to know exactly on which platform
configuration tests have been executed. Specifically, the "kernel_version"
and "kernel_sha1" can be useful to cross-check a kernel with the expected
results. Moreover, some of these information (e.g. kernel_version) is
useful to tune some analysis methods depending on expected kernel feature
(e.g. available tracepoint, constants, etc.)

Signed-off-by: Patrick Bellasi <patrick.bellasi@arm.com>
This column is not currently required by the latency analysis moreover this
field is not more produced since kernel v4.4. Let's disregard it.

Signed-off-by: Patrick Bellasi <patrick.bellasi@arm.com>
Certain tasks flags are available only on more recent kernel versions.
Let's use the platform reported kernel_version number to add them when
the trace support them.

Signed-off-by: Patrick Bellasi <patrick.bellasi@arm.com>
@jlelli
Copy link
Contributor

jlelli commented Feb 27, 2017

Looks good to me. I've also indirectly tested it by using the new latency analysis API.

@jlelli jlelli merged commit 955bab2 into ARM-software:master Feb 27, 2017
douglas-raillard-arm added a commit to douglas-raillard-arm/lisa that referenced this pull request Dec 4, 2019
f7a3ecbbf Merge pull request ARM-software#302 from valschneider/ftrace_function
2ea55a58e ftrace: Add support to parse function tracing ('function' tracer)
412924f28 Merge pull request ARM-software#301 from JaviMerino/fix_documentation
e231aca72 Merge pull request ARM-software#300 from douglas-raillard-arm/fix_custom_scope
b25800328 doc: Update Dynamic traces
5babb40b6 ftrace: Add endtime attribute
23a35ecae ftrace: Avoid storing trace-cmd report output in memory
9d67e5555 tests: Fix unit tests
08c7da476 ftrace: Only add dynamic classes on non-custom scopes

git-subtree-dir: external/trappy
git-subtree-split: f7a3ecbbfb4a92031431dca31c48eb67ac9db08d
douglas-raillard-arm added a commit to douglas-raillard-arm/lisa that referenced this pull request Dec 4, 2019
f7a3ecbbf Merge pull request ARM-software#302 from valschneider/ftrace_function
2ea55a58e ftrace: Add support to parse function tracing ('function' tracer)
412924f28 Merge pull request ARM-software#301 from JaviMerino/fix_documentation
e231aca72 Merge pull request ARM-software#300 from douglas-raillard-arm/fix_custom_scope
b25800328 doc: Update Dynamic traces
5babb40b6 ftrace: Add endtime attribute
23a35ecae ftrace: Avoid storing trace-cmd report output in memory
9d67e5555 tests: Fix unit tests
08c7da476 ftrace: Only add dynamic classes on non-custom scopes

git-subtree-dir: external/trappy
git-subtree-split: f7a3ecbbfb4a92031431dca31c48eb67ac9db08d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants