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

[Maryam Tahhan <mtahhan@redhat.com>]

On 24/10/2022 13:12, Jesper Dangaard Brouer wrote:
> 
> First of all, I'm super happy that we are getting documentation added 
> for this.
> 
> Comments inlined below.
> 
> On 17/10/2022 11.47, mtahhan@redhat.com wrote:
>> diff --git a/Documentation/bpf/redirect.rst 
>> b/Documentation/bpf/redirect.rst
>> new file mode 100644
>> index 000000000000..5a0377a67ff0
>> --- /dev/null
>> +++ b/Documentation/bpf/redirect.rst
> 
> Naming the file 'redirect.rst' is that in anticipating that TC-BPF also 
> support invoking the bpf_redirect helper?
> 
> IMHO we should remember to *also* promote TC-BPF redirect, and it would 
> likely be good to have this in same file with XDP-redirect so end-users 
> see this.
> 

So I will leave the name as is...

> 
>> @@ -0,0 +1,46 @@
>> +.. SPDX-License-Identifier: GPL-2.0-only
>> +.. Copyright (C) 2022 Red Hat, Inc.
>> +
>> +============
>> +XDP_REDIRECT
>> +============
>> +
>> +XDP_REDIRECT works by a three-step process, implemented as follows:
>> +
>> +1. The ``bpf_redirect()`` and ``bpf_redirect_map()`` helpers will 
>> lookup the
>> +   target of the redirect and store it (along with some other 
>> metadata) in a
>> +   per-CPU ``struct bpf_redirect_info``. This is where the maps above 
>> come into
>> +   play.
>> +
>> +2. When the program returns the ``XDP_REDIRECT`` return code, the 
>> driver will
>> +   call ``xdp_do_redirect()`` which will use the information in ``struct
>> +   bpf_redirect_info`` to actually enqueue the frame into a map 
>> type-specific
>> +   bulk queue structure.
>> +
>> +3. Before exiting its NAPI poll loop, the driver will call 
>> ``xdp_do_flush()``,
>> +   which will flush all the different bulk queues, thus completing the
>> +   redirect.
> 
> Is this text more or less copied from net/core/filter.c ?
> 
> I will suggest directly including this from the code via the DOC text
> trick.  (note I've added these DOC tags in other XDP + page_pool code,
> but not fully utilized these yet).
> 

Ok, I will update this. I had the v5 sent in before I saw your email.

> 
>> +Pointers to the map entries will be kept around for this whole 
>> sequence of
>> +steps, protected by RCU. However, there is no top-level 
>> ``rcu_read_lock()`` in
>> +the core code; instead, the RCU protection relies on everything 
>> happening
>> +inside a single NAPI poll sequence.
>> +
>> +.. note::
>> +    Not all drivers support transmitting frames after a redirect, and 
>> for
>> +    those that do, not all of them support non-linear frames. 
>> Non-linear xdp
>> +    bufs/frames are bufs/frames that contain more than one fragment.
>> +
> 
> I would like for us to extend this redirect.rst document with
> information on how to troubleshoot when XDP-redirect "silently" drops
> packets.
> 
> Above note it one issue (but not visible to readers).
> Plus we should describe how to catch these silent drops, via tracepoints
> and even point to xdpdump tool.
> 
> I recently helped someone on Slack debug a XDP redirect issue.
> During this session I wrote some bpftrace oneliners, that I added to 
> XDP-tutorial sub-README[1]
> 
> [1] 
> https://github.com/xdp-project/xdp-tutorial/blob/master/tracing02-xdp-monitor/README.org
> 
Ok, I will see what I can do.

> 
>> +XDP_REDIRECT works with the following map types:
>> +
>> +- BPF_MAP_TYPE_DEVMAP
>> +- BPF_MAP_TYPE_DEVMAP_HASH
>> +- BPF_MAP_TYPE_CPUMAP
>> +- BPF_MAP_TYPE_XSKMAP
>> +
>> +For more information on these maps, please see the specific map 
>> documentation.
>> +
>> +References
>> +===========
>> +
>> +-https://elixir.bootlin.com/linux/latest/source/net/core/filter.c#L4106
> 
> I don't think this reference with a line-number will be stable.

Yep, will move to the doc reference as you suggested earlier.

> 
> --Jesper
>