From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-qt1-f169.google.com (mail-qt1-f169.google.com [209.85.160.169]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 47E3A3FB22E for ; Thu, 8 Jan 2026 11:33:15 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=pass smtp.client-ip=209.85.160.169 ARC-Seal:i=2; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1767871999; cv=pass; b=uK87pxwaW9T0xEpRb2+kjsR0ZSGPNzHcwLmCwMookz1UA6Q4hopx5jHoFn/CX/DhnCZCdyERNvaoDF9t2rdSxwwAFN/bfL49aNNHw2FpZNJ9Bkub/G1WG+vt1n6IpUynGpGv4VH0avLNj5+hXgCcdHPYwW2DZDikJ62zs7/5TLk= ARC-Message-Signature:i=2; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1767871999; c=relaxed/simple; bh=ZZ7jjc2Z7MlEyWpqG25mdLQhus67sDNziV9lhVgNUPg=; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject: To:Cc:Content-Type; b=LbvsYrB6scyGFWRplKdSDMPMCN5XCiUd9H2q2rdyo/VWg/EtJ2qEFvlcx0nVOPxn8z3HD5oIhASdZlNUOmcVEpUbtNRSeT/X50KNzUGZUY7CkUeONYHcg4HwqfBzBeZCfImN++agi88HiaxlSdTFpsAgN9WiDpidw86rnTMo6qE= ARC-Authentication-Results:i=2; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com; spf=pass smtp.mailfrom=google.com; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b=v1qbVaSl; arc=pass smtp.client-ip=209.85.160.169 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=google.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="v1qbVaSl" Received: by mail-qt1-f169.google.com with SMTP id d75a77b69052e-4ee243b98caso794331cf.1 for ; Thu, 08 Jan 2026 03:33:14 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1767871993; cv=none; d=google.com; s=arc-20240605; b=jH559X9aSX4Y2dz1PbNpQt4kRzh1hRXFtdo1miGs6eLrWRBhs43MBNreB2cR9na+db uT75jCQKsMBupnI3MIZbBaKE3m28eN2XIgXiCLGcpcvFUx6DQJ0RdSnQRdFW87V02NmO 8XgFdEEluYecQNfUPveYJXIg+i2YztBYC15KklcfS47VthNlyGb22Yf49+dF47r1Pnnc sZMTybxFoaw7rY+V4+aEo/MW11krrmaEvPNGcu75nPIyZtWr80d3bt/HjOns3hj4RWwb tA/jspec9YqNGzRgJq0Tb1nZGWur/qOOdQQf5UvQ5uob2SB5G5HpdK4KhSoYLPWY3GuK m1Vw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20240605; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:dkim-signature; bh=6uOaQcEvqH/Hd3NqzripTRIq0Qw/v1bGv7C8UVSeJmM=; fh=f26YjzTpfPwnE6Gb730oHDIxrDTO44yQoLUDMNl0YfE=; b=NPwJL5b1yi1YN2XPLNf2ZWE30jYV/8FeyDSIUQDUYBw77LXjxfK2tBk+iaxPVWYgJa tVSAkngxd3ZgNTQdxBy2/2oO2GrXE1b6+cjkC/ic42GZW//m2LsyEj/q++336Bd1JxAL nixkvgiFS2Dk9DM/T/lYPE3xdkSAq8UF1OBoZCycWTVA+jwFgLu49JOe9dlptvjuug7/ /QltBlesUTXAWPoYpXYRwxFfOgOlN8cptHOZYc1R9KZ54bMp2Ssl0SqgIAL+3yNrqjhw rV1w4bdJ8BbtozfRn0MFbJIx/ddg2KS34wOsXkryO8q2A28aAkCK2tJVGze5kNKNIu3b Ly5g==; darn=vger.kernel.org ARC-Authentication-Results: i=1; mx.google.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1767871993; x=1768476793; darn=vger.kernel.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=6uOaQcEvqH/Hd3NqzripTRIq0Qw/v1bGv7C8UVSeJmM=; b=v1qbVaSlXZO5Dg/96VIhv1YCX88SQkORRImgXHLREXnEDjcsrXdD672O+v7TLngaUj pP6hDfODCi8SaWuxXypDLaoI6U1hbQ8rTuaXAHgAb+AlKpthnwPZ3/LEp0lhxgqCOU1a UyRucnw9ajJF5UzwVrFGp8jVVuaBT64w6WFFJLoMwUjL+b0NjnX+YHz8eqLS5d2+P1Ti raGEd5/mWybXqSSBxHaypvZRzdOXhSi9J77dN5uLypASQcnXaW2DjX2QGojnJiMowYU7 TcGKTftXEhGv7nZjuxEFDezDrXSDirL+XOIZ1R27CiinpCVslxlBXIRIO7dL23OPEMLt LTWg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1767871993; x=1768476793; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-gg:x-gm-message-state:from :to:cc:subject:date:message-id:reply-to; bh=6uOaQcEvqH/Hd3NqzripTRIq0Qw/v1bGv7C8UVSeJmM=; b=gYhV2xHnqUSyydzTMdKxco4JNbfmKHBpIkc/wSxeQP+uY6tA+4N9Jd1I6CyYj6yV+h LRxqEGZX0X/C5U7qtXFrQpsWiPt+rLnpxN0SVZCC67ZXFpE8fPV5pdfXwUbKO3MJE7kW adUoGLAduXgIwHftA8+uVl1+VNanq17dDMPP4IPZ8p24QtgaJvphgfd4dpeOJUzG8eeK ZtqPcC3SHs0fkx7bu6nKVzPXO+nx/tg0gvcaDjS+OFjCMmKNVYY9sE9k8SGWZwcI2PJG rs9RBTpmkksUUWqo/rq9txnw/HbRyNAjb89sfpQhLMOXJo+b6JtINpvR1sHVar9gtAM2 WJ/Q== X-Forwarded-Encrypted: i=1; AJvYcCVL/lRzadhoAusdqj2kJjI0Q3IA8zPb6egRx/Lp7gZr8bkyxCCOF+8m9cu13zcBy4LpMN9u/L/0JWyZeKs=@vger.kernel.org X-Gm-Message-State: AOJu0YzQz5a984h1S+tpCGtCwGRirGsku1TZ4AMdl+lmFaBQcuINtYFk MTo71qVwdNY1/xDesRBLQcdluTzoRqwNvhoTd9yrbu8Ant6Rh/lnuJPkH8xYk7IDDO0iMbOQx9u k4+7pcqs+aP/MZHJHMzvlMUT9oZeVXQWXy0p7cnCj X-Gm-Gg: AY/fxX7iEmEREn6nSQ/u7DTWxT/T0VC4ykICqrzG+V3z6mQrS8MuWIFUdKcCYeKrrb4 QE4JWHibCcmr5KRtGeXNg8gPmtcC2W2bnn70AYcZ75CQf//DEEwWTP+xZ1YEZbAwUCD/HdRVS3g PBWN4o79Y7k36fvsBa84fHyMPe56YfMCAKuKxE52lRX/ES8tL9yj7xw0gZxjhgzjCjrW01WTbj/ XdhIu8qvvr+PaTGKA9pYdumonjHcIbAoGp5mnJoNoWAVAo2WJ7PyMix0SpL7M0MFHL6CwdNAIkE GIa53ot16MX3ZmlwpZhfJ6MYKAOjHw== X-Received: by 2002:ac8:5ac3:0:b0:4ed:ff77:1a85 with SMTP id d75a77b69052e-4ffc0a77967mr7353911cf.17.1767871992772; Thu, 08 Jan 2026 03:33:12 -0800 (PST) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 References: <20260106162200.2223655-1-smostafa@google.com> <20260106162200.2223655-4-smostafa@google.com> In-Reply-To: From: Mostafa Saleh Date: Thu, 8 Jan 2026 11:33:01 +0000 X-Gm-Features: AQt7F2pw41zErLif-3uwryiAOrSOrZpnIkyTexwr9_jBpysRxSFYAMim5EQTGb8 Message-ID: Subject: Re: [PATCH v5 3/4] iommu: debug-pagealloc: Track IOMMU pages To: Pranjal Shrivastava Cc: linux-mm@kvack.org, iommu@lists.linux.dev, linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org, corbet@lwn.net, joro@8bytes.org, will@kernel.org, robin.murphy@arm.com, akpm@linux-foundation.org, vbabka@suse.cz, surenb@google.com, mhocko@suse.com, jackmanb@google.com, hannes@cmpxchg.org, ziy@nvidia.com, david@redhat.com, lorenzo.stoakes@oracle.com, Liam.Howlett@oracle.com, rppt@kernel.org, xiaqinxin@huawei.com, baolu.lu@linux.intel.com, rdunlap@infradead.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Thu, Jan 8, 2026 at 11:06=E2=80=AFAM Mostafa Saleh = wrote: > > On Wed, Jan 07, 2026 at 03:21:41PM +0000, Pranjal Shrivastava wrote: > > On Tue, Jan 06, 2026 at 04:21:59PM +0000, Mostafa Saleh wrote: > > > Using the new calls, use an atomic refcount to track how many times > > > a page is mapped in any of the IOMMUs. > > > > > > For unmap we need to use iova_to_phys() to get the physical address > > > of the pages. > > > > > > We use the smallest supported page size as the granularity of trackin= g > > > per domain. > > > This is important as it is possible to map pages and unmap them with > > > larger sizes (as in map_sg()) cases. > > > > > > Reviewed-by: Lu Baolu > > > Signed-off-by: Mostafa Saleh > > > --- > > > drivers/iommu/iommu-debug-pagealloc.c | 91 +++++++++++++++++++++++++= ++ > > > 1 file changed, 91 insertions(+) > > > > > > diff --git a/drivers/iommu/iommu-debug-pagealloc.c b/drivers/iommu/io= mmu-debug-pagealloc.c > > > index 1d343421da98..86ccb310a4a8 100644 > > > --- a/drivers/iommu/iommu-debug-pagealloc.c > > > +++ b/drivers/iommu/iommu-debug-pagealloc.c > > > @@ -29,19 +29,110 @@ struct page_ext_operations page_iommu_debug_ops = =3D { > > > .need =3D need_iommu_debug, > > > }; > > > > > > +static struct page_ext *get_iommu_page_ext(phys_addr_t phys) > > > +{ > > > + struct page *page =3D phys_to_page(phys); > > > + struct page_ext *page_ext =3D page_ext_get(page); > > > + > > > + return page_ext; > > > +} > > > + > > > +static struct iommu_debug_metadata *get_iommu_data(struct page_ext *= page_ext) > > > +{ > > > + return page_ext_data(page_ext, &page_iommu_debug_ops); > > > +} > > > + > > > +static void iommu_debug_inc_page(phys_addr_t phys) > > > +{ > > > + struct page_ext *page_ext =3D get_iommu_page_ext(phys); > > > + struct iommu_debug_metadata *d =3D get_iommu_data(page_ext); > > > + > > > + WARN_ON(atomic_inc_return_relaxed(&d->ref) <=3D 0); > > > + page_ext_put(page_ext); > > > +} > > > + > > > +static void iommu_debug_dec_page(phys_addr_t phys) > > > +{ > > > + struct page_ext *page_ext =3D get_iommu_page_ext(phys); > > > + struct iommu_debug_metadata *d =3D get_iommu_data(page_ext); > > > + > > > + WARN_ON(atomic_dec_return_relaxed(&d->ref) < 0); > > > + page_ext_put(page_ext); > > > +} > > > + > > > +/* > > > + * IOMMU page size doesn't have to match the CPU page size. So, we u= se > > > + * the smallest IOMMU page size to refcount the pages in the vmemmap= . > > > + * That is important as both map and unmap has to use the same page = size > > > + * to update the refcount to avoid double counting the same page. > > > + * And as we can't know from iommu_unmap() what was the original pag= e size > > > + * used for map, we just use the minimum supported one for both. > > > + */ > > > +static size_t iommu_debug_page_size(struct iommu_domain *domain) > > > +{ > > > + return 1UL << __ffs(domain->pgsize_bitmap); > > > +} > > > + > > > void __iommu_debug_map(struct iommu_domain *domain, phys_addr_t phys= , size_t size) > > > { > > > + size_t off, end; > > > + size_t page_size =3D iommu_debug_page_size(domain); > > > + > > > + if (WARN_ON(!phys || check_add_overflow(phys, size, &end))) > > > + return; > > > + > > > + for (off =3D 0 ; off < size ; off +=3D page_size) { > > > + if (!pfn_valid(__phys_to_pfn(phys + off))) > > > + continue; > > > + iommu_debug_inc_page(phys + off); > > > + } > > > +} > > > + > > > +static void __iommu_debug_update_iova(struct iommu_domain *domain, > > > + unsigned long iova, size_t size, bo= ol inc) > > > +{ > > > + size_t off, end; > > > + size_t page_size =3D iommu_debug_page_size(domain); > > > + > > > + if (WARN_ON(check_add_overflow(iova, size, &end))) > > > + return; > > > + > > > + for (off =3D 0 ; off < size ; off +=3D page_size) { > > > + phys_addr_t phys =3D iommu_iova_to_phys(domain, iova + of= f); > > > + > > > + if (!phys || !pfn_valid(__phys_to_pfn(phys))) > > > + continue; > > > + > > > + if (inc) > > > + iommu_debug_inc_page(phys); > > > + else > > > + iommu_debug_dec_page(phys); > > > + } > > > > This might loop for too long when we're unmapping a big buffer (say 1GB= ) > > which is backed by multiple 4K mappings (i.e. not mapped using large > > mappings) it may hold the CPU for too long, per the above example: > > > > 1,073,741,824 / 4096 =3D 262,144 iterations each with an iova_to_phys w= alk > > in a tight loop, could hold the CPU for a little too long and could > > potentially result in soft lockups (painful to see in a debug kernel). > > Since, iommu_unmap can be called in atomic contexts (i.e. interrupts, > > spinlocks with pre-emption disabled) we cannot simply add cond_resched(= ) > > here as well. > > > > Maybe we can cross that bridge once we get there, but if we can't solve > > the latency now, it'd be nice to explicitly document this risk > > (potential soft lockups on large unmaps) in the Kconfig or cmdline help= text? > > > > Yes, I am not sure how bad that would be, looking at the code, the closes= t > pattern I see in that path is for SWIOTLB, when it=E2=80=99s enabled it w= ill do a > lot of per-page operations on unmap. > There is a disclaimer already in dmesg and the Kconfig about the performa= nce > overhead, and you would need to enable a config + cmdline to get this, so > I=E2=80=99d expect someone enabling it to have some expectations of what = they are > doing. But I can add more info to Kconfig if that makes sense. > > > > } > > > > > > void __iommu_debug_unmap_begin(struct iommu_domain *domain, > > > unsigned long iova, size_t size) > > > { > > > + __iommu_debug_update_iova(domain, iova, size, false); > > > } > > > > > > void __iommu_debug_unmap_end(struct iommu_domain *domain, > > > unsigned long iova, size_t size, > > > size_t unmapped) > > > { > > > + if (unmapped =3D=3D size) > > > + return; > > > + > > > + /* > > > + * If unmap failed, re-increment the refcount, but if it unmapped > > > + * larger size, decrement the extra part. > > > + */ > > > + if (unmapped < size) > > > + __iommu_debug_update_iova(domain, iova + unmapped, > > > + size - unmapped, true); > > > + else > > > + __iommu_debug_update_iova(domain, iova + size, > > > + unmapped - size, false); > > > } > > > > I'm a little concerned about this part, when we unmap more than request= ed, > > the __iommu_debug_update_iova relies on > > iommu_iova_to_phys(domain, iova + off) to find the physical page to > > decrement. However, since __iommu_debug_unmap_end is called *after* the > > IOMMU driver has removed the mapping (in __iommu_unmap). Thus, the > > iommu_iova_to_phys return 0 (fail) causing the loop in update_iova: > > `if (!phys ...)` to silently continue. > > > > Since the refcounts for the physical pages in the range: > > [iova + size, iova + unmapped] are never decremented. Won't this result > > in false positives (warnings about page leaks) when those pages are > > eventually freed? > > > > For example: > > > > - A driver maps a 2MB region (512 x 4KB). All 512 pgs have refcount =3D= 1. > > > > - A driver / IOMMU-client calls iommu_unmap(iova, 4KB) > > > > - unmap_begin(4KB) calls iova_to_phys, succeeds, and decrements the > > refcount for the 1st page to 0. > > > > - __iommu_unmap calls the IOMMU driver. The driver (unable to split the > > block) zaps the entire 2MB range and returns unmapped =3D 2MB. > > > > - unmap_end(size=3D4KB, unmapped=3D2MB) sees that more was unmapped tha= n > > requested & attempts to decrement refcounts for the remaining 511 pgs > > > > - __iommu_debug_update_iova is called for the remaining range, which > > ends up calling iommu_iova_to_phys. Since the mapping was destroyed, > > iova_to_phys returns 0. > > > > - The loop skips the decrement causing the remaining 511 pages to leak > > with refcount =3D 1. > > > > Agh, yes, iova_to_phys will always return zero, so the > __iommu_debug_update_iova() will do nothing in that case. > > I am not aware which drivers are doing this, I added this logic > because I saw the IOMMU core allow it. I vaguely remember that > had something about splitting blocks which might be related to VFIO, > but I don't think that is needed anymore. > > I am happy just to drop it or even preemptively warn in that case, as > it is impossible to retrieve the old addresses. > > And maybe, that's a chance to re-evaluate we allow this behviour. > I have this, it should have the same effect + a WARN, I will include it in the new version diff --git a/drivers/iommu/iommu-debug-pagealloc.c b/drivers/iommu/iommu-debug-pagealloc.c index 5353417e64f9..64ec0795fe4c 100644 --- a/drivers/iommu/iommu-debug-pagealloc.c +++ b/drivers/iommu/iommu-debug-pagealloc.c @@ -146,16 +146,12 @@ void __iommu_debug_unmap_end(struct iommu_domain *dom= ain, if (unmapped =3D=3D size) return; - /* - * If unmap failed, re-increment the refcount, but if it unmapped - * larger size, decrement the extra part. - */ + /* If unmap failed, re-increment the refcount. */ if (unmapped < size) __iommu_debug_update_iova(domain, iova + unmapped, size - unmapped, true); else - __iommu_debug_update_iova(domain, iova + size, - unmapped - size, false); + WARN_ONCE(1, "iommu: unmap larger than requested is not supported in debug_pagealloc\n"); } void iommu_debug_init(void) Thanks, Mostafa > Thanks, > Mostafa > > > Thanks, > > Praan