WIP: a usable CQ completion channel #83

dragonJACson · 2025-11-30T04:23:07Z

gemini-code-assist · 2025-11-30T04:23:20Z

Summary of Changes

Hello @dragonJACson, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request lays the groundwork for a usable completion channel by integrating the ibv_get_cq_event mechanism. It establishes a robust way to associate Rust CompletionQueue objects with their corresponding C ibv_cq handles, allowing the system to retrieve the correct Rust object when a completion event is signaled. This is achieved through careful management of weak references and unsafe FFI interactions, ensuring proper resource management and preventing memory leaks or use-after-free issues.

Highlights

CQ Event Retrieval: Implemented the get_cq_event method for CompletionChannel, allowing the retrieval of GenericCompletionQueue instances when events occur.
Weak Reference Integration: Introduced WeakGenericCompletionQueue and modified CompletionQueueBuilder to store weak references to Rust CompletionQueue objects within the cq_context of the underlying ibv_cq structures. This enables safe retrieval of Rust objects from C callbacks.
Error Handling: Added new error types, GetCompletionEventError and GetCompletionEventErrorKind, to handle potential failures during completion event retrieval.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request introduces functionality to get completion queue events from a completion channel. This is achieved by storing a weak reference to the completion queue in its context field, which can be retrieved later. While the overall approach is sound, there are several critical issues that need to be addressed. Specifically, there are memory leaks due to improper handling of Box::into_raw, and a regression that breaks the existing setup_cq_context functionality. There is also a potential panic in get_cq_event that should be handled as an error. I've left detailed comments on these points.

gemini-code-assist · 2025-11-30T04:25:21Z

src/ibverbs/completion.rs

+            let weak_cq = Arc::downgrade(&cq_wrapper.clone());
+            let boxed = Box::new(WeakGenericCompletionQueue::Extended(weak_cq));
+            let raw_box = Box::into_raw(boxed);
+
+            unsafe {
+                (*cq_ex).cq_context = raw_box as *mut std::ffi::c_void;
+            }


This block of code introduces some issues:

Memory Leak (Critical): Box::into_raw is used to store a pointer to WeakGenericCompletionQueue in cq_context. This transfers ownership, but the Drop implementation for ExtendedCompletionQueue (lines 417-423) doesn't deallocate this memory, causing a leak.

API Regression (High): This change overwrites any user-provided context set via setup_cq_context, breaking that functionality.

Redundant Clone (Medium): The call to cq_wrapper.clone() on line 530 is not necessary. Arc::downgrade takes a reference, so Arc::downgrade(&cq_wrapper) is sufficient.

A potential solution for the first two points is to create a new struct that holds both the user's context and the WeakGenericCompletionQueue, and then store a pointer to an instance of this struct in cq_context. You would then need to manage its allocation and deallocation carefully.

gemini-code-assist · 2025-11-30T04:25:21Z

src/ibverbs/completion.rs

+            let weak_cq = Arc::downgrade(&cq_wrapper.clone());
+            let boxed = Box::new(WeakGenericCompletionQueue::Basic(weak_cq));
+            let raw_box = Box::into_raw(boxed);
+
+            unsafe {
+                (*cq).cq_context = raw_box as *mut std::ffi::c_void;
+            }


This block of code has issues similar to build_ex:

Memory Leak (Critical): Box::into_raw is used to store a pointer to WeakGenericCompletionQueue in cq_context. This transfers ownership, but the Drop implementation for BasicCompletionQueue (lines 335-340) doesn't deallocate this memory, causing a leak.

API Regression (High): This change overwrites any user-provided context set via setup_cq_context, breaking that functionality.

Redundant Clone (Medium): The call to cq_wrapper.clone() on line 567 is not necessary. Arc::downgrade takes a reference, so Arc::downgrade(&cq_wrapper) is sufficient.

A potential solution for the first two points is to create a new struct that holds both the user's context and the WeakGenericCompletionQueue, and then store a pointer to an instance of this struct in cq_context. You would then need to manage its allocation and deallocation carefully.

gemini-code-assist · 2025-11-30T04:25:21Z

src/ibverbs/completion.rs

+
+        let weak_cq = unsafe { (*cq_wrapper.as_ptr()).upgrade() };
+
+        Ok(weak_cq.unwrap())


The use of .unwrap() here will cause a panic if weak_cq.upgrade() returns None. This can happen if the CompletionQueue is dropped before get_cq_event is called. This should be handled by returning an error, which is consistent with the function's Result return type.

Suggested change

Ok(weak_cq.unwrap())

weak_cq.ok_or_else(|| GetCompletionEventErrorKind::Ibverbs(io::Error::new(io::ErrorKind::NotFound, "completion queue has been dropped")).into())

codecov · 2025-11-30T04:27:46Z

Codecov Report

❌ Patch coverage is 90.90909% with 5 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/ibverbs/completion.rs	90.90%	5 Missing ⚠️

Files with missing lines	Coverage Δ
src/ibverbs/completion.rs	`82.86% <90.90%> (+0.91%)`	⬆️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Signed-off-by: Luke Yue <lukedyue@gmail.com>

dragonJACson marked this pull request as draft November 30, 2025 04:23

dragonJACson self-assigned this Nov 30, 2025

dragonJACson added this to the Release v0.5.0 milestone Nov 30, 2025

gemini-code-assist bot reviewed Nov 30, 2025

View reviewed changes

WIP: a usable CQ completion channel

967871f

Signed-off-by: Luke Yue <lukedyue@gmail.com>

dragonJACson force-pushed the dev/cq-event branch from 4d939d2 to 967871f Compare November 30, 2025 07:39

feat(examples): adapt CQ events for rc_pingpong

a2fa883

Signed-off-by: Luke Yue <lukedyue@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

WIP: a usable CQ completion channel #83

WIP: a usable CQ completion channel #83

Uh oh!

dragonJACson commented Nov 30, 2025 •

edited

Loading

Uh oh!

gemini-code-assist bot commented Nov 30, 2025

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

gemini-code-assist bot Nov 30, 2025

Uh oh!

gemini-code-assist bot Nov 30, 2025

Uh oh!

gemini-code-assist bot Nov 30, 2025

Uh oh!

codecov bot commented Nov 30, 2025 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants


		let weak_cq = unsafe { (*cq_wrapper.as_ptr()).upgrade() };

		Ok(weak_cq.unwrap())

	Ok(weak_cq.unwrap())
	weak_cq.ok_or_else(\|\| GetCompletionEventErrorKind::Ibverbs(io::Error::new(io::ErrorKind::NotFound, "completion queue has been dropped")).into())

WIP: a usable CQ completion channel #83

Are you sure you want to change the base?

WIP: a usable CQ completion channel #83

Uh oh!

Conversation

dragonJACson commented Nov 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist bot commented Nov 30, 2025

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Nov 30, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Nov 30, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Nov 30, 2025

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Nov 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dragonJACson commented Nov 30, 2025 •

edited

Loading

codecov bot commented Nov 30, 2025 •

edited

Loading