Simplify/refactor the current program model #141

csujedihy · 2023-01-30T02:53:39Z

The new model always allows program sharing. Thus, meta program and sharing related flags and structs have been deleted.
Now, when a program is plumbed, a program "binding" is inserted to the RX queue. More programs just mean more bindings on a RX queue.
All rules are still placed on a contiguous memory to remain the performance of the data path. When add/removing a program, a new "compiled" program is generated on control path from all bindings on the RX queue.

This refactoring IMO would make binding to all queues easier to implement. We would just need to extend program object to have an array of program bindings.

src/xdp/rx.c

csujedihy · 2023-01-30T07:49:02Z

This optimization is broken by this PR. The code might choose the fast path initially, and then subsequently added programs will be incorrectly ignored.

https://github.com/microsoft/xdp-for-windows/pull/141/files#diff-ffbb7352b99db671bfaad2a1bf9999d75fa6f92c1e32e7b11086fc72dc87cdceL794

    //
    // Implement a fast path for a single XSK receiving all frames.
    //
    if (XdpProgramCanXskBypass(RxQueue->Program)) {
        RxQueue->Dispatch = XdpRxExclusiveXskDispatch;
    } else {
        RxQueue->Dispatch = XdpRxDispatch;
    }

published/external/xdpapi.h

src/xdp/program.c

src/xdp/rx.c

mtfriesen · 2023-01-30T18:47:52Z

src/xdp/rx.c

