-
Notifications
You must be signed in to change notification settings - Fork 434
[for testing AI review] unified epoll for ebpf #2322
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.
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
base: main
Are you sure you want to change the base?
Conversation
* unify the retry cache and change pollperfbuffer to consumeperfbuffer * fix merge issue and fix clang format * fix perf buffer special cases * delete poll perf buffer of process * add mEpollFdToPluginType map lock * fix ut and add ut * fix ut
There was a problem hiding this 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 implements unified epoll monitoring for eBPF plugins to replace individual polling mechanisms. The changes aim to improve performance by monitoring multiple perf buffer file descriptors through a single epoll instance.
- Introduces unified epoll system for monitoring eBPF perf buffers across all plugins
- Refactors RetryableEventCache to be shared across managers instead of per-manager instances
- Enables FileSecurityManager tests that were previously commented out
Reviewed Changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
ProcessCacheManagerWrapper.h | Updates constructor to use shared RetryableEventCache reference |
NetworkObserverUnittest.cpp | Updates test setup to use shared RetryableEventCache |
ManagerUnittest.cpp | Enables FileSecurityManager tests and updates constructors |
FileSecurityManagerUnittest.cpp | Updates test setup with proper EBPFAdapter and RetryableEventCache |
EBPFServerUnittest.cpp | Adds comprehensive tests for unified epoll and retry cache functionality |
ProcessSecurityManager.h | Adds ConsumePerfBufferData override method |
NetworkObserverManager.h | Adds ConsumePerfBufferData override method |
FileSecurityManager.h | Updates constructor signature to accept RetryableEventCache reference |
FileSecurityManager.cpp | Removes PollPerfBuffer method and cache management logic |
ProcessCacheManager.h/cpp | Refactors to use shared RetryableEventCache and separate polling concerns |
AbstractManager.h | Adds default ConsumePerfBufferData implementation |
eBPFDriver.h/cpp | Implements functions for getting epoll FDs and consuming perf buffer data |
BPFWrapper.h | Adds methods for perf buffer epoll integration |
EBPFServer.h/cpp | Implements unified epoll monitoring system and shared event cache management |
EBPFAdapter.h/cpp | Adds support for epoll FD retrieval and data consumption |
execveEvent->mPKtime = 6789; | ||
|
||
// 测试缓存更新 | ||
mProcessCacheManager->mProcessCache.AddCache(key, execveEvent); |
Copilot
AI
Jul 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The AddCache method is being called with a shared_ptr directly instead of using std::move(). This could lead to unexpected reference counting behavior. Consider using std::move(execveEvent) to transfer ownership clearly.
mProcessCacheManager->mProcessCache.AddCache(key, execveEvent); | |
mProcessCacheManager->mProcessCache.AddCache(key, std::move(execveEvent)); |
Copilot uses AI. Check for mistakes.
mProcessCacheManager->mProcessCache.AddCache(key, execveEvent); | ||
auto pExecveEvent = std::make_shared<ProcessCacheValue>(); | ||
data_event_id pkey{2345, 6789}; | ||
mProcessCacheManager->mProcessCache.AddCache(pkey, pExecveEvent); |
Copilot
AI
Jul 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the previous call, consider using std::move(pExecveEvent) to transfer ownership clearly when adding to the cache.
mProcessCacheManager->mProcessCache.AddCache(pkey, pExecveEvent); | |
mProcessCacheManager->mProcessCache.AddCache(pkey, std::move(pExecveEvent)); |
Copilot uses AI. Check for mistakes.
for (int testFd : testFds) { | ||
uint64_t value = 1; | ||
ssize_t written = write(testFd, &value, sizeof(value)); | ||
(void)written; |
Copilot
AI
Jul 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The write() return value is being explicitly ignored. Consider checking the return value to ensure the write operation succeeded, especially in test code where failures should be detected.
(void)written; | |
if (written != sizeof(value)) { | |
LOG_ERROR(sLogger, ("Failed to write to eventfd", testFd)("written", written)("expected", sizeof(value))); | |
keepRunning = false; // Stop the test if write fails | |
} |
Copilot uses AI. Check for mistakes.
return; | ||
} | ||
|
||
int numEvents = epoll_wait(mUnifiedEpollFd, mEpollEvents.data(), mEpollEvents.size(), 100); |
Copilot
AI
Jul 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The timeout value of 100ms is hardcoded. Consider defining this as a named constant to improve maintainability and make the timeout configurable if needed.
int numEvents = epoll_wait(mUnifiedEpollFd, mEpollEvents.data(), mEpollEvents.size(), 100); | |
int numEvents = epoll_wait(mUnifiedEpollFd, mEpollEvents.data(), mEpollEvents.size(), kEpollTimeoutMs); |
Copilot uses AI. Check for mistakes.
return; | ||
} | ||
|
||
mEpollEvents.resize(1024); |
Copilot
AI
Jul 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The epoll events buffer size of 1024 is hardcoded. Consider defining this as a named constant to improve maintainability and make it easier to tune performance.
mEpollEvents.resize(1024); | |
mEpollEvents.resize(DEFAULT_EPOLL_EVENTS_BUFFER_SIZE); |
Copilot uses AI. Check for mistakes.
No description provided.