Re: [bpf-next V2 PATCH 00/15] XDP redirect memory return API

[Alexander Duyck <alexander.duyck@gmail.com>]

On Thu, Mar 8, 2018 at 5:07 AM, Jesper Dangaard Brouer
<brouer@redhat.com> wrote:
> This patchset works towards supporting different XDP RX-ring memory
> allocators.  As this will be needed by the AF_XDP zero-copy mode.
>
> The patchset uses mlx5 as the sample driver, which gets implemented
> XDP_REDIRECT RX-mode, but not ndo_xdp_xmit (as this API is subject to
> change thought the patchset).
>
> A new struct xdp_frame is introduced (modeled after cpumap xdp_pkt).
> And both ndo_xdp_xmit and the new xdp_return_frame end-up using this.
>
> Support for a driver supplied allocator is implemented, and a
> refurbished version of page_pool is the first return allocator type
> introduced.  This will be a integration point for AF_XDP zero-copy.
>
> The mlx5 driver evolve into using the page_pool, and see a performance
> increase (with ndo_xdp_xmit out ixgbe driver) from 6Mpps to 12Mpps.

So one question I would have is what effect does your patch set have
on packets moving in the opposite direction, or ixgbe to ixgbe. My
concern is how much of a hit we are taking in ixgbe to support a more
generic solution.

> The patchset stop at the 15 patches limit, but more API changes are
> planned.  Specifically extending ndo_xdp_xmit and xdp_return_frame
> APIs to support bulking.  As this will address some known limits.
>
> V2: Updated according to Tariq's feedback

I am assuming you are submitting this as an RFC aren't you? If not you
have some general quality issues to work on. I saw c99 style "//"
comments in patch 2, for example:
+struct xdp_mem_info {
+       u32 type; /* enum mem_type, but known size type */
+       u32 id; // Needed later (to lookup struct xdp_rxq_info)
+};
+

You should probably just go with the standard "/* */" style comments
and be consistent.

There are also "FIXME" comments in patch 6, either drop them entirely
so you don't need to remove them in patch 15, or address the issue of
explaining why you needed the call.

- Alex