• debing.sun's avatar
    Make RM_Yield thread-safe (#12905) · ca1f67af
    debing.sun authored
    ## Issues and solutions from #12817
    1. Touch ProcessingEventsWhileBlocked and calling moduleCount() without
    GIL in afterSleep()
        - Introduced: 
           Version: 7.0.0
           PR: #9963
    
       - Harm Level: Very High
    If the module thread calls `RM_Yield()` before the main thread enters
    afterSleep(),
    and modifies `ProcessingEventsWhileBlocked`(+1), it will cause the main
    thread to not wait for GIL,
    which can lead to all kinds of unforeseen problems, including memory
    data corruption.
    
       - Initial / Abandoned Solution:
          * Added `__thread` specifier for ProcessingEventsWhileBlocked.
    `ProcessingEventsWhileBlocked` is used to protect against nested event
    processing, but event processing
    in the main thread and module threads should be completely independent
    and unaffected, so it is safer
             to use TLS.
    * Adding a cached module count to keep track of the current number of
    modules, to avoid having to use `dictSize()`.
        
        - Related Warnings:
    ```
    WARNING: ThreadSanitizer: data race (pid=1136)
      Write of size 4 at 0x0001045990c0 by thread T4 (mutexes: write M0):
        #0 processEventsWhileBlocked networking.c:4135 (redis-server:arm64+0x10006d124)
        #1 RM_Yield module.c:2410 (redis-server:arm64+0x10018b66c)
        #2 bg_call_worker <null>:83232836 (blockedclient.so:arm64+0x16a8)
    
      Previous read of size 4 at 0x0001045990c0 by main thread:
        #0 afterSleep server.c:1861 (redis-server:arm64+0x100024f98)
        #1 aeProcessEvents ae.c:408 (redis-server:arm64+0x10000fd64)
        #2 aeMain ae.c:496 (redis-server:arm64+0x100010f0c)
        #3 main server.c:7220 (redis-server:arm64+0x10003f38c)
    ```
    
    2. aeApiPoll() is not thread-safe
    When using RM_Yield to handle events in a module thread, if the main
    thread has not yet
    entered `afterSleep()`, both the module thread and the main thread may
    touch `server.el` at the same time.
    
        - Introduced: 
           Version: 7.0.0
           PR: #9963
    
       - Old / Abandoned Solution:
    Adding a new mutex to protect timing between after beforeSleep() and
    before afterSleep().
    Defect: If the main thread enters the ae loop without any IO events, it
    will wait until
    the next timeout or until there is any event again, and the module
    thread will
    always hang until the main thread leaves the event loop.
    
        - Related Warnings:
    ```
    SUMMARY: ThreadSanitizer: data race ae_kqueue.c:55 in addEventMask
    ==================
    ==================
    WARNING: ThreadSanitizer: data race (pid=14682)
      Write of size 4 at 0x000100b54000 by thread T9 (mutexes: write M0):
        #0 aeApiPoll ae_kqueue.c:175 (redis-server:arm64+0x100010588)
        #1 aeProcessEvents ae.c:399 (redis-server:arm64+0x10000fb84)
        #2 processEventsWhileBlocked networking.c:4138 (redis-server:arm64+0x10006d3c4)
        #3 RM_Yield module.c:2410 (redis-server:arm64+0x10018b66c)
        #4 bg_call_worker <null>:16042052 (blockedclient.so:arm64+0x169c)
    
      Previous write of size 4 at 0x000100b54000 by main thread:
        #0 aeApiPoll ae_kqueue.c:175 (redis-server:arm64+0x100010588)
        #1 aeProcessEvents ae.c:399 (redis-server:arm64+0x10000fb84)
        #2 aeMain ae.c:496 (redis-server:arm64+0x100010da8)
        #3 main server.c:7238 (redis-server:arm64+0x10003f51c)
    ```
    
    ## The final fix as the comments:
    https://github.com/redis/redis/pull/12817#discussion_r1436427232
    Optimized solution based on the above comment:
    
    First, we add `module_gil_acquring` to indicate whether the main thread
    is currently in the acquiring GIL state.
    
    When the module thread starts to yield, there are two possibilities(we
    assume the caller keeps the GIL):
    1. The main thread is in the mid of beforeSleep() and afterSleep(), that
    is, `module_gil_acquring` is not 1 now.
    At this point, the module thread will wake up the main thread through
    the pipe and leave the yield,
    waiting for the next yield when the main thread may already in the
    acquiring GIL state.
        
    2. The main thread is in the acquiring GIL state.
    The module thread release the GIL, yielding CPU to give the main thread
    an opportunity to start
    event processing, and then acquire the GIL again until the main thread
    releases it.
    This is what
    https://github.com/redis/redis/pull/12817#discussion_r1436427232
    
    
    mentioned direction.
    
    ---------
    Co-authored-by: default avatarOran Agra <oran@redislabs.com>
    ca1f67af
server.c 289 KB