-
Notifications
You must be signed in to change notification settings - Fork 107
Move warning about too many pids to error path #834
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
Move warning about too many pids to error path #834
Conversation
| kill -s KILL $pid 2>/dev/null | ||
| return 2 | ||
| pid_to_kill=$pid | ||
| # else |
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.
Maybe add an explanation of why there are other processess running?
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.
I don't think we actually know, but we will emit a message on line 298 if we don't find a pid to kill.
ndokos
left a comment
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
29da9a9 to
9727034
Compare
| return 2 | ||
| fi | ||
|
|
||
| if [[ $len > 4 || ($len > 3 && $tool != "turbostat") ]] ;then |
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.
Should there be a comment here that explain why there is a special case for turbostat?
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.
Yes there should be. Does anybody know the history of that check?
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.
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.
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.
Sorry - I don't remember. I'll try to instrument safer_kill and see why turbostat seems to behave differently from everything else.
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.
I get 3 pids from pidof -x turbostat and also 3 pids from pidof -x iostat. The only difference is that there is a turbostat-datalog between the pbench-tool turbostat call and the "real" one, but pidof does not count the datalog process. So far, there does not seem to be a reason for treating turbostat differently.
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.
Okay, can we track this as a separate piece of work in issue #887, and not make further changes here, since this PR did not change that behavior?
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.
k-rister
left a comment
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
No description provided.