Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversion of InnerFlowId to SDT args appears expensive #462

Closed
FelixMcFelix opened this issue Feb 21, 2024 · 1 comment · Fixed by #475
Closed

Conversion of InnerFlowId to SDT args appears expensive #462

FelixMcFelix opened this issue Feb 21, 2024 · 1 comment · Fixed by #475
Labels

Comments

@FelixMcFelix
Copy link
Collaborator

We seem to be spending around 13% of Port::process by calling flow_id_sdt_arg::from for the benefit of its many dtrace probes -- many times per packet, as we have pre/post-processing and per-layer probes. We should either cache this, or find an adequate #[repr(C)] for InnerFlowId so that we don't need to perform a conversion.

image
@FelixMcFelix
Copy link
Collaborator Author

I spent some time noodling on this before now, and returned to it today. Removing all calls to flow_id_sdt_arg and port_process_return_probe (#460) gives us ballpark xde_rx processing times like the below client-to-server iperf (using raw output to save time):

BeforeAfter
  rx                                                
           value  ------------- Distribution ------------- count    
           < 256 |                                         0        
             256 |                                         6        
             512 |                                         372      
             768 |                                         8798     
            1024 |@@                                       277299   
            1280 |@@@@@@@@@@@@@@@@@@@@@                    2381870  
            1536 |@@@@@@@                                  782531   
            1792 |@@@                                      360863   
            2048 |@@@                                      333294   
            2304 |@@                                       194406   
            2560 |                                         55506    
            2816 |                                         26438    
            3072 |                                         18613    
...
  rx                                                
           value  ------------- Distribution ------------- count    
             256 |                                         0        
             512 |                                         28       
             768 |@@                                       176626   
            1024 |@@@@@@@@@@@@@@@@@@@                      1987676  
            1280 |@@@@@@@                                  747631   
            1536 |@@@                                      324862   
            1792 |@@@@                                     383692   
            2048 |@                                        127578   
            2304 |                                         45411    
            2560 |                                         25437    
            2816 |                                         37834    
            3072 |@                                        84534    
...

So these are very worthwhile (in combination with #460), but I'm seeing some bizarreness in two locations when using either a valid #[repr(C)] flow ID or e.g. passing a null ptr for the flow_id_arg to the SDT. Calling either __dtrace_probe_uft__tcp__closed or __dtrace_probe_flow__expired causes a kernel panic due to a null-ptr (or other bad address) dereference several functions up (e.g., in update_tcp_entry when trying to match on &TcpDirection), but only when these probes are called -- and not on all codepaths. Register values for supposed pointers also contain values like 2, which smells like an enum discriminant to me. I'll need to look into whether black_boxing the probe calls will prevent whatever weirdness is happening.

FelixMcFelix added a commit that referenced this issue May 10, 2024
#475)

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.

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*, with
comparable bandwidth increase.

Closes #462.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant