KM XDPAPI prototype #409

nigriMSFT · 2023-11-02T23:38:17Z

KM XDPAPI prototype + hacks to test with xskbench and measure perf

nigriMSFT · 2023-11-02T23:44:49Z

Some results comparing the user-mode and kernel-mode APIs that were taken on a physical machine. The difference is mostly insignificant, which would be expected.

perf suite commands:

.\tools\xskperfsuite.ps1 -Verbose -Fndis -RawResultsFile "artifacts/logs/xskperfsuite-user.csv" -XperfDirectory "artifacts/logs" -Config Release
.\tools\xskperfsuite.ps1 -Verbose -Fndis -RawResultsFile "artifacts/logs/xskperfsuite-kernel.csv" -XperfDirectory "artifacts/logs" -KernelMode -Config Release

PS N:\repos\xdp-for-windows> .\tools\xskperfcompare.ps1 -DataFile1 N:\shared\nigri\httpperf\xskperfsuite-user.csv -DataFile2 N:\shared\nigri\httpperf\xskperfsuite-kernel-cleaned.csv
Test Case Avg1 Avg2 %Diff Significance
XDPMP-NATIVE-RX-BUSY-2048chunksize-64iosize-FNDIS 19314 19178 -1 NOT Significant
XDPMP-NATIVE-RX-BUSY-2048chunksize-1514iosize-FNDIS 5491 5478 0 NOT Significant
XDPMP-NATIVE-TX-BUSY-2048chunksize-64iosize-FNDIS 34434 34267 0 NOT Significant
XDPMP-NATIVE-TX-BUSY-2048chunksize-1514iosize-FNDIS 34462 34305 0 NOT Significant
XDPMP-NATIVE-FWD-BUSY-2048chunksize-64iosize-FNDIS 12897 12956 0 NOT Significant
XDPMP-NATIVE-FWD-BUSY-2048chunksize-1514iosize-FNDIS 4791 4784 0 NOT Significant
XDPMP-GENERIC-RX-BUSY-2048chunksize-64iosize-FNDIS 8699 8896 2 NOT Significant
XDPMP-GENERIC-RX-BUSY-2048chunksize-1514iosize-FNDIS 3832 3809 -1 NOT Significant
XDPMP-GENERIC-TX-BUSY-2048chunksize-64iosize-FNDIS 5691 5805 2 NOT Significant
XDPMP-GENERIC-TX-BUSY-2048chunksize-1514iosize-FNDIS 3455 3529 2 NOT Significant
XDPMP-GENERIC-FWD-BUSY-2048chunksize-64iosize-FNDIS 1841 1935 5 Significant
XDPMP-GENERIC-FWD-BUSY-2048chunksize-1514iosize-FNDIS 1214 1277 5 NOT Significant

*xskperfsuite-kernel-cleaned.csv as I had to undo the "KERNEL" added to the test case name in order for xskperfcompare to do its thing.

published/external/xdpapi.h

src/xdp/apikernel.c

src/xdp/program.h

src/xdp/xsk.c

test/xskbench/km/xskbenchdrvioctl.h

test/xskbench/um/xskbench_user.h

test/xskbench/xskbench.c

published/external/xdpapi.h

src/xdp/offload.c

mtfriesen · 2023-11-03T12:59:51Z

test/xskbench/km/sys/xskbench_kernel.h

+
+inline
+VOID
+PlatWaitThread(PLAT_THREAD *P, ULONG TimeoutMs)


Can we take a dependency on the CxPlat library used by MsQuic/usersim?

Unfortunately, not yet. We haven't done all the refactoring to move stuff into microsoft/cxplat yet.

That cxplat repository looks stale. SHould we take a dependency on / contribute to https://github.com/microsoft/usersim/tree/main/cxplat ?

@nibanks / @mtfriesen ^

No, don't add any extra dependencies yet.

Hmm, why not? A dependency seems better than duplicating code to me.

Ok I have the context now that providing facilities in cxplat is the end goal, but it just hasn't been prioritized yet. I think we can cheaply add a cxplat copy into xdp now and migrate to the proper cxplat once it is in place. This avoids any extra work adding and then untangling a temporary dependency on usersim.

microsoft/usersim#167

@nibanks This is being brought up again. cxplat copies are littered throughout microsoft github repositories (msquic, ebpf-for-windows, usersim, ntosebpfext, win-net-test). Adding another in xdp brings the count higher than can be counted on one hand. What is the tipping point here? Would it be reasonable to start populating the cxplat repo with what xdp needs and have xdp be the first consumer?

