[Linux] Run only one GLib main event loop #23320
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andy31415
reviewed
Oct 24, 2022
OK, thanks for info. I'll check that. EDIT: It seems that the |
arkq
force-pushed
the
linux-glib-main-loop
branch
from
b1617e4 to
1f9c43c
Compare
arkq
force-pushed
the
linux-glib-main-loop
branch
3 times, most recently
from
3ac0239 to
624e26c
Compare
|
PR #23320: Size comparison from 51a5082 to 624e26c Increases above 0.2%:
Increases (8 builds for bl602, esp32, linux, psoc6, telink)
Decreases (18 builds for esp32, linux, nrfconnect, psoc6)
Full report (49 builds for bl602, bl702, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
The D-Bus connection handler stored in the PlatformManager is not used anywhere. It's not required to pre-connect to D-Bus, because the connection (for given bus type) returned by the g_bus_get_sync() function is shared with other callers.
It is not necessary to start new GLib main event loop for every new D-Bus communication with external service. Single main event loop can handle all such communication.
TSAN thinks that memory accessed before the call to g_source_attach() (which is internally used by e.g. g_main_context_invoke() and g_idle_add()) needs to be guarded by a mutex, otherwise that's a race condition between the thread that is creating shared data (the current thread) and the glib main event loop thread. In fact such race condition does not occur because g_source_attach() acts here as pthread_create() - code is not executed on the other thread before the g_source_attach() is called.
arkq
force-pushed
the
linux-glib-main-loop
branch
from
624e26c to
a1de3ca
Compare
|
PR #23320: Size comparison from 51a5082 to a1de3ca Increases above 0.2%:
Increases (10 builds for bl602, bl702, esp32, linux, telink)
Decreases (19 builds for cc13x2_26x2, esp32, k32w, linux, psoc6)
Full report (49 builds for bl602, bl702, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
pullapprove
bot
requested review from
anush-apple,
Byungjoo-Lee,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chshu,
chulspro and
Damian-Nordic
andy31415
approved these changes
Oct 27, 2022
|
PR #23320: Size comparison from f1ebea8 to 564dc0a Increases above 0.2%:
Increases (9 builds for esp32, linux, psoc6, qpg, telink)
Decreases (17 builds for bl702, cyw30739, k32w, linux)
Full report (49 builds for bl602, bl702, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
@arkq , I suspect this fixes #9214. Can you confirm? |
|
@msandstedt It will definitely "fix" that issue, because the Right now there will be only one thread started and it's joined in the |
OK, that is great. But just to check, can we still do this? |
|
Yes, such pattern is possible. Twice or trice in a row I should say :) |
adbridge
pushed a commit
to ARM-software/connectedhomeip
that referenced
this pull request
Nov 18, 2022
* Unregister from BlueZ when shutting down BLE manager * Return CHIP_ERROR from all BlueZ helper functions * Remove not used class method * Remove D-Bus connection handler from PlatformManager The D-Bus connection handler stored in the PlatformManager is not used anywhere. It's not required to pre-connect to D-Bus, because the connection (for given bus type) returned by the g_bus_get_sync() function is shared with other callers. * Run only one GLib main event loop per Matter SDK It is not necessary to start new GLib main event loop for every new D-Bus communication with external service. Single main event loop can handle all such communication. * Run WiFi IP address change listener in GLib main loop * Properly release cancellable object on cleanup * Explicitly wait for glib main thread to exit * Workaround for TSAN false positive reports with glib TSAN thinks that memory accessed before the call to g_source_attach() (which is internally used by e.g. g_main_context_invoke() and g_idle_add()) needs to be guarded by a mutex, otherwise that's a race condition between the thread that is creating shared data (the current thread) and the glib main event loop thread. In fact such race condition does not occur because g_source_attach() acts here as pthread_create() - code is not executed on the other thread before the g_source_attach() is called. Co-authored-by: Andrei Litvin <andy314@gmail.com>
adbridge
pushed a commit
to ARM-software/connectedhomeip
that referenced
this pull request
Nov 18, 2022
* Unregister from BlueZ when shutting down BLE manager * Return CHIP_ERROR from all BlueZ helper functions * Remove not used class method * Remove D-Bus connection handler from PlatformManager The D-Bus connection handler stored in the PlatformManager is not used anywhere. It's not required to pre-connect to D-Bus, because the connection (for given bus type) returned by the g_bus_get_sync() function is shared with other callers. * Run only one GLib main event loop per Matter SDK It is not necessary to start new GLib main event loop for every new D-Bus communication with external service. Single main event loop can handle all such communication. * Run WiFi IP address change listener in GLib main loop * Properly release cancellable object on cleanup * Explicitly wait for glib main thread to exit * Workaround for TSAN false positive reports with glib TSAN thinks that memory accessed before the call to g_source_attach() (which is internally used by e.g. g_main_context_invoke() and g_idle_add()) needs to be guarded by a mutex, otherwise that's a race condition between the thread that is creating shared data (the current thread) and the glib main event loop thread. In fact such race condition does not occur because g_source_attach() acts here as pthread_create() - code is not executed on the other thread before the g_source_attach() is called. Co-authored-by: Andrei Litvin <andy314@gmail.com>
|
If using glib main loop, can't initChipStack in GUI application, such as Qt/Gtk |
|
@yzm157 what do you mean "can't initChipStack in GUI application"? There is a deadlock, segfault, etc. Do you got any logs? |
I created an application using pyqt5, |
|
Can you attach to deadlocked process with gdb and dump backtraces of all threads |
|
@arkq The following Python code can reproduce this problem: |
|
Looks like it's caused by mutex lock, could removing the lock cause other problems? |
|
Yes, however, this block of code is more or less required. Right now I've got some other important work to do, but I'll try to investigate this issue tomorrow. Sorry for inconvenience. One thing you can check is whether the |
No, not executed |
|
OK, I think the problem is with |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
When Linux platform uses WiFi (wpa_supplicant) and BLE (BlueZ) it runs one GLib main event loop per remote D-Bus service (one for wpa_supplicant and one for BlueZ). It is possible to handle all D-Bus related processing in one event loop - it's not required to start new thread for every D-Bus connection (D-Bus socket messages are handled in only one context any way - D-Bus connection is shared). Also, new WiFi address changer thread is started every time the
PlatformManagerImpl::_InitChipStack()is called, but previous one is not terminated when callingPlatformManagerImpl::_Shutdown()- thread leakage can be seen when running./tests/TestPlatformMgrtest.Changes
PlatformManagerImpl::_Shutdown()Testing
Tested locally with chip-tool and lighting-app.
CI should also check correctness of this PR.