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

Move device-specific parts of EthernetInterface to new EthernetDevice type #49

Closed

Conversation

phil-opp
Copy link
Contributor

@phil-opp phil-opp commented Oct 4, 2017

Also:

  • add a write_into_buffer and a Packet::required_buffer_size method
  • the device specific buffer allocation is moved to a EthernetDevice::dispatch method
  • instead of allocating the buffer on demand, it is passed when calling the write_into_buffer method
  • lookup_hardware_addr no longer sends ARP request packets. Instead it returns a new UnknownEthernetAddress error, which is caught by the EthernetDevice::dispatch method, which in turn sends an ARP request.
  • make Packet and process_frame (named process_ethernet before) public
  • make ChecksumCapabilities Copy (I see no reason why it shouldn't be Copy)
  • some renamings and documentation updates to adapt to the changes

The goal is to make EthernetInterface independent from the underlying device and allow one to use it without implementing the phy::Device trait. Feedback appreciated!

TODO:

  • Add getters/setters for device capabilities on EthernetInterface

cc @oli-obk

Sorry, something went wrong.

@phil-opp phil-opp force-pushed the split-ethernet-interface branch from cce219e to 0415463 Compare October 4, 2017 16:31
@whitequark
Copy link
Contributor

You'll need to motivate these changes significantly better, because so far I don't understand the rationale for most of them.

make ChecksumCapabilities Copy (I see no reason why it shouldn't be Copy)

It's passed around a lot, and it seems like a completely pointless structure to copy.

The goal is to make EthernetInterface independent from the underlying device and allow one to use it without implementing the phy::Device trait. Feedback appreciated!

So... why do you want that?

@phil-opp
Copy link
Contributor Author

phil-opp commented Oct 4, 2017

See embed-rs/net#1 (comment) for some reasons why we don't like the device trait in its current form. This change would allow us to reuse most parts of smoltcp (e.g. the TCP state machine, packet construction and processing) with our own device API.

I also think that the separation makes sense: The EthernetInterface is responsible for the [u8] <--> Packet conversion and the EthernetDevice for sending and receiving [u8] slices.

Sorry, something went wrong.

@whitequark
Copy link
Contributor

whitequark commented Oct 4, 2017

See embed-rs/net#1 (comment) for some reasons why we don't like the device trait in its current form. This change would allow us to reuse most parts of smoltcp (e.g. the TCP state machine, packet construction and processing) with our own device API.

I have a few issues with this approach. I've mentioned before that I really don't like the Device trait as it exists before, right? Well, I've just figured out how I want to do it.

Beyond that, I think that the way you've done the split doesn't result in an interface that makes sense in the context of design choices of smoltcp.

  • ARP is an integral part of IPv4-over-Ethernet and cannot be meaningfully separated.
  • If I tried to split EthernetInterface, which I want at some point, I would split off the IP parts so that they can be used on top of PPPoX, L2TP, Ethernet, SLIP (we actually might need this ourselves).
  • The new hyper-specific error is kind of a hack.

In the future, please discuss such changes with me first, this will save both of us time and effort.


In a nutshell, the new design for the Device trait that I have in mind is presented below:
mod phy {
    pub trait Device<'a> {
        type RxToken: RxToken;
        type TxToken: TxToken;

        fn receive(&'a mut self) -> Option<(Self::RxToken, Self::TxToken)>;
        fn transmit(&'a mut self) -> Option<Self::TxToken>;
    }

    pub trait RxToken {
        fn consume<R, F: FnOnce(&[u8]) -> R>(self, f: F) -> R;
    }

    pub trait TxToken {
        fn consume<R, F: FnOnce(&mut [u8]) -> R>(self, len: usize, f: F) -> R;
    }
}

struct StmPhy {
    rx_buffer: [u8; 1536],
    tx_buffer: [u8; 1536],
}

impl<'a> StmPhy {
    fn new() -> StmPhy {
        StmPhy {
            rx_buffer: [0; 1536],
            tx_buffer: [0; 1536],
        }
    }
}

impl<'a> phy::Device<'a> for StmPhy {
    type RxToken = StmPhyRxToken<'a>;
    type TxToken = StmPhyTxToken<'a>;

    fn receive(&'a mut self) -> Option<(Self::RxToken, Self::TxToken)> {
        Some((StmPhyRxToken(&mut self.rx_buffer[..]),
              StmPhyTxToken(&mut self.tx_buffer[..])))
    }

    fn transmit(&'a mut self) -> Option<Self::TxToken> {
        Some(StmPhyTxToken(&mut self.tx_buffer[..]))
    }
}

struct StmPhyRxToken<'a>(&'a [u8]);

impl<'a> phy::RxToken for StmPhyRxToken<'a> {
    fn consume<R, F: FnOnce(&[u8]) -> R>(self, f: F) -> R {
        let ret = f(self.0);
        println!("rx called");
        ret
    }
}

struct StmPhyTxToken<'a>(&'a mut [u8]);

