issue: 4432879 Refactor event_handler_manager

[pasis]

Description

Avoid start_thread with each reg action.
Refactor destructor.

What

Avoid start_thread with each reg action.
Refactor destructor.

Why ?

Potential CPU improvement for CPS and code cleanup.

Change type

What kind of change does this PR introduce?

• Bugfix
• Feature
• Code style update
• Refactoring (no functional changes, no api changes)
• Build related changes
• CI related changes
• Documentation content changes
• Tests
• Other

Check list

• Code follows the style de facto guidelines of this project
• Comments have been inserted in hard to understand places
• Documentation has been updated (if necessary)
• Test has been added (if possible)

[galnoam]

@pasis, can we keep this in your privare fork?

[galnoam]

bot:retest

[galnoam]

bot:retest

[galnoam]

bot:retest

[galnoam]

/review

[galnoam]

/improve

[pr-review-bot-app]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Resource Cleanup Ensure that the m_epfd resource is properly closed and reset in the destructor to avoid resource leaks or undefined behavior. if (m_epfd >= 0) { SYSCALL(close, m_epfd); } m_epfd = -1; Thread Affinity Handling Validate the logic for setting thread affinity and fallback mechanisms in start_thread to ensure compatibility and robustness across different environments. bool affinity_requested = false; if (!m_b_continue_running) { errno = ECANCELED; return -1; } if (m_event_handler_tid != 0) { return 0; } ret = pthread_attr_init(&tattr); if (ret != 0) { return -1; } cpu_set = safe_mce_sys().internal_thread_affinity; if (strcmp(safe_mce_sys().internal_thread_affinity_str, "-1") && !strcmp(safe_mce_sys().internal_thread_cpuset, MCE_DEFAULT_INTERNAL_THREAD_CPUSET)) { ret = pthread_attr_setaffinity_np(&tattr, sizeof(cpu_set), &cpu_set); if (ret != 0) { evh_logwarn("Failed to set event handler thread affinity. (errno=%d)", errno); } affinity_requested = (ret == 0); } else { evh_logdbg("Internal thread affinity not set."); } ret = pthread_create(&m_event_handler_tid, &tattr, event_handler_thread, this); if (ret != 0 && affinity_requested) { // Try without affinity in case this is a cset issue. evh_logwarn("Failed to start event handler thread with a thread affinity. Trying default " "thread affinity. (errno=%d)", errno); pthread_attr_destroy(&tattr); ret = pthread_attr_init(&tattr) ?: pthread_create(&m_event_handler_tid, &tattr, event_handler_thread, this); } // Destroy will either succeed or return EINVAL if the init fails in the above block. pthread_attr_destroy(&tattr); if (ret == 0) { evh_logdbg("Started event handler thread."); } else { evh_logerr("Failed to start event handler thread. (errno=%d)", errno); } return ret; Exception Handling Confirm that throwing an exception when start_thread fails in do_global_ctors_helper is the appropriate behavior and won't cause unintended side effects during initialization. if (rc != 0) { BULLSEYE_EXCLUDE_BLOCK_START throw_xlio_exception("Failed to start event handler thread.\n"); BULLSEYE_EXCLUDE_BLOCK_END }

[pr-review-bot-app]

PR Code Suggestions ✨

Category	Suggestion	Impact
Possible issue	Add error handling for thread initialization Add error handling for the pthread_attr_init and pthread_create calls to ensure that any failure in these operations is logged or handled appropriately, avoiding potential undefined behavior. src/core/event/event_handler_manager.cpp [347-348] -ret = pthread_attr_init(&tattr) - ?: pthread_create(&m_event_handler_tid, &tattr, event_handler_thread, this); +ret = pthread_attr_init(&tattr); +if (ret != 0) { + evh_logerr("Failed to initialize thread attributes. (errno=%d)", errno); + return ret; +} +ret = pthread_create(&m_event_handler_tid, &tattr, event_handler_thread, this); +if (ret != 0) { + evh_logerr("Failed to create event handler thread. (errno=%d)", errno); + pthread_attr_destroy(&tattr); + return ret; +} Suggestion importance[1-10]: 8 __ Why: The suggestion improves robustness by adding error handling for pthread_attr_init and pthread_create. This ensures that failures in thread initialization are logged and handled properly, reducing the risk of undefined behavior.	Medium
	Add null check for pointer access Add a null check for m_p_reg_action_q_to_push_to before accessing it to prevent potential segmentation faults if the pointer is null. src/core/event/event_handler_manager.cpp [446] -is_empty = m_p_reg_action_q_to_push_to->empty(); +is_empty = (m_p_reg_action_q_to_push_to && m_p_reg_action_q_to_push_to->empty()); Suggestion importance[1-10]: 8 __ Why: Adding a null check for m_p_reg_action_q_to_push_to prevents potential segmentation faults, which is a significant improvement for code safety and stability.	Medium
	Prevent resource leaks in thread attributes Ensure that the pthread_attr_destroy is called after the second pthread_attr_init to avoid resource leaks in case of repeated initialization. src/core/event/event_handler_manager.cpp [341-349] if (ret != 0 && affinity_requested) { evh_logwarn("Failed to start event handler thread with a thread affinity. Trying default " "thread affinity. (errno=%d)", errno); pthread_attr_destroy(&tattr); - ret = pthread_attr_init(&tattr) - ?: pthread_create(&m_event_handler_tid, &tattr, event_handler_thread, this); + ret = pthread_attr_init(&tattr); + if (ret == 0) { + ret = pthread_create(&m_event_handler_tid, &tattr, event_handler_thread, this); + pthread_attr_destroy(&tattr); + } } Suggestion importance[1-10]: 7 __ Why: The suggestion ensures that pthread_attr_destroy is called after the second pthread_attr_init, preventing resource leaks. This is a good improvement for resource management and aligns with best practices.	Medium
General	Log error code before exception Log the error code before throwing the exception to provide more context for debugging failures in starting the event handler thread. src/core/main.cpp [1000-1003] rc = g_p_event_handler_manager->start_thread(); if (rc != 0) { + evh_logerr("Failed to start event handler thread. (rc=%d)", rc); BULLSEYE_EXCLUDE_BLOCK_START throw_xlio_exception("Failed to start event handler thread.\n"); BULLSEYE_EXCLUDE_BLOCK_END } Suggestion importance[1-10]: 6 __ Why: Logging the error code before throwing an exception provides valuable debugging information. While not critical, it enhances the ability to diagnose issues when the thread fails to start.	Low

[galnoam]

Hi @pasis , @AlexanderGrissik , please check pr agent review comments.

[pasis]

bot:retest

[greptile-apps]

Greptile Overview

Greptile Summary

Refactors event handler manager initialization to start the thread once during XLIO initialization instead of on every registration action.

Key changes:

• Moved start_thread() call from post_new_reg_action() to do_global_ctors_helper() in main.cpp
• Made start_thread() public to enable explicit initialization control
• Removed free_evh_resources() method, inlining cleanup logic into destructor
• Simplified constructor by setting m_b_continue_running = true unconditionally before mode check
• Improved error handling in start_thread() with better logging and explicit error returns

Benefits:

• Eliminates redundant thread creation checks on every registration (CPS performance improvement)
• Cleaner separation of concerns - initialization vs. registration
• More predictable thread lifecycle management

Critical issue found:

• Line 338-339 in event_handler_manager.cpp: Logic error in fallback path where pthread_create may be called with uninitialized attributes if second pthread_attr_init fails

Confidence Score: 2/5

• This PR contains a critical logic bug that must be fixed before merging
• The refactoring concept is sound and improves performance, but the retry logic in start_thread() (lines 338-339) has a critical bug where pthread_create can be called with an uninitialized tattr if the second pthread_attr_init fails. This could cause undefined behavior or crashes during thread creation fallback scenarios.
• src/core/event/event_handler_manager.cpp (lines 338-339) - fix the compound statement logic error

Important Files Changed

File Analysis

Filename	Score	Overview
src/core/event/event_handler_manager.cpp	2/5	Refactored start_thread with improved error handling but has critical logic bug in retry path (line 338-339)
src/core/event/event_handler_manager.h	5/5	Made start_thread() public and removed free_evh_resources() declaration - clean refactoring
src/core/main.cpp	5/5	Added explicit start_thread() call with error handling during initialization - improves control flow

Sequence Diagram

sequenceDiagram
    participant Main as main.cpp
    participant EHM as event_handler_manager
    participant Thread as event_handler_thread
    participant Reg as Registration callers

    Note over Main: XLIO initialization (do_global_ctors_helper)
    Main->>EHM: NEW_CTOR(g_p_event_handler_manager, event_handler_manager())
    activate EHM
    EHM->>EHM: m_b_continue_running = true
    EHM->>EHM: epoll_create(m_epfd)
    EHM->>EHM: wakeup_set_epoll_fd(m_epfd)
    EHM-->>Main: return
    deactivate EHM
    
    Main->>EHM: start_thread()
    activate EHM
    EHM->>EHM: pthread_attr_init(&tattr)
    EHM->>EHM: pthread_attr_setaffinity_np (optional)
    EHM->>Thread: pthread_create(&m_event_handler_tid, &tattr, event_handler_thread, this)
    activate Thread
    EHM-->>Main: return 0 (success)
    deactivate EHM
    
    Note over Main: Continue with initialization<br/>Create g_p_ib_ctx_handler_collection,<br/>g_p_net_device_table_mgr, etc.
    
    par Event Handler Thread Loop
        Thread->>Thread: thread_loop()
        loop while m_b_continue_running
            Thread->>Thread: m_timer.update_timeout()
            Thread->>Thread: epoll_wait(m_epfd, ...)
            Thread->>Thread: process registration actions
            Thread->>Thread: process events
        end
    and Registration Actions (from other components)
        Reg->>EHM: register_ibverbs_event()
        activate EHM
        EHM->>EHM: post_new_reg_action(reg_action)
        Note over EHM: OLD: Called start_thread() here<br/>NEW: Thread already running
        EHM->>EHM: m_p_reg_action_q_to_push_to->push_back()
        EHM->>EHM: do_wakeup()
        EHM-->>Reg: return
        deactivate EHM
        EHM-->>Thread: wakeup signal
        Thread->>Thread: handle_registration_action()
    end
    
    Note over Main: Shutdown sequence
    Main->>EHM: ~event_handler_manager()
    activate EHM
    EHM->>EHM: stop_thread()
    EHM->>EHM: m_b_continue_running = false
    EHM->>Thread: do_wakeup()
    Thread->>Thread: exit thread_loop()
    Thread->>Thread: process_remaining_registration_actions()
    deactivate Thread
    EHM->>EHM: pthread_join(m_event_handler_tid)
    EHM->>EHM: close(m_epfd)
    deactivate EHM

Loading

_{3 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Greptile Overview

Greptile Summary

Refactors event handler manager to start the internal thread once during global initialization instead of on each registration action, improving CPS performance.

Key Changes:

• Moved start_thread() call from post_new_reg_action() to global initialization in main.cpp:1084
• Simplified destructor by removing free_evh_resources() and moving cleanup logic directly into destructor
• Improved error handling in start_thread() with proper return values and cleanup
• Moved m_b_continue_running initialization outside internal thread mode check for consistency

Issues:

• None identified; the refactoring is clean and improves the design

Confidence Score: 4/5

• This PR is safe to merge with minimal risk - it's a clean refactoring that improves performance
• Score reflects well-structured refactoring with proper error handling. The change moves thread initialization from lazy (on first registration) to eager (during global init), which is cleaner and improves CPS performance. Error handling is improved with explicit return codes. The only minor concern is the errno usage pattern, but this is consistent with the existing codebase
• No files require special attention - all changes are straightforward refactoring

Important Files Changed

File Analysis

Filename	Score	Overview
src/core/event/event_handler_manager.cpp	4/5	Refactored thread initialization to occur during global construction instead of on each registration action; improved error handling and removed free_evh_resources() method
src/core/event/event_handler_manager.h	5/5	Moved start_thread() to public interface and removed free_evh_resources() declaration
src/core/main.cpp	4/5	Added explicit event handler thread startup during global initialization with error checking

Sequence Diagram

sequenceDiagram
    participant Main as main.cpp
    participant EHM as event_handler_manager
    participant Thread as event_handler_thread
    
    Note over Main: Global Initialization
    Main->>EHM: NEW_CTOR(g_p_event_handler_manager)
    activate EHM
    EHM->>EHM: Constructor (internal_thread_mode=true)
    EHM->>EHM: m_epfd = epoll_create()
    EHM->>EHM: m_b_continue_running = true
    EHM-->>Main: event_handler_manager instance
    deactivate EHM
    
    Main->>EHM: start_thread()
    activate EHM
    EHM->>EHM: pthread_attr_init(&tattr)
    alt affinity configured
        EHM->>EHM: pthread_attr_setaffinity_np()
    end
    EHM->>Thread: pthread_create()
    activate Thread
    Thread->>Thread: thread_loop()
    EHM-->>Main: 0 (success)
    deactivate EHM
    
    Note over Main: Registration Actions
    Main->>EHM: register_timer_event() / register_ibverbs_event()
    activate EHM
    EHM->>EHM: post_new_reg_action()
    Note over EHM: Thread already running<br/>No start_thread() call
    EHM->>Thread: do_wakeup()
    EHM-->>Main: 
    deactivate EHM
    
    Thread->>Thread: process registration actions
    
    Note over Main: Shutdown
    Main->>EHM: ~event_handler_manager()
    activate EHM
    EHM->>EHM: stop_thread()
    EHM->>Thread: m_b_continue_running = false
    EHM->>Thread: do_wakeup()
    EHM->>Thread: pthread_join()
    Thread-->>EHM: thread exits
    deactivate Thread
    EHM->>EHM: close(m_epfd)
    deactivate EHM

Loading

[greptile-apps]

_{3 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile
React with 👍 or 👎 to share your feedback on this new summary format}

[greptile-apps]

_{3 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}