Re: [PATCH bpf-next v2 1/1] docs: BPF_MAP_TYPE_XSKMAP

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Donald,

On Fri, 18 Nov 2022 09:33:21 +0000, Donald Hunter wrote:
> Akira Yokosawa <akiyks@xxxxxxxxx> writes:
>>
>> So you have two declarations of bpf_map_lookup_elem() in map_xskmap.rst.
>>
>> This will cause "make htmldocs" with Sphinx >=3.1 to emit a warning of:
>>
>> /linux/Documentation/bpf/map_xskmap.rst:100: WARNING: Duplicate C declaration, also defined at map_xskmap:71.
>> Declaration is '.. c:function:: int bpf_map_lookup_elem(int fd, const void *key, void *value)'.
>>
>> , in addition to a bunch of similar warnings observed at bpf-next:
>>
>> /linux/Documentation/bpf/map_cpumap.rst:50: WARNING: Duplicate C declaration, also defined at map_array:43.
>> Declaration is '.. c:function:: int bpf_map_update_elem(int fd, const void *key, const void *value, __u64 flags);'.
>> /linux/Documentation/bpf/map_cpumap.rst:72: WARNING: Duplicate C declaration, also defined at map_array:35.
>> Declaration is '.. c:function:: int bpf_map_lookup_elem(int fd, const void *key, void *value);'.
>> /linux/Documentation/bpf/map_hash.rst:37: WARNING: Duplicate C declaration, also defined at map_array:43.
>> Declaration is '.. c:function:: long bpf_map_update_elem(struct bpf_map *map, const void *key, const void *value, u64 flags)'.
>> ... [bunch of similar warnings]
> 
> That's unfortunate, and I'm responsible for some of those. Not sure how
> we'd know to check for warnings with Sphinx >= 3.1 when
> Documentation/doc-guide/sphinx.rst and
> Documentation/sphinx/requirements.txt both specify version 2.4.4

Sorry, I didn't mean to blame anyone. :-/

I think I need to share some background.
Please read on.

> 
>> You might want to say you don't care, but they would annoy those
>> who do test "make htmldocs".
>>
>> So let me explain why sphinx complains.
>>
>> C domain declarations in kernel documentation are for kernel APIs.
>> By default, c:function declarations belong to the top-level namespace,
>> which is intended for kernel APIs.
>>
>> IIUC, most APIs described in map*.rst files don't belong to kernel.
>> So I think the way to go is to use the c:namespace directive.
>>
>> See: https://www.sphinx-doc.org/en/master/usage/restructuredtext/domains.html#namespacing
>>
>> As mentioned there, namespacing works with Sphinx >=3.1.
>> Currently, kernel documentation build scripts support only the
>> "c:namespace" directive, which means you can't switch namespaces in the
>> middle of a .rst file. This limitation comes from the fact that Sphinx
>> 1.7.9 is still in the list for htmldocs at the moment and build scripts
>> emulate namespacing for Sphinx <3.1 in a limited way.
> 
> What's the reason for keeping support for Sphinx 1.7.9 and pinning to
> 2.4.4 in Documentation/sphinx/requirements.txt if we want to support
> Sphinx >= 3.1? Given that the latest Sphinx release is 5.3.0, and Python
> 2 support was dropped in Sphinx 2.0.0 it seems that we need to have a
> higher minimum version and a higher default version.

Middle term, progressing to recent versions of Sphinx is highly hoped
as we'd really like to utilize the namespacing capability in its full
strength.

Unfortunately, as is mentioned in ./scripts/sphinx_pre:

   Please note that Sphinx >= 3.0 will currently produce false-positive
   warning when the same name is used for more than one type (functions,
   structs, enums,...). This is known Sphinx bug. For more details, see:
	https://github.com/sphinx-doc/sphinx/pull/8313
 
, later Sphinx emits a dozen of false positive warnings of duplicates.
Hence we stick to Sphinx 2.4.4 in doc-guide documents. 2.4.4 or 1.7.9
is good enough for catching easy-to-fix errors in .rst files.

See https://lore.kernel.org/linux-doc/20220702122311.358c0219@xxxxxxx/
for Mauro's thoughts on these issues.

What I'm doing now is manually distinguishing false and real positives
and asking fixes of the latter, so that transition to later Sphinx
versions would be smooth as possible. On of such fixes is commit
c18c20f16219 ("mm, slab: remove duplicate kernel-doc comment for
ksize()") which landed v6.1-rc5.

> 
>> So please avoid putting function declarations of the same name in
>> a .rst file.
> 
> The same function name, with different signature gets used as a BPF
> helper and as a userspace function. We'd really like to be able to
> document the semantics of both for a given BPF map type, all on the same
> page.
> 
> Is there a better way for us to highlight the function signature,
> without using the c:function:: directive, since they're not really
> function declarations?
If you don't feel like bothering with namespacing, your option would
be to use the code-block directive.
I see a couple of APIs presented as plain literal blocks in
map_cgrp_storage.rst.

If you change them to "code-block"s as follows:

diff --git a/Documentation/bpf/map_cgrp_storage.rst b/Documentation/bpf/map_cgrp_storage.rst
index 5d3f603efffa..be31f250453b 100644
--- a/Documentation/bpf/map_cgrp_storage.rst
+++ b/Documentation/bpf/map_cgrp_storage.rst
@@ -18,14 +18,18 @@ Usage
 =====
 
 The map key must be ``sizeof(int)`` representing a cgroup fd.
-To access the storage in a program, use ``bpf_cgrp_storage_get``::
+To access the storage in a program, use ``bpf_cgrp_storage_get``:
+
+.. code-block:: c
 
     void *bpf_cgrp_storage_get(struct bpf_map *map, struct cgroup *cgroup, void *value, u64 flags)
 
 ``flags`` could be 0 or ``BPF_LOCAL_STORAGE_GET_F_CREATE`` which indicates that
 a new local storage will be created if one does not exist.
 
-The local storage can be removed with ``bpf_cgrp_storage_delete``::
+The local storage can be removed with ``bpf_cgrp_storage_delete``:
+
+.. code-block:: c
 
     long bpf_cgrp_storage_delete(struct bpf_map *map, struct cgroup *cgroup)
 
------------
, you can get highlighted signatures. I'm not sure if you like the
way they are highlighted, though.

Hope this helps.

Please feel free to ask if you have further questions.

Akira

> 
>> The other duplicate warnings shown above can be silenced by the
>> change attached below. It is only as a suggestion and I'm not putting
>> a S-o-b tag.
>>
>> Hope this helps,
>>
>> Akira



[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux