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

[Andrea Arcangeli <aarcange@redhat.com>]

Hi Lucas,

On Wed, Jun 15, 2011 at 09:18:34PM -0300, Lucas Meneghel Rodrigues wrote:
> +class THPNotSupportedError(THPError):
> +    """
> +    Thrown when host does not support tansparent hugepages.
> +    """
> +    pass

s/tansparent/transparent/

> +class THPWriteConfigError(THPError):
> +    """
> +    Thrown when host does not support tansparent hugepages.
> +    """
> +    pass

same as above.

> +class TransparentHugePageConfig(object):
> +    def __init__(self, test, params):
> +        """
> +        Find paths for transparent hugepages and kugepaged configuration. Also,
> +        back up original host configuration so it can be restored during
> +        cleanup.
> +        """
> +        self.params = params
> +
> +        RH_THP_PATH = "/sys/kernel/mm/redhat_transparent_hugepage"
> +        UPSTREAM_THP_PATH = "/sys/kernel/mm/transparent_hugepage"
> +        if os.path.isdir(RH_THP_PATH):
> +            self.thp_path = RH_THP_PATH
> +        elif os.path.isdir(UPSTREAM_THP_PATH):
> +            self.thp_path = UPSTREAM_THP_PATH
> +        else:
> +            raise THPNotSupportedError("System doesn't support transparent "
> +                                       "hugepages")
> +
> +        # 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.

> +                  "%s/khugepaged/scan_sleep_millisecs" % self.thp_path: "100",

So this is meant to increase khugepaged scan rate compared to the
default.

> +                  "%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.

> +                  "/sys/kernel/mm/ksm/run": "1",
> +                  "/proc/sys/vm/nr_hugepages":"0"}

nr_hugepages is unlikely to matter.

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