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. Thanks for your feedback, Michael! To add to Quentin's points, from BPF kernel maintainer side we're fully on board with this effort. There's also some additional information in his cover letter as well [0] also cc'ed to linux-man@xxxxxxxxxxxxxxx back then. The issue we currently have with bpf(2) is that right now it is quite outdated and while I can see some changes in the git log there haven't been major additions since the initial merge. I'm still hoping we can close this big gap to upstream. In terms of BPF helpers, there's even more changes than to the core API itself for each release. While the helper documentation prior to Quentin's major efforts were suboptimal at best, we decided to kill two birds with one stone: it was known that some BPF devs prefer the uapi header as documentation when developing BPF programs while others prefer working with man pages, and here we have the unique ability as maintainers to enforce proper and reviewed documentation for BPF helpers since they need to be enlisted in the __BPF_FUNC_MAPPER() in the bpf uapi header anyway. This approach seems to be working fine so far and since initial merge proper documentation for several other new helpers have landed as well. Thanks to Quentin's script this would allow us to have a bpf-helpers.7 that is always up to date. For an initial merge into the man pages project there might be some more edits needed, but we'd be more than happy to help working with you through this process of course. [0] http://patchwork.ozlabs.org/cover/904619/ >> 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. > >> * 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. 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. > > Best regards, > Quentin > > > >> Thanks, >> >> Michael >> >> >>> 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). >> >> >>> Cc: Alexei Starovoitov <ast@xxxxxxxxxx> >>> Cc: Daniel Borkmann <daniel@xxxxxxxxxxxxx> >>> Signed-off-by: Quentin Monnet <quentin.monnet@xxxxxxxxxxxxx> >>> --- >>> man2/bpf.2 | 1 + >>> man7/bpf-helpers.7 | 1983 ++++++++++++++++++++++++++++++++++++++++++++++++++++ >>> 2 files changed, 1984 insertions(+) >>> create mode 100644 man7/bpf-helpers.7 -- 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
