Replace the fastforward path with tryforward which does not require a sysctl and will always be on. The former split between default and fast forwarding is removed by this commit while preserving the ability to use all network stack features.
ClosedPublic
Actions

Authored by gnn on Oct 30 2015, 8:26 PM.

Details

Reviewers

melifaro
ae
olivier_cochard.me
bz
rwatson

Group Reviewers

network

Commits

rS295285: MFC: r290383,295282,295283
rS290383: Replace the fastforward path with tryforward which does not require a

Summary

...es. This version addresses
the IPSEC issues now that IPSEC has been updated to be default
in GENERIC

Test Plan

Tested with iperf3 and Conductor and results presented at VBSDCon 2015

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 1016
Build 1019: arc lint + arc unit

Event Timeline

gnn updated this revision to Diff 9834.Oct 30 2015, 8:26 PM

gnn retitled this revision from to Replace the fastforward path with tryforward which does not require a sysctl and will always be on. The former split between default and fast forwarding is removed by this commit while preserving the ability to use all network stack features..

gnn updated this object.

gnn edited the test plan for this revision. (Show Details)

Herald added a subscriber: imp. · View Herald TranscriptOct 30 2015, 8:26 PM

gnn edited the test plan for this revision. (Show Details)Oct 30 2015, 8:28 PM

gnn added reviewers: network, rwatson, bz, melifaro, olivier_cochard.me.

loos added a subscriber: loos.Oct 30 2015, 8:49 PM

Just a cosmetic/documentation remark:

In sys/netinet/ip_fastfwd.c, comment "Step 1: check for packet drop conditions (and sanity checks)" was removed.
Then comment "Step 2: fallback conditions to normal ip_input path processing" (lines 186 to 188) should be renamed or removed.

You made a system acting as router by default, even when net.inet.ip.forwarding is zero. Is it an intended change?

Good point. I'll add that check.

Add check of forwarding sysctl before calling into tryforward.

Updating D4042: Replace the fastforward path with tryforward which does not require

a sysctl and will always be on. The former split between default
and fast forwarding is removed by this commit while preserving the
ability to use all network stack features.

Remove unnecessary comment per Olivier Couchard.

Updating D4042: Replace the fastforward path with tryforward which does not require

a sysctl and will always be on. The former split between default
and fast forwarding is removed by this commit while preserving the
ability to use all network stack features.

ae added inline comments.Nov 4 2015, 10:29 AM

sys/netinet/ip_input.c
83	What you think about reducing namespace pollution and create a simple function in ip_ipsec.c to check SPD availability? int ip_ipsec_skip() { return ((key_havesp(IPSEC_DIR_INBOUND) \|\| key_havesp(IPSEC_DIR_OUTBOUND)) == 0); }
518	This block now can be moved under one #ifdef IPSEC.

gnn marked an inline comment as done.Nov 4 2015, 2:31 PM

gnn added inline comments.

sys/netinet/ip_input.c
83	I had thought about this originally but then the number of places it would be used was too small and I didn't think we'd be adding that many more of them. It could be done outside the context of this particular patch set if people feel really strong about it

ae accepted this revision.Nov 4 2015, 4:55 PM

ae added a reviewer: ae.

This revision is now accepted and ready to land.Nov 4 2015, 4:55 PM

I did several forwarding benchmarks based on projects/routing patched with diff 9834.
I tested on 2x E5-2660 (HT off) @ Chelsio T580 HW.
Actual test consists of IPv4 forwarding (128x192 src/dst pairs resulting in close-to-equal traffic distribution on NIC RX rings).
Pure projects/routing results:
1 core: 1.10 mpps, 8 cores: 7.8mpps, 16 cores: 12.7 mpps
Projects/routing w/patch:
1 core: 1.07mpps, 8 cores: 7.95 mpps, 16 cores: 12.7 mpps

I have no good explanation for 8 cores result, but anyway, it looks like performance is roughly the same at least on amd64.

Closed by commit rS290383: Replace the fastforward path with tryforward which does not require a (authored by gnn). · Explain WhyNov 5 2015, 12:43 PM

This revision was automatically updated to reflect the committed changes.