Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 17 May 2023 20:46:32 -0700
From:   Jakub Kicinski <kuba@kernel.org>
To:     Ratheesh Kannoth <rkannoth@marvell.com>
Cc:     <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <sgoutham@marvell.com>, <davem@davemloft.net>,
        <edumazet@google.com>, <pabeni@redhat.com>, <sbhatta@marvell.com>,
        <gakula@marvell.com>, <schalla@marvell.com>, <hkelam@marvell.com>
Subject: Re: [PATH net-next v1] octeontx2-pf: Add support for page pool
Message-ID: <20230517204632.5f80a7bf@kernel.org>
In-Reply-To: <20230517041511.2532997-1-rkannoth@marvell.com>
References: <20230517041511.2532997-1-rkannoth@marvell.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Wed, 17 May 2023 09:45:11 +0530 Ratheesh Kannoth wrote:
> Page pool for each rx queue enhance rx side performance
> by reclaiming buffers back to each queue specific pool. DMA
> mapping is done only for first allocation of buffers.
> As subsequent buffers allocation avoid DMA mapping,
> it results in performance improvement.
> 
> Image        |  Performance with Linux kernel Packet Generator
> ------------ | -----------------------------------------------
> Vannila      |   3Mpps
>              |
> with this    |   42Mpps
> change	     |
> ----------------------------------------------------------------
> 
> Signed-off-by: Ratheesh Kannoth <rkannoth@marvell.com>
> 

Put an extra --- here, to place the change log outside the normal
commit message.

> ChangeLog
> v0 -> v1: Removed CONFIG_PAGE_POOL #ifdefs in code
> 	  Used compound page APIs
> 	  Replaced page_pool_put_page API with page_pool_put_full_page API

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> index f9286648e45c..49df1876eca3 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> @@ -518,11 +518,36 @@ void otx2_config_irq_coalescing(struct otx2_nic *pfvf, int qidx)
>  		     (pfvf->hw.cq_ecount_wait - 1));
>  }
>  
> +static int otx2_alloc_pool_buf(struct otx2_nic *pfvf, struct otx2_pool *pool,
> +			       dma_addr_t *dma)
> +{
> +	unsigned int offset = 0;
> +	struct page *page;
> +	size_t sz;
> +
> +	sz = SKB_DATA_ALIGN(pool->rbsize);
> +	sz = ALIGN(sz, OTX2_ALIGN);
> +
> +	page = page_pool_alloc_frag(pool->page_pool, &offset, sz,
> +				    (in_interrupt() ? GFP_ATOMIC : GFP_KERNEL) |

in_interrupt() should not be used in drivers, AFAIR.
Pass the correct flags from the caller (or don't -- it seems like 
the only caller assumes softirq context already).

> +				    GFP_DMA);

GFP_DMA? Why?

> +	if (unlikely(!page)) {
> +		netdev_err(pfvf->netdev, "Allocation of page pool failed\n");

No prints on allocation errors, please, it only adds stress to 
the system. You can add a statistic if you want.

> +		return -ENOMEM;
> +	}
> +
> +	*dma = page_pool_get_dma_addr(page) + offset;
> +	return 0;
> +}

> +	pp_params.flags = PP_FLAG_PAGE_FRAG | PP_FLAG_DMA_MAP;
> +	pp_params.pool_size = numptrs;
> +	pp_params.nid = NUMA_NO_NODE;
> +	pp_params.dev = pfvf->dev;
> +	pp_params.dma_dir = DMA_FROM_DEVICE;
> +	pool->page_pool = page_pool_create(&pp_params);
> +	if (!pool->page_pool) {
> +		netdev_err(pfvf->netdev, "Creation of page pool failed\n");
> +		return -EFAULT;

EFAULT == "Bad address", doesn't sound right

> +	}
> +
>  	return 0;
>  }
-- 
pw-bot: cr