Ok Michael, please send me the diff between the patches so I can apply it!
On Fri, Aug 7, 2009 at 6:45 PM, Michael Goldish<mgoldish@xxxxxxxxxx> wrote:
>
> ----- "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.
> _______________________________________________
> Autotest mailing list
> Autotest@xxxxxxxxxxxxxxx
> http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
>
--
Lucas Meneghel
--
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
