Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

env_process: Refactor huge pages setup/cleanup steps #4054

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

env_process: Refactor huge pages setup/cleanup steps #4054

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 2 additions & 33 deletions virttest/env_process.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
from virttest.test_setup.gcov import ResetQemuGCov
from virttest.test_setup.kernel import ReloadKVMModules
from virttest.test_setup.libvirt_setup import LibvirtdDebugLogConfig
from virttest.test_setup.memory import HugePagesSetup
from virttest.test_setup.migration import MigrationEnvSetup
from virttest.test_setup.networking import (
BridgeConfig,
Expand Down Expand Up @@ -1023,6 +1024,7 @@ def preprocess(test, params, env):
_setup_manager.register(CheckVirtioWinVersion)
_setup_manager.register(CheckLibvirtVersion)
_setup_manager.register(LogVersionInfo)
_setup_manager.register(HugePagesSetup)
_setup_manager.do_setup()

vm_type = params.get("vm_type")
Expand All @@ -1031,24 +1033,6 @@ def preprocess(test, params, env):

libvirtd_inst = None

# If guest is configured to be backed by hugepages, setup hugepages in host
if params.get("hugepage") == "yes":
params["setup_hugepages"] = "yes"

if params.get("setup_hugepages") == "yes":
global _pre_hugepages_surp
h = test_setup.HugePageConfig(params)
_pre_hugepages_surp = h.ext_hugepages_surp
suggest_mem = h.setup()
if suggest_mem is not None:
params["mem"] = suggest_mem
if not params.get("hugepage_path"):
params["hugepage_path"] = h.hugepage_path
if vm_type == "libvirt":
if libvirtd_inst is None:
libvirtd_inst = utils_libvirtd.Libvirtd()
libvirtd_inst.restart()

if params.get("setup_thp") == "yes":
thp = test_setup.TransparentHugePageConfig(test, params, env)
thp.setup()
Expand Down Expand Up @@ -1534,21 +1518,6 @@ def postprocess(test, params, env):
libvirtd_inst = None
vm_type = params.get("vm_type")

if params.get("setup_hugepages") == "yes":
global _post_hugepages_surp
try:
h = test_setup.HugePageConfig(params)
h.cleanup()
if vm_type == "libvirt":
if libvirtd_inst is None:
libvirtd_inst = utils_libvirtd.Libvirtd()
libvirtd_inst.restart()
except Exception as details:
err += "\nHP cleanup: %s" % str(details).replace("\\n", "\n ")
LOG.error(details)
else:
_post_hugepages_surp = h.ext_hugepages_surp

if params.get("setup_thp") == "yes":
try:
thp = test_setup.TransparentHugePageConfig(test, params, env)
Expand Down
27 changes: 27 additions & 0 deletions virttest/test_setup/memory.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
from virttest import env_process, test_setup, utils_libvirtd
from virttest.test_setup.core import Setuper


class HugePagesSetup(Setuper):
def setup(self):
# If guest is configured to be backed by hugepages, setup hugepages in host
if self.params.get("hugepage") == "yes":
self.params["setup_hugepages"] = "yes"
if self.params.get("setup_hugepages") == "yes":
h = test_setup.HugePageConfig(self.params)
env_process._pre_hugepages_surp = h.ext_hugepages_surp
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_pre_hugepages_surp and _post_hugepages_surp are for hugepage leak check, with this Setuper, I would suggest dropping them. by returning a variable after do_cleanup. so leak_num = _post_hugepages_surp - _pre_hugepages_surp can short to leak_num = <new_var>.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understood correctly, you are proposing to make the cleanup method of this Setuper return the leak_num value?
That would also involve updating the SetupManager behavior to meet the demands of HugePagesSetup. If that's the case, we would permit every Setuper return a value, which goes against the current implementation. We would then have to update the SetupManager do_cleanup logic to handle that.

In my opinion, if we were to do that, we would have to think of a protocol of some sort to make this implementable by each Setuper instead of adding specific Setuper logic into the rather general SetupManager. Could something like adding a post_cleanup_check function into the core Setuper and calling it after the cleanup method has been called from SetupManager.do_cleanup be the answer to that issue?

I also thought on other approaches, as implementing a core Singleton abstraction, so we would be able to reach Setuper instances instead of classes from within env_process so we could call extra functions on demand after the cleanup would have terminated. However, this approach sounds too complex for a workaround, and it could introduce further issues, as Setuper instances "surviving" from one test case run to the next one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understood correctly, you are proposing to make the cleanup method of this Setuper return the leak_num value?
No, the workaround is the Setuper will calculate _post_hugepages_surp - _pre_hugepages_surp and set env_process._hugepage_leaks(take this name as example).

The complex solution you mentioned is that Setuper can return something. I agree we can do this, but not now, the implementation can closely combine env_process and Setuper.

suggest_mem = h.setup()
if suggest_mem is not None:
self.params["mem"] = suggest_mem
if not self.params.get("hugepage_path"):
self.params["hugepage_path"] = h.hugepage_path
if self.params.get("vm_type") == "libvirt":
utils_libvirtd.Libvirtd().restart()

def cleanup(self):
if self.params.get("setup_hugepages") == "yes":
h = test_setup.HugePageConfig(self.params)
h.cleanup()
if self.params.get("vm_type") == "libvirt":
utils_libvirtd.Libvirtd().restart()
env_process._post_hugepages_surp = h.ext_hugepages_surp