Skip to content

Conversation

ValentaTomas
Copy link
Member

@ValentaTomas ValentaTomas commented Aug 7, 2025

  • Extract low level UFFD handler to a separate package
  • Remove unused struct
  • Improve uffd tests (ensure they are not flaky)
    • Add the 2M hugpages tests

Note

Extracts UFFD logic into a new userfaultfd package with memory mapping abstractions, updates orchestrator integration, and adds comprehensive tests and helpers.

  • Refactor UFFD handling:
    • Move low-level logic into packages/orchestrator/internal/sandbox/uffd/userfaultfd with a userfaultfd wrapper (Serve, Register, AddWriteProtection, Close, copy).
    • Replace mapping with memory module: introduce MemoryMap interface and MemfileMap with GetOffset; update Firecracker mapping to package memory.
    • Update orchestrator uffd.go to use memory.MemfileMap and the new userfaultfd instance (fd management via wrapper, Serve method); add writeProtection field.
    • Tweak UFFD constants/bindings (add UFFDIO_COPY_MODE_DONTWAKE, UFFDIO_WAKE; adjust address types and copy dst handling).
  • Testing:
    • Add testutils (contiguous map, mmap creators, test data helpers).
    • Add userfaultfd tests for missing, write-protect, combined modes, and cross-process double registration.

Written by Cursor Bugbot for commit 67b1bfb. This will update automatically on new commits. Configure here.

@ValentaTomas ValentaTomas self-assigned this Aug 7, 2025
@ValentaTomas ValentaTomas added the improvement Improvement for current functionality label Aug 7, 2025
@ValentaTomas ValentaTomas changed the title Extract low level uffd handler to a separate package Extract low level UFFD handler to a separate package Aug 7, 2025
@ValentaTomas ValentaTomas removed their assignment Sep 21, 2025
@ValentaTomas ValentaTomas marked this pull request as ready for review October 4, 2025 00:13
@ValentaTomas
Copy link
Member Author

I'll improve the tests a little, but the rest should be ready.

@ValentaTomas ValentaTomas requested review from Copilot and removed request for dobrac and jakubno October 4, 2025 00:22
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR extracts the low-level user fault file descriptor (UFFD) handler into a separate userfaultfd package, improving code organization and modularity. The extraction refactors the UFFD functionality out of the main uffd package into a dedicated userfaultfd subpackage.

  • Moved UFFD handling logic from uffd package to new userfaultfd package
  • Created abstraction layer with memory mapping interfaces
  • Added comprehensive test coverage for UFFD functionality including cross-process scenarios

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
packages/orchestrator/internal/sandbox/uffd/userfaultfd/userfaultfd_test.go Comprehensive test suite for UFFD missing page and write protection scenarios
packages/orchestrator/internal/sandbox/uffd/userfaultfd/userfaultfd_cross_process_test.go Cross-process UFFD tests simulating FC-orchestrator interaction
packages/orchestrator/internal/sandbox/uffd/userfaultfd/userfaultfd.go Core UFFD operations including registration and copy functionality
packages/orchestrator/internal/sandbox/uffd/userfaultfd/serve.go UFFD event handling and page fault processing logic
packages/orchestrator/internal/sandbox/uffd/userfaultfd/constants.go UFFD constants and helper functions with improved address handling
packages/orchestrator/internal/sandbox/uffd/uffd.go Updated to use new userfaultfd package and memory interfaces
packages/orchestrator/internal/sandbox/uffd/testutils/ Test utilities for memory mapping and data preparation
packages/orchestrator/internal/sandbox/uffd/memory/ Memory mapping interfaces and Firecracker-specific implementations
Comments suppressed due to low confidence (2)

packages/orchestrator/internal/sandbox/uffd/userfaultfd/constants.go:1

  • The constant UFFD_FEATURE_WP_HUGETLBFS_SHMEM is being removed but may be needed for write protection features. Verify this removal is intentional and doesn't break functionality.
package userfaultfd

packages/orchestrator/internal/sandbox/uffd/uffd.go:1

  • The TrackAndReturnNil method is being removed. Ensure all callers of this method have been updated to handle its removal.
package uffd

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@ValentaTomas
Copy link
Member Author

bugbot run

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no bugs!


@ValentaTomas
Copy link
Member Author

ValentaTomas commented Oct 5, 2025

✅ Extracted all non-relevant (mostly write-protection and direct memory access related) code to separate PRs so it can be merged in steps.

Copy link
Contributor

@djeebus djeebus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Just added some opinions, none of which are critical.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement for current functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants