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 X-Spam-Level: X-Spam-Status: No, score=-5.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE, SPF_PASS,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E976EC43600 for ; Mon, 17 May 2021 08:25:51 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 97A6C61221 for ; Mon, 17 May 2021 08:25:51 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 97A6C61221 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=huawei.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 334B86B006E; Mon, 17 May 2021 04:25:51 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 2E3AA6B0070; Mon, 17 May 2021 04:25:51 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 10F976B0071; Mon, 17 May 2021 04:25:51 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0177.hostedemail.com [216.40.44.177]) by kanga.kvack.org (Postfix) with ESMTP id D3A246B006E for ; Mon, 17 May 2021 04:25:50 -0400 (EDT) Received: from smtpin13.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id 680A99086 for ; Mon, 17 May 2021 08:25:50 +0000 (UTC) X-FDA: 78150039660.13.B51606F Received: from szxga05-in.huawei.com (szxga05-in.huawei.com [45.249.212.191]) by imf26.hostedemail.com (Postfix) with ESMTP id 0C7C340002E3 for ; Mon, 17 May 2021 08:25:47 +0000 (UTC) Received: from dggems705-chm.china.huawei.com (unknown [172.30.72.59]) by szxga05-in.huawei.com (SkyGuard) with ESMTP id 4FkBvF2f8lzQpPG; Mon, 17 May 2021 16:22:17 +0800 (CST) Received: from dggpemm500005.china.huawei.com (7.185.36.74) by dggems705-chm.china.huawei.com (10.3.19.182) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2176.2; Mon, 17 May 2021 16:25:45 +0800 Received: from [127.0.0.1] (10.69.30.204) by dggpemm500005.china.huawei.com (7.185.36.74) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256) id 15.1.2176.2; Mon, 17 May 2021 16:25:44 +0800 Subject: Re: [PATCH net-next v5 3/5] page_pool: Allow drivers to hint on SKB recycling To: Ilias Apalodimas CC: Matteo Croce , , , Ayush Sawal , "Vinay Kumar Yadav" , Rohit Maheshwari , "David S. Miller" , Jakub Kicinski , Thomas Petazzoni , Marcin Wojtas , Russell King , Mirko Lindner , Stephen Hemminger , "Tariq Toukan" , Jesper Dangaard Brouer , "Alexei Starovoitov" , Daniel Borkmann , "John Fastabend" , Boris Pismenny , Arnd Bergmann , Andrew Morton , "Peter Zijlstra (Intel)" , Vlastimil Babka , Yu Zhao , Will Deacon , Fenghua Yu , Roman Gushchin , Hugh Dickins , Peter Xu , Jason Gunthorpe , Jonathan Lemon , Alexander Lobakin , Cong Wang , wenxu , Kevin Hao , Jakub Sitnicki , Marco Elver , Willem de Bruijn , Miaohe Lin , Guillaume Nault , , , , Matthew Wilcox , Eric Dumazet , David Ahern , Lorenzo Bianconi , Saeed Mahameed , Andrew Lunn , Paolo Abeni , Sven Auhagen References: <20210513165846.23722-1-mcroce@linux.microsoft.com> <20210513165846.23722-4-mcroce@linux.microsoft.com> <798d6dad-7950-91b2-46a5-3535f44df4e2@huawei.com> <212498cf-376b-2dac-e1cd-12c7cc7910c6@huawei.com> From: Yunsheng Lin Message-ID: Date: Mon, 17 May 2021 16:25:43 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.2.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit X-Originating-IP: [10.69.30.204] X-ClientProxiedBy: dggeme712-chm.china.huawei.com (10.1.199.108) To dggpemm500005.china.huawei.com (7.185.36.74) X-CFilter-Loop: Reflected Authentication-Results: imf26.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=huawei.com; spf=pass (imf26.hostedemail.com: domain of linyunsheng@huawei.com designates 45.249.212.191 as permitted sender) smtp.mailfrom=linyunsheng@huawei.com X-Rspamd-Server: rspam05 X-Rspamd-Queue-Id: 0C7C340002E3 X-Stat-Signature: tybcqi1dujcr96wk9g1uxkk4puopxgzz X-HE-Tag: 1621239947-317042 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: On 2021/5/17 14:38, Ilias Apalodimas wrote: > [...] >> >>> >>>> Maybe we can add a parameter in "struct page_pool_params" to let driver >>>> to decide if the page pool ptr is stored in page->pp while the page pool >>>> owns the page? >>> >>> Then you'd have to check the page pool config before saving the meta-data, >> >> I am not sure what the "saving the meta-data" meant? > > I was referring to struct page_pool* and the signature we store in struct > page. > >> >>> and you would have to make the skb path aware of that as well (I assume you >>> mean replace pp_recycle with this?). >> >> I meant we could set the in page->pp when the page is allocated from >> alloc_pages() in __page_pool_alloc_pages_slow() unconditionally or >> according to a newly add filed in pool->p, and only clear it in >> page_pool_release_page(), between which the page is owned by page pool, >> right? >> >>> If not and you just want to add an extra flag on page_pool_params and be able >>> to enable recycling depending on that flag, we just add a patch afterwards. >>> I am not sure we need an extra if for each packet though. >> >> In that case, the skb_mark_for_recycle() could only set the skb->pp_recycle, >> but not the pool->p. >> >>> >>>> >>>> Another thing accured to me is that if the driver use page from the >>>> page pool to form a skb, and it does not call skb_mark_for_recycle(), >>>> then there will be resource leaking, right? if yes, it seems the >>>> skb_mark_for_recycle() call does not seems to add any value? >>>> >>> >>> Not really, the driver has 2 choices: >>> - call page_pool_release_page() once it receives the payload. That will >>> clean up dma mappings (if page pool is responsible for them) and free the >>> buffer >> >> The is only needed before SKB recycling is supported or the driver does not >> want the SKB recycling support explicitly, right? >> > > This is needed in general even before recycling. It's used to unmap the > buffer, so once you free the SKB you don't leave any stale DMA mappings. So > that's what all the drivers that use page_pool call today. As my understanding: 1. If the driver is using page allocated from page allocator directly to form a skb, let's say the page is owned by skb(or not owned by anyone:)), when a skb is freed, the put_page() should be called. 2. If the driver is using page allocated from page pool to form a skb, let's say the page is owned by page pool, when a skb is freed, page_pool_put_page() should be called. What page_pool_release_page() mainly do is to make page in case 2 return back to case 1. And page_pool_release_page() is replaced with skb_mark_for_recycle() in patch 4/5 to avoid the above "case 2" -> "case 1" changing, so that the page is still owned by page pool, right? So the point is that skb_mark_for_recycle() does not really do anything about the owner of the page, it is still owned by page pool, so it makes more sense to keep the page pool ptr instead of setting it every time when skb_mark_for_recycle() is called? > >>> - call skb_mark_for_recycle(). Which will end up recycling the buffer. >> >> If the driver need to add extra flag to enable recycling based on skb >> instead of page pool, then adding skb_mark_for_recycle() makes sense to >> me too, otherwise it seems adding a field in pool->p to recycling based >> on skb makes more sense? >> > > The recycling is essentially an SKB feature though isn't it? You achieve the > SKB recycling with the help of page_pool API, not the other way around. So I > think this should remain on the SKB and maybe in the future find ways to turn > in on/off? As above, does it not make more sense to call page_pool_release_page() if the driver does not need the SKB recycling? Even if when skb->pp_recycle is 1, pages allocated from page allocator directly or page pool are both supported, so it seems page->signature need to be reliable to indicate a page is indeed owned by a page pool, which means the skb->pp_recycle is used mainly to short cut the code path for skb->pp_recycle is 0 case, so that the page->signature does not need checking? > > Thanks > /Ilias