Re: [PATCH 1/3] virt: Add Transparent Hugepages setup v2

[Lucas Meneghel Rodrigues <lmr@redhat.com>]

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. 

> > +        # 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.

> > +                  "%s/khugepaged/scan_sleep_millisecs" % self.thp_path: "100",
> 
> So this is meant to increase khugepaged scan rate compared to the
> default.

Yep.

> > +                  "%s/khugepaged/pages_to_scan" % self.thp_path: "4096",
> > +                  "%s/khugepaged/alloc_sleep_millisecs" % self.thp_path: "100",
> 
> An allocation failure is not very likely and it's generally better to
> keep this bigger than the scan_sleep_millisecs. I would keep a factor
> of 6 of difference, so you could set it to 600 if scan_sleep_millisecs
> is set to 100.

Ok, will do.

> > +                  "/sys/kernel/mm/ksm/run": "1",
> > +                  "/proc/sys/vm/nr_hugepages":"0"}
> 
> nr_hugepages is unlikely to matter.

Dropping this then.

> > +        if os.path.isfile("%s/khugepaged/enabled" % self.thp_path):
> > +            test_cfg["%s/khugepaged/enabled" % self.thp_path] = "always"
> > +        if os.path.isfile("%s/khugepaged/max_ptes_none" % self.thp_path):
> > +            test_cfg["%s/khugepaged/max_ptes_none" % self.thp_path] = "511"
> > +            test_cfg["%s/defrag" % self.thp_path] = "always"
> > +
> > +        tmp_list = []
> > +        test_config = self.params.get("test_config", None)
> > +        if test_config is not None:
> > +            tmp_list = re.split(';', test_config)
> > +        while len(tmp_list) > 0:
> > +            tmp_cfg = tmp_list.pop()
> > +            test_cfg[re.split(":", tmp_cfg)[0]] = sre.split(":", tmp_cfg)[1]
> > +
> > +        self.original_config = {}
> > +        # Save host current config, so we can restore it during cleanup
> > +        for path in test_cfg:
> > +            param = open(path, 'r').read()
> > +            if ("enabled" in param) or ("defrag" in param):
> > +                param = re.split("\[|\]", param)[1] + '\n'
> > +            self.original_config[path] = param
> > +
> > +        self.test_config = test_cfg
> > +
> > +
> > +    def set_env(self):
> > +        """
> > +        Applies test configuration on the host.
> > +        """
> > +        if self.test_config:
> > +            for path in self.test_config.keys():
> > +                file(path, 'w').write(self.test_config[path])
> > +
> > +
> > +    def set_params(self, path_dict={}, filename="", value=""):
> > +        """
> > +        Sets the value of a config file for a given path.
> > +
> > +        @param path_dict: Dict of files' paths {path : value}
> > +        @param filename: Name of file to setup
> > +        @param value: Value set to the configuration files
> > +        """
> > +        for path in path_dict.keys():
> > +            if path in filename:
> > +                try:
> > +                    file(path, "w").write(value)
> > +                except IOError, e:
> > +                    raise THPWriteConfigError("Can not set %s to %s: %s" %
> > +                                              (value, filename, e))
> > +
> > +
> > +    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.