Skip to content

Conversation

@quanwenli
Copy link
Contributor

@quanwenli quanwenli commented Nov 7, 2025

ID : 4678

Summary by CodeRabbit

  • Refactor
    • Internal thread-pinning logic updated to include VM context during test setup; improves handling in performance tests with no direct user-facing changes.

@coderabbitai
Copy link

coderabbitai bot commented Nov 7, 2025

Walkthrough

A helper function _pin_vm_threads in generic/tests/pktgen_perf.py was modified to accept a VM object as its first parameter. The signature changed from _pin_vm_threads(node) to _pin_vm_threads(vm, node) and its docstring was updated to document the new vm parameter. The call site in init_vm_and_login was updated to pass vm as the first argument: _pin_vm_threads(vm, params.get("numa_node")). The function body now invokes pin_vm_threads with the VM object and the existing NUMA node argument.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Verify the _pin_vm_threads implementation correctly uses the vm parameter when calling pin_vm_threads.
  • Confirm there are no other call sites for _pin_vm_threads that need updating.
  • Check the updated call site that vm is in scope and the params.get("numa_node") value remains appropriate.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: adding a vm parameter to the _pin_vm_threads function call.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b7d7e7d and 16b6325.

📒 Files selected for processing (1)
  • generic/tests/pktgen_perf.py (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • generic/tests/pktgen_perf.py

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
generic/tests/pktgen_perf.py (1)

48-57: Excellent bug fix! Update the docstring to document the new parameter.

This change fixes a critical scoping issue. Previously, _pin_vm_threads relied on closure to access vm from the outer run function scope, which would have been None (line 115) at the time of the call. Now it correctly receives the initialized VM object as an explicit parameter.

The docstring should be updated to document the new vm parameter.

Apply this diff to update the docstring:

     def _pin_vm_threads(vm, node):
         """
         pin guest vcpu and vhost threads to cpus of a numa node repectively
 
+        :param vm: the VM object to pin threads for
         :param node: which numa node to pin
         """
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 94d4cc8 and b7d7e7d.

📒 Files selected for processing (1)
  • generic/tests/pktgen_perf.py (2 hunks)
🔇 Additional comments (1)
generic/tests/pktgen_perf.py (1)

69-69: LGTM!

The call site correctly passes the VM object that was initialized at line 63, fixing the closure scoping issue.

@quanwenli
Copy link
Contributor Author

JOB LOG : /root/avocado/job-results/job-2025-11-07T03.44-575c0a8/job.log
(1/1) xl710.bridge_test.1q.acceptance.repeat1.Host_RHEL.m9.u2.qcow2.virtio_blk.up.virtio_net.Guest.RHEL.9.8.0.x86_64.io-github-autotest-qemu.pktgen_perf.q35: STARTED
(1/1) xl710.bridge_test.1q.acceptance.repeat1.Host_RHEL.m9.u2.qcow2.virtio_blk.up.virtio_net.Guest.RHEL.9.8.0.x86_64.io-github-autotest-qemu.pktgen_perf.q35: PASS (364.41 s)
RESULTS : PASS 1 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0
JOB HTML : /root/avocado/job-results/job-2025-11-07T03.44-575c0a8/results.html
JOB TIME : 365.97 s

@PaulYuuu could you help review this small fix ? thanks a lot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants