Hi Michael, 2018-05-28 10:19 UTC+0200 ~ Michael Kerrisk (man-opages) <mtk.manpages@xxxxxxxxx> > Hello Quentin, Daniel, > > On 05/23/2018 09:03 PM, Quentin Monnet wrote: >> Hi Michael, >> >> Thanks a lot for your review and feedback! Please find some comments >> inline below. >> >> 2018-05-23 17:46 UTC+0200 ~ Michael Kerrisk (man-pages) >> <mtk.manpages@xxxxxxxxx> >>> Hello Quentin, >>> >>> On 30 April 2018 at 18:37, Quentin Monnet >>> <quentin.monnet@xxxxxxxxxxxxx> wrote: >>>> eBPF sub-system on Linux can use "helper functions", functions >>>> implemented in the kernel that can be called from within a eBPF program >>>> injected by a user on Linux. The kernel already supports a long list of >>>> such helpers (sixty-seven at this time, new ones are under review). >>>> Therefore, it is proposed to create a new manual page, separate from >>>> bpf(2), to document those helpers for people willing to develop new >>>> eBPF >>>> programs. >>> >>> Thank you for all of this work! And, agreed, a separate manual page >>> is best. >>> >>>> Additionally, in an effort to keep this documentation in >>>> synchronisation >>>> with what is implemented in the kernel, it is further proposed to keep >>>> the documentation itself in the kernel sources, as comments in file >>>> "include/uapi/linux/bpf.h", and to generate the man page from there. >>> >>> Can you say some more about this. I mean, putting the man-page source >>> in has some disadvantages from my point of view. So, I want to ask: >>> will putting all the documentation in a .h file really assist >>> synchronization (with reasoning please! ;-)). >> >> To keep the man page up-to-date, the best is obviously to edit it each >> time a kernel patch updates the eBPF helpers. The idea here is to have >> authors of the patches related to these helpers to update the >> documentation when they submit their code. Since the first chunks of the >> documentation were merged on the kernel side, eBPF maintainers have been >> able to ask this of people who proposed new helpers. As a result, since >> I posted the present patch for the manual page, five more helper >> functions have been added and documented this way. We did not have to >> update the descriptions of existing functions in that time interval, but >> occasional changes of behaviour (new flags supported, new eBPF map types >> supported, etc.) have occurred in the past and will likely happen again >> from time to time. >> >> If the documentation was instead a straight manual page in the man-pages >> repository, I am very unsure it would be up-to-date even as of today. >> From my point of view (which seems to be shared by other eBPF >> maintainers?), it is more delicate to ask to the authors of those >> functions to go and update the documentation in a *separate* repository: >> - It adds some overhead to the process, by making them send a patch set >> to another project, and to follow-up for reviews after that. >> - Using groff is, well, not exactly attracting. If documentation cannot >> be added in the same patch set as the code, I fear it may deter authors >> from updating the page. reStructuredText seems complex enough already, >> we had to fix a couple of formatting mistakes in the description of the >> last helpers. >> - With all due respect, I believe that netdev mailing list is the best >> place to get technical reviews on the contents of this documentation. >> Having it in the kernel makes it easy for eBPF contributors to review >> the changes. >> >> So in short: I expect most of the contributors to this documentation >> will be people updating those helpers, and it makes it much easier, in >> terms of process as in terms of technical reviews, to ask them to edit >> the description if the doc is at the same location as the actual code. >> >>> There are some downsides, from my point of view: >>> >>> * The generated man page source is kind of ugly. >> >> I agree (although raw groff pages in general rarely look appealing to >> me). But if the page is to be automatically generated anyway, this is >> not really a major issue in my opinion. >> >>> * The generated man page renders better than I would expect, but there >>> are places where things are a bit wrong. For example, using ".sp" as a >>> paragrap separator renders poorly when producing PostScript/PDF >>> versions of the page. >> >> This is more annoying. I would be ready to work to improve this. I >> suppose we could post-process the page; adding a third tool to the work >> flow may not be ideal, but might be worth if it really improves the >> final output. > > That was the only issue I noticed so far. Maybe there are others > that I'll see later, but as I said, the page looks better than I might > have expected for a generated page. The incorrect use of ".sp" just > bothers me because a year or so back I fixed that issue in hundreds > of the existing pages. > >>> * People are likely to send me patches for the generated page. I won't >>> be able to accept those. Instead someone (possibly me), will have to >>> turn them into patches against the .h file. (This will be amplified by >>> the fact that many more people are likely to look at the manual page >>> than the at the kernel source file.) >> >> Could you maybe redirect those people to the .h file instead of >> translating the patches yourself? I certainly do not have your >> experience as a project maintainer, maybe redirecting volunteer >> contributors proposing their patches is more delicate than I can see? >> >> On a side note, I believe that most contributors to this page are likely >> to be more or less familiar with contributing to the kernel anyway. > > Not to be too pessimistic, but I've found it's often a bit of a > challenge to get man-pages contributions from kernel developers... > >> I >> had a look at the list of people who edited the bpf(2) manual page, and >> it seems that most of them could have sent their patch against the .h >> with little to no difficulty: I would expect most contributors to >> bpf-helpers(2) to be in a similar situation, or even to be the very >> authors of the documented changes. >> >>> Overall, I think I'd prefer to have this as a straight manual page, >>> rather than something generated from a kernel source file. My >>> reasoning is that I'm not convinced about the argument that putting >>> the documentation into a .h file will really hellp in synchronizing >>> with the implementation. And having a true manual page would allow me >>> to avoid the issues mentioned above (as well as some others that I >>> probably haven't thought of right now). Obviously I can't insist on >>> this, but it would I think make my life easier. >> >> I certainly do not want to make things harder for you. And in the other >> way round, I cannot and (do not want) to force you to take the generated >> page either :). >> >> To be honest I started to write the documentation as a straight manual >> page, in groff. I did not mind going all the way with it, but after >> several discussions with eBPF folks (mostly Alexei and Daniel, both >> CC-ed) came the idea of moving it to reStructuredText, and placing it in >> the bpf.h header file. The consensus, if I remember correctly, was that >> it would be easier to have all the documentation in one place, >> preferably in the kernel (we discussed placing it under Documentation/, >> then moved to bpf.h where a brief description of the helpers was present >> already), for the reasons I provided above. >> >> Although not relevant here, another advantage of parsing this >> description is the possibility to reuse the contents to produce other >> kinds of output (for example, the list of function prototypes can be >> used to automatically generate a new, up-to-date .h header file usable >> with eBPF programs). >> >>> I'd be happy to work with you on just converting this to a straight >>> manual page, which might simply mean that I edit the currently >>> generated version to produce something that fits better with current >>> norms for the man-pages project. >> >> We could come to that if you do not want the generated page. But it >> seems to me that creating it from the kernel side is the best compromise >> we (on the eBPF side) could find. Again, I do not want to impose it on >> you. But I would really like to convince you it is worth it! :) Please >> let me know what you think of those arguments, or if you have further >> questions. > > So, I think we should try it your way. I guess we'll need a while > to see how well things work, and if need be, we can revisit how we do > things. Thanks a lot! Of course, I'm totally open to revisiting it in the future, if need be. We can also see if the use of ".sp" is really a problem, or if any other formatting issue arise (please let me know in that case). I could add some post-processing script if it appears to be worth it. >>>> This patch adds the new man page, generated from kernel sources, to the >>>> man-pages repository. For each eBPF helper function, a description of >>>> the helper, of its arguments and of the return value is provided. The >>>> idea is that all future changes for this page should be redirected to >>>> the kernel file "include/uapi/linux/bpf.h", and the modified page >>>> generated from there. >>>> >>>> Generating the page itself is a two-step process. First, the >>>> documentation is extracted from include/uapi/linux/bpf.h, and converted >>>> to a RST (reStructuredText-formatted) page, with the relevant script >>>> from Linux sources: >>>> >>>> $ ./scripts/bpf_helpers_doc.py > /tmp/bpf-helpers.rst >>>> >>>> The second step consists in turning the RST document into the final man >>>> page, with rst2man: >>>> >>>> $ rst2man /tmp/bpf-helpers.rst > bpf-helpers.7 >>>> >>>> The bpf.h file was taken at kernel commit 081023a31129 (that should be >>>> in Linux 4.18). > > So, I suppose the procedure is that I should resync by performing this > step every couple of months? This, or I can also help by doing the sync and sending a patch from time to time. Updating the page once for each kernel release would be ideal, I believe? Please do not merge this version of the patch, I'll send a v2 updated with the latest helpers added to the kernel. Best, Quentin -- To unsubscribe from this list: send the line "unsubscribe linux-man" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html