Refine select nodelay

[yizhuoliang]

Description

Before this, select() on inet sockets experience lock contention on SocketHandles and degrades the performance of nginx. So now we added domain and system raw fd into SocketDest, so select() won't need to acquire sockhandle locks for every fd now. Note that the domain is set at the point of socket creation, but the rawfd field is set at listen, connect, and accept. The fundamental reason we can do this is that, both domain and rawfd can't change once it is set.

Also changed the options field of a scokethandle to be socket_options and tcp_options, as we are handling two different levels of socket options. Thus changing getsockopt and setsockopt.

Also because select() on inet sockets don't access sockhanldes, we added checks at recv_syscall to see if the INPROGRESS state is already changed to CONNECTED.

Type of change

• Bug fix (non-breaking change which fixes an issue)

[rennergade]

theres a lot of nesting here, maybe condense some of this to one liners for readability

[yzhang71]

Following Nick's suggestion, the nested loop can be modified as something like this:

use std::collections::HashMap;

pub fn set_from_kernelfds_and_translate(&mut self, kernel_fds: &FdSet, nfds: i32, rawfd_lindfd_tuples: &Vec<(i32, i32)>) {
    // Create a HashMap from the rawfd_lindfd_tuples for O(1) look-up
    let fd_map: HashMap<i32, i32> = rawfd_lindfd_tuples.iter().cloned().collect();
    for fd in 0..nfds {
        if kernel_fds.is_set(fd) {
            if let Some(lindfd) = fd_map.get(&fd) {
                self.set(*lindfd);
            }
        }
    }
}

[rennergade]

this is probably not a great idea because I believe creating a hashmap is slow, so even though the algorithmic complexity may be better N should never be large enough where it surpasses that allocation cost.

[rennergade]

some initial thoughts

[rennergade]

I think this is wrong to return kernel ret, we may have increased the return number earlier and at this point don't want to return an error. right?

[rennergade]

Also probably should handle kernel_ret = 0

[Yaxuan-w]

I didn't fully understand what is

we may have increased the return number earlier and at this point don't want to return an error

Does that mean we should return syscall_error when kernel_ret < 0?

[rennergade]

I meant we may have found a ready descriptor that isnt a kernel socket before we do the internal select. If thats the case we still want to return that information.

[justyoyo123]

These changes look good to me.

[yzhang71]

Except for the parts with the nested loops (see detailed comments), everything looks good to me.

[Yaxuan-w]

We may need to handle error cases here? or in select_readfs()

[Yaxuan-w]

The overall implementation logic makes sense to me! Great work!!

[rennergade]

I think this struct should probably be in net.rs

[rennergade]

looks great, minor change request then good to merge

[rennergade]

I think we don't actually need to import this. Can't we just use ints?

[yizhuoliang]

nice catch

[rennergade]

One last thing, I think we dont need to import the RawFD type

[rennergade]

also we import mem:size_of above, maybe combine and just import what you need?

[rennergade]

Awesome. Great job!