Hi!

On Thu, Jun 06, 2024 at 07:06:05PM GMT, Joe Damato wrote:

On Thu, Jun 06, 2024 at 11:39:58PM +0200, Alejandro Colomar wrote:

quoted

Hi Joe,

On Tue, Jun 04, 2024 at 06:17:40PM GMT, Joe Damato wrote:

quoted

Signed-off-by: Joe Damato <redacted>

Thanks for the patch!  Please see a few comments below.

I've included some questions below just to make sure I give you a v2
that is much closer to correct :)

Nice :)

quoted

quoted

+.TH ioctl_epoll 2 (date) "Linux man-pages (unreleased)"
+.SH NAME
+ioctl_epoll \- ioctl() operations for epoll file descriptors

Please add link pages <man2const/EPIOCSPARAMS.2const> and
<man2const/EPIOCGPARAMS.2const>.

And after adding those, I'd add those to SEE ALSO and I'd omit the
description of them from the ioctl_epoll.2 page (because they'd have
their own pages) ?

Nope.  The manual page would still be <ioctl_epoll.2>.  The other two
would be link pages, that is, if you `man EPIOCSPARAMS`, you'd get the
manual page ioctl_epoll(2).

See for example the manual page slist(3):

	$ head man/man3/slist.3
	.\" Copyright (c) 1993
	.\"    The Regents of the University of California.  All rights reserved.
	.\" and Copyright (c) 2020 by Alejandro Colomar [off-list ref]
	.\"
	.\" SPDX-License-Identifier: BSD-3-Clause
	.\"
	.\"
	.TH SLIST 3 (date) "Linux man-pages (unreleased)"
	.SH NAME
	SLIST_EMPTY,

It has several aliases, one for each macro that it documents.  Here are
a few examples:

	$ head man/man3/SLIST_EMPTY.3
	.so man3/slist.3
	$ head man/man3/SLIST_FOREACH.3 
	.so man3/slist.3

That .so roff(7) request is similar to a C #include, so that they're
actually the same page.  You can check in your terminal, with
`man slist` and `man SLIST_FOREACH`, which will give you the same page.

So, basically, I want you to write these:

	$ cat man/man2const/EPIOCSPARAMS.2const
	.so man2/ioctl_epoll.2
	$ cat man/man2const/EPIOCGPARAMS.2const
	.so man2/ioctl_epoll.2

Nothing else is required.

quoted

quoted

+In the above,
+.I fd
+is a file descriptor referring to an epoll file descriptor obtained with a
+call to
+.BR epoll_create (2).
+.I op
+is one of the operations listed below, and
+.I argp
+is a pointer to the data structure described below.

If argp is a pointer to a structure, then the prototype should document
that:

.BI "int ioctl(int " fd ", int " op ", struct epoll_params *" argp );

Also, I would document the two operations separately:

.BI "int ioctl(int " fd ", EPIOCSPARAMS, const struct epoll_params *" argp );
.BI "int ioctl(int " fd ", EPIOCGPARAMS, struct epoll_params *" argp );

This allows documenting that the 'S' one doesn't modify the argp
(AFAICS).

Do you mean that I should omit the generic 

 .BI "int ioctl(int " fd ", int " op ", struct epoll_params *" argp );

and instead have the above with the two ops?

Exactly.

As in for example, PR_SET_MM_START_DATA.2const.  You can have a look at
that page in the repository, because it hasn't yet been released.

I'll paste here a copy of it, since it's short:

$ MANWIDTH=72 man PR_SET_MM_START_DATA | cat
PR_SET_MM_START_DATA(2const)               PR_SET_MM_START_DATA(2const)

NAME
     PR_SET_MM_START_DATA,  PR_SET_MM_END_DATA  -  modify kernel memory
     map descriptor fields

LIBRARY
     Standard C library (libc, -lc)

SYNOPSIS
     #include <linux/prctl.h>  /* Definition of PR_* constants */
     #include <sys/prctl.h>

     int prctl(PR_SET_MM, PR_SET_MM_START_DATA, unsigned long addr, 0L, 0L);
     int prctl(PR_SET_MM, PR_SET_MM_END_DATA, unsigned long addr, 0L, 0L);

