Re: [patch] bpf-helpers.7: Add new man page for eBPF helper functions

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Kernel Documentation]     [Netdev]     [Linux Ethernet Bridging]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]

  Powered by Linux