update socket config and create builder pattern #3929

gregcusack · 2024-12-05T00:12:42Z

Problem

We allocate large send and receive buffers for all sockets. However, not all sockets read and write equally from/to their buffers. In fact, 6 sockets are primarily Read and 2 are primarily Write. Meaning we are allocating memory that is never used.

Summary of Changes

preparation for reducing socket buffer sizes. This PR leaves socket buffer sizes as is.

Update SocketConfig to use builder pattern
Deprecate unused net-utils methods that use default SocketConfig

Services/Sockets
Read/Write

Gossip
RPC - TCP
Ip_echo - TCP
Tvu
tvu_quic
Repair
Repair_quic
Serve_repair
Serve_repair_quic
Ancestor_hashes_requests_quic
Ancestor_hashes_requests

Primarily Read

Tpu
Tpu_forwards
Tpu_vote
Tpu_quic
Tpu_forwards_quic
Tpu_vote_quic

Primarily Write

Retransmitter
Broadcast

Follow Up PR:

get rid of target gating
Reduce size of unused side of socket
Make better sizing decisions for the sockets that don't need a full 128MB buffer

gossip/src/cluster_info.rs

t-nelson · 2024-12-06T03:12:42Z

net-utils/src/lib.rs

 }

+#[cfg(not(any(windows, target_os = "ios")))]
 impl Default for SocketConfig {


pretty sure this was my doing, but i don't really think a "default" socket config makes sense. default makes it too easy for people to instantiate a thing without thinking and in the case of these sockets, i think we want people to think about the config they're specifying

is the idea that we only expose a SocketConfig constructor that takes in: reuseport, send_buffer_size, and receive_buffer_size? and the user would have to explicitly set these?

I worry people would set these incorrectly. and having some default at least falls back on the original settings (using the equivalent buffer sizes previously set using sysctl)

if we don't have a default socket config, should we also not have DEFAULT_RECV_BUFFER_SIZE and DEFAULT_SEND_BUFFER_SIZE? just worried people are going to complain that they have to set these manually. And if they have to set them manually, they may just pick some large number (> 128MB) that ends up defeating the whole point of these PRs.

Also, the kernel doubles the buffer sizes passed in via setsockopt. So someone trying to set a buffer size of 128MB is unknowingly allocating 256MB to their sockets per side.

We could force users to set buffer sizes within some range. Minimum ~4MB and max 128MB? That way we limit misconfigurations to some extent

I kind of agree with Greg, if we're going to have default constants for fields, then I don't think it is too much of a stretch to have a default for the struct that contains those fields.

An alternative could be instead of Default implementation, we could have default_ro, default_wo and default_rw that

ya we could have these default_ro, default_wo, default_rw. and then can have an option to set reuseport. we can also have the scaffold for setting buffer sizes directly like:

// set read buffer to 16MB and reuseport to true let socket = SocketConfig::default_ro().recv_buffer_size(16 * 1024 * 1024).reuseport(true);

For this PR we are not changing any of the actual buffer sizes. So, I propose for this PR, we provide default_wr that will set buffer sizes to the current 128MB. We leave out default_ro() and default_wo() on this PR since these defaults don't really make sense if we're not changing the buffer sizes on this PR

And then we at least have the functionality for the next PR where we can add default_ro() and default_wo() and set the buffer sizes

does that sound good?

Works for me; thoughts @t-nelson ?

more defaults are the wrong direction IMO. how about this concession..

#[derive(Default)] pub struct SocketConfig { reuseport: bool, recv_buffer_size: Option<usize>, send_buffer_size: Option<usize>, }

then when we build the socket, only even call setsockopt() when the field is Some(size)

in this case we just leave the socket buffer size set at whatever the default OS buffer size is? at the end of the day, someone is setting a default. it's either us or the OS. I'm ok with this idea tho

yeah we have little control over what the os does. odds are we shouldn't have custom "default" in the first place. the value from the tuning guide was just cribbed out of some blog post and tossed in iirc. no rigor to it. we should be tuning the socket buffers intelligently or not allocating sockets at all

how about this concession..

Works for me

net-utils/src/lib.rs

steviez

Think we're getting close; just one question and a couple nits

steviez · 2024-12-13T16:24:54Z

gossip/src/cluster_info.rs

+        let tpu_vote_quic_config = SocketConfig::default().reuseport(true);
+        let (tpu_vote_port, tpu_vote) =
+            Self::bind_with_config(bind_ip_addr, port_range, tpu_vote_udp_config.clone());
+        // using udp port for quic really because we need to reusport set to false, since Self::bind() defaults to false


Suggested change

// using udp port for quic really because we need to reusport set to false, since Self::bind() defaults to false

// using udp port for quic really because we need to set reusport to false, since Self::bind() defaults to false

nit: grammar

Also, can provide a little more detail on this piece; seems a little tricky

honestly, no idea why i wrote this. doesn't really make sense. but happy to get some more details on this

steviez · 2024-12-13T16:28:30Z

gossip/src/cluster_info.rs

-        let (_, broadcast) = Self::bind(bind_ip_addr, port_range);
-        let (_, ancestor_hashes_requests) = Self::bind(bind_ip_addr, port_range);
-        let (_, ancestor_hashes_requests_quic) = Self::bind(bind_ip_addr, port_range);
+        let write_only_socket_config = SocketConfig::default();


The change to wo is a change in behavior right in this PR right ?

ya i forgot to change this back. i am going to change this. and set as just socket_config and also want to avoid using "read only" or "write only" since that's not technically correct with quic sockets.

steviez · 2024-12-13T16:30:01Z

gossip/src/cluster_info.rs

-            multi_bind_in_range(bind_ip_addr, port_range, num_tvu_sockets.get())
-                .expect("tvu multi_bind");
-        let (tvu_quic_port, tvu_quic) = Self::bind(bind_ip_addr, port_range);
+        let tvu_config = SocketConfig::default().reuseport(true);


nit: For consistency with higher in this function, it seems we might name this tvu_udp_config. There are a couple cases below too; I don't feel too strongly which way we go but I do prefer consistency

so was thinking for this PR, since we are just adding this default, we just stick with:

let socket_config = SocketConfig::default(); // and let socket_config_reuseport = SocketConfig::default().reuseport(true)

for the next PR when we change buffer sizes, maybe it makes sense to label these explicitly like tvu_config, tpu_forwards_config, etc. and here we would be setting buffer sizes.

behzadnouri · 2024-12-13T19:19:29Z

net-utils/src/lib.rs

+#[derive(Clone, Debug, Default)]
 pub struct SocketConfig {
-    pub reuseport: bool,
+    reuseport: bool,
+    #[cfg(not(any(windows, target_os = "ios")))]
+    recv_buffer_size: Option<usize>,
+    #[cfg(not(any(windows, target_os = "ios")))]
+    send_buffer_size: Option<usize>,


You can add #[derive(Copy)] here and wouldn't need config.clone() elsewhere in the code.

Also, using #[cfg(..)] for struct fields probably isn't the best.
The fewer code differences we have across platforms is better, and here I don't think you need to change struct definition.

behzadnouri · 2024-12-13T19:24:33Z

net-utils/src/lib.rs

+    // allow here to supress unused warnings from windows/ios builds
+    #[allow(unused_mut, unused_variables)]
+    pub fn recv_buffer_size(mut self, size: usize) -> Self {
+        #[cfg(not(any(windows, target_os = "ios")))]


similarly here (and perhaps other places in the code) there is no harm keeping the code the same across platforms.
so #[cfg(...)] here is probably unnecessary.

steviez

LGTM!

Outside of the scope of this PR, but there is a little duplication in gossip/src/cluster_info.rs. For example, prior to this PR, this exact block shows up twice:

        let (_, repair) = Self::bind(bind_ip_addr, port_range);
        let (_, repair_quic) = Self::bind(bind_ip_addr, port_range);
        let (serve_repair_port, serve_repair) = Self::bind(bind_ip_addr, port_range);
        let (serve_repair_quic_port, serve_repair_quic) = Self::bind(bind_ip_addr, port_range);

Maybe as the SocketConfig tuning becomes more sophisticated in subsequent PR(s), it will make sense to refactor some stuff in a common helper (ie bind_repair()).

Wouldn't be the worst thing to get another approval from @behzadnouri or @t-nelson to make sure I didn't miss anything

steviez · 2024-12-19T07:44:31Z

net-utils/src/lib.rs

-#[cfg(not(any(windows, target_os = "ios")))]
-const DEFAULT_RECV_BUFFER_SIZE: usize = 64 * 1024 * 1024; // 64 MB - Doubled to 128MB by the kernel
-#[cfg(not(any(windows, target_os = "ios")))]
-const DEFAULT_SEND_BUFFER_SIZE: usize = 64 * 1024 * 1024; // 64 MB - Doubled to 128MB by the kernel


I think this is line with intent from #3929 (comment), but just calling out this a potential change in behavior. I'm fine with it tho as:

People who aren't knowledgable on this probably copy/pasted tuning guide

People who are knowledgable probably set custom values

Subsequent PR(s) will set intelligent values, and those will hopefully come in quicker with the groundwork laid in this PR

gregcusack requested review from steviez and t-nelson December 5, 2024 00:31

gregcusack commented Dec 5, 2024

View reviewed changes

gossip/src/cluster_info.rs Outdated Show resolved Hide resolved

gregcusack force-pushed the socket-config-builder branch 3 times, most recently from 707a6f2 to e8410b7 Compare December 6, 2024 01:13

gregcusack requested review from steviez and removed request for steviez December 6, 2024 02:38

t-nelson reviewed Dec 6, 2024

View reviewed changes

gregcusack requested a review from t-nelson December 9, 2024 19:10

gregcusack added 3 commits December 11, 2024 19:22

update socket config and create builder pattern

d0255d6

reuse new code for deprecated methods

c373d6f

add default config for readwrite socket

cd98ecf

gregcusack force-pushed the socket-config-builder branch from e9c753e to cd98ecf Compare December 11, 2024 19:22

leave default buffer size to OS

ba21675

gregcusack requested review from steviez and t-nelson and removed request for steviez and t-nelson December 12, 2024 22:29

steviez reviewed Dec 13, 2024

View reviewed changes

switch to unified socket config by reuseport

296b463

gregcusack force-pushed the socket-config-builder branch from b84b41e to 296b463 Compare December 13, 2024 18:01

behzadnouri reviewed Dec 13, 2024

View reviewed changes

remove cfg gating and add derive copy

f567051

gregcusack requested review from steviez and behzadnouri December 14, 2024 00:51

steviez approved these changes Dec 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update socket config and create builder pattern #3929

update socket config and create builder pattern #3929

gregcusack commented Dec 5, 2024 •

edited

Loading

t-nelson Dec 6, 2024

gregcusack Dec 6, 2024

gregcusack Dec 6, 2024 •

edited

Loading

steviez Dec 10, 2024

gregcusack Dec 11, 2024

steviez Dec 11, 2024

t-nelson Dec 12, 2024

gregcusack Dec 12, 2024

t-nelson Dec 12, 2024

steviez Dec 12, 2024

steviez left a comment

steviez Dec 13, 2024

gregcusack Dec 13, 2024

steviez Dec 13, 2024

gregcusack Dec 13, 2024 •

edited

Loading

steviez Dec 13, 2024

gregcusack Dec 13, 2024

behzadnouri Dec 13, 2024

behzadnouri Dec 13, 2024

steviez left a comment

steviez Dec 19, 2024

	// using udp port for quic really because we need to reusport set to false, since Self::bind() defaults to false
	// using udp port for quic really because we need to set reusport to false, since Self::bind() defaults to false

update socket config and create builder pattern #3929

Are you sure you want to change the base?

update socket config and create builder pattern #3929

Conversation

gregcusack commented Dec 5, 2024 • edited Loading

Problem

Summary of Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregcusack Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steviez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregcusack Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steviez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregcusack commented Dec 5, 2024 •

edited

Loading

gregcusack Dec 6, 2024 •

edited

Loading

gregcusack Dec 13, 2024 •

edited

Loading