netif: new package with netdev redesign ideas #629

soypat · 2023-12-18T23:47:32Z

Created ~~nets~~ netif package with new design ideas learned from building a network stack in Go over the last month.

Observations, suggestions, constructive criticism most welcome! I took some inspiration from gvisor's registration.go.

Note: I tried to keep true to Scott's original layout best I could, reusing netlink and netdev names- but you'll notice some shifting around, notably:

netdev.Netdever interface is now netif.Stack
- Reason: The method set represents what's often referred to as a Network Stack- and the method set has no reference to the actual Device.
probe.Probe() shall now return a netif.Netdev (new Probe not yet implemented)
- Reason: More wieldy to have a single interface that contains the union of old Netlink and Netdev.

scottfeldman

I appreciate your effort to re-use names/interfaces/structs with existing code, but I can see it's constraining you and things are still a little messy. Would you consider a fresh rewrite? Maybe just keep the interfaces and get rid of the noisy #defines.

I was looking at net.Interface type. It's something that is not implemented with the current TinyGo's "net" package, but I'd like to port it over. It provides a lot of things I see duplicated here, like MAC addr, IP addr, MTU, etc.

Interface might be a better name than Link.

Also, I was looking at net.Resolver. It's also not implemented with TinyGo's "net" package, but I think we could and it would take the place of GetHostByName().

Stack could be the top-level interface used by the "net" package:

type Stack interface {
    Socketer
    Interfaces() net.Interfaces
    Resolver() net.Resolver
}

type Socketer interface {
    // syscall replacements for socket(2) calls
    Socket(...)
    Bind(...)
    ...
    // These move:
    //   GetHostByName() moves to net.Resolver.LookupIp()
    //   Addr() moves to net.Interface.Addrs()
}

soypat · 2023-12-20T02:08:07Z

I was looking at net.Interface type. It's something that is not implemented with the current TinyGo's "net" package, but I'd like to port it over. It provides a lot of things I see duplicated here, like MAC addr, IP addr, MTU, etc.

That's actually a great idea! With net/interface.go added I think nets.Link could be rewritten as:

// Interface is the minimum interface that need be implemented by any network
// device driver.
type Interface interface {
	// HardwareAddr6 returns the device's 6-byte [MAC address].
	//
	// [MAC address]: https://en.wikipedia.org/wiki/MAC_address
	HardwareAddr6() ([6]byte, error)
	// Flags returns the net.Flag values for the interface. It includes state of connection.
	Flags() net.Flags
	// MTU returns the maximum transmission unit size.
	MTU() int
}

I'd avoid relying on the net.Interface type, as it seems as it is a heavily OS dependendent type.

Also, I was looking at net.Resolver. It's also not implemented with TinyGo's "net" package, but I think we could and it would take the place of GetHostByName().

It would seem that rather than a Stack be composed of a net.Resolver- the net.Resolver uses a stack to resolve addresses, see the Dial field. That said I'm not knowledgable on the subject and would for the time being avoid adding resolvers to the interface until I understand them better. For now I've implemented ARP address resolution via seqs.

type Stack interface {
Socketer
Interfaces() net.Interfaces
Resolver() net.Resolver
}

Hmmm- so I do agree that this is the abstraction Go has in the net package, maybe we should replace each individually though I feel. I feel each Interface would have it's own Socketer (I've called it SocketStack)- though honestly I'm still not sure how that works behind the scenes. Man, I should really get around to reading https://man7.org/tlpi/...

I've applied some of the changes to this PR mentioned above. Might still require more research?

scottfeldman · 2023-12-20T06:01:17Z

nets/nets.go

+	// Disconnect device from network
+	NetDisconnect()
+	// Notify to register callback for network events
+	NetNotify(cb func(Event))


NetNotify isn't wifi specific. It's for wired/wireless interfaces to indicate interface UP/DOWN.

scottfeldman · 2023-12-20T06:01:22Z

nets/nets.go

+type InterfaceWifi interface {
+	Interface
+	// Connect device to network
+	NetConnect(params WifiParams) error


These weren't intended to be Wifi only. I need them for this ch9120 eth driver also. SSID/Passphrase are specific to Wifi, but connect time-out, retries, watchdog timeout, dhcp mode (static or dynamic), and maybe more.

From what I recall programming the ENC28J60, the connection for RJ45 connections is passive, is it not?

scottfeldman · 2023-12-20T06:06:38Z

nets/nets.go

+
+	// Addr returns IP address assigned to the interface, either by
+	// DHCP or statically
+	Addr() (netip.Addr, error)


this should move to Interface, correct?

IP Addresses are not actually Interface specific- it's network-stack level.

Also, I think I'd leave it with no error returned since there is really no information that can be returned inside the error other than the IP address has not been set- and that piece of information can already be returned in-band inside netip.Addr's zero value.

scottfeldman · 2023-12-20T06:07:44Z

nets/nets.go

+	Bind(sockfd int, ip netip.AddrPort) error
+	Connect(sockfd int, host string, ip netip.AddrPort) error
+	Listen(sockfd int, backlog int) error
+	Accept(sockfd int, ip netip.AddrPort) (int, error)


BTW, just so we don't regress, this is now:

Accept(sockfd int) (int, netip.AddrPort, error)

scottfeldman · 2023-12-20T06:08:40Z

nets/nets.go

+)
+
+//go:linkname UseSocketStack net.useNetdev
+func UseSocketStack(stack SocketStack)


