Page MenuHomeFreeBSD

direct vlan handling in ixgbe
Needs RevisionPublic

Authored by ae on Aug 16 2017, 9:11 AM.

Details

Reviewers
sbruno
erj
Group Reviewers
Intel Networking
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() ...

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 11099

Event Timeline

ae created this revision.Aug 16 2017, 9:11 AM
oleg added a subscriber: oleg.Aug 16 2017, 10:37 AM
oleg added a comment.Aug 16 2017, 11:20 AM
  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 added a comment.Aug 16 2017, 12:59 PM
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.

  1. 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 added a comment.Aug 16 2017, 2:01 PM

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 added a comment.Aug 16 2017, 2:22 PM
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.

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 added a comment.Aug 17 2017, 7:19 PM

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 requested changes to this revision.Aug 24 2017, 10:47 PM
erj added a subscriber: erj.

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

This revision now requires changes to proceed.Aug 24 2017, 10:47 PM