You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "bneradt (via GitHub)" <gi...@apache.org> on 2023/08/02 21:05:12 UTC
[GitHub] [trafficserver] bneradt opened a new pull request, #10141: Fix dl_load_lock and DbgCtl deadlock
bneradt opened a new pull request, #10141:
URL: https://github.com/apache/trafficserver/pull/10141
This patch addresses a deadlock between the flushing thread and loading a plugin with DbgCtl configured. The lock would happen like so:
* The flushing thread would grab a lock for the DbgCtl::_RegistryAccessor::_mtx mutex through one of its Debug log messages.
* Meanwhile, the traffic_server main thread will load plugins via dlopen. dlopen implicitly grabs the dl_load_lock.
* The flushing thread would then process the debug tag via PCRE which, on first invocation for the thread, would try to grab the dl_load_lock. This however, is held by the thread loading the plugin.
* The plugin would then try to initialize its DbgCtl and try to grab a lock for the DbgCtl::_RegistryAccessor::_mtx. This however is held by the flushing thread.
This deadlock is addressed by ensuring that the flushing thread, as well as other threads, initialize their PCRE jit storage before plugins are loaded.
Fixes #10129
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a diff in pull request #10141: Fix dl_load_lock and DbgCtl deadlock
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #10141:
URL: https://github.com/apache/trafficserver/pull/10141#discussion_r1282452960
##########
src/tscore/DbgCtl.cc:
##########
@@ -129,6 +129,7 @@ DbgCtl::_new_reference(char const *tag)
TSDbgCtl ctl;
ctl.tag = tag;
+ ctl.on = diags() && diags()->tag_activated(tag, DiagsTagType_Debug);
Review Comment:
Actually, things still do deadlock if I revert this part of the patch. A later function in the periodic logger tries to Debug log, and sets `ctl.on` via this call while the DbgCtl lock is held and deadlocks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a diff in pull request #10141: Fix dl_load_lock and DbgCtl deadlock
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #10141:
URL: https://github.com/apache/trafficserver/pull/10141#discussion_r1282434172
##########
iocore/eventsystem/UnixEventProcessor.cc:
##########
@@ -477,6 +477,12 @@ EventProcessor::spawn_event_threads(EventType ev_type, int n_threads, size_t sta
void
EventProcessor::initThreadState(EThread *t)
{
+ // #10129: Initialize DbgCtl early on so that we process the PCRE regex before
+ // we load plugins. Otherwise, PCRE compilation on first use will try to grab
+ // the dl_load_lock, which might race and deadlock with our dlopen calls for
+ // plugins.
+ DbgCtl::update();
Review Comment:
The idea is that it will initialize the thread_local PCRE structure which acquires the dl_load_lock. If we do that early on, before plugins are loaded, then we avoid the race condition for the threads that initialize themselves through this function.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #10141: Fix dl_load_lock and DbgCtl deadlock
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #10141:
URL: https://github.com/apache/trafficserver/pull/10141#issuecomment-1664169267
@ywkaras has a few other ideas that resolves this. I'm closing this PR in favor of one of those.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt closed pull request #10141: Fix dl_load_lock and DbgCtl deadlock
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt closed pull request #10141: Fix dl_load_lock and DbgCtl deadlock
URL: https://github.com/apache/trafficserver/pull/10141
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #10141: Fix dl_load_lock and DbgCtl deadlock
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #10141:
URL: https://github.com/apache/trafficserver/pull/10141#issuecomment-1662990488
[approve ci autest]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a diff in pull request #10141: Fix dl_load_lock and DbgCtl deadlock
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #10141:
URL: https://github.com/apache/trafficserver/pull/10141#discussion_r1282437567
##########
proxy/logging/Log.cc:
##########
@@ -1150,6 +1156,11 @@ Log::create_threads()
ink_atomiclist_init(flush_data_list, "Logging flush buffer list", 0);
Continuation *flush_cont = new LoggingFlushContinuation(0);
eventProcessor.spawn_thread(flush_cont, "[LOG_FLUSH]", stacksize);
+
+ {
+ std::unique_lock<std::mutex> lk(flush_condition_mutex);
+ flush_condition_variable.wait(lk, [] { return flush_thread_is_initialized; });
+ }
Review Comment:
Right. It isn't the only thread. But the flushing thread doesn't go through the UnixEventProcessor.cc DbgCtl::update() logic that's also in this patch. Thus we ensure that we log here early before the dlopen calls for plugins.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a diff in pull request #10141: Fix dl_load_lock and DbgCtl deadlock
Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on code in PR #10141:
URL: https://github.com/apache/trafficserver/pull/10141#discussion_r1282430693
##########
proxy/logging/Log.cc:
##########
@@ -1150,6 +1156,11 @@ Log::create_threads()
ink_atomiclist_init(flush_data_list, "Logging flush buffer list", 0);
Continuation *flush_cont = new LoggingFlushContinuation(0);
eventProcessor.spawn_thread(flush_cont, "[LOG_FLUSH]", stacksize);
+
+ {
+ std::unique_lock<std::mutex> lk(flush_condition_mutex);
+ flush_condition_variable.wait(lk, [] { return flush_thread_is_initialized; });
+ }
Review Comment:
I don't think we know if this is the only thread that can cause this problem. May be for now, but this could bite us, if in the future we add more threads that start before plugin initialization.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] cmcfarlen commented on pull request #10141: Fix dl_load_lock and DbgCtl deadlock
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on PR #10141:
URL: https://github.com/apache/trafficserver/pull/10141#issuecomment-1662993159
> I'm halfway done with a different PR to fix (work around) this problem.
Does your PR involve changing the Debug calls in log.cc to be Dbg calls with static DbgCtl objects? I think this is the better route if so. It avoids the possibility of regex processing at the call sites.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a diff in pull request #10141: Fix dl_load_lock and DbgCtl deadlock
Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on code in PR #10141:
URL: https://github.com/apache/trafficserver/pull/10141#discussion_r1282428502
##########
iocore/eventsystem/UnixEventProcessor.cc:
##########
@@ -477,6 +477,12 @@ EventProcessor::spawn_event_threads(EventType ev_type, int n_threads, size_t sta
void
EventProcessor::initThreadState(EThread *t)
{
+ // #10129: Initialize DbgCtl early on so that we process the PCRE regex before
+ // we load plugins. Otherwise, PCRE compilation on first use will try to grab
+ // the dl_load_lock, which might race and deadlock with our dlopen calls for
+ // plugins.
+ DbgCtl::update();
Review Comment:
I don't see how this would do any good. This function doesn't result in a call to Diags::tag_actived(), while the Registry mutex is unlocked.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a diff in pull request #10141: Fix dl_load_lock and DbgCtl deadlock
Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on code in PR #10141:
URL: https://github.com/apache/trafficserver/pull/10141#discussion_r1282423166
##########
src/tscore/DbgCtl.cc:
##########
@@ -129,6 +129,7 @@ DbgCtl::_new_reference(char const *tag)
TSDbgCtl ctl;
ctl.tag = tag;
+ ctl.on = diags() && diags()->tag_activated(tag, DiagsTagType_Debug);
Review Comment:
If you set "on" without the Registry mutex locked, there's a race condition, at least hypothetically. It may not get updated for a change to the regex tags are matched to, to see if they are on.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a diff in pull request #10141: Fix dl_load_lock and DbgCtl deadlock
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #10141:
URL: https://github.com/apache/trafficserver/pull/10141#discussion_r1282439685
##########
src/tscore/DbgCtl.cc:
##########
@@ -129,6 +129,7 @@ DbgCtl::_new_reference(char const *tag)
TSDbgCtl ctl;
ctl.tag = tag;
+ ctl.on = diags() && diags()->tag_activated(tag, DiagsTagType_Debug);
Review Comment:
I can revert this patch. The idea was to perform this outside of the lock to minimize where we can deadlock. But it shouldn't been needed to avoid the deadlock.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on pull request #10141: Fix dl_load_lock and DbgCtl deadlock
Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on PR #10141:
URL: https://github.com/apache/trafficserver/pull/10141#issuecomment-1662986036
I'm halfway done with a different PR to fix (work around) this problem.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org