Use a `repr(C)` flow ID to reduce repeated per-packet conversion costs by FelixMcFelix · Pull Request #475 · oxidecomputer/opte

FelixMcFelix · 2024-03-12T14:03:23Z

This PR replaces InnerFlowId with an equivalent C-ABI friendly struct, from which we can directly pass pointers to our dtrace SDTs. This has proven necessary as we are reconstructing this many times per packet in both the UFT slowpath and fastpath.

Unfortunately, this has unearthed #476 for which we have a temporary hack, NopDrop, which uses a black_box'd Drop implementation to prevent problem SDTs from being tail-called into.

Additionally, this makes partial progress on #460 by using the new DError machinery whenever a packet is Ok(Modified | Hairpin | Bypass) to elide string formats on layer/port processing. The Err(_) and Ok(Drop{ .. }) cases remain open work, but are less common and in those cases we have at least removed a superfluous realloc + memcpy on the formatted string.

From local IGB<->IGB results and comparing 4e2ad7e against master in CI, this seems like O(10%) reduction in packet latency spent in XDE. Certainly less of a total improvement once we account for the rest of MAC, and throughput impact on T6s is TBD.

Closes #462.

`uintptr_t` maybe be easy to stick in a probe, but it is also a footgun. This fixdes the display for e.g. port-process-return, but does not affect the actual panics mentioned in #476.

It appears that JMP'd dtrace probes are being rewritten at load time without a RTN. Near as I can tell, rustc offers no way to say 'don't tail-call optimise this thing', so I'm forcing in a ZST with a drop impl to achieve similar behaviour. Hopefully this suffices.

FelixMcFelix · 2024-04-08T09:18:52Z

Testing on sn9/14 over a single 100GbE link (cxgbe1<->cxgbe1) (on a release build of Helios):

Before After

Run 1/5...1762.141Mbps
Run 2/5...1785.854Mbps
Run 3/5...1759.315Mbps
Run 4/5...1741.879Mbps
Run 5/5...1741.848Mbps

Run 1/5...1944.113Mbps
Run 2/5...1939.164Mbps
Run 3/5...1934.922Mbps
Run 4/5...1930.715Mbps
Run 5/5...1930.84Mbps

Mean 1758.207 Mean 1935.951

Not sure whether/if we'd expect more improvement with cxgbe0s also connected, but a 10.1% mean BW increase is pretty nice.

FelixMcFelix · 2024-04-25T11:15:49Z

lib/opte/src/engine/flow_table.rs

    last_hit: Option<Moment>,
    now: Option<Moment>,
 ) {
+    let a = crate::NopDrop;


All NopDrop code will be removed once illumos#16480 is merged in and available on the helios-2.0 lab image.

FelixMcFelix · 2024-04-25T11:18:31Z

lib/opte/src/engine/layer.rs

 }

-#[derive(Debug)]
+// TODO: Represent `name` as joint C+RStr to implement fully.


This would fall under the work remaining for #460.

lib/opte/src/lib.rs

dtrace/lib/common.d

lib/opte/src/engine/port.rs

dtrace/lib/common.d

lib/opte/src/engine/packet.rs

lib/opte/src/engine/print.rs

lib/opte/src/engine/port.rs

Realised when I was out that this struct only has 2B alignment, so there was a possibility of passing up an unaligned reference to an SDT.

Given that we're now using these to handle nested `Ok(...)` types rather than just error variants, a rename was a bit in order. `DError` might also benefit?

luqmana

Thanks Kyle! Since the fix for #476 has landed in helios I'd say remove the workarounds but otherwise LGTM.

FelixMcFelix added 5 commits March 4, 2024 17:15

Perf: rework InnerFlowId with a C ABI repr

d5d2479

Merge branch 'master' into cheaper-flowid

cc1dd8b

Fix compile of bad USDT.

48b831d

Temporarily disable miscompile-y probes

cbbe568

...Oops.

d84fba7

FelixMcFelix mentioned this pull request Mar 12, 2024

SDTs optimised into tail-calls panic #476

Closed

