Re: [PATCH RFC v2 0/7] epoll: Introduce new syscalls, epoll_ctl_batch and epoll_pwait1

[Michael Kerrisk (man-pages)]

Hello Fam Zheng,

On 02/04/2015 11:36 AM, Fam Zheng wrote:
> Changes from v1 (https://lkml.org/lkml/2015/1/20/189):
> 
>   - As discussed in previous thread [1], split the call to epoll_ctl_batch and
>     epoll_pwait. [Michael]
> 
>   - Fix memory leaks. [Omar]
> 
>   - Add a short comment about the ignored copy_to_user failure. [Omar]
> 
>   - Cover letter rewritten.
> 
> This adds two new system calls as described below. I mentioned glibc wrapping
> of sigarg in epoll_pwait1 description, so don't read it as end user man page
> yet.

Fair enough. But I think it would be helpful to say a little more already.
See my comment below.

> One caveat is the possible failure of the last copy_to_user in epoll_ctl_batch,
> which returns per command error code. Ideas to improve that are welcome!
>
> 1) epoll_ctl_batch
> ------------------
> 
> NAME
>        epoll_ctl_batch - modify an epoll descriptor in batch
> 
> SYNOPSIS
> 
>        #include <sys/epoll.h>
> 
>        int epoll_ctl_batch(int epfd, int flags,
>                            int ncmds, struct epoll_ctl_cmd *cmds);
> 
> DESCRIPTION
> 
>        The system call performs a batch of epoll_ctl operations. It allows
>        efficient update of events on this epoll descriptor.
> 
>        Flags is reserved and must be 0.
> 
>        Each operation is specified as an element in the cmds array, defined as:
> 
>            struct epoll_ctl_cmd {
> 
>                   /* Reserved flags for future extension, must be 0. */
>                   int flags;
> 
>                   /* The same as epoll_ctl() op parameter. */
>                   int op;
> 
>                   /* The same as epoll_ctl() fd parameter. */
>                   int fd;
> 
>                   /* The same as the "events" field in struct epoll_event. */
>                   uint32_t events;
> 
>                   /* The same as the "data" field in struct epoll_event. */
>                   uint64_t data;
> 
>                   /* Output field, will be set to the return code after this
>                    * command is executed by kernel */
>                   int error_hint;

Why 'error_hint', rather than just stay 'status' or 'result'? I mean
is it really a "hint"? Also, it can be 0 meaning "success" (no error).

>            };
> 
>        Commands are executed in their order in cmds, and if one of them failed,
>        the rest after it are not tried.
> 
>        Not that this call isn't atomic in terms of updating the epoll
>        descriptor, which means a second epoll_ctl or epoll_ctl_batch call
>        during the first epoll_ctl_batch may make the operation sequence
>        interleaved. However, each single epoll_ctl_cmd operation has the same
>        semantics as a epoll_ctl call.

(Good to include that warning!)

> RETURN VALUE
> 
>        If one or more of the parameters are incorrect, -1 is returned and errno
>        is set appropriately. Otherwise, the number of succeeded commands is
>        returned.
> 
>        Each error_hint field may be set to the error code or 0, depending on
>        the result of the command. If there is some error in returning the error
>        to user, it may also be unchanged, even though the command may actually
>        be executed. In this case, it's still ensured that the i-th (i = ret)
>        command is the failed command.

Sorry -- I'm not clear here. Can you say some more here? What sort
of error might there be when "returning the error to the user"?
 
> ERRORS
> 
>        Errors for the overall system call (in errno) are:
> 
>        EINVAL flags is non-zero, or ncmds is less than or equal to zero, or
>               cmds is NULL.
> 
>        ENOMEM There was insufficient memory to handle the requested op control
>               operation.
> 
>        EFAULT The memory area pointed to by cmds is not accessible with write
>               permissions.
> 
> 
>        Errors for each command (in error_hint) are:
> 
>        EBADF  epfd or fd is not a valid file descriptor.
> 
>        EEXIST op was EPOLL_CTL_ADD, and the supplied file descriptor fd is
>               already registered with this epoll instance.
> 
>        EINVAL epfd is not an epoll file descriptor, or fd is the same as epfd,
>               or the requested operation op is not supported by this interface.
> 
>        ENOENT op was EPOLL_CTL_MOD or EPOLL_CTL_DEL, and fd is not registered
>               with this epoll instance.
> 
>        ENOMEM There was insufficient memory to handle the requested op control
>               operation.
> 
>        ENOSPC The limit imposed by /proc/sys/fs/epoll/max_user_watches was
>               encountered while trying to register (EPOLL_CTL_ADD) a new file
>               descriptor on an epoll instance.  See epoll(7) for further
>               details.
> 
>        EPERM  The target file fd does not support epoll.
> 
> CONFORMING TO
> 
>        epoll_ctl_batch() is Linux-specific.
> 
> SEE ALSO
> 
>        epoll_create(2), epoll_ctl(2), epoll_wait(2), epoll_pwait(2), epoll(7)
> 
> 
> 2) epoll_pwait1
> ---------------
> 
> NAME
>        epoll_pwait1 - wait for an I/O event on an epoll file descriptor
> 
> SYNOPSIS
> 
>        #include <sys/epoll.h>
> 
>        int epoll_pwait1(int epfd, int flags,
>                         struct epoll_event *events,
>                         int maxevents,
>                         struct timespec *timeout,
>                         struct sigargs *sig);
> 
> DESCRIPTION
> 
>        The epoll_pwait1 system call differs from epoll_pwait only in parameter
>        types. The first difference is timeout, a pointer to timespec structure
>        which allows nanosecond presicion; the second difference, which should
>        probably be wrapper by glibc and only expose a sigset_t pointer as in
>        pselect6.

Here I think it still helps to explain that 'struct sigags' is a sigset_t* +
the size of the pointed-to set.

>        If timeout is NULL, it's treated as if 0 is specified in epoll_pwait

The convention I would expect here is that NULL means infinite timeout,
like select(), and timeout == {0,0} would get the "return immediately"
behavior. Why did you choose your convention? And, how does one otherwise 
request an infinite timeout?

>        (return immediately). Otherwise it's converted to nanosecond scalar,
>        again, with the same convention as epoll_pwait's timeout.
> 
> RETURN VALUE
> 
>        The same as said in epoll_pwait(2).
> 
> ERRORS
> 
>        The same as said in man epoll_pwait(2), plus:
> 
>        EINVAL flags is not zero.
> 
> 
> CONFORMING TO
> 
>        epoll_pwait1() is Linux-specific.
> 
> SEE ALSO
> 
>        epoll_create(2), epoll_ctl(2), epoll_wait(2), epoll_pwait(2), epoll(7)

In your earlier patch set, there was the ability to select the clock
used for timeouts. Why did this go away? I'm not sure whether we need
that functionality or not, but it would be good to know why you
dropped it this time.

Thanks,

Michael



-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/