Fraggle rock

[wingo]

A beginning to a solution to #180. The perf seems to be 3.49+3.49 MPPS, which is a little worse than what I was hoping for. There is still an end-to-end.sh bug regarding DF in the tunneled IPv4 packet; the code I made punts, but the current code in the lwaftr sends an ICMPv4 error message. Perhaps @aperez or @kbara knows if the current code is the right thign.

[dpino]

LGTM.

[kbara]

The new ICMP packets violate the documentation we provide on our RFC compliance:

https://github.com/Igalia/snabbswitch/blob/lwaftr_olive/src/program/snabb_lwaftr/doc/README.rfccompliance.md

ICMPv4 packets are [RFC 1812](https://tools.ietf.org/html/rfc1812) compliant:
that is, they contain up to 576 bytes, rather than having an
[RFC 792](https://tools.ietf.org/html/rfc792) style IPv4 header + 8 octets
payload.

As RFC 1222 says:

In general, no host is required to accept an IP
 datagram larger than 576 bytes (including header and
 data), so a host must not send a larger datagram
 without explicit knowledge or prior arrangement with
 the destination host. 

---

The differences between icmpv4-fromlwaftr-replyto-tcp-frominet-bound1494-DF.pcap in lwaftr_olive and in this branch, in order, are:

• Two pcap header bytes; this is (probably) a consequence of the content.
• Length; this is a genuine problem
• IP and ICMP checksums (3 bytes differ): this is a side-effect of length

---

Given that end-to-end.sh passes and this branch has two new pcap files, I'm presuming that those are to avoid the test failures. What did you mean in the first comment about your code punting?

[kbara]

Is this perhaps the root cause of the too-large-ICMP-packet bug I mentioned in the 'conversation' tab?

[kbara]

Aside from that, nice work. I really like separating out the fragmentation handling - that should be pretty cheap. I'm less convinced about factoring out the vlan handling, or the overall performance hit. What does performance (for purely-unfragmented and purely-fragmented workloads) look like if fragmentation is factored out, but vlan handling is spread out like now rather than factored into an app?

[kbara]

Re: out-of-band discussion (thanks Andy), I'm now convinced that this branch's length handling is superior, because the 576 byte limit should be on the IP datagram itself, not the whole packet. In fact, it could reasonably go farther, as https://tools.ietf.org/html/rfc1812 says:

Therefore, the ICMP
   datagram SHOULD contain as much of the original datagram as possible
   without the length of the ICMP datagram exceeding 576 bytes. 

This mentions the length of the ICMP datagram rather than IP datagram - though following it requires ignoring RFC 1222's warning (previous post). In practice, either interpretation is unlikely to cause trouble on the types of hosts that realistically exist on today's internet, as far as I know.

[wingo]

Regarding punting, my first branch failed tests. I force-pushed to update it before review started. Now it passes at each commit. 68feb75 changes the expectations without switching to the new vlan elements; the last patch switches to separate soft vlan taggers/untaggers, but only if the hardware can't do it for us. In practice I expect that in most cases we are interested in, hardware can untag (it's certainly the case for intel hardware running non-virtualized). I see no speed difference between using the hardware to untag and doing it inside the lwaftr itself (i.e., the code before this patch); though there are some maintenance benefits towards separating the vlan and lwaftr concerns, and it will possibly make performance improvements easier to make. For testing we obviously use the soft vlan taggers/untaggers though.

[kbara]

Ahh... I wondered if it was an up to date comment, thanks!

I'm a bit reluctant to rely on the hardware, especially if it's not giving us a speed increase. I think that's potentially asking for trouble when someone runs it on a different card. (I'm quite biased towards (my understanding of) the Snabb philosophy of "do it in software if it's not too expensive", rather than relying on offloading as much as possible to hardware).

[wingo]

Yeah I know what you mean! I think though that "the snabb way" to handle vlans is with separate apps. With the current packet structure though there's a not insignificant perf overhead, see #184 or snabbco#648 for discussion.

In short, VLAN handling in apps > VLAN handling in hardware > VLAN handling in unrelated elements like the lwAFTR. WDYT?

[kbara]

I strongly agree that VLAN handling in apps > VLAN handling in unrelated elements, though I'm less convinced that the latter is < VLAN handling in hardware. I had Luke's comment in #184 in mind when I said that. I'd really like to see if the offset-packet approach made the first feasible; I wish I had the hack time to find out.

[wingo]

Given that you think that vlan in apps >> vlan in lwaftr, I take that as an OK. Whether to use the NIC to handle VLANs is then orthogonal -- the current patch takes advantage of the hardware if present but doesn't require its use. Thanks for the feedback! Porting this patch to rambutan.

[wingo]

Closing in favor of #197. Olive is dead!

[kbara]

It's an ok, with the caveat that that's ideology, and performance may trump it. :-)