From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6162ED711BC for ; Wed, 20 Nov 2024 16:18:06 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id F03F66B009F; Wed, 20 Nov 2024 11:18:05 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id EB36B6B009E; Wed, 20 Nov 2024 11:18:05 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id DD2CF6B009F; Wed, 20 Nov 2024 11:18:05 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id C19B46B009D for ; Wed, 20 Nov 2024 11:18:05 -0500 (EST) Received: from smtpin28.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id 79E1640D5A for ; Wed, 20 Nov 2024 16:18:05 +0000 (UTC) X-FDA: 82806979626.28.1183D3F Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf15.hostedemail.com (Postfix) with ESMTP id 8907FA0016 for ; Wed, 20 Nov 2024 16:17:10 +0000 (UTC) Authentication-Results: imf15.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf15.hostedemail.com: domain of robin.murphy@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=robin.murphy@arm.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1732119348; a=rsa-sha256; cv=none; b=tU7Jp/inVA+/g4XFLV4SFKdNE5yT5JkBEw840ljQMGERklG0tmW64snqA3LRJTxTy9ABfx d1bOHTdeKTgIgvClOpbfKH8chxwleEVWVXtyRc2NC6MYYyX4WZYpV86M/CMsbCqLyN+b2/ 8e0N9Fc5m68WBn96vsr+K/8pf+tPnrU= ARC-Authentication-Results: i=1; imf15.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf15.hostedemail.com: domain of robin.murphy@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=robin.murphy@arm.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1732119348; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=GYylPIuwW1pxjL+rxABwyggqbIas8xxuYYLTBOBb2K4=; b=QM7qNGAaCsIAOOmv72ZkgwTxwUfrznTv3McHpCvlnX4uCsIAgEQsXLjwppbbXH3tkhvVic O1HvVL8pBj34lxNT73SfsRYNVKHvVycEnk9YvwDO5FwuF3TkDiPc8Z5r6VqY5GuWwmbTGn ex2qPxGXfpYPA0LSKJml3NmFK4AVuQQ= Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 86D201480; Wed, 20 Nov 2024 08:18:32 -0800 (PST) Received: from [10.1.196.40] (e121345-lin.cambridge.arm.com [10.1.196.40]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 97A8A3F66E; Wed, 20 Nov 2024 08:18:00 -0800 (PST) Message-ID: <15f937d7-7ac1-4a7f-abcd-abfede191b51@arm.com> Date: Wed, 20 Nov 2024 16:17:59 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH RFC v4 3/3] page_pool: skip dma sync operation for inflight pages To: Yunsheng Lin , davem@davemloft.net, kuba@kernel.org, pabeni@redhat.com Cc: liuyonglong@huawei.com, fanghaiqing@huawei.com, zhangkun09@huawei.com, Alexander Duyck , Andrew Morton , IOMMU , MM , Jesper Dangaard Brouer , Ilias Apalodimas , Eric Dumazet , Simon Horman , netdev@vger.kernel.org, linux-kernel@vger.kernel.org References: <20241120103456.396577-1-linyunsheng@huawei.com> <20241120103456.396577-4-linyunsheng@huawei.com> From: Robin Murphy Content-Language: en-GB In-Reply-To: <20241120103456.396577-4-linyunsheng@huawei.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Rspamd-Queue-Id: 8907FA0016 X-Stat-Signature: 6i1nisquqnfs4gmqu54ju9qpuzxbaq76 X-Rspam-User: X-Rspamd-Server: rspam05 X-HE-Tag: 1732119430-211327 X-HE-Meta: U2FsdGVkX1/8epAbzDH9/9cq2sKmPapMoi4p+KSTrANGX1EPy7BXDDrGd4NQPL+ku7LmPFz68mgYmgRzPYMztxFQsGIai9mAKZmcL5H+PvVSrnc6MXjcfVlhdjvBee4vTeRCzhlb7jMFNg2TzlorMSd+6sgzJk2ynThWIw/xN79bhTe4Fdw1pTxUoVqcYWOewGu2XfdHngOuryrwYRDOJq5Zj4Mc5gxyODpFVkMXexH8zS/ep0LywOkJIE64qE8pbCFxSRp1ZHIpi22CF3m0d0BzSys5cM7B5Du7sSIqNhxeiUOT/QmEjgSmmknnTeavYfuIpQYAdeqt9s7fr0A2vPD6+QFgLHanVZelEGgyqz/IysgG6wrJLVDLq3z2KJbmKUHOWXxboRRICMK+EXXEfUypJ5NX4TGYZBJjDGFz8h9tcDgzXaZueLLOY1d00qWm/0QEVuLl7OUg00RIBPILFGAdmJVRfm+pXLfP1JyT+cS4Keoj9NQD42NN/he31El2OIetFjicH5RdQ2vxXg1MEBsV+iU7hFx/VrR1j0EgRXirhj/A8atPsDJ4Gs7b31bSBZkslyCv44MlfKT7zeAQco1ByjWNec5BcVW/ZMy5JlF6rK69BuWFgj4ytfyi1M30Q+JwL8PGXUOZ3dmw6mhc+JPnAuXv325Rt58p3lqKv9eObGaLFWYxWxq38MyvUXhdLZ7vviqpfovAeYvYfPgh5l9RMB+uYUIATLcG8pc32W6ZtQbFz72E8V6FM+AZtUN73fdsdUMOQkZti6VEJG5gm8i1+NwPxJmOnUQaokL6UX2e/8LzPnACkxf/YE6J2Bbc9lXDuWo/PWUK0smdDwd3fGU5Kax6mxYMFNrIjbkiVFG1YIEYvdYUj6pkJ4SYgmsBKBnurDENghLYw5s/Xrwh1g5NCyQhITFEmjSBs5se33c0A/cY+JNf1KdFHFCqhUCvq/JmsSOCHDx4QlPXmvy 6M3SomKX 34Es3p1v04wjwXvEOWtFiegIYO75NPvY6Y7RvNjoD8MoJ3ddr2yz5+NnvKE5MZsoGb5TzCVOlMbOY+cdXfTrfHJSXemEIHvih0kN0VYFtmW/+UePQ9475nI216TVWLmjSENYJCO+AW2dt8rynannkMSPBXyl+jQe4liYUyI0BSFwzd4vgOQpdYCmVwyzi29CbMVmCvzidlduoFasXyNcRP/90tlsKKxmFKPjiTKzCAbJeqJCvBZ/7l/cBDEO1u77ES7hpvyg2nBQODW5Hb3Wri04eUUw3BFHwN7lKvaPPk4oJJh6aY7bLV3h3QvjWqZW3Zu5tp7yXxOk0PxsRAVf1Wy0WwTKQNJ1Egq8d351+4GPQ8jfxZxH3uuEO9pCmAHxR6PlPikUE5bQL4M/jNU5VD9ZcVyBbUn83wS3Yv2FI4nLF9Qb5oMd39wJmZOCDhKOG/VQeoU5j0btAjIxjilaDVsC6pBohWiKJqY8u/H7lf8mjsTbeoYTp4U+GyQ1BnkOuF52h X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On 20/11/2024 10:34 am, Yunsheng Lin wrote: > Skip dma sync operation for inflight pages before the > page_pool_destroy() returns to the driver as DMA API > expects to be called with a valid device bound to a > driver as mentioned in [1]. > > After page_pool_destroy() is called, the page is not > expected to be recycled back to pool->alloc cache and > dma sync operation is not needed when the page is not > recyclable or pool->ring is full, so only skip the dma > sync operation for the infilght pages by clearing the > pool->dma_sync under protection of rcu lock when page > is recycled to pool->ring to ensure that there is no > dma sync operation called after page_pool_destroy() is > returned. Something feels off here - either this is a micro-optimisation which I wouldn't really expect to be meaningful, or it means patch #2 doesn't actually do what it claims. If it really is possible to attempt to dma_sync a page *after* page_pool_inflight_unmap() has already reclaimed and unmapped it, that represents yet another DMA API lifecycle issue, which as well as being even more obviously incorrect usage-wise, could also still lead to the same crash (if the device is non-coherent). Otherwise, I don't imagine it's really worth worrying about optimising out syncs for any pages which happen to get naturally returned after page_pool_destroy() starts but before they're explicitly reclaimed. Realistically, the kinds of big server systems where reclaim takes an appreciable amount of time are going to be coherent and skipping syncs anyway. Thanks, Robin. > 1. https://lore.kernel.org/all/caf31b5e-0e8f-4844-b7ba-ef59ed13b74e@arm.com/ > CC: Robin Murphy > CC: Alexander Duyck > CC: Andrew Morton > CC: IOMMU > CC: MM > Fixes: f71fec47c2df ("page_pool: make sure struct device is stable") > Signed-off-by: Yunsheng Lin > --- > net/core/page_pool.c | 25 ++++++++++++++++++------- > 1 file changed, 18 insertions(+), 7 deletions(-) > > diff --git a/net/core/page_pool.c b/net/core/page_pool.c > index 33a314abbba4..0bde7c6c781a 100644 > --- a/net/core/page_pool.c > +++ b/net/core/page_pool.c > @@ -712,7 +712,8 @@ static void page_pool_return_page(struct page_pool *pool, netmem_ref netmem) > rcu_read_unlock(); > } > > -static bool page_pool_recycle_in_ring(struct page_pool *pool, netmem_ref netmem) > +static bool page_pool_recycle_in_ring(struct page_pool *pool, netmem_ref netmem, > + unsigned int dma_sync_size) > { > int ret; > /* BH protection not needed if current is softirq */ > @@ -723,10 +724,13 @@ static bool page_pool_recycle_in_ring(struct page_pool *pool, netmem_ref netmem) > > if (!ret) { > recycle_stat_inc(pool, ring); > - return true; > + > + rcu_read_lock(); > + page_pool_dma_sync_for_device(pool, netmem, dma_sync_size); > + rcu_read_unlock(); > } > > - return false; > + return !ret; > } > > /* Only allow direct recycling in special circumstances, into the > @@ -779,10 +783,11 @@ __page_pool_put_page(struct page_pool *pool, netmem_ref netmem, > if (likely(__page_pool_page_can_be_recycled(netmem))) { > /* Read barrier done in page_ref_count / READ_ONCE */ > > - page_pool_dma_sync_for_device(pool, netmem, dma_sync_size); > - > - if (allow_direct && page_pool_recycle_in_cache(netmem, pool)) > + if (allow_direct && page_pool_recycle_in_cache(netmem, pool)) { > + page_pool_dma_sync_for_device(pool, netmem, > + dma_sync_size); > return 0; > + } > > /* Page found as candidate for recycling */ > return netmem; > @@ -845,7 +850,7 @@ void page_pool_put_unrefed_netmem(struct page_pool *pool, netmem_ref netmem, > > netmem = > __page_pool_put_page(pool, netmem, dma_sync_size, allow_direct); > - if (netmem && !page_pool_recycle_in_ring(pool, netmem)) { > + if (netmem && !page_pool_recycle_in_ring(pool, netmem, dma_sync_size)) { > /* Cache full, fallback to free pages */ > recycle_stat_inc(pool, ring_full); > page_pool_return_page(pool, netmem); > @@ -903,14 +908,18 @@ void page_pool_put_page_bulk(struct page_pool *pool, void **data, > > /* Bulk producer into ptr_ring page_pool cache */ > in_softirq = page_pool_producer_lock(pool); > + rcu_read_lock(); > for (i = 0; i < bulk_len; i++) { > if (__ptr_ring_produce(&pool->ring, data[i])) { > /* ring full */ > recycle_stat_inc(pool, ring_full); > break; > } > + page_pool_dma_sync_for_device(pool, (__force netmem_ref)data[i], > + -1); > } > recycle_stat_add(pool, ring, i); > + rcu_read_unlock(); > page_pool_producer_unlock(pool, in_softirq); > > /* Hopefully all pages was return into ptr_ring */ > @@ -1200,6 +1209,8 @@ void page_pool_destroy(struct page_pool *pool) > if (!page_pool_release(pool)) > return; > > + pool->dma_sync = false; > + > /* Paired with rcu lock in page_pool_napi_local() to enable clearing > * of pool->p.napi in page_pool_disable_direct_recycling() is seen > * before returning to driver to free the napi instance.