Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sendmmsg() in v0.26 do not support different content in control messages among messages #1954

Open
hunts opened this issue Dec 31, 2022 · 11 comments · May be fixed by #2038
Open

sendmmsg() in v0.26 do not support different content in control messages among messages #1954

hunts opened this issue Dec 31, 2022 · 11 comments · May be fixed by #2038

Comments

@hunts
Copy link

hunts commented Dec 31, 2022

For a UDP server program, it is very useful to use sendmmsg() and recvmmsg() to improve the performance by reducing syscalls.

And it is not a uncommon setup that a single socket can receive IP packets with difference destination addresses. In this type of setup, we obtain the real destination addresses of messages from the Ipv4PacketInfo/Ipv6PacketInfo control message when using recvmmsg(). Corresponding to that, Ipv4PacketInfo/Ipv6PacketInfo control messages are used to set source addresss when sending out multiple messages at once via sendmmsg().

In the breaking change introduced by 0.26 about sendmmsg() api (#1744), it seems it was assumed that control messages are shared/same across all the messages in the batch, I think this is a false assumption.

Did I read the code wrong? If so, is there any example of how to use the new API correctly in this use case.

@hunts hunts changed the title sendmmsg() in v0.26 do not support different content in control messages across messages sendmmsg() in v0.26 do not support different content in control messages among messages Dec 31, 2022
@hunts
Copy link
Author

hunts commented Jan 5, 2023

@pacak any idea on this. If we can confirm this is truly a problem. I can try to make a fix.

@pacak
Copy link
Contributor

pacak commented Jan 5, 2023

The comment says that control messages are shared and that's what the code does I'm afraid. I guess C could be an iterator of some sort too.

@hunts
Copy link
Author

hunts commented Jan 5, 2023

@pacak thanks for the confirmation. I'll try to make a fix to allow individual msg to have their own cmsg contents.

@asomers
Copy link
Member

asomers commented Feb 9, 2023

Could you please paste a sample of some code that worked with the old implementation but not with the new? Maybe we should revert that PR.

@pacak
Copy link
Contributor

pacak commented Feb 9, 2023

A working code sample would be great, I can change current code to support old behavior.

@vkrasnov
Copy link

I don't have a specific code in mind, but for example it is very useful to be able to send multiple messages with a different TxTime message.

@hunts
Copy link
Author

hunts commented Apr 19, 2023

Sorry guys, I didn't find time to work on this. For an working example, I cannot copy-paste my code, but this is the pseudo code showing the use case:

type Item = (bytes, peer_addr, local_addr);

let recv_queue: VecDeque<Item> = VecDeque::with_capacity(BUF_LEN);
let send_queue: VecDeque<Item> = VecDeque::with_capacity(BUF_LEN);

// Get per-msg local addr from CMSG of each msg.
let incoming: Vec<Item> = match recvmmsg(sock.as_raw_fd(), &mut data, msg_flags, None) { ... };

... push incoming items to the recv_queue
... processing in another thread

let responses = send_queue.iter().take(BATCH_LEN);

// Write per-msg local addr to a CMSG for each SendMmsgData
let send_data: Vec<SendMmsgData<IoSlice, ControlMessage, SocketaddrStorage>> = make_responses_into_send_data();

let results = match sendmmsg(sock, send_data, MsgFlags::empty()) { ... };

Basically, for each individual RecvMsg, we need to find its actual destination address from the packet info CMSG, and pass that info all the way to SendMmsgData where we need to set a packet info CMSG to indicate its actual source address.

@pacak
Copy link
Contributor

pacak commented Apr 19, 2023

Okay, I get the idea. Will try to make a change to support it too.

@nox
Copy link
Contributor

nox commented May 11, 2023

@pacak Any news on this? Can we help in some way?

@pacak
Copy link
Contributor

pacak commented May 11, 2023

No news yet, this change is next in the priority queue, will hopefully make some progress this weekend.

@pacak
Copy link
Contributor

pacak commented May 13, 2023

It was much smaller change than I expected originally, sorry for taking this much time.

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 a pull request may close this issue.

5 participants