Received-SPF: pass (google.com: domain of linux-kernel+bounces-150312-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <2227faf4-dfc2-4c30-bfe8-874ed1d22743@intel.com>
Date: Thu, 18 Apr 2024 16:29:53 +0200
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH net-next v10 00/10] net: intel: start The Great Code Dedup
 + Page Pool for iavf
Content-Language: en-US
To: Alexander Lobakin <aleksander.lobakin@intel.com>, "David S. Miller"
	<davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski
	<kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>
CC: Alexander Duyck <alexanderduyck@fb.com>, Yunsheng Lin
	<linyunsheng@huawei.com>, Jesper Dangaard Brouer <hawk@kernel.org>, "Ilias
 Apalodimas" <ilias.apalodimas@linaro.org>, Christoph Lameter <cl@linux.com>,
	Vlastimil Babka <vbabka@suse.cz>, Andrew Morton <akpm@linux-foundation.org>,
	<nex.sw.ncis.osdt.itp.upstreaming@intel.com>, <netdev@vger.kernel.org>,
	<intel-wired-lan@lists.osuosl.org>, <linux-mm@kvack.org>,
	<linux-kernel@vger.kernel.org>
References: <20240418113616.1108566-1-aleksander.lobakin@intel.com>
From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
In-Reply-To: <20240418113616.1108566-1-aleksander.lobakin@intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?b29kdTcvKzRqSHRqVHg1YTlLT3gwSndpTVRFWWx5amIrb09xUGZYa3ZNQVJx?=
 =?utf-8?B?WWd6KzhabXd5QjJVdXE4bHQ4dUkwVVVNeDZyUzFEeXZYUFVpNGhkZGlQR3py?=
 =?utf-8?B?MExITXl2dVl0TnNmQU9JdWp2TE5yUENwc3hxNlZTUFBsTDNXU3I1VHZLNVVR?=
 =?utf-8?B?WXR2OWtkYUo1RWN5eHhrTStGRE80UENSV25qSkJHaFdjU2N3WWRSSkRmNFhX?=
 =?utf-8?B?eENkcGlJRVZEZGN1eU5jdno2c3pmc2lad2J4WCtpVVJENUpNUEtIanFyQ2dD?=
 =?utf-8?B?aU5qWldQbEprRFBDQWJmTlBoRWc0SUhQQk1uaTlodW42T0xXMU43dml4RzJq?=
 =?utf-8?B?TVJRVTlLUnlINzZ1clQwdTJlSTJmUmtIbGJCbXc2djJmUkZXUEZHMTVtanlY?=
 =?utf-8?B?TEs1b0c1WnBxY3NMb0s1TEpBelNtYStRT2xmZ25SVDVnZnVEQ2hrSTFsN0JM?=
 =?utf-8?B?aElmQ1l5TmdCRGEwZ01rcEcrN3h1Q0U1UDJGN1VNWVlldkYyR09GRUZiSXZQ?=
 =?utf-8?B?cGJETkI0cUJGK2NGcGt4blZjemtsK2N0OTBUSjUrZTZlNUpCQWtHdjV6Mk1o?=
 =?utf-8?B?aDJvUk1LOXRqa3BUUG9IVEN3aTE1M0MrSVFVL1JiM21mNmF0WXdnZDgyT3ZO?=
 =?utf-8?B?ZjFlK0MwMURsNS9HRkNLTGJPWk15UmhOWFpWN0N6UVVvUTZqUWJsNWlXSnhz?=
 =?utf-8?B?dEhEQ2dQMmxWRHFDS0tjdCtBZkZIemM5RkVmWDNzNTFsQ1NONkU2NFhDVzZC?=
 =?utf-8?B?ajJmVXpLNjA2OG1VR3hoMGx5Skk0dUF4Q2ZLMHV2bU9aYkNGWFZ0UXdYdy8v?=
 =?utf-8?B?aWFLMlJsZER3UjNPWGVveVNtVXlKcjJZTzhoWkdIaHpGVVU2OWZjTDZudzBq?=
 =?utf-8?B?dGhtYzFMZTZYaElTa3ZIUlJZVWZKRXd6L1VLRW1MODQrWkFhSHhrTUdLdk91?=
 =?utf-8?B?Sk53UzhFK2h4NE9PMlN0cEIyNEhMMU1wbGxNWGVBYktiWE41c29DVFBSUS9z?=
 =?utf-8?B?OU5uSEpvb1gxbC9YWjdPWkJQVi9lU1J2OURVRWdLaHFDK0Npa3YxbmduV1hu?=
 =?utf-8?B?WkIzdDJ1azhzajZhR3ppUzh4b1JPR05DU1FpWGRhMXYrY3ZZQmp2Q05nNHlW?=
 =?utf-8?B?TkRwbktXNjZoK05wdGhYQjFEQ0dMUi9WRTVDVHZ5RkhmeXpCVUJ6NW9sOFlZ?=
 =?utf-8?B?SXF5R0FRVjZoOGpLdEdyR3BZRnZuYUVFZ0lHZnE1M1daYk9vbW9lZlNSZW1I?=
 =?utf-8?B?YS9rYXduNnhtRE5yMWFtVkJFWGpnK05uUzd5Y0k0MmZNSGp4SjQvSmNFcDlh?=
 =?utf-8?B?V0J6U0xiZXVYdUIyZVJadkFKcmgrTUpUc1VZRlRTTFZhTmJCYm5VWjdnNnV4?=
 =?utf-8?B?ZTRmVFVLS2JMdFcxUTh5akl5U3BrdXlaZjhWWDVycTU2RnJVTDYrWVhCTm5s?=
 =?utf-8?B?SlByV05rOG9IeHE5OUw1Z2tudjE2RVJsVXZJM0tmbWd4bTl3NFRNdVJPbDh5?=
 =?utf-8?B?YVVuMkQ0RE1mVHd3R05rYVNSeDhjVXVCRWN5ZVJ6bi9XUTZsZmd5TThxYkc0?=
 =?utf-8?B?ZFNXN3E0OE5iSHBMQnUyK0RpblZIYlJCRTgxa3VjVmNGaFBDM2pFaUpMVTlU?=
 =?utf-8?B?bFpvQnR5TlhxaUp1RXh6dFJkeWxUYjRwNE1yWHZ2alVUanRRZmtjZUh6a2V2?=
 =?utf-8?B?cWVmbFloQnhlVlljUjJ1WWNsZ29Ob3dBLzVjTDdySEkwdkVCeGxuSmd6dGgy?=
 =?utf-8?B?VGExRHowQmx5MnVPK1R2NkRERzRGN1JLQ2tTVXY4NmovS1djTUJUZk5NcGM2?=
 =?utf-8?B?ejJ0SDJZQjJmeXNiNWRCcnFKMytlMTh2dmhhL2UzS2hpYWlCQjBlVTh5ZXBz?=
 =?utf-8?B?anU4STRwZFZERCs2dEpxYnVIQU1JcVlmaU1MMUplZURGUjNLWEU2dXlubTRp?=
 =?utf-8?B?OWRvSjNWeWVreHA0YkwrTWVvK3RZS3Uza2pmbHpHSTc1TWFvajBuWEp0WVlU?=
 =?utf-8?B?UVdNcUx5b01HTnp1cWdKWHJETm43cWtrMG1ZT0t6Z1lvUEszUTJ0L1JWKysv?=
 =?utf-8?B?WFNLQStGYnd2c3BGZnlwRVZUZnErZDhiUGg0MGFQTGJXWlRUOHg4VldnSVBL?=
 =?utf-8?B?M2xXRDdLbld2S1M4Ulh6cGFoWVF6NjRYTGQzWUM1Q1A3QzI0dFlEYW9wOUNS?=
 =?utf-8?B?UlE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 608c00d8-362d-4828-c7b2-08dc5fb40756
X-MS-Exchange-CrossTenant-AuthSource: CH0PR11MB8086.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Apr 2024 14:30:00.3158
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: X1/f28qJI5Gv71e98fNUioL9kCJK4wXwyhxBXV5TNsFfhZAIxZ+/t1xjdbqb29LsOtWE4F5mqYhxht8oe7dcuq04pvt9OH/H7aVF7rSi6rg=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN0PR11MB6109
X-OriginatorOrg: 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.

// ...