Re: [PATCH bpf-next v4] docs/bpf: Add documentation for BPF_MAP_TYPE_SK_STORAGE

[Martin KaFai Lau <martin.lau@linux.dev>]

On 12/9/22 9:54 AM, Alexei Starovoitov wrote:
> On Fri, Dec 9, 2022 at 9:52 AM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>>
>> On 12/9/22 3:24 AM, Donald Hunter wrote:
>>> Add documentation for the BPF_MAP_TYPE_SK_STORAGE including
>>> kernel version introduced, usage and examples.
>>
>> Thanks for writing the doc for sk_storage!
>>
>>> +User space
>>> +----------
>>> +
>>> +bpf_map_update_elem()
>>> +~~~~~~~~~~~~~~~~~~~~~
>>> +
>>> +.. code-block:: c
>>> +
>>> +   int bpf_map_update_elem(int map_fd, const void *key, const void *value, __u64 flags)
>>> +
>>> +Socket-local storage for the socket identified by ``key`` belonging to
>>> +``map_fd`` can be added or updated using the ``bpf_map_update_elem()`` libbpf > +function. ``key`` must be a pointer to a valid ``fd`` in the user space
>>> +program. The ``flags`` parameter can be used to control the update behaviour:
>>
>> The "``key`` belonging to ``map_fd``" seems confusing.  Also, it is useful to
>> highlight the ``key`` is a _socket_ ``fd``.
>>
>> May be something like:
>>
>> A socket-local storage can be added/updated locally to a socket identified by a
>> _socket_ ``fd`` stored in the pointer ``key``.  The pointer ``value`` has the
>> data to be added/updated to the socket ``fd``.  The type and size of ``value``
>> should be the same as the value type of the map definition.
>>
>> Feel free to rephrase the above in a better way.
>>
>>> +
>>> +- ``BPF_ANY`` will create storage for ``fd`` or update existing storage.
>>> +- ``BPF_NOEXIST`` will create storage for ``fd`` only if it did not already
>>> +  exist, otherwise the call will fail with ``-EEXIST``.
>>> +- ``BPF_EXIST`` will update existing storage for ``fd`` if it already exists,
>>> +  otherwise the call will fail with ``-ENOENT``.
>>> +
>>> +Returns ``0`` on success, or negative error in case of failure.
>>> +
>>> +bpf_map_lookup_elem()
>>> +~~~~~~~~~~~~~~~~~~~~~
>>> +
>>> +.. code-block:: c
>>> +
>>> +   int bpf_map_lookup_elem(int map_fd, const void *key, void *value)
>>> +
>>> +Socket-local storage for the socket identified by ``key`` belonging to
>>> +``map_fd`` can be retrieved using the ``bpf_map_lookup_elem()`` libbpf
>>> +function. ``key`` must be a pointer to a valid ``fd`` in the user space
>>
>> Same here.
>>
>>> +program. Returns ``0`` on success, or negative error in case of failure.
>>> +
>>> +bpf_map_delete_elem()
>>> +~~~~~~~~~~~~~~~~~~~~~
>>> +
>>> +.. code-block:: c
>>> +
>>> +   int bpf_map_delete_elem(int map_fd, const void *key)
>>> +
>>> +Socket-local storage for the socket identified by ``key`` belonging to
>>> +``map_fd`` can be deleted using the ``bpf_map_delete_elem()`` libbpf
>>> +function. Returns ``0`` on success, or negative error in case of failure.
>>
>> Same here.
> 
> 
> Sorry Martin. I just applied it without seeing your comments.
> Should I revert or this can be done in the follow up?
Ah, just noticed that also.  My bad that only catching up till v4.  It can 
definitely be a followup.