[PATCH v28 0/2] MR464: Implement _StructuredTaskCollection::_Schedule{,_loc}
- Based on !463 - _RunAndWait and exception handling implemented in later commits @piotr -- v28: msvcr100: Implement _StructuredTaskCollection::_Schedule and _Schedule_loc. msvcr100: Factor out the mapping of a context to a scheduler. https://gitlab.winehq.org/wine/wine/-/merge_requests/464
From: Torge Matthies <tmatthies(a)codeweavers.com> Signed-off-by: Torge Matthies <tmatthies(a)codeweavers.com> --- dlls/msvcrt/concurrency.c | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/dlls/msvcrt/concurrency.c b/dlls/msvcrt/concurrency.c index d0c9924c631..f8abdff9f5d 100644 --- a/dlls/msvcrt/concurrency.c +++ b/dlls/msvcrt/concurrency.c @@ -697,29 +697,38 @@ static Context* get_current_context(void) return ret; } +static Scheduler* get_scheduler_from_context(Context *ctx) +{ + ExternalContextBase *context = (ExternalContextBase*)ctx; + + if (context->context.vtable != &ExternalContextBase_vtable) + return NULL; + return context->scheduler.scheduler; +} + static Scheduler* try_get_current_scheduler(void) { - ExternalContextBase *context = (ExternalContextBase*)try_get_current_context(); + Context *context = try_get_current_context(); + Scheduler *ret; if (!context) return NULL; - if (context->context.vtable != &ExternalContextBase_vtable) { + ret = get_scheduler_from_context(context); + if (!ret) ERR("unknown context set\n"); - return NULL; - } - return context->scheduler.scheduler; + return ret; } static Scheduler* get_current_scheduler(void) { - ExternalContextBase *context = (ExternalContextBase*)get_current_context(); + Context *context = get_current_context(); + Scheduler *ret; - if (context->context.vtable != &ExternalContextBase_vtable) { + ret = get_scheduler_from_context(context); + if (!ret) ERR("unknown context set\n"); - return NULL; - } - return context->scheduler.scheduler; + return ret; } /* ?CurrentContext(a)Context@Concurrency@@SAPAV12(a)XZ */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/464
From: Torge Matthies <tmatthies(a)codeweavers.com> Signed-off-by: Torge Matthies <tmatthies(a)codeweavers.com> --- dlls/msvcrt/concurrency.c | 190 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 183 insertions(+), 7 deletions(-) diff --git a/dlls/msvcrt/concurrency.c b/dlls/msvcrt/concurrency.c index f8abdff9f5d..013219ab5bd 100644 --- a/dlls/msvcrt/concurrency.c +++ b/dlls/msvcrt/concurrency.c @@ -24,6 +24,8 @@ #include "windef.h" #include "winternl.h" #include "wine/debug.h" +#include "wine/exception.h" +#include "wine/list.h" #include "msvcrt.h" #include "cxx.h" @@ -135,6 +137,7 @@ typedef struct { int shutdown_size; HANDLE *shutdown_events; CRITICAL_SECTION cs; + struct list scheduled_chores; } ThreadScheduler; extern const vtable_ptr ThreadScheduler_vtable; @@ -164,15 +167,32 @@ typedef struct yield_func yield_func; } SpinWait; +#define FINISHED_INITIAL 0x80000000 typedef struct { - char dummy; -} _UnrealizedChore; + void *unk1; + unsigned int unk2; + void *unk3; + Context *context; + volatile LONG count; + volatile LONG finished; + void *exception; + void *event; +} _StructuredTaskCollection; -typedef struct +typedef struct _UnrealizedChore { - char dummy; -} _StructuredTaskCollection; + const vtable_ptr *vtable; + void (__cdecl *chore_proc)(struct _UnrealizedChore*); + _StructuredTaskCollection *task_collection; + void (__cdecl *chore_wrapper)(struct _UnrealizedChore*); + void *unk[6]; +} _UnrealizedChore; + +struct scheduled_chore { + struct list entry; + _UnrealizedChore *chore; +}; /* keep in sync with msvcp90/msvcp90.h */ typedef struct cs_queue @@ -629,6 +649,7 @@ DEFINE_RTTI_DATA1(scheduler_resource_allocation_error, 0, &cexception_rtti_base_ DEFINE_CXX_DATA1(improper_lock, &cexception_cxx_type_info, cexception_dtor) DEFINE_CXX_DATA1(improper_scheduler_attach, &cexception_cxx_type_info, cexception_dtor) DEFINE_CXX_DATA1(improper_scheduler_detach, &cexception_cxx_type_info, cexception_dtor) +DEFINE_CXX_DATA1(invalid_multiple_scheduling, &cexception_cxx_type_info, cexception_dtor) DEFINE_CXX_DATA1(invalid_scheduler_policy_key, &cexception_cxx_type_info, cexception_dtor) DEFINE_CXX_DATA1(invalid_scheduler_policy_thread_specification, &cexception_cxx_type_info, cexception_dtor) DEFINE_CXX_DATA1(invalid_scheduler_policy_value, &cexception_cxx_type_info, cexception_dtor) @@ -839,6 +860,25 @@ bool __thiscall ExternalContextBase_IsSynchronouslyBlocked(const ExternalContext return FALSE; } +static void remove_scheduled_chores(Scheduler *scheduler, const ExternalContextBase *context) +{ + ThreadScheduler *tscheduler = (ThreadScheduler*)scheduler; + struct scheduled_chore *sc, *next; + + if (tscheduler->scheduler.vtable != &ThreadScheduler_vtable) + return; + + EnterCriticalSection(&tscheduler->cs); + LIST_FOR_EACH_ENTRY_SAFE(sc, next, &tscheduler->scheduled_chores, + struct scheduled_chore, entry) { + if (sc->chore->task_collection->context == &context->context) { + list_remove(&sc->entry); + operator_delete(sc); + } + } + LeaveCriticalSection(&tscheduler->cs); +} + static void ExternalContextBase_dtor(ExternalContextBase *this) { struct scheduler_list *scheduler_cur, *scheduler_next; @@ -854,10 +894,12 @@ static void ExternalContextBase_dtor(ExternalContextBase *this) } if (this->scheduler.scheduler) { + remove_scheduled_chores(this->scheduler.scheduler, this); call_Scheduler_Release(this->scheduler.scheduler); for(scheduler_cur=this->scheduler.next; scheduler_cur; scheduler_cur=scheduler_next) { scheduler_next = scheduler_cur->next; + remove_scheduled_chores(scheduler_cur->scheduler, this); call_Scheduler_Release(scheduler_cur->scheduler); operator_delete(scheduler_cur); } @@ -1157,6 +1199,7 @@ void __thiscall SchedulerPolicy_dtor(SchedulerPolicy *this) static void ThreadScheduler_dtor(ThreadScheduler *this) { int i; + struct scheduled_chore *sc, *next; if(this->ref != 0) WARN("ref = %ld\n", this->ref); SchedulerPolicy_dtor(&this->policy); @@ -1167,6 +1210,12 @@ static void ThreadScheduler_dtor(ThreadScheduler *this) this->cs.DebugInfo->Spare[0] = 0; DeleteCriticalSection(&this->cs); + + if (!list_empty(&this->scheduled_chores)) + ERR("scheduled chore list is not empty\n"); + LIST_FOR_EACH_ENTRY_SAFE(sc, next, &this->scheduled_chores, + struct scheduled_chore, entry) + operator_delete(sc); } DEFINE_THISCALL_WRAPPER(ThreadScheduler_Id, 4) @@ -1375,6 +1424,8 @@ static ThreadScheduler* ThreadScheduler_ctor(ThreadScheduler *this, InitializeCriticalSection(&this->cs); this->cs.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ": ThreadScheduler"); + + list_init(&this->scheduled_chores); return this; } @@ -1824,6 +1875,114 @@ void __thiscall _StructuredTaskCollection_dtor(_StructuredTaskCollection *this) #endif /* _MSVCR_VER >= 120 */ +static ThreadScheduler *get_thread_scheduler_from_context(Context *context) +{ + Scheduler *scheduler = get_scheduler_from_context(context); + if (scheduler && scheduler->vtable == &ThreadScheduler_vtable) + return (ThreadScheduler*)scheduler; + return NULL; +} + +static void CALLBACK chore_wrapper_finally(BOOL normal, void *data) +{ + _UnrealizedChore *chore = data; + LONG prev_finished, new_finished; + volatile LONG *ptr; + + TRACE("(%u %p)\n", normal, data); + + if (!chore->task_collection) + return; + ptr = &chore->task_collection->finished; + chore->task_collection = NULL; + + do { + prev_finished = *ptr; + if (prev_finished == FINISHED_INITIAL) + new_finished = 1; + else + new_finished = prev_finished + 1; + } while (InterlockedCompareExchange(ptr, new_finished, prev_finished) + != prev_finished); +} + +static void __cdecl chore_wrapper(_UnrealizedChore *chore) +{ + TRACE("(%p)\n", chore); + + __TRY + { + if (chore->chore_proc) + chore->chore_proc(chore); + } + __FINALLY_CTX(chore_wrapper_finally, chore) +} + +static void __cdecl _StructuredTaskCollection_scheduler_cb(void *data) +{ + ThreadScheduler *scheduler = (ThreadScheduler*)get_current_scheduler(); + struct list *entry; + struct scheduled_chore *sc; + _UnrealizedChore *chore; + + TRACE("(%p)\n", scheduler); + + if (scheduler->scheduler.vtable != &ThreadScheduler_vtable) + { + ERR("unknown scheduler set\n"); + return; + } + + EnterCriticalSection(&scheduler->cs); + entry = list_head(&scheduler->scheduled_chores); + if (entry) + list_remove(entry); + LeaveCriticalSection(&scheduler->cs); + if (!entry) + return; + + sc = LIST_ENTRY(entry, struct scheduled_chore, entry); + chore = sc->chore; + operator_delete(sc); + + chore->chore_wrapper(chore); +} + +static bool schedule_chore(_StructuredTaskCollection *this, + _UnrealizedChore *chore, Scheduler **pscheduler) +{ + struct scheduled_chore *sc; + ThreadScheduler *scheduler; + + if (chore->task_collection) { + invalid_multiple_scheduling e; + invalid_multiple_scheduling_ctor_str(&e, "Chore scheduled multiple times"); + _CxxThrowException(&e, &invalid_multiple_scheduling_exception_type); + return FALSE; + } + + if (!this->context) + this->context = get_current_context(); + scheduler = get_thread_scheduler_from_context(this->context); + if (!scheduler) { + ERR("unknown context or scheduler set\n"); + return FALSE; + } + + sc = operator_new(sizeof(*sc)); + sc->chore = chore; + + chore->task_collection = this; + chore->chore_wrapper = chore_wrapper; + InterlockedIncrement(&this->count); + + EnterCriticalSection(&scheduler->cs); + list_add_head(&scheduler->scheduled_chores, &sc->entry); + LeaveCriticalSection(&scheduler->cs); + *pscheduler = &scheduler->scheduler; + return TRUE; +} + #if _MSVCR_VER >= 110 /* ?_Schedule(a)_StructuredTaskCollection@details(a)Concurrency@@QAAXPAV_UnrealizedChore(a)23@PAVlocation(a)3@@Z */ @@ -1834,7 +1993,15 @@ void __thiscall _StructuredTaskCollection__Schedule_loc( _StructuredTaskCollection *this, _UnrealizedChore *chore, /*location*/void *placement) { - FIXME("(%p %p %p): stub!\n", this, chore, placement); + Scheduler *scheduler; + + TRACE("(%p %p %p)\n", this, chore, placement); + + if (schedule_chore(this, chore, &scheduler)) + { + call_Scheduler_ScheduleTask_loc(scheduler, + _StructuredTaskCollection_scheduler_cb, NULL, placement); + } } #endif /* _MSVCR_VER >= 110 */ @@ -1846,7 +2013,15 @@ DEFINE_THISCALL_WRAPPER(_StructuredTaskCollection__Schedule, 8) void __thiscall _StructuredTaskCollection__Schedule( _StructuredTaskCollection *this, _UnrealizedChore *chore) { - FIXME("(%p %p): stub!\n", this, chore); + Scheduler *scheduler; + + TRACE("(%p %p)\n", this, chore); + + if (schedule_chore(this, chore, &scheduler)) + { + call_Scheduler_ScheduleTask(scheduler, + _StructuredTaskCollection_scheduler_cb, NULL); + } } /* ?_RunAndWait(a)_StructuredTaskCollection@details(a)Concurrency@@QAA?AW4_TaskCollectionStatus(a)23@PAV_UnrealizedChore(a)23@@Z */ @@ -3105,6 +3280,7 @@ void msvcrt_init_concurrency(void *base) init_improper_lock_cxx(base); init_improper_scheduler_attach_cxx(base); init_improper_scheduler_detach_cxx(base); + init_invalid_multiple_scheduling_cxx(base); init_invalid_scheduler_policy_key_cxx(base); init_invalid_scheduler_policy_thread_specification_cxx(base); init_invalid_scheduler_policy_value_cxx(base); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/464
I've pushed modified version of your patches, here's a short description of changes: - remove unneeded changes from this series (e.g. ExceptionPtr class is not used yet) - don't call Scheduler_Attach when adding new task - this part is a little tricky, in your implementation we might leak scheduler reference if ScheduleTask fails, changed code depends on scheduler task running under the same scheduler that creates it (which is not enforced yet but should be true in most cases) Does it look good for you? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/464#note_6487
On Mon Aug 22 19:06:46 2022 +0000, Piotr Caban wrote:
I've pushed modified version of your patches, here's a short description of changes: - remove unneeded changes from this series (e.g. ExceptionPtr class is not used yet) - don't call Scheduler_Attach when adding new task - this part is a little tricky, in your implementation we might leak scheduler reference if ScheduleTask fails, changed code depends on scheduler task running under the same scheduler that creates it (which is not enforced yet but should be true in most cases) Does it look good for you? Hello,
This will fail the test "test that scheduled chores are executed even if _RunAndWait is not called" if you create and attach a new Scheduler to the main thread, since the chore threads currently don't inherit the Scheduler. A quick test shows that the chore threads do inherit the attached Scheduler, so once we implement that, this implementation should work without problem. (I will need to test if this inheriting of the Scheduler is a property of StructuredTaskCollection or of ThreadScheduler. I suspect the latter but I could be wrong) I see that you removed my attempt at being thread safe in `schedule_chore`. I'm not sure how native msvcr handles concurrency but the function is probably not intended to be thread-safe anyway so no complaints there. I originally put the `operator_new` call above the `this->context` assignment so that if allocation fails we wouldn't leave `this->context` set to non-NULL. But I also never tested this (I don't even know how to trigger this case in native msvcr) so I guess this is fine too. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/464#note_6679
This merge request was approved by Piotr Caban. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/464
participants (3)
-
Piotr Caban (＠piotr) -
Torge Matthies
-
Torge Matthies (＠tmatthies)