Hello.
On Tue, Jun 23, 2015 at 9:02 AM, Stefan Weil <s...@weilnetz.de> wrote:
> Am 22.06.2015 um 23:54 schrieb Zavadovsky Yan:
>
>> Calling SuspendThread() is not enough to suspend Win32 thread.
>> We need to call GetThreadContext() after SuspendThread()
>> to make sure that OS have really suspended target thread.
>> But GetThreadContext() needs for THREAD_GET_CONTEXT
>> access right on thread object.
>>
>> This patch adds THREAD_GET_CONTEXT to OpenThread() arguments
>> and change 'while(GetThreadContext() == SUCCESS)' to
>> 'while(GetThreadContext() == FAILED)'.
>> So this 'while' loop will stop only after successful grabbing
>> of thread context(i.e. when thread is really suspended).
>> Not after the one failed GetThreadContext() call.
>>
>> Signed-off-by: Zavadovsky Yan <zavadovsky....@gmail.com>
>> ---
>> cpus.c | 2 +-
>> util/qemu-thread-win32.c | 4 ++--
>> 2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/cpus.c b/cpus.c
>> index b85fb5f..83d5eb5 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -1097,7 +1097,7 @@ static void qemu_cpu_kick_thread(CPUState *cpu)
>> * suspended until we can get the context.
>> */
>> tcgContext.ContextFlags = CONTEXT_CONTROL;
>> - while (GetThreadContext(cpu->hThread, &tcgContext) != 0) {
>> + while (GetThreadContext(cpu->hThread, &tcgContext) == 0) {
>> continue;
>> }
>> diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
>> index 406b52f..823eca1 100644
>> --- a/util/qemu-thread-win32.c
>> +++ b/util/qemu-thread-win32.c
>> @@ -406,8 +406,8 @@ HANDLE qemu_thread_get_handle(QemuThread *thread)
>> EnterCriticalSection(&data->cs);
>> if (!data->exited) {
>> - handle = OpenThread(SYNCHRONIZE | THREAD_SUSPEND_RESUME, FALSE,
>> - thread->tid);
>> + handle = OpenThread(SYNCHRONIZE | THREAD_SUSPEND_RESUME |
>> THREAD_GET_CONTEXT,
>> + FALSE, thread->tid);
>> } else {
>> handle = NULL;
>> }
>>
>
>
> I added the contributers of the original code to the cc list.
>
> The modifications look reasonable - if GetThreadContext is needed at all.
>
GetThreadContext is needed to avoid races on multicore system. As it wrote
in comment from original contributors of this code.
We should add an URL to reliable documentation which supports that
> claim.
>
Unfortunately, MSDN says only "SuspendThread suspends the thread. It's
designed for debuggers. Don't use in applications.":
https://msdn.microsoft.com/en-us/library/windows/desktop/ms686345(v=vs.85).aspx
And nothing more useful.
So when I found this piece of code with Suspend/Resume and failed
GetContext I did some googling.
And found this article:
http://blogs.msdn.com/b/oldnewthing/archive/2015/02/05/10591215.aspx
In which:
>The SuspendThread function tells the scheduler to suspend the thread but
does not wait for an acknowledgment from the scheduler that the suspension
has actually occurred
>If you want to make sure the thread really is suspended, you need to
perform a synchronous operation that is dependent on the fact that the
thread is suspended
>The traditional way of doing this is to call GetThreadContext, since
this requires the kernel to read from the context of the suspended thread,
which has as a prerequisite that the context be saved in the first place,
which has as a prerequisite that the thread be suspended
> Is it a good idea to run a busy waiting loop?
I think no. Because infinite loop is possible. If someone make regress in
future.
Maybe better use same handling as Suspend/Resume uses?
'if (GetThreadContext() == FAILED) { print_error; error_exit; }'
GetThreadContext either works or not. So if it fails first call it will
fail all next calls.
> Or would a Sleep(0)
No. Sleep is "hardcode" without any guarantees.
