[for testing AI review] unified epoll for ebpf #2322
[for testing AI review] unified epoll for ebpf #2322xiongyunn wants to merge 1 commit intoalibaba:mainfrom
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.
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); |
There was a problem hiding this comment.
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)); |
| mProcessCacheManager->mProcessCache.AddCache(key, execveEvent); | ||
| auto pExecveEvent = std::make_shared<ProcessCacheValue>(); | ||
| data_event_id pkey{2345, 6789}; | ||
| mProcessCacheManager->mProcessCache.AddCache(pkey, pExecveEvent); |
There was a problem hiding this comment.
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)); |
| for (int testFd : testFds) { | ||
| uint64_t value = 1; | ||
| ssize_t written = write(testFd, &value, sizeof(value)); | ||
| (void)written; |
There was a problem hiding this comment.
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 | |
| } |
| return; | ||
| } | ||
|
|
||
| int numEvents = epoll_wait(mUnifiedEpollFd, mEpollEvents.data(), mEpollEvents.size(), 100); |
There was a problem hiding this comment.
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); |
| return; | ||
| } | ||
|
|
||
| mEpollEvents.resize(1024); |
There was a problem hiding this comment.
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); |
No description provided.