Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add and remove CPU from epoch computation on active and idle #3920

Closed
wants to merge 4 commits into from
Closed

Add and remove CPU from epoch computation on active and idle #3920

wants to merge 4 commits into from
+559 −43

Conversation

Alan-Jowett
Copy link
Member

@Alan-Jowett Alan-Jowett commented Oct 8, 2024 •
edited
Loading

Description

Restore the changes reverted by 205802b and fix a bug that was causing use after free (the for loop for computing oldest epoch was malformed resulting in a no-op).

Note: This is intentionally three or more commits:

  1. Restore the reverted code (including the bug).
  2. Fix the bug.
  3. PR feedback and addition of new tests.

Testing

CI/CD + 24 hour run of api_test.exe (previously failing after 2 hours).

Documentation

Yes, in the files.

Installation

No.

@Alan-Jowett
Copy link
Member Author

@CodiumAI-Agent /review

@CodiumAI-Agent
Copy link

CodiumAI-Agent commented Oct 8, 2024 •
edited
Loading

PR Reviewer Guide 🔍

(Review updated until commit af048fb)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Thread Safety
The code modifies shared state without sufficient synchronization mechanisms in several places, which could lead to race conditions or data corruption in a multi-threaded environment.

Resource Leak
The new implementation introduces potential resource leaks where allocated resources on error paths are not always properly freed or handled.

Error Handling
Error paths may not correctly restore previous states, potentially leaving the system in an inconsistent state after an error.

dthaler
dthaler reviewed Oct 8, 2024
View reviewed changes
dthaler
dthaler reviewed Oct 8, 2024
View reviewed changes
@Alan-Jowett Alan-Jowett force-pushed the cpu_activation branch 6 times, most recently from 7b60ca4 to 6dfd02b Compare October 9, 2024 18:24
hawkinsw
hawkinsw reviewed Oct 10, 2024
View reviewed changes
Copy link
Collaborator

@hawkinsw hawkinsw left a comment

Choose a reason for hiding this comment

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

I hope that what I wrote isn't too annoying.

@Alan-Jowett Alan-Jowett marked this pull request as ready for review October 10, 2024 04:11
@Alan-Jowett
Copy link
Member Author

I hope that what I wrote isn't too annoying.

No, I loved it. I dislike that en-gb and en-us differ like this :-P

@CodiumAI-Agent
Copy link

Persistent review updated to latest commit af048fb

dthaler
dthaler reviewed Oct 10, 2024
View reviewed changes
@Alan-Jowett Alan-Jowett force-pushed the cpu_activation branch 2 times, most recently from 1b357e6 to 0bb37d0 Compare October 10, 2024 18:53
@Alan-Jowett Alan-Jowett marked this pull request as draft October 16, 2024 22:14
@Alan-Jowett
Copy link
Member Author

Converting to draft until usersim bug is fixed.

@Alan-Jowett Alan-Jowett force-pushed the cpu_activation branch 2 times, most recently from 88d6e89 to 98263a7 Compare November 3, 2024 23:00
@Alan-Jowett Alan-Jowett force-pushed the cpu_activation branch 2 times, most recently from d4a4a2b to 9ed7807 Compare November 21, 2024 23:39
@Alan-Jowett Alan-Jowett marked this pull request as ready for review November 21, 2024 23:39
@Alan-Jowett
Copy link
Member Author

Alan-Jowett commented Dec 3, 2024 •
edited
Loading

Switching to draft until usersim is fixed microsoft/usersim#224

@Alan-Jowett Alan-Jowett marked this pull request as draft December 3, 2024 17:16
@Alan-Jowett Alan-Jowett marked this pull request as ready for review January 6, 2025 20:28
@Alan-Jowett Alan-Jowett requested a review from Copilot January 21, 2025 19:03
Copilot
Copilot AI reviewed Jan 21, 2025
View reviewed changes

Choose a reason for hiding this comment

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

Copilot wasn't able to review any files in this pull request.

Files not reviewed (5)
  • libs/runtime/ebpf_epoch.c: Language not supported
  • libs/runtime/ebpf_epoch.h: Language not supported
  • libs/runtime/ebpf_work_queue.c: Language not supported
  • libs/runtime/ebpf_work_queue.h: Language not supported
  • libs/runtime/unit/platform_unit_test.cpp: Language not supported
shankarseal
shankarseal reviewed Feb 14, 2025
View reviewed changes
libs/runtime/ebpf_epoch.c
/**
* @brief Lock to ensure a consistent view of the active CPUs.
*/
static ebpf_lock_t _ebpf_epoch_active_cpu_list_lock; ///< Lock to protect the active CPU list.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider renaming as this protects the cpu entry table and not only the active cpu list.

shankarseal
shankarseal reviewed Feb 14, 2025
View reviewed changes
libs/runtime/ebpf_epoch.c
@@ -379,6 +450,10 @@ ebpf_epoch_enter(_Out_ ebpf_epoch_state_t* epoch_state)
epoch_state->epoch = cpu_entry->current_epoch;
ebpf_list_insert_tail(&cpu_entry->epoch_state_list, &epoch_state->epoch_list_entry);

if (!cpu_entry->active) {
_ebpf_epoch_activate_cpu(epoch_state->cpu_id);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider having a _ebpf_epoch_activate_cpu_in_dispatch variation of the function as well, to make the code more readable.

shankarseal
shankarseal reviewed Feb 14, 2025
View reviewed changes
libs/runtime/ebpf_epoch.c
// If the epoch was EBPF_EPOCH_UNKNOWN_EPOCH (i.e. the CPU was activating when this memory was queued), then
// update the freed_epoch for all items in the free list now that we know the current epoch. This occurs when
// the CPU is activated and continues until the first epoch is proposed.
if (cpu_entry->current_epoch == EBPF_EPOCH_UNKNOWN_EPOCH) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion: assert this CPU active flag is set to TRUE.

@Alan-Jowett
Copy link
Member Author

Closing out this PR as it makes the already complex epoch code more complicated.

auto-merge was automatically disabled February 14, 2025 20:02

Pull request was closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dthaler dthaler dthaler left review comments

@hawkinsw hawkinsw hawkinsw left review comments

@shankarseal shankarseal shankarseal left review comments

Copilot code review Copilot Copilot left review comments

@mikeagun mikeagun mikeagun approved these changes

@poornagmsft poornagmsft Awaiting requested review from poornagmsft

@saxena-anurag saxena-anurag Awaiting requested review from saxena-anurag

@dv-msft dv-msft Awaiting requested review from dv-msft

@gtrevi gtrevi Awaiting requested review from gtrevi

@shpalani shpalani Awaiting requested review from shpalani

@matthewige matthewige Awaiting requested review from matthewige

@mtfriesen mtfriesen Awaiting requested review from mtfriesen

@rectified95 rectified95 Awaiting requested review from rectified95

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Alan-Jowett @CodiumAI-Agent @mikeagun @dthaler @hawkinsw @shankarseal