thread-context: support new object thread-context #3726

mcasquer · 2023-07-24T07:51:35Z

thread-context: support new object thread-context

Includes support in avocado-vt for a new object:
thread-context. The intention of this new object
is to be used to make NUMA aware of the memory
preallocation threads.

ID: 2224513
Signed-off-by: mcasquer [email protected]

mcasquer · 2023-07-24T07:53:20Z

virttest/qemu_vm.py

@@ -1252,6 +1252,13 @@ def set_value(opt_string, key, fallback=None):

            return secret_cmdline + " -spice %s" % (",".join(spice_opts))

+        def add_thread_context(devices, params):
+            devs = []
+            dev = devices.thread_context_define_by_params(params, "tc1")


Here the ID has been hardcoded, how should I proceed? Thanks!

To make it configurable usually we would introduce a param to let user specify the id of the logical device(s), see the example below. And such a param is also in charge of pointing out the counts of devices, usually, from 0 up to N.

Looks we could introduce a new one for the thread context objects, and I'd suggest using vm_thread_contexts or vm_thread_context_objects as the name.

(btw, can we add multiple thread context objects to a guest?)

avocado-vt/virttest/qemu_vm.py

Line 2233 in fb39a11

for group in params.objects("throttle_groups"):

throttle_groups = tg1 tg2 tg3

Updated now, thanks!

(btw, can we add multiple thread context objects to a guest?)

@luckyh actually it's supported by QEMU, but let me confirm if it is feasible/makes sense

mcasquer · 2023-07-24T08:01:09Z

A 'draft' debug.log has been attached to the corresponding bug

mcasquer · 2023-07-24T08:01:18Z

@luckyh @PaulYuuu @YongxueHong could you review this PR? Thanks!

virttest/qemu_devices/qcontainer.py

YongxueHong · 2023-08-08T02:10:09Z

virttest/qemu_devices/qdevices.py

+    def __init__(self, backend, params=None):
+        super(ThreadContext, self).__init__(backend, params)


Suggested change

def __init__(self, backend, params=None):

super(ThreadContext, self).__init__(backend, params)

def __init__(self, params=None):

super(ThreadContext, self).__init__("thread-context", params)

We could specify the backend as "thread-context" if we use the new subclass to represent the thread-context object.

@YongxueHong updated now!

YongxueHong · 2023-08-08T02:20:03Z

virttest/qemu_devices/qdevices.py

+
+    def verify_hotplug(self, monitor):
+        """Verify if it is plugged into VM."""
+        return self._query(monitor)


I didn't see the definition of the private method _query, just see it is implemented by class QIOThread(QObject) and QThrottleGroup(QObject). Please correct me if I was wrong.

@YongxueHong updated now!

virttest/qemu_devices/qcontainer.py

PaulYuuu · 2023-08-15T02:08:55Z

virttest/qemu_vm.py

+                thread_context_params = params.object_params(thread_context)
+                dev = devices.thread_context_define_by_params(thread_context_params, thread_context)
+                set_cmdline_format_by_cfg(dev, self._get_cmdline_format_cfg(),
+                                          "vm_thread_contexts")


I think renaming the type name to thread_context(s) would be better.
_get_cmdline_format_cfg is in the qemu command context, so the vm_ prefix is unnecessary.

Also, the parameter names are not uniform. I think dev type defines the format of a kind of device, so it doesn't need to be represented in the plural(like thread_contexts, nics).

Updated now, thanks!

mcasquer · 2023-08-16T09:48:22Z

@PaulYuuu @YongxueHong could you review this PR again? Thanks!

mcasquer · 2023-08-21T09:49:20Z

Kindly reminder, @PaulYuuu @YongxueHong could you review again this PR? Thanks!

YongxueHong · 2023-08-22T08:19:41Z

virttest/qemu_vm.py

+                thread_context_params = params.object_params(thread_context)
+                dev = devices.thread_context_define_by_params(thread_context_params, thread_context)
+                set_cmdline_format_by_cfg(dev, self._get_cmdline_format_cfg(),
+                                          "thread_context")


Suggested change

"thread_context")

"thread_contexts")

I think it should be plural to correspond to the vm_thread_contexts
CC @nickzhq

Updated now, thanks!

Disagree, here the name is device type, a type should not be plural.

Disagree, here the name is device type, a type should not be plural.

I remember that it is not just related to a device type but relates to a vt object, like, images, monitors, mem_devs, and so on.
Here we set the cmd line format base on the object vm_thread_contexts which is defined by vt.

Hi @nickzhq
Please correct me if I was wrong since you developed this part.
Thanks.

vm_thread_contexts

Hello @YongxueHong ,
in the beginning of vt json format design, the vt object is preferred instead device type. Among images, monitors, mem_devs are the concepts of vt framework.
For my opinion, use the object which is defined by vt.

