Fwd: Re: [PATCH 2/2] Quickly retry sendmsg or recvmsg if errno is EAGAIN or EINTR.

[Max TenEyck Woodbury]

-------- Original Message --------
Subject: Re: [PATCH 2/2] Quickly retry sendmsg or recvmsg if errno is 
EAGAIN or EINTR.
Date: Wed, 26 Feb 2014 13:33:22 -0500
From: Max Woodbury <mtewoodbury at gmail.com>
To: Ken Thomases <ken at codeweavers.com>

On 02/23/2014 05:23 AM, Ken Thomases wrote:
> On Feb 21, 2014, at 6:35 PM, mtewoodbury at gmail.com wrote:
>
>> Note that this greatly improves network reliability.
>
> How did you determine that?  How could we confirm that?

Feature of Guild Wars 2 (trading post) that failed to work on wine now
works.  Lower Game latency.  Fewer login failures.

>
>
>> -    if ( (n = recvmsg(fd, &hdr, wsa->flags)) == -1 )
>> +    n = recvmsg(fd, &hdr, wsa->flags);
>> +    if (n == -1 && (errno == EINTR || errno == EAGAIN))
>> +        n = recvmsg(fd, &hdr, wsa->flags);
>> +    if (n == -1)
>> +
>>          return -1;
>
> What's with that blank line between the last "if" and its corresponding statement?

To emphasize the exit from the function.  I find that I often want to
know where such exits are and this makes them easier to fined.
Personal preference...

>
>> -    if (ret >= 0)
>> -    {
>> +    if (ret == -1 && (errno == EINTR || errno == EAGAIN))
>> +        ret = sendmsg(fd, &hdr, wsa->flags);                              /* Try it again quickly */
>> +    if (ret >= 0) {
>
> Why did you change the existing lines?  You could have just added your two lines.

Mostly just personal preference...

>
> In general:
>
> Is this actually the second patch of a two-patch series as indicated by the "[PATCH 2/2]"
> in the subject?  I didn't receive the first patch.

No, my dyslexia strikes again.
>
> Have you tested to see if just retrying on EINTR is sufficient to improve the
> reliability?  EAGAIN is not typically a situation where an immediate
retry helps.

EAGAIN says specifically resources _temporarily_ unavailable.  How
temporary is not specified, so one quick retry could fix the problem.
>
> The callers of WS2_recv() and WS2_send() already have tests for EINTR and EAGAIN.
> The strategy for dealing with those should probably exist in just one
place.
> For my part, I think that all interruptible syscalls should be in a
loop that
> immediately retries for EINTR, but EAGAIN should usually go back to some select(),
> poll(), kevent() or similar wait function.

I'd more or less agree except that there is, if I recall correctly, some
other stuff that has to happen before the retry at the higher levels
and I was getting game feature failures and latency waiting for those
fixes.  By all means, something like this could and should be done for
most system calls.  This one just annoyed me enough to try fiddling
with it.  Since it seemed to make a difference, I posted it...
>
> -Ken