DESCRIPTION
     PR_SET_MM_START_DATA
            Set the address above which initialized  and  uninitialized
            (bss)  data are placed.  The corresponding memory area must
            be readable and writable, but not executable or shareable.

     PR_SET_MM_END_DATA
            Set the address below which initialized  and  uninitialized
            (bss)  data are placed.  The corresponding memory area must
            be readable and writable, but not executable or shareable.

RETURN VALUE
     On success, 0 is returned.  On error, -1 is returned, and errno is
     set to indicate the error.

ERRORS
     EINVAL
            addr is greater than TASK_SIZE (the limit on  the  size  of
            the user address space for this architecture).

     EINVAL
            The permissions of the corresponding memory area are not as
            required.

STANDARDS
     Linux.

HISTORY
     Linux 3.3.

SEE ALSO
     prctl(2)

Linux man‐pages 6.8‐151‐g58... 2024‐06‐01  PR_SET_MM_START_DATA(2const)


BTW, now I realize that page is missing a reference to PR_SET_MM(2const)
in the SEE ALSO section.  I'll fix that in a moment.

quoted

quoted

+.\"
+.P
+All supported
+.I op
+values (described below) use an
+.I argp
+argument which is a pointer to a
+.I epoll_params
+structure, defined as:
+.P
+.in +4n
+.EX
+struct epoll_params {
+    uint32_t busy_poll_usecs;   /* Number of usecs to busy poll */
+    uint16_t busy_poll_budget;  /* Maximum number of packets to retrieve per poll */
+    uint8_t prefer_busy_poll;   /* Boolean to enable or disable prefer busy poll  */
+
+    /* pad the struct to a multiple of 64bits */
+    uint8_t __pad;              /* Must be zero */
+};

You could add this type definition to the SYNOPSIS, below the function
prototypes.

OK, I moved it.

I'm not sure if it is formatted properly, though. I'll see if I can
find other examples of this style to check against.

See timespec(3type) or sockaddr(3type) for examples of pages documenting
structures in the SYNOPSIS.

quoted

quoted

+The specified
+.I op
+is invalid.
+.TP
+.B EINVAL
+The
+.I fd
+specified is not an epoll file descriptor, or the
+.I op
+specified is invalid, or the
+.I __pad
+was not initialized to zero, or
+.I busy_poll_usecs
+exceeds
+.B INT_MAX ,
+or
+.I prefer_busy_poll
+is not 0 or 1.

Please separate the different conditions for EINVAL into separate
entries.  The ones that are related can go in the same one, but the
unrelated ones are better split.

Thanks!

I did something like this:

.TP
.B EINVAL
The
.I fd
specified is not an epoll file descriptor.

Please use the same exact wording as in ioctl(2), for consistency.

.TP
.B EINVAL
The
.I op
specified is invalid.

I would remove this one, since we're documenting that the calls be done
with specific operations.  There's no 'op' variable.  The variable 'op'
error is already documented in ioctl(2).

.TP
.B EINVAL
The

I would remove the above 'The'.

.I argp.__pad
was not initialized to zero.

Maybe remove 'initialized to', to abbreviate.  If it's not zero, it's
of course because it wasn't initialized to zero.  :)

is that what you meant?

Yup.

quoted

quoted

+(which is 64 as of Linux 6.9).
+.TP
+.B EFAULT
+.I argp
+does not point to a valid memory address.
+.SH EXAMPLES
+.EX

Could you write an entire program, with a main()?

If not, it's fine; this is better than nothing.  But we prefer having
entire programs that users can paste and try.

Hmm.. I can give it a try! It's a bit tricky because to actually use
busy polling, you need to have a program that accepts incoming
connections and calls epoll_wait (after setting the appropriate
values with the ioctl).

I could write something simple that does that but it would be a bit
long, I think.

Hmmm, please do write it, but if it's so big, please do it in a second
patch, so that we're always in time to revert that one if it's too much.
Keep the current example in the patch that adds the page.

Thank you,
and have a lovely day!
Alex

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