From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id E3DE913A3F2; Mon, 12 Aug 2024 17:57:35 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1723485456; cv=none; b=LBhpTtMbWfM2vQeCTBAWP9/lwHB2GxkSFngMO70cJnRmmTabkwCR6B5jG/6dFSJ3nlZU5L8FnNuMBEU9cDYZBROWjxf9tIsc+QHmQ5+Ucwj9vaqKBevzTPyZ+3hjzLt079lfERf2fTJEkWkD8e9Q1YHd1ACXr5eYdnitT6HBKoI= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1723485456; c=relaxed/simple; bh=PjEd0BdogsUTaZfCUMRZ31Ra4P2TNDube5tSGJ9f9sw=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=MloUkPoBGISIjK0uzF5sGypuqpOgnLmJKvGEYPxG7r3O+Yyzvt9nO3F3IfA1DmfB1LZnnfV2AkQ/cyT9F4blXzmQh8LplYDgUphOqchY4/684dZP101Eho8QCcO4oktwK4H5E7CZnzdJjPEiBLX/hS4TaJgsx1+Dmdwyb9BipeI= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=G9pSXQbI; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="G9pSXQbI" Received: by smtp.kernel.org (Postfix) with ESMTPSA id A5E1DC32782; Mon, 12 Aug 2024 17:57:33 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1723485455; bh=PjEd0BdogsUTaZfCUMRZ31Ra4P2TNDube5tSGJ9f9sw=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=G9pSXQbIwNKKdmOVG04ccy/1dIHLraWQ0G+g5z/MctkmVUUUXo81tnVQfF1RR+c1J Cvxt8qGSPA8JqYgFK4Wsisbb0RFx5KEJ2r00aZDsq7jtURbAetSPgcZbzv7tLhq3In X8cW65UhPResK30p4YVjVef/gIBciCKXIICBbwOv+MOkXOxEPR0kGDviaLOmGrQHjv 1kVsTsVsho3L4Id+uxmWhHD1b/Ou3noRjVmbmLBl93blSGZxY3SAluidKUwiBL8YOv TqI1IrR/S8Qznu2rcbJ2YiWkSSF8CbSPSyNf8gpJaBr19k4+X1N8VCNVUteNnwEgHU 0shtOIic2eEhw== Date: Mon, 12 Aug 2024 10:57:32 -0700 From: Jakub Kicinski To: Pavel Begunkov Cc: Mina Almasry , netdev@vger.kernel.org, linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org, linux-alpha@vger.kernel.org, linux-mips@vger.kernel.org, linux-parisc@vger.kernel.org, sparclinux@vger.kernel.org, linux-trace-kernel@vger.kernel.org, linux-arch@vger.kernel.org, linux-kselftest@vger.kernel.org, bpf@vger.kernel.org, linux-media@vger.kernel.org, dri-devel@lists.freedesktop.org, Donald Hunter , "David S. Miller" , Eric Dumazet , Paolo Abeni , Jonathan Corbet , Richard Henderson , Ivan Kokshaysky , Matt Turner , Thomas Bogendoerfer , "James E.J. Bottomley" , Helge Deller , Andreas Larsson , Jesper Dangaard Brouer , Ilias Apalodimas , Steven Rostedt , Masami Hiramatsu , Mathieu Desnoyers , Arnd Bergmann , Steffen Klassert , Herbert Xu , David Ahern , Willem de Bruijn , Shuah Khan , Sumit Semwal , Christian =?UTF-8?B?S8O2bmln?= , Bagas Sanjaya , Christoph Hellwig , Nikolay Aleksandrov , Taehee Yoo , David Wei , Jason Gunthorpe , Yunsheng Lin , Shailend Chand , Harshitha Ramamurthy , Shakeel Butt , Jeroen de Borst , Praveen Kaligineedi , Willem de Bruijn , Kaiyuan Zhang Subject: Re: [PATCH net-next v18 07/14] memory-provider: dmabuf devmem memory provider Message-ID: <20240812105732.5d2845e4@kernel.org> In-Reply-To: <48f3a61f-9e04-4755-b50c-8fae6e6112eb@gmail.com> References: <20240805212536.2172174-1-almasrymina@google.com> <20240805212536.2172174-8-almasrymina@google.com> <20240806135924.5bb65ec7@kernel.org> <20240808192410.37a49724@kernel.org> <20240809205236.77c959b0@kernel.org> <48f3a61f-9e04-4755-b50c-8fae6e6112eb@gmail.com> Precedence: bulk X-Mailing-List: linux-doc@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit On Sun, 11 Aug 2024 22:51:13 +0100 Pavel Begunkov wrote: > > I think we're talking about 2 slightly different flags, AFAIU.> > > Pavel and I are suggesting the driver reports "I support memory > > providers" directly to core (via the queue-api or what not), and we > > check that flag directly in netdev_rx_queue_restart(), and fail > > immediately if the support is not there. > > I might've misread Jakub, but yes, I believe it's different. It'd > communicate about support for providers to upper layers, so we can > fail even before attempting to allocate a new queue and init a > page pool. Got it. Since allocating memory happens before stopping traffic I think it's acceptable to stick to a single flag. > > Jakub is suggesting a page_pool_params flag which lets the driver > > report "I support memory providers". If the driver doesn't support it > > but core is trying to configure that, then the page_pool_create will > > fail, which will cause the queue API operation > > (ndo_queue_alloc_mem_alloc) to fail, which causes > > netdev_rx_queue_restart() to fail. > > And I'm not against this way either if we explicitly get an error > back instead of trying to figure it out post-factum like by > checking the references and possibly reverting the allocation. > Maybe that's where I was confused, and that refcount thing was > suggested as a WARN_ONCE? Yup, the refcount (now: check of the page pool list) was meant as a WARN_ONCE() to catch bad drivers. > FWIW, I think it warrants two flags. The first saying that the > driver supports providers at all: > > page_pool_init() { > if (rxq->mp_params) > if (!(flags & PP_PROVIDERS_SUPPORTED)) > goto fail; > } > > And the second telling whether the driver wants to install > providers for this particular page pool, so if there is a > separate pool for headers we can set it with plain old kernel > pages. The implementation of the queue API should be resilient against failures in alloc, and not being MP capable is just a form of alloc failure. I don't see the upside of double-flag. > payload_pool = page_pool_create(rqx, PP_PROVIDERS_SUPPORTED); > header_pool = page_pool_create(rqx, PP_PROVIDERS_SUPPORTED | > PP_IGNORE_PROVIDERS); Also don't see the upside of the explicit "non-capable" flag, but I haven't thought of that. Is there any use? One important note. The flag should not be tied to memory providers but rather to netmem, IOW unreadable memory. MP is an internal detail, the important fact from the driver-facing API perspective is that the driver doesn't need struct pages. > (or invert the flag). That's assuming page_pool_params::queue is > a generic thing and we don't want to draw equivalence between > it and memory providers.