Re: [PATCH v2 1/1] ioctl_epoll.2: New page describing epoll ioctl(2)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Joe,

On Mon, Jun 10, 2024 at 11:12:06PM GMT, Joe Damato wrote:
> A new page is added which describes epoll fd ioctls: EPIOCSPARAMS and
> EPIOCGPARAMS which allow the user to control epoll-based busy polling.
> 
> Also add link pages for EPIOCSPARAMS and EPIOCGPARAMS.
> 
> Signed-off-by: Joe Damato <jdamato@xxxxxxxxxx>

Thanks!

> ---
>  man/man2/epoll_create.2           |   1 +
>  man/man2/epoll_ctl.2              |   1 +
>  man/man2/ioctl.2                  |   1 +
>  man/man2/ioctl_epoll.2            | 171 ++++++++++++++++++++++++++++++

I'm working on a general refactor of all ioctl manual pages, and I'm
making the pages have a name consistent with the UAPI header that
provides them.  You can see the progress here:
<https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/log/?h=ioctl>

For consistency, please rename the page as ioctl_eventpoll(2).

>  man/man2const/EPIOCGPARAMS.2const |   1 +
>  man/man2const/EPIOCSPARAMS.2const |   1 +
>  man/man7/epoll.7                  |   1 +
>  7 files changed, 177 insertions(+)
>  create mode 100644 man/man2/ioctl_epoll.2
>  create mode 100644 man/man2const/EPIOCGPARAMS.2const
>  create mode 100644 man/man2const/EPIOCSPARAMS.2const
> 
> diff --git a/man/man2/epoll_create.2 b/man/man2/epoll_create.2
> index f0327e8ba..2aa1745f5 100644
> --- a/man/man2/epoll_create.2
> +++ b/man/man2/epoll_create.2
> @@ -141,4 +141,5 @@ on overrun.
>  .BR close (2),
>  .BR epoll_ctl (2),
>  .BR epoll_wait (2),
> +.BR ioctl_epoll (2),
>  .BR epoll (7)
> diff --git a/man/man2/epoll_ctl.2 b/man/man2/epoll_ctl.2
> index 6d5bc032e..24bbe7405 100644
> --- a/man/man2/epoll_ctl.2
> +++ b/man/man2/epoll_ctl.2
> @@ -425,5 +425,6 @@ flag.
>  .SH SEE ALSO
>  .BR epoll_create (2),
>  .BR epoll_wait (2),
> +.BR ioctl_epoll (2),
>  .BR poll (2),
>  .BR epoll (7)
> diff --git a/man/man2/ioctl.2 b/man/man2/ioctl.2
> index 5b8c28a9c..d96777d1f 100644
> --- a/man/man2/ioctl.2
> +++ b/man/man2/ioctl.2
> @@ -225,6 +225,7 @@ for the various architectures.
>  .BR ioctl_ns (2),
>  .BR ioctl_tty (2),
>  .BR ioctl_userfaultfd (2),
> +.BR ioctl_epoll (2),
>  .BR open (2),
>  .\" .BR mt (4),
>  .BR sd (4),
> diff --git a/man/man2/ioctl_epoll.2 b/man/man2/ioctl_epoll.2
> new file mode 100644
> index 000000000..458e72e9a
> --- /dev/null
> +++ b/man/man2/ioctl_epoll.2
> @@ -0,0 +1,171 @@
> +.\" Copyright (c) 2024, Joe Damato

Please reformat as:

.\" Copyright 2024, Joe Damato <jdamato@xxxxxxxxxx>

(or another email if you want, but that's the format I'm trying to use
consistently.)

> +.\" Written by Joe Damato <jdamato@xxxxxxxxxx>

You can remove this line.

> +.\"
> +.\" SPDX-License-Identifier: Linux-man-pages-copyleft
> +.\"
> +.TH ioctl_epoll 2 (date) "Linux man-pages (unreleased)"
> +.SH NAME
> +ioctl_epoll \- ioctl() operations for epoll file descriptors

Please reformat as:

ioctl_eventpoll,
EPIOCSPARAMS,
EPIOCGPARAMS
\-
ioctl() operations for epoll file descriptors

(which has '\-' on a line of its own, and has the individual ops
listed.)

> +.SH LIBRARY
> +Standard C library
> +.RI ( libc ", " \-lc )
> +.SH SYNOPSIS
> +.EX
> +.BR "#include <linux/eventpoll.h>" "  /* Definition of " EPIOC* " constants and struct epoll_params */"

