Date:   Mon, 13 Mar 2023 07:59:47 -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: <20230313125947.GB2392@maniforge>
References: <20230310180928.2462527-1-ssreevani@meta.com>
 <ZA7wm8scokV+XPav@debian.me>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZA7wm8scokV+XPav@debian.me>
User-Agent: Mutt/2.2.9 (2022-11-12)
Precedence: bulk

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.

> 
> The patch description should have been "Document overview of libbpf,
> including its features for developing BPF programs.".
> 
> > +######
> >  libbpf
> > -======
> > +######
> 
> 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

> 
> > +The following code snippet shows how to read the parent field of a kernel
> > +``task_struct`` using BPF CO-RE and libbf. The basic helper to read a field in a
> > +CO-RE relocatable manner is ``bpf_core_read(dst, sz, src)``, which will read
> > +``sz`` bytes from the field referenced by ``src`` into the memory pointed to by
> > +``dst``.
> > +
> > +  .. code-block:: C
> > +    :emphasize-lines: 6
> > +
> > +    //...
> > +    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 */
> 
> 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

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

> 
> > +Also, find the libbpf API documentation `here
> > +<https://libbpf.readthedocs.io/en/latest/api.html>`_
> 
> "See also `libbpf API documentation <link>`_".
> 
> > +
> > +libbpf and Rust
> > +===============
> > +
> > +If you are building BPF applications in Rust, it is recommended to use the
> > +`Libbpf-rs <https://github.com/libbpf/libbpf-rs>`_ library instead of bindgen
> > +bindings directly to libbpf. Libbpf-rs wraps libbpf functionality in
> > +Rust-idiomatic interfaces and provides libbpf-cargo plugin to handle BPF code
> > +compilation and skeleton generation. Using Libbpf-rs will make building user
> > +space part of the BPF application easier. Note that the BPF program themselves
> > +must still be written in plain C.
> 
> 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?

> 
> Thanks.
> 
> [1]: https://lore.kernel.org/linux-doc/ZAqzeQZLNMyaZOck@debian.me/
> 
> -- 
> An old man doll... just what I always wanted! - Clara