XDP by default - #12119
Conversation
4d113c2 to
c898197
Compare
07d0d3c to
af9c08f
Compare
af9c08f to
da27354
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #12119 +/- ##
===========================================
+ Coverage 72.1% 83.2% +11.0%
===========================================
Files 748 855 +107
Lines 227236 341670 +114434
===========================================
+ Hits 164042 284518 +120476
+ Misses 63194 57152 -6042 🚀 New features to boost your workflow:
|
1e17c53 to
e39da1a
Compare
|
made a replacement PR here: #13192
although, I happy to build on top of this PR is and get rid of mine. but if we go that way, we need to add the things mentioned above. |
This is not changing the mode used by default, and agave default is copy mode.
This was rejected by @bw-solana in #12119 (comment) |
gregcusack
left a comment
There was a problem hiding this comment.
ignore my comment above! sorry, i completely misread that this had xdp copy mode on by default. closed my other PR. this looks good! few thoughts!
also should probably promote experimental poh arg before we merge this one imo. see: #13191
| --no-wait-for-vote-to-start-leader | ||
| --full-rpc-api | ||
| --allow-private-addr | ||
| --disable-xdp |
There was a problem hiding this comment.
per brooks' comment and brennan's thumbs up in slack. let's change this back to --no-xdp
There was a problem hiding this comment.
yes, apologies for causing the thrash. I'm redacted again
| if bind_addresses.len() > 1 && public_tvu_addr.is_some() { | ||
| Err(String::from( | ||
| "--public-tvu-address can not be used in a multihoming context", | ||
| ))?; | ||
| Err(String::from("XDP can not be used in a multihoming context"))?; | ||
| } |
There was a problem hiding this comment.
we can probably remove this? doesn't seem right to base this on public_tvu_addr. in fact if someone passes in two bind addresses, we should force them to also pass in --no-xdp.
| * XDP transmit is now enabled by default on Linux. The validator requires `CAP_NET_ADMIN` and | ||
| `CAP_NET_RAW` capabilities (plus `CAP_BPF` and `CAP_PERFMON` for `--xdp-zero-copy`). Pass | ||
| `--disable-xdp` to disable XDP and fall back to UDP sockets. |
There was a problem hiding this comment.
i would just mention that xdp in skb mode (copy mode) is enabled to be clear.
| * The `--experimental-retransmit-xdp-interface`, `--experimental-retransmit-xdp-cpu-cores`, and | ||
| `--experimental-retransmit-xdp-zero-copy` flags have been deprecated. Use `--xdp-interface`, |
There was a problem hiding this comment.
haven't these flags already been deprecated? so this should be a separate PR. they should probably be removed in 4.2, right? or we need to wait for major version bump?
this last comment should stay though:
XDP is now enabled by default; use `--disable-xdp` to opt out.
There was a problem hiding this comment.
Yep just needed to rebase.
| `--experimental-retransmit-xdp-zero-copy` flags have been deprecated. Use `--xdp-interface`, | ||
| `--xdp-cpu-cores`, and `--xdp-zero-copy` instead. XDP is now enabled by default; use `--disable-xdp` to opt out. | ||
| #### Changes | ||
| * Added `--disable-xdp` flag to disable XDP transmit and fall back to UDP sockets. |
There was a problem hiding this comment.
don't need to mentioned --disable-xdp twice. either leave here or leave above. i'd probably recommend leaving here and getting rid above
| * Added `--xdp-interface` to specify the network interface for XDP transmit (auto-detected from | ||
| default route if not specified). | ||
| * Added `--xdp-cpu-cores` to specify CPU cores for XDP transmit (defaults to 1 auto-selected core | ||
| that avoids the PoH core and its hyperthread sibling). | ||
| * Added `--xdp-zero-copy` to enable XDP zero-copy mode (requires hardware/driver support). |
There was a problem hiding this comment.
this should be separate PR since these were already added in a previous PR.
There was a problem hiding this comment.
yes this somehow bypassed rebase.
| To run without XDP, pass `--disable-xdp` instead (no capabilities required in this case). | ||
|
|
There was a problem hiding this comment.
tbh i wouldn't advertise that you can run without xdp. so i would delete this. make them look for it if they really want to turn it off lol
| #### Breaking | ||
| * `--block-production-method central-scheduler` is no longer supported. If passed, a warning is emitted and behavior | ||
| will default to the greedy-scheduler implementation. | ||
| * XDP transmit is now enabled by default on Linux. The validator requires `CAP_NET_ADMIN` and |
There was a problem hiding this comment.
i would also add in here that we now enforce pinned cpu core does not overlap with xdp core.
| let poh_pinned_cpu_core = | ||
| value_of(matches, "poh_pinned_cpu_core").or(poh_service::DEFAULT_PINNED_CPU_CORE); | ||
| let xdp_cpu_cores = matches | ||
| .value_of("xdp_cpu_cores") | ||
| .or_else(|| matches.value_of("experimental_retransmit_xdp_cpu_cores")); | ||
| let cpus = if let Some(cpu_str) = xdp_cpu_cores { | ||
| if bind_addresses.len() > 1 { | ||
| Err(String::from( | ||
| "--xdp-cpu-cores cannot be used in a multihoming context", | ||
| ))?; | ||
| } | ||
| Some(parse_cpu_ranges(cpu_str).expect("clap validator already accepted this CPU list")) | ||
| } else { | ||
| // Auto-select a single core for XDP. | ||
| match cpu_affinity(None) { | ||
| Ok(allowed) => { | ||
| match allowed | ||
| .iter() | ||
| .rev() | ||
| .map(|cpu| **cpu) | ||
| .find(|cpu| Some(*cpu) != poh_pinned_cpu_core) | ||
| { | ||
| Some(cpu) => Some(vec![cpu]), | ||
| None => { | ||
| return Err(format!( | ||
| "XDP requires a dedicated CPU core separate from PoH (core \ | ||
| {poh_pinned_cpu_core:?}), but none is available. Pass \ | ||
| --disable-xdp to disable XDP." | ||
| ))?; | ||
| } | ||
| } | ||
| } | ||
| Err(e) => { | ||
| return Err(format!( | ||
| "failed to query CPU affinity: {e}. Pass --disable-xdp to disable XDP, or \ | ||
| provide --xdp-cpu-cores explicitly." | ||
| ))?; | ||
| } | ||
| } | ||
| }; | ||
| cpus.map(|cpus| { | ||
| info!("XDP enabled on CPU cores: {cpus:?}"); | ||
| XdpConfig::new(xdp_interface, cpus, xdp_zero_copy) | ||
| }) | ||
| }; |
There was a problem hiding this comment.
can we pull this out into a function so we can write a test or two to make sure we haven't screwed up slash don't screw up cli args
| let cpus = if let Some(cpu_str) = xdp_cpu_cores { | ||
| if bind_addresses.len() > 1 { | ||
| Err(String::from( | ||
| "--xdp-cpu-cores cannot be used in a multihoming context", | ||
| ))?; | ||
| } | ||
| Some(parse_cpu_ranges(cpu_str).expect("clap validator already accepted this CPU list")) |
There was a problem hiding this comment.
if you explicitly pass in the same xdp core as poh core, this will not fail. so probably want to ensure these don't conflict here.
e39da1a to
8195995
Compare
gregcusack
left a comment
There was a problem hiding this comment.
looks better! thank you! will you also update docs/src/operations/running-with-af-xdp.md with the changes to that md file seen here please: https://github.com/anza-xyz/agave/pull/13192/changes
| been permitted to the current process: {missing_caps:?}. If XDP is not needed, \ | ||
| pass --no-xdp to disable it", |
There was a problem hiding this comment.
nit: i would remove this part lol:
If XDP is not needed, pass --no-xdp to disable it
| if bind_addresses.len() > 1 && public_tvu_addr.is_some() { | ||
| Err(String::from( | ||
| "--public-tvu-address can not be used in a multihoming context", | ||
| ))?; | ||
| } | ||
|
|
There was a problem hiding this comment.
spooky action at a distance. reverted.
| $ sudo setcap 'cap_net_admin,cap_net_raw+eip' target/release/agave-validator | ||
| ``` | ||
|
|
||
| For XDP zero-copy mode (`--xdp-zero-copy`), additional capabilities are needed: | ||
|
|
||
| ```bash | ||
| $ sudo setcap 'cap_net_admin,cap_net_raw,cap_bpf,cap_perfmon+eip' target/release/agave-validator | ||
| ``` |
There was a problem hiding this comment.
above it says run ./cargo build but then we say to use setcap on target/release/agave-validator but ./cargo build won't put the agave-validator binary into release/
| fn test_explicit_xdp_core_conflicts_with_poh_core_is_error() { | ||
| let default_args = DefaultArgs::default(); | ||
| let app = add_args(clap::App::new("agave-validator"), &default_args); | ||
| // poh_pinned_cpu_core defaults to DEFAULT_PINNED_CPU_CORE (1); set XDP to the same. |
There was a problem hiding this comment.
I've just removed the comment.
8195995 to
9865524
Compare
Docs would have to be another PR to another repo - docs were moved. |
they removed docs?!?! sad. ok now it's harder to match code updates to doc updates. but i get why they did it. |
| } | ||
| Some(parsed) | ||
| } else { | ||
| // Auto-select a single core, avoiding the PoH core and its hyperthread sibling. |
There was a problem hiding this comment.
nit: "and its hyperthread sibling." i am not sure this is actually true...
There was a problem hiding this comment.
yes this used to be true with previous version that did not use cpu-utils. stale.
| $ sudo setcap 'cap_net_admin,cap_net_raw+eip' target/debug/agave-validator | ||
| ``` | ||
|
|
||
| For XDP zero-copy mode (`--xdp-zero-copy`), additional capabilities are needed: | ||
|
|
||
| ```bash | ||
| $ sudo setcap 'cap_net_admin,cap_net_raw,cap_bpf,cap_perfmon+eip' target/debug/agave-validator |
There was a problem hiding this comment.
nit: sorry, maybe we should just say
sudo setcap 'cap_net_admin,cap_net_raw+eip' <path-to-agave-validator-binary>
because if we're running in debug mode, why are we using xdp. but then again above it says:
> [!NOTE]
> Note that this builds a debug version that is **not suitable for running a testnet or mainnet validator**. Please read [`docs/src/cli/install.md`](docs/src/cli/install.md#build-from-source) for instructions to build a release version for test and production uses.
^ so maybe that's clear enough that if you are actually going to run a validator you don't want to run in debug mode.
| let poh_pinned_cpu_core = | ||
| value_of(matches, "poh_pinned_cpu_core").or(poh_service::DEFAULT_PINNED_CPU_CORE); |
There was a problem hiding this comment.
ok so we also need to check for the experimental version of this pinned poh flag as well. or else we won't catch that the xdp and poh cores overlap
There was a problem hiding this comment.
Gotta love ancient version of clap..
9865524 to
e9c098a
Compare
|
Tick the box to add this pull request to the merge queue (same as
|
Problem
XDP should not be experimental anymore https://discord.com/channels/428295358100013066/478692221441409024/1496253676891930865
Summary of Changes
Testing