----- "Lucas Meneghel Rodrigues" <lmr@xxxxxxxxxx> wrote:
> 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
It would actually be
get_time(session, time_command, time_filter_re, time_format).
> 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
I just sent a corrected version of the patch because I didn't notice you've
already committed it. If you can't use the corrected patch I'll send a new
one that contains only the difference between the two.
--
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
