-
Notifications
You must be signed in to change notification settings - Fork 447
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
Conversation
There was a problem hiding this 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;
?
src/iface/ethernet.rs
Outdated
@@ -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<()> { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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).
src/phy/tracer.rs
Outdated
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? |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this necessary?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this seems sensible.
There was a problem hiding this comment.
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
.
src/phy/fault_injector.rs
Outdated
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? |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
src/phy/pcap_writer.rs
Outdated
lower: D, | ||
sink: S, | ||
mode: PcapMode, | ||
phantom: PhantomData<&'a ()>, |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
src/iface/ethernet.rs
Outdated
@@ -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> { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
src/phy/loopback.rs
Outdated
Some(TxToken { | ||
queue: self.0.clone(), | ||
queue: &mut self.queue, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Pays off already!
src/iface/ethernet.rs
Outdated
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); |
There was a problem hiding this comment.
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.
src/iface/ethernet.rs
Outdated
inner: InterfaceInner<'b, 'c>, | ||
} | ||
|
||
struct InterfaceInner<'b, 'c> { |
There was a problem hiding this comment.
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.
@phil-opp looks like github has marked a few comments as "outdated" where I didn't intend that, so please expand them all. |
There was a problem hiding this 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.
src/iface/ethernet.rs
Outdated
} | ||
}; | ||
processed_any = true; | ||
let &mut Self { ref mut device, ref mut inner } = self; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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
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. |
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. |
0e8205e
to
98a684f
Compare
I rebased the PR, renamed the generic parameters ( (Note that I only checked that the examples compile, I didn't execute them). |
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
There was a problem hiding this 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.
60db0f0
to
b8f9d35
Compare
Rebased and fixed new interface tests |
There was a problem hiding this 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
performed performed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, fixed!
Can you please rebase again? |
The problem is that the Rx/TxTokens borrow self until they're used. So we can't borrow self again for `dispatch`. By splitting the device off the rest of the Interface, only the device stays borrowed.
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
Example taken from smoltcp-rs#49 (comment)
3c8b95e
to
c99eb9e
Compare
Done |
Thanks a lot for your work on this! |
This changes the design of the
phy::Device
trait as proposed here: #49 (comment)