Hi Joe, On Mon, Jun 10, 2024 at 05:29:06PM GMT, Joe Damato wrote: > On Tue, Jun 11, 2024 at 01:45:57AM +0200, Alejandro Colomar wrote: > > 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! > > Thanks again for your careful review. Sorry this wasn't the winning > revision :) No problem. Sorry for being so pedantic. (Not sorry, actually.) :-) And thanks for your patience on my review. [...] > > > +.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> > > Hmm, that's the linux sources header file, I think. > > Should I be showing the glibc header instead? > > https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/sys/epoll.h;h=45e546fa4440a83bb94288c220bfbe9295f02cc9;hb=92c270d32caf3f8d5a02b8e46c7ec5d9d0315158#l91 Ahh, sure, and for the constants too. We prefer glibc headers when available. I had the inertia that most ioctl(2)s do not have glibc headers. > Which would be: > > .B #include <sys/epoll.h> > > It's in the same header as epoll_create(2) and > epoll_create1(2). > > Let me know what you think. Like. [...] > > > +description below to learn what configuration is > > > +supported. > > > +.TP > > > +.B EPIOCGPARAMS > > > +Get the current > > > +.I epoll_params > > > +configuration settings. > > > +.TP > > I think this .TP should be a .P, not a .TP. It looks better as a .P, > at least :) > > Let me know what you think. Yup. BTW, here's the mnemonics: P paragraph TP tagged paragraph IP indented paragraph [...] > > > +.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?) > > I've made them: > > .\" linux.git commit 18e2bf0edf4dd88d9656ec92395aa47392e85b61 > .\" glibc.git commit 92c270d32caf3f8d5a02b8e46c7ec5d9d0315158 Good. I'll start using that format everywhere. > > > +.\" glibc commit 92c270d32caf3f8d5a02b8e46c7ec5d9d0315158 > > > +.P > > > > I would use a subsection (.SS) for documenting the structure. > > Sure, I can do that. > > .SS > The epoll_params structure > .I argp.busy_poll_usecs > > Is that OK for a heading? > > I saw this is how man/man2/stat.2 does the subsection. > > Let me know what you think. Yep. Except that the title goes on the SS line: $ grep '\.SS' man/man2/stat.2 .SS The stat structure .SS fstatat() .SS C library/kernel differences [...] > > > +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. > > Changed this to: > > retrieve on each poll attempt. This value cannot exceed > .B NAPI_POLL_WEIGHT > (which is 64 as of Linux 6.9), unless the process is run with > .B CAP_NET_ADMIN. > > How is that? Much better. (But still needs to use semantic newlines.) [...] > > > +.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. > > This line here has a weird indentation compared to the rest of the > errors when rendered. > > Maybe I am doing something wrong with this one? Nope; it's all right. When the tag of the tagged paragraph is larger than the indentation, the paragraph is moved to the next line, so that it starts with the same indentation. [...] > > > +.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(¶ms, 0, sizeof(struct epoll_params)); > > > +\& > > > +params.busy_poll_usecs = 25; > > > +params.busy_poll_budget = 8; > > > +params.prefer_busy_poll = 1; > > > +\& > > > +if (ioctl(epollfd, EPIOCSPARAMS, ¶ms) == \-1) { > > > + perror("ioctl"); > > > + exit(EXIT_FAILURE); > > > +} > > > +\& > > > +/* Code to show how to retrieve the current settings */ > > > +\& > > > +memset(¶ms, 0, sizeof(struct epoll_params)); > > > +\& > > > +if (ioctl(epollfd, EPIOCGPARAMS, ¶ms) == \-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.) > > Change this to '\e' and tested it. It looks like it works to me :) Hmm, yep, both work the same. I remember there's a small difference in meaning, but I don't know why we use \e. Anyway. $ cat esc.man .TH a s d f .SH example .EX a\\b\ec .EE $ groff -man -Tutf8 esc.man -rLL=72n a(s) a(s) example a\b\c f d a(s) Have a lovely day! Alex -- <https://www.alejandro-colomar.es/>
Attachment:
signature.asc
Description: PGP signature
