Re: [PATCH V3 bpf-next] BPF, docs: libbpf Overview Document

[Sreevani Sreejith <ssreevani@meta.com>]

> On Mar 14, 2023, at 10:53 PM, David Vernet <void@manifault.com> wrote:
> 
> !-------------------------------------------------------------------|
>  This Message Is From an External Sender
> 
> |-------------------------------------------------------------------!
> 
> On Wed, Mar 15, 2023 at 10:28:56AM +0700, Bagas Sanjaya wrote:
>> On Mon, Mar 13, 2023 at 07:59:47AM -0500, David Vernet wrote:
>>> On Mon, Mar 13, 2023 at 04:44:59PM +0700, Bagas Sanjaya wrote:
>>>> On Fri, Mar 10, 2023 at 10:09:28AM -0800, Sreevani Sreejith wrote:
>>>>> From: Sreevani <ssreevani@meta.com>
>>>>> 
>>>>> Summary: Document that provides an overview of libbpf features for BPF
>>>>> application development.
>>>> 
>>>> It seems like you ignore some of my reviews at [1]. Anyway, I
>>>> repeat them here, augmenting my new comments.
>>> 
>>> Sreevani, please be sure to reply to and address all reviewers'
>>> comments. I've also requested that we not use these internal Meta tags
>>> on more than one occasion, so please be mindful of it for future
>>> patches, and take a bit of extra time to double check that you've
>>> addressed all reviewers' concerns. I also suggest reading over [0],
>>> which specifies that new versions of patches should include descriptions
>>> of what's changed from prior versions. Please see Joanne's patch set in
>>> [1] which serves as a very nice example.
>>> 
>>> [0]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#the-canonical-patch-format
>>> [1]: https://lore.kernel.org/all/20230301154953.641654-1-joannelkoong@gmail.com/
>>> 
>>> Bagas -- just FYI, a quick git log would have shown that this is only
>>> Sreevani's second patch. I don't think she intentionally ignored
>>> anything. It's likely just an artifact of getting used to the kernel
>>> review process.
>> 
>> Oops, you mean this v3 is actually v2, right?
> 
> Oh, I just meant that this is her second patch submission to the Linux
> kernel in general, (the first was [0]), so she likely just accidentally
> forgot to address your comments rather than intentionally ignoring them.
> Of course, it's good that you highlighted them again here in v3, as they
> certainly need to be addressed.
> 
> [0]: https://lore.kernel.org/all/20221202221710.320810-2-ssreevani@meta.com/

Bagas, apologies for not addressing one of your comments. As David mentioned,
 it was not intentional. I am still getting used to filtering out comments from the rest
 of the documentation. 
> 
>> 
>>>> Why did you add heading overline and change the heading character marker?
>>> 
>>> I assume that Sreevani is following python documentation conventions [0], which
>>> suggest that #### with overline refers to the highest-level heading in a page.
>>> This is suggested in Sphinx documentation [1] as well.
>>> 
>>> [0]: https://devguide.python.org/documentation/markup/#sections 
>>> [1]: https://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html#sections 
>> 
>> OK.
>> 
>>>> You may want to also add :lineos: option or manually add line numbers
>>>> if you add :emphasize-lines: so that readers can see the line number
>>>> it refers to.
>>> 
>>> What is :lineos:? I don't see it anywhere else in Documentation/ and if
>>> I add it, the docs build complains:
>>> 
>>> Documentation/bpf/libbpf/libbpf_overview.rst:177: WARNING: Error in "code-block" directive:
>>> unknown option: "lineos".
>>> 
>>> .. code-block:: C
>>>  :lineos:
>>>  :emphasize-lines: 6
>> 
>> You forget to indent both options (see [1]).
> 
> The indentation was correct ;-) The option is actually ":linenos", not
> ":lineos:". That said, it's a neat option, so thank you for pointing it
> out.
> 
>>> 
>>>  //...
>>>  struct task_struct *task = (void *)bpf_get_current_task();
>>>  struct task_struct *parent_task;
>>>  int err;
>>> 
>>>  err = bpf_core_read(&parent_task, sizeof(void *), &task->parent);
>>>  if (err) {
>>>    /* handle error */
>>>  }
>>> 
>>>  /* parent_task contains the value of task->parent pointer */
>>> 
>>> I personally think adding line numbers is overkill. The highlighting is
>>> already a nice touch, and gets the point across without the additional
>>> visual cue of line numbers.
>> 
>> But if the snippet above is instead long, how can one looking for the
>> emphasized line number when reading doc (especially in .rst source) other
>> than manually counting from the first line of the snippet? See
>> Documentation/RCU/rcubarrier.rst for example of manual line numbering
>> (and [2] for the related patch).
> 
> Well, that's a bit of a hypothetical problem given that in this case
> we're only talking about 6 lines ;-) In any case, I don't really mind
> one way or the other, but given that none of the other example
> codeblocks in the BPF docs have line numbers, I'd personally err on the
> side of not adding them here.
> 
>>>> BPF apps are application that use BPF program, right? I thought that
>>>> despite there is libbpf-rs, I still have to develop BPF apps in C.
>>> 
>>> It says that at the end of the paragraph?
>>> 
>> 
>> I was confused between BPF apps and BPF programs, since I was accustomed
>> that apps and programs refer to the same thing.
> 
> This is alluded to a bit earlier in this document:
> 
>> A BPF application consists of one or more BPF programs (either
>> cooperating or completely independent), BPF maps, and global
>> variables.
> 
> "Program" in the context of BPF has a very specific meaning. We need to
> improve our documentation on them, but see [1] for a bit more detail.
> The TL;DR is that the BPF program is the part that runs in kernel space.
> 
> [1]: https://www.kernel.org/doc/html/latest/bpf/libbpf/program_types.html#program-types-and-elf
> 
> Thanks,
> David