Re: [Qemu-devel] [PATCH 2/2] qemu-timer: move timeBeginPeriod/timeEndPeriod to os-win32

Re: [Qemu-devel] [PATCH 2/2] qemu-timer: move timeBeginPeriod/timeEndPeriod to os-win32

[Paolo Bonzini]

Il 20/02/2013 14:43, Paolo Bonzini ha scritto:
> These are needed for any of the Win32 alarm timer implementations.
> They are not tied to mmtimer exclusively.
> 
> Jacob tested this patch with both mmtimer and Win32 timers.
> 
> Cc: qemu-stable@nongnu.org
> Tested-by: Jacob Kroon <jacob.kroon@gmail.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>         Jacob, this is the same as "patch 1" you tested.

Stefan, can you pick up this patch again, together with Fabien's series?

Paolo

>  os-win32.c   | 10 ++++++++++
>  qemu-timer.c | 26 ++++++--------------------
>  2 files changed, 16 insertions(+), 20 deletions(-)
> 
> diff --git a/os-win32.c b/os-win32.c
> index 3d43604..a0740ef 100644
> --- a/os-win32.c
> +++ b/os-win32.c
> @@ -23,6 +23,7 @@
>   * THE SOFTWARE.
>   */
>  #include <windows.h>
> +#include <mmsystem.h>
>  #include <unistd.h>
>  #include <fcntl.h>
>  #include <signal.h>
> @@ -67,9 +67,19 @@ static BOOL WINAPI qemu_ctrl_handler(DWORD type)
>      return TRUE;
>  }
>  
> +static TIMECAPS mm_tc;
> +
> +static void os_undo_timer_resolution(void)
> +{
> +    timeEndPeriod(mm_tc.wPeriodMin);
> +}
> +
>  void os_setup_early_signal_handling(void)
>  {
>      SetConsoleCtrlHandler(qemu_ctrl_handler, TRUE);
> +    timeGetDevCaps(&mm_tc, sizeof(mm_tc));
> +    timeBeginPeriod(mm_tc.wPeriodMin);
> +    atexit(os_undo_timer_resolution);
>  }
>  
>  /* Look for support files in the same directory as the executable.  */
> diff --git a/qemu-timer.c b/qemu-timer.c
> index 8fb5c75..50109a1 100644
> --- a/qemu-timer.c
> +++ b/qemu-timer.c
> @@ -623,29 +622,15 @@ static void CALLBACK mm_alarm_handler(UINT uTimerID, UINT uMsg,
>  
>  static int mm_start_timer(struct qemu_alarm_timer *t)
>  {
>      timeGetDevCaps(&mm_tc, sizeof(mm_tc));
> -
> -    timeBeginPeriod(mm_tc.wPeriodMin);
> -
> -    mm_timer = timeSetEvent(mm_tc.wPeriodMin,   /* interval (ms) */
> -                            mm_tc.wPeriodMin,   /* resolution */
> -                            mm_alarm_handler,   /* function */
> -                            (DWORD_PTR)t,       /* parameter */
> -                            TIME_ONESHOT | TIME_CALLBACK_FUNCTION);
> -
> -    if (!mm_timer) {
> -        fprintf(stderr, "Failed to initialize win32 alarm timer\n");
> -        timeEndPeriod(mm_tc.wPeriodMin);
> -        return -1;
> -    }
> -
>      return 0;
>  }
>  
>  static void mm_stop_timer(struct qemu_alarm_timer *t)
>  {
> -    timeKillEvent(mm_timer);
> -    timeEndPeriod(mm_tc.wPeriodMin);
> +    if (mm_timer) {
> +        timeKillEvent(mm_timer);
> +    }
>  }
>  
>  static void mm_rearm_timer(struct qemu_alarm_timer *t, int64_t delta)
> @@ -657,7 +641,9 @@ static void mm_rearm_timer(struct qemu_alarm_timer *t, int64_t delta)
>          nearest_delta_ms = mm_tc.wPeriodMax;
>      }
>  
> -    timeKillEvent(mm_timer);
> +    if (mm_timer) {
> +        timeKillEvent(mm_timer);
> +    }
>      mm_timer = timeSetEvent((UINT)nearest_delta_ms,
>                              mm_tc.wPeriodMin,
>                              mm_alarm_handler,
> 

Re: [Qemu-devel] [PATCH 2/2] qemu-timer: move timeBeginPeriod/timeEndPeriod to os-win32

[Stefan Weil]

Am 09.04.2013 18:21, schrieb Paolo Bonzini:
> Il 20/02/2013 14:43, Paolo Bonzini ha scritto:
>> These are needed for any of the Win32 alarm timer implementations.
>> They are not tied to mmtimer exclusively.
>>
>> Jacob tested this patch with both mmtimer and Win32 timers.
>>
>> Cc: qemu-stable@nongnu.org
>> Tested-by: Jacob Kroon <jacob.kroon@gmail.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>         Jacob, this is the same as "patch 1" you tested.
> Stefan, can you pick up this patch again, together with Fabien's series?
>
> Paolo


Hi Paolo,

I plan sending a pull request on Friday or Saturday for those patches.
Richard, the same applies to your TCI series (which I did not forget).

Cheers,
Stefan

Re: [Qemu-devel] [PATCH 1/2] win32: do not set CPU affinity

[Fabien Chouteau]

Looks like I missed an important discussion once again :) In the future
don't hesitate to put me in copy for Windows and or IO loop subjects.

On 02/21/2013 09:13 AM, Roy Tam wrote:
> 2013/2/21 Roy Tam <roytam@gmail.com>:
>> 2013/2/21 Paolo Bonzini <pbonzini@redhat.com>:
>>> Il 21/02/2013 01:35, Roy Tam ha scritto:
>>>>>> QEMU system emulation has been thread-safe for a long time, and
>>>>>> setting the CPU affinity is hurting performance badly.  Remove
>>>>>> the bogus code.

Actually it was not thread-safe on Windows because we don't have
signals. On Linux, cpu_signal() is executed in the TCG thread context,
on Windows, it's executed in IO thread context. This leads to memory
synchronization issues.

>>>>>>
>>>>>> Jacob Kroon reports that the time to boot his VxWorks image goes from
>>>>>> "3 minutes passed and I still haven't made it that far" to ~140s.
>>>>>>
>>>> Yes it does work better for fdostest.img, but when I tried to boot
>>>> ttylinux-i686-14.0.iso, qemu freezes after showing "loading vmlinuz
>>>> ..."
>>>>
>>>
>>> Did it work without the patch?
>>
>> Yes, at least linux kernel starts but stalls when hitting IO-APIC
>> thingy, with noapic boot switch it stalls after detecting ttyS0
>>
>> With patch, QEMU is unresponsive when loading vmlinuz (not even
>> showing "Decompressing Linux" message)
>
> Clarify that it boots fine sometimes, but when QEMU freezes, there is
> a thread busy-waiting(seems so):
> http://i.imgur.com/LUJjd8r.png
>

As I said in the patch set submitted yesterday, we've be through this
kind of problem already. And it was not easy to debug (to say the
least).

The freezes that your are observing is due to a bad memory
synchronization, between exit_request (global) and cpu->exit_request.
This is fixed by the second patch of my set "Ensure good ordering of
memory instruction in cpu_exec".

-- 
Fabien Chouteau