-
Notifications
You must be signed in to change notification settings - Fork 478
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 broken kernel tracing in presence of offline CPUs #1849
Conversation
0772e53
to
f27da39
Compare
With the PR merged,
will produce kernel trace output (note the hard-coded path to libmcount, I am running this from the build environment) |
f27da39
to
8d88be5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
utils/kernel.c
Outdated
@@ -615,10 +615,10 @@ int record_kernel_trace_pipe(struct uftrace_kernel_writer *kernel, int cpu, int | |||
if (n < 0) { | |||
if (errno == EINTR) | |||
goto retry; | |||
if (errno == EAGAIN) | |||
if ((errno == EAGAIN) || (errno == ENODEV)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shoffmeister @honggyukim
There is no need (and it's not usual) to add the extra braces around ==
because it precedence and order of evaluation comes before the logical or ||
:
if ((errno == EAGAIN) || (errno == ENODEV)) | |
if (errno == EAGAIN || errno == ENODEV) |
A detailed reference: https://learn.microsoft.com/en-us/cpp/c-language/precedence-and-order-of-evaluation?view=msvc-170
Otherwise, LGTM.
If @shoffmeister did not deny maintainer write access, uftrace maintainers should be able to apply my suggestion by clicking the button "commit suggestion".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, cool. I didn't notice this feature. Yes I prefer no parenthesis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the redundant brackets and force pushed for a simple commit history.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks the brackets are still there. Please check again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, sorry + fixed!
8d88be5
to
e303d86
Compare
Offline CPUs may be encountered in a number of scenarios, for instance in the presence of virtualization through VMware Workstation, where the product advertises 128 CPUs as present, but then only makes a (configurable) subset of these available and online for use. Signed-off-by: Stefan Hoffmeister <stefan.hoffmeister@econos.de>
e303d86
to
8f68eb3
Compare
Offline CPUs may be encountered in a number of scenarios, for instance in the presence of virtualization through VMware Workstation, where the product advertises 128 CPUs as present, but then only makes a (configurable) subset of these available and online for use.
Fixes #1846