I think I liked UseStack and Stack better.

scottfeldman · 2023-12-20T06:22:03Z

nets/nets.go

+
+// WifiStack is returned by `Probe` function for devices that communicate
+// on the OSI level 4 (transport) layer.
+type WifiStack interface {


I'm not following how this works. Can you add probe.go file(s) to show how this works for wired/wireless, and hw/sw stack devices? e.g. cyw43 wireless sw stack, wifinina wireless hw stack, ch9120 wired hw stack, lan8720 wired sw stack.

scottfeldman · 2023-12-20T06:30:41Z

nets/nets.go

+	AuthTypeWPA2Mixed        // WPA2/WPA mixed authorization
+)
+
+type WifiAutoconnectParams struct {


These aren't wifi-specific. Need same for wired.

scottfeldman · 2023-12-20T06:32:07Z

nets/nets.go

+		return ErrConnectModeNoGood
+	}
+	go func() {
+		// Wifi autoconnect algorithm in one place,


Ya, that seems good. But for wired also.

scottfeldman · 2023-12-20T06:32:58Z

nets/nets.go

+}
+
+type EthPoller interface {
+	Interface


Did you mean to have Interface here?

An EthPoller should implement Interface as well I think. At least it should have the Flags method to check if the link is still up.

scottfeldman · 2023-12-20T06:33:53Z

nets/nets.go

+	// HardwareAddr6 returns the device's 6-byte [MAC address].
+	//
+	// [MAC address]: https://en.wikipedia.org/wiki/MAC_address
+	HardwareAddr6() ([6]byte, error)


net.HardwareAddr?

I'd avoid net.HardwareAddr for the same reasons we switched to netip.Addr, it'd be a pain to have to start checking probe implementations for the implementation details of the HardwareAddr method and if the memory was copied or a reference was passed. Also there's the issue of heap memory usage. All in all, if we can avoid net.HardwareAddr I'd do so.

scottfeldman · 2024-01-10T06:03:19Z

+1 netif

deadprogram · 2024-01-10T06:13:13Z

@soypat note CI failure here https://github.com/tinygo-org/drivers/actions/runs/7440065478/job/20240657521?pr=629#step:7:128

nets/nets.go was replaced by netif/netif.go

scottfeldman · 2024-02-01T19:48:25Z

looking it over...

soypat · 2024-02-25T02:48:48Z

I've removed several sentinel errors which had not fully convinced me of their usefulness. In any case we can always add them back in the future, but once added they are there forever.

deadprogram · 2024-02-25T10:33:28Z

netif/netif.go

+type Stack interface {
+	// GetHostByName returns the IP address of either a hostname or IPv4
+	// address in standard dot notation
+	// GetHostByName(name string) (netip.Addr, error)


Is this supposed to be commented?

Yes, We can delete that- I've created the Resolver interface for that purpose.

deadprogram · 2024-02-25T10:37:12Z

Overall this seems like a good set of patterns. I would love to see a simple mermaid diagram or something added to help better understand how it all fits together.

I presume that the idea is once this PR was merged, the next step would be to replace the implementations for the current wifi devices?

soypat · 2024-02-25T15:33:19Z

@deadprogram yes, the idea is to start using these abstractions once merged to get a better idea of issues with them if any. I'd go easy and replace them one at a time, or better yet, start by adding the Pico W (since its not in drivers yet) and explore by not breaking anything new.

scottfeldman · 2024-02-26T22:30:17Z

I have to update the tinygo net package periodically to back-port upstream changes, which I'm happy to do but would rather not...it's error prone and not so much fun. With this new PR, we'd still need to do that.

I think we should revisit fully porting the net package into tinygo with the goal of getting tinygo-specific code pushed upstream. The net package is structured to target multiple OS targets; to me it looks like we could add tinygo as a new target without too much work.

And the tinygo port would inform us of the interface(s) a tinygo stack must implement. For example, tcpsock_tinygo.go would provide sysDialer.dialTCP(), returning a netFD. The netFD is specific to tinygo but would implement the net.Conn interface.

A suggested approach would be to 1) do the full port using existing netdev so we can test with known good devices/drivers, and 2) replace netdev with what interface(s) naturally fall out of the full porting process.

