Re: [Autotest] [KVM-AUTOTEST] KSM-overcommit test v.2 (python version)

["Lukáš Doktor" <ldoktor@redhat.com>]

Hello Dor,

Thank you for your review. I have few questions about your comments:

----------- snip -----------
>> + stat += "Guests memsh = {"
>> + for vm in lvms:
>> + if vm.is_dead():
>> + logging.info("Trying to get informations of death VM: %s"
>> + % vm.name)
>> + continue
>
> You can fail the entire test. Afterwards it will be hard to find the issue.
>

Well if it's what the community wants, we can change it. We just didn't 
want to lose information about the rest of the systems. Perhaps we can 
set some DIE flag and after collecting all statistics raise an Error.

----------- snip -----------
>> + def get_true_pid(vm):
>> + pid = vm.process.get_pid()
>> + for i in range(1,10):
>> + pid = pid + 1
>
> What are you trying to do here? It's seems like a nasty hack that might
> fail on load.
>

Yes and I'm really sorry for this ugly hack. The qemu command has 
changed since the first patch was made. Nowadays the "vm.pid" returns 
PID of the command itself, not the actual qemu process.
We need to have the PID of the actual qemu process, which is executed by 
the command with PID "vm.pid". That's why first I try finding the qemu 
process as the following "vm.pid" PID. I haven't found another solution 
yet (in case we don't want to change the qemu command back in the 
framework).
We have tested this solution under heavy process load and either first 
or second part always finds the right value.

----------- snip -----------
>> + if (params['ksm_test_size'] == "paralel") :
>> + vmsc = 1
>> + overcommit = 1
>> + mem = host_mem
>> + # 32bit system adjustment
>> + if not params['image_name'].endswith("64"):
>> + logging.debug("Probably i386 guest architecture, "\
>> + "max allocator mem = 2G")
>
> Better not to relay on the guest name. You can test percentage of the
> guest mem.
>

What do you mean by "percentage of the guest mem"? This adjustment is 
made because the maximum memory for 1 process in 32 bit OS is 2GB.
Testing of the 'image_name' showed to be most reliable method we found.

----------- snip -----------
>> + # Guest can have more than 2G but kvm mem + 1MB (allocator itself)
>> + # can't
>> + if (host_mem> 2048):
>> + mem = 2047
>> +
>> +
>> + if os.popen("uname -i").readline().startswith("i386"):
>> + logging.debug("Host is i386 architecture, max guest mem is 2G")
>
> There are bigger 32 bit guests.
>
How do you mean this note? We are testing whether the host machine is 32 
bit. If so, the maximum process allocation is 2GB (similar case to 32 
bit guest) but this time the whole qemu process (2GB qemu machine + 64 
MB qemu overhead) can't exceeded 2GB.
Still the maximum memory used in test is the same (as we increase the VM 
count - host_mem = quest_mem * vm_count; quest_mem is decreased, 
vm_count is increased)

----------- snip -----------
>> +
>> + # Copy the allocator.c into guests
>
> .py
>
yes indeed.

----------- snip -----------
>> + # Let kksmd works (until shared mem rich expected value)
>> + shm = 0
>> + i = 0
>> + cmd = "cat/proc/%d/statm" % get_true_pid(vm)
>> + while shm< ksm_size:
>> + if i> 64:
>> + logging.info(get_stat(lvms))
>> + raise error.TestError("SHM didn't merged the memory until "\
>> + "the DL on guest: %s"% (vm.name))
>> + logging.debug("Sleep(%d)" % (ksm_size / 200 * perf_ratio))
>> + time.sleep(ksm_size / 200 * perf_ratio)
>> + try:
>> + shm = int(os.popen(cmd).readline().split()[2])
>> + shm = shm * 4 / 1024
>> + i = i + 1
>
> Either you have nice statistic calculation function or not.
> I vote for the first case.
>

Yes, we are using the statistics function for the output. But in this 
case we just need to know the shm value, not to log anything.
If this is a big problem even for others, we can split the statistics 
function into 2:
int = _get_stat(vm) - returns shm value
string = get_stat(vm) - Uses _get_stats and creates a nice log output

----------- snip -----------
>> + """ Check if memory in max loading guest is allright"""
>> + logging.info("Starting phase 3b")
>> +
>> + """ Kill rest of machine"""
>
> We should have a function for it for all kvm autotest
>

you think lsessions[i].close() instead of (status,data) = 
lsessions[i].get_command_status_output("exit;",20)?
Yes, it would be better.

>> + for i in range(last_vm+1, vmsc):
>> + (status,data) = lsessions[i].get_command_status_output("exit;",20)
>> + if i == (vmsc-1):
>> + logging.info(get_stat([lvms[i]]))
>> + lvms[i].destroy(gracefully = False)

----------- snip -----------
>> + def phase_paralel():
>> + """ Paralel page spliting """
>> + logging.info("Phase 1: Paralel page spliting")
>> + # We have to wait until allocator is finished (it waits 5 seconds to
>> + # clean the socket
>> +
>
> The whole function is very similar to phase_separate_first_guest please
> refactor them.
>
Yes, those functions are a bit similar. On the other hand there are some 
differences. Instead of creating more complex function we agreed to 
split them for better readability of the code.

----------- snip -----------
>> + while shm< ksm_size:
>> + if i> 64:
>> + logging.info(get_stat(lvms))
>> + raise error.TestError("SHM didn't merged the memory until DL")
>> + logging.debug("Sleep(%d)" % (ksm_size / 200 * perf_ratio))
>> + time.sleep(ksm_size / 200 * perf_ratio)
>> + try:
>> + shm = int(os.popen(cmd).readline().split()[2])
>> + shm = shm * 4 / 1024
>> + except:
>> + raise error.TestError("Could not fetch shmem info from proc")
>
> Didn't you needed to increase i?
>
yes, you are right. This line somehow disappeard "i = i + 1"...

----------- snip -----------
>> + def compare_page(self,original,inmem):
>> + """
>> + compare memory
>> +
>
>
> Why do you need it? Is it to really check ksm didn't do damage?
> Interesting, I never doubted ksm for that. Actually it is good idea to
> test...
>

We were asked to do so (be paranoid, everything could happened). We can 
make this voluntary in the config.


Once again thanks, I'm looking forward to your replay.

Best regards,
Lukáš Doktor