June 2022 - Wine-Devel - list.winehq.org

Re: [PATCH v3 0/9] MR272: d3dx10: Add D3DX10CreateThreadPump implementation.

by Matteo Bruni (＠Mystral)

Matteo Bruni (@Mystral) commented about dlls/d3dx10_43/d3dx10_43_main.c: > + if (thread_pump->proc_count == THREAD_PUMP_EXITING) > + { > + ReleaseSRWLockExclusive(&thread_pump->proc_lock); > + work_item_free(work_item, TRUE); > + return 0; > + } > + > + list_add_tail(&thread_pump->proc_queue, &work_item->entry); > + thread_pump->proc_count++; > + ReleaseSRWLockExclusive(&thread_pump->proc_lock); > + RtlWakeAddressAll((void *)&thread_pump->proc_count); > + } > + return 0; > +} > + > +static DWORD WINAPI proc_thread(void *arg) Have you thought about merging the two thread functions into one? They basically do the exact same thing except for the d3dx functions they call and the queue / counter / lock they work on. The generic thread function could take a "role" parameter and key the stage-specific behavior off of it. I haven't actually tried the idea, it might be too confusing and not be worth it. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/272#note_2440

June 21, 2022

Re: [PATCH v3 0/9] MR272: d3dx10: Add D3DX10CreateThreadPump implementation.

by Matteo Bruni (＠Mystral)

Matteo Bruni (@Mystral) commented about dlls/d3dx10_43/d3dx10_43_main.c: > + if (!work_item) > + return E_OUTOFMEMORY; > + > + work_item->loader = loader; > + work_item->processor = processor; > + work_item->result = result; > + work_item->object = object; > + > + if (object) > + *object = NULL; > + > + AcquireSRWLockExclusive(&thread_pump->io_lock); > + thread_pump->io_count++; > + list_add_tail(&thread_pump->io_queue, &work_item->entry); > + ReleaseSRWLockExclusive(&thread_pump->io_lock); > + RtlWakeAddressAll((void *)&thread_pump->io_count); Would it make sense to call RtlWakeAddressSingle() here (and in the other places where we add a work item to a queue) instead? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/272#note_2439

June 21, 2022

Re: [PATCH v3 0/9] MR272: d3dx10: Add D3DX10CreateThreadPump implementation.

by Matteo Bruni (＠Mystral)

