Change: api revisions for zero-alloc sendmmsg and recvmmsg

[cfzimmerman]

Change *mmsg to support memory reuse

In short, these changes adjust the generics on sendmmsg and recvmmsg so that both can be used in a loop without requiring new heap allocations from the caller on each iteration.

Motivation

First, thanks much to the maintainers of this library. It's very solid, and I use it heavily. However, with some frustration, I've been unable to find a way to use sendmmsg and recvmmsg in a loop without making any heap allocations. If someone knows how to with the current API, please just show me a compiling example and I'll close this PR. Otherwise, here's the issue as I understand it. I'll use sendmmsg for analysis, but both have the same issue:

Current:

pub fn sendmmsg<'a, XS, AS, C, I, S>(
..
    slices: XS,
..
) -> crate::Result<MultiResults<'a, S>>
    where
        XS: IntoIterator<Item = &'a I>,
..
        I: AsRef<[IoSlice<'a>]> + 'a,
..
{

Proposed:

pub fn sendmmsg<'a, XS, AS, C, I, S>(
..
    slices: XS,
..
) -> crate::Result<MultiResults<'a, S>>
    where
        XS: IntoIterator<Item = I>,
..
        I: AsRef<[IoSlice<'a>]> + 'a,
..
{

sendmmsg requires its buffers to be wrapped in an IoSlice. Because IoSlice takes the lifetime of the buffer it wraps, the underlying buffer cannot be mutated until the IoSlice is dropped. So, if the buffer allocation is reused between calls to sendmmsg, then the IoSlice needs to be dropped. If there's a non-constant number of IoSlices, then they need to be in an iterator or some dynamic collection.

Because I is borrowed in the signature, the IoSlice cannot be produced by an iterator because a reference cannot be returned to an IoSlice created within the same iterator. That basically mandates that theIoSlice instances are collected into a Vec before sendmmsg is called.

However, I don't think a reference is actually needed here. Making I an &'a AsRef<_> is redundant because we use AsRef to borrow the data anyway. My changes remove the borrow over I from sendmmsg and recvmmsg. This allows the IoSlice and IoSliceMut instances to be yielded from an iterator instead of a heap-allocated collection, which gives a meaningful performance boost to my code using these functions.

Also, I noticed someone else recently made an issue related to this. Perhaps this would satisfy them as well? #2457

Validation

I made a small repo to demo my use case and how these changes help. It can be found here: https://github.com/cfzimmerman/nix-alloc-demo/blob/main/src/lib.rs

The important part is this comparison in extension of the example above.

Using the current API:

    pub fn send(
        fd: i32,
        hdrs: &mut MultiHeaders<SockaddrStorage>,
        bufs: &[([u8; 32], Vec<u8>)],
        addrs: &[Option<SockaddrStorage>],
    ) -> anyhow::Result<usize> {
        let slices: Vec<_> = bufs
            .iter()
            .map(|(buf1, buf2)| [IoSlice::new(buf1), IoSlice::new(buf2)])
            .collect();
        Ok(sendmmsg(fd, hdrs, &slices, addrs, [], MsgFlags::empty())?.count())
    }

Using the proposed API:

    pub fn send(
        fd: i32,
        hdrs: &mut MultiHeaders<SockaddrStorage>,
        bufs: &[([u8; 32], Vec<u8>)],
        addrs: &[Option<SockaddrStorage>],
    ) -> anyhow::Result<usize> {
        let slices = bufs
            .iter()
            .map(|(buf1, buf2)| [IoSlice::new(buf1), IoSlice::new(buf2)]);
        Ok(sendmmsg(fd, hdrs, slices, addrs, [], MsgFlags::empty())?.count())
    }

I do not know if this is a breaking API change. From the limited examples I've tried, it doesn't seem to be. However, I can't guarantee that and would appreciate input from someone with more experience.

Also, I encountered a test that seems flaky, but maybe there are issues on my side. Both before and after my changes, the test test_recvmm2 failed for me occasionally. Is this expected? Here's a demo of failure before I made any changes. If this is a reason not to merge, please let me know and I'll try to debug first.

Thanks for considering these changes!

Checklist:

• I have read CONTRIBUTING.md
• I have written necessary tests and rustdoc comments
  • Is this desired/necessary for a change this small?
• A change log has been added if this PR modifies nix's API

[SteveLauC]

First, thanks much to the maintainers of this library. It's very solid, and I use it heavily.

Thanks for the kind words, though I would say the credit goes to asomers.

---

The issue described here does exist, it enforces user to heap-allocate the[IoSlice;2] sequence so that references to them can be created.

Thanks for that demo repo! BTW.

However, I don't think a reference is actually needed here. Making I an &'a AsRef<_> is redundant because we use AsRef to borrow the data anyway.

And I agree that the reference in the signature is unnecessary considering we already borrow the buffer through AsRef.

---

Also, I noticed someone else recently made an issue related to this. Perhaps this would satisfy them as well? #2457

#2457 is different from this, sendmsg(2) heap allocates every time for the cmsg buffer, that issue asks to allow users to pre-allocate the buffer so that we don't need to do it on every call.

---

I do not know if this is a breaking API change. From the limited examples I've tried, it doesn't seem to be.

Yeah, it depends on the usage. We will have breaking changes in the next release, so no need to worry about it.

---

Also, I encountered a test that seems flaky, but maybe there are issues on my side. Both before and after my changes, the test test_recvmm2 failed for me occasionally. Is this expected? Here's a demo of failure before I made any changes. If this is a reason not to merge, please let me know and I'll try to debug first.

Did you see this in our CI? I don't remember I have seen it before:<

[cfzimmerman]

Appreciate it! Sounds great.
No issues with CI, that was just locally (Linux 6.8.1, Nixos 23.11). I'm not too concerned about it, just wanted to leave a note in case it was important.

[cfzimmerman]

Hey! Really sorry, I didn't realize until after trying to use these changes and seeing unexpected output, but I think removing the borrow here might actually cause Undefined Behavior. My new understanding is below. I'm closing the PR to prevent merge. If what I have below is wrong, please let me know and I'll re-open it:

Anyway, here's the function signature AND the first few lines of sendmmsg:

pub fn sendmmsg<'a, XS, AS, C, I, S>(
    fd: RawFd,
    data: &'a mut MultiHeaders<S>,
    slices: XS,
    // one address per group of slices
    addrs: AS,
    // shared across all the messages
    cmsgs: C,
    flags: MsgFlags
) -> crate::Result<MultiResults<'a, S>>
    where
        XS: IntoIterator<Item = I>,
        AS: AsRef<[Option<S>]>,
        I: AsRef<[IoSlice<'a>]> + 'a,
        C: AsRef<[ControlMessage<'a>]> + 'a,
        S: SockaddrLike + 'a,
{
...
    for (i, ((slice, addr), mmsghdr)) in slices.into_iter().zip(addrs.as_ref()).zip(data.items.iter_mut()).enumerate() {
        let p = &mut mmsghdr.msg_hdr;
        p.msg_iov = slice.as_ref().as_ptr().cast_mut().cast();
        p.msg_iovlen = slice.as_ref().len() as _;

In my example from the initial summary, the iterator yields this: [IoSlice::new(buf1), IoSlice::new(buf2)]
That passes ownership of an array to sendmmsg. Then, a pointer to that array is put into the msg_hdr. This is fine, but the function returns an iterator. When the function exits, the owned array is dropped from the stack, but the msghdr.msg_iov field still points to that array. When the iterator tries to dereference the msg_iov, there's undefined behavior from dereferencing uninitialized stack memory.

I was frustrated that this function requires the IoSlices to be in a borrowed collection. However, I think it was written that way because the msghdr struct takes a pointer to an array of iovecs. A zero-alloc *mmsg scheme for this in Rust still feels possible, but I don't think it'll be nearly as trivial as I expected.

[SteveLauC]

Huh, thanks for catching it! That is indeed a dangling pointer, and since we are using raw pointer here, it wasn't caught by the borrow checker. And taking a closer look at the sendmmsg(2) interface, I guess this is probably the reason why data: &'a mut MultiHeaders<S> and XS: IntoIterator<Item = &'a I> both have lifetime 'a.

Thanks again for catching it before I merge the PR!