direct vlan handling in ixgbe

[ae]

Summary

The main idea of this patch is reducing an overhead that we have on RX path, when we use vlans.
Basically we have the following code path for input packet:

ixgbe_rx_input() -> ether_input() -> netisr_dispatch() -> ether_nh_input() -> ether_input_internal() -> ether_demux() -> vlan_input() -> ether_input() -> netisr_dispatch() -> ether_nh_input() -> ether_input_internal() -> ether_demux() -> netisr_dispatch() -> ip_input() ...

With this patch it becomes shorter:

ixgbe_rx_input() -> ether_input() -> netisr_dispatch() -> ether_nh_input() -> ether_input_internal() -> ether_demux() -> netisr_dispatch() -> ip_input() ...

[oleg]

1. Why is ck library used and not atomic(9) ?
2. yndx_vlan_set(): is that fence really required? May be store with release semantic is enough?

[ae]

In D12040#249728, @oleg wrote:

1. Why is ck library used and not atomic(9) ?

Why not? I'm going to use ck library in ipfw, and put related code here to reduce diffs.

2. yndx_vlan_set(): is that fence really required? May be store with release semantic is enough?

I'm not sure. I think this doesn't matter, because vlan_set is rare operation. Also it doesn't seem possible that vlan_config events can be invoked in parallel.
Also this patch can lead to panic without this one https://people.freebsd.org/~ae/netgc.diff
And it have side effect due to some calls missed, e.g. you will not see inbound packets on the parent interface in the BPF hook.

[oleg]

just few thoughts:

Why not? I'm going to use ck library in ipfw, and put related code here to reduce diffs.

perhaps you can avoid ck/atomic: protect yndx_vlan_set() call with IXGBE_RX_LOCK()

I'm not sure. I think this doesn't matter, because vlan_set is rare operation. Also it doesn't seem possible that vlan_config events can be invoked in parallel.
Also this patch can lead to panic without this one https://people.freebsd.org/~ae/netgc.diff

about panic: you are storing ifnet pointers but i can't see any if_ref()/if_rele()

[ae]

In D12040#249735, @oleg wrote:

Why not? I'm going to use ck library in ipfw, and put related code here to reduce diffs.

perhaps you can avoid ck/atomic: protect yndx_vlan_set() call with IXGBE_RX_LOCK()

The previous implementation has used IXGBE_RX_LOCK() and I tried to go away from this.
We kept vlans array in each rx queue to avoid lock contention between different queues. Now I'm trying to make the patch simpler to be able use similar code for each used driver (ixgbe, cxgbe, mlx5en).

I'm not sure. I think this doesn't matter, because vlan_set is rare operation. Also it doesn't seem possible that vlan_config events can be invoked in parallel.
Also this patch can lead to panic without this one https://people.freebsd.org/~ae/netgc.diff

about panic: you are storing ifnet pointers but i can't see any if_ref()/if_rele()

Yes, the GC code does delayed free() for ifnet pointer, to get some time for packets that are being processed in the time when ifnet goes away.

[cramerj_intel.com]

Perhaps a rename of the new files to reflect the technology/enhancement you are proposing? Most people won't associate Yandex with VLANs. Regardless, to really benefit FreeBSD in general, you should probably include patches to other drivers as well, since VLANs are not specific to Intel devices.

[ae]

In D12040#250190, @cramerj_intel.com wrote:

Perhaps a rename of the new files to reflect the technology/enhancement you are proposing? Most people won't associate Yandex with VLANs. Regardless, to really benefit FreeBSD in general, you should probably include patches to other drivers as well, since VLANs are not specific to Intel devices.

Just to clarify, this patch was discussed in this https://reviews.freebsd.org/D11370 thread, so I posted it. It is not targeted to be committed yet, since I don't think it is good enough :-)
But we can discuss the possible ways to make such improvement a general to be able use it in stock FreeBSD.

[erj]

I guess formally recognize the current state the patch is in.