Check whether the pid exists before killing. #199
Conversation
ndokos
force-pushed
the
wip-check-turbostat-kill
branch
from
56161af to
409c3a8
Compare
agent/tool-scripts/kvm-spinlock
Outdated
| # check that the pid corresponds to the tool | ||
| pidlist=$(pidof $tool) | ||
| for p in $pidlist ;do | ||
| if [ $p == $pid ] ;then |
There was a problem hiding this comment.
Do we record if we found the given pid in the pidlist? It does not appear so.
ndokos
force-pushed
the
wip-check-turbostat-kill
branch
from
409c3a8 to
9ca0707
Compare
|
This looks good to me, but I have not tested it. Having some kind of testing infrastructure for it would be great. |
ndokos
force-pushed
the
wip-check-turbostat-kill
branch
3 times, most recently
from
7a837bc to
a55d7dc
Compare
|
@portante: test-07 is wonky (it does not go through any kill - kvmtrace does some peculiar things) so I'll probably replace it with some other tool, but the rest (test-05 through -10) should be OK. Let me know what you think. |
ndokos
force-pushed
the
wip-check-turbostat-kill
branch
from
a55d7dc to
646d1c1
Compare
|
@portante: I replaced the kvmtrace with its peculiarities with the more generic vmstat. This should be ready to merge. |
agent/tool-scripts/perf
Outdated
| tool_pid_file=$pbench_tmp/$group.$iteration.$tool.pid | ||
| tool_output_file=$tool_output_dir/$tool.txt | ||
|
|
||
| mkdir $tool_output_dir |
There was a problem hiding this comment.
Are we sure we always want to do this? It used to only occur on start. Do we need this just to make unit tests pass?
There was a problem hiding this comment.
I thought it was needed for unit tests to pass, but after I reverted it, the unit tests passed anyway, so it is completely unnecessary.
ndokos
force-pushed
the
wip-check-turbostat-kill
branch
2 times, most recently
from
82337e2 to
60a6e07
Compare
Check that the pid corresponds to the tool before killing it. If there is no process with that pid, eat the error. If the kill succeeds, return success. Otherwise check if the process is still running: kill it forcibly if necessary, but return failure in that case. pbench-kill-tools: Check that the list of pids to kill is not empty.
The kvmstat, vmstat, numastat and turbostat test cases actually go through safe_kill. The iostat and sar cases do not currently. Assuming that no problems are found with safe_kill, it will be ported to the rest of the tools.
Each tool script belongs to a 3- or 4-deep process hierarchy of which pidof returned one pid (and not necessarily the one that was stored in the tool pid file); consequently, safe_kill would skip it and there were tools (and their screens) hanging around at the end. Use pidof -x to get the complete list. Change the relevant unit tests (and the mocked pidof) to accommodate this change.
ndokos
force-pushed
the
wip-check-turbostat-kill
branch
from
60a6e07 to
75ca51c
Compare
2 participants
This is probably most important for turbostat (it does not run
in VMs), but we may have to extend it to other tools.