netdev - Re: [PATCH net-next v8 3/4] enic: Use the Page Pool API for RX

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <76025962-594e-442e-928f-d88ed1d73ab5@huawei.com>
Date: Thu, 6 Feb 2025 17:34:59 +0800
From: Yunsheng Lin <linyunsheng@...wei.com>
To: John Daley <johndale@...co.com>, <benve@...co.com>, <satishkh@...co.com>,
	<andrew+netdev@...n.ch>, <davem@...emloft.net>, <edumazet@...gle.com>,
	<kuba@...nel.org>, <pabeni@...hat.com>, <netdev@...r.kernel.org>
CC: Nelson Escobar <neescoba@...co.com>
Subject: Re: [PATCH net-next v8 3/4] enic: Use the Page Pool API for RX

On 2025/2/6 7:54, John Daley wrote:
> The Page Pool API improves bandwidth and CPU overhead by recycling pages
> instead of allocating new buffers in the driver. Make use of page pool
> fragment allocation for smaller MTUs so that multiple packets can share
> a page. For MTUs larger than PAGE_SIZE, adjust the 'order' page
> parameter so that contiguous pages can be used to receive the larger
> packets.
> 
> The RQ descriptor field 'os_buf' is repurposed to hold page pointers
> allocated from page_pool instead of SKBs. When packets arrive, SKBs are
> allocated and the page pointers are attached instead of preallocating SKBs.
> 
> 'alloc_fail' netdev statistic is incremented when page_pool_dev_alloc()
> fails.
> 
> Co-developed-by: Nelson Escobar <neescoba@...co.com>
> Signed-off-by: Nelson Escobar <neescoba@...co.com>
> Co-developed-by: Satish Kharat <satishkh@...co.com>
> Signed-off-by: Satish Kharat <satishkh@...co.com>
> Signed-off-by: John Daley <johndale@...co.com>
> ---
>  drivers/net/ethernet/cisco/enic/enic.h      |  3 +
>  drivers/net/ethernet/cisco/enic/enic_main.c | 33 +++++++-
>  drivers/net/ethernet/cisco/enic/enic_rq.c   | 94 ++++++++-------------
>  drivers/net/ethernet/cisco/enic/vnic_rq.h   |  2 +
>  4 files changed, 71 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cisco/enic/enic.h b/drivers/net/ethernet/cisco/enic/enic.h
> index 10b7e02ba4d0..2ccf2d2a77db 100644
> --- a/drivers/net/ethernet/cisco/enic/enic.h
> +++ b/drivers/net/ethernet/cisco/enic/enic.h
> @@ -17,6 +17,7 @@
>  #include "vnic_nic.h"
>  #include "vnic_rss.h"
>  #include <linux/irq.h>
> +#include <net/page_pool/helpers.h>
>  
>  #define DRV_NAME		"enic"
>  #define DRV_DESCRIPTION		"Cisco VIC Ethernet NIC Driver"
> @@ -158,6 +159,7 @@ struct enic_rq_stats {
>  	u64 pkt_truncated;		/* truncated pkts */
>  	u64 no_skb;			/* out of skbs */
>  	u64 desc_skip;			/* Rx pkt went into later buffer */
> +	u64 pp_alloc_fail;		/* page pool alloc failure */

why not consider adding the above to pool->alloc_stats so that other
drivers doesn't need to implement something similar?

>  };
>  
>  struct enic_wq {
> @@ -169,6 +171,7 @@ struct enic_wq {
>  struct enic_rq {
>  	struct vnic_rq vrq;
>  	struct enic_rq_stats stats;
> +	struct page_pool *pool;
>  } ____cacheline_aligned;
>  
>  /* Per-instance private data structure */
> diff --git a/drivers/net/ethernet/cisco/enic/enic_main.c b/drivers/net/ethernet/cisco/enic/enic_main.c
> index 1d9f109346b8..447c54dcd89b 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_main.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_main.c
> @@ -1736,6 +1736,17 @@ static int enic_open(struct net_device *netdev)
>  	struct enic *enic = netdev_priv(netdev);
>  	unsigned int i;
>  	int err, ret;
> +	unsigned int max_pkt_len = netdev->mtu + VLAN_ETH_HLEN;
> +	struct page_pool_params pp_params = {
> +		.order = get_order(max_pkt_len),
> +		.pool_size = enic->config.rq_desc_count,
> +		.nid = dev_to_node(&enic->pdev->dev),
> +		.dev = &enic->pdev->dev,
> +		.dma_dir = DMA_FROM_DEVICE,
> +		.max_len = (max_pkt_len > PAGE_SIZE) ? max_pkt_len : PAGE_SIZE,

Perhaps change the checking to ((max_pkt_len << 1) > PAGE_SIZE) to avoid
overhead of dma sync operation as much as possible?

Also, there is a similar checking page_pool_alloc(), maybe add an inline
helper for that checking so that there is a consistent checking for both
page_pool core and drivers.

> +		.netdev = netdev,
> +		.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV,
> +	};