I'd have one sugestion to make on this patch:
On Wed, Aug 5, 2009 at 9:32 AM, Michael Goldish<mgoldish@xxxxxxxxxx> wrote:
> 1. As suggested by Yolkfull Chow, use a get_time() helper function that returns
> the current host and guest times.
>
> 2. Use helper functions that set and restore the CPU affinity of a process.
>
> 3. In these helper functions, set the affinity of all threads of the process,
> not just the main thread.
>
> Signed-off-by: Michael Goldish <mgoldish@xxxxxxxxxx>
> ---
> client/tests/kvm/kvm_tests.py | 68 +++++++++++++++++++++++++++-------------
> 1 files changed, 46 insertions(+), 22 deletions(-)
>
> diff --git a/client/tests/kvm/kvm_tests.py b/client/tests/kvm/kvm_tests.py
> index 308db97..67cf713 100644
> --- a/client/tests/kvm/kvm_tests.py
> +++ b/client/tests/kvm/kvm_tests.py
> @@ -604,6 +604,45 @@ def run_timedrift(test, params, env):
> @param params: Dictionary with test parameters.
> @param env: Dictionary with the test environment.
> """
> + # Helper functions
> + def set_cpu_affinity(pid, mask):
> + """
> + Set the CPU affinity of all threads of the process with PID pid.
> +
> + @param pid: The process ID.
> + @param mask: The CPU affinity mask.
> + @return: A dict containing the previous mask for each thread.
> + """
> + tids = commands.getoutput("ps -L --pid=%s -o lwp=" % pid).split()
> + prev_masks = {}
> + for tid in tids:
> + prev_mask = commands.getoutput("taskset -p %s" % tid).split()[-1]
> + prev_masks[tid] = prev_mask
> + commands.getoutput("taskset -p %s %s" % (mask, tid))
> + return prev_masks
> +
> + def restore_cpu_affinity(prev_masks):
> + """
> + Restore the CPU affinity of several threads.
> +
> + @param prev_masks: A dict containing TIDs as keys and masks as values.
> + """
> + for tid, mask in prev_masks.items():
> + commands.getoutput("taskset -p %s %s" % (mask, tid))
> +
> + def get_time():
> + """
> + Returns the host time and guest time.
> +
> + @return: A tuple containing the host time and guest time.
> + """
> + host_time = time.time()
> + session.sendline(time_command)
> + (match, s) = session.read_up_to_prompt()
> + s = re.findall(time_filter_re, s)[0]
> + guest_time = time.mktime(time.strptime(s, time_format))
> + return (host_time, guest_time)
The above function. It would be slightly clearer to make it to take
the time_command as a parameter rather than taking it from the global
namespace 'implicitely'.
> vm = kvm_utils.env_get_vm(env, params.get("main_vm"))
> if not vm:
> raise error.TestError("VM object not found in environment")
> @@ -641,19 +680,12 @@ def run_timedrift(test, params, env):
> guest_load_sessions = []
> host_load_sessions = []
>
> - # Remember the VM's previous CPU affinity
> - prev_cpu_mask = commands.getoutput("taskset -p %s" % vm.get_pid())
> - prev_cpu_mask = prev_cpu_mask.split()[-1]
> # Set the VM's CPU affinity
> - commands.getoutput("taskset -p %s %s" % (cpu_mask, vm.get_pid()))
> + prev_affinity = set_cpu_affinity(vm.get_pid(), cpu_mask)
>
> try:
> # Get time before load
> - host_time_0 = time.time()
> - session.sendline(time_command)
> - (match, s) = session.read_up_to_prompt()
> - s = re.findall(time_filter_re, s)[0]
> - guest_time_0 = time.mktime(time.strptime(s, time_format))
> + (host_time_0, guest_time_0) = get_time()
So the above call would be get_time(time_command). This way we can see
clearly that the time_command is being passed to the function.
However, not entirely necessary. Patch looks good, applied.
http://autotest.kernel.org/changeset/3514
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