Matteo Bruni (@Mystral) commented about dlls/d3dx10_43/d3dx10_43_main.c: > thread_pump_GetQueueStatus > }; > > +static DWORD WINAPI io_thread(void *arg) > +{ > + struct thread_pump *thread_pump = arg; > + struct work_item *work_item; > + UINT zero = 0; > + HRESULT hr; > + > + TRACE("%p thread started.\n", thread_pump); > + > + while (1) > + { > + RtlWaitOnAddress((void *)&thread_pump->io_count, &zero, sizeof(zero), NULL); Nitpick, we prefer "for (;;)" for infinite loops. You can drop these casts in RtlWaitOnAddress() calls too, I think. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/272#note_2438

June 21, 2022

Re: [PATCH v3 0/9] MR272: d3dx10: Add D3DX10CreateThreadPump implementation.

by Matteo Bruni (＠Mystral)

Matteo Bruni (@Mystral) commented about dlls/d3dx10_43/d3dx10_43_main.c: > + thread_pump->io_count = THREAD_PUMP_EXITING; > + ReleaseSRWLockExclusive(&thread_pump->io_lock); > + RtlWakeAddressAll((void *)&thread_pump->io_count); > + > + AcquireSRWLockExclusive(&thread_pump->proc_lock); > + thread_pump->proc_count = THREAD_PUMP_EXITING; > + ReleaseSRWLockExclusive(&thread_pump->proc_lock); > + RtlWakeAddressAll((void *)&thread_pump->proc_count); > + > + AcquireSRWLockExclusive(&thread_pump->device_lock); > + thread_pump->device_count = THREAD_PUMP_EXITING; > + ReleaseSRWLockExclusive(&thread_pump->device_lock); > + > + for (i = 0; i < thread_pump->threads_no; i++) > + { > + if (!thread_pump->threads[i]) continue; More nitpick, "if" body on a separate line. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/272#note_2437

June 21, 2022

Re: [PATCH v3 0/9] MR272: d3dx10: Add D3DX10CreateThreadPump implementation.

by Matteo Bruni (＠Mystral)

Matteo Bruni (@Mystral) commented about dlls/d3dx10_43/d3dx10_43_main.c: > + { > + AcquireSRWLockExclusive(&thread_pump->io_lock); > + thread_pump->io_count = THREAD_PUMP_EXITING; > + ReleaseSRWLockExclusive(&thread_pump->io_lock); > + RtlWakeAddressAll((void *)&thread_pump->io_count); > + > + AcquireSRWLockExclusive(&thread_pump->proc_lock); > + thread_pump->proc_count = THREAD_PUMP_EXITING; > + ReleaseSRWLockExclusive(&thread_pump->proc_lock); > + RtlWakeAddressAll((void *)&thread_pump->proc_count); > + > + AcquireSRWLockExclusive(&thread_pump->device_lock); > + thread_pump->device_count = THREAD_PUMP_EXITING; > + ReleaseSRWLockExclusive(&thread_pump->device_lock); > + > + for (i = 0; i < thread_pump->threads_no; i++) Nitpick, we usually do "++i" in d3d code (obviously when either option works). Similar for ++io_count and such. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/272#note_2436

June 21, 2022

Re: [PATCH v3 0/9] MR272: d3dx10: Add D3DX10CreateThreadPump implementation.

by Matteo Bruni (＠Mystral)

Matteo Bruni (@Mystral) commented about dlls/d3dx10_43/d3dx10_43_main.c: > { > struct thread_pump *thread_pump = impl_from_ID3DX10ThreadPump(iface); > ULONG refcount = InterlockedDecrement(&thread_pump->refcount); > + struct work_item *iter, *next; > + struct list list; > + int i; We generally prefer using "unsigned int" for loop counter variables (assuming there is no reason to prefer a signed integer here). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/272#note_2435

June 21, 2022

Re: [PATCH v3 0/9] MR272: d3dx10: Add D3DX10CreateThreadPump implementation.

by Matteo Bruni (＠Mystral)

Matteo Bruni (@Mystral) commented about dlls/d3dx10_43/d3dx10_43_main.c: > { > struct thread_pump *thread_pump = impl_from_ID3DX10ThreadPump(iface); > ULONG refcount = InterlockedDecrement(&thread_pump->refcount); > + struct work_item *iter, *next; Not a big deal, here especially, but "iter" could be "item" instead. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/272#note_2434

June 21, 2022

Re: [PATCH v3 0/9] MR272: d3dx10: Add D3DX10CreateThreadPump implementation.

by Matteo Bruni (＠Mystral)

Matteo Bruni (@Mystral) commented about dlls/d3dx10_43/d3dx10_43_main.c: > { > struct thread_pump *thread_pump = impl_from_ID3DX10ThreadPump(iface); > ULONG refcount = InterlockedDecrement(&thread_pump->refcount); > + struct work_item *iter, *next; > + struct list list; > + int i; > > TRACE("%p decreasing refcount to %lu.\n", iface, refcount); > > if (!refcount) > + { > + AcquireSRWLockExclusive(&thread_pump->io_lock); > + thread_pump->io_count = THREAD_PUMP_EXITING; > + ReleaseSRWLockExclusive(&thread_pump->io_lock); > + RtlWakeAddressAll((void *)&thread_pump->io_count); It should be possible to drop the cast here and in all the other RtlWakeAddressAll() calls. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/272#note_2433

June 21, 2022

Re: [PATCH v3 0/9] MR272: d3dx10: Add D3DX10CreateThreadPump implementation.

by Matteo Bruni (＠Mystral)

Matteo Bruni (@Mystral) commented about dlls/d3dx10_43/d3dx10_43_main.c: > LONG refcount; > + > + SRWLOCK io_lock; > + UINT io_count; > + struct list io_queue; > + > + SRWLOCK proc_lock; > + UINT proc_count; > + struct list proc_queue; > + > + SRWLOCK device_lock; > + UINT device_count; > + struct list device_queue; > + > + int threads_no; > + HANDLE threads[1]; I think we can make all these _count variables "unsigned int". Relatedly, threads_no -> thread_count. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/272#note_2432

June 21, 2022

[PATCH 1/1] iphlpapi/tests: Allow the route's age to differ by one second.

by Huw Davies

From: Huw Davies <huw(a)codeweavers.com> Signed-off-by: Huw Davies <huw(a)codeweavers.com> --- dlls/iphlpapi/tests/iphlpapi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/dlls/iphlpapi/tests/iphlpapi.c b/dlls/iphlpapi/tests/iphlpapi.c index 29256eb2774..214256c18b4 100644 --- a/dlls/iphlpapi/tests/iphlpapi.c +++ b/dlls/iphlpapi/tests/iphlpapi.c @@ -350,7 +350,8 @@ static void testGetIpForwardTable(void) else ok( buf->table[i].dwForwardType == MIB_IPROUTE_TYPE_INDIRECT, "got %ld\n", buf->table[i].dwForwardType ); ok( row->dwForwardProto == row2->Protocol, "got %ld vs %d\n", row->dwForwardProto, row2->Protocol ); - ok( row->dwForwardAge == row2->Age, "got %ld vs %ld\n", row->dwForwardAge, row2->Age ); + ok( row->dwForwardAge == row2->Age || row->dwForwardAge + 1 == row2->Age, + "got %ld vs %ld\n", row->dwForwardAge, row2->Age ); ok( row->dwForwardNextHopAS == 0, "got %08lx\n", row->dwForwardNextHopAS ); /* FIXME: need to add the interface's metric from GetIpInterfaceTable() */ ok( row->dwForwardMetric1 >= row2->Metric, "got %ld vs %ld\n", row->dwForwardMetric1, row2->Metric ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/290

June 21, 2022