Refactor UFFD #1409
Refactor UFFD #1409
Conversation
…el uffd handling to a subpackage
| } | ||
|
|
||
| func TestParallelMissingWrite(t *testing.T) { | ||
| // TODO: At around 10k+ parallel operations the test often freezes. |
There was a problem hiding this comment.
Same as for the parallel missing.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| hostVirtAddr: 0x1500, | ||
| expectedOffset: 0x5500, // 0x5000 + (0x1500 - 0x1000) | ||
| expectedSize: 4096, |
There was a problem hiding this comment.
if we're going to use hex, can we be consistent?
| hostVirtAddr: 0x1500, | |
| expectedOffset: 0x5500, // 0x5000 + (0x1500 - 0x1000) | |
| expectedSize: 4096, | |
| hostVirtAddr: 0x1500, | |
| expectedOffset: 0x5500, // 0x5000 + (0x1500 - 0x1000) | |
| expectedSize: 0x1000, |
| { | ||
| name: "address in gap between regions", | ||
| hostVirtAddr: 0x4000, | ||
| expectError: true, |
There was a problem hiding this comment.
Can we verify what the error was, and not just that there's an error and it's not nil?
There was a problem hiding this comment.
Currently, we don't return a specific error if the address cannot be resolved; the whole FC is essentially unrecoverable anyway.
|
|
||
| var _ MemoryBackend = (*Uffd)(nil) | ||
|
|
||
| func New(memfile block.ReadonlyDevice, socketPath string, blockSize int64) (*Uffd, error) { |
There was a problem hiding this comment.
can we get some tests covering this struct?
There was a problem hiding this comment.
Theoretically, yes, but this whole interaction is FC-specific (essentially passing fd + their struct), so not sure if that is worth it.
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This should be only a refactor now—I tried to minimize all changes.
The serving loop should be the same too.
The biggest problem I encountered is that there are seems to be performance limits of UFFD that we probably also can encounter in prod now. There are illustrated by the parallel missing and missing write tests (there is a
TODOthere).I'm not sure how we can resolve these—from the debugging I did it seems we won't even reach our code and it gets stuck before.
Note
Refactors UFFD to an object-based userfaultfd type and adds a dedicated memory mapping module with tests, updating UFFD wiring and tests accordingly.
userfaultfdpackage withUserfaultfdtype (Serve,Register,Close, internal copy/configure helpers).Serveinuffdwith instance-baseduserfaultfd.Userfaultfd.Serve; updatesuffd.Uffdto construct and manage handler viaNewUserfaultfdFromFd.uintptr.memorypackage withRegionandMapping(newGetOffsetlogic); deletes oldmappingpackage and Firecracker-specific types.mapping_test.go).userfaultfdAPI; centralizes missing request tracking viaUserfaultfd.regionMappingsSize), plumbing of fd-exit and logging through the new API.Written by Cursor Bugbot for commit 30fbc35. This will update automatically on new commits. Configure here.