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 DC807C48297 for ; Sat, 10 Feb 2024 02:21:51 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id E19716B0071; Fri, 9 Feb 2024 21:21:50 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id DC9186B0072; Fri, 9 Feb 2024 21:21:50 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C91B56B0075; Fri, 9 Feb 2024 21:21:50 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id BB6576B0071 for ; Fri, 9 Feb 2024 21:21:50 -0500 (EST) Received: from smtpin30.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 6134C14043E for ; Sat, 10 Feb 2024 02:21:50 +0000 (UTC) X-FDA: 81774293580.30.EB37079 Received: from mail-oa1-f45.google.com (mail-oa1-f45.google.com [209.85.160.45]) by imf05.hostedemail.com (Postfix) with ESMTP id 85E44100010 for ; Sat, 10 Feb 2024 02:21:48 +0000 (UTC) Authentication-Results: imf05.hostedemail.com; dkim=none ("invalid DKIM record") header.d=soleen.com header.s=google header.b=JkR2lEi+; dmarc=none; spf=none (imf05.hostedemail.com: domain of pasha.tatashin@soleen.com has no SPF policy when checking 209.85.160.45) smtp.mailfrom=pasha.tatashin@soleen.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1707531708; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=HxzMCuaeyhn3J9cU+FUkQXDQEw1s8FDPbrSZbrZddcs=; b=6f3z4NZb1DbcnB0UVn1srQLunx6jC9xRpesm844ZqliZrO3t+G1tBj4Zd+4Cy6NhCYJx/Y 5+WGwxEq2ieoZuTrxbEelehHalW6raaoKJcs2YCxxMySPm4+GL/lIiXCt6PMP7L1oCZTfs +Vbc6at5Alc/72OirKb/DnscyNVJSlA= ARC-Authentication-Results: i=1; imf05.hostedemail.com; dkim=none ("invalid DKIM record") header.d=soleen.com header.s=google header.b=JkR2lEi+; dmarc=none; spf=none (imf05.hostedemail.com: domain of pasha.tatashin@soleen.com has no SPF policy when checking 209.85.160.45) smtp.mailfrom=pasha.tatashin@soleen.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1707531708; a=rsa-sha256; cv=none; b=Cex5WQWmQ8QIOPXVv0TSXzx3Re5JZheo2q2u9DlaVAfu8UJrgn3x1BZe90EJJbELOj9LCI hC9Wpm45FLoVO3Y9dJdh+k8dJY/4PhpB5+GTjUX3oxcXfNfi7KWHpkewOwlwYvQ2lkaSlp H61XP6Kz2/+ZJQBksv+VJqyyCkW5BCQ= Received: by mail-oa1-f45.google.com with SMTP id 586e51a60fabf-214c940145bso848679fac.1 for ; Fri, 09 Feb 2024 18:21:48 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=soleen.com; s=google; t=1707531707; x=1708136507; darn=kvack.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=HxzMCuaeyhn3J9cU+FUkQXDQEw1s8FDPbrSZbrZddcs=; b=JkR2lEi+5sY52mCW8dwzayZYvM2RK6+PfY/thHrEfRYJZujTMlJ1zT9YoWEqdeyo+U TGBzk2cDzQj3SzxqG9tWAvA3MBZrFgmx+mNrGlOoR7Logn4IeEzq34T6j4i2TrakGuN1 k7asohnvAJPngdq/ww0vLDdKq/BKJSzz/7v1ePikfkkJDHctW6ElFuvGx89SUztSH0gz QLFfOsZIvFUsc3XpUPY2hmrcAhYkPIve1uYPi8cZGRP+fLR7THy1TGxFBY72oN+JrbQO ql9H+acx19ksGHgn9LWnN9D4vnOhaD+kKtNw3xuOgz00XkUs4Zktu3Uz8T9nq0z0as2m Y9zg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1707531707; x=1708136507; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=HxzMCuaeyhn3J9cU+FUkQXDQEw1s8FDPbrSZbrZddcs=; b=paMA2qyHyk4Ks9h+hgZmQS/CUxDJsdWTmoeOF/+INNFo99zf8azI0pztHQKkRHTIfp /3Qk8KwR9/DnBRWciAiSuhcTUNmi0qg6bVR0crnE3aJPF498QpOIOYd11mqkYec3xhH8 XMjB4OPzahN/Sq1kn1/0DsRpyjMMr03vpH3Ka26n3xBdDcCle/cPFJn7osucwn0OAKKU z4hhkDXqy7CQq96n9lTQoaoQ18e1xhxbq36PYqvTeTvMu49VIfRX1fc8uMktFgzKYCh8 YYG/ZsBoIQ9VOzvpomPS/2eYMl0K9Tbew1GyPXiKxix8urASZlZI82skaOGDAWOZCzYU fAbQ== X-Forwarded-Encrypted: i=1; AJvYcCWhzdvAQ9piYtwSsIAhgU6uboObDedG0+3+s12W/zdhw9zjYlVGAd+YVyYTGZONFBlL41QCYg40rA2zuo9ZsuUyI5E= X-Gm-Message-State: AOJu0YyBNXUtfIPrGfd1odpqyfVgJtei4QjS8L4xZ4ds5LjvR6cGqYmY cuAPoezO8iY6pugEEYiNRowDfhEoJf1YUL6U4wg0tm/aaHJ1Qwuu4NYeb13LYxmzCxQ3O+N/PV+ LOzROSIMGUTCYrTTkPXGaHu0zUfVKX/45vBCa3A== X-Google-Smtp-Source: AGHT+IFmCDzBaOTRJEg/f8FLkRS6yQpqqh3yX5ZW6I0WCsOQ72Tibv6RAzofMrIiMT9QniCTSK/5rtfk2WW8zRu6CiM= X-Received: by 2002:a05:6870:211:b0:218:f001:10bf with SMTP id j17-20020a056870021100b00218f00110bfmr1150766oad.35.1707531707564; Fri, 09 Feb 2024 18:21:47 -0800 (PST) MIME-Version: 1.0 References: <20240207174102.1486130-1-pasha.tatashin@soleen.com> <20240207174102.1486130-2-pasha.tatashin@soleen.com> <8ce2cd7b-7702-45aa-b4c8-25a01c27ed83@arm.com> In-Reply-To: <8ce2cd7b-7702-45aa-b4c8-25a01c27ed83@arm.com> From: Pasha Tatashin Date: Fri, 9 Feb 2024 21:21:10 -0500 Message-ID: Subject: Re: [PATCH v4 01/10] iommu/vt-d: add wrapper functions for page allocations To: Robin Murphy Cc: akpm@linux-foundation.org, alim.akhtar@samsung.com, alyssa@rosenzweig.io, asahi@lists.linux.dev, baolu.lu@linux.intel.com, bhelgaas@google.com, cgroups@vger.kernel.org, corbet@lwn.net, david@redhat.com, dwmw2@infradead.org, hannes@cmpxchg.org, heiko@sntech.de, iommu@lists.linux.dev, jernej.skrabec@gmail.com, jonathanh@nvidia.com, joro@8bytes.org, krzysztof.kozlowski@linaro.org, linux-doc@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, linux-rockchip@lists.infradead.org, linux-samsung-soc@vger.kernel.org, linux-sunxi@lists.linux.dev, linux-tegra@vger.kernel.org, lizefan.x@bytedance.com, marcan@marcan.st, mhiramat@kernel.org, m.szyprowski@samsung.com, paulmck@kernel.org, rdunlap@infradead.org, samuel@sholland.org, suravee.suthikulpanit@amd.com, sven@svenpeter.dev, thierry.reding@gmail.com, tj@kernel.org, tomas.mudrunka@gmail.com, vdumpa@nvidia.com, wens@csie.org, will@kernel.org, yu-cheng.yu@intel.com, rientjes@google.com, bagasdotme@gmail.com, mkoutny@suse.com Content-Type: text/plain; charset="UTF-8" X-Rspam-User: X-Rspamd-Server: rspam12 X-Rspamd-Queue-Id: 85E44100010 X-Stat-Signature: 6nrcftdauhtaj5agbch6jfqtfftrzmow X-HE-Tag: 1707531708-613733 X-HE-Meta: U2FsdGVkX1/UtTTMYpWGBk0ZIJAp8a0TlPlFsTU/YxbB/w88tiv9EcDyerIO76fhCQgdSpthAvf20ife1sMS8OLWfd4OkE3PNRL47lBePy3RXIcnhGUaeo5C1IqAHqqsVhPU5oricrLn0NFzrcagf/PbxReEIkXMcNDDjrzEtb4XPNtgBmEBwmH2LOx9N28Tm3yHykuEaKVcbPyKjOOc+zQ1xfwvaarCQSZa06r4mhIQjS6MVFJUDeKS7G3JVfM4ZxHt1gIfbopcVuN1yB8QJaLI3+17kzluZS6Xz0MtED9ot48KxpdRu3109UtBxKJSR2gERTgfG5DAGIEj0FAYjQgkPQx6sobQ7OsEnRXxAy4QYzWkmWN8dVhmST/wzTzqZgleCyD142hfX6QBvX8/BWYDu1pI1D+gxR2CQ3CqFKvOMgWamybiZ2D0ocTXYI9TnM4W5OVwFbFvya6EuZ2T2lwgscX5FRgiAeswhfzExJ5iPdjJTHaIUVWNzOA9gPV5HWrVElB1RsGEjl6hinrtBP6ce3AXwODQ1FtotByphAx40qHYF3GQ5xC1z7MB08fdiHzkmPrF+yLoHlaV/V7nAydUv0HjdfCO6J/bp2d5JVUTQkYLk4A+KcyTXxdBlk6lNZDbN4ocGmVMiiUySWck60/TX+Hfoi2ZfhUYWlh4IknvhxlM6qVh7K3quIC5+RofLOgLDXLLM1dQiLb3D5TQt24JtZHevtWUZ8Ww2RCQb8WQOUT/qFBUMl6dhH2aZPMF1y36svqzRD/EdFVsexsXhCdS+XQoBeAGWQ0H6gJjLW1uMgpb6xiiMfwvcG40Gn7SN2lCfBbcjSl94GTVp0F7S6I71pd6305H6AQdbWSg0Gj52xgA+sTQJplxioxX+vsC6ejDoS1+Y2gIcmMQCkHjYSnsJ+m7y4RppxRrDXmWrpKQYZZiJY8UrTzOWeOqApsKAuoheeV8fm5zCq41aGv Nagh8AEQ OfFxW09oVUZSYRS/ow1Ug2UWD0d0ie2Qq8Wexb0IxpQDORfbgT/W8hhKC9KwgkkD8RG3jA3Jv3pV+8rnHXQCzX36gMlJd2eviREQjnqDL1neN1LNpCv3+BEYXgcX9VixQrkativu4u227NWkpnmxny4tiODLgBlLOJqlPN1vdnU9kHPsNWvq+NA8L8jdLvCs2WWkTdrhg429syiQ1xvL+lQOZP3PRVeAghZmS 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: Hi Robin, Thank you for reviewing this. > > +#ifndef __IOMMU_PAGES_H > > +#define __IOMMU_PAGES_H > > + > > +#include > > +#include > > +#include > > + > > +/* > > + * All page allocation that are performed in the IOMMU subsystem must use one of > > "All page allocations" is too broad; As before, this is only about > pagetable allocations, or I guess for the full nuance, allocations of > pagetables and other per-iommu_domain configuration structures which are > reasonable to report as "pagetables" to userspace. I will update the comment. > > > + * the functions below. This is necessary for the proper accounting as IOMMU > > + * state can be rather large, i.e. multiple gigabytes in size. > > + */ > > + > > +/** > > + * __iommu_alloc_pages_node - allocate a zeroed page of a given order from > > + * specific NUMA node. > > + * @nid: memory NUMA node id > > + * @gfp: buddy allocator flags > > + * @order: page order > > + * > > + * returns the head struct page of the allocated page. > > + */ > > +static inline struct page *__iommu_alloc_pages_node(int nid, gfp_t gfp, > > + int order) > > +{ > > + struct page *page; > > + > > + page = alloc_pages_node(nid, gfp | __GFP_ZERO, order); > > + if (unlikely(!page)) > > + return NULL; > > + > > + return page; > > +} > > All 3 invocations of this only use the returned struct page to trivially > derive page_address(), so we really don't need it; just clean up these > callsites a bit more. I will remove this function, and update all invocations to use iommu_alloc_pages_node() directly. > > + * __iommu_alloc_pages - allocate a zeroed page of a given order. > > + * @gfp: buddy allocator flags > > + * @order: page order > > + * > > + * returns the head struct page of the allocated page. > > + */ > > +static inline struct page *__iommu_alloc_pages(gfp_t gfp, int order) > > +{ > > + struct page *page; > > + > > + page = alloc_pages(gfp | __GFP_ZERO, order); > > + if (unlikely(!page)) > > + return NULL; > > + > > + return page; > > +} > > Same for the single invocation of this one. I kept this function, but removed __iommu_alloc_page() that depends on it. This is because tegra-smmu needs a "struct page" allocator. > > > + > > +/** > > + * __iommu_alloc_page_node - allocate a zeroed page at specific NUMA node. > > + * @nid: memory NUMA node id > > + * @gfp: buddy allocator flags > > + * > > + * returns the struct page of the allocated page. > > + */ > > +static inline struct page *__iommu_alloc_page_node(int nid, gfp_t gfp) > > +{ > > + return __iommu_alloc_pages_node(nid, gfp, 0); > > +} > > There are no users of this at all. Yes, I added it just for completeness, I will remove it. > > + * __iommu_alloc_page - allocate a zeroed page > > + * @gfp: buddy allocator flags > > + * > > + * returns the struct page of the allocated page. > > + */ > > +static inline struct page *__iommu_alloc_page(gfp_t gfp) > > +{ > > + return __iommu_alloc_pages(gfp, 0); > > +} > > + > > +/** > > + * __iommu_free_pages - free page of a given order > > + * @page: head struct page of the page > > + * @order: page order > > + */ > > +static inline void __iommu_free_pages(struct page *page, int order) > > +{ > > + if (!page) > > + return; > > + > > + __free_pages(page, order); > > +} > > + > > +/** > > + * __iommu_free_page - free page > > + * @page: struct page of the page > > + */ > > +static inline void __iommu_free_page(struct page *page) > > +{ > > + __iommu_free_pages(page, 0); > > +} > > Beyond one more trivial Intel cleanup for __iommu_alloc_pages(), these 3 > are then only used by tegra-smmu, so honestly I'd be inclined to just > open-code there page_address()/virt_to_page() conversions as appropriate > there (once again I think the whole thing could in fact be refactored to > not use struct pages at all because all it's ever ultimately doing with > them is page_address(), but that would be a bigger job so definitely > out-of-scope for this series). I removed __iommu_free_page(), but kept __iommu_free_pages() variant. > > > + > > +/** > > + * iommu_alloc_pages_node - allocate a zeroed page of a given order from > > + * specific NUMA node. > > + * @nid: memory NUMA node id > > + * @gfp: buddy allocator flags > > + * @order: page order > > + * > > + * returns the virtual address of the allocated page > > + */ > > +static inline void *iommu_alloc_pages_node(int nid, gfp_t gfp, int order) > > +{ > > + struct page *page = __iommu_alloc_pages_node(nid, gfp, order); > > + > > + if (unlikely(!page)) > > + return NULL; > > As a general point I'd prefer to fold these checks into the accounting > function itself rather than repeat them all over. For the free functions this saves a few cycles by not repeating this check again inside __free_pages(), to keep things symmetrical it makes sense to keep __iomu_free_account and __iomu_alloc_account the same. With the other clean-up there are not that many of these checks left. > > + */ > > +static inline void *iommu_alloc_page_node(int nid, gfp_t gfp) > > +{ > > + return iommu_alloc_pages_node(nid, gfp, 0); > > +} > > TBH I'm not entirely convinced that saving 4 characters per invocation > times 11 invocations makes this wrapper worthwhile :/ Let's keep them. After the clean-up that you suggested, there are fewer functions left in this file, but I think that it is cleaner to keep these remaining, as it is beneficial to easily distinguish when exactly one page is allocated vs when multiple are allocated via code search. > > + * > > + * There are no locking requirement for these pages, as they are going to be > > + * put on a free list as soon as refcount reaches 0. Pages are put on this LRU > > + * list once they are removed from the IOMMU page tables. However, they can > > + * still be access through debugfs. > > + */ > > +static inline void iommu_free_pages_list(struct list_head *page) > > Nit: I'd be inclined to call this iommu_put_pages_list for consistency. I will rename it to iommu_put_pages_list(), indeed a better name. > > > +{ > > + while (!list_empty(page)) { > > + struct page *p = list_entry(page->prev, struct page, lru); > > + > > + list_del(&p->lru); > > + put_page(p); > > + } > > +} > > I realise now you've also missed the common freelist freeing sites in > iommu-dma. Ah yes, thank you for catching that. I will fix it. Pasha