From c60de48a300a80c7ee9c3f09322510727f14d741 Mon Sep 17 00:00:00 2001 From: Michael Birtwell Date: Fri, 14 Jan 2022 16:51:43 +0000 Subject: [PATCH] Upgrade smoltcp 0.6.0 -> 0.8.0 Main changes: Deal with interfaces now being generic over mediums, update interface name and initialisation. Interfaces now own their sockets. So we store a reference to the Interface instead of the SocketSet in Scheduler and IO. Sockets are no longer reference counted. We never called the function to increase the socket's reference count, so now we just remove it where it was previously released. This will result in the socket being dropped at a different time, but I think that should be fine. Tested firmware upload to the bootloader and spamming artiq_coremgmt log calls to download the log from the firmware. Signed-off-by: Michael Birtwell --- artiq/firmware/Cargo.lock | 14 ++- artiq/firmware/bootloader/Cargo.toml | 2 +- artiq/firmware/bootloader/main.rs | 23 ++--- artiq/firmware/libboard_misoc/Cargo.toml | 2 +- artiq/firmware/runtime/Cargo.toml | 2 +- artiq/firmware/runtime/main.rs | 51 ++++------- artiq/firmware/runtime/sched.rs | 107 +++++++++++++---------- 7 files changed, 105 insertions(+), 96 deletions(-) diff --git a/artiq/firmware/Cargo.lock b/artiq/firmware/Cargo.lock index e5d303a00..8e0c7f95a 100644 --- a/artiq/firmware/Cargo.lock +++ b/artiq/firmware/Cargo.lock @@ -240,6 +240,12 @@ version = "0.7.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c75de51135344a4f8ed3cfe2720dc27736f7711989703a0b43aadf3753c55577" +[[package]] +name = "managed" +version = "0.8.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0ca88d725a0a943b096803bd34e73a4437208b6077654cc4ecb2947a5f91618d" + [[package]] name = "memchr" version = "2.4.1" @@ -321,7 +327,7 @@ dependencies = [ "io", "log", "logger_artiq", - "managed", + "managed 0.7.2", "proto_artiq", "riscv", "smoltcp", @@ -365,13 +371,13 @@ checksum = "388a1df253eca08550bef6c72392cfe7c30914bf41df5269b68cbd6ff8f570a3" [[package]] name = "smoltcp" -version = "0.6.0" +version = "0.8.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0fe46639fd2ec79eadf8fe719f237a7a0bd4dac5d957f1ca5bbdbc1c3c39e53a" +checksum = "d2308a1657c8db1f5b4993bab4e620bdbe5623bd81f254cf60326767bb243237" dependencies = [ "bitflags", "byteorder", - "managed", + "managed 0.8.0", ] [[package]] diff --git a/artiq/firmware/bootloader/Cargo.toml b/artiq/firmware/bootloader/Cargo.toml index 5c2a54ba5..9e5ba820a 100644 --- a/artiq/firmware/bootloader/Cargo.toml +++ b/artiq/firmware/bootloader/Cargo.toml @@ -16,5 +16,5 @@ build_misoc = { path = "../libbuild_misoc" } byteorder = { version = "1.0", default-features = false } crc = { version = "1.7", default-features = false } board_misoc = { path = "../libboard_misoc", features = ["uart_console", "smoltcp"] } -smoltcp = { version = "0.6.0", default-features = false, features = ["ethernet", "proto-ipv4", "proto-ipv6", "socket-tcp"] } +smoltcp = { version = "0.8.0", default-features = false, features = ["medium-ethernet", "proto-ipv4", "proto-ipv6", "socket-tcp"] } riscv = { version = "0.6.0", features = ["inline-asm"] } diff --git a/artiq/firmware/bootloader/main.rs b/artiq/firmware/bootloader/main.rs index 555f8b0b6..535c6234d 100644 --- a/artiq/firmware/bootloader/main.rs +++ b/artiq/firmware/bootloader/main.rs @@ -18,6 +18,8 @@ use board_misoc::slave_fpga; use board_misoc::{clock, ethmac, net_settings}; use board_misoc::uart_console::Console; use riscv::register::{mcause, mepc, mtval}; +use smoltcp::iface::SocketStorage; +use smoltcp::wire::HardwareAddress; fn check_integrity() -> bool { extern { @@ -396,6 +398,9 @@ fn network_boot() { println!("Initializing network..."); + // Assuming only one socket is ever needed by the bootloader. + // The smoltcp reuses the listening socket when the connection is established. + let mut sockets = [SocketStorage::EMPTY]; let mut net_device = unsafe { ethmac::EthernetDevice::new() }; net_device.reset_phy_if_any(); @@ -412,15 +417,15 @@ fn network_boot() { let mut interface = match net_addresses.ipv6_addr { Some(addr) => { ip_addrs[2] = IpCidr::new(addr, 0); - smoltcp::iface::EthernetInterfaceBuilder::new(net_device) - .ethernet_addr(net_addresses.hardware_addr) + smoltcp::iface::InterfaceBuilder::new(net_device, &mut sockets[..]) + .hardware_addr(HardwareAddress::Ethernet(net_addresses.hardware_addr)) .ip_addrs(&mut ip_addrs[..]) .neighbor_cache(neighbor_cache) .finalize() } None => - smoltcp::iface::EthernetInterfaceBuilder::new(net_device) - .ethernet_addr(net_addresses.hardware_addr) + smoltcp::iface::InterfaceBuilder::new(net_device, &mut sockets[..]) + .hardware_addr(HardwareAddress::Ethernet(net_addresses.hardware_addr)) .ip_addrs(&mut ip_addrs[..2]) .neighbor_cache(neighbor_cache) .finalize() @@ -429,14 +434,10 @@ fn network_boot() { let mut rx_storage = [0; 4096]; let mut tx_storage = [0; 128]; - let mut socket_set_entries: [_; 1] = Default::default(); - let mut sockets = - smoltcp::socket::SocketSet::new(&mut socket_set_entries[..]); - let tcp_rx_buffer = smoltcp::socket::TcpSocketBuffer::new(&mut rx_storage[..]); let tcp_tx_buffer = smoltcp::socket::TcpSocketBuffer::new(&mut tx_storage[..]); let tcp_socket = smoltcp::socket::TcpSocket::new(tcp_rx_buffer, tcp_tx_buffer); - let tcp_handle = sockets.add(tcp_socket); + let tcp_handle = interface.add_socket(tcp_socket); let mut net_conn = NetConn::new(); let mut boot_time = None; @@ -446,7 +447,7 @@ fn network_boot() { loop { let timestamp = clock::get_ms() as i64; { - let socket = &mut *sockets.get::(tcp_handle); + let socket = &mut *interface.get_socket::(tcp_handle); match boot_time { None => { @@ -475,7 +476,7 @@ fn network_boot() { } } - match interface.poll(&mut sockets, smoltcp::time::Instant::from_millis(timestamp)) { + match interface.poll(smoltcp::time::Instant::from_millis(timestamp)) { Ok(_) => (), Err(smoltcp::Error::Unrecognized) => (), Err(err) => println!("Network error: {}", err) diff --git a/artiq/firmware/libboard_misoc/Cargo.toml b/artiq/firmware/libboard_misoc/Cargo.toml index 0d8d705cc..69c1d46d3 100644 --- a/artiq/firmware/libboard_misoc/Cargo.toml +++ b/artiq/firmware/libboard_misoc/Cargo.toml @@ -15,7 +15,7 @@ build_misoc = { path = "../libbuild_misoc" } [dependencies] byteorder = { version = "1.0", default-features = false } log = { version = "0.4", default-features = false, optional = true } -smoltcp = { version = "0.6.0", default-features = false, optional = true } +smoltcp = { version = "0.8.0", default-features = false, optional = true } riscv = { version = "0.6.0", features = ["inline-asm"] } [features] diff --git a/artiq/firmware/runtime/Cargo.toml b/artiq/firmware/runtime/Cargo.toml index ee987ad2d..241246821 100644 --- a/artiq/firmware/runtime/Cargo.toml +++ b/artiq/firmware/runtime/Cargo.toml @@ -27,7 +27,7 @@ board_misoc = { path = "../libboard_misoc", features = ["uart_console", "smoltcp logger_artiq = { path = "../liblogger_artiq" } board_artiq = { path = "../libboard_artiq" } proto_artiq = { path = "../libproto_artiq", features = ["log", "alloc"] } -smoltcp = { version = "0.6.0", default-features = false, features = ["alloc", "ethernet", "proto-ipv4", "proto-ipv6", "socket-tcp"] } +smoltcp = { version = "0.8.0", default-features = false, features = ["alloc", "medium-ethernet", "proto-ipv4", "proto-ipv6", "socket-tcp"] } riscv = { version = "0.6.0", features = ["inline-asm"] } [dependencies.fringe] diff --git a/artiq/firmware/runtime/main.rs b/artiq/firmware/runtime/main.rs index 45fb76f91..f8843352d 100644 --- a/artiq/firmware/runtime/main.rs +++ b/artiq/firmware/runtime/main.rs @@ -27,7 +27,7 @@ extern crate riscv; use core::cell::RefCell; use core::convert::TryFrom; -use smoltcp::wire::IpCidr; +use smoltcp::wire::{IpCidr, HardwareAddress}; use board_misoc::{csr, ident, clock, spiflash, config, net_settings, pmp, boot}; #[cfg(has_ethmac)] @@ -123,38 +123,33 @@ fn startup() { net_device.reset_phy_if_any(); let net_device = { - use smoltcp::time::Instant; - use smoltcp::wire::PrettyPrinter; - use smoltcp::wire::EthernetFrame; + use smoltcp::phy::Tracer; - fn net_trace_writer(timestamp: Instant, printer: PrettyPrinter>) { - print!("\x1b[37m[{:6}.{:03}s]\n{}\x1b[0m\n", - timestamp.secs(), timestamp.millis(), printer) - } - - fn net_trace_silent(_timestamp: Instant, _printer: PrettyPrinter>) {} - - let net_trace_fn: fn(Instant, PrettyPrinter>); + // We can't create the function pointer as a separate variable here because the type of + // the packet argument Packet isn't accessible and rust's type inference isn't sufficient + // to propagate in to a local var. match config::read_str("net_trace", |r| r.map(|s| s == "1")) { - Ok(true) => net_trace_fn = net_trace_writer, - _ => net_trace_fn = net_trace_silent + Ok(true) => Tracer::new(net_device, |timestamp, packet| { + print!("\x1b[37m[{:6}.{:03}s]\n{}\x1b[0m\n", + timestamp.secs(), timestamp.millis(), packet) + }), + _ => Tracer::new(net_device, |_, _| {}), } - smoltcp::phy::EthernetTracer::new(net_device, net_trace_fn) }; let neighbor_cache = smoltcp::iface::NeighborCache::new(alloc::collections::btree_map::BTreeMap::new()); let net_addresses = net_settings::get_adresses(); info!("network addresses: {}", net_addresses); - let mut interface = match net_addresses.ipv6_addr { + let interface = match net_addresses.ipv6_addr { Some(addr) => { let ip_addrs = [ IpCidr::new(net_addresses.ipv4_addr, 0), IpCidr::new(net_addresses.ipv6_ll_addr, 0), IpCidr::new(addr, 0) ]; - smoltcp::iface::EthernetInterfaceBuilder::new(net_device) - .ethernet_addr(net_addresses.hardware_addr) + smoltcp::iface::InterfaceBuilder::new(net_device, vec![]) + .hardware_addr(HardwareAddress::Ethernet(net_addresses.hardware_addr)) .ip_addrs(ip_addrs) .neighbor_cache(neighbor_cache) .finalize() @@ -164,8 +159,8 @@ fn startup() { IpCidr::new(net_addresses.ipv4_addr, 0), IpCidr::new(net_addresses.ipv6_ll_addr, 0) ]; - smoltcp::iface::EthernetInterfaceBuilder::new(net_device) - .ethernet_addr(net_addresses.hardware_addr) + smoltcp::iface::InterfaceBuilder::new(net_device, vec![]) + .hardware_addr(HardwareAddress::Ethernet(net_addresses.hardware_addr)) .ip_addrs(ip_addrs) .neighbor_cache(neighbor_cache) .finalize() @@ -184,7 +179,7 @@ fn startup() { drtio_routing::interconnect_disable_all(); let aux_mutex = sched::Mutex::new(); - let mut scheduler = sched::Scheduler::new(); + let mut scheduler = sched::Scheduler::new(interface); let io = scheduler.io(); rtio_mgt::startup(&io, &aux_mutex, &drtio_routing_table, &up_destinations); @@ -211,19 +206,7 @@ fn startup() { let mut net_stats = ethmac::EthernetStatistics::new(); loop { scheduler.run(); - - { - let sockets = &mut *scheduler.sockets().borrow_mut(); - loop { - let timestamp = smoltcp::time::Instant::from_millis(clock::get_ms() as i64); - match interface.poll(sockets, timestamp) { - Ok(true) => (), - Ok(false) => break, - Err(smoltcp::Error::Unrecognized) => (), - Err(err) => debug!("network error: {}", err) - } - } - } + scheduler.run_network(); if let Some(_net_stats_diff) = net_stats.update() { debug!("ethernet mac:{}", ethmac::EthernetStatistics::new()); diff --git a/artiq/firmware/runtime/sched.rs b/artiq/firmware/runtime/sched.rs index d6a347269..991569cd1 100644 --- a/artiq/firmware/runtime/sched.rs +++ b/artiq/firmware/runtime/sched.rs @@ -9,11 +9,13 @@ use fringe::generator::{Generator, Yielder, State as GeneratorState}; use smoltcp::time::Duration; use smoltcp::Error as NetworkError; use smoltcp::wire::IpEndpoint; -use smoltcp::socket::{SocketHandle, SocketRef}; +use smoltcp::iface::{Interface, SocketHandle}; use io::{Read, Write}; use board_misoc::clock; use urc::Urc; +use board_misoc::ethmac::EthernetDevice; +use smoltcp::phy::Tracer; #[derive(Fail, Debug)] pub enum Error { @@ -31,8 +33,6 @@ impl From for Error { } } -type SocketSet = ::smoltcp::socket::SocketSet<'static, 'static, 'static>; - #[derive(Debug)] struct WaitRequest { event: Option<*mut dyn FnMut() -> bool>, @@ -59,7 +59,7 @@ impl Thread { unsafe fn new(io: &Io, stack_size: usize, f: F) -> ThreadHandle where F: 'static + FnOnce(Io) + Send { let spawned = io.spawned.clone(); - let sockets = io.sockets.clone(); + let network = io.network.clone(); // Add a 4k stack guard to the stack of any new threads let stack = OwnedStack::new(stack_size + 4096); @@ -67,8 +67,8 @@ impl Thread { generator: Generator::unsafe_new(stack, |yielder, _| { f(Io { yielder: Some(yielder), - spawned: spawned, - sockets: sockets + spawned, + network }) }), waiting_for: WaitRequest { @@ -115,19 +115,21 @@ impl ThreadHandle { } } +type Network = Interface<'static, Tracer>; + pub struct Scheduler { threads: Vec, spawned: Urc>>, - sockets: Urc>, + network: Urc>, run_idx: usize, } impl Scheduler { - pub fn new() -> Scheduler { + pub fn new(network: Network) -> Scheduler { Scheduler { threads: Vec::new(), spawned: Urc::new(RefCell::new(Vec::new())), - sockets: Urc::new(RefCell::new(SocketSet::new(Vec::new()))), + network: Urc::new(RefCell::new(network)), run_idx: 0, } } @@ -136,13 +138,11 @@ impl Scheduler { Io { yielder: None, spawned: self.spawned.clone(), - sockets: self.sockets.clone() + network: self.network.clone() } } pub fn run(&mut self) { - self.sockets.borrow_mut().prune(); - self.threads.append(&mut *self.spawned.borrow_mut()); if self.threads.len() == 0 { return } @@ -188,8 +188,17 @@ impl Scheduler { } } - pub fn sockets(&self) -> &RefCell { - &*self.sockets + pub fn run_network(&mut self) { + let mut interface = self.network.borrow_mut(); + loop { + let timestamp = smoltcp::time::Instant::from_millis(clock::get_ms() as i64); + match interface.poll(timestamp) { + Ok(true) => (), + Ok(false) => break, + Err(smoltcp::Error::Unrecognized) => (), + Err(err) => debug!("network error: {}", err) + } + } } } @@ -197,7 +206,7 @@ impl Scheduler { pub struct Io<'a> { yielder: Option<&'a Yielder>, spawned: Urc>>, - sockets: Urc>, + network: Urc>, } impl<'a> Io<'a> { @@ -291,10 +300,10 @@ impl<'a> Drop for MutexGuard<'a> { macro_rules! until { ($socket:expr, $ty:ty, |$var:ident| $cond:expr) => ({ - let (sockets, handle) = ($socket.io.sockets.clone(), $socket.handle); + let (network, handle) = ($socket.io.network.clone(), $socket.handle); $socket.io.until(move || { - let mut sockets = sockets.borrow_mut(); - let $var = sockets.get::<$ty>(handle); + let mut network = network.borrow_mut(); + let $var = network.get_socket::<$ty>(handle); $cond }) }) @@ -316,9 +325,9 @@ impl<'a> TcpListener<'a> { fn new_lower(io: &'a Io<'a>, buffer_size: usize) -> SocketHandle { let rx_buffer = vec![0; buffer_size]; let tx_buffer = vec![0; buffer_size]; - io.sockets + io.network .borrow_mut() - .add(TcpSocketLower::new( + .add_socket(TcpSocketLower::new( TcpSocketBuffer::new(rx_buffer), TcpSocketBuffer::new(tx_buffer))) } @@ -333,9 +342,9 @@ impl<'a> TcpListener<'a> { } fn with_lower(&self, f: F) -> R - where F: FnOnce(SocketRef) -> R { - let mut sockets = self.io.sockets.borrow_mut(); - let result = f(sockets.get(self.handle.get())); + where F: FnOnce(&mut TcpSocketLower) -> R { + let mut network = self.io.network.borrow_mut(); + let result = f(network.get_socket(self.handle.get())); result } @@ -353,7 +362,7 @@ impl<'a> TcpListener<'a> { pub fn listen>(&self, endpoint: T) -> Result<(), Error> { let endpoint = endpoint.into(); - self.with_lower(|mut s| s.listen(endpoint)) + self.with_lower(|s| s.listen(endpoint)) .map(|()| { self.endpoint.set(endpoint); () @@ -365,10 +374,10 @@ impl<'a> TcpListener<'a> { // We're waiting until at least one half of the connection becomes open. // This handles the case where a remote socket immediately sends a FIN-- // that still counts as accepting even though nothing may be sent. - let (sockets, handle) = (self.io.sockets.clone(), self.handle.get()); + let (network, handle) = (self.io.network.clone(), self.handle.get()); self.io.until(move || { - let mut sockets = sockets.borrow_mut(); - let socket = sockets.get::(handle); + let mut network = network.borrow_mut(); + let socket = network.get_socket::(handle); socket.may_send() || socket.may_recv() })?; @@ -385,14 +394,14 @@ impl<'a> TcpListener<'a> { } pub fn close(&self) { - self.with_lower(|mut s| s.close()) + self.with_lower(|s| s.close()) } } impl<'a> Drop for TcpListener<'a> { fn drop(&mut self) { - self.with_lower(|mut s| s.close()); - self.io.sockets.borrow_mut().release(self.handle.get()) + self.with_lower(|s| s.close()); + self.io.network.borrow_mut().remove_socket(self.handle.get()); } } @@ -416,9 +425,9 @@ impl<'a> TcpStream<'a> { } fn with_lower(&self, f: F) -> R - where F: FnOnce(SocketRef) -> R { - let mut sockets = self.io.sockets.borrow_mut(); - let result = f(sockets.get(self.handle)); + where F: FnOnce(&mut TcpSocketLower) -> R { + let mut network = self.io.network.borrow_mut(); + let result = f(network.get_socket(self.handle)); result } @@ -455,7 +464,7 @@ impl<'a> TcpStream<'a> { } pub fn set_timeout(&self, value: Option) { - self.with_lower(|mut s| s.set_timeout(value.map(Duration::from_millis))) + self.with_lower(|s| s.set_timeout(value.map(Duration::from_millis))) } pub fn keep_alive(&self) -> Option { @@ -463,11 +472,11 @@ impl<'a> TcpStream<'a> { } pub fn set_keep_alive(&self, value: Option) { - self.with_lower(|mut s| s.set_keep_alive(value.map(Duration::from_millis))) + self.with_lower(|s| s.set_keep_alive(value.map(Duration::from_millis))) } pub fn close(&self) -> Result<(), Error> { - self.with_lower(|mut s| s.close()); + self.with_lower(|s| s.close()); until!(self, TcpSocketLower, |s| !s.is_open())?; // right now the socket may be in TIME-WAIT state. if we don't give it a chance to send // a packet, and the user code executes a loop { s.listen(); s.read(); s.close(); } @@ -481,23 +490,33 @@ impl<'a> Read for TcpStream<'a> { fn read(&mut self, buf: &mut [u8]) -> Result { // Only borrow the underlying socket for the span of the next statement. - let result = self.with_lower(|mut s| s.recv_slice(buf)); + let result = self.with_lower(|s| s.recv_slice(buf)); match result { // Slow path: we need to block until buffer is non-empty. Ok(0) => { until!(self, TcpSocketLower, |s| s.can_recv() || !s.may_recv())?; - match self.with_lower(|mut s| s.recv_slice(buf)) { + match self.with_lower(|s| s.recv_slice(buf)) { Ok(length) => Ok(length), + Err(NetworkError::Finished) | Err(NetworkError::Illegal) => Ok(0), - _ => unreachable!() + Err(e) => { + panic!("Unexpected error from smoltcp: {}", e); + } } } // Fast path: we had data in buffer. Ok(length) => Ok(length), + // We've received a fin. + Err(NetworkError::Finished) | // Error path: the receive half of the socket is not open. Err(NetworkError::Illegal) => Ok(0), // No other error may be returned. - Err(_) => unreachable!() + Err(e) => { + // This could return Err(Error::Network(e)) rather than panic, + // but I expect that'll just cause a panic later perhaps with + // less interesting context. + panic!("Unexpected error from smoltcp: {}", e); + } } } } @@ -508,12 +527,12 @@ impl<'a> Write for TcpStream<'a> { fn write(&mut self, buf: &[u8]) -> Result { // Only borrow the underlying socket for the span of the next statement. - let result = self.with_lower(|mut s| s.send_slice(buf)); + let result = self.with_lower(|s| s.send_slice(buf)); match result { // Slow path: we need to block until buffer is non-full. Ok(0) => { until!(self, TcpSocketLower, |s| s.can_send() || !s.may_send())?; - match self.with_lower(|mut s| s.send_slice(buf)) { + match self.with_lower(|s| s.send_slice(buf)) { Ok(length) => Ok(length), Err(NetworkError::Illegal) => Ok(0), _ => unreachable!() @@ -540,7 +559,7 @@ impl<'a> Write for TcpStream<'a> { impl<'a> Drop for TcpStream<'a> { fn drop(&mut self) { - self.with_lower(|mut s| s.close()); - self.io.sockets.borrow_mut().release(self.handle) + self.with_lower(|s| s.close()); + self.io.network.borrow_mut().remove_socket(self.handle); } }