Re: [PATCH net-next v9 03/20] net: generalise net_iov chunk owners

[Mina Almasry <almasrymina@google.com>]

On Tue, Dec 17, 2024 at 4:37 PM David Wei <dw@davidwei.uk> wrote:
>
> From: Pavel Begunkov <asml.silence@gmail.com>
>
> Currently net_iov stores a pointer to struct dmabuf_genpool_chunk_owner,
> which serves as a useful abstraction to share data and provide a
> context. However, it's too devmem specific, and we want to reuse it for
> other memory providers, and for that we need to decouple net_iov from
> devmem. Make net_iov to point to a new base structure called
> net_iov_area, which dmabuf_genpool_chunk_owner extends.
>
> Reviewed-by: Mina Almasry <almasrymina@google.com>
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> Signed-off-by: David Wei <dw@davidwei.uk>
> ---
>  include/net/netmem.h | 21 ++++++++++++++++++++-
>  net/core/devmem.c    | 25 +++++++++++++------------
>  net/core/devmem.h    | 25 +++++++++----------------
>  3 files changed, 42 insertions(+), 29 deletions(-)
>
> diff --git a/include/net/netmem.h b/include/net/netmem.h
> index 1b58faa4f20f..c61d5b21e7b4 100644
> --- a/include/net/netmem.h
> +++ b/include/net/netmem.h
> @@ -24,11 +24,20 @@ struct net_iov {
>         unsigned long __unused_padding;
>         unsigned long pp_magic;
>         struct page_pool *pp;
> -       struct dmabuf_genpool_chunk_owner *owner;
> +       struct net_iov_area *owner;
>         unsigned long dma_addr;
>         atomic_long_t pp_ref_count;
>  };
>
> +struct net_iov_area {
> +       /* Array of net_iovs for this area. */
> +       struct net_iov *niovs;
> +       size_t num_niovs;
> +
> +       /* Offset into the dma-buf where this chunk starts.  */

Probably could use a rewriting of the comment. I think net_iov_area is
meant to be a memory-type agnostic struct, so it shouldn't refer to
dma-bufs here.

But on second thought I'm not sure base_virtual belongs in the common
struct because i'm not sure yet how it applies to other memory
providers. for dmabuf, each 'chunk' is physically and virtually
contiguous, and the virtual address is well-defined to be the offset
in bytes into the dmabuf, but that is very dmabuf specific.

Consider keeping this in dmabuf_genpool_chunk_owner if we don't plan
to use this same field with the same semantics for other memory
providers, or re-write the comment.

-- 
Thanks,
Mina