ntdll-CriticalSection: Remove patch set.

This patch set is currently broken, as the RtlEnterCriticalSection()
implementation has changed without corresponding changes to the inline version.

More broadly, it is very surprising, and never really substantiated, that
inlining helps significantly. There is no record of why the patches were
written, but my guess is that they were written in an attempt to optimize heap
allocation, and my further guess is that targeting critical sections in
particular was motivated by perf traces.

Besides the fact that perf traces are unreliable on the best days, and that
anything that spins or uses atomics like our CS implementation is going to be
overrepresented especially relative to the practical impact, the heap
implementation was optimized for cases that matter with the introduction of the
LFH, and crucially, the LFH is lock free.

As for threadpools, I suspect that Sebastian took note of them as the only other
user of locking in ntdll, and that the inline version was used there because
there was no real reason not to.

At the end of the day, these patches are incorrect, probably help nothing, and
even if we did find they helped something we'd want to do a lot more
investigation and probably solve the problem a different way. Remove them.
This commit is contained in:
Zebediah Figura 2024-04-28 18:46:00 -05:00
parent ff5ea043b5
commit 6919d12eba
3 changed files with 0 additions and 535 deletions

View File

@ -1,65 +0,0 @@
From 49fa193ec777f68372b549752482ef82fb63a7b7 Mon Sep 17 00:00:00 2001
From: Sebastian Lackner <sebastian@fds-team.de>
Date: Sat, 5 Aug 2017 03:38:38 +0200
Subject: [PATCH] ntdll: Add inline versions of RtlEnterCriticalSection /
RtlLeaveCriticalSections.
---
dlls/ntdll/ntdll_misc.h | 34 ++++++++++++++++++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/dlls/ntdll/ntdll_misc.h b/dlls/ntdll/ntdll_misc.h
index 171ded98c67..54a65e3bae5 100644
--- a/dlls/ntdll/ntdll_misc.h
+++ b/dlls/ntdll/ntdll_misc.h
@@ -29,6 +29,7 @@
#include "winternl.h"
#include "rtlsupportapi.h"
#include "unixlib.h"
+#include "wine/debug.h"
#include "wine/asm.h"
#define MAX_NT_PATH_LENGTH 277
@@ -106,6 +107,39 @@ extern void (FASTCALL *pBaseThreadInitThunk)(DWORD,LPTHREAD_START_ROUTINE,void *
extern struct _KUSER_SHARED_DATA *user_shared_data;
+/* inline version of RtlEnterCriticalSection */
+static inline void enter_critical_section( RTL_CRITICAL_SECTION *crit )
+{
+ if (InterlockedIncrement( &crit->LockCount ))
+ {
+ if (crit->OwningThread == ULongToHandle(GetCurrentThreadId()))
+ {
+ crit->RecursionCount++;
+ return;
+ }
+ RtlpWaitForCriticalSection( crit );
+ }
+ crit->OwningThread = ULongToHandle(GetCurrentThreadId());
+ crit->RecursionCount = 1;
+}
+
+/* inline version of RtlLeaveCriticalSection */
+static inline void leave_critical_section( RTL_CRITICAL_SECTION *crit )
+{
+ WINE_DECLARE_DEBUG_CHANNEL(ntdll);
+ if (--crit->RecursionCount)
+ {
+ if (crit->RecursionCount > 0) InterlockedDecrement( &crit->LockCount );
+ else ERR_(ntdll)( "section %p is not acquired\n", crit );
+ }
+ else
+ {
+ crit->OwningThread = 0;
+ if (InterlockedDecrement( &crit->LockCount ) >= 0)
+ RtlpUnWaitCriticalSection( crit );
+ }
+}
+
#ifdef _WIN64
static inline TEB64 *NtCurrentTeb64(void) { return NULL; }
#else
--
2.43.0

View File

@ -1,74 +0,0 @@
From 85e5938c50ad5a1eebc7ec21d6c01ea7f4faf49d Mon Sep 17 00:00:00 2001
From: Sebastian Lackner <sebastian@fds-team.de>
Date: Sat, 5 Aug 2017 03:39:23 +0200
Subject: [PATCH] ntdll: Use fast CS functions for heap locking.
---
dlls/ntdll/heap.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/dlls/ntdll/heap.c b/dlls/ntdll/heap.c
index cb1ff12c679..757e0246925 100644
--- a/dlls/ntdll/heap.c
+++ b/dlls/ntdll/heap.c
@@ -476,13 +476,13 @@ static inline ULONG heap_get_flags( const struct heap *heap, ULONG flags )
static inline void heap_lock( struct heap *heap, ULONG flags )
{
if (flags & HEAP_NO_SERIALIZE) return;
- RtlEnterCriticalSection( &heap->cs );
+ enter_critical_section( &heap->cs );
}
static inline void heap_unlock( struct heap *heap, ULONG flags )
{
if (flags & HEAP_NO_SERIALIZE) return;
- RtlLeaveCriticalSection( &heap->cs );
+ leave_critical_section( &heap->cs );
}
static void heap_set_status( const struct heap *heap, ULONG flags, NTSTATUS status )
@@ -1392,9 +1392,9 @@ HANDLE WINAPI RtlCreateHeap( ULONG flags, PVOID addr, SIZE_T totalSize, SIZE_T c
/* link it into the per-process heap list */
if (process_heap)
{
- RtlEnterCriticalSection( &process_heap->cs );
+ enter_critical_section( &process_heap->cs );
list_add_head( &process_heap->entry, &heap->entry );
- RtlLeaveCriticalSection( &process_heap->cs );
+ leave_critical_section( &process_heap->cs );
}
else if (!addr)
{
@@ -1451,9 +1451,9 @@ HANDLE WINAPI RtlDestroyHeap( HANDLE handle )
if (heap == process_heap) return handle; /* cannot delete the main process heap */
/* remove it from the per-process list */
- RtlEnterCriticalSection( &process_heap->cs );
+ enter_critical_section( &process_heap->cs );
list_remove( &heap->entry );
- RtlLeaveCriticalSection( &process_heap->cs );
+ leave_critical_section( &process_heap->cs );
heap->cs.DebugInfo->Spare[0] = 0;
RtlDeleteCriticalSection( &heap->cs );
@@ -1968,7 +1968,7 @@ ULONG WINAPI RtlGetProcessHeaps( ULONG count, HANDLE *heaps )
ULONG total = 1; /* main heap */
struct list *ptr;
- RtlEnterCriticalSection( &process_heap->cs );
+ enter_critical_section( &process_heap->cs );
LIST_FOR_EACH( ptr, &process_heap->entry ) total++;
if (total <= count)
{
@@ -1976,7 +1976,7 @@ ULONG WINAPI RtlGetProcessHeaps( ULONG count, HANDLE *heaps )
LIST_FOR_EACH( ptr, &process_heap->entry )
*heaps++ = LIST_ENTRY( ptr, struct heap, entry );
}
- RtlLeaveCriticalSection( &process_heap->cs );
+ leave_critical_section( &process_heap->cs );
return total;
}
--
2.38.1

View File

@ -1,396 +0,0 @@
From 6d5610f8950d0319dbec724ef4d1f0ed140a97ec Mon Sep 17 00:00:00 2001
From: Sebastian Lackner <sebastian@fds-team.de>
Date: Sat, 5 Aug 2017 03:39:37 +0200
Subject: [PATCH] ntdll: Use fast CS functions for threadpool locking.
---
dlls/ntdll/threadpool.c | 90 ++++++++++++++++++++---------------------
1 file changed, 45 insertions(+), 45 deletions(-)
diff --git a/dlls/ntdll/threadpool.c b/dlls/ntdll/threadpool.c
index 99525f831e1..36d1351aedc 100644
--- a/dlls/ntdll/threadpool.c
+++ b/dlls/ntdll/threadpool.c
@@ -1063,7 +1063,7 @@ static void CALLBACK timerqueue_thread_proc( void *param )
TRACE( "starting timer queue thread\n" );
set_thread_name(L"wine_threadpool_timerqueue");
- RtlEnterCriticalSection( &timerqueue.cs );
+ enter_critical_section( &timerqueue.cs );
for (;;)
{
NtQuerySystemTime( &now );
@@ -1136,7 +1136,7 @@ static void CALLBACK timerqueue_thread_proc( void *param )
}
timerqueue.thread_running = FALSE;
- RtlLeaveCriticalSection( &timerqueue.cs );
+ leave_critical_section( &timerqueue.cs );
TRACE( "terminating timer queue thread\n" );
RtlExitUserThread( 0 );
@@ -1181,7 +1181,7 @@ static NTSTATUS tp_timerqueue_lock( struct threadpool_object *timer )
timer->u.timer.period = 0;
timer->u.timer.window_length = 0;
- RtlEnterCriticalSection( &timerqueue.cs );
+ enter_critical_section( &timerqueue.cs );
/* Make sure that the timerqueue thread is running. */
if (!timerqueue.thread_running)
@@ -1202,7 +1202,7 @@ static NTSTATUS tp_timerqueue_lock( struct threadpool_object *timer )
timerqueue.objcount++;
}
- RtlLeaveCriticalSection( &timerqueue.cs );
+ leave_critical_section( &timerqueue.cs );
return status;
}
@@ -1215,7 +1215,7 @@ static void tp_timerqueue_unlock( struct threadpool_object *timer )
{
assert( timer->type == TP_OBJECT_TYPE_TIMER );
- RtlEnterCriticalSection( &timerqueue.cs );
+ enter_critical_section( &timerqueue.cs );
if (timer->u.timer.timer_initialized)
{
/* If timer was pending, remove it. */
@@ -1234,7 +1234,7 @@ static void tp_timerqueue_unlock( struct threadpool_object *timer )
timer->u.timer.timer_initialized = FALSE;
}
- RtlLeaveCriticalSection( &timerqueue.cs );
+ leave_critical_section( &timerqueue.cs );
}
/***********************************************************************
@@ -1253,7 +1253,7 @@ static void CALLBACK waitqueue_thread_proc( void *param )
TRACE( "starting wait queue thread\n" );
set_thread_name(L"wine_threadpool_waitqueue");
- RtlEnterCriticalSection( &waitqueue.cs );
+ enter_critical_section( &waitqueue.cs );
for (;;)
{
@@ -1302,10 +1302,10 @@ static void CALLBACK waitqueue_thread_proc( void *param )
/* All wait objects have been destroyed, if no new wait objects are created
* within some amount of time, then we can shutdown this thread. */
assert( num_handles == 0 );
- RtlLeaveCriticalSection( &waitqueue.cs );
+ leave_critical_section( &waitqueue.cs );
timeout.QuadPart = (ULONGLONG)THREADPOOL_WORKER_TIMEOUT * -10000;
status = NtWaitForMultipleObjects( 1, &bucket->update_event, TRUE, bucket->alertable, &timeout );
- RtlEnterCriticalSection( &waitqueue.cs );
+ enter_critical_section( &waitqueue.cs );
if (status == STATUS_TIMEOUT && !bucket->objcount)
break;
@@ -1315,7 +1315,7 @@ static void CALLBACK waitqueue_thread_proc( void *param )
handles[num_handles] = bucket->update_event;
RtlLeaveCriticalSection( &waitqueue.cs );
status = NtWaitForMultipleObjects( num_handles + 1, handles, TRUE, bucket->alertable, &timeout );
- RtlEnterCriticalSection( &waitqueue.cs );
+ enter_critical_section( &waitqueue.cs );
if (status >= STATUS_WAIT_0 && status < STATUS_WAIT_0 + num_handles)
{
@@ -1399,7 +1399,7 @@ static void CALLBACK waitqueue_thread_proc( void *param )
if (!--waitqueue.num_buckets)
assert( list_empty( &waitqueue.buckets ) );
- RtlLeaveCriticalSection( &waitqueue.cs );
+ leave_critical_section( &waitqueue.cs );
TRACE( "terminating wait queue thread\n" );
@@ -1429,7 +1429,7 @@ static NTSTATUS tp_waitqueue_lock( struct threadpool_object *wait )
wait->u.wait.timeout = 0;
wait->u.wait.handle = INVALID_HANDLE_VALUE;
- RtlEnterCriticalSection( &waitqueue.cs );
+ enter_critical_section( &waitqueue.cs );
/* Try to assign to existing bucket if possible. */
LIST_FOR_EACH_ENTRY( bucket, &waitqueue.buckets, struct waitqueue_bucket, bucket_entry )
@@ -1486,7 +1486,7 @@ static NTSTATUS tp_waitqueue_lock( struct threadpool_object *wait )
}
out:
- RtlLeaveCriticalSection( &waitqueue.cs );
+ leave_critical_section( &waitqueue.cs );
return status;
}
@@ -1497,7 +1497,7 @@ static void tp_waitqueue_unlock( struct threadpool_object *wait )
{
assert( wait->type == TP_OBJECT_TYPE_WAIT );
- RtlEnterCriticalSection( &waitqueue.cs );
+ enter_critical_section( &waitqueue.cs );
if (wait->u.wait.bucket)
{
struct waitqueue_bucket *bucket = wait->u.wait.bucket;
@@ -1509,7 +1509,7 @@ static void tp_waitqueue_unlock( struct threadpool_object *wait )
NtSetEvent( bucket->update_event, NULL );
}
- RtlLeaveCriticalSection( &waitqueue.cs );
+ leave_critical_section( &waitqueue.cs );
}
static void CALLBACK ioqueue_thread_proc( void *param )
@@ -1787,7 +1787,7 @@ static NTSTATUS tp_threadpool_lock( struct threadpool **out, TP_CALLBACK_ENVIRON
pool = default_threadpool;
}
- RtlEnterCriticalSection( &pool->cs );
+ enter_critical_section( &pool->cs );
/* Make sure that the threadpool has at least one thread. */
if (!pool->num_workers)
@@ -1801,7 +1801,7 @@ static NTSTATUS tp_threadpool_lock( struct threadpool **out, TP_CALLBACK_ENVIRON
pool->objcount++;
}
- RtlLeaveCriticalSection( &pool->cs );
+ leave_critical_section( &pool->cs );
if (status != STATUS_SUCCESS)
return status;
@@ -1817,9 +1817,9 @@ static NTSTATUS tp_threadpool_lock( struct threadpool **out, TP_CALLBACK_ENVIRON
*/
static void tp_threadpool_unlock( struct threadpool *pool )
{
- RtlEnterCriticalSection( &pool->cs );
+ enter_critical_section( &pool->cs );
pool->objcount--;
- RtlLeaveCriticalSection( &pool->cs );
+ leave_critical_section( &pool->cs );
tp_threadpool_release( pool );
}
@@ -1957,10 +1957,10 @@ static void tp_object_initialize( struct threadpool_object *object, struct threa
struct threadpool_group *group = object->group;
InterlockedIncrement( &group->refcount );
- RtlEnterCriticalSection( &group->cs );
+ enter_critical_section( &group->cs );
list_add_tail( &group->members, &object->group_entry );
object->is_group_member = TRUE;
- RtlLeaveCriticalSection( &group->cs );
+ leave_critical_section( &group->cs );
}
if (is_simple_callback)
@@ -1987,7 +1987,7 @@ static void tp_object_submit( struct threadpool_object *object, BOOL signaled )
assert( !object->shutdown );
assert( !pool->shutdown );
- RtlEnterCriticalSection( &pool->cs );
+ enter_critical_section( &pool->cs );
/* Start new worker threads if required. */
if (pool->num_busy_workers >= pool->num_workers &&
@@ -2010,7 +2010,7 @@ static void tp_object_submit( struct threadpool_object *object, BOOL signaled )
RtlWakeConditionVariable( &pool->update_event );
}
- RtlLeaveCriticalSection( &pool->cs );
+ leave_critical_section( &pool->cs );
}
/***********************************************************************
@@ -2023,7 +2023,7 @@ static void tp_object_cancel( struct threadpool_object *object )
struct threadpool *pool = object->pool;
LONG pending_callbacks = 0;
- RtlEnterCriticalSection( &pool->cs );
+ enter_critical_section( &pool->cs );
if (object->num_pending_callbacks)
{
pending_callbacks = object->num_pending_callbacks;
@@ -2038,7 +2038,7 @@ static void tp_object_cancel( struct threadpool_object *object )
object->u.io.skipped_count += object->u.io.pending_count;
object->u.io.pending_count = 0;
}
- RtlLeaveCriticalSection( &pool->cs );
+ leave_critical_section( &pool->cs );
while (pending_callbacks--)
tp_object_release( object );
@@ -2067,7 +2067,7 @@ static void tp_object_wait( struct threadpool_object *object, BOOL group_wait )
{
struct threadpool *pool = object->pool;
- RtlEnterCriticalSection( &pool->cs );
+ enter_critical_section( &pool->cs );
while (!object_is_finished( object, group_wait ))
{
if (group_wait)
@@ -2075,7 +2075,7 @@ static void tp_object_wait( struct threadpool_object *object, BOOL group_wait )
else
RtlSleepConditionVariableCS( &object->finished_event, &pool->cs, NULL );
}
- RtlLeaveCriticalSection( &pool->cs );
+ leave_critical_section( &pool->cs );
}
static void tp_ioqueue_unlock( struct threadpool_object *io )
@@ -2129,13 +2129,13 @@ static BOOL tp_object_release( struct threadpool_object *object )
{
struct threadpool_group *group = object->group;
- RtlEnterCriticalSection( &group->cs );
+ enter_critical_section( &group->cs );
if (object->is_group_member)
{
list_remove( &object->group_entry );
object->is_group_member = FALSE;
}
- RtlLeaveCriticalSection( &group->cs );
+ leave_critical_section( &group->cs );
tp_group_release( group );
}
@@ -2337,7 +2337,7 @@ static void CALLBACK threadpool_worker_proc( void *param )
TRACE( "starting worker thread for pool %p\n", pool );
set_thread_name(L"wine_threadpool_worker");
- RtlEnterCriticalSection( &pool->cs );
+ enter_critical_section( &pool->cs );
for (;;)
{
while ((ptr = threadpool_get_next_item( pool )))
@@ -2377,7 +2377,7 @@ static void CALLBACK threadpool_worker_proc( void *param )
}
}
pool->num_workers--;
- RtlLeaveCriticalSection( &pool->cs );
+ leave_critical_section( &pool->cs );
TRACE( "terminating worker thread for pool %p\n", pool );
tp_threadpool_release( pool );
@@ -2625,7 +2625,7 @@ NTSTATUS WINAPI TpCallbackMayRunLong( TP_CALLBACK_INSTANCE *instance )
return STATUS_SUCCESS;
pool = object->pool;
- RtlEnterCriticalSection( &pool->cs );
+ enter_critical_section( &pool->cs );
/* Start new worker threads if required. */
if (pool->num_busy_workers >= pool->num_workers)
@@ -2640,7 +2640,7 @@ NTSTATUS WINAPI TpCallbackMayRunLong( TP_CALLBACK_INSTANCE *instance )
}
}
- RtlLeaveCriticalSection( &pool->cs );
+ leave_critical_section( &pool->cs );
this->may_run_long = TRUE;
return status;
}
@@ -2721,13 +2721,13 @@ VOID WINAPI TpDisassociateCallback( TP_CALLBACK_INSTANCE *instance )
return;
pool = object->pool;
- RtlEnterCriticalSection( &pool->cs );
+ enter_critical_section( &pool->cs );
object->num_associated_callbacks--;
if (object_is_finished( object, FALSE ))
RtlWakeAllConditionVariable( &object->finished_event );
- RtlLeaveCriticalSection( &pool->cs );
+ leave_critical_section( &pool->cs );
this->associated = FALSE;
}
@@ -2779,7 +2779,7 @@ VOID WINAPI TpReleaseCleanupGroupMembers( TP_CLEANUP_GROUP *group, BOOL cancel_p
TRACE( "%p %u %p\n", group, cancel_pending, userdata );
- RtlEnterCriticalSection( &this->cs );
+ enter_critical_section( &this->cs );
/* Unset group, increase references, and mark objects for shutdown */
LIST_FOR_EACH_ENTRY_SAFE( object, next, &this->members, struct threadpool_object, group_entry )
@@ -2805,7 +2805,7 @@ VOID WINAPI TpReleaseCleanupGroupMembers( TP_CLEANUP_GROUP *group, BOOL cancel_p
list_init( &members );
list_move_tail( &members, &this->members );
- RtlLeaveCriticalSection( &this->cs );
+ leave_critical_section( &this->cs );
/* Cancel pending callbacks if requested */
if (cancel_pending)
@@ -2928,10 +2928,10 @@ VOID WINAPI TpSetPoolMaxThreads( TP_POOL *pool, DWORD maximum )
TRACE( "%p %lu\n", pool, maximum );
- RtlEnterCriticalSection( &this->cs );
+ enter_critical_section( &this->cs );
this->max_workers = max( maximum, 1 );
this->min_workers = min( this->min_workers, this->max_workers );
- RtlLeaveCriticalSection( &this->cs );
+ leave_critical_section( &this->cs );
}
/***********************************************************************
@@ -2944,7 +2944,7 @@ BOOL WINAPI TpSetPoolMinThreads( TP_POOL *pool, DWORD minimum )
TRACE( "%p %lu\n", pool, minimum );
- RtlEnterCriticalSection( &this->cs );
+ enter_critical_section( &this->cs );
while (this->num_workers < minimum)
{
@@ -2959,7 +2959,7 @@ BOOL WINAPI TpSetPoolMinThreads( TP_POOL *pool, DWORD minimum )
this->max_workers = max( this->min_workers, this->max_workers );
}
- RtlLeaveCriticalSection( &this->cs );
+ leave_critical_section( &this->cs );
return !status;
}
@@ -2975,7 +2975,7 @@ VOID WINAPI TpSetTimer( TP_TIMER *timer, LARGE_INTEGER *timeout, LONG period, LO
TRACE( "%p %p %lu %lu\n", timer, timeout, period, window_length );
- RtlEnterCriticalSection( &timerqueue.cs );
+ enter_critical_section( &timerqueue.cs );
assert( this->u.timer.timer_initialized );
this->u.timer.timer_set = timeout != NULL;
@@ -3035,7 +3035,7 @@ VOID WINAPI TpSetTimer( TP_TIMER *timer, LARGE_INTEGER *timeout, LONG period, LO
this->u.timer.timer_pending = TRUE;
}
- RtlLeaveCriticalSection( &timerqueue.cs );
+ leave_critical_section( &timerqueue.cs );
if (submit_timer)
tp_object_submit( this, FALSE );
@@ -3051,7 +3051,7 @@ VOID WINAPI TpSetWait( TP_WAIT *wait, HANDLE handle, LARGE_INTEGER *timeout )
TRACE( "%p %p %p\n", wait, handle, timeout );
- RtlEnterCriticalSection( &waitqueue.cs );
+ enter_critical_section( &waitqueue.cs );
assert( this->u.wait.bucket );
this->u.wait.handle = handle;
@@ -3090,7 +3090,7 @@ VOID WINAPI TpSetWait( TP_WAIT *wait, HANDLE handle, LARGE_INTEGER *timeout )
NtSetEvent( bucket->update_event, NULL );
}
- RtlLeaveCriticalSection( &waitqueue.cs );
+ leave_critical_section( &waitqueue.cs );
}
/***********************************************************************
--
2.38.1