Re: [PATCH net-next v10 00/10] net: intel: start The Great Code Dedup + Page Pool for iavf

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

On 4/18/24 13:36, Alexander Lobakin wrote:
> Here's a two-shot: introduce {,Intel} Ethernet common library (libeth and
> libie) and switch iavf to Page Pool. Details are in the commit messages;
> here's a summary:
> 
> Not a secret there's a ton of code duplication between two and more Intel
> ethernet modules. Before introducing new changes, which would need to be
> copied over again, start decoupling the already existing duplicate
> functionality into a new module, which will be shared between several
> Intel Ethernet drivers. The first name that came to my mind was
> "libie" -- "Intel Ethernet common library". Also this sounds like
> "lovelie" (-> one word, no "lib I E" pls) and can be expanded as
> "lib Internet Explorer" :P
> The "generic", pure-software part is placed separately, so that it can be
> easily reused in any driver by any vendor without linking to the Intel
> pre-200G guts. In a few words, it's something any modern driver does the
> same way, but nobody moved it level up (yet).
> The series is only the beginning. From now on, adding every new feature
> or doing any good driver refactoring will remove much more lines than add
> for quite some time. There's a basic roadmap with some deduplications
> planned already, not speaking of that touching every line now asks:
> "can I share this?". The final destination is very ambitious: have only
> one unified driver for at least i40e, ice, iavf, and idpf with a struct
> ops for each generation. That's never gonna happen, right? But you still
> can at least try.
> PP conversion for iavf lands within the same series as these two are tied
> closely. libie will support Page Pool model only, so that a driver can't
> use much of the lib until it's converted. iavf is only the example, the
> rest will eventually be converted soon on a per-driver basis. That is
> when it gets really interesting. Stay tech.
> 
> Alexander Lobakin (10):
>    net: intel: introduce {,Intel} Ethernet common library
>    iavf: kill "legacy-rx" for good
>    iavf: drop page splitting and recycling
>    slab: introduce kvmalloc_array_node() and kvcalloc_node()
>    page_pool: constify some read-only function arguments
>    page_pool: add DMA-sync-for-CPU inline helper
>    libeth: add Rx buffer management
>    iavf: pack iavf_ring more efficiently
>    iavf: switch to Page Pool
>    MAINTAINERS: add entry for libeth and libie
> 
>   MAINTAINERS                                   |  20 +
>   drivers/net/ethernet/intel/Kconfig            |   7 +
>   drivers/net/ethernet/intel/libeth/Kconfig     |   9 +
>   drivers/net/ethernet/intel/libie/Kconfig      |  10 +
>   drivers/net/ethernet/intel/Makefile           |   3 +
>   drivers/net/ethernet/intel/libeth/Makefile    |   6 +
>   drivers/net/ethernet/intel/libie/Makefile     |   6 +
>   include/net/page_pool/types.h                 |   4 +-
>   .../net/ethernet/intel/i40e/i40e_prototype.h  |   7 -
>   drivers/net/ethernet/intel/i40e/i40e_type.h   |  88 ---
>   drivers/net/ethernet/intel/iavf/iavf.h        |   2 +-
>   .../net/ethernet/intel/iavf/iavf_prototype.h  |   7 -
>   drivers/net/ethernet/intel/iavf/iavf_txrx.h   | 146 +----
>   drivers/net/ethernet/intel/iavf/iavf_type.h   |  90 ---
>   .../net/ethernet/intel/ice/ice_lan_tx_rx.h    | 320 ----------
>   include/linux/net/intel/libie/rx.h            |  50 ++
>   include/linux/slab.h                          |  17 +-
>   include/net/libeth/rx.h                       | 242 ++++++++
>   include/net/page_pool/helpers.h               |  34 +-
>   drivers/net/ethernet/intel/i40e/i40e_common.c | 253 --------
>   drivers/net/ethernet/intel/i40e/i40e_main.c   |   1 +
>   drivers/net/ethernet/intel/i40e/i40e_txrx.c   |  72 +--
>   drivers/net/ethernet/intel/iavf/iavf_common.c | 253 --------
>   .../net/ethernet/intel/iavf/iavf_ethtool.c    | 140 -----
>   drivers/net/ethernet/intel/iavf/iavf_main.c   |  40 +-
>   drivers/net/ethernet/intel/iavf/iavf_txrx.c   | 551 +++---------------
>   .../net/ethernet/intel/iavf/iavf_virtchnl.c   |  17 +-
>   drivers/net/ethernet/intel/ice/ice_main.c     |   1 +
>   drivers/net/ethernet/intel/ice/ice_txrx_lib.c | 111 +---
>   drivers/net/ethernet/intel/libeth/rx.c        | 150 +++++
>   drivers/net/ethernet/intel/libie/rx.c         | 124 ++++
>   net/core/page_pool.c                          |  10 +-
>   32 files changed, 836 insertions(+), 1955 deletions(-)
>   create mode 100644 drivers/net/ethernet/intel/libeth/Kconfig
>   create mode 100644 drivers/net/ethernet/intel/libie/Kconfig
>   create mode 100644 drivers/net/ethernet/intel/libeth/Makefile
>   create mode 100644 drivers/net/ethernet/intel/libie/Makefile
>   create mode 100644 include/linux/net/intel/libie/rx.h
>   create mode 100644 include/net/libeth/rx.h
>   create mode 100644 drivers/net/ethernet/intel/libeth/rx.c
>   create mode 100644 drivers/net/ethernet/intel/libie/rx.c
> 
> ---
> libeth has way more generic functionality and code in the idpf XDP
> tree[0], take a look if you want to have more complete picture of
> what this really is about.
> 
>  From v9[1]:
> * pick Acked-by from Vlastimil and a couple Reviewed-by from Przemek;

thanks for the updates too!
I've read the code ~two times across the life of this series, nothing
bad spot, so for the series:
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>

> * mention that the libeth_fq::fp kernel-doc generates a warning and the
>    fix for that is pending on the linux-doc ML (Jakub);
> * no functional changes.

// ...