netkvm: Add a case for packet loss when slowly reusing memory buffers #4247

heywji · 2025-01-07T07:31:42Z

Under various conditions, when the host-to-device packet rate is high, we lose packets in QEMU due to a lack of guest-allocated buffers. Look also at virtio-win/kvm-guest-drivers-windows#1012

ID: 2405

Signed-off-by: wji [email protected]

heywji · 2025-01-08T07:51:09Z

depends on avocado-framework/avocado-vt#4045

heywji · 2025-01-14T06:09:46Z

@leidwang, call you to review this patch again. Thanks in advance.

Test result:

1-Host_RHEL.m10.u0.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.Win2019.x86_64.io-github-autotest-qemu.unattended_install.cdrom.extra_cdrom_ks.default_install.aio_threads.q35 Finshed 2025-01-14 00:31:42 2025-01-14 00:44:10 PASS 747.58115
2-Host_RHEL.m10.u0.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.Win2019.x86_64.io-github-autotest-qemu.netkvm_buffer_shortage.q35 Finshed 2025-01-14 00:44:10 2025-01-14 01:05:00 PASS 1250.003384
Summary: 
Finshed=2, PASS=2

leidwang · 2025-01-22T01:32:32Z

Hi @heywji Please remember to sign the commits, thanks.

heywji · 2025-01-22T02:23:17Z

The commits are signed now. Please help review it again when you are free. @leidwang

leidwang · 2025-02-12T01:42:07Z

Hi @heywji I will review this MR once avocado-vt avocado-framework/avocado-vt#4045 done, thanks.

heywji · 2025-02-18T07:07:28Z

Hi @leidwang, I have removed the "avocado-vt avocado-framework/avocado-vt#4045" related codes.

Could you help review it again since this patch can be passed by applying Houqi's patch first?

Thanks,
Wecom

leidwang · 2025-02-19T07:20:14Z

qemu/tests/cfg/netkvm_buffer_shortage.cfg

+    start_vm_vm2 = no
+    smp = 2
+    queues = ${smp}
+    vectors = 1024


Is it a must for this test?

Yes. This part comes from our product bug: "RHEL-24992 BSOD occurs when the vector=1024 is set"., we need to set vectors = 1024.

Okay, thanks.

leidwang · 2025-02-19T07:29:35Z

qemu/tests/netkvm_buffer_shortage.py

