Re: [PATCHv2 2/7] xread: poll on non blocking fds

[Stefan Beller <sbeller@google.com>]

On Thu, Dec 17, 2015 at 12:42 PM, Torsten Bögershausen <tboegi@web.de> wrote:
> On 17.12.15 21:22, Stefan Beller wrote:
>> On Thu, Dec 17, 2015 at 12:12 PM, Torsten Bögershausen <tboegi@web.de> wrote:
>>> On 16.12.15 01:04, Stefan Beller wrote:
>>>> The man page of read(2) says:
>>>>
>>>>   EAGAIN The file descriptor fd refers to a file other than a socket
>>>>        and has been marked nonblocking (O_NONBLOCK), and the read
>>>>        would block.
>>>>
>>>>   EAGAIN or EWOULDBLOCK
>>>>        The file descriptor fd refers to a socket and has been marked
>>>>        nonblocking (O_NONBLOCK), and the read would block.  POSIX.1-2001
>>>>        allows either error to be returned for this case, and does not
>>>>        require these constants to have the same value, so a portable
>>>>        application should check for both possibilities.
>>>>
>>>> If we get an EAGAIN or EWOULDBLOCK the fd must have set O_NONBLOCK.
>>>> As the intent of xread is to read as much as possible either until the
>>>> fd is EOF or an actual error occurs, we can ease the feeder of the fd
>>>> by not spinning the whole time, but rather wait for it politely by not
>>>> busy waiting.
>>>>
>>>> We should not care if the call to poll failed, as we're in an infinite
>>>> loop and can only get out with the correct read().
>>> I'm not sure if this is valid under all circumstances:
>>> This is what "man poll" says under Linux:
>>> []
>>>  ENOMEM There was no space to allocate file descriptor tables.
>>> []
>>> And this is from Mac OS, ("BSD System Calls Manual")
>>> ERRORS
>>>      Poll() will fail if:
>>>
>>>      [EAGAIN]           Allocation of internal data structures fails.  A sub-
>>>                         sequent request may succeed.
>>> And this is opengroup:
>>> http://pubs.opengroup.org/onlinepubs/9699919799//functions/poll.html:
>>> [EAGAIN]
>>>     The allocation of internal data structures failed but a subsequent request may succeed.
>>>
>>> read() may return EAGAIN, but poll() may fail to allocate memory, and fail.
>>> Is it always guaranteed that the loop is terminated?
>>
>> In case poll fails (assume a no op for it), the logic should not have
>> changed by this patch?
>>
>> Looking closely:
>>
>>>>       while (1) {
>>>>               nr = read(fd, buf, len);
>>>> -             if ((nr < 0) && (errno == EAGAIN || errno == EINTR))
>>>> -                     continue;
>>>> +             if (nr < 0) {
>>>> +                     if (errno == EINTR)
>>>> +                             continue;
>>>> +                     if (errno == EAGAIN || errno == EWOULDBLOCK) {
>>>> +                             struct pollfd pfd;
>>>> +                             pfd.events = POLLIN;
>>>> +                             pfd.fd = fd;
>>>> +                             /*
>>>> +                              * it is OK if this poll() failed; we
>>>> +                              * want to leave this infinite loop
>>>> +                              * only when read() returns with
>>>> +                              * success, or an expected failure,
>>>> +                              * which would be checked by the next
>>>> +                              * call to read(2).
>>>> +                              */
>>>> +                             poll(&pfd, 1, -1);
>>
>> Or do you mean to insert another continue in here?
> I was thinking that we run into similar loop as before:
> read() returns -1; errno = EAGAIN  /* No data to read */

which is expected for non blocking fds,

> poll() returns -1; errno = EAGAIN /* poll failed. If the fd was OK, the failure may be temporaly,
>                                     as much as poll() can see.
>                                     But most probably we run out ouf memory */

Before this patch we would not have asked poll, but had just a continue here,
so I think we need to have it here again no matter of the return code
of the poll.

If poll determines it is low on memory, this should not make this function fail,
we can still do as good as we did before by just asking read
repeatedly again, though?

So I'd be convinced now we'd want to have:

    poll(&pfd, 1, -1); /* this is only buying time
                        for the fd to deliver data, in case it fails
                        we don't care but just fall back to old
                        behavior before this patch with busy spinning*/
    continue;


>
> So the code would look like this:
>
>    if (!poll(&pfd, 1, -1))
>       return -1;
>
>
>>
>
>>>> +                     }
>>>> +             }
>>>>               return nr;
>>>>       }
>>>>  }
>>>>
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe git" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>