Re: [Qemu-devel] [PATCH v3] glib: fix g_poll early timeout on windows

Re: [Qemu-devel] [PATCH v3] glib: fix g_poll early timeout on windows

[Stefan Hajnoczi]

On Fri, Apr 18, 2014 at 08:24:03PM +0400, Stanislav Vorobiov wrote:
> From: Sangho Park <sangho1206.park@samsung.com>
> 
> g_poll has a problem on Windows when using
> timeouts < 10ms, in glib/gpoll.c:
> 
> /* If not, and we have a significant timeout, poll again with
>  * timeout then. Note that this will return indication for only
>  * one event, or only for messages. We ignore timeouts less than
>  * ten milliseconds as they are mostly pointless on Windows, the
>  * MsgWaitForMultipleObjectsEx() call will timeout right away
>  * anyway.
>  */
> if (retval == 0 && (timeout == INFINITE || timeout >= 10))
>   retval = poll_rest (poll_msgs, handles, nhandles, fds, nfds, timeout);
> 
> so whenever g_poll is called with timeout < 10ms it does
> a quick poll instead of wait, this causes significant performance
> degradation of QEMU, thus we should use WaitForMultipleObjectsEx
> directly
> 
> Signed-off-by: Stanislav Vorobiov <s.vorobiov@samsung.com>
> ---
>  include/glib-compat.h |   19 +++++++++
>  include/qemu-common.h |   12 ------
>  util/oslib-win32.c    |  112 +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 131 insertions(+), 12 deletions(-)

What is the status of this patch?

I haven't followed the discussions around the issue but can review/merge
if there is agreement now.

Stefan

Re: [Qemu-devel] [PATCH v3] glib: fix g_poll early timeout on windows

[Paolo Bonzini]

Il 06/05/2014 14:23, Stefan Hajnoczi ha scritto:
>> > Signed-off-by: Stanislav Vorobiov <s.vorobiov@samsung.com>
>> > ---
>> >  include/glib-compat.h |   19 +++++++++
>> >  include/qemu-common.h |   12 ------
>> >  util/oslib-win32.c    |  112 +++++++++++++++++++++++++++++++++++++++++++++++++
>> >  3 files changed, 131 insertions(+), 12 deletions(-)
> What is the status of this patch?
>
> I haven't followed the discussions around the issue but can review/merge
> if there is agreement now.

The GNOME folks haven't followed up, so I think we should pick it.

Paolo

Re: [Qemu-devel] [PATCH v3] glib: fix g_poll early timeout on windows

[Alex Bligh]

On 6 May 2014, at 13:38, Paolo Bonzini wrote:

> Il 06/05/2014 14:23, Stefan Hajnoczi ha scritto:
>>> > Signed-off-by: Stanislav Vorobiov <s.vorobiov@samsung.com>
>>> > ---
>>> >  include/glib-compat.h |   19 +++++++++
>>> >  include/qemu-common.h |   12 ------
>>> >  util/oslib-win32.c    |  112 +++++++++++++++++++++++++++++++++++++++++++++++++
>>> >  3 files changed, 131 insertions(+), 12 deletions(-)
>> What is the status of this patch?
>> 
>> I haven't followed the discussions around the issue but can review/merge
>> if there is agreement now.
> 
> The GNOME folks haven't followed up, so I think we should pick it.

I haven't checked it on win32 but was happy with it otherwise.

-- 
Alex Bligh

Re: [Qemu-devel] [PATCH v3] glib: fix g_poll early timeout on windows

[Stefan Weil]

Am 06.05.2014 17:49, schrieb Alex Bligh:
> 
> On 6 May 2014, at 13:38, Paolo Bonzini wrote:
> 
>> Il 06/05/2014 14:23, Stefan Hajnoczi ha scritto:
>>>>> Signed-off-by: Stanislav Vorobiov <s.vorobiov@samsung.com>
>>>>> ---
>>>>>  include/glib-compat.h |   19 +++++++++
>>>>>  include/qemu-common.h |   12 ------
>>>>>  util/oslib-win32.c    |  112 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>  3 files changed, 131 insertions(+), 12 deletions(-)
>>> What is the status of this patch?
>>>
>>> I haven't followed the discussions around the issue but can review/merge
>>> if there is agreement now.
>>
>> The GNOME folks haven't followed up, so I think we should pick it.
> 
> I haven't checked it on win32 but was happy with it otherwise.


There was another patch on the list which moved g_poll code from
qemu-common.h to glib-compat.h, so when both patches get merged I expect
a (trivial) merge conflict.

The patch looks good. I already have applied it to my local queue, but I
won't be able to test it before end of next week. If someone wants to
pick it up earlier, I would not mind.

Cheers,
Stefan

Re: [Qemu-devel] [PATCH v3] glib: fix g_poll early timeout on windows

[Stefan Hajnoczi]

On Fri, Apr 18, 2014 at 08:24:03PM +0400, Stanislav Vorobiov wrote:

Please fix the following compiler warning with gcc 4.8.2:

> +        } else if ((res == WAIT_TIMEOUT) || (res == WAIT_IO_COMPLETION) ||
> +                   ((int)res < WAIT_OBJECT_0) ||
> +                   (res >= (WAIT_OBJECT_0 + nhandles))) {
> +            break;
> +        }