Thanks!
cc @PaulYuuu @mcasquer

vm_thread_contexts

Hello @YongxueHong , in the beginning of vt json format design, the vt object is preferred instead device type. Among images, monitors, mem_devs are the concepts of vt framework. For my opinion, use the object which is defined by vt.

Thanks! cc @PaulYuuu @mcasquer

OK, if it is VT type, then this is good to me, but we still have a mix in the current VT code, like tpm vs tpms

avocado-vt/virttest/qemu_vm.py

Lines 2670 to 2677 in 7c6bf78

# Add TPM devices

for tpm in params.objects("tpms"):

tpm_params = params.object_params(tpm)

devs = devices.tpm_define_by_params("%s_%s" % (self.name, tpm),

tpm_params)

for dev in devs:

set_cmdline_format_by_cfg(dev, self._get_cmdline_format_cfg(),

"tpm")

YongxueHong · 2023-08-22T08:32:47Z

virttest/qemu_devices/qcontainer.py

+        prefix = 'vm_thread_context_'
+        for key in list(params.keys()):
+            if key.startswith(prefix):
+                new_key = key.rsplit(prefix)[1]
+                tc_params[new_key] = params[key]


Hi @mcasquer
I did not get the point of this part, why do we still need to filter the parameters by the prefix vm_thread_context_ to get the related key values?
And how do we handle those params which are without the prefix vm_thread_context_? ignore them?
Would you like to provide an example cfg?

IMO, I think that the argument params of thread_context_define_by_params already represents the related parameters of the thread-context object, so we don't filter them again.
like the implementation throttle_groups

avocado-vt/virttest/qemu_vm.py

Lines 2237 to 2243 in ac53d1f

# Add object throttle group

for group in params.objects("throttle_groups"):

group_params = params.object_params(group)

dev = devices.throttle_group_define_by_params(group_params, group)

set_cmdline_format_by_cfg(dev, self._get_cmdline_format_cfg(),

"images")

devices.insert(dev)

avocado-vt/virttest/qemu_devices/qcontainer.py

Lines 2903 to 2906 in ac53d1f

def throttle_group_define_by_params(self, group_params, name):

props = json.loads(group_params.get("throttle_group_parameters", "{}"))

return QThrottleGroup(name, props)

Please correct me if I misunderstood, Thanks.

@YongxueHong
If I skip this filtering, then all the params will compose the qemu-kvm cmd line, maybe this is related with the cfg file, what do you think?
e.g.

[stdlog] -rf %s,indirect_image_blacklist=/dev/hda[\d]* /dev/sda[\d]* /dev/sg0 /dev/md0,inactivity_treshold=3600,inactivity_watcher=log,vfd_size=1440k,install_virtio=no,enable_libvirtd_debug_log=yes,libvirtd_debug_level=2,libvirtd_log_cleanup=yes,enable_split_libvirtd_feature=no,enable_host_sosreport=no,enable_remote_host_sosreport=no,rpmbuild_path=/root/rpmbuild/,sysprep_required=no,sysprep_options=--operations machine-id,gcov_qemu=no,gcov_qemu_reset=yes,gcov_qemu_collect_cmd_opts=--html,gcov_qemu_compress=no,stress_install_from_repo=no,stress_args=--cpu 4 \ [stdlog] --io 4 \ [stdlog] --vm 2 \ [stdlog] --vm-bytes 256M,download_url_stress=stress/stress-1.0.4.tar.gz,enable_guest_sosreport=no,gluster_managed_by_test=yes,vm_monitor_exit_status=no,kickstart_reboot_bug=no,force_reset_go_down_check=qmp,qemu_stop=on,qemu_preconfig=off,vm_type=qemu,kvm_ver_cmd=uname \ [stdlog] -r,kvm_userspace_ver_cmd=rpm \ [stdlog] -qa | grep \ [stdlog] -E 'qemu-kvm(-(rhev|ma|core))?-[0-9]+\.' | head \

Hi @mcasquer
I misunderstand, you are right.

YongxueHong · 2023-08-23T01:15:20Z

virttest/qemu_devices/qcontainer.py

