On 07/26/2012 07:18 AM, Satheesh Rajendran wrote:
> Hi,
>
> Thanks a lot for your comments, working on them, I have few comments
> below inline.
>
>
> On Wed, 2012-07-25 at 11:32 -0400, Chris Evich wrote:
>> Hi,
>>
>> Thanks for the contribution and updates. Here are some comments that
>> hopefully will help polish this up further. Overall it's coming
>> together nicely
>>
>> Tip: This is a fairly short set of patches, so don't bother here. But
>> in the future for anything more complex, it's helpful to reviewers if
>> you use multiple discrete commits. Then use git format-patch
>> cover-letter option to make multiple patch e-mails.
>
>> On 07/24/2012 05:30 AM, sathn...@linux.vnet.ibm.com wrote:
...cut...
>>> + # Get the vcpus pid
>>> + vcpus_pid = guest_vcpu_pids(domname)
>>> + vcpu_pid=vcpus_pid[vcpu]
>>> +
>>> + # Get the actual cpu affinity value in the proc entry
>>> + cmd = "cat /proc/%s/task/%s/status|grep Cpus_allowed:| awk '{print
>>> $2}'" % (pid,vcpu_pid)
>>> + output = str(utils.run(cmd, ignore_status=False).stdout)
>>> + actual_output_proc = int(output,16)
>>
>> This is really cool functionality! I can see how it might be useful in
>> other tests, even beyond the virt. autotest subtests. Maybe consider
>> adding a cpu_affinity_by_task() function into a client/shared/ module
>> somewhere?
>>
>
> Yes this can be placed in client/shared/base_utils.py
> would place it.
>
Sounds great. Please send this as a separate patch this way we can
commit it even if other new code needs more work.
...cut...
>>> +
>>> + # Get the vm name, pid of vm and check for alive
>>> + vm_name = params.get("main_vm")
>>> + vm = env.get_vm(params["main_vm"])
>>> + vm.verify_alive()
>>> + pid = vm.get_pid()
>>> +
>>> + # Get the host cpu count
>>> + cmd = "cat /proc/cpuinfo |grep processor|wc -l"
>>> + host_cpu_count_result = utils.run(cmd, ignore_status=False)
>>> + host_cpu_count = host_cpu_count_result.stdout.strip()
>>
>> I've seen a few tests that query the # of host CPUs. Maybe consider
>> adding this as a generic function to a client/virt or client/shared
>> module? It's good to share generic code like this between tests for
>> maintenance/enhancement reasons.
>>
> Yes,I would place this in client/shared/base_utils.py
>
Awesome. Again, send this as a separate patch or include with
cpu_affinity_by_task() so we can review/commit in parallel to other
work. Thanks!
>>> +
>>> + # Get the guest vcpu count
>>> + guest_vcpu_count = libvirt_vm.virsh_vcpucount_live(vm_name)
>>> +
>>> + # Run test case
>>> + for vcpu in range(int(guest_vcpu_count)):
>>> + for cpu in range(int(host_cpu_count)):
>>> + libvirt_vm.virsh_vcpupin(vm_name,vcpu,cpu)
>>> + virsh_check_vcpupin(vm_name,vcpu,cpu,pid)
>>
>> This is good, I like the simplicity of doing this in a loop.
>>
>
> Thanks, would add virsh_vcpupin to VM class and can be used like
> vm.vcpupin(vcpu,cpu)
>
Actually let's avoid this for now, having it in the test code is fine.
I think it's a niche enough case than just having it in a test is good
enough. Putting it as a method into main code only makes sense if you
think more than three or four separate tests will make use of it. If it
becomes the case later, we can always move it. I think it's okay to
just leave it in your test module for now.
...cut...
Thanks! I'm looking forward to v3!
--
Chris Evich, RHCA, RHCE, RHCDS, RHCSS
Quality Assurance Engineer
e-mail: cevich + `@' + redhat.com o: 1-888-RED-HAT1 x44214
_______________________________________________
Autotest mailing list
Autotest@test.kernel.org
http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