I absolutely want to move stuff there, but just don't have the time. But I do think we've reached that tipping point.

I can start this work by bringing in the cxplat pieces that xdp needs. Other workstreams can contribute more to cxplat as needed and eliminate cxplat copies when ready/possible. Does that seem like a reasonable plan?

test/xskbench/xskbench.c

src/xdp/xsk.c

mtfriesen · 2023-11-03T14:15:31Z

Should XskRequiresTxBounceBuffer return false for kernel sockets? The idea was user mode sockets in generic mode cannot be trusted for TOC/TOU, but for kernel, we could rely on the caller to promise not to modify the packet during TX.

mtfriesen · 2023-11-03T15:52:06Z

src/xdp/xsk.c

-        goto Exit;
-    }
+    if (RequestorMode != KernelMode) {
+        Umem->Mapping.Mdl =


Don't we always need an MDL, or at least need it if the bounce buffer is disabled and the underlying interface uses the MDL extension? Should be easy to repro a bugcheck if the XskDisableTxBounce regkey is set.

Fixed. Needs a test case though

src/xdp/xsk.c

published/external/afxdp.h

published/private/xdpioctl.h

test/functional/taef/tests.cpp

test/xskbench/km/sys/xskbenchdrv.vcxproj

mtfriesen · 2024-09-30T21:03:04Z

test/xskbench/km/sys/xskbenchdrv.vcxproj

+    </Link>
+  </ItemDefinitionGroup>
+  <Target Name="BuildCxPlat" BeforeTargets="PrepareForBuild">
+      <MSBuild Projects="$(SolutionDir)submodules\cxplat\src\lib\cxplat.kernel.vcxproj" Properties="UndockedOut=$(OutDir)cxplat\"/>


hm, we need to find a way to not build cxplat multiple times. we probably need to create a single dummy project in xdp now that it's being referenced in multiple places.

…T/km-proto

nibanks · 2024-10-02T15:06:16Z

published/external/xdpapi.h

+
+inline
+NTSTATUS
+XdpNmrClientDetachProvider(


According to MSDN this may be called at DISPATCH_LEVEL. This means we must not use locks that require PASSIVE_LEVEL, right?

Also, for the question of client cleaning up in the Detach call, that means it could not wait for cleanup, right? That seems weird, to require clean up always supporting DISPATCH_LEVEL...

Also in the documentation, a client can return STATUS_PENDING and then call NmrClientDetachProviderComplete at a later time. Client "cleanup" logic can be moved to ClientCleanupBindingContext which happens after both client and provider have completed detach.

As far as I understand, it is possible to use PASSIVE_LEVEL lock with DISPATCH_LEVEL function.
If multiple level components are mixed, lower level IRQL need to be used. PASSIVE_LEVEL.

It is possible, but the code will be inherently buggy. NickB is correct that we must not use passive level locks. Windows Internals is a highly recommended resource for this concept, I'll send a link offline

clientDetachProvider and providerDetachClient are called at dispatch level only when you deregister client/provider at dispatch level. If we have not treated these as functions that can be called at dispatch elsewhere in xdp like XdpNmrClientDetachProvider, why do we care now?

As far as I understand, it is possible to use PASSIVE_LEVEL lock with DISPATCH_LEVEL function.

As NickG mentioned, you cannot use a passive lock at DISPATCH_LEVEL (because the lock can block/wait, which isn't allowed at dispatch). But you can use a dispatch lock at PASSIVE_LEVEL.

clientDetachProvider and providerDetachClient are called at dispatch level only when you deregister client/provider at dispatch level. If we have not treated these as functions that can be called at dispatch elsewhere in xdp like XdpNmrClientDetachProvider, why do we care now?

The NMR interface is generically defined to allow DISPATCH_LEVEL. My preference would be to not impose restrictions based on some assumptions for how XDP uses the interface today. What if XDP needs to be updated to deregister at dispatch in the future? @mtfriesen any opinion here?

For code that is compiled into 3rd party code rather than something we own ourselves, we should do the right thing. IMO we should also fix code in XDP that makes incorrect assumptions about IRQL, but do that in a separate PR.

* unified close handle api * add one more API to XDP_FILE_DISPATCH * XdpDeleteProgram to follow the change, fix static annotations