+        """
+        tc_params = Params()
+        prefix = 'vm_thread_context_'
+        for key in list(params.keys()):


Suggested change

for key in list(params.keys()):

for key in params:

Looks like we could call the magic method __iter__ of the params object(class Params(IterableUserDict)).

Updated now, thanks!

Includes support in avocado-vt for a new object: thread-context. The intention of this new object is to be used to make NUMA aware of the memory preallocation threads. Signed-off-by: mcasquer <[email protected]>

mcasquer · 2023-08-24T06:32:34Z

@YongxueHong could you review again this PR? Thanks!

mcasquer · 2023-08-28T06:19:57Z

@YongxueHong this is a kindly reminder, could you review again this PR? Thanks!

YongxueHong

LGTM.

YongxueHong · 2023-08-28T07:48:10Z

Thanks all,let's merge it.

mcasquer commented Jul 24, 2023

View reviewed changes

luckyh reviewed Jul 28, 2023

View reviewed changes

virttest/qemu_devices/qcontainer.py Outdated Show resolved Hide resolved

luckyh reviewed Jul 28, 2023

View reviewed changes

virttest/qemu_devices/qcontainer.py Outdated Show resolved Hide resolved

mcasquer force-pushed the 2224513_thread_context branch 2 times, most recently from fcc326f to 4fbc6f3 Compare July 28, 2023 08:13

mcasquer marked this pull request as ready for review July 28, 2023 09:35

YongxueHong reviewed Aug 8, 2023

View reviewed changes

mcasquer force-pushed the 2224513_thread_context branch from 4fbc6f3 to 98bbe79 Compare August 11, 2023 09:07

PaulYuuu reviewed Aug 15, 2023

View reviewed changes

mcasquer force-pushed the 2224513_thread_context branch from 98bbe79 to 357f800 Compare August 16, 2023 09:12

PaulYuuu approved these changes Aug 22, 2023

View reviewed changes

YongxueHong reviewed Aug 22, 2023

View reviewed changes

mcasquer force-pushed the 2224513_thread_context branch from 357f800 to 51aabf8 Compare August 22, 2023 12:32

YongxueHong reviewed Aug 23, 2023

View reviewed changes

thread-context: support new object thread-context

5de3df8

Includes support in avocado-vt for a new object: thread-context. The intention of this new object is to be used to make NUMA aware of the memory preallocation threads. Signed-off-by: mcasquer <[email protected]>

mcasquer force-pushed the 2224513_thread_context branch from 51aabf8 to 5de3df8 Compare August 23, 2023 08:34

YongxueHong approved these changes Aug 28, 2023

View reviewed changes

PaulYuuu approved these changes Aug 28, 2023

View reviewed changes

luckyh approved these changes Aug 28, 2023

View reviewed changes

YongxueHong merged commit 1f33d7f into avocado-framework:master Aug 28, 2023
50 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

thread-context: support new object thread-context #3726

thread-context: support new object thread-context #3726

mcasquer commented Jul 24, 2023

mcasquer Jul 24, 2023

luckyh Jul 27, 2023

mcasquer Jul 28, 2023

mcasquer Jul 28, 2023

mcasquer commented Jul 24, 2023

mcasquer commented Jul 24, 2023

YongxueHong Aug 8, 2023

mcasquer Aug 11, 2023

YongxueHong Aug 8, 2023

mcasquer Aug 11, 2023

PaulYuuu Aug 15, 2023

mcasquer Aug 16, 2023

mcasquer commented Aug 16, 2023

mcasquer commented Aug 21, 2023

YongxueHong Aug 22, 2023

mcasquer Aug 22, 2023

PaulYuuu Aug 23, 2023

YongxueHong Aug 23, 2023

nickzhq Aug 24, 2023 •

edited

Loading

PaulYuuu Aug 28, 2023

YongxueHong Aug 22, 2023

mcasquer Aug 22, 2023

YongxueHong Aug 23, 2023

YongxueHong Aug 23, 2023

mcasquer Aug 23, 2023

mcasquer commented Aug 24, 2023

mcasquer commented Aug 28, 2023

YongxueHong left a comment

YongxueHong commented Aug 28, 2023

		def __init__(self, backend, params=None):
		super(ThreadContext, self).__init__(backend, params)

	# Add TPM devices
	for tpm in params.objects("tpms"):
	tpm_params = params.object_params(tpm)
	devs = devices.tpm_define_by_params("%s_%s" % (self.name, tpm),
	tpm_params)
	for dev in devs:
	set_cmdline_format_by_cfg(dev, self._get_cmdline_format_cfg(),
	"tpm")

	# Add object throttle group
	for group in params.objects("throttle_groups"):
	group_params = params.object_params(group)
	dev = devices.throttle_group_define_by_params(group_params, group)
	set_cmdline_format_by_cfg(dev, self._get_cmdline_format_cfg(),
	"images")
	devices.insert(dev)

	def throttle_group_define_by_params(self, group_params, name):
	props = json.loads(group_params.get("throttle_group_parameters", "{}"))
	return QThrottleGroup(name, props)

thread-context: support new object thread-context #3726

thread-context: support new object thread-context #3726

Conversation

mcasquer commented Jul 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcasquer commented Jul 24, 2023

mcasquer commented Jul 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcasquer commented Aug 16, 2023

mcasquer commented Aug 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickzhq Aug 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcasquer commented Aug 24, 2023

mcasquer commented Aug 28, 2023

YongxueHong left a comment

Choose a reason for hiding this comment

YongxueHong commented Aug 28, 2023

nickzhq Aug 24, 2023 •

edited

Loading