> On Mar 14, 2023, at 10:53 PM, David Vernet <void@xxxxxxxxxxxxx> 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@xxxxxxxx> >>>>> >>>>> 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@xxxxxxxxx/ >>> >>> 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@xxxxxxxx/ 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