Remove ' and struct ...' from that comment.  We only have constants in
those comments (except in a few pages, where I'm fixing it at the
moment).

> +.B "#include <sys/ioctl.h>"
> +.P
> +.BI "int ioctl(int " fd ", EPIOCSPARAMS, const struct epoll_params *" argp );
> +.BI "int ioctl(int " fd ", EPIOCGPARAMS, struct epoll_params *" argp );

To document the header that provides this structure, let's add here:

.P
.B #include <linux/eventpoll.h>

> +.P
> +.B struct epoll_params {
> +.BR "    uint32_t busy_poll_usecs;" "  /* Number of usecs to busy poll */"
> +.BR "    uint16_t busy_poll_budget;" " /* Maximum number of packets to retrieve per poll */"
> +.BR "    uint8_t prefer_busy_poll;" "  /* Boolean to enable or disable prefer busy poll  */"
> +\&
> +.BR " " "   /* pad the struct to a multiple of 64bits */"
> +.BR "    uint8_t __pad;"            "  /* Must be zero */"
> +.B };
> +.EE
> +.SH DESCRIPTION
> +.TP
> +.B EPIOCSPARAMS
> +Set the
> +.I epoll_params
> +structure to configure the operation of epoll. Refer to the structure

Please use semantic newlines.  See man-pages(7):

$ MANWIDTH=72 man man-pages | sed -n '/Use semantic newlines/,/^$/p'
   Use semantic newlines
     In the source of a manual page, new sentences should be started on
     new lines, long sentences should be split  into  lines  at  clause
     breaks  (commas,  semicolons, colons, and so on), and long clauses
     should be split at phrase boundaries.  This convention,  sometimes
     known as "semantic newlines", makes it easier to see the effect of
     patches, which often operate at the level of individual sentences,
     clauses, or phrases.

> +description below to learn what configuration is
> +supported.
> +.TP
> +.B EPIOCGPARAMS
> +Get the current
> +.I epoll_params
> +configuration settings.
> +.TP
> +All
> +.BR ioctl (2)

We can omit 'ioctl(2)' here, since it's obvious from the context, I
think.  How about 'All operations documented ...'?

> +operations documented above must be performed on an epoll file descriptor,
> +which can be created with a call to

s/created/obtained/?

> +.BR epoll_create (2)
> +or
> +.BR epoll_create1 (2).
> +.\" kernel commit 18e2bf0edf4dd88d9656ec92395aa47392e85b61

Let's reformat these:

.\" linux.git 18e2bf0edf4dd88d9656ec92395aa47392e85b61
.\" glibc.git 92c270d32caf3f8d5a02b8e46c7ec5d9d0315158

(maybe say linux.git commit 1234...?  What do you prefer?)

> +.\" glibc commit 92c270d32caf3f8d5a02b8e46c7ec5d9d0315158
> +.P

I would use a subsection (.SS) for documenting the structure.

> +.I argp.busy_poll_usecs
> +field denotes the number of microseconds that the network stack will busy

s/field //?

> +poll. During this time period, the network device will be polled
> +repeatedly for packets. This value cannot exceed
> +.B INT_MAX.
> +.in
> +.P
> +.I argp.busy_poll_budget
> +field denotes the maximum number of packets that the network stack will

s/field //?

> +retrieve on each poll attempt. This value cannot exceed
> +.B NAPI_POLL_WEIGHT
> +which, as of Linux 6.9, is 64, unless the process is run with
> +.B CAP_NET_ADMIN.

This seems a bit ambiguous: 'unless the process is run with
CAP_NET_ADMIN' could refer to 'cannot exceed' or 'is 64'.  Using
parentheses instead of commas, it would be unambiguous.

> +.P
> +.I argp.prefer_busy_poll
> +field is a boolean field and must be either 0 (disabled) or 1 (enabled). If

s/field is/is/?

> +enabled, this indicates to the network stack that busy poll is the
> +preferred method of processing network data and the network stack should
> +give the application the opportunity to busy poll. Without this option,
> +very busy systems may continue to do network processing via the normal
> +method of IRQs triggering softIRQ and NAPI.
> +.P
> +.I argp.__pad
> +must be zero.
> +.P

.P is redundant right before .SH

> +.SH RETURN VALUE
> +On success, 0 is returned.
> +On failure, \-1 is returned, and
> +.I errno
> +is set to indicate the error.
> +.SH ERRORS
> +.TP
> +.B EOPNOTSUPP
> +The kernel was not compiled with busy poll support.
> +.TP
> +.B EINVAL
> +.I fd
> +is not a valid file descriptor.
> +.TP
> +.B EINVAL
> +.I op
> +specified is invalid.

Let's not document this one, since it's already documented in ioctl(2).

> +.TP
> +.B EINVAL
> +.I argp.__pad
> +was not zero.
> +.TP
> +.B EINVAL
> +.I argp.busy_poll_usecs
> +exceeds

There's a bit of an inconsistency: the previous entry uses the past
tense, but this one uses the present.  I prefer to use the present in
both.  See also the next one.

> +.B INT_MAX .
> +.TP
> +.B EINVAL
> +.I argp.prefer_busy_poll
> +is not 0 or 1.
> +.TP
> +.B EPERM
> +The process is being run without
> +.I CAP_NET_ADMIN
> +and the specified
> +.I argp.busy_poll_budget
> +exceeds
> +.B NAPI_POLL_WEIGHT
> +(which is 64 as of Linux 6.9).

I prefer to not repeat the 64 here.

> +.TP
> +.B EFAULT
> +.I argp
> +does not point to a valid memory address.
> +.SH EXAMPLES
> +.EX
> +/* Code to set the epoll params to enable busy polling */
> +\&
> +int epollfd = epoll_create1(0);
> +struct epoll_params params;
> +\&
> +if (epollfd == \-1) {
> +    perror("epoll_create1");
> +    exit(EXIT_FAILURE);
> +}
> +\&
> +memset(&params, 0, sizeof(struct epoll_params));
> +\&
> +params.busy_poll_usecs = 25;
> +params.busy_poll_budget = 8;
> +params.prefer_busy_poll = 1;
> +\&
> +if (ioctl(epollfd, EPIOCSPARAMS, &params) == \-1) {
> +    perror("ioctl");
> +    exit(EXIT_FAILURE);
> +}
> +\&
> +/* Code to show how to retrieve the current settings */
> +\&
> +memset(&params, 0, sizeof(struct epoll_params));
> +\&
> +if (ioctl(epollfd, EPIOCGPARAMS, &params) == \-1) {
> +    perror("ioctl");
> +    exit(EXIT_FAILURE);
> +}
> +\&
> +/* params struct now contains the current parameters */
> +\&
> +fprintf(stderr, "epoll usecs: %lu\\n", params.busy_poll_usecs);

We use '\e', not '\\'.  (I haven't checked whether it also works, and
don't remember.)

> +fprintf(stderr, "epoll packet budget: %u\\n", params.busy_poll_budget);
> +fprintf(stderr, "epoll prefer busy poll: %u\\n", params.prefer_busy_poll);
> +\&
> +.SH History
> +Linux 6.9, glibc 2.40.

Let's reformat this as:

Linux 6.9.
glibc 2.40.

> +.SH SEE ALSO
> +.BR ioctl (2),
> +.BR epoll_create (2),
> +.BR epoll_create1 (2),
> +.BR epoll (7)
> +.P
> +.I linux.git/Documentation/networking/napi.rst
> +.P
> +and
> +.P

I think we can remove the 'and'.

> +.I linux.git/Documentation/admin-guide/sysctl/net.rst
> diff --git a/man/man2const/EPIOCGPARAMS.2const b/man/man2const/EPIOCGPARAMS.2const
> new file mode 100644
> index 000000000..6fbc5f0f8
> --- /dev/null
> +++ b/man/man2const/EPIOCGPARAMS.2const
> @@ -0,0 +1 @@
> +.so man2/ioctl_epoll.2
> diff --git a/man/man2const/EPIOCSPARAMS.2const b/man/man2const/EPIOCSPARAMS.2const
> new file mode 100644
> index 000000000..6fbc5f0f8
> --- /dev/null
> +++ b/man/man2const/EPIOCSPARAMS.2const
> @@ -0,0 +1 @@
> +.so man2/ioctl_epoll.2
> diff --git a/man/man7/epoll.7 b/man/man7/epoll.7
> index e7892922e..4ad032bdd 100644
> --- a/man/man7/epoll.7
> +++ b/man/man7/epoll.7
> @@ -606,5 +606,6 @@ is present in an epoll instance.
>  .BR epoll_create1 (2),
>  .BR epoll_ctl (2),
>  .BR epoll_wait (2),
> +.BR ioctl_epoll (2),
>  .BR poll (2),
>  .BR select (2)
> -- 
> 2.34.1

Have a lovely night!
Alex

-- 
<https://www.alejandro-colomar.es/>

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [Kernel Documentation]     [Netdev]     [Linux Ethernet Bridging]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]

  Powered by Linux