netem: add Gilbert-Elliot (gemodel) loss support - #1210
Conversation
The kernel's netem qdisc supports a Gilbert-Elliot two-state loss model (TCA_NETEM_LOSS / NETEM_LOSS_GE, struct tc_netem_gemodel) as an alternative to the basic Loss/LossCorr correlated-loss model, letting callers model bursty packet loss instead of independent/iid loss. This attribute was previously unimplemented in this package even though the TCA_NETEM_LOSS constant already existed. Adds TcNetemGemodel to the nl package mirroring the kernel ABI directly (net/sched/sch_netem.c, include/uapi/linux/pkt_sched.h), and GELossP/R/H/K1 fields on NetemQdiscAttrs/Netem, wired through NewNetem and the netem qdisc encode/decode path the same way Corrupt/Reorder already are.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughNetem now supports Gilbert-Elliot burst-loss parameters. The change adds kernel-compatible encoding, nested qdisc serialization and deserialization, public configuration fields, and round-trip tests. ChangesNetem Gilbert-Elliot Loss
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant NewNetem
participant NetemSerializer
participant KernelQdisc
Client->>NewNetem: Configure GELoss percentages
NewNetem->>NewNetem: Convert percentages to kernel values
NewNetem->>NetemSerializer: Provide Netem with GELoss fields
NetemSerializer->>KernelQdisc: Send nested TCA_NETEM_LOSS attributes
KernelQdisc-->>NetemSerializer: Return nested NETEM_LOSS_GE attributes
NetemSerializer-->>Client: Restore GELoss fields
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@qdisc_linux.go`:
- Around line 669-682: Update the netem loss parsing switch in QdiscList to
handle the unflagged nl.TCA_NETEM_LOSS attribute as well as the existing nested
form, reusing the same ParseRouteAttr and GELoss* assignment logic so dumped
Gilbert-Elliot configurations populate their fields.
- Around line 253-263: Update the GE model condition in the qdisc serialization
path to emit the nested NETEM_LOSS_GE attributes whenever any GE parameter is
configured, including when GELossP is zero. Use the existing GELossP, GELossR,
GELossH, and GELossK1 fields to detect configuration, while preserving the
current gemodel serialization and legacy-model exclusivity.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e53dd92e-ac0c-4f82-846c-98183cd291c2
📒 Files selected for processing (5)
class_test.gonl/tc_linux.gonl/tc_linux_test.goqdisc.goqdisc_linux.go
| // Gilbert-Elliot loss model. Mutually exclusive with the basic | ||
| // Loss/LossCorr model: the kernel selects whichever was supplied. | ||
| if qdisc.GELossP > 0 { | ||
| gemodel := nl.TcNetemGemodel{} | ||
| gemodel.P = qdisc.GELossP | ||
| gemodel.R = qdisc.GELossR | ||
| gemodel.H = qdisc.GELossH | ||
| gemodel.K1 = qdisc.GELossK1 | ||
| loss := options.AddRtAttr(nl.TCA_NETEM_LOSS|unix.NLA_F_NESTED, nil) | ||
| loss.AddRtAttr(nl.NETEM_LOSS_GE, gemodel.Serialize()) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate qdisc_linux.go =="
fd -a 'qdisc_linux\.go$' . || true
echo "== relevant symbol excerpts =="
file="$(fd 'qdisc_linux\.go$' . | head -n1)"
if [ -n "${file:-}" ]; then
wc -l "$file"
sed -n '55,95p' "$file" | cat -n
sed -n '235,270p' "$file" | cat -n
sed -n '655,690p' "$file" | cat -n
fi
echo "== git diff stat/name =="
git diff --stat || true
git diff --name-only || trueRepository: vishvananda/netlink
Length of output: 4410
🌐 Web query:
Linux kernel netem NETEM_LOSS_GE nla nested p = 0 accepted
💡 Result:
In the Linux kernel's netem (Network Emulator) implementation, NETEM_LOSS_GE refers to the Gilbert-Elliot packet loss model [1]. This model uses a nested Netlink attribute structure to pass parameters between user space (typically the tc tool) and the kernel [2][3]. The p = 0 value in your query refers to the transition probability parameter within the Gilbert-Elliot model configuration [4][3]. When configuring netem loss gemodel via tc, the parameters correspond to the structure struct tc_netem_gemodel [3][1]: - p: Probability of transitioning to the bad (lossy) state [4][5]. - r: Probability of exiting the bad state [4][5]. - h: Loss probability in the bad state (often represented as 1-h in documentation) [4][5]. - k1: Loss probability in the good state (often represented as 1-k in documentation) [4][3]. The kernel's get_loss_clg function handles the parsing of these nested attributes [2][3]. It expects the data size to match sizeof(struct tc_netem_gemodel) [3][6]. If the parameters are provided as p = 0, the kernel accepts this value as a valid probability (specifically, 0% probability of transitioning to the bad state in this context) [4][3]. The input is mapped directly to the clg.a1 field in the kernel's internal netem_sched_data structure [3][7]. There is no specific mechanism that rejects p=0 as an invalid configuration; rather, it is processed as a standard integer value for the Gilbert-Elliot model [3]. Documentation confirms that the Bernoulli model is a special case of the Gilbert-Elliot model, where setting specific parameters (like p) allows the model to function as a simpler random loss generator [4][8].
Citations:
- 1: https://github.com/torvalds/linux/blob/master/include/uapi/linux/pkt_sched.h
- 2: https://gbmc.googlesource.com/linux/+/88fac17500f4ea49c7bac136cf1b27e7b9980075/net/sched/sch_netem.c
- 3: https://github.com/torvalds/linux/blob/master/net/sched/sch_netem.c
- 4: https://linux.org/docs/man8/tc-netem.html
- 5: https://man7.org/linux/man-pages/man8/tc-netem.8.html
- 6: https://git.zx2c4.com/linux-dev/plain/net/sched/sch_netem.c
- 7: https://android.googlesource.com/kernel/common/+/6d5c2c1877e509d7701a2c18808b9f930f46d6e1/net/sched/sch_netem.c
- 8: https://manpages.debian.org/bookworm/iproute2/tc-netem.8.en.html
🌐 Web query:
site:codebrowser.dev sch_netem.c get_loss_clg TCA_NETEM_LOSS NLA_F_NESTED NETEM_LOSS_GE
💡 Result:
In the Linux kernel, specifically within the network traffic control (tc) subsystem, the terms identified relate to the configuration and handling of netem (Network Emulator) attributes via Netlink messages. 1. TCA_NETEM_LOSS: This is an enumeration value in the Linux kernel (defined in include/uapi/linux/pkt_sched.h) used as a Netlink attribute type to configure packet loss for the netem qdisc [1]. 2. NETEM_LOSS_GE: This represents the "Gilbert-Elliot" packet loss model, also defined in include/uapi/linux/pkt_sched.h within the NETEM_LOSS enumeration [1]. It is used to specify advanced loss patterns beyond simple random loss [1]. 3. NLA_F_NESTED: This is a Netlink flag (defined in include/uapi/linux/netlink.h) used to indicate that an attribute contains a nested list of further attributes [2]. When configuring complex netem attributes (like specific loss models), the kernel often expects them to be nested within a parent Netlink attribute, requiring this flag to be set to ensure proper parsing and validation by the netlink attribute validation logic [3][4]. 4. sch_netem.c and get_loss_clg: While get_loss_clg is not a standard, globally exposed symbol, it refers to the pattern of internal helper functions found in sch_netem.c (and other qdisc implementations) that parse and "get" configuration data—such as loss model parameters (clg likely standing for "Gilbert-Elliot" or a similar configuration model)—from Netlink attributes [1][4][5]. In the context of traffic control, code paths like these utilize the NLA_F_NESTED flag to safely unpack nested structures describing complex loss behaviors, such as the parameters for the NETEM_LOSS_GE model, into the kernel's internal qdisc structures [4][5].
Citations:
- 1: https://codebrowser.dev/linux/linux/include/uapi/linux/pkt_sched.h.html
- 2: https://codebrowser.dev/linux/linux/include/uapi/linux/netlink.h.html
- 3: https://codebrowser.dev/linux/linux/include/net/netlink.h.html
- 4: https://codebrowser.dev/linux/linux/lib/nlattr.c.html
- 5: https://codebrowser.dev/linux/linux/net/sched/sch_dualpi2.c.html
🌐 Web query:
sch_netem.c nla_attr_size TCA_NETEM_LOSS NETEM_LOSS_GE nla_f_nested
💡 Result:
In the Linux kernel's Network Emulator (netem) implementation (sch_netem.c), these constants and identifiers are used to manage loss models via Netlink attributes [1][2]. TCA_NETEM_LOSS is a Netlink attribute type used to group nested loss model parameters [1][3]. It is defined with the NLA_NESTED policy [3][4], allowing multiple specific loss model parameters to be included within the same nested structure [1][5]. NETEM_LOSS_GE refers to the Gilbert-Elliot loss model, which is one of the supported loss models in netem [1][2]. When userspace configures this model, it passes a struct tc_netem_gemodel containing parameters (p, r, h, k1) [1][3]. NLA_F_NESTED is a flag used by userspace tools (such as iproute2's tc command) when creating nested Netlink attributes [6][7]. When the tc command adds the TCA_NETEM_LOSS attribute, it performs bitwise OR with NLA_F_NESTED (TCA_NETEM_LOSS | NLA_F_NESTED) to indicate to the kernel that the attribute contains further nested attributes [6][8]. The kernel's netem implementation in sch_netem.c includes a get_loss_clg function that uses nla_for_each_nested to iterate through the attributes inside the TCA_NETEM_LOSS nest, identifies them by type (e.g., NETEM_LOSS_GE), and parses the data accordingly [1][5]. The use of nla_attr_size is typically used in the kernel to validate the size of incoming Netlink attributes, including nested ones [3][2].
Citations:
- 1: https://raw.githubusercontent.com/torvalds/linux/master/net/sched/sch_netem.c
- 2: https://github.com/torvalds/linux/blob/master/net/sched/sch_netem.c
- 3: https://kernel.googlesource.com/pub/scm/linux/kernel/git/torvalds/linux/+/88afbb21d4b36fee6acaa167641f9f0fc122f01b/net/sched/sch_netem.c
- 4: https://git.zx2c4.com/linux-dev/plain/net/sched/sch_netem.c
- 5: https://kernel.googlesource.com/pub/scm/linux/kernel/git/torvalds/linux/+/45e34c8af58f23db4474e2bfe79183efec09a18b/net/sched/sch_netem.c
- 6: https://fossies.org/linux/iproute2/tc/q_netem.c
- 7: https://android.googlesource.com/platform/external/iproute2/+/681064afafdb131ff0825c5a59bb4a1623fcea3f/tc/q_netem.c
- 8: https://github.com/sivasankariit/iproute2/blob/master/tc/q_netem.c
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('/home/jailuser/git/qdisc_linux.go')
text = p.read_text()
checks = {
"serialize_condition": "if qdisc.GELossP > 0" in text,
"gemodel_assignments_present": all(s in text for s in [
"gemodel := nl.TcNetemGemodel{}",
"gemodel.P = qdisc.GELossP",
"gemodel.R = qdisc.GELossR",
"gemodel.H = qdisc.GELossH",
"gemodel.K1 = qdisc.GELossK1",
]),
"nested_loss_attribute_present": "nl.TCA_NETEM_LOSS|unix.NLA_F_NESTED" in text,
"ge_add_attribute_present": "loss.AddRtAttr(nl.NETEM_LOSS_GE" in text,
"ge_fields_retained_from_nattrs": all(s in text for s in [
"geLossP := Percentage2u32(nattrs.GELossP)",
"geLossR := Percentage2u32(nattrs.GELossR)",
"geLossH := Percentage2u32(nattrs.GELossH)",
"geLossK1 := Percentage2u32(nattrs.GELossK1)",
]),
"dump_case_exact": "case nl.TCA_NETEM_LOSS | unix.NLA_F_NESTED:" in text,
}
for name, ok in checks.items():
print(f"{name}: {ok}")
# Simulate the relevant branch invariants for representative configured inputs.
class Qdisc:
GELossP = 0
GELossR = 0
GELossH = 0
GELossK1 = 0
inputs = [
{"labels": ["GELossP=0,GELossK1=1"], "q": (0, 0, 0, 1)},
{"labels": ["GELossR=1"], "q": (0, 1, 0, 0)},
{"labels": ["GELossH=1"], "q": (0, 0, 1, 0)},
{"labels": ["GELossP=1"], "q": (1, 0, 0, 0)},
]
for case in inputs:
qdisc = Qdisc()
qdisc.GELossP = case["q"][0]
qdisc.GELossR = case["q"][1]
qdisc.GELossH = case["q"][2]
qdisc.GELossK1 = case["q"][3]
legacy_branch = not (qdisc.GELossP > 0 or qdisc.GELossR > 0 or qdisc.GELossH > 0 or qdisc.GELossK1 > 0)
print(f"configured input {case['labels']}: GELoss config branch selected = {not legacy_branch}")
PYRepository: vishvananda/netlink
Length of output: 630
Serialize the GE model when any GE parameter is configured.
p = 0 is valid for the GE ABI, so GELossP = 0 with any nonzero GE parameter still requires TCA_NETEM_LOSS to select the GE loss model. The current GELossP > 0 check can fall back to the legacy loss model.
Proposed fix
- if qdisc_GELossP > 0 {
+ if qdisc.GELossP > 0 || qdisc.GELossR > 0 ||
+ qdisc.GELossH > 0 || qdisc.GELossK1 > 0 {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@qdisc_linux.go` around lines 253 - 263, Update the GE model condition in the
qdisc serialization path to emit the nested NETEM_LOSS_GE attributes whenever
any GE parameter is configured, including when GELossP is zero. Use the existing
GELossP, GELossR, GELossH, and GELossK1 fields to detect configuration, while
preserving the current gemodel serialization and legacy-model exclusivity.
CI caught this: TestClassAddDel round-tripped GELossP as 0 instead of the value that was set. The parse-side switch case required TCA_NETEM_LOSS | NLA_F_NESTED, matching how iproute2 builds the attribute on the way in (tc/q_netem.c). But net/sched/sch_netem.c's dump_loss_model() builds its reply with nla_nest_start_noflag(), which deliberately omits the flag bit on the way out, so the case never matched and the GE fields stayed zeroed.
1 participant
Summary
The kernel's
netemqdisc supports a Gilbert-Elliot two-state loss model (TCA_NETEM_LOSS/NETEM_LOSS_GE,struct tc_netem_gemodel), used to model bursty/correlated packet loss instead of the independent/iid loss the existingLoss/LossCorrfields provide. TheTCA_NETEM_LOSSconstant already existed in this package but had no encode/decode support at all.TcNetemGemodelto thenlpackage, a direct mirror of the kernel'sstruct tc_netem_gemodel(include/uapi/linux/pkt_sched.h), following the exactSerialize/Deserialize/Lenpattern already used byTcNetemCorrupt/TcNetemReorder.GELossP,GELossR,GELossH,GELossK1toNetemQdiscAttrs(percent, human-facing) andNetem(raw kernel-scaled values), wired throughNewNetem.TCA_NETEM_LOSS | NLA_F_NESTEDattribute containingNETEM_LOSS_GE, matching howiproute2'stc/q_netem.cbuilds the same attribute (addattr_nest(n, ..., TCA_NETEM_LOSS | NLA_F_NESTED)).Netem.GELossH/GELossK1map directly onto the kernel'sh/k1fields, not thetcCLI's "1-h"/"1-k" convention (the CLI performs that complement itself before calling into the kernel — seeq_netem.c); this is documented on the struct and onNetemQdiscAttrs.net/sched/sch_netem.cthat the GE model and the basicLoss/LossCorrmodel are mutually exclusive at the kernel level (selected byq->loss_model); documented on the new fields.NETEM_LOSS_GI(4-state) is out of scope for this PR; only the enum value is added since it lives in the same kernel attribute.Test plan
go build ./...go test ./nl/...— newTestTcNetemGemodelDeserializeSerialize(random-byte round-trip, no root required) passesgo test .— extendedTestClassAddDelwithGELossP/R/H/K1values and added assertions comparing the qdisc as added vs. as read back fromQdiscList; this test is root-gated like the rest of the suite (skipUnlessRoot) and was confirmed to skip cleanly in an unprivileged sandbox — a maintainer/CI with root will exercise the actual netlink round-trip against a livevcan/ifbinterfaceSummary by CodeRabbit
New Features
Tests