Re: [PATCH bpf-next v6 1/1] doc: DEVMAPs and XDP_REDIRECT

[Yonghong Song <yhs@meta.com>]

On 11/8/22 9:48 AM, mtahhan@redhat.com wrote:
> From: Maryam Tahhan <mtahhan@redhat.com>
> 
> Add documentation for BPF_MAP_TYPE_DEVMAP and
> BPF_MAP_TYPE_DEVMAP_HASH including kernel version
> introduced, usage and examples.
> 
> Add documentation that describes XDP_REDIRECT.
> 
> Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>

LGTM with a couple of nits below.

Acked-by: Yonghong Song <yhs@fb.com>

> ---
>   Documentation/bpf/index.rst      |   1 +
>   Documentation/bpf/map_devmap.rst | 221 +++++++++++++++++++++++++++++++
>   Documentation/bpf/redirect.rst   |  81 +++++++++++
>   net/core/filter.c                |   8 +-
>   4 files changed, 309 insertions(+), 2 deletions(-)
>   create mode 100644 Documentation/bpf/map_devmap.rst
>   create mode 100644 Documentation/bpf/redirect.rst
> 
> diff --git a/Documentation/bpf/index.rst b/Documentation/bpf/index.rst
> index 1b50de1983ee..1088d44634d6 100644
> --- a/Documentation/bpf/index.rst
> +++ b/Documentation/bpf/index.rst
> @@ -29,6 +29,7 @@ that goes into great technical depth about the BPF Architecture.
>      clang-notes
>      linux-notes
>      other
> +   redirect
>   
>   .. only::  subproject and html
>   
[...]
> +
> +User space
> +----------
> +
> +The following code snippet shows how to update a devmap called ``tx_port``.
> +
> +.. code-block:: c
> +
> +    int update_devmap(int ifindex, int redirect_ifindex)
> +    {
> +        int ret = -1;

There is no need to initialize the value to -1, the below is 'ret = ...'.

> +
> +        ret = bpf_map_update_elem(bpf_map__fd(tx_port), &ifindex, &redirect_ifindex, 0);
> +        if (ret < 0) {
> +            fprintf(stderr, "Failed to update devmap_ value: %s\n",
> +                strerror(errno));
> +        }
> +
> +        return ret;
> +    }
> +
> +The following code snippet shows how to update a hash_devmap called ``forward_map``.
> +
> +.. code-block:: c
> +
> +    int update_devmap(int ifindex, int redirect_ifindex)
> +    {
> +        struct bpf_devmap_val devmap_val = { .ifindex = redirect_ifindex };
> +        int ret = -1;

same here, no need to have '-1'.

> +
> +        ret = bpf_map_update_elem(bpf_map__fd(forward_map), &ifindex, &devmap_val, 0);
> +        if (ret < 0) {
> +            fprintf(stderr, "Failed to update devmap_ value: %s\n",
> +                strerror(errno));
> +        }
> +        return ret;
> +    }
> +
[...]