On 06/17/2011 04:21 PM, Andrea Arcangeli wrote:
> On Thu, Jun 16, 2011 at 01:34:54PM -0300, Lucas Meneghel Rodrigues wrote:
>> On Thu, 2011-06-16 at 17:56 +0200, Andrea Arcangeli wrote:
>>> Hi Lucas,
>> Hi Andrea, thanks for the review! Yiqiao is working on the patchset, v3
>> or v4 will contain the fixes you have pointed out.
> Thanks! BTW, I'm in the process of reviewing the other patches too.
>
>>>> + # test_cfg holds all the desired host config values we want to set
>>>> + # before THP tests
>>>> + test_cfg={"%s/defrag" % self.thp_path: "yes",
>>> upstream>=3.0.0 this would value would be 1 (on 2.6.38 it's still "yes").
>>>
>>>> + "%s/enabled" % self.thp_path: "always",
>>>> + "%s/khugepaged/defrag" % self.thp_path: "yes",
>>> Upstream>=3.0.0 it's 1.
>> Ok, we'll set the values conditionally depending on the running kernel
>> version.
> Checking the kernel version may be less reliable than reading it back
> and seeing if it's yes|no or 0|1 (not sure how the stable kernels will
> work with 3.0.0 etc..), the future numbering seems a bit variable
> these days. Theoretically checking the version number is better, but I
> suspect reading it and handling both formats without regard of the
> kernel version is simpler in the end. But it's definitely up to you.
>
Hi Andrea
I am improving this part. I will go through the thp config path and find
all files which are writeable. Then will read from these files and
change it based on the default value store in it. So it will not depend
on the kernel version any more.
>>>> + "%s/khugepaged/scan_sleep_millisecs" % self.thp_path:
>>>> "100",
>>> So this is meant to increase khugepaged scan rate compared to the
>>> default.
>> Yep.
> Sounds good!
>
>>>> + "/sys/kernel/mm/ksm/run": "1",
>>>> + "/proc/sys/vm/nr_hugepages":"0"}
>>> nr_hugepages is unlikely to matter.
>> Dropping this then.
> I think you can drop it yes. I think this only could cause hugetlbfs
> allocation failures if there's some VM backed by hugetlbfs pages that
> could then fail allocation and they have no fallback like THP.
>
> hugetlbfs and THP are completely separated handled, even if they share
> plenty code for the clear-copy and
> put_page/get_page/head/tail/compound logics.
>
OK.
>>>> + def khugepaged_test(self):
>>>> + """
>>>> + Start, stop and frequency change test for khugepaged.
>>>> + """
>>>> + status_list = ["never", "always", "never"]
>>>> + for status in status_list:
>>>> + self.set_params(self.test_config, "enabled", status)
>>>> + try:
>>>> + utils.run('pgrep khugepaged')
>>>> + except error.CmdError:
>>>> + raise THPKhugepagedError("khugepaged can not be set to "
>>>> + "status %s" % status)
>>> khugepaged will quit when enabled is set to never, but it's not a bug
>>> (it's to save the memory of the kernel thread stack when THP is
>>> disabled). So I'm unsure if the pgrep is going to create spurious errors.
>> Ok, will improve this check then, based on the expected behavior.
> If you wait a few seconds (a few seconds should be enough even with
> heavy loads) khugepaged should disappear after setting enabled=never,
> and again in a few seconds it should appear after setting
> enabled=always.
>
> So when setting enabled=never, you can wait with a timeout until
> pgrep throws the CmdError as expected, and print an error if it pgrep
> keeps succeeding. The opposite when setting enabled=always.
>
> This is a bit kernel dependent (for example other kernel daemons
> always are present even when the feature is disabled, I handled the
> races in shutting down and turning on the daemon by optimizing it
> more, but it's not very important to check that the khugepaged goes
> away, so you may just limit the pgrep check to when enabled=always to
> verify khugepaged is there, but you should wait a few seconds before
> failing as it may take a reschedule for khugepaged to start).
>
OK, thanks. I will write this part again and set a sleep time in this
test. And the disable check is using for make sure the init and exit
part code works well. We want to cover the code as much as we could. But
as you said there are some kernel daemons always presented, I should add
a option as "check_disable" in our configuration files. If we set it to
"yes" we will check it, and if we set it to "no" in our test
configuration files we will not check it in our test.
> Thanks,
> Andrea
_______________________________________________
Autotest mailing list
[email protected]
http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
