doc/wg/network/notes/network-notes-2024-10-21.md
Branden: I made these notes after reviewing the PacketBuffer implementation and provided to Leon. Posting here for the future
General notes:
PacketBufferMut impl?? It would be really nice if this file made clear what the externally-usable calls are and what the internal crap is. I guess that's the pub keyword? But the impl of PacketSliceMut is pub too...Starting in PacketBufferDyn:
reclaim_headroom, the comment could be more clear here. I think it's taking space from the buffer and giving it to the headroom? Maybe a quick diagram. So false would mean that there wasn't enough buffer space?reclaim_tailroom, it will presumably not move past the headroom marker.reset, what does it do to tailroom? It's unclear to me how reset is different from reclaim_headroomcopy_from_slice_or_err and append_from_slice_max, those apply to the buffer data itself, right? They could use comments. I don't really understand what they do from the names alone. I'm also not sure why these two functions exist? What makes them the "proper" interface?prepend_unchecked operation? I guess the idea is that the checks are already happening at compile time before calling this?In impl PacketBufferMut:
reset, that's a runtime assert, right? Is that necessary?For PacketSliceMut:
_inner is a u8 here. And the comment above it does nothing to help that. It seems to never be used because we just refer to self instead. I kind of thought it would be [u8] since it's a transmuted array of data...?new, the comments don't seem to match the code about starting with zero headroom. I think it starts with payload length, headroom bytes of headroom, and length - headroom bytes of tailroom.get_inner_slice_length()? It seems like it should be pretty optimized, but I'm wondering if it actually happens in practice. Also that the panic isn't there.data_slice and data_slice_mut have slightly different constructionsIn impl PacketBufferDyn for PacketSliceMut:
len() correct? The comment there looks plausible, but the implementation looks wrong.copy_from_slice_or_err eat up tailroom? I assumed it would write into the payload and error if it didn't fit in the payload (slice.len() - headroom - tailroom).Leon: Looked through Branden's PacketBuffer feedback. Generally I'm pleased that the concept makes sense. The points you raised seemed correct.
Branden: Any of these worth having a discussion about?
Leon: Went through writing some docs and tests. There was a bug stopping things from being compile-time evaluated previously, which I had to fix. Added some internal complexity.
Leon: Another issue worth bringing up. There's a tradeoff that we are unable to provide information in the compile-time errors as to which call site caused the mismatch. The only information you get is where the assertion is located. You get file and line, but it's basically always the same file and line where the assertion is.
Leon: Let's give an example of this.
Leon: Explains what's going on in this example. It's a trick to force the compiler to monomorphize the const generics and then run an assertion on that, all at compile-time.
#[inline(always)]
pub fn reduce_headroom<const NEW_HEAD: usize>(self) -> PacketBufferMut<NEW_HEAD, TAIL> {
// Const items are global by default and do not have access to generic
// paramters of the other item (this `reduce_headroom` function). Hence,
// the following simple const-assertion doesn't work:
//
// ```
// const _: () = assert!(NEW_HEAD <= HEAD);
// ```
//
// We instead trick the compiler into generating a monomorphized
// instance of a variant-less enum (which ensures that it will not
// generate code by not being instantiable) that itself contains a
// const-assertion, using type parameters from the monomorphized
// instance instead. This monomorphized instance is still "global", but
// specialized to this particular monomorphized call site.
//
// Inspired by https://users.rust-lang.org/t/cant-use-type-parameters-of-outer-function-but-only-in-constant-expression/96023/2
enum AssertionHelper<const HEAD: usize, const NEW_HEAD: usize> {}
impl<const HEAD: usize, const NEW_HEAD: usize> AssertionHelper<HEAD, NEW_HEAD> {
const ASSERTION: () = assert!(
NEW_HEAD <= HEAD,
"{}", // This is special-cased to work in const expressions
concat!(
"reduce_headroom cannot increase the headroom of a PacketBufferMut<",
// TODO: insert actual `HEAD` value if / when `const_format_args!` is stabilized
"HEAD",
", _> to create a PacketBufferMut<",
// TODO: insert actual `NEW_HEAD` value if / when `const_format_args!` is stabilized
"NEW_HEAD",
", _>",
)
);
}
// Force monomorphization of the const assertion:
let _: () = AssertionHelper::<HEAD, NEW_HEAD>::ASSERTION;
PacketBufferMut { inner: self.inner }
}
Leon: Here's the error message that's generated from that.
---- kernel/src/utilities/packet_buffer.rs - utilities::packet_buffer::PacketBufferMut<HEAD,TAIL>::reduce_headroom (line 185) stdout ----
error[E0080]: evaluation of `kernel::utilities::packet_buffer::PacketBufferMut::<HEAD, TAIL>::reduce_headroom::AssertionHelper::<4, 6>::ASSERTION` failed
--> /home/leons/proj/tock/kernel/kernel/src/utilities/packet_buffer.rs:236:28
|
236 | const ASSERTION: () = assert!(
| _______________________________^
237 | | NEW_HEAD <= HEAD,
238 | | "{}", // This is special-cased to work in const expressions
239 | | concat!(
... |
247 | | )
248 | | );
| |_________^ the evaluated program panicked at 'reduce_headroom cannot increase the headroom of a PacketBufferMut<HEAD, _> to create a PacketBufferMut<NEW_HEAD, _>', /home/leons/proj/tock/kernel/kernel/src/utilities/packet_buffer.rs:236:31
|
= note: this error originates in the macro `$crate::panic::panic_2021` which comes from the expansion of the macro `assert` (in Nightly builds, run with -Z macro-backtrace for more info)
note: the above error was encountered while instantiating `fn kernel::utilities::packet_buffer::PacketBufferMut::<4, 0>::reduce_headroom::<6>`
--> kernel/src/utilities/packet_buffer.rs:197:51
|
15 | let larger_headroom_pb: PacketBufferMut::<6, 0> = pb.reduce_headroom();
| ^^^^^^^^^^^^^^^^^^^^
error: aborting due to 1 previous error
Leon: So, line 120 actually does show us the exact callsite where this fails. Which is nice and sufficient to figure out what was going wrong.
reset() is a good example of this. I can understand what it does internally (with some help), but I'm missing the context of why it exists and what you'd use it for