impl<'a> phy::TxToken for StmPhyTxToken<'a> {
    fn consume<R, F: FnOnce(&mut [u8]) -> R>(self, len: usize, f: F) -> R {
        let ret = f(&mut self.0[..len]);
        println!("tx called {}", len);
        ret
    }
}

fn main() {
    use phy::{Device, TxToken, RxToken};

    let mut phy = StmPhy::new();
    if let Some(tx_token) = phy.transmit() {
        tx_token.consume(40, |buf| {
            println!("got tx buf len {}", buf.len());
        });
    }
    if let Some((rx_token, tx_token)) = phy.receive() {
        rx_token.consume(|buf| {
            println!("got rx buf");
        });
        tx_token.consume(80, |buf| {
            println!("got tx buf len {}", buf.len());
        });
    }
}

This has some desirable properties:

  • That for every received packet there is always space to transmit one. (This is actually already assumed, just not documented.)
  • That you can only use each token once.
  • Interposer devices can now run code before and after the inner device, which is a major source of headache in the existing fault injector.

@phil-opp
Copy link
Contributor Author

phil-opp commented Oct 5, 2017

In the future, please discuss such changes with me first, this will save both of us time and effort.

Well, sometimes it's difficult to anticipate how certain changes would work out without implementing them.

I already tried to change smoltcp to a closure based API in this branch. It worked out very well, but it relies on the assumption that it's always possible to cleanly separate the TX and RX halves of a device. For our use case, this assumption was fine, but I assumed that it wouldn't be for smoltcp. Therefore I tried to come of with a smaller set of changes that would allow us to implement such an API externally.

Your new design exploits the same idea (separating the TX and RX halves), so I'm really happy about it. I think it would solve most of our problems.

@whitequark
Copy link
Contributor

Well, sometimes it's difficult to anticipate how certain changes would work out without implementing them.

Yes, but I've already considered, if briefly, most of the challenges smoltcp faces. It's not really documented anywhere, though, hence, discussion.

@whitequark
Copy link
Contributor

Oh, and please create a meta-issue like the redox people did for whatever it is you want to use smoltcp for. It's handy for me to have a bird's eye view.

@whitequark
Copy link
Contributor

@phil-opp Do you think you can take the abovementioned outline and implement this yourself? I'm fairly busy right now.

@phil-opp
Copy link
Contributor Author

@phil-opp Do you think you can take the abovementioned outline and implement this yourself? I'm fairly busy right now.

Sure, I will give it a try.

@batonius
Copy link
Contributor

Should we coordinate this effort with #55? It would be great if the new Device trait or the new Interface struct could be used as trait objects.

@whitequark
Copy link
Contributor

@batonius I disagree with your approach on #55, but haven't had the time to write that down yet. In general I am quite opposed to trait objects in smoltcp; you can notice that in the existing code...

@batonius
Copy link
Contributor

@whitequark Sure, I just think we should keep the issues in sync.

@whitequark
Copy link
Contributor

Superseded by #57 I think.

@whitequark whitequark closed this Oct 24, 2017
phil-opp added a commit to phil-opp/smoltcp that referenced this pull request Oct 25, 2017
phil-opp added a commit to phil-opp/smoltcp that referenced this pull request Oct 29, 2017
phil-opp added a commit to phil-opp/smoltcp that referenced this pull request Nov 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants