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

x/sys/unix: add support for linux' TCP_CC_INFO #68232

Closed
costela opened this issue Jun 28, 2024 · 14 comments
Closed

x/sys/unix: add support for linux' TCP_CC_INFO #68232

costela opened this issue Jun 28, 2024 · 14 comments

Comments

@costela
Copy link
Contributor

costela commented Jun 28, 2024

Update, Aug 14 2024 The proposal is #68232 (comment)


Proposal Details

The TCP_INFO support in x/sys/unix should be expanded to also cover TCP_CC_INFO.

There is at least one external package providing this functionality, but since it requires unsafe conversions of kernel structs, it is prone to subtle errors (e.g. "converted pointer straddles multiple allocations").

Therefore this functionality would profit from using the existing code-generation in x/sys/unix to keep it less error-prone.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/595676 mentions this issue: linux: add tcp_cc_info and its related types

@ianlancetaylor
Copy link
Contributor

This API is unusual for x/sys/unix, so sending it through the proposal process.

Proposal is

// GetsockoptTCPCCInfo returns algorithm specific congestion control information for the socket.
// It requires a type parameter to specify the type of congestion control algorithm being used. E.g. for BBR:
//
//	info, err := unix.GetsockoptTCPCCInfo[TCPBBRInfo](fd, unix.IPPROTO_TCP, unix.TCP_CC_INFO)
//
// The socket's congestion control algorighm can be retrieved via [GetsockoptString] with the [TCP_CONGESTION] option.
func GetsockoptTCPCCInfo[T TCPVegasInfo | TCPDCTCPInfo | TCPBBRInfo](fd, level, opt int) (*T, error) {
	var value tcpCCInfo
	vallen := _Socklen(SizeofTCPCCInfo)
	err := getsockopt(fd, level, opt, unsafe.Pointer(&value[0]), &vallen)
	out := (*T)(unsafe.Pointer(&value[0]))
	return out, err
}

type TCPVegasInfo struct {
	...
}

type TCPDCTCPInfo struct {
	...	
}

type TCPBBRInfo struct {
	...	
}

@rsc
Copy link
Contributor

rsc commented Jul 25, 2024

This is a puzzling API. I don't see how the caller knows which of the three structs to use. There must be some other getsockopt that tells you which congestion control algorithm the socket uses, and then you know which struct to provide. Given that the choice of argument type is not happening statically at compile time but instead dynamically based on that getsockopt (or perhaps reading a file in /proc, but surely different TCP sockets can use different algorithms), it seems like it would be better to take an any and document that it must be one of the three types and use a type switch inside the function to distinguish the three. It also seems like maybe the opt should be dropped, since it must be hard-coded to unxi.TCP_CC_INFO, right?

// GetsockoptTCPCCINFO calls getsockopt using opt = TCP_CC_INFO.
// The info must have type * TCPVegasInfo, *TCPDCTCPInfo, or *TCPBBRInfo,
// according to the congestion control algorithm used on the TCP connection.
// (To find out the algorithm, use TODO FILL THIS IN.)
func GetsockoptTCPCCINFO(fd, level int, info any) error

@rsc
Copy link
Contributor

rsc commented Jul 25, 2024

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@costela
Copy link
Contributor Author

costela commented Jul 25, 2024

Given that the choice of argument type is not happening statically at compile time but instead dynamically based on that getsockopt [...] it seems like it would be better to take an any and document that it must be one of the three types and use a type switch inside the function to distinguish the three.

Good point.
My only reasoning against this was that it would differ even more from the other (many!) existing GetSockopt* functions, which all return the requested value instead of storing into a provided argument:

// generic
func GetsockoptByte(fd, level, opt int) (value byte, err error)
func GetsockoptInet4Addr(fd, level, opt int) (value [4]byte, err error)
func GetsockoptInt(fd, level, opt int) (value int, err error)
func GetsockoptString(fd, level, opt int) (string, error)
func GetsockoptUint64(fd, level, opt int) (value uint64, err error)
// specialized
func GetsockoptICMPv6Filter(fd, level, opt int) (*ICMPv6Filter, error)
func GetsockoptIPMreq(fd, level, opt int) (*IPMreq, error)
func GetsockoptIPMreqn(fd, level, opt int) (*IPMreqn, error)
func GetsockoptIPv6MTUInfo(fd, level, opt int) (*IPv6MTUInfo, error)
func GetsockoptIPv6Mreq(fd, level, opt int) (*IPv6Mreq, error)
func GetsockoptLinger(fd, level, opt int) (*Linger, error)
func GetsockoptTCPInfo(fd, level, opt int) (*TCPInfo, error) // <-- in particular
func GetsockoptTimeval(fd, level, opt int) (*Timeval, error)
func GetsockoptTpacketStats(fd, level, opt int) (*TpacketStats, error)
func GetsockoptTpacketStatsV3(fd, level, opt int) (*TpacketStatsV3, error)
func GetsockoptUcred(fd, level, opt int) (*Ucred, error)

