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/net/ipv{4,6}: adopt net/netip address types #54883

Open
matzf opened this issue Sep 6, 2022 · 11 comments
Open

x/net/ipv{4,6}: adopt net/netip address types #54883

matzf opened this issue Sep 6, 2022 · 11 comments

Comments

@matzf
Copy link

matzf commented Sep 6, 2022

The x/net/ipv{4,6} packages currently use net.IP and net.Addr types everywhere, with all the performance issues that the new types in the net/netip package were designed to resolve. In particular, the performance sensitive function ReadBatch must allocate the returned source addresses on every call (see related #26838).

In order to introduce this without breaking changes, parts of the API needs to be duplicated, analogous to, for example, net.ReadFromUDPAddrPort added in go-1.18.
To avoid too much API bloat, it might be sufficient to implement this for performance sensitive functions like Read/WriteBatch.

Losely related to proposal #45886; when a fast net.Read/WriteUDPMsgs functionality is available, we may simply no longer need x/net.Read/WriteBatch for some use cases.

@matzf matzf added the Proposal label Sep 6, 2022
@gopherbot gopherbot added this to the Proposal milestone Sep 6, 2022
@ianlancetaylor
Copy link
Contributor

CC @neild

@rsc
Copy link
Contributor

rsc commented Oct 12, 2022

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

@rsc
Copy link
Contributor

rsc commented Oct 26, 2022

What is the concrete API being proposed?

@matzf
Copy link
Author

matzf commented Oct 31, 2022

The minimal change to support netip.AddrPort for Read/WriteBatch in both x/net/ipv4 and x/net/ipv6:

type Message struct {
	Buffers [][]byte
	OOB     []byte
	Addr    net.Addr
	AddrPort    netip.AddrPort  // + Specifies destination address when writing with WriteAddrPort functions. Contains source address after reading with ReadAddrPort functions.
	N       int
	NN      int
	Flags   int
}

func (c *PacketConn) ReadBatchAddrPort(ms []Message, flags int) (int, error)
func (c *PacketConn) WriteBatchAddrPort(ms []Message, flags int) (int, error)
func (c *RawConn) ReadBatchAddrPort(ms []Message, flags int) (int, error)
func (c *RawConn) WriteBatchAddrPort(ms []Message, flags int) (int, error)

Note that for UDP, this almost identical to #45886. The remaining difference is the flags int parameter on the Read/WriteBatchAddrPort which does not exist on net.UDPConn.Read/WriteUDPMsgs.

Additionally, we could extend the ControlMessage type in the same style and duplicate the ReadFrom/WriteTo functions analogously. This would lead a lot of clutter though, and I'm not sure whether there will be enough of a performance boost to justify this bloat.

An alternative option could be to add a new package, e.g. x/net/ip, replacing the ipv4 and ipv6 packages with a new API based solely around netip.Addr and AddrPort. If this seems like a useful idea, I can work out a detailed proposal for this.

@rsc
Copy link
Contributor

rsc commented Nov 9, 2022

If we pattern off of #45886, maybe we should define IPMsg matching the new UDPMsg
(using its field names, not the ones in ipv4.Message)
and add ReadIPMsgs and WriteIPMsgs methods taking []IPMsg?

Then at least people using the UDP version would know how to use the IP version and vice versa?
Also then there's not two different addresses in Message to worry about.
We'd also be able to drop the flags arguments, since the flags field is in IPMsg.

@rsc
Copy link
Contributor

rsc commented Nov 16, 2022

@martin-sucha @database64128 you commented on #45886. Any thoughts about my previous comment, essentially applying the same API here?

@rsc
Copy link
Contributor

rsc commented Nov 30, 2022

If we apply the #45886 API then we get

type IPMsg struct {
  Buffer  [][]byte // must be pre-allocated by caller to non-zero len; callee fills, re-slices elements
  Control [][]byte // like Buffer (pre-allocated, re-sliced), may be zero-length
  Addr    netip.AddrPort

  // Flags is an OS-specific bitmask.
  Flags int
}

func (c *PacketConn) ReadIPMsgs(ms []IPMsg, flags int) (msgsRead int, err error)
func (c *PacketConn) WriteIPMsgs(ms []IPMsg, flags int) (msgsSent int, err error)
func (c *RawConn) ReadIPMsgs(ms []IPMsg, flags int) (msgsRead int, err error)
func (c *RawConn) WriteIPMsgs(ms []IPMsg, flags int) (msgsSent int, err error)

Do I have that right? Does that sound like a reasonable API?

@matzf
Copy link
Author

matzf commented Nov 30, 2022

The comment // populated by callee on IPMsg.Addr does not seem right for the write APIs.

I see that the there is no int flags arguments on the Read/Write functions, as you had proposed further up:

We'd also be able to drop the flags arguments, since the flags field is in IPMsg.

Note that the C-API has a flags field in the message header and a flags parameter for the send/receive functions. The flags field in the message header is only used to return flags on received messages.
To set per-call options, like MSG_OOB, MSG_MORE, MSG_DONTWAIT, the flags parameter is used.
Now we could of course say that we use the flags field of the first message to set these per-call options, but I'm not sure that we're really gaining much from this.

Otherwise, this looks great to me, thanks for taking this up!

@rsc
Copy link
Contributor

rsc commented Dec 7, 2022

Add flags bits to the argument list and removed the populated by callee comment.
Any other comments on the API?
Does anyone object to adding this API?

@rsc
Copy link
Contributor

rsc commented Dec 14, 2022

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

@rsc
Copy link
Contributor

rsc commented Dec 21, 2022

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

@rsc rsc changed the title proposal: x/net/ipv{4,6}: adopt net/netip address types x/net/ipv{4,6}: adopt net/netip address types Dec 21, 2022
@rsc rsc modified the milestones: Proposal, Backlog Dec 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Accepted
Development

No branches or pull requests

4 participants