af_packet: zero copy is broken

[styleex]

In commit 9d0c2435c1e2a38eef8f8d67e0377212388f8d0d was added code that creates a new buffer in memory for each package:

https://github.com/google/gopacket/commit/9d0c2435c1e2a38eef8f8d67e0377212388f8d0d#diff-4b6810f6883b99dcc5dcebf00351c417R69

[styleex]

I now checked it on my virtual machine: when rx(tx)-vlan-offloading is disabled linux gives a full ethernet package, including information about vlan. And gopacket reads it correctly:

PACKET LAYER: Ethernet
PACKET LAYER: Dot1Q
PACKET LAYER: IPv4
PACKET LAYER: ICMPv4
PACKET LAYER: Payload

Accordingly, the question: why did you choose to implement such a hack, instead of disabling the unnecessary function of the network card. In this case, this behavior of linux is documented.

My suggestion: instead of this hack document the need to disable vlan-offloading on the network card.

From the pros: for vlan traffic will also be zero-copy reading packets.

[gconnell]

Happy to accept a fix for this, if you'd like to send a pull :)

[safchain]

sorry for the delay, I was on vacation.

I do understand your concern about the performances but I think that is a bit weird to change the configuration of something you want to monitor/troubleshoot, moreover the kernel gives the information so I think it's worth it to provide it. Why not capturing the untagged interfaces ? I would rather add an option to enable vlan header insertion or adding the vlan info as part of the capture info ?

[gconnell]

Hey, Антон and Sylvain,

The overall point of this library is to do the right thing as much as possible, even if/when things aren't otherwise configured to exact specifications. That said, we also want fidelity of packets to be a high priority, and it seems like disabling rx-vlan-offloading might aid in that (and keep performance better by avoiding a packet-copy). Therefore, from my understanding of things, I think the best approach is:

1) keep adding in the vlan header if it's not captured but is available in the AF_PACKET info, as we do right now 2) explore auto-disabling rx/tx-vlan-offloading, and if it has no other impact on performance, switch it off by default and disable the code from 1 3) regardless, in cases where rx/tx-vlan-offloading is turned off, make sure that we don't double-add VLAN headers

Does that sound good to folks?

--Graeme

PS: Please avoid using words like "hack" when describing any code that isn't your own. While I'm sure it wasn't meant to be derogatory, it can be read that way. Everyone's contributing code here because they want things to be improved, and they're all volunteers. :)

[styleex]

I think that adding extra headers in this case is incorrect (as long as the user did not say this explicitly), because in the documentation for this binding, it is written that mmap is used to access the package data. That on all manuals to the kernel speaks about access to a package without copying. The behavior with vlan shows a completely different model.

In my opinion the most correct solution would be:

1. Detect the presence of the included vlan offloading and log this event as "warning".
2. Make a switch (and preferably a separate API) to access the packages with the corrected headers.
3. In the documentation we describe a problem with vlan for linux. On the Internet, even links to the discussion of this point in the kernel code, which you can attach to the documentation (I can find).

Next: about reading from the vlan interface. Thought is good, but only as long as the network does not walk dozens of different vlan, which are not known in advance. In my case, things are the same, creating a bunch of interfaces instead of one there is no desire.

[styleex]

Auto-disabling vlan-offloading is, in my opinion, a bad idea, because potentially can affect the performance of the system as a whole, although not significantly (as far as I know). However, this change in hardware parameters. even linux does not do this automatically, but adapts itself to the hardware configuration (although it can work with vlan).

Also, the ability to change this parameter may depend on the drivers and the hardware itself (for example, virtio drivers can not enable vlan offloading).

Therefore, auto-disabling of this function is not possible on all hardware configurations.

P.S. yes, I can do the necessary PR, but first I want to agree on the architecture of the future solution

[gconnell]

I've got an idea for a quick solution to this: how about just adding an afpacket option to re-add the vlan header? We've already got plenty of other options, what's one more?

I'm leaning towards defaulting this to false (don't re-add VLAN), allowing afpacket users to turn it on if they wish. That makes the more performant (but slightly less intuitive) option the default.

I've got a PR I'm working on now to handle things in this way. Just experimenting with it now.

[gconnell]

Comments on https://github.com/google/gopacket/pull/447 appreciated. If noone objects strongly, I'll pull it in on Monday.

[notti]

447 was merged, therefore this was resolved.