-XDP_PROGRAM *
-XdpRxQueueGetProgram(
+LIST_ENTRY *
+XdpRxQueueGetProgramBindingList(


IMO this abstraction is going to be fragile once we integrate with eBPF, because we'll want to invoke either eBPF or our legacy built-in programs, but not both. It would be best if the RX queue exposes an opaque XDP_PROGRAM * pointer getter/setter and then invokes the XDP_PROGRAM on the data path.

Sorry, I am not quite following here. Are you saying this new concept of "program binding" would not exist with ebpf? Or this abstraction allows mixing ebpf and non-ebpf programs?

Hmm, a notion of "program binding" definitely exists with eBPF, so the issue is the mixing of different types of programs, and to a lesser extent the leakage of the chain of non-eBPF programs into the RX queue. One option here is to create a top-level program binding (it could contain either an eBPF program or a list of non-eBPF programs) and then have the RX queue deal only with that.

For the non-eBPF programs, then the program.c module could peek into the list stored in that top-level binding object.

I agree. That would be cleaner. Do you want me to work on that in this PR?

(tests are all green now except a flaky functional test case)

Why do we need to keep a legacy mechanism once ebpf integration is supported?

We may be in a partially-integrated state for a while, where some features of our built-in program implementation are yet to be lit up through eBPF. Once everyone depending on those features can switch to eBPF, we should be able to migrate away from built-in programs.

Sounds like you're suggesting two separate steps, i.e., first add ebpf integration, then remove built-in programs? The word "replace" in the title of issue #7 implied to me that there is no "partially integrated state for a while", it's all at the same time. I'm asking whether you actually need a partially integrated state for a while instead of just doing it.

@dthaler MsQuic is dependent on the built-in program support. And we are going forward with deployments leveraging that. Until we have a complete replacement for those deployments, we cannot remove the built-in program support.

mtfriesen · 2023-01-30T18:54:32Z

This optimization is broken by this PR. The code might choose the fast path initially, and then subsequently added programs will be incorrectly ignored.
//
// Implement a fast path for a single XSK receiving all frames.
//
if (XdpProgramCanXskBypass(RxQueue->Program)) {
    RxQueue->Dispatch = XdpRxExclusiveXskDispatch;
} else {
    RxQueue->Dispatch = XdpRxDispatch;
}

This micro-optimization actually improved perf a lot for "give me everything" workloads, but IDK if it's even possible to express this in eBPF. Theoretically we can also swap dispatch tables on the fly (i.e. after any program changes) if you want to experiment with that. You'd just swap the dispatch table within a callback from XdpRxQueueSync in theory.

csujedihy · 2023-01-30T23:39:13Z

This optimization is broken by this PR. The code might choose the fast path initially, and then subsequently added programs will be incorrectly ignored.
//
// Implement a fast path for a single XSK receiving all frames.
//
if (XdpProgramCanXskBypass(RxQueue->Program)) {
    RxQueue->Dispatch = XdpRxExclusiveXskDispatch;
} else {
    RxQueue->Dispatch = XdpRxDispatch;
}
This micro-optimization actually improved perf a lot for "give me everything" workloads, but IDK if it's even possible to express this in eBPF. Theoretically we can also swap dispatch tables on the fly (i.e. after any program changes) if you want to experiment with that. You'd just swap the dispatch table within a callback from XdpRxQueueSync in theory.

Made an attempt to fix it by swapping dispatch tables on the fly.

src/xdp/program.c

csujedihy · 2023-01-31T19:34:58Z

Hit this in local spinxsk run with FNDIS. It seems like the newly added rule and/or this PR make the fuzz'd socket setup tend to fail more often?

PS C:\Users\Administrator\Desktop\xdp> .\tools\spinxsk.ps1 -XdpmpPollProvider FNDIS -QueueCount 2 -Minutes 100
q[0]: socket setup success rate: (172 / 527) 32%
(successPct >= successThresholdPercent) failed line 1934
Press Ctrl+C to cancel the stop operation.
100%  [>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>]
The trace was successfully saved.
Press Ctrl+C to cancel the stop operation.
100%  [>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>]
The trace was successfully saved.
SpinXsk failed with 1
At C:\Users\Administrator\Desktop\xdp\tools\spinxsk.ps1:146 char:13
+             throw "SpinXsk failed with $LastExitCode"
+             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : OperationStopped: (SpinXsk failed with 1:String) [], RuntimeException
    + FullyQualifiedErrorId : SpinXsk failed with 1

mtfriesen · 2023-02-01T16:30:14Z

Hit this in local spinxsk run with FNDIS. It seems like the newly added rule and/or this PR make the fuzz'd socket setup tend to fail more often?

PS C:\Users\Administrator\Desktop\xdp> .\tools\spinxsk.ps1 -XdpmpPollProvider FNDIS -QueueCount 2 -Minutes 100
q[0]: socket setup success rate: (172 / 527) 32%
(successPct >= successThresholdPercent) failed line 1934
Press Ctrl+C to cancel the stop operation.
100%  [>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>]
The trace was successfully saved.
Press Ctrl+C to cancel the stop operation.
100%  [>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>]
The trace was successfully saved.
SpinXsk failed with 1
At C:\Users\Administrator\Desktop\xdp\tools\spinxsk.ps1:146 char:13
+             throw "SpinXsk failed with $LastExitCode"
+             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : OperationStopped: (SpinXsk failed with 1:String) [], RuntimeException
    + FullyQualifiedErrorId : SpinXsk failed with 1

Hmm, is this with the recommended driver verifier settings, including randomized allocation failures? If so, it is expected that the success rate is lower than the default value used in the script.

csujedihy · 2023-02-01T17:15:12Z

I believe so. I ran these commands:
.\tools\prepare-machine.ps1 -ForSpinxskTest
.\tools\spinxsk.ps1 -XdpmpPollProvider FNDIS -QueueCount 2 -Minutes 100

Ok, you're good, then. I usually run with a minimum success rate of 1% with driver verifier allocation failures enabled, which I think is similar to our CI/CD.

src/xdp/rx.c

csujedihy added 4 commits January 29, 2023 17:52

Done but still testing

12583f1

fix list not reset after removing entry

2a4d29f

remove sharing flag

e75b784

spell

ce9d98b

csujedihy requested a review from a team as a code owner January 30, 2023 02:53

csujedihy commented Jan 30, 2023

View reviewed changes

src/xdp/rx.c Outdated Show resolved Hide resolved

fix clean up in program attach

8c0086a

nibanks reviewed Jan 30, 2023

View reviewed changes

published/external/xdpapi.h Show resolved Hide resolved

mtfriesen reviewed Jan 30, 2023

View reviewed changes

src/xdp/program.c Outdated Show resolved Hide resolved

mtfriesen reviewed Jan 30, 2023

View reviewed changes

src/xdp/program.c Outdated Show resolved Hide resolved

mtfriesen reviewed Jan 30, 2023

View reviewed changes

src/xdp/rx.c Outdated Show resolved Hide resolved

mtfriesen reviewed Jan 30, 2023

View reviewed changes

cr feedback

f95ba4c

fix null deref found in spinxsk

a444c54

mtfriesen reviewed Jan 31, 2023

View reviewed changes

src/xdp/program.c Outdated Show resolved Hide resolved

mtfriesen reviewed Jan 31, 2023

View reviewed changes

src/xdp/program.c Outdated Show resolved Hide resolved

mtfriesen previously approved these changes Jan 31, 2023

View reviewed changes

fine grained pool tags and program rule bound check

3b15e49

csujedihy dismissed mtfriesen’s stale review via 3b15e49 January 31, 2023 01:31

csujedihy added 2 commits January 30, 2023 18:31

fix a bug in prog attach failure path

5677ed1

fix incorrect assert

18179d9

mtfriesen reviewed Feb 1, 2023

View reviewed changes

src/xdp/rx.c Outdated Show resolved Hide resolved

csujedihy added 2 commits February 1, 2023 11:02

cr feedback

17a5715

clean up

8d8ab2c

mtfriesen approved these changes Feb 1, 2023

View reviewed changes

csujedihy merged commit 5742c4b into main Feb 1, 2023

csujedihy deleted the huanyi/simplify-program-model branch February 1, 2023 21:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify/refactor the current program model #141

Simplify/refactor the current program model #141

csujedihy commented Jan 30, 2023 •

edited

csujedihy commented Jan 30, 2023 •

edited

mtfriesen Jan 30, 2023

csujedihy Jan 30, 2023

mtfriesen Jan 30, 2023

csujedihy Jan 31, 2023

csujedihy Jan 31, 2023

dthaler Feb 1, 2023

mtfriesen Feb 1, 2023

dthaler Feb 1, 2023

nibanks Feb 1, 2023

mtfriesen commented Jan 30, 2023 •

edited

csujedihy commented Jan 30, 2023

csujedihy commented Jan 31, 2023

mtfriesen commented Feb 1, 2023

csujedihy commented Feb 1, 2023 •

edited by mtfriesen

Simplify/refactor the current program model #141

Simplify/refactor the current program model #141

Conversation

csujedihy commented Jan 30, 2023 • edited

csujedihy commented Jan 30, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtfriesen commented Jan 30, 2023 • edited

csujedihy commented Jan 30, 2023

csujedihy commented Jan 31, 2023

mtfriesen commented Feb 1, 2023

csujedihy commented Feb 1, 2023 • edited by mtfriesen

csujedihy commented Jan 30, 2023 •

edited

csujedihy commented Jan 30, 2023 •

edited

mtfriesen commented Jan 30, 2023 •

edited

csujedihy commented Feb 1, 2023 •

edited by mtfriesen