[PATCH v2 1/3] ntdll/tests: Test updating TickCount in user_shared_data.
Original patch from: Andrew Wesie <awesie(a)gmail.com> Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- v2: This should fix a race condition with the previous version where the service could quit and detach USD view from other services although they would still be using it. This version now relies on both the processes and the service open predetermined named sections and each map it into its address space. The service can now start late or exit early without impacting the processes. It can now also catch up with previously started services and have the timestamp updates working for them as well. dlls/ntdll/tests/time.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/dlls/ntdll/tests/time.c b/dlls/ntdll/tests/time.c index 5382a952d8d..6ce56f39fd8 100644 --- a/dlls/ntdll/tests/time.c +++ b/dlls/ntdll/tests/time.c @@ -18,7 +18,9 @@ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA */ +#define NONAMELESSUNION #include "ntdll_test.h" +#include "ddk/wdm.h" #define TICKSPERSEC 10000000 #define TICKSPERMSEC 10000 @@ -29,6 +31,7 @@ static VOID (WINAPI *pRtlTimeFieldsToTime)( PTIME_FIELDS TimeFields, PLARGE_IN static NTSTATUS (WINAPI *pNtQueryPerformanceCounter)( LARGE_INTEGER *counter, LARGE_INTEGER *frequency ); static NTSTATUS (WINAPI *pRtlQueryTimeZoneInformation)( RTL_TIME_ZONE_INFORMATION *); static NTSTATUS (WINAPI *pRtlQueryDynamicTimeZoneInformation)( RTL_DYNAMIC_TIME_ZONE_INFORMATION *); +static ULONG (WINAPI *pNtGetTickCount)(void); static const int MonthLengths[2][12] = { @@ -153,12 +156,35 @@ static void test_RtlQueryTimeZoneInformation(void) wine_dbgstr_w(tzinfo.DaylightName)); } +static void test_NtGetTickCount(void) +{ + KSHARED_USER_DATA *user_shared_data = (void *)0x7ffe0000; + ULONG diff; + int i; + + if (!pNtGetTickCount) + { + win_skip("NtGetTickCount is not available\n"); + return; + } + + for (i = 0; i < 5; ++i) + { + diff = (user_shared_data->u.TickCountQuad * user_shared_data->TickCountMultiplier) >> 24; + diff = pNtGetTickCount() - diff; + todo_wine + ok(diff < 100, "NtGetTickCount - TickCountQuad too high, expected < 100 got %d\n", diff); + Sleep(50); + } +} + START_TEST(time) { HMODULE mod = GetModuleHandleA("ntdll.dll"); pRtlTimeToTimeFields = (void *)GetProcAddress(mod,"RtlTimeToTimeFields"); pRtlTimeFieldsToTime = (void *)GetProcAddress(mod,"RtlTimeFieldsToTime"); pNtQueryPerformanceCounter = (void *)GetProcAddress(mod, "NtQueryPerformanceCounter"); + pNtGetTickCount = (void *)GetProcAddress(mod,"NtGetTickCount"); pRtlQueryTimeZoneInformation = (void *)GetProcAddress(mod, "RtlQueryTimeZoneInformation"); pRtlQueryDynamicTimeZoneInformation = @@ -169,5 +195,6 @@ START_TEST(time) else win_skip("Required time conversion functions are not available\n"); test_NtQueryPerformanceCounter(); + test_NtGetTickCount(); test_RtlQueryTimeZoneInformation(); } -- 2.26.1
On startup, every process will create a \Device\WineUsd\<PID> section and will map it in place of its temporary user shared data. If creating the section or mapping it fails, the process will continue as before without having user shared data timestamp updates. If it succeeds, it will then open the \Device\WineUsd\Control device to notify the WineUsd service of a new process startup. The service then opens the process section and maps it in its address space. If the service is not yet started, then opening the device will fail and the process will continue without having the timestamp updates until it does. On startup, the WineUsd service will go through the process list and try to open the section to catch up. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- configure.ac | 1 + dlls/wineusd.sys/Makefile.in | 6 + dlls/wineusd.sys/wineusd.sys.spec | 1 + dlls/wineusd.sys/wineusd_main.c | 290 ++++++++++++++++++++++++++++++ include/wine/usd.h | 27 +++ loader/wine.inf.in | 12 ++ 6 files changed, 337 insertions(+) create mode 100644 dlls/wineusd.sys/Makefile.in create mode 100644 dlls/wineusd.sys/wineusd.sys.spec create mode 100644 dlls/wineusd.sys/wineusd_main.c create mode 100644 include/wine/usd.h diff --git a/configure.ac b/configure.ac index fb775d7a73c..038b4175a6f 100644 --- a/configure.ac +++ b/configure.ac @@ -3779,6 +3779,7 @@ WINE_CONFIG_MAKEFILE(dlls/wineps.drv) WINE_CONFIG_MAKEFILE(dlls/wineps16.drv16,enable_win16) WINE_CONFIG_MAKEFILE(dlls/winepulse.drv) WINE_CONFIG_MAKEFILE(dlls/wineqtdecoder) +WINE_CONFIG_MAKEFILE(dlls/wineusd.sys) WINE_CONFIG_MAKEFILE(dlls/winevulkan) WINE_CONFIG_MAKEFILE(dlls/winex11.drv) WINE_CONFIG_MAKEFILE(dlls/wing.dll16,enable_win16) diff --git a/dlls/wineusd.sys/Makefile.in b/dlls/wineusd.sys/Makefile.in new file mode 100644 index 00000000000..0792860c09b --- /dev/null +++ b/dlls/wineusd.sys/Makefile.in @@ -0,0 +1,6 @@ +MODULE = wineusd.sys +IMPORTS = ntoskrnl +EXTRADLLFLAGS = -Wl,--subsystem,native -mno-cygwin + +C_SRCS = \ + wineusd_main.c diff --git a/dlls/wineusd.sys/wineusd.sys.spec b/dlls/wineusd.sys/wineusd.sys.spec new file mode 100644 index 00000000000..76421d7e35b --- /dev/null +++ b/dlls/wineusd.sys/wineusd.sys.spec @@ -0,0 +1 @@ +# nothing to export diff --git a/dlls/wineusd.sys/wineusd_main.c b/dlls/wineusd.sys/wineusd_main.c new file mode 100644 index 00000000000..58b6d1eb38b --- /dev/null +++ b/dlls/wineusd.sys/wineusd_main.c @@ -0,0 +1,290 @@ +/* + * User shared data update service + * + * Copyright 2019 Rémi Bernon for CodeWeavers + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA + */ + +#include <assert.h> +#include <stdarg.h> +#include "ntstatus.h" +#define WIN32_NO_STATUS +#include "winternl.h" +#include "winioctl.h" +#include "ddk/wdm.h" +#include "wine/debug.h" +#include "wine/heap.h" +#include "wine/list.h" +#include "wine/usd.h" + +static HANDLE directory_obj; +static DEVICE_OBJECT *device_obj; + +WINE_DEFAULT_DEBUG_CHANNEL(wineusd); + +#define DECLARE_CRITICAL_SECTION(cs) \ + static CRITICAL_SECTION cs; \ + static CRITICAL_SECTION_DEBUG cs##_debug = \ + { 0, 0, &cs, { &cs##_debug.ProcessLocksList, &cs##_debug.ProcessLocksList }, \ + 0, 0, { (DWORD_PTR)(__FILE__ ": " # cs) }}; \ + static CRITICAL_SECTION cs = { &cs##_debug, -1, 0, 0, 0, 0 }; + +DECLARE_CRITICAL_SECTION(wineusd_cs); + +static struct list wineusd_entries = LIST_INIT(wineusd_entries); +static HANDLE wineusd_thread, wineusd_thread_stop; + +struct wineusd_entry +{ + struct list link; + ULONG32 pid; + HANDLE section; + void *page; +}; + +static NTSTATUS wineusd_create(HANDLE pid, struct wineusd_entry **entry_ptr) +{ + static const WCHAR section_formatW[] = {'\\','D','e','v','i','c','e','\\','W','i','n','e','U','s','d','\\','%','0','8','x',0}; + struct wineusd_entry *entry; + OBJECT_ATTRIBUTES attr = {sizeof(attr)}; + UNICODE_STRING string; + NTSTATUS status; + SIZE_T size = 0; + WCHAR section_nameW[64]; + + LIST_FOR_EACH_ENTRY(entry, &wineusd_entries, struct wineusd_entry, link) + if (entry->pid == HandleToUlong(PsGetCurrentProcessId())) + goto done; + + if (!(entry = heap_alloc_zero(sizeof(*entry)))) + return STATUS_NO_MEMORY; + entry->pid = HandleToUlong(pid); + + swprintf(section_nameW, ARRAY_SIZE(section_nameW), section_formatW, entry->pid); + RtlInitUnicodeString(&string, section_nameW); + InitializeObjectAttributes(&attr, &string, 0, NULL, NULL); + if ((status = NtOpenSection(&entry->section, SECTION_ALL_ACCESS, &attr))) + { + /* the main process may be starting up, we should get notified later again */ + WARN("Failed to open section for process %08x, status: %x.\n", entry->pid, status); + goto error; + } + + entry->page = NULL; + size = 0; + if ((status = NtMapViewOfSection(entry->section, NtCurrentProcess(), &entry->page, 0, 0, 0, + &size, ViewShare, 0, PAGE_READWRITE))) + { + ERR("Failed to map section to driver memory, status: %x.\n", status); + goto error; + } + + list_add_head(&wineusd_entries, &entry->link); + TRACE("Created user shared data for process %08x.\n", entry->pid); + +done: + *entry_ptr = entry; + return STATUS_SUCCESS; + +error: + if (entry && entry->section) NtClose(entry->section); + if (entry) heap_free(entry); + return status; +} + +static void wineusd_close(struct wineusd_entry *entry) +{ + TRACE("Closing user shared data for process %08x.\n", entry->pid); + + list_remove(&entry->link); + NtUnmapViewOfSection(NtCurrentProcess(), entry->page); + NtClose(entry->section); + heap_free(entry); +} + +static NTSTATUS wineusd_initialize(void) +{ + SYSTEM_PROCESS_INFORMATION *spi; + struct wineusd_entry *entry; + NTSTATUS status; + ULONG size = 0x4000; + char *buffer; + + if (!(buffer = heap_alloc(size))) + return STATUS_NO_MEMORY; + + while ((status = NtQuerySystemInformation(SystemProcessInformation, buffer, size, NULL)) + == STATUS_INFO_LENGTH_MISMATCH) + { + size *= 2; + if (!(buffer = heap_realloc(buffer, size))) + return STATUS_NO_MEMORY; + } + + if (status) + { + ERR("Failed to list existing processes, status:%x\n", status); + goto done; + } + + spi = (SYSTEM_PROCESS_INFORMATION*)buffer; + do + { + wineusd_create(spi->UniqueProcessId, &entry); + if (spi->NextEntryOffset == 0) break; + spi = (SYSTEM_PROCESS_INFORMATION *)((char *)spi + spi->NextEntryOffset); + } + while ((char *)spi < buffer + size); + +done: + if (buffer) heap_free(buffer); + return status; +} + +static DWORD WINAPI wineusd_thread_proc(void *arg) +{ + struct wineusd_entry *entry; + ULARGE_INTEGER interrupt; + ULARGE_INTEGER tick; + LARGE_INTEGER now; + NTSTATUS status; + + EnterCriticalSection(&wineusd_cs); + if ((status = wineusd_initialize())) + WARN("Failed to initialize process list, status:%x\n", status); + LeaveCriticalSection(&wineusd_cs); + + TRACE("Started user shared data thread.\n"); + + while (WaitForSingleObject(wineusd_thread_stop, 16) == WAIT_TIMEOUT) + { + NtQuerySystemTime(&now); + RtlQueryUnbiasedInterruptTime(&interrupt.QuadPart); + + tick = interrupt; + tick.QuadPart /= 10000; + + EnterCriticalSection(&wineusd_cs); + LIST_FOR_EACH_ENTRY(entry, &wineusd_entries, struct wineusd_entry, link) + { + KSHARED_USER_DATA *usd = entry->page; + + usd->SystemTime.High2Time = now.u.HighPart; + usd->SystemTime.LowPart = now.u.LowPart; + usd->SystemTime.High1Time = now.u.HighPart; + + usd->InterruptTime.High2Time = interrupt.HighPart; + usd->InterruptTime.LowPart = interrupt.LowPart; + usd->InterruptTime.High1Time = interrupt.HighPart; + + usd->TickCount.High2Time = tick.HighPart; + usd->TickCount.LowPart = tick.LowPart; + usd->TickCount.High1Time = tick.HighPart; + usd->TickCountLowDeprecated = tick.LowPart; + usd->TickCountMultiplier = 1 << 24; + } + LeaveCriticalSection(&wineusd_cs); + } + + TRACE("Stopped user shared data thread.\n"); + + return 0; +} + +static NTSTATUS WINAPI wineusd_dispatch_create(DEVICE_OBJECT *device, IRP *irp) +{ + struct wineusd_entry *entry = NULL; + IO_STACK_LOCATION *stack = IoGetCurrentIrpStackLocation(irp); + NTSTATUS status = STATUS_SUCCESS; + + EnterCriticalSection(&wineusd_cs); + status = wineusd_create(PsGetCurrentProcessId(), &entry); + LeaveCriticalSection(&wineusd_cs); + + stack->FileObject->FsContext = entry; + irp->IoStatus.Status = status; + IoCompleteRequest(irp, IO_NO_INCREMENT); + return status; +} + +static NTSTATUS WINAPI wineusd_dispatch_close(DEVICE_OBJECT *device, IRP *irp) +{ + IO_STACK_LOCATION *stack = IoGetCurrentIrpStackLocation(irp); + struct wineusd_entry *entry = stack->FileObject->FsContext; + + if (entry) + { + EnterCriticalSection(&wineusd_cs); + wineusd_close(entry); + LeaveCriticalSection(&wineusd_cs); + } + + irp->IoStatus.Status = STATUS_SUCCESS; + IoCompleteRequest(irp, IO_NO_INCREMENT); + return STATUS_SUCCESS; +} + +static void WINAPI wineusd_unload(DRIVER_OBJECT *driver) +{ + struct wineusd_entry *entry, *cursor; + + SetEvent(wineusd_thread_stop); + WaitForSingleObject(wineusd_thread, INFINITE); + CloseHandle(wineusd_thread); + CloseHandle(wineusd_thread_stop); + + EnterCriticalSection(&wineusd_cs); + LIST_FOR_EACH_ENTRY_SAFE(entry, cursor, &wineusd_entries, struct wineusd_entry, link) + wineusd_close(entry); + + IoDeleteDevice(device_obj); + NtClose(directory_obj); + LeaveCriticalSection(&wineusd_cs); +} + +NTSTATUS WINAPI DriverEntry(DRIVER_OBJECT *driver, UNICODE_STRING *path) +{ + static const WCHAR directory_nameW[] = {'\\','D','e','v','i','c','e','\\','W','i','n','e','U','s','d',0}; + static const WCHAR device_nameW[] = {'\\','D','e','v','i','c','e','\\','W','i','n','e','U','s','d','\\','C','o','n','t','r','o','l',0}; + OBJECT_ATTRIBUTES attr = {sizeof(attr)}; + UNICODE_STRING string; + NTSTATUS status; + + TRACE("Driver %p, path %s.\n", driver, debugstr_w(path->Buffer)); + + RtlInitUnicodeString(&string, directory_nameW); + InitializeObjectAttributes(&attr, &string, 0, NULL, NULL); + if ((status = NtCreateDirectoryObject(&directory_obj, 0, &attr)) && + status != STATUS_OBJECT_NAME_COLLISION) + ERR("Failed to create directory, status: %x\n", status); + + RtlInitUnicodeString(&string, device_nameW); + if ((status = IoCreateDevice(driver, 0, &string, FILE_DEVICE_UNKNOWN, 0, FALSE, &device_obj))) + { + ERR("Failed to create user shared data device, status: %x\n", status); + NtClose(directory_obj); + return status; + } + + driver->MajorFunction[IRP_MJ_CREATE] = wineusd_dispatch_create; + driver->MajorFunction[IRP_MJ_CLOSE] = wineusd_dispatch_close; + driver->DriverUnload = wineusd_unload; + + wineusd_thread_stop = CreateEventW(NULL, FALSE, FALSE, NULL); + wineusd_thread = CreateThread(NULL, 0, wineusd_thread_proc, NULL, 0, NULL); + + return STATUS_SUCCESS; +} diff --git a/include/wine/usd.h b/include/wine/usd.h new file mode 100644 index 00000000000..e92efa5e29e --- /dev/null +++ b/include/wine/usd.h @@ -0,0 +1,27 @@ +/* + * Copyright 2019 Rémi Bernon for CodeWeavers + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA + */ + +#ifndef __WINE_WINE_USD_H +#define __WINE_WINE_USD_H + +#include <windef.h> +#include <winioctl.h> + +#define IOCTL_WINEUSD_INITIALIZE CTL_CODE(FILE_DEVICE_UNKNOWN, 0x800, METHOD_BUFFERED, FILE_ANY_ACCESS) + +#endif /* __WINE_WINE_USD_H */ diff --git a/loader/wine.inf.in b/loader/wine.inf.in index d321c4c8268..67f19dea9c0 100644 --- a/loader/wine.inf.in +++ b/loader/wine.inf.in @@ -161,6 +161,7 @@ AddService=FontCache3.0.0.0,0,WPFFontCacheService AddService=LanmanServer,0,LanmanServerService AddService=FontCache,0,FontCacheService AddService=Schedule,0,TaskSchedulerService +AddService=WineUsd,0,WineUsdService AddService=Winmgmt,0,WinmgmtService AddService=wuauserv,0,wuauService @@ -178,6 +179,7 @@ AddService=FontCache3.0.0.0,0,WPFFontCacheService AddService=LanmanServer,0,LanmanServerService AddService=FontCache,0,FontCacheService AddService=Schedule,0,TaskSchedulerService +AddService=WineUsd,0,WineUsdService AddService=Winmgmt,0,WinmgmtService AddService=wuauserv,0,wuauService @@ -195,6 +197,7 @@ AddService=FontCache3.0.0.0,0,WPFFontCacheService AddService=LanmanServer,0,LanmanServerService AddService=FontCache,0,FontCacheService AddService=Schedule,0,TaskSchedulerService +AddService=WineUsd,0,WineUsdService AddService=Winmgmt,0,WinmgmtService AddService=wuauserv,0,wuauService @@ -212,6 +215,7 @@ AddService=FontCache3.0.0.0,0,WPFFontCacheService AddService=LanmanServer,0,LanmanServerService AddService=FontCache,0,FontCacheService AddService=Schedule,0,TaskSchedulerService +AddService=WineUsd,0,WineUsdService AddService=Winmgmt,0,WinmgmtService AddService=wuauserv,0,wuauService @@ -3637,6 +3641,14 @@ ServiceType=32 StartType=3 ErrorControl=1 +[WineUsdService] +Description="User shared data update service" +DisplayName="Wine User Shared Data" +ServiceBinary="%12%\WineUsd.sys" +ServiceType=1 +StartType=0 +ErrorControl=1 + [WinmgmtService] Description="Provides access to Windows Management Instrumentation" DisplayName="Windows Management Instrumentation Service" -- 2.26.1
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=70420 Your paranoid android. === debiant (build log) === error: patch failed: configure.ac:3779 Task: Patch failed to apply === debiant (build log) === error: patch failed: configure.ac:3779 Task: Patch failed to apply
It conflicts with the recent wineusb addition, and I'm now thinking that wineusd/wineusb may be confusing. It could be winekusd instead but I'm open to suggestions. Then maybe it's also not worth resending it if it's not the right direction to implement user shared data timestamp update. -- Rémi Bernon <rbernon(a)codeweavers.com>
I might have missed something, but is there any reason why clients can't do a readonly mapping of a single page from the service? The service will have to update just one page in this case. Also, is it really not possible to all that with wineserver, which will avoid all that issues of startup dependency. I understand why the wineserver operation is wanted to stay single threaded, but does a completely independent thread which just updates a single page in memory and does not interfere with anything else hurt? On 4/24/20 15:36, Rémi Bernon wrote:
On startup, every process will create a \Device\WineUsd\<PID> section and will map it in place of its temporary user shared data. If creating the section or mapping it fails, the process will continue as before without having user shared data timestamp updates.
If it succeeds, it will then open the \Device\WineUsd\Control device to notify the WineUsd service of a new process startup. The service then opens the process section and maps it in its address space.
If the service is not yet started, then opening the device will fail and the process will continue without having the timestamp updates until it does.
On startup, the WineUsd service will go through the process list and try to open the section to catch up.
On 4/24/20 2:54 PM, Paul Gofman wrote:
I might have missed something, but is there any reason why clients can't do a readonly mapping of a single page from the service? The service will have to update just one page in this case. Also, is it really not possible to all that with wineserver, which will avoid all that issues of startup dependency. I understand why the wineserver operation is wanted to stay single threaded, but does a completely independent thread which just updates a single page in memory and does not interfere with anything else hurt?
On 4/24/20 15:36, Rémi Bernon wrote:
On startup, every process will create a \Device\WineUsd\<PID> section and will map it in place of its temporary user shared data. If creating the section or mapping it fails, the process will continue as before without having user shared data timestamp updates.
If it succeeds, it will then open the \Device\WineUsd\Control device to notify the WineUsd service of a new process startup. The service then opens the process section and maps it in its address space.
If the service is not yet started, then opening the device will fail and the process will continue without having the timestamp updates until it does.
On startup, the WineUsd service will go through the process list and try to open the section to catch up.
The problem with having a single page is that Wine is able to emulate different windows versions for every process, and the so USD may contain different data for every process. It would maybe be reduced to one page per version or something like that but it's also simpler to have a page per process. I've assumed from the past discussions on the staging patches that having it done in wineserver is going to be much more difficult to get accepted, but that's my interpretation. There was a discussion in previous WineConf where I believe Alexandre said it would be OK to share readonly memory from wineserver to processes, so from that point of view it may be alright. But I also understand that some applications expect timestamps to be updated very regularly. We could try to do that on every server request, but at the risk that the regularity to be less accurate than having a separate thread do the work. And having a separate thread in wineserver for this sole purpose starts introducing concurrency anyway, even if it's for a simple task, and I got the impression that this is never going to be OK. Then this is my interpretation, and I would be happy to explore another route if it's considered a better approach. -- Rémi Bernon <rbernon(a)codeweavers.com>
On 4/24/20 16:06, Rémi Bernon wrote:
On 4/24/20 2:54 PM, Paul Gofman wrote:
The problem with having a single page is that Wine is able to emulate different windows versions for every process, and the so USD may contain different data for every process. It would maybe be reduced to one page per version or something like that but it's also simpler to have a page per process.
Oh yeah, I see, thanks.
But I also understand that some applications expect timestamps to be updated very regularly. We could try to do that on every server request, but at the risk that the regularity to be less accurate than having a separate thread do the work. And having a separate thread in wineserver for this sole purpose starts introducing concurrency anyway, even if it's for a simple task, and I got the impression that this is never going to be OK.
What I am missing is what can be bad about an independent thread sleeping and waking to update its own pages in memory. The rest of wineserver can even not share its memory with it, there is no great difference between thread itself (unless it wants to do share and sync something with the main thread) and a separate process, except for signal handling. Maybe signal handling is more difficult to isolate cleanly than I thought? If yes, maybe it would be easier to consider native separate process spawned by wineserver instead of going through Win service infrastructure?
On 4/24/20 3:22 PM, Paul Gofman wrote:
On 4/24/20 16:06, Rémi Bernon wrote:
On 4/24/20 2:54 PM, Paul Gofman wrote:
The problem with having a single page is that Wine is able to emulate different windows versions for every process, and the so USD may contain different data for every process. It would maybe be reduced to one page per version or something like that but it's also simpler to have a page per process.
Oh yeah, I see, thanks.
But I also understand that some applications expect timestamps to be updated very regularly. We could try to do that on every server request, but at the risk that the regularity to be less accurate than having a separate thread do the work. And having a separate thread in wineserver for this sole purpose starts introducing concurrency anyway, even if it's for a simple task, and I got the impression that this is never going to be OK.
What I am missing is what can be bad about an independent thread sleeping and waking to update its own pages in memory. The rest of wineserver can even not share its memory with it, there is no great difference between thread itself (unless it wants to do share and sync something with the main thread) and a separate process, except for signal handling. Maybe signal handling is more difficult to isolate cleanly than I thought? If yes, maybe it would be easier to consider native separate process spawned by wineserver instead of going through Win service infrastructure?
My understanding is that it would still possibly require some synchronization when a new process is created or destroyed, especially if we need a page per process. And once you have a second thread, then why not a third for some other "simple" task, etc. As I understand it, the single thread wineserver is a principle which suffers from no exception for the sake of simplicity. And that will hold until the moment where there's no other way -but I don't see that happening soon. -- Rémi Bernon <rbernon(a)codeweavers.com>
On 4/24/20 16:29, Rémi Bernon wrote:
And once you have a second thread, then why not a third for some other "simple" task, etc. As I understand it, the single thread wineserver is a principle which suffers from no exception for the sake of simplicity. And that will hold until the moment where there's no other way -but I don't see that happening soon.
Can't wineserver just spawn another native wineusd server then, give it a pipe so wineusd can die from SIGPIPE when main server dies, and let ntdll request the shm object from the second server? If that second server dies, everything will crash, just like if wineserver dies, but the life time of such native process does not depend on the win stuff within Wine prefix. And will avoid Windows mappings which can be detectable by applications and annoy some excessively sensitive ones. That's IMO would avoid some issues.
If the device cannot be opened, then the service may not be started yet, or the current process is the service itself. In which case, we can ignore the error. The service will catch up by going through the existing process list on startup. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=29168 Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- Notes: When submitting the patch on testbot, the service was not started and the test failed. It may be the same there and I think the wine.inf update requires a server restart to be accounted for. I'm not sure how to force the new service to start right away. dlls/ntdll/ntdll_misc.h | 1 + dlls/ntdll/server.c | 2 ++ dlls/ntdll/tests/time.c | 1 - dlls/ntdll/thread.c | 75 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 78 insertions(+), 1 deletion(-) diff --git a/dlls/ntdll/ntdll_misc.h b/dlls/ntdll/ntdll_misc.h index b62239de5b4..bd4caef5d0c 100644 --- a/dlls/ntdll/ntdll_misc.h +++ b/dlls/ntdll/ntdll_misc.h @@ -202,6 +202,7 @@ extern void virtual_set_large_address_space(void) DECLSPEC_HIDDEN; extern void virtual_fill_image_information( const pe_image_info_t *pe_info, SECTION_IMAGE_INFORMATION *info ) DECLSPEC_HIDDEN; extern struct _KUSER_SHARED_DATA *user_shared_data DECLSPEC_HIDDEN; +extern void user_shared_data_init(void); /* completion */ extern NTSTATUS NTDLL_AddCompletion( HANDLE hFile, ULONG_PTR CompletionValue, diff --git a/dlls/ntdll/server.c b/dlls/ntdll/server.c index d6b9b2ff0ab..dcd1d44e3d8 100644 --- a/dlls/ntdll/server.c +++ b/dlls/ntdll/server.c @@ -1490,6 +1490,8 @@ void server_init_process_done(void) } SERVER_END_REQ; + user_shared_data_init(); + assert( !status ); signal_start_process( entry, suspend ); } diff --git a/dlls/ntdll/tests/time.c b/dlls/ntdll/tests/time.c index 6ce56f39fd8..df08d2c972d 100644 --- a/dlls/ntdll/tests/time.c +++ b/dlls/ntdll/tests/time.c @@ -172,7 +172,6 @@ static void test_NtGetTickCount(void) { diff = (user_shared_data->u.TickCountQuad * user_shared_data->TickCountMultiplier) >> 24; diff = pNtGetTickCount() - diff; - todo_wine ok(diff < 100, "NtGetTickCount - TickCountQuad too high, expected < 100 got %d\n", diff); Sleep(50); } diff --git a/dlls/ntdll/thread.c b/dlls/ntdll/thread.c index 6baeb610fc4..3e5993d369c 100644 --- a/dlls/ntdll/thread.c +++ b/dlls/ntdll/thread.c @@ -46,10 +46,13 @@ #include "wine/server.h" #include "wine/debug.h" #include "ntdll_misc.h" +#include "wine/unicode.h" +#include "wine/usd.h" #include "ddk/wdm.h" #include "wine/exception.h" WINE_DEFAULT_DEBUG_CHANNEL(thread); +WINE_DECLARE_DEBUG_CHANNEL(wineusd); #ifndef PTHREAD_STACK_MIN #define PTHREAD_STACK_MIN 16384 @@ -212,6 +215,78 @@ static void set_process_name( int argc, char *argv[] ) #endif /* HAVE_PRCTL */ } +void user_shared_data_init(void) +{ + static const WCHAR directory_nameW[] = {'\\','D','e','v','i','c','e','\\','W','i','n','e','U','s','d',0}; + static const WCHAR device_nameW[] = {'\\','D','e','v','i','c','e','\\','W','i','n','e','U','s','d','\\','C','o','n','t','r','o','l',0}; + static const WCHAR section_formatW[] = {'\\','D','e','v','i','c','e','\\','W','i','n','e','U','s','d','\\','%','0','8','x',0}; + OBJECT_ATTRIBUTES attr = {sizeof(attr)}; + IO_STATUS_BLOCK io; + UNICODE_STRING string; + NTSTATUS status; + HANDLE directory, section, device; + WCHAR section_nameW[64]; + + struct _KUSER_SHARED_DATA tmp; + LARGE_INTEGER section_size; + SIZE_T size = 0x10000; + void *addr; + + section_size.HighPart = 0; + section_size.LowPart = size; + + RtlInitUnicodeString( &string, directory_nameW ); + InitializeObjectAttributes( &attr, &string, 0, NULL, NULL ); + if ((status = NtCreateDirectoryObject( &directory, 0, &attr )) && status != STATUS_OBJECT_NAME_COLLISION) + WARN_(wineusd)( "Failed to create directory, status: %x\n", status ); + + sprintfW( section_nameW, section_formatW, GetCurrentProcessId() ); + RtlInitUnicodeString( &string, section_nameW ); + InitializeObjectAttributes( &attr, &string, 0, NULL, NULL ); + if ((status = NtCreateSection( §ion, SECTION_ALL_ACCESS, &attr, §ion_size, + PAGE_READWRITE, SEC_COMMIT, NULL ))) + { + WARN_(wineusd)( "Failed to create section, status: %x\n", status ); + return; + } + + NtClose( directory ); + + addr = (void *)user_shared_data; + size = 0; + tmp = *user_shared_data; + if ((status = NtFreeVirtualMemory( NtCurrentProcess(), &addr, &size, MEM_RELEASE ))) + { + ERR_(wineusd)( "Failed to release memory, status: %x\n", status ); + NtClose( section ); + return; + } + + addr = (void *)user_shared_data; + size = 0; + if ((status = NtMapViewOfSection( section, NtCurrentProcess(), &addr, 0, 0, 0, + &size, ViewShare, 0, PAGE_READWRITE ))) + { + WARN_(wineusd)( "wine: failed to map section, status: %x\n", status ); + NtClose( section ); + + if ((status = NtAllocateVirtualMemory( NtCurrentProcess(), &addr, 0, &size, + MEM_RESERVE|MEM_COMMIT, PAGE_READWRITE ))) + { + MESSAGE( "wine: failed to remap the user shared data: %08x\n", status ); + exit(1); + } + } + user_shared_data = addr; + *user_shared_data = tmp; + + RtlInitUnicodeString( &string, device_nameW ); + InitializeObjectAttributes( &attr, &string, 0, NULL, NULL ); + if ((status = NtCreateFile( &device, 0, &attr, &io, NULL, + FILE_ATTRIBUTE_NORMAL, 0, FILE_OPEN, + FILE_NON_DIRECTORY_FILE, NULL, 0 ))) + WARN_(wineusd)( "Failed to open device, status: %x\n", status ); +} /*********************************************************************** * thread_init -- 2.26.1
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=70421 Your paranoid android. === build (build log) === error: patch failed: configure.ac:3779 Task: Patch failed to apply === debiant (build log) === error: patch failed: configure.ac:3779 Task: Patch failed to apply === debiant (build log) === error: patch failed: configure.ac:3779 Task: Patch failed to apply
participants (3)
-
Marvin -
Paul Gofman -
Rémi Bernon