From:   Sreevani Sreejith <ssreevani@meta.com>
To:     Bagas Sanjaya <bagasdotme@gmail.com>
CC:     Sreevani Sreejith <ssreevani@meta.com>,
        "psreep@gmail.com" <psreep@gmail.com>,
        "bpf@vger.kernel.org" <bpf@vger.kernel.org>,
        "Linux-kernel@vger.kernel.org" <Linux-kernel@vger.kernel.org>,
        "andrii@kernel.org" <andrii@kernel.org>,
        "mykola@meta.com" <mykola@meta.com>,
        "linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
        David Vernet <void@manifault.com>
Subject: Re: [PATCH V3 bpf-next] BPF, docs: libbpf Overview Document
Thread-Topic: [PATCH V3 bpf-next] BPF, docs: libbpf Overview Document
Thread-Index: AQHZU3uIPS1/hSPDT0upU7tPQaXGO674efyAgAA2bYCAAoUrAIAAKHuAgACzdgA=
Date:   Wed, 15 Mar 2023 16:36:19 +0000
Message-ID: <C5E014CB-1A9D-4A29-93AC-1025CBCFC2BF@fb.com>
References: <20230310180928.2462527-1-ssreevani@meta.com>
 <ZA7wm8scokV+XPav@debian.me> <20230313125947.GB2392@maniforge>
 <ZBE7eMsAifEQgRQv@debian.me> <20230315055349.GA20638@maniforge>
In-Reply-To: <20230315055349.GA20638@maniforge>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?tjTn/UK6htE8suGv5NHEdu0q981CfF5WfgXsOukb6M2FWVH3kuXlvjmfZ9l2?=
 =?us-ascii?Q?kpvS7t4jYLPmY1vYWa4P/bkhpMDMxwjigSP40bxmoqwIac5HXbb7qasvdmjz?=
 =?us-ascii?Q?65KvgvaBYAq7JntXh0x7AL1JLs63o0QnNVzYdlYZMSAxYO6GO4LDE+2HBFXb?=
 =?us-ascii?Q?1Kcpqn/O6CDO4qO4aJNVTGKSzM/XZiKt9/2Dp2k3KNNRUgglqmg9LLHdXQLC?=
 =?us-ascii?Q?1Jx1ogUVHqj7ZwcSTc3T1xnWZFy6ls/b9JopY5qCD3ljo2oGdwkWzqukFrpL?=
 =?us-ascii?Q?bLfCdPzq4HdaerbtanctONIIOLrphW8zI4B1juJU8OoqCoEXbziwqdgKG1rX?=
 =?us-ascii?Q?HgabD04M3ATwyU81T4r4d3UmLKDpDmI2CvHLuN5B9H2J+8qKlVmLZRCDuXLR?=
 =?us-ascii?Q?dXtcbnMlyx0NZvDFwPh/zVuXonyrQg7dOa4Kjpu0ru3U13OHf30mpdq1BCGM?=
 =?us-ascii?Q?G8/BgtpqmN6hzHoa4CIwZMANugq+o15UJkUhg4cS8XVvQN2HPBchS8cyA45W?=
 =?us-ascii?Q?KRRFvEtjCkBemKUgliYI8MB0woTV8FBdJmyTKuMecv0Q2hIGWdgha3093O9C?=
 =?us-ascii?Q?eWtIAzlUAE92v+uLZKg52NhGe2vLRyCkh0S0cfC12VH+f4oplLOA3noSyKCF?=
 =?us-ascii?Q?5pzL2Iw5e0ipslS5xoyKA3vN9277+VN050xXt3B0MDl9xs17OhSRTe7236sq?=
 =?us-ascii?Q?lAq44+c6IImQL7LuYXxo/wfJRdq6O2W6DJZeus2oFnuAoYKJFxHMeGdkdbTI?=
 =?us-ascii?Q?OKdG+y1w8K52im7drzvlYkV6+LXVgyX+Zk6qTXK7cFIJ071j+g02OsQZWTTC?=
 =?us-ascii?Q?AL4Lbvh2zxld94T6z+7J7wbd6pm4PWAghn1ptT+gkjfjvCnALZSrZMasMoQH?=
 =?us-ascii?Q?0hcvuc+LP98X6OyDSoQQJZiKyhNGeGmKpgMXYWepWQqg0iDF2cjDNspj0r3j?=
 =?us-ascii?Q?403wjw/Yzf6P0Y5/w5LewhabrqyTFSlPVW4qingJTIu2lgwAUHehtfXe0/1n?=
 =?us-ascii?Q?iezRO+DRcQDoeH/8/o35ve3RtQwRBZec4cedxfaKz1bWN5YtOcooEnKAL7VW?=
 =?us-ascii?Q?4aI8iObAFJtgEhHwdQmBsIy1c9n4y9v1E5taekivMHrpEvOMM6X9ME+Kg0oX?=
 =?us-ascii?Q?I0fry6g3hWKc7NYXngRBxSE9Y0xpAaPgqUBvzx+Gwdyihk+0QCBJ4op6w85c?=
 =?us-ascii?Q?Ja7BF+8IVmQMbTUG7Tw/3A+GQTgv2qh4JdHQO3RUl9ujIF/Ox2AvjpKRnBPb?=
 =?us-ascii?Q?snp3bpDcGIO7VqPpGhf7wvWx3p3pAFUst18yyQV0pwkylwQBCcXvgw6fqOb5?=
 =?us-ascii?Q?+nIxsS9BTgQFSxHGqKfKcvB32V5DtnXvp+B6qE+6TlSSryCsEaUTim92yXtD?=
 =?us-ascii?Q?sbnmEXRT4GREsKFRcDmfHmEthzf5zqunhsM4WRaVIRfyfklpA1yIJiuYTeMn?=
 =?us-ascii?Q?ktxHFg+6es0sJ1QrevPaNMBsybf4i/TGeah6XHCGVt312aJgFapm6mVsCj9c?=
 =?us-ascii?Q?jE/CEuwU4yOCvyU5WEJHvWueaVM6kqxrhaa17ej7tOXkjvV2ax4yeVwY7Nf5?=
 =?us-ascii?Q?E7c4an4bvWgcqKvytmMJ6DmdrjRmBxlqTjQo7ozRfKYpNCAMY6kYibk0ljdz?=
 =?us-ascii?Q?9uIz/tKfLUC1AgMtFp6R2L4=3D?=
Content-ID: <9229BB9BEB3D714F822A90DE58D530B0@namprd15.prod.outlook.com>
X-OriginatorOrg: meta.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DM4PR15MB5354.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: e5408e1a-2e2a-4592-99bd-08db257367bf
X-MS-Exchange-CrossTenant-originalarrivaltime: 15 Mar 2023 16:36:19.4821
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: rAnULjKxD6IEsNXN/owhlyR3XTFOwHXpKHXIptZMrMcc78W2Lx+538hGNtqtUW7t2BauG7Bd5gwWZOHc4K8O3A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR15MB3813
X-Proofpoint-GUID: fG1dK5uky--z2FzXuhAqUGN7W2dl1zBh
X-Proofpoint-ORIG-GUID: fG1dK5uky--z2FzXuhAqUGN7W2dl1zBh
Content-Type: text/plain; charset="us-ascii"
X-Proofpoint-UnRewURL: 2 URL's were un-rewritten
MIME-Version: 1.0
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.942,Hydra:6.0.573,FMLib:17.11.170.22
 definitions=2023-03-15_08,2023-03-15_01,2023-02-09_01
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> 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