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.
> > > + "%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.
> > > + 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).
Thanks,
Andrea
--
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