I'm excluding net/http. We can tackle a full port of net/http separate from this effort.

soypat · 2024-02-27T22:04:16Z

@scottfeldman Interesting observations- Maybe we're going about this the wrong way and the API we wish to expose is simpler?

seqs already exposes a way of generating a net.Conn, but the HAL we are choosing in seqs is Berkeley Socket which means to link net to our seqs we must:

Make the seqs stacks.TCPConn implement Berkeley sockets by way of some intermediary framework
So that we can link that intermediary framework with the net.useNetdev which receives a berkeley socket abstraction
So that we can use the Berkeley socket abstraction to make a net.TCPConn

So we just did extra work to get to the same place and probably also took a performance hit...

scottfeldman · 2024-02-27T22:25:46Z

@soypat Ya, I saw that seqs exposes a net.Conn, so it's already aligned with where I think we're going if we do a full net package port. My hope is the full port exercise would tell us exactly the interfaces needed from a stack. It's likely the existing netdev interfaces disappear and for the embedded stack devices (wifinina, rtl8720n) have a stack shim. Or maybe they just implement net.Conn and be done with it. There are other details like resolver and interfaces to work out, but again, I think a full port of net would tell us what's needed.

Should we try it (the full port)? I have some time to work on it, but you have first dibs.

The risks:

Full port doesn't fit (flash/sram), or doesn't leave much for app
We hit something that's not ported or can't be ported to TinyGo (unlikely)

nets: new package with netdev redesign ideas

731362a

soypat requested a review from deadprogram December 18, 2023 23:47

soypat added 4 commits December 18, 2023 15:56

add EthLinkPoller;

60cdb7f

add more methods to Link interface

7e45551

change EthLinkPoller name

570e3d2

remove old comment

b4f43b4

scottfeldman reviewed Dec 19, 2023

View reviewed changes

apply some of @scottfeldman's suggestions

728bdbb

rethink netdev name again

d4705e5

scottfeldman reviewed Dec 20, 2023

View reviewed changes

soypat added 4 commits December 20, 2023 10:26

add examples

6913eaf

rename package: nets -> netif

f657269

rename file

68ccd70

modify netif.Resolver to use net.DefaultResolver method set

cdc76f3

soypat changed the title ~~nets: new package with netdev redesign ideas~~ netif: new package with netdev redesign ideas Jan 7, 2024

soypat added 2 commits January 10, 2024 09:11

Delete nets/nets.go

1690c2c

nets/nets.go was replaced by netif/netif.go

Delete nets/example_test.go

9b74ecb

delay addition of extra error types until verifiably needed

85a28fb

deadprogram reviewed Feb 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

netif: new package with netdev redesign ideas #629

netif: new package with netdev redesign ideas #629

soypat commented Dec 18, 2023 •

edited

scottfeldman left a comment •

edited

soypat commented Dec 20, 2023

scottfeldman Dec 20, 2023

scottfeldman Dec 20, 2023

soypat Dec 20, 2023

scottfeldman Dec 20, 2023

soypat Dec 20, 2023 •

edited

soypat Dec 20, 2023

scottfeldman Dec 20, 2023

scottfeldman Dec 20, 2023

scottfeldman Dec 20, 2023

scottfeldman Dec 20, 2023

scottfeldman Dec 20, 2023

scottfeldman Dec 20, 2023

soypat Dec 20, 2023

scottfeldman Dec 20, 2023

soypat Dec 20, 2023

scottfeldman commented Jan 10, 2024

deadprogram commented Jan 10, 2024

scottfeldman commented Feb 1, 2024

soypat commented Feb 25, 2024

deadprogram Feb 25, 2024

soypat Feb 25, 2024

deadprogram commented Feb 25, 2024

soypat commented Feb 25, 2024

scottfeldman commented Feb 26, 2024

soypat commented Feb 27, 2024

scottfeldman commented Feb 27, 2024 •

edited

netif: new package with netdev redesign ideas #629

Are you sure you want to change the base?

netif: new package with netdev redesign ideas #629

Conversation

soypat commented Dec 18, 2023 • edited

scottfeldman left a comment • edited

Choose a reason for hiding this comment

soypat commented Dec 20, 2023

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

soypat Dec 20, 2023 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottfeldman commented Jan 10, 2024

deadprogram commented Jan 10, 2024

scottfeldman commented Feb 1, 2024

soypat commented Feb 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deadprogram commented Feb 25, 2024

soypat commented Feb 25, 2024

scottfeldman commented Feb 26, 2024

soypat commented Feb 27, 2024

scottfeldman commented Feb 27, 2024 • edited

soypat commented Dec 18, 2023 •

edited

scottfeldman left a comment •

edited

soypat Dec 20, 2023 •

edited

scottfeldman commented Feb 27, 2024 •

edited