util/oslib-win32.c: In function 'g_poll_fixed':
util/oslib-win32.c:324:21: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
                    ((int)res < WAIT_OBJECT_0) ||
		                         ^

Re: [Qemu-devel] [PATCH v3] glib: fix g_poll early timeout on windows

[Stanislav Vorobiov]

Hi,

Hm, but (int)res expression is not unsigned, it's signed. I've also had this warning,
but with this expression: (res < WAIT_OBJECT_0), that's why I put (int) there. Could it be that
for some reason your compiler treats "int" and "unsigned int", that would be really strange though...

On 05/07/2014 12:02 PM, Stefan Hajnoczi wrote:
> On Fri, Apr 18, 2014 at 08:24:03PM +0400, Stanislav Vorobiov wrote:
> 
> Please fix the following compiler warning with gcc 4.8.2:
> 
>> +        } else if ((res == WAIT_TIMEOUT) || (res == WAIT_IO_COMPLETION) ||
>> +                   ((int)res < WAIT_OBJECT_0) ||
>> +                   (res >= (WAIT_OBJECT_0 + nhandles))) {
>> +            break;
>> +        }
> 
> util/oslib-win32.c: In function 'g_poll_fixed':
> util/oslib-win32.c:324:21: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
>                     ((int)res < WAIT_OBJECT_0) ||
> 		                         ^
> 

Re: [Qemu-devel] [PATCH v3] glib: fix g_poll early timeout on windows

[Alex Bligh]

On 7 May 2014, at 09:36, Stanislav Vorobiov wrote:

> Hi,
> 
> Hm, but (int)res expression is not unsigned, it's signed. I've also had this warning,
> but with this expression: (res < WAIT_OBJECT_0), that's why I put (int) there. Could it be that
> for some reason your compiler treats "int" and "unsigned int", that would be really strange though...

I suspect the problem is that WAIT_OBJECT_0 is defined as an unsigned long:

#define WAIT_OBJECT_0       ((STATUS_WAIT_0 ) + 0 )

#define STATUS_WAIT_0       ((DWORD)0x00000000L)

So IIRC under the 'usual conversions', and int compared with it will be cast to an unsigned long too.

I think you want to cast WAIT_OBJECT_0 to a long or similar.

Alex


> On 05/07/2014 12:02 PM, Stefan Hajnoczi wrote:
>> On Fri, Apr 18, 2014 at 08:24:03PM +0400, Stanislav Vorobiov wrote:
>> 
>> Please fix the following compiler warning with gcc 4.8.2:
>> 
>>> +        } else if ((res == WAIT_TIMEOUT) || (res == WAIT_IO_COMPLETION) ||
>>> +                   ((int)res < WAIT_OBJECT_0) ||
>>> +                   (res >= (WAIT_OBJECT_0 + nhandles))) {
>>> +            break;
>>> +        }
>> 
>> util/oslib-win32.c: In function 'g_poll_fixed':
>> util/oslib-win32.c:324:21: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
>>                    ((int)res < WAIT_OBJECT_0) ||
>> 		                         ^
>> 
> 
> 
> 

-- 
Alex Bligh

Re: [Qemu-devel] [PATCH v3] glib: fix g_poll early timeout on windows

[Stanislav Vorobiov]

Hi,

Yes it's probably the cause, thanks.

On 05/07/2014 12:49 PM, Alex Bligh wrote:
> 
> On 7 May 2014, at 09:36, Stanislav Vorobiov wrote:
> 
>> Hi,
>>
>> Hm, but (int)res expression is not unsigned, it's signed. I've also had this warning,
>> but with this expression: (res < WAIT_OBJECT_0), that's why I put (int) there. Could it be that
>> for some reason your compiler treats "int" and "unsigned int", that would be really strange though...
> 
> I suspect the problem is that WAIT_OBJECT_0 is defined as an unsigned long:
> 
> #define WAIT_OBJECT_0       ((STATUS_WAIT_0 ) + 0 )
> 
> #define STATUS_WAIT_0       ((DWORD)0x00000000L)
> 
> So IIRC under the 'usual conversions', and int compared with it will be cast to an unsigned long too.
> 
> I think you want to cast WAIT_OBJECT_0 to a long or similar.
> 
> Alex
> 
> 
>> On 05/07/2014 12:02 PM, Stefan Hajnoczi wrote:
>>> On Fri, Apr 18, 2014 at 08:24:03PM +0400, Stanislav Vorobiov wrote:
>>>
>>> Please fix the following compiler warning with gcc 4.8.2:
>>>
>>>> +        } else if ((res == WAIT_TIMEOUT) || (res == WAIT_IO_COMPLETION) ||
>>>> +                   ((int)res < WAIT_OBJECT_0) ||
>>>> +                   (res >= (WAIT_OBJECT_0 + nhandles))) {
>>>> +            break;
>>>> +        }
>>>
>>> util/oslib-win32.c: In function 'g_poll_fixed':
>>> util/oslib-win32.c:324:21: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
>>>                    ((int)res < WAIT_OBJECT_0) ||
>>> 		                         ^
>>>
>>
>>
>>
>