Date:   Wed, 15 Mar 2023 00:53:49 -0500
From:   David Vernet <void@manifault.com>
To:     Bagas Sanjaya <bagasdotme@gmail.com>
Cc:     Sreevani Sreejith <ssreevani@meta.com>, psreep@gmail.com,
        bpf@vger.kernel.org, Linux-kernel@vger.kernel.org,
        andrii@kernel.org, mykola@meta.com, linux-doc@vger.kernel.org
Subject: Re: [PATCH V3 bpf-next] BPF, docs: libbpf Overview Document
Message-ID: <20230315055349.GA20638@maniforge>
References: <20230310180928.2462527-1-ssreevani@meta.com>
 <ZA7wm8scokV+XPav@debian.me>
 <20230313125947.GB2392@maniforge>
 <ZBE7eMsAifEQgRQv@debian.me>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZBE7eMsAifEQgRQv@debian.me>
User-Agent: Mutt/2.2.9 (2022-11-12)
Precedence: bulk

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/

> 
> > > 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