Multi packet size support - WIP#21756
Multi packet size support - WIP#21756ryleung-solana wants to merge 34 commits intosolana-labs:masterfrom
Conversation
steviez
force-pushed
the
multi_packet_size
branch
2 times, most recently
from
19268c0 to
0aa9e73
Compare
ryleung-solana
force-pushed
the
multi_packet_size
branch
from
e2e3168 to
94e6aaf
Compare
ryleung-solana
force-pushed
the
multi_packet_size
branch
from
e3930eb to
26991ac
Compare
jstarry
left a comment
jstarry
left a comment
There was a problem hiding this comment.
Looking good so far, anything in particular you want feedback on? I was thinking that there may be a way around using the packet interface type you added if you have each packet just contain a slice into some larger data buffer allocated for each batch.
| ) -> Result<Self> { | ||
| tx.sanitize()?; | ||
|
|
||
| let size = bincode::serialized_size(&tx).map_err(|_| TransactionError::SanitizeFailure)?; |
There was a problem hiding this comment.
it would be better to avoid this extra serialization if possible
There was a problem hiding this comment.
Got rid of one serialization in one case, though agreed that ideally this wouldn't be here at all.
| // TODO verify. It seems that if one were using this with a regular packet | ||
| // the smaller data field would imply the original PACKET_DATA_SIZE bound anyway. | ||
| // Is there any reason we would not want to use EXTENDED_PACKET_DATA_SIZE here to | ||
| // be able to work with ExtendPacket as well? |
| #[derive(Clone)] | ||
| #[repr(C)] | ||
| pub struct ExtendedPacket { | ||
| pub data: [u8; EXTENDED_PACKET_DATA_SIZE], |
There was a problem hiding this comment.
What if packet data was just a slice into a larger chunk of data stored in a "PacketBatch"? That way we don't need multiple types for packets
There was a problem hiding this comment.
Maybe; it's an interesting idea, and would also get rid of some of the other complexity we have with needing a separate port for the ExtendedPackets. Will take a look.
| ); | ||
| warn!("elem len: {}", elems.len() as u32); | ||
| warn!("packet sizeof: {}", size_of::<P>() as u32); | ||
| // todo: figure out what this means; the data field max size |
There was a problem hiding this comment.
what are you trying to figure out? PACKET_DATA_SIZE should be the same as size_of::<Packet>() so that's what the trace log was probably for
There was a problem hiding this comment.
I was mostly just wondering what this information this debug message was trying to convey, and if it had any importance now that we're modifying the max data size in the ExtendedPacket case. It was kind of confusing since the max data size doesn't actually seem to be used in figuring out any of the offsets. Not a huge deal, and will remove the TODO soon.
Nothing in particular, was just hoping to start getting some eyes on this. |
This PacketInterface trait is plumbed through all the way from socket recv() / send() code up to TPU/TVU stages. The existing Packet struct provides an implementation of PacketInterface called StandardPacket. For all cases where the "regular" sized packets are sufficient, we just provide the StandardPacket struct. For cases where we may desire larger packets, we keep the code "templated" with the PacketInterface trait and allow caller(s) to choose which packet they desire. Sigverify / GPU code in general will still need some work to support multiple sized packets, as this code uses some fixed offsets into Packet arrays to function. As of this commit, this branch builds with several warnings. Co-authored-by: Ryan Leung <ryan.leung@solana.com>
Also removed some stale comments
…an can fit into a normal Packet
ExtendedPacket sockets bind to these new ports and know to pull larger packets from the network. These sockets are populated in the Node struct, and then propagated throughout Tpu and the associated Tpu stages.
…rt and add forwarding support for ExtendedPacket
May not be necessary on a cluster, but definitely needed for testing with solana-test-validator. Also fixed an inversion error on function to say whether extended needed or not.
This is more clean in terms of less lines of code, and should be algorithmically more efficient than previous approach as we only need to scan each transaction once now instead of twice.
…warding, sigverify_stage and send_transaction_service. Setting the log level of these messages to warn for now to allow for less verbose logs. TODO: clean up (and possibly add configuration options/remove some of this as development progresses)
… the new ExtendedPacket path
ryleung-solana
force-pushed
the
multi_packet_size
branch
from
46305ef to
2ece71b
Compare
…ibly undesirable
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
stale
Bot
added
the
stale
3 participants
Problem
We need to support larger transaction sizes than fit in 1 MTU. This is a first step, aiming to implement support for sending larger-than-1-MTU packets down to the OS's networking stack and letting it handle frag/defrag. Here, we want to support larger packet sizes, but don't want to incur the extra memory/memory bandwidth costs when unnecessary, so we will abstract the Packet struct to allow for different sized packet structures as needed, as well as adding the relevant code to send/receive larger packets, and do sigverify on them.
Summary of Changes
WIP - adds abstraction of the Packet struct into the PacketInterface trait, and future changes to implement a larger packet struct and add support for listening on different ports for the larger packets and doing sigverify on them.
Fixes #