FelixMcFelix added 6 commits March 13, 2024 11:30

DTrace script fixes, trying to understand odd behaviour.

4de9c4a

Correctly type all InnerFlowId SDT args

cb20ed2

`uintptr_t` maybe be easy to stick in a probe, but it is also a footgun. This fixdes the display for e.g. port-process-return, but does not affect the actual panics mentioned in #476.

I suppose that's technically an API change.

60e0b84

Unformat (most of) the Ok cases for packet probes

baed8d0

Fixup dtrace scripts, nicer format on drop

4e2ad7e

FelixMcFelix marked this pull request as ready for review March 15, 2024 19:33

FelixMcFelix added this to the 8 milestone Apr 11, 2024

FelixMcFelix mentioned this pull request Apr 11, 2024

Want smarter use of existing structure in Packet::emit_packet_headers #481

Closed

FelixMcFelix self-assigned this Apr 16, 2024

FelixMcFelix mentioned this pull request Apr 16, 2024

Cache routes received from next_hop #499

Merged

FelixMcFelix added feature Planned and requested features perf and removed feature Planned and requested features labels Apr 22, 2024

FelixMcFelix commented Apr 25, 2024

View reviewed changes

lib/opte/src/lib.rs Outdated Show resolved Hide resolved

Minor nit caught on self-review.

74bed98

mkeeter reviewed Apr 25, 2024

View reviewed changes

dtrace/lib/common.d Outdated Show resolved Hide resolved

mkeeter reviewed Apr 25, 2024

View reviewed changes

lib/opte/src/engine/port.rs Outdated Show resolved Hide resolved

luqmana reviewed Apr 25, 2024

View reviewed changes

dtrace/lib/common.d Outdated Show resolved Hide resolved

lib/opte/src/engine/packet.rs Outdated Show resolved Hide resolved

lib/opte/src/engine/print.rs Outdated Show resolved Hide resolved

lib/opte/src/engine/port.rs Outdated Show resolved Hide resolved

FelixMcFelix added 2 commits April 26, 2024 11:29

Review feedback: Matt (hidden tail-call prevention)

a2cab3b

Merge branch 'master' into cheaper-flowid

0eca6c9

FelixMcFelix added 5 commits April 26, 2024 11:30

Re-bump API version.

1e7506e

Review Feedback: Luqman pt.1

fffbaba

InnerFlowId needs 4B alignment itself.

0bf91e6

Realised when I was out that this struct only has 2B alignment, so there was a possibility of passing up an unaligned reference to an SDT.

Review Feedback: Luqman pt.2

ddc1b98

Friendlier names -- LabelBlock, ::from_nested

0edd7e6

Given that we're now using these to handle nested `Ok(...)` types rather than just error variants, a rename was a bit in order. `DError` might also benefit?

luqmana approved these changes Apr 29, 2024

View reviewed changes

Remove NopDrop and its uses

8237f42

FelixMcFelix modified the milestones: 8, 9 May 10, 2024

FelixMcFelix merged commit 976f4dc into master May 10, 2024

FelixMcFelix deleted the cheaper-flowid branch May 10, 2024 11:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a `repr(C)` flow ID to reduce repeated per-packet conversion costs#475

Use a `repr(C)` flow ID to reduce repeated per-packet conversion costs#475
FelixMcFelix merged 20 commits intomasterfrom
cheaper-flowid

FelixMcFelix commented Mar 12, 2024 •

edited

Loading

Uh oh!

FelixMcFelix commented Apr 8, 2024 •

edited

Loading

Uh oh!

FelixMcFelix Apr 25, 2024

Uh oh!

FelixMcFelix Apr 25, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

luqmana left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

FelixMcFelix commented Mar 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FelixMcFelix commented Apr 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FelixMcFelix Apr 25, 2024

Choose a reason for hiding this comment

Uh oh!

FelixMcFelix Apr 25, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

luqmana left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

FelixMcFelix commented Mar 12, 2024 •

edited

Loading

FelixMcFelix commented Apr 8, 2024 •

edited

Loading