Skip to content

Conversation

@e-oak
Copy link
Contributor

@e-oak e-oak commented Aug 1, 2025

Creates versions of Send/Read/Write with an options argument. For now that field only controls send flags, we could use to add additional arguments (e.g. qp_type) to those methods later on without breaking existing code if needed.

@codecov-commenter
Copy link

codecov-commenter commented Aug 1, 2025

Codecov Report

❌ Patch coverage is 0% with 47 lines in your changes missing coverage. Please review.
✅ Project coverage is 6.3%. Comparing base (fa92aa3) to head (337f3be).

Files with missing lines Patch % Lines
ibverbs/src/lib.rs 0.0% 47 Missing ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
ibverbs/src/lib.rs 5.0% <0.0%> (-0.3%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@xmakro
Copy link
Collaborator

xmakro commented Aug 11, 2025

Thanks for the PR. Before merging, I'm wondering what other arguments could SendOptions be used for? I think qp_type is set when initializing the QueuePair instead. If send_flags is likely to be one of the only fields in SendOptions, then maybe it makes more sense to unpack the struct into function arguments

@e-oak
Copy link
Contributor Author

e-oak commented Aug 25, 2025

Sorry about the delay!

What about passing a linked list of work through next? I personally don't need that at the moment, so it smells of YAGNI, but if we just inline the field if someone needs that we need a new method, which seems pretty terrible (I even dislike having two send calls =/).

Your call tho, happy to inline if you feel strongly either way

@jeronimosg
Copy link

I would be more inclined to have/see a rustified version of a SendWR rather than having something new, keeping the API similar to the C one.

Something like struct SendWR { wr_id: u32, send_op: SendOpKind, send_flags: ffi::ibv_send_flags, }

But no support for next whatsoever tho...

@xmakro
Copy link
Collaborator

xmakro commented Sep 5, 2025

Introducing next would require adding wr_id, im_data, data to the SendOptions/SendWR struct. I'm not sure that next should be a field of SendWR, as a safe API would require Box with an allocation. Instead, it seems more natural for the API to accept impl IntoIterator<Item = SendWR>. For supporting next, it then seems better to right away introduce SendWR which holds all input fields, and have the current API redirect to this new API. WDYT?

@jeronimosg
Copy link

Having an iterator would be nice for ergonomics, and it works well for now.
Eventually though, we will need to look into adding next to issue multiple WRs in a single ibv_post_send, ideally without extra allocations or copies into C land

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.

4 participants