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 E8BCFC35274 for ; Thu, 21 Dec 2023 14:59:02 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 4ED446B008C; Thu, 21 Dec 2023 09:59:02 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 49D306B0092; Thu, 21 Dec 2023 09:59:02 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 364886B0093; Thu, 21 Dec 2023 09:59:02 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 27FAF6B008C for ; Thu, 21 Dec 2023 09:59:02 -0500 (EST) Received: from smtpin16.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 5F7F61C1756 for ; Thu, 21 Dec 2023 14:59:01 +0000 (UTC) X-FDA: 81591132882.16.02761F2 Received: from mail-oo1-f54.google.com (mail-oo1-f54.google.com [209.85.161.54]) by imf25.hostedemail.com (Postfix) with ESMTP id 5246EA002C for ; Thu, 21 Dec 2023 14:58:59 +0000 (UTC) Authentication-Results: imf25.hostedemail.com; dkim=pass header.d=soleen.com header.s=google header.b=eiWx5S8H; dmarc=none; spf=pass (imf25.hostedemail.com: domain of pasha.tatashin@soleen.com designates 209.85.161.54 as permitted sender) smtp.mailfrom=pasha.tatashin@soleen.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1703170739; 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:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=ADgCcEWTSu7tJptHDbPcOYurSOtNDMB0td089W0nlvQ=; b=V8349H9gRbfmmIN3ztRdo4lEOsdhd05s8qwenUYC5oDmfTyS8512B1z9IAwMFGNGT0mc3s OSMEnqTNfG/zppui9eNBW97+Pjwga3Ye34B0VQkH7XHALV1PX/iyPn2iGLNVCkw9qcw4rS GwS0jv1du0pSPuhXGDzZxiqofS613NY= ARC-Authentication-Results: i=1; imf25.hostedemail.com; dkim=pass header.d=soleen.com header.s=google header.b=eiWx5S8H; dmarc=none; spf=pass (imf25.hostedemail.com: domain of pasha.tatashin@soleen.com designates 209.85.161.54 as permitted sender) smtp.mailfrom=pasha.tatashin@soleen.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1703170739; a=rsa-sha256; cv=none; b=uWyUOoWkYdkO4Q15X7vWIs+Qe9naBDT140X3WCDakvwJ6q59lOS2xoxF3Antxc6rqwx8p5 mfGdLqBINWdGjXaeCOLcJRv4sXNLu9Kl9xKYxBHkj1d8G49oBnvRwuwK5vT8scaF6ZuoMb VUPh6ezZxwUDf0EGk1XXMB7YBrJkoYI= Received: by mail-oo1-f54.google.com with SMTP id 006d021491bc7-594196f5081so351453eaf.2 for ; Thu, 21 Dec 2023 06:58:59 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=soleen.com; s=google; t=1703170738; x=1703775538; darn=kvack.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=ADgCcEWTSu7tJptHDbPcOYurSOtNDMB0td089W0nlvQ=; b=eiWx5S8HCYXrlbJekXbpWCJ1scauUHrgMHKqXvpCkzvnVAA6YPx/1CXBjN607T1bmS IgupvjTjN3wVSz0/OHDuGOILw6XSI4iLpKhXG0Bi/bUU9vhK2hshuXn5tlkWF15Am2Na JP+h31ERy63jObyb9DnjE0oXijhqMx+z2hSg3184lZQmI77bWwmBAV8Q8/cky+3IH+Dx Ln74Y+w12z/4Y3d9Kkty8mr18913rfd7mDrrq49WhiPs3oj9SUCOq55Rg2grzmP/RRvS k8B24toZQY2YdCxLXq2MpzkU01thQZmQPhXwQk3UrIsf0lYky/OS9Kg25ssFzk2lysiV xnbQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1703170738; x=1703775538; h=content-transfer-encoding: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=ADgCcEWTSu7tJptHDbPcOYurSOtNDMB0td089W0nlvQ=; b=PH5oNOYcVneWny3ov2gIjnDSNq6I2aZT0GhckEu/uzAl0gJfRQ+tBDkeHgqbMsJDIB jiP47ri8vT1ptZeck+WMbHRhxMANIaaEx+cfZPQ3u9b3mlxTOQVYwbRqVSEIaphv8nJm sVhfh4zjXCPNfzUztVpXHQU97VYLIj1qvIHOncEuDScjUK5UJaWlZd2iWu70TQ8R58YF Ala6t7g+cbbRvOlH3g/0ElmOlg7/Y7YyHemfwv0b/SvTgylaqIVeJjXosSwd0e1qdFV1 2+2VvKscCRVSLzmG9GlnxBjaFZkzdgpPQbCgtg+YwPBiHeUTPgLju09NAtDYjbPvveh2 TcEQ== X-Gm-Message-State: AOJu0Yy5Q/UsBf++m5Jth6nyfO5nGOgh5sCBN+aVQ6U3gK4GeYP+2iy8 NQ8mwkbfQfMCahK3badBC79vp1/p/bsD9snge/7YOQ== X-Google-Smtp-Source: AGHT+IETQwSqKPcARlSTqvGAKMyDahi+ld9dUal/ANt/1wL90IX8u4caH+uMMeSXpiFCQt+AXRLu31ANep+1kMW99Es= X-Received: by 2002:a05:6870:148e:b0:203:30c6:295 with SMTP id k14-20020a056870148e00b0020330c60295mr1281924oab.0.1703170738280; Thu, 21 Dec 2023 06:58:58 -0800 (PST) MIME-Version: 1.0 References: <20231221031915.619337-1-pasha.tatashin@soleen.com> In-Reply-To: From: Pasha Tatashin Date: Thu, 21 Dec 2023 09:58:20 -0500 Message-ID: Subject: Re: [RFC 0/3] iommu/intel: Free empty page tables on unmaps To: Matthew Wilcox Cc: akpm@linux-foundation.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, rientjes@google.com, dwmw2@infradead.org, baolu.lu@linux.intel.com, joro@8bytes.org, will@kernel.org, robin.murphy@arm.com, iommu@lists.linux.dev Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Stat-Signature: k8hmpuy96niewratqipe9robxzuz1ofm X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: 5246EA002C X-HE-Tag: 1703170739-58401 X-HE-Meta: U2FsdGVkX19XYi7r1FcJqRHi68DzteGlwPG9Et+QKLG/A39TDJ/5DQ6PCxtzhJDWNbmP+UnQNS4gs8zv/pJySL8oerTzx77ttFcMw8tf8d8AbUtQP2VqQBmDcodBFWJWuxWULUtjMdKrh5YkSlllUqpM4Ap4oAtM+qX1L0/kbhY4waQBhS0ROqt/NtsQnW1H8SDo7G1cg0jr2PbuzjX+tkBqCYqGQk3m+2/WsKo/HONx41QuwkUfABOfEcgPQQvmxZkPkMGrk3tem3X9/U9tfjXkpOoyShRvXhJTHWdBv/q/mLd82579jUCVdo/wn93yasB0UWykjO67M5OBQGkXqHBDKJfQ95DtY8n8tzIKdOHbdv9bfU79hXUbHGmKjamP4ME2Rk4mB4ImNOql61ij8xss7LXJDD252h/ujUV0f9j3oFxxxX5bNC7YOMmKBH/mKHtC6/PkJxkRttJQGH1/4q4xHD2xFUqndz/mGB45j0HCw5LXDqWOPWWBYijj7JMnO22liAfUEQyPPyZoNMXO/DCQoldk6bnp+tZvCtHhR0o1KUVnyP0+hIZTNSJpMX0meRiE9+onIhxMgm8ReTAOmGNUuqBc6gZgBbxG2cRhhGeaHVcy/KHRBr25MXl7R3nJ9N6HQXJ4cS19P2pndlo9qpokcEsxyxLS2Lc6M0p2lUSaHAbtiHvANe48li65frQjCmTvc6tTmQqh59kb4AdoauhU9H4j6zlT1bpwMUZ0Yp0BQnDGCmNb1q4mGwPPgsxLRptxSfJpA7o+hUv/qtnxCQx8x83LIx9wZNI5mjpcrFvwtDdOZf3ObwGd1iN5P93pxBF8wp+cWgUjZIi1p/rJPDKnXkrW1uJ8iQDfOqZQlEiCO/h+tpIGkVpaeGAJDsOJqLhqKzipgqveEZJ2iz6NDJf54B+N+2iincucPyrq1XpaupSutUlvLlODDNxxuWYDKgqNI0D8D+zwQs287RP vSf5chl8 54Vub0XS9qbR9hvuIbuvr0rhAENQHs4HKAlYIaJ96XVdxr2B8AK4bcEYQWa/U3RVF7IkYxU0hDMZ92Bf5EZds9/88q06gTAnhara5tMAdYf51Am8ZNrezandkyQKaq0CI2azDuqBXvg4pIHv1AzAwggX2jQ4Y4f+vLGrRMOjNE3KAK4nqcDSbXGkc47bfvTe1QVsheaGzCFS/4+TN0+pQn/eqnOcOxf/Hs6glyEtAlwvvHy7EKzI21r5/vbOMFsUQts9GBSuBACX/iiMv5mtz06Wt+7XYNu1EbfGhcHjPVfbl0R/mjmiM0oZQu7g9oo8Ru3/Gi3QgVjhMBfBiy/IiG3GaO+QU4Cs0Gygd 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: On Thu, Dec 21, 2023 at 9:06=E2=80=AFAM Matthew Wilcox wrote: > > On Thu, Dec 21, 2023 at 12:42:41AM -0500, Pasha Tatashin wrote: > > On Thu, Dec 21, 2023 at 12:13=E2=80=AFAM Pasha Tatashin > > wrote: > > > > > > On Wed, Dec 20, 2023 at 11:16=E2=80=AFPM Matthew Wilcox wrote: > > > > > > > > On Thu, Dec 21, 2023 at 03:19:12AM +0000, Pasha Tatashin wrote: > > > > > This series frees empty page tables on unmaps. It intends to be a > > > > > low overhead feature. > > > > > > > > > > The read-writer lock is used to synchronize page table, but most = of > > > > > time the lock is held is reader. It is held as a writer for short > > > > > period of time when unmapping a page that is bigger than the curr= ent > > > > > iova request. For all other cases this lock is read-only. > > > > > > > > > > page->refcount is used in order to track number of entries at eac= h page > > > > > table. > > > > > > > > Have I not put enough DANGER signs up around the page refcount? > > > > > > > > * If you want to use the refcount field, it must be used in such a= way > > > > * that other CPUs temporarily incrementing and then decrementing t= he > > > > * refcount does not cause problems. On receiving the page from > > > > * alloc_pages(), the refcount will be positive. > > > > > > > > You can't use refcount for your purpose, and honestly I'm shocked y= ou > > > > haven't seen any of your WARNings trigger. > > > > > > Hi Matthew, > > > > > > Thank you for looking at this. > > > > > > Could you please explain exactly why refcount can't be used like this= ? > > > > > > After alloc_page() refcount is set to 1, we never reduce it to 0, > > > every new entry in a page table adds 1, so we get up-to 513, that is > > > why I added warn like this: WARN_ON_ONCE(rc > 513 || rc < 2); to > > > > I guess, what you mean is that other CPUs could temporarily > > increase/decrease refcount outside of IOMMU management, do you have an > > example of why that would happen? I could remove the above warning, > > and in the worst case we would miss an opportunity to free a page > > table during unmap, not a big deal, it can be freed during another > > map/unmap event. Still better than today, where we never free them > > during unmaps. > > Both GUP-fast and the page cache will find a page under RCU protection, > inc it's refcount if not zero, check the page is still the one they were > looking for, and if not will dec the refcount again. That means if a > page has been in the page cache or process page tables and you can't > guarantee that all CPUs have been through the requisite grace periods, > you might see the refcount increased. Interesting scenario, it sounds like this could only happen for a short period of time at the beginning of the life of a page in the IOMMU Page Table. > I'm not prepared to make a guarantee that these are the only circumstance= s > under which you'll see a temporarily higher refcount than you expect. > Either currently or in the future. If you use the refcount as anything > other than a refcount, you're living dangerously. And if you think that > you'll be the one to do the last refcount put, you're not necessarily > correct (see the saga around __free_pages() which ended up as commit > e320d3012d25 fixed by 462a8e08e0e6 (which indicates the rare race does > actually happen)). > > Now, it seems like from your further explanation that the consequence > of getting this wrong is simply that you fail to free the page early. > That seems OK, but I insist that you insert some comments explaining > what is going on and why it's safe so somebody auditing uses of refcount > doesn't have to reanalyse the whole thing for themself. Or worse that > somebody working on the iommu sees this and thinks they can "improve" > on it. Yes, I can add detailed comments explaining how refcount is used here. Alternatively, I was thinking of using mapcount: >From mm_types.h: * If your page will not be mapped to userspace, you can also use the four * bytes in the mapcount union, but you must call page_mapcount_reset() * before freeing it. It sounds like we can safely use _mapcount for our needs, and do page_mapcount_reset() before freeing pages. Pasha