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

New device trait design #57

Merged
merged 33 commits into from
Nov 3, 2017
Merged

Conversation

phil-opp
Copy link
Contributor

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

This changes the design of the phy::Device trait as proposed here: #49 (comment)

Copy link
Contributor

@whitequark whitequark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this PR overall 👍 Just a few minor issues. Also, can you please put spaces before } and after {, like in let Self { foo, bar } = self; ?

@@ -477,7 +480,7 @@ impl<'a, 'b, 'c, DeviceT: Device + 'a> Interface<'a, 'b, 'c, DeviceT> {
}
}

fn dispatch(&mut self, timestamp: u64, packet: Packet) -> Result<()> {
fn dispatch<T: TxToken>(&mut self, tx_token: T, timestamp: u64, packet: Packet) -> Result<()> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can just be tx_token: DeviceT::TxToken

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method is defined on InterfaceInner, which has no generic DeviceT type (since it's device independent).

self.inner.receive().map(|(rx_token, tx_token)| {
let rx = RxToken { token: rx_token, writer: self.writer };
let tx = TxToken { token: tx_token, writer: self.writer };
(rx, tx) // TODO is copying `writer` desired?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

writer is a thin pointer so why not

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense!

src/phy/mod.rs Outdated
@@ -239,7 +239,7 @@ pub trait Device {
}

pub trait RxToken {
fn consume<R, F: FnOnce(&[u8]) -> R>(self, timestamp: u64, f: F) -> R;
fn consume<R, F: FnOnce(&[u8]) -> Result<R>>(self, timestamp: u64, f: F) -> Result<R>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we want to return an Err from some middleware consume functions. One example is tap_interface:

fn consume<R, F: FnOnce(&[u8]) -> Result<R>>(self, _timestamp: u64, f: F) -> Result<R> {
    []
   match lower.recv(&mut buffer[..]) {
       Ok(size) => []
       Err(ref err) if err.kind() == io::ErrorKind::WouldBlock => {
            Err(Error::Exhausted)
        }
        Err(err) => panic!("{}", err)
    }
}

If we returned R, we would need to call f with a dummy packet in case of error to get a valid return type.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we also give TxToken::consume similar treatment, for the same reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I implemented it

match self.buffer {
Some(ref mut buf) => buf.as_mut(),
None => &mut self.junk[..self.length]
if drop {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this condition inverted? I think you can just move it into the else branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The else branch happens below. This is the case for when the packet should be dropped. The problem is that consume doesn't return a Result but a plain R, so we need some R to return. So we let f construct its packet in the junk buffer (which is thrown away afterwards) and return its return value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an alternative we could change the return type of TxToken::consume to Result<R> (like for RxToken::consume). Then we could return an Err without constructing a junk packet.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this seems sensible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed TxToken::consume to also return a Result<R>, but I think it's still better to keep the junk buffer. The reason is that a packet drop on send should be transparent to the sender and not return an Err.

fn receive(&mut self, timestamp: u64) -> Result<Self::RxBuffer> {
let mut buffer = self.inner.receive(timestamp)?;
// TODO clone state on each transmit/receive?
// use refcell/mutex alternatively?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need a mutex because state is mutable. Without a mutex it will always perform the same action on every packet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I thought so. Does it have to be thread safe (i.e. mutex or spinlock) or does a RefCell suffice? (I use a RefCell for now, but I'm happy to change it)

In the current code, state is cloned on every transmit: https://github.com/m-labs/smoltcp/blob/bbb54fe2a2f35e05fd3a61d2f9fa81a2b7caabe3/src/phy/fault_injector.rs#L236

I didn't keep that behavior and used a RefCell for this, too. Or was this behavior intended?

lower: D,
sink: S,
mode: PcapMode,
phantom: PhantomData<&'a ()>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really needed? I thought it's enough that Device is using 'a.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't present anymore in the latest revision. So it seems like githubs "outdated` marker was right once :D.

@@ -25,13 +24,12 @@ use super::ArpCache;
/// The network interface logically owns a number of other data structures; to avoid
/// a dependency on heap allocation, it instead owns a `BorrowMut<[T]>`, which can be
/// a `&mut [T]`, or `Vec<T>` if a heap is available.
pub struct Interface<'a, 'b, 'c, 'd: 'a, DeviceT: Device<'d> + 'a> {
pub struct Interface<'a, 'b, 'c, DeviceT: for<'d> Device<'d> + 'a> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, this also works. Sweet, this is the 1st time I find higher-ranker lifetimes useful, personally.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Me too :). I think it's even necessary here because transmit/receive take &'a mut self to support tokens with a lifetime. Without the higher-ranked lifetime, transmit would always borrow for the fixed 'd lifetime defined in Interface, which means that the borrow lasts for the whole device lifetime. So we could only transmit once. With higher-ranked lifetimes, arbitrarily short borrows are possible (because DeviceT must implement Device for all possible lifetimes).

Some(TxToken {
queue: self.0.clone(),
queue: &mut self.queue,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Pays off already!

Interface { device, arp_cache, ethernet_addr, ip_addrs, ipv4_gateway}
}
let caps = device.capabilities();
let inner = InterfaceInner::new(arp_cache, ethernet_addr, ip_addrs, ipv4_gateway, caps);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an implementation detail so just initialize it with struct initialization syntax. No need for a separate new method etc.

inner: InterfaceInner<'b, 'c>,
}

struct InterfaceInner<'b, 'c> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should document the reason for InterfaceInner's existence.

@whitequark
Copy link
Contributor

@phil-opp looks like github has marked a few comments as "outdated" where I didn't intend that, so please expand them all.

Copy link
Contributor

@whitequark whitequark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no major issues with the PR \o/ Style nit: can we use where Tx: phy::TxToken, Rx: phy::RxToken (note the type variable names) consistently? T suggests a container.

What's remaining here? I'd like to merge it ASAP.

}
};
processed_any = true;
let &mut Self { ref mut device, ref mut inner } = self;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious, is this really the only way to write this? Or would direct access to self.device and self.inner also work? No objection to this pattern anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know of a better way. Direct access to self.* does not work, since the compiler then tries to borrow the complete self in the closure.

@phil-opp
Copy link
Contributor Author

phil-opp commented Oct 25, 2017

What's remaining here? I'd like to merge it ASAP.

There is a lifetime problem when nesting middleware types. I encountered it when I tried to fix the tests.

One example of a problematic function is parse_middleware_options, which returns a FaultInjector<EthernetTracer<PcapWriter<D, Rc<PcapSink>>>>. The error is rather cryptic:

expected bound lifetime parameter 'b, found concrete lifetime

I constructed a minimal example and it seems like it has to do with the associated type containing a lifetime. I'm still trying to understand the problem, so that I can fix it.

@phil-opp
Copy link
Contributor Author

Found the problem and was able to fix it 🎉!

(Documentation for myself: annotated debug source, solution)

I will fix some remaining issues and then push the corrected version.

Sorry, something went wrong.

@phil-opp phil-opp changed the title [WIP] New device trait design New device trait design Oct 25, 2017
@phil-opp
Copy link
Contributor Author

I rebased the PR, renamed the generic parameters (Rx/Tx instead of T for phy::{Rx, Tx}Token), and updated the documentation. It compiles and all tests pass, so I think this is ready for merging/final review.

(Note that I only checked that the examples compile, I didn't execute them).

Sorry, something went wrong.

src/phy/mod.rs Outdated
fn capabilities(&self) -> DeviceCapabilities {
let mut caps = DeviceCapabilities::default();
caps.max_transmission_unit = 1536;
caps.max_burst_size = Some(2);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has only a single buffer so max_burst_size should be Some(1), to cap the receive window.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Copy link
Contributor

@whitequark whitequark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am very happy with this PR. I think after the Result change it can be merged.

@phil-opp phil-opp force-pushed the new-device-trait branch 2 times, most recently from 60db0f0 to b8f9d35 Compare October 29, 2017 01:49
@phil-opp
Copy link
Contributor Author

Rebased and fixed new interface tests

Copy link
Collaborator

@dlrobertson dlrobertson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took a look to see what sort of changes I'll need to make to some stuff I'm working on now and noticed a typo

src/phy/mod.rs Outdated
type TxBuffer: AsRef<[u8]> + AsMut<[u8]>;
/// The interface is based on _tokens_, which are types that allow to receive/transmit a
/// single packet. The `receive` and `transmit` functions only construct such tokens, the
/// real sending/receiving operation are performed performed when the tokens are
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

performed performed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, fixed!

@whitequark
Copy link
Contributor

Can you please rebase again?

Also: Remove Copy trait from State to avoid accidental copying.
The problem was that the DR/DT types are concrete types with a specific lifetime, so that the where bound required that the associated types of D are always of type DR/DT for all possible lifetimes 'b. This is not the case since the lifetimes of the associated types depend on the trait lifetime. Thus, the device trait was not implemented for some nested types.

(For a minimal code example demonstrating the problem, see [1] [2] [3].)

[1]: https://play.rust-lang.org/?gist=c2e947e3c18a983042e2fb44749ace89&version=stable
[2]: https://play.rust-lang.org/?gist=2da7d2f3d9ffdade1079bc102ed3ba83&version=stable
[3]: https://play.rust-lang.org/?gist=23f5bdd67b75fafe7e4e8cedcc9185ce&version=stable
@phil-opp
Copy link
Contributor Author

phil-opp commented Nov 2, 2017

Done

@whitequark whitequark merged commit 198fe23 into smoltcp-rs:master Nov 3, 2017
@whitequark
Copy link
Contributor

Thanks a lot for your work on this!

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