Following that line of thought, one could also return any and type-switch on that, but that seemed even less idiomatic?

For completeness: a simpler alternative would be multiple functions that each return one of the supported types.

func GetsockoptTCPCCVegasInfo(fd, level, opt int) (*TCPVegasInfo, error)
func GetsockoptTCPCCDCTCPInfo(fd, level, opt int) (*TCPDCTCPInfo, error)
func GetsockoptTCPCCBBRInfo(fd, level, opt int) (*TCPBBRInfo, error)

It also seems like maybe the opt should be dropped, since it must be hard-coded to unxi.TCP_CC_INFO, right?

That's also related to the existing functions mentioned above. I totally agree that this seems weird, but I suspect it would be worse to have some functions in this family with a different pattern. Maybe something for x/sys/unix/v2? 😇

@rsc
Copy link
Contributor

rsc commented Jul 31, 2024

I think this is most in keeping with the current x/sys:

func GetsockoptTCPCCVegasInfo(fd, level, opt int) (*TCPVegasInfo, error)
func GetsockoptTCPCCDCTCPInfo(fd, level, opt int) (*TCPDCTCPInfo, error)
func GetsockoptTCPCCBBRInfo(fd, level, opt int) (*TCPBBRInfo, error)

I remain confused about how you are supposed to use these APIs though. How do you know which to call? It looks like you don't even get an error if you guess wrong, so "try all three" doesn't work.

@costela
Copy link
Contributor Author

costela commented Jul 31, 2024

Yes, AFAIK there's no way to verify which of the C union types one got?

I see two main realistic use-cases:

  1. either one knows what congestion control algo one wants, sets it via Setsockopt and errors out if that doesn't work.
  2. or one uses Getsockopt and switches on the answer.

I'm not sure we can do much better without a higher level API?

@rsc
Copy link
Contributor

rsc commented Aug 7, 2024

At the least we should document how to figure out which one to expect and make sure we have APIs to use to do that. What are the APIs? What is the specific Getsockopt call to use?

@costela
Copy link
Contributor Author

costela commented Aug 7, 2024

The Getsockopt call is:

algo, err := unix.GetsockoptString(fd, unix.IPPROTO_TCP, unix.TCP_CONGESTION)

Note that not all of the current congestion control algorithms expose information via TCP_CC_INFO. The call above can return e.g. cubic, for which no equivalent GetsockoptTCPCC*Info exists.

And yes, absolutely agree that this should be documented.

@rsc
Copy link
Contributor

rsc commented Aug 14, 2024

Great, thanks. Then the API would be:

func GetsockoptTCPCCVegasInfo(fd, level, opt int) (*TCPVegasInfo, error)
func GetsockoptTCPCCDCTCPInfo(fd, level, opt int) (*TCPDCTCPInfo, error)
func GetsockoptTCPCCBBRInfo(fd, level, opt int) (*TCPBBRInfo, error)

and we'd document for each one that you call

algo, err := unix.GetsockoptString(fd, unix.IPPROTO_TCP, unix.TCP_CONGESTION)

ahead of time and also document for each which algo to expect to make that call (e.g., GetsockoptTCPCCVegasInfo) meaningful.

@rsc
Copy link
Contributor

rsc commented Aug 14, 2024

Have all remaining concerns about this proposal been addressed?

The proposal is at #68232 (comment)

@rsc
Copy link
Contributor

rsc commented Aug 29, 2024

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

The proposal is at #68232 (comment)

@rsc
Copy link
Contributor

rsc commented Sep 4, 2024

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

The proposal is at #68232 (comment)

@rsc rsc changed the title proposal: x/sys/unix: add support for linux' TCP_CC_INFO x/sys/unix: add support for linux' TCP_CC_INFO Sep 4, 2024
@rsc rsc modified the milestones: Proposal, Backlog Sep 4, 2024
@costela
Copy link
Contributor Author

costela commented Sep 9, 2024

the current proposal implementation is in https://go-review.googlesource.com/c/sys/+/595676

if someone could "unhold" it, that would be awesome! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Accepted
Development

Successfully merging a pull request may close this issue.

4 participants