On Fri, Jun 26, 2015 at 1:28 PM, Olga Krishtal <[email protected]> wrote:
> On 25/06/15 15:17, Zavadovsky Yan wrote: > > On Thu, Jun 25, 2015 at 12:11 PM, Olga Krishtal <[email protected]> > <[email protected]> > wrote: > > > On 24/06/15 15:25, Zavadovsky Yan wrote: > > > 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. > More info about this technique can be found > here:http://blogs.msdn.com/b/oldnewthing/archive/2015/02/05/10591215.aspx > > This patch adds THREAD_GET_CONTEXT to OpenThread() arguments > and change oddity 'while(GetThreadContext() == SUCCESS)' to > 'if(GetThreadContext() == FAILED){exit(1);}'. > So this block of code will continue only after successful > grabbing of thread context(i.e. when thread is really suspended). > And halts otherwise with more verbose error message than previous. > Signed-off-by: Zavadovsky Yan <[email protected]> > <[email protected]> > --- > cpus.c | 14 ++++++++------ > util/qemu-thread-win32.c | 4 ++-- > 2 files changed, 10 insertions(+), 8 deletions(-) > > diff --git a/cpus.c b/cpus.c > index 4f0e54d..0df6a7d 100644 > --- a/cpus.c > +++ b/cpus.c > @@ -1089,8 +1089,8 @@ static void qemu_cpu_kick_thread(CPUState *cpu) > CONTEXT tcgContext; > if (SuspendThread(cpu->hThread) == (DWORD)-1) { > - fprintf(stderr, "qemu:%s: GetLastError:%lu\n", __func__, > - GetLastError()); > + fprintf(stderr, "qemu:%s: SuspendThread GetLastError:%lu\n", > + __func__, GetLastError()); > exit(1); > } > @@ -1098,15 +1098,17 @@ 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) { > - continue; > > > I would like to ask you if you have faced this situation in reality? > I have some doubts about changing the while() loop. According to the > article - > > > SuspendThread is async operation. But GetContext is not async. And > GetContext forces this pending suspension to complete. > > thread may mot be suspended immediately after code, due to the busy > > scheduler. > If we do exit(1) just right after check of GetThreadContext(..) we can > miss the situation when > scheduler is too busy at the moment and just go down. From this point of > view this while - > is busy loop and gives the scheduler the opportunity to do its job. > > If GetThreadContext fails in first call it will fail in next consecutive > calls for this thread handle. > > Afaik GetThreadContext can fail if the thread is in running state, it > means that > thread has not been suspended yet, for example it is running on other cpu > with hight irql. > Citation from http://blogs.msdn.com/b/oldnewthing/archive/2015/02/05/10591215.aspx : >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 GetÂThreadÂContext, 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. It's written by MS insider. Qemu maintainers treat him as reliable source - http://lists.nongnu.org/archive/html/qemu-devel/2015-06/msg05929.html Also you can compile sample from this article and run it. You will get "Huh? The variable lValue was modified by a suspended thread?" many times. But if you modify this sample in this manner: while (1) { memset(&Context, 0, sizeof(Context)); Context.ContextFlags = CONTEXT_CONTROL; if (SuspendThread(thread) == (DWORD)-1) abort(); if (GetThreadContext(thread, &Context) == 0) { printf("GetThreadContext failed\n"); } if (InterlockedOr(&lValue, 0) != InterlockedOr(&lValue, 0)) { printf("Huh? The variable lValue was modified by a suspended thread?\n"); } ResumeThread(thread); } Compile, run and you will see that "Huh? The variable lValue was modified by a suspended thread?" message will not shown anymore. > Instead of waiting we are going to go down. I agree that busy loop should > be somehow changed, > but going down every time. > > So, I am not sure about it. > > > + if (GetThreadContext(cpu->hThread, &tcgContext) == 0) { > + fprintf(stderr, "qemu:%s: GetThreadContext > GetLastError:%lu\n", > + __func__, GetLastError()); > + exit(1); > } > cpu_signal(0); > if (ResumeThread(cpu->hThread) == (DWORD)-1) { > - fprintf(stderr, "qemu:%s: GetLastError:%lu\n", __func__, > - GetLastError()); > + fprintf(stderr, "qemu:%s: ResumeThread GetLastError:%lu\n", > + __func__, GetLastError()); > exit(1); > } > } > 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); > > > What was before the usage of this flag? > > GetThreadContext returns zero, i.e. error. GetLastError says "5" i.e. > "access denied". > > I mean the behavior? As I can see it worked even without this flag. > > This code works because: > a)GetThreadContext works as Sleep(N) and scheduler have enough time to > finish async suspend. > or > b)GetThreadContext always forces suspension to complete before checking > handle's desired access and doing its own job. > or > c)something else, documentation on SuspendThread is weak. > > > > + handle = OpenThread(SYNCHRONIZE | THREAD_SUSPEND_RESUME | > > THREAD_GET_CONTEXT, > + FALSE, thread->tid); > } else { > handle = NULL; > } > > > >
