On Tue, Jun 11, 2024 at 10:54:27AM +0200, Alejandro Colomar wrote: > 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. No problem at all; I really do appreciate your work here keeping the man pages consistent and usable. Thanks for giving your time to help me get this man page setup properly. > [...] > > > > > +.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. Ah right forgot about the constants, so what I did instead was this, which includes the header only once: .SH SYNOPSIS .EX .BR "#include <sys/epoll.h>" " /* Definition of " EPIOC* " constants */" .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 ); .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 }; Does that look OK? [...] > > 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 OK, fixed! > > > > +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.) Hmm, I need to go back and re-read the semantic newline email because I made this section look like this: .P .I argp.busy_poll_budget the maximum number of packets that the network stack will 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. But I get the feeling this is still incorrect. > [...] > [...] > > > > +.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. I tried to follow the discussion you and Branden had in the following emails. I apologize, but I don't think I quite follow what I should be using as a result of that conversation? \en \\n or something else? Thanks, Joe
