Skip to content

Commit

Permalink
net: remove MAC RX activity watchdog.
Browse files Browse the repository at this point in the history
Now that #750 is closed and we're pretty confident in the fix, we can
stop rebooting the netstack periodically. This should save resources (by
not constantly dumping and restarting), and also get us back into the
state where any task's generation number advancing is Probably Bad.
  • Loading branch information
cbiffle committed Feb 21, 2024
1 parent 37ed81c commit e062735
Show file tree
Hide file tree
Showing 4 changed files with 4 additions and 58 deletions.
31 changes: 2 additions & 29 deletions task/net/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,6 @@ mod idl {
use core::sync::atomic::{AtomicU32, Ordering};
use enum_map::Enum;
use multitimer::{Multitimer, Repeat};
use task_jefe_api::Jefe;
use task_net_api::MacAddressBlock;
use zerocopy::{AsBytes, U16};

Expand Down Expand Up @@ -148,10 +147,6 @@ const TX_RING_SZ: usize = 4;

const RX_RING_SZ: usize = 4;

/// How long to wait with no received packets before we decide the driver is
/// b0rked and restart it.
const RX_WATCHDOG_INTERVAL: u64 = 60_000;

/////////////////////////////////////////////////////////////////////////////
// Main driver loop.

Expand All @@ -163,7 +158,6 @@ static ITER_COUNT: AtomicU32 = AtomicU32::new(0);
fn main() -> ! {
let sys = SYS.get_task_id();
let sys = Sys::from(sys);
let jefe = Jefe::from(JEFE.get_task_id());

// Do any preinit tasks specific to this board. For hardware which requires
// explicit clock configuration, this is where the `net` tasks waits for
Expand Down Expand Up @@ -221,11 +215,11 @@ fn main() -> ! {
// Turn on our IRQ.
userlib::sys_irq_control(notifications::ETH_IRQ_MASK, true);

// We use two timers:
// We use only one timer, but we're using a multitimer in case we need to
// add a second one again (we previously had a second one for the watchdog):
#[derive(Copy, Clone, Enum)]
enum Timers {
Wake,
Watchdog,
}
let mut multitimer =
Multitimer::<Timers>::new(notifications::WAKE_TIMER_BIT);
Expand All @@ -242,9 +236,6 @@ fn main() -> ! {
);
}

// Start the watchdog timer running.
multitimer.set_timer(Timers::Watchdog, now + RX_WATCHDOG_INTERVAL, None);

// Go!
loop {
ITER_COUNT.fetch_add(1, Ordering::Relaxed);
Expand All @@ -253,18 +244,6 @@ fn main() -> ! {
let now = sys_get_timer().now;
let activity = server.poll(now);

if activity.mac_rx {
// Whenever we observe activity we bump the timer forward. Because
// we're going to poll the timer below (after doing this) and we
// always poll and immediately consume iter_fired, this will prevent
// the timer from firing this iteration.
multitimer.set_timer(
Timers::Watchdog,
now + RX_WATCHDOG_INTERVAL,
None,
);
}

if activity.ip {
// Ask the server to iterate over sockets looking for work
server.wake_sockets();
Expand All @@ -276,9 +255,6 @@ fn main() -> ! {
server.wake();
// timer is set to auto-repeat
}
Timers::Watchdog => {
jefe.restart_me();
}
}
}
let mut msgbuf = [0u8; idl::INCOMING_SIZE];
Expand All @@ -291,9 +267,6 @@ fn main() -> ! {
pub(crate) struct Activity {
/// Did the IP stack do anything? (i.e. do we need to process socket events)
ip: bool,
/// Did the MAC have anything available to receive? (i.e. is it still
/// working)
mac_rx: bool,
}

/// We can map an Ethernet MAC address into the IPv6 space as follows.
Expand Down
6 changes: 1 addition & 5 deletions task/net/src/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -242,8 +242,6 @@ where
}

pub trait DeviceExt: smoltcp::phy::Device {
fn read_and_clear_activity_flag(&self) -> bool;

fn make_meta(
&self,
port: u16,
Expand Down Expand Up @@ -444,19 +442,17 @@ where
// Do not be tempted to use `Iterator::any` here, it short circuits and
// we really do want to poll all of them.
let mut ip = false;
let mut mac_rx = false;
for vlan in &mut self.vlan_state {
ip |= vlan.iface.poll(
instant,
&mut vlan.device,
&mut vlan.socket_set,
);
// Test and clear our receive activity flag.
mac_rx |= vlan.device.read_and_clear_activity_flag();
ip |= vlan.check_socket_watchdog();
}

crate::Activity { ip, mac_rx }
crate::Activity { ip }
}

/// Iterate over sockets, waking any that can do work.
Expand Down
17 changes: 1 addition & 16 deletions task/net/src/server_basic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ use crate::{
server::{DeviceExt, GenServerImpl, Storage},
MacAddressBlock,
};
use core::cell::Cell;
use mutable_statics::mutable_statics;
use task_net_api::UdpMetadata;

Expand Down Expand Up @@ -55,15 +54,11 @@ where

pub struct Smol<'d> {
eth: &'d eth::Ethernet,
mac_rx: Cell<bool>,
}

impl<'d> From<&'d eth::Ethernet> for Smol<'d> {
fn from(eth: &'d eth::Ethernet) -> Self {
Self {
eth,
mac_rx: Cell::new(false),
}
Self { eth }
}
}

Expand Down Expand Up @@ -105,12 +100,6 @@ impl<'a> smoltcp::phy::Device for Smol<'a> {
// Note that the can_recv and can_send checks remain valid because
// the token mutably borrows the phy.
if self.eth.can_recv() && self.eth.can_send() {
// We record this as "data available from the MAC" because it's
// sufficient to catch the bug we're defending against with the
// watchdog, even if the IP stack decides not to consume the token
// for some reason (that'd be a software bug instead).
self.mac_rx.set(true);

Some((OurRxToken(self.eth), OurTxToken(self.eth)))
} else {
None
Expand All @@ -134,10 +123,6 @@ impl<'a> smoltcp::phy::Device for Smol<'a> {
}

impl DeviceExt for Smol<'_> {
fn read_and_clear_activity_flag(&self) -> bool {
self.mac_rx.take()
}

fn make_meta(
&self,
port: u16,
Expand Down
8 changes: 0 additions & 8 deletions task/net/src/server_vlan.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@

use drv_stm32h7_eth as eth;

use core::cell::Cell;
use mutable_statics::mutable_statics;
use task_net_api::UdpMetadata;

Expand All @@ -32,7 +31,6 @@ fn claim_server_storage_statics() -> &'static mut [Storage; VLAN_COUNT] {
pub struct VLanEthernet<'a> {
pub eth: &'a eth::Ethernet,
pub vid: u16,
mac_rx: Cell<bool>,
}

impl<'a> smoltcp::phy::Device for VLanEthernet<'a> {
Expand All @@ -44,7 +42,6 @@ impl<'a> smoltcp::phy::Device for VLanEthernet<'a> {
_timestamp: smoltcp::time::Instant,
) -> Option<(Self::RxToken<'a>, Self::TxToken<'a>)> {
if self.eth.vlan_can_recv(self.vid, VLAN_RANGE) && self.eth.can_send() {
self.mac_rx.set(true);
Some((
VLanRxToken(self.eth, self.vid),
VLanTxToken(self.eth, self.vid),
Expand All @@ -69,10 +66,6 @@ impl<'a> smoltcp::phy::Device for VLanEthernet<'a> {
}

impl DeviceExt for VLanEthernet<'_> {
fn read_and_clear_activity_flag(&self) -> bool {
self.mac_rx.take()
}

fn make_meta(
&self,
port: u16,
Expand Down Expand Up @@ -133,7 +126,6 @@ where
|i| VLanEthernet {
eth,
vid: generated::VLAN_RANGE.start + i as u16,
mac_rx: Cell::new(false),
},
)
}

0 comments on commit e062735

Please sign in to comment.