+    :param env: Dictionary of test environment details.
+    """
+
+    def analyze_ping_results(session, count, timeout):


Suggested change

def analyze_ping_results(session, count, timeout):

def analyze_ping_results(dest, session, count, timeout):

leidwang · 2025-02-19T07:31:17Z

qemu/tests/netkvm_buffer_shortage.py

+        """
+
+        status, output = utils_net.ping(
+            s_vm_ip, session=c_session, count=count, timeout=timeout


Suggested change

s_vm_ip, session=c_session, count=count, timeout=timeout

dest, session=session, count=count, timeout=timeout

leidwang · 2025-02-19T07:32:16Z

qemu/tests/netkvm_buffer_shortage.py

+        pattern = r"(\d+)% loss"
+        match = re.search(pattern, output)
+        if match:


Suggested change

pattern = r"(\d+)% loss"

match = re.search(pattern, output)

if match:

if re.search(r"(\d+)% loss", output):

leidwang · 2025-02-19T07:37:04Z

qemu/tests/netkvm_buffer_shortage.py

+        param vm: the selected vm
+        param netkvmco_name: the netkvm driver parameter to modify
+        param value: the value to set to


Please remember to keep a same format for the docstring.

Thanks, @leidwang, but what's the same format meaning?

Got it, thanks.

leidwang · 2025-02-19T07:40:27Z

qemu/tests/netkvm_buffer_shortage.py

+        if match:
+            return match.group(1)
+
+    def modify_and_analyze_params_result(vm, netkvmco_name, value):


Suggested change

def modify_and_analyze_params_result(vm, netkvmco_name, value):

def modify_and_analyze_params_result(vm, param_name, value):

I guess param_name should be more clear to describe it's meaning.Because it's a netkvm parameter's name instead netkvmco name.

leidwang · 2025-02-19T07:43:18Z

qemu/tests/netkvm_buffer_shortage.py

+        utils_net.set_netkvm_param_value(vm, netkvmco_name, value)
+        cur_value = utils_net.get_netkvm_param_value(vm, netkvmco_name)
+        if cur_value != value:
+            test.fail(f"Current value '{cur_value}' was not equires '{value}'")


Suggested change

test.fail(f"Current value '{cur_value}' was not equires '{value}'")

test.fail(f"Failed to set '{param_name}' to '{value}'")

Thank you, Leidong. I agree with you, and this explanation is better than mine.

leidwang · 2025-02-19T07:44:22Z

qemu/tests/netkvm_buffer_shortage.py

+        param session:  session to execute commands on the target machine.
+        port: the port number to monitor.
+        script_to_run:  the path to the Python script to execute.


Same as the docstring formatting issue above.

leidwang · 2025-02-19T07:50:22Z

qemu/tests/netkvm_buffer_shortage.py

+        c_pip_copy_cmd = params.get("c_pip_copy_cmd")
+        c_pip_cmd = params.get("c_pip_cmd")
+        c_py_copy_cmd = params.get("c_py_copy_cmd")
+        s_py_copy_cmd = params.get("s_py_copy_cmd")
+        status, output = session.cmd_status_output(check_live_python, timeout=1200)
+        if status == 0:
+            return
+        session.cmd(dest_location)
+        if "server" in script_to_run:
+            error_context.context(
+                "Run python3 code runs on the server node", test.log.info
+            )
+            s_py_copy_cmd = utils_misc.set_winutils_letter(session, s_py_copy_cmd)
+            session.cmd(s_py_copy_cmd)


I think we can move the script and whl file copy part to run function or write a function for it, it's a test env preparation step.I do not think we need to copy them every time when we want to run server.py or client.py.

leidwang · 2025-02-19T07:52:00Z

qemu/tests/netkvm_buffer_shortage.py

+    for value in param_values.split(" "):
+        modify_and_analyze_params_result(vm=c_vm, netkvmco_name=param_name, value=value)


Would you please clarify why we need to modify the parameter's value before the end?Thanks.

Hi @leidwang,
This step comes from our product bug: "RHEL-24992 BSOD occurs when the vector=1024 is set". So here, I want to simulate the operation on the client side, open the NIC properties, and modify the MinRxBufferPercent parameter.

Okay, I am just curious why we modify the parameter's value but do not perform any testing.

Emm. you gave me some inspiration about this operation. I need to add a ping function here to check whether BSOD happens.

leidwang · 2025-02-19T08:41:42Z

qemu/tests/netkvm_buffer_shortage.py

+        """
+        conduct a ping test to check the packet loss on slow memory buffer reallocation
+
+        :param session: Local executon hint or session to execute the ping command.


Suggested change

:param session: Local executon hint or session to execute the ping command.

:param session: Local execution hint or session to execute the ping command.

leidwang · 2025-02-19T08:42:06Z

qemu/tests/netkvm_buffer_shortage.py

+        param vm: the selected vm
+        param netkvmco_name: the netkvm driver parameter to modify
+        param value: the value to set to


heywji · 2025-03-07T08:37:49Z

@leidwang Hi Leidong, Could you help review this patch again?

Test result: PASS

 (1/1) Host_RHEL.m9.u5.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.Win10.x86_64.io-github-autotest-qemu.netkvm_buffer_shortage.q35: PASS (1486.81 s)

Thanks!

leidwang · 2025-03-12T01:44:58Z

Hi @heywji Please sign the commit, thanks.

heywji · 2025-03-13T04:53:10Z

@leidwang Done. Please help review other problems. Thank you!

leidwang · 2025-03-25T02:28:24Z

qemu/tests/netkvm_buffer_shortage.py

+        if cur_value != value:
+            test.fail(f"Failed to set '{param_name}' to '{value}'")
+
+    def check_and_restart_port(session, port, script_to_run):


port is not called in this function

Suggested change

def check_and_restart_port(session, port, script_to_run):

def check_and_restart_port(session, script_to_run):

Modified. Thank leidong for reviewing me.

leidwang · 2025-03-25T02:30:39Z

qemu/tests/cfg/netkvm_buffer_shortage.cfg

+    c_pip_copy_cmd = 'xcopy "WIN_UTILS:\packet_loss_scripts\${psutil_whl}" ${copy_dest}'
+    c_pip_cmd = "py -m pip install ${psutil_whl}"
+    s_py_copy_cmd = 'xcopy "WIN_UTILS:\packet_loss_scripts\${s_py}" ${copy_dest}'
+    s_py_cmd = "start cmd /c py ${s_py} ${port_num}"
+    c_py_copy_cmd = 'xcopy "WIN_UTILS:\packet_loss_scripts\${c_py}" ${copy_dest}'
+    c_py_cmd = "start cmd /c py ${c_py} 99999 %s ${port_num}"


Suggested change

c_pip_copy_cmd = 'xcopy "WIN_UTILS:\packet_loss_scripts\${psutil_whl}" ${copy_dest}'

c_pip_cmd = "py -m pip install ${psutil_whl}"

s_py_copy_cmd = 'xcopy "WIN_UTILS:\packet_loss_scripts\${s_py}" ${copy_dest}'

s_py_cmd = "start cmd /c py ${s_py} ${port_num}"

c_py_copy_cmd = 'xcopy "WIN_UTILS:\packet_loss_scripts\${c_py}" ${copy_dest}'

c_py_cmd = "start cmd /c py ${c_py} 99999 %s ${port_num}"

copy_file_cmd = 'xcopy "WIN_UTILS:\packet_loss_scripts\%s" ${copy_dest}'

install_psutil_cmd = "py -m pip install ${psutil_whl}"

server_cmd = "start cmd /c py ${s_py} ${port_num}"

client_cmd = "start cmd /c py ${c_py} 99999 %s ${port_num}"

We can try to merge some similar code.

It's a bit hard. Let me do more tries.

Maybe we can copy the files together since they are small.

Thank you, Leidong. I am running the test results now and will let you and Qianqian review them together.

Under various conditions, when the host-to-device packet rate is high, we lose packets in QEMU due to a lack of guest-allocated buffers. Look also at virtio-win/kvm-guest-drivers-windows#1012 Signed-off-by: wji <[email protected]>

heywji · 2025-04-02T09:54:59Z

@leidwang @vivianQizhu Could you help me review this patch?

Test Result: PASS

(1/2) Host_RHEL.m9.u5.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.Win10.x86_64.io-github-autotest-qemu.unattended_install.cdrom.extra_cdrom_ks.default_install.aio_threads.q35: STARTED 
(1/2) Host_RHEL.m9.u5.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.Win10.x86_64.io-github-autotest-qemu.unattended_install.cdrom.extra_cdrom_ks.default_install.aio_threads.q35: PASS (1496.10 s) 
(2/2) Host_RHEL.m9.u5.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.Win10.x86_64.io-github-autotest-qemu.netkvm_buffer_shortage.q35: STARTED 
(2/2) Host_RHEL.m9.u5.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.Win10.x86_64.io-github-autotest-qemu.netkvm_buffer_shortage.q35: PASS (1479.14 s)

leidwang · 2025-04-25T07:33:11Z

qemu/tests/cfg/netkvm_buffer_shortage.cfg

+    x86_64:
+        psutil_whl = "psutil-6.1.1-cp37-abi3-win_amd64.whl"
+    pip_cmd = "py -m pip install ${psutil_whl}"
+    dest_location = "pushd ${copy_dest}"


Can we use the absolute path directly so that we don't have to switch directories again and again?

I tried, but the Python script under winutils reports some errors. This script was provided to us by the upstream reporter, and it will take extra time to modify it.

Okay, if so, that's fine with me.

heywji changed the title ~~Add a case for packet loss when slowly reusing memory buffers~~ netkvm: Add a case for packet loss when slowly reusing memory buffers Jan 7, 2025

heywji force-pushed the KVMAUTOMA-2405 branch 3 times, most recently from d1b7c9e to 0b9815b Compare January 14, 2025 05:27

heywji force-pushed the KVMAUTOMA-2405 branch 2 times, most recently from d410739 to b2fea54 Compare January 22, 2025 02:15

heywji mentioned this pull request Feb 14, 2025

QEMU_VM: Add nic_extra_param_types support avocado-framework/avocado-vt#4045

Closed

heywji force-pushed the KVMAUTOMA-2405 branch from b2fea54 to e92e461 Compare February 18, 2025 06:54

leidwang reviewed Feb 19, 2025

View reviewed changes

heywji force-pushed the KVMAUTOMA-2405 branch 2 times, most recently from f52c96a to 8d9d3db Compare March 7, 2025 07:59

heywji force-pushed the KVMAUTOMA-2405 branch from 8d9d3db to 3640bfa Compare March 13, 2025 04:50

leidwang reviewed Mar 25, 2025

View reviewed changes

heywji force-pushed the KVMAUTOMA-2405 branch 5 times, most recently from c08ee32 to 5ae0e1e Compare April 1, 2025 06:55

heywji force-pushed the KVMAUTOMA-2405 branch from 5ae0e1e to d04ae05 Compare April 1, 2025 10:10

leidwang reviewed Apr 25, 2025

View reviewed changes

leidwang approved these changes Apr 25, 2025

View reviewed changes

	def analyze_ping_results(session, count, timeout):
	def analyze_ping_results(dest, session, count, timeout):

	s_vm_ip, session=c_session, count=count, timeout=timeout
	dest, session=session, count=count, timeout=timeout

	def modify_and_analyze_params_result(vm, netkvmco_name, value):
	def modify_and_analyze_params_result(vm, param_name, value):

	test.fail(f"Current value '{cur_value}' was not equires '{value}'")
	test.fail(f"Failed to set '{param_name}' to '{value}'")

		for value in param_values.split(" "):
		modify_and_analyze_params_result(vm=c_vm, netkvmco_name=param_name, value=value)

	:param session: Local executon hint or session to execute the ping command.
	:param session: Local execution hint or session to execute the ping command.

	def check_and_restart_port(session, port, script_to_run):
	def check_and_restart_port(session, script_to_run):

netkvm: Add a case for packet loss when slowly reusing memory buffers #4247

Are you sure you want to change the base?

netkvm: Add a case for packet loss when slowly reusing memory buffers #4247

Conversation

heywji commented Jan 7, 2025

heywji commented Jan 8, 2025

heywji commented Jan 14, 2025

leidwang commented Jan 22, 2025

heywji commented Jan 22, 2025

leidwang commented Feb 12, 2025

heywji commented Feb 18, 2025

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

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

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

heywji commented Mar 7, 2025

leidwang commented Mar 12, 2025

heywji commented Mar 13, 2025

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

heywji commented Apr 2, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment