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=-3.6 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS 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 B9AC0C43461 for ; Mon, 17 May 2021 06:38:49 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 323FB606A5 for ; Mon, 17 May 2021 06:38:49 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 323FB606A5 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linaro.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 496D76B0036; Mon, 17 May 2021 02:38:48 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 446DF6B006E; Mon, 17 May 2021 02:38:48 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 272F56B0070; Mon, 17 May 2021 02:38:48 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0208.hostedemail.com [216.40.44.208]) by kanga.kvack.org (Postfix) with ESMTP id E670D6B0036 for ; Mon, 17 May 2021 02:38:47 -0400 (EDT) Received: from smtpin09.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id 782DB18084CDF for ; Mon, 17 May 2021 06:38:47 +0000 (UTC) X-FDA: 78149769894.09.5EDDEDB Received: from mail-ej1-f46.google.com (mail-ej1-f46.google.com [209.85.218.46]) by imf26.hostedemail.com (Postfix) with ESMTP id 0421D40002C9 for ; Mon, 17 May 2021 06:38:45 +0000 (UTC) Received: by mail-ej1-f46.google.com with SMTP id s22so7245719ejv.12 for ; Sun, 16 May 2021 23:38:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to; bh=hHE3Ba5BaIKMhzZEapVVGkggYuEK/rT98XnlOxk2Lk0=; b=zgp+HceZllI4jeH/IGwDMnHeDTO7kEJCoqby+N/Y9Z49mDVItFBAcsqOLPxR8wQYOl YwooHxyiZpX132mexXtCbcxXBjFFKKXCFeukOGytdRF9eVEOOddNV/RFLaPqLP0LFRys W5+9YfmVXjotu/UW/8YTJTazBKsgFJCYJ/GeRuRgKCBCNd6SCQSYELnp0GimxNotIVzK 8ktD8lduJNgDMqMiY3umUCshZ4dS96yxSG+OwmFGG5WGhjD3gH1L9158lsflUVQ0Vewx 6ivzkQNnFPB+aLsPxEi+phjfPI79eYwICrahrQYQ+Nl+2NvfTgzuIPoEjgnROteA0zmy jWsg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=hHE3Ba5BaIKMhzZEapVVGkggYuEK/rT98XnlOxk2Lk0=; b=RwepesBdwrrufmfAJmBBHwn0MnjFT4903QrutFjlRa/QyyS7rFdIuTz2tQSuVAli1T YgcuaKJ/Ik9vWGngODhiTO3J9zV2oSPH9K7xxytqYw9Oe3ULE4ij8f4rdAC+qgUcQ1Pj C3EeN1HGc/YKYXrtdLsnFPkJILV6J6rsZhrC6bZAS3DQY1Gckma9EE1ppCf3G3Os8QDm YMh17WRWH6yW6G92rh7+NeoygOdXU5SOE+oFuvSkws/SSuawTSDyNTf4a0xEr2X3pMVq D3NeC+kfa4aNCjOV/YbHnm0pjGD3VRpN0ArClDgShLCwK34OP9BlxL8p0+jsyyJZE8qS njEg== X-Gm-Message-State: AOAM530JrM6EQp1z1L2ErlV4X9+fWiF3YBcG35MqMdghVUMCeQM8jRQs UffD1QMSw58b2joQGDSRZMT+FA== X-Google-Smtp-Source: ABdhPJzHmjg2DHXQbZ9ONI9wCSF2TuCgLaYyGoyeS7Uxnvw5bFS5uVuOo+yGk3kPWMATV6GYEQ5evg== X-Received: by 2002:a17:906:594f:: with SMTP id g15mr3036208ejr.103.1621233525756; Sun, 16 May 2021 23:38:45 -0700 (PDT) Received: from enceladus ([94.69.77.156]) by smtp.gmail.com with ESMTPSA id z17sm8094191ejc.69.2021.05.16.23.38.41 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 16 May 2021 23:38:45 -0700 (PDT) Date: Mon, 17 May 2021 09:38:40 +0300 From: Ilias Apalodimas To: Yunsheng Lin Cc: Matteo Croce , netdev@vger.kernel.org, linux-mm@kvack.org, 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 , linux-kernel@vger.kernel.org, linux-rdma@vger.kernel.org, bpf@vger.kernel.org, Matthew Wilcox , Eric Dumazet , David Ahern , Lorenzo Bianconi , Saeed Mahameed , Andrew Lunn , Paolo Abeni , Sven Auhagen Subject: Re: [PATCH net-next v5 3/5] page_pool: Allow drivers to hint on SKB recycling Message-ID: 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> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Authentication-Results: imf26.hostedemail.com; dkim=pass header.d=linaro.org header.s=google header.b=zgp+HceZ; dmarc=pass (policy=none) header.from=linaro.org; spf=pass (imf26.hostedemail.com: domain of ilias.apalodimas@linaro.org designates 209.85.218.46 as permitted sender) smtp.mailfrom=ilias.apalodimas@linaro.org X-Rspamd-Server: rspam05 X-Rspamd-Queue-Id: 0421D40002C9 X-Stat-Signature: rtdf8poszaoo51nn6w6qnyj4n7uoudku X-HE-Tag: 1621233525-878781 Content-Transfer-Encoding: quoted-printable 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: [...] > >>>> by the page pool? so that we do not need to set and clear it every > >>>> time the page is recycled=E3=80=82 > >>>> > >>> > >>> If the page cannot be recycled, page->pp will not probably be set t= o begin > >>> with. Since we don't embed the feature in page_pool and we require = the > >>> driver to explicitly enable it, as part of the 'skb flow', I'd rath= er keep=20 > >>> it as is. When we set/clear the page->pp, the page is probably alr= eady in=20 > >>> cache, so I doubt this will have any measurable impact. > >> > >> The point is that we already have the skb->pp_recycle to let driver = to > >> explicitly enable recycling, as part of the 'skb flow, if the page p= ool keep > >> the page->pp while it owns the page, then the driver may only need t= o call > >> one skb_mark_for_recycle() for a skb, instead of call skb_mark_for_r= ecycle() > >> for each page frag of a skb. > >> > >=20 > > The driver is meant to call skb_mark_for_recycle for the skb and > > page_pool_store_mem_info() for the fragments (in order to store page-= >pp). > > Nothing bad will happen if you call skb_mark_for_recycle on a frag th= ough, > > but in any case you need to store the page_pool pointer of each frag = to > > struct page. >=20 > Right. Nothing bad will happen when we keep the page_pool pointer in > page->pp while page pool owns the page too, even if the skb->pp_recycle > is not set, right? Yep, nothing bad will happen. Both functions using this (__skb_frag_unref= and skb_free_head) always check the skb bit as well. >=20 > >=20 > >> Maybe we can add a parameter in "struct page_pool_params" to let dri= ver > >> to decide if the page pool ptr is stored in page->pp while the page = pool > >> owns the page? > >=20 > > Then you'd have to check the page pool config before saving the meta-= data, >=20 > 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. >=20 > > and you would have to make the skb path aware of that as well (I assu= me you > > mean replace pp_recycle with this?). >=20 > 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? >=20 > > If not and you just want to add an extra flag on page_pool_params and= be able=20 > > to enable recycling depending on that flag, we just add a patch after= wards. > > I am not sure we need an extra if for each packet though. >=20 > In that case, the skb_mark_for_recycle() could only set the skb->pp_rec= ycle, > but not the pool->p. >=20 > >=20 > >> > >> 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? > >> > >=20 > > Not really, the driver has 2 choices: > > - call page_pool_release_page() once it receives the payload. That wi= ll > > clean up dma mappings (if page pool is responsible for them) and fr= ee the > > buffer >=20 > The is only needed before SKB recycling is supported or the driver does= not > want the SKB recycling support explicitly, right? >=20 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. > > - call skb_mark_for_recycle(). Which will end up recycling the buffer= . >=20 > 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? >=20 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? Thanks /Ilias > >=20 > > If you call none of those, you'd leak a page, but that's a driver bug= . > > patches [4/5, 5/5] do that for two marvell drivers. > > I really want to make drivers opt-in in the feature instead of always > > enabling it. > >=20 > > Thanks > > /Ilias > >> > >>> > >>>>> + page_pool_put_full_page(pp, virt_to_head_page(data), false); > >>>>> + > >>>>> C(end); > >>> > >>> [...] > >> > >> > >=20 > > . > >=20 >=20