Conversation
Co-authored-by: benaadams <1142958+benaadams@users.noreply.github.com>
Co-authored-by: benaadams <1142958+benaadams@users.noreply.github.com>
Co-authored-by: benaadams <1142958+benaadams@users.noreply.github.com>
Co-authored-by: benaadams <1142958+benaadams@users.noreply.github.com>
Co-authored-by: benaadams <1142958+benaadams@users.noreply.github.com>
…sage Co-authored-by: benaadams <1142958+benaadams@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a new architecture guide documenting Magma’s internal layering, packet parsing/processing patterns, memory ownership model, integration with Kestrel transports, and performance-related implementation details.
Changes:
- Introduces
docs/ARCHITECTURE.mddescribing Magma’s layered architecture and core design patterns (TryConsume/type-punning, buffer ownership, checksum strategy). - Documents end-to-end receive/transmit flows with illustrative code and diagrams.
- Adds a performance considerations section with optimization rationale and benchmark-style summaries.
docs/ARCHITECTURE.md
Outdated
| var transport = new AF_XDPTransport("eth0"); | ||
| await foreach (var packet in transport.ReceivePacketsAsync()) | ||
| { | ||
| // Process raw packet bytes | ||
| ProcessPacket(packet.Memory.Span); | ||
| packet.Dispose(); | ||
| } |
There was a problem hiding this comment.
The “Low-Level (Direct Packet Access)” example uses new AF_XDPTransport("eth0") and transport.ReceivePacketsAsync(), but AF_XDPTransport in this repo is a Kestrel ITransport and doesn’t expose that constructor or a ReceivePacketsAsync API. Consider marking this as pseudocode or updating it to reflect the actual public surface area.
There was a problem hiding this comment.
Fixed in commit 4431a99. Replaced the example with a conceptual note clarifying that applications typically interact at the transport or receiver level, not directly with the low-level ring APIs.
docs/ARCHITECTURE.md
Outdated
| var receiver = new TcpTransportReceiver<AF_XDPTransport>( | ||
| endpoint, transport, dispatcher); |
There was a problem hiding this comment.
The “Mid-Level (Protocol Layers)” snippet constructs new TcpTransportReceiver<AF_XDPTransport>(...), but TcpTransportReceiver<TTransmitter> is constrained to TTransmitter : IPacketTransmitter and is instantiated in the codebase with transmit-ring types (e.g., AF_XDPTransmitRing / NetMapTransmitRing), not the AF_XDPTransport itself. Updating the example to use the correct generic type (or documenting how transports create receivers internally) would prevent confusion.
| var receiver = new TcpTransportReceiver<AF_XDPTransport>( | |
| endpoint, transport, dispatcher); | |
| // Obtain an IPacketTransmitter from your transport (e.g., AF_XDPTransmitRing) | |
| var receiver = new TcpTransportReceiver<AF_XDPTransmitRing>( | |
| endpoint, transmitter, dispatcher); |
There was a problem hiding this comment.
Fixed in commit 4431a99. Updated to use AF_XDPTransmitRing as the generic parameter and added a clarifying comment about how transports create receivers internally.
|
|
||
| **Custom Packet Processing:** | ||
|
|
||
| ```csharp |
There was a problem hiding this comment.
The “Custom Packet Processing” example uses PacketReceiver.CreateDefault() and sets receiver.IPv4Consumer, but PacketReceiver is internal (see src/Magma.Network/DefaultPacketReceiver.cs) and isn’t accessible to consumers of the library. Consider switching this example to a public type/API (or explicitly stating it’s an internal implementation detail).
| ```csharp | |
| ```csharp | |
| // Internal example: PacketReceiver is an internal implementation detail of Magma's built-in pipeline |
There was a problem hiding this comment.
Fixed in commit 4431a99. Replaced the example with a complete IPacketReceiver implementation and added a note that PacketReceiver is an internal implementation detail.
docs/ARCHITECTURE.md
Outdated
| - Structs use `Pack = 1` to match network byte order | ||
| - Input is always validated before type-punning | ||
| - Endianness conversion is explicit where needed |
There was a problem hiding this comment.
In the “Type-Punning with Unsafe Code” section, Pack = 1 is described as matching “network byte order”. Struct packing affects field alignment/padding, not endianness; byte order is handled separately via explicit conversions. Consider rewording to avoid implying Pack addresses endianness.
| - Structs use `Pack = 1` to match network byte order | |
| - Input is always validated before type-punning | |
| - Endianness conversion is explicit where needed | |
| - Structs use `Pack = 1` to ensure a tightly packed layout that matches the wire-format field layout (no padding) | |
| - Input is always validated before type-punning | |
| - Network byte order is handled via explicit endianness conversion where needed |
There was a problem hiding this comment.
Fixed in commit 4431a99. Clarified that Pack = 1 ensures tightly packed layout matching wire format (no padding between fields), and that network byte order (endianness) is handled separately via explicit conversion.
docs/ARCHITECTURE.md
Outdated
| Unsafe.SizeOf<TcpV4PseudoHeader>()); | ||
|
|
||
| // Later, calculate full checksum by adding TCP header + data | ||
| var checksum = Checksum.Calculate(ref tcpHeader, length, pseudoPartialSum); |
There was a problem hiding this comment.
The pseudo-header checksum example calls Checksum.Calculate(ref tcpHeader, length, pseudoPartialSum), but in the codebase Checksum.Calculate takes ref byte (e.g., ref Unsafe.As<Tcp, byte>(ref tcpHeader) or ref MemoryMarshal.GetReference(span)). As written, the snippet won’t compile and may mislead readers; please adjust it to match the actual API usage pattern.
| var checksum = Checksum.Calculate(ref tcpHeader, length, pseudoPartialSum); | |
| var checksum = Checksum.Calculate(ref Unsafe.As<Tcp, byte>(ref tcpHeader), length, pseudoPartialSum); |
There was a problem hiding this comment.
Fixed in commit 4431a99. Updated to use ref Unsafe.As<TcpV4PseudoHeader, byte>(ref tcpHeader) to match the actual API signature that takes ref byte.
|
@copilot apply changes based on the comments in this thread |
…d clarify internal types Co-authored-by: benaadams <1142958+benaadams@users.noreply.github.com>
All review comments have been addressed in commit 4431a99:
|
Co-authored-by: benaadams <1142958+benaadams@users.noreply.github.com>
Architecture Guide Documentation - Complete ✓
docs/ARCHITECTURE.md(880 lines) covering:Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.