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 B4FC4FC6175 for ; Fri, 13 Sep 2024 20:31:38 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 251D86B00B3; Fri, 13 Sep 2024 16:31:38 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 202D76B00B7; Fri, 13 Sep 2024 16:31:38 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 05E1A6B00C7; Fri, 13 Sep 2024 16:31:37 -0400 (EDT) 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 D56EE6B00C5 for ; Fri, 13 Sep 2024 16:31:37 -0400 (EDT) Received: from smtpin25.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 8B65EA0AF8 for ; Fri, 13 Sep 2024 20:31:37 +0000 (UTC) X-FDA: 82560860634.25.B599A7B Received: from mail-wr1-f43.google.com (mail-wr1-f43.google.com [209.85.221.43]) by imf16.hostedemail.com (Postfix) with ESMTP id 9426D18000E for ; Fri, 13 Sep 2024 20:31:35 +0000 (UTC) Authentication-Results: imf16.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=Brqxa+nG; spf=pass (imf16.hostedemail.com: domain of aliceryhl@google.com designates 209.85.221.43 as permitted sender) smtp.mailfrom=aliceryhl@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1726259354; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to: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=hp0hM+TBi8EPvvh0ukI/PN3iFtwci/KfwR5JqGqtS98=; b=7WgLPtaE6N59ZWojJ/Br3pKsu7ZFo1+srrWBwFfcvkCQ/QwQfCSwmcHCbDUfbcFaL231Zw aEyQGHDKT/vfs1EvdDLaWEhJHgOoVqvhW8t0VcnWiQYW9SgdwiQXmz3FwphVOvLWGJa2ot 8GlEvEMzzX57Nnseaz92fHgq5EAyrbI= ARC-Authentication-Results: i=1; imf16.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=Brqxa+nG; spf=pass (imf16.hostedemail.com: domain of aliceryhl@google.com designates 209.85.221.43 as permitted sender) smtp.mailfrom=aliceryhl@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1726259354; a=rsa-sha256; cv=none; b=VEinOS21hgmIcHoMKiJtf0S7e00qL13dLD5tRp00w0C/rZPhQ/8uNITOP0gNtvVeeowj/Y 23weztVYSHZpOmKtbl6vnLQJDuNtIkiXgeAR60x8gl3mMleUOGbhda7z+PPI0fqJhdsMAn 3xjDbhz3k9zjdFmNOJTTBWZIHKP5Al0= Received: by mail-wr1-f43.google.com with SMTP id ffacd0b85a97d-371941bbfb0so1829902f8f.0 for ; Fri, 13 Sep 2024 13:31:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1726259494; x=1726864294; darn=kvack.org; h=content-transfer-encoding:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=hp0hM+TBi8EPvvh0ukI/PN3iFtwci/KfwR5JqGqtS98=; b=Brqxa+nGfS8n31wfR4PtxkW84sWnuOkCYLhLJ36gUDoIuM/Lpkpp13n/gOzOhUQp6P EfZZlM/1oMTTx3+yTrTc2k0L66jUpUfiYaJgYsNIDXtZAEWy43uV//JOo/W0H/Q3Zdrv Ggz1SaTq96IivSyVC/Bj6FtKe8sWhvcqTQdAv4gWokyKcSp2dkav0yT+ml08b1U2eonF wb4/ww/jB1f3HsFQJKuPXYk8ltRNlSQdb66zVNQTLtWzvfx+0o1/skuvWojbI1SfnU2a yYD2HSoLaKcvtI6eHPA8WTyAkC7QY2zZjIYo9DnHQOnVuW7RiMYLbYQ1sKAug9TK53Q4 Oj2Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1726259494; x=1726864294; h=content-transfer-encoding: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=hp0hM+TBi8EPvvh0ukI/PN3iFtwci/KfwR5JqGqtS98=; b=VMlZrYiE8E66+sMUQrQrSH9C44gjvNYjV78lvw1b0TFg1MNZllawh32VC3UX2EoIMc IKksrN8n0FR3q1fkR8JYAEr4qctVRhNCgbnMJO/MCdNWqt204JbjBZWVphxQ1LqhOJQ0 XTpgx+JPlyDG2OPJvX4HV8ybIPkjnTNrJoGEBoZ4Z5sSicpuItX4wWtEjR/ZzZgoLMZS kvaYHBruxurwP07wJnWmQmKv24GbHUkxYwK3te/bgkeU6QLiQqzHMY5GV9kURwEHSSaC X8HBD3V27KLEV7mTmFtTJB+pHFeQpQEbH4vbvrXEXPE51v4aGrIPItRhDR4UXUCmxtwI es3Q== X-Forwarded-Encrypted: i=1; AJvYcCU8Wc3sW+6ih6Vx1CDyIhc9y9UTt8P1QlwDC+4EKVxN4Lyr4clKOwyxqlHsYW1lqP6f5YkzrUl7aQ==@kvack.org X-Gm-Message-State: AOJu0YzJkZn8MYGiiIUN9496uZxMXM78yHRpt4jfj5PS/FBMa3gXpQ/J dLYeaKlFmLH5bav8r+FhaHQVFxWS9JYedFBw1a8lFZUPw7GOWBT/eglHyllsExbiAAD4NI6FH7K u5RpHJmjKVQdGf+djwonJyJJCwMOWnYhiwlqE X-Google-Smtp-Source: AGHT+IF1fnPp9mwLpsUqju5x2gFBE+hqrz3Dt6yYJy5S9tNWMgR4qg6InTZcC0d9XddxCBbc+zqQtwEuta3AOEXvO18= X-Received: by 2002:a5d:650c:0:b0:368:5bb4:169b with SMTP id ffacd0b85a97d-378c2cd5b95mr4775105f8f.4.1726259493423; Fri, 13 Sep 2024 13:31:33 -0700 (PDT) MIME-Version: 1.0 References: <20240912-shrinker-v1-1-18b7f1253553@google.com> In-Reply-To: From: Alice Ryhl Date: Fri, 13 Sep 2024 22:31:20 +0200 Message-ID: Subject: Re: [PATCH] rust: shrinker: add shrinker abstraction To: Alice Ryhl , Miguel Ojeda , Andrew Morton , Dave Chinner , Qi Zheng , Roman Gushchin , Muchun Song , Boqun Feng , Gary Guo , =?UTF-8?Q?Bj=C3=B6rn_Roy_Baron?= , Benno Lossin , Andreas Hindborg , Trevor Gross , linux-kernel@vger.kernel.org, linux-mm@kvack.org, rust-for-linux@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam06 X-Rspamd-Queue-Id: 9426D18000E X-Stat-Signature: 6je17ax7j9r43zojqcyjdnjzhwighzww X-Rspam-User: X-HE-Tag: 1726259495-800950 X-HE-Meta: U2FsdGVkX1+cLOU05SUKjdsFBHpF4Cd8KDod7DxHyiy3wbjzcwaujDVRZ0ncQyczp6vGRoWfMXP4488HovaG21ec87b4DJo2pswVog1NZbrCg+2hwUdGL/scKON4rXzyek31CEUSb2uNzen2K3PhGtA+In7K8Z5usWCnTlqLl5RfSIsnl27ZLRHxP7WLn2vMw3LIwTIGv/Y1UNWBFAW13LFFAh2aQcVNuKgu3Jm1G7AVBrt7gUFNsUp2rWw5twmvdrroKxlm8ypyX/0bTypQ63xvynEiEqKKtCA8euq9kANAoOcaByr1MAmwRZ29Bvi7hTzfEwAWrBXK9XtbnDAX+hoOslg5HsXBofNHysjql/oBQdoclno5LhDMwprE18lsLHBf4pehECVzP3UTqRxLVDq5/ZXm3E/LDWbokex8SEjRRrK3HGLPzwiVW4lpL36xbNhmf/n3TA0YafGe5FpM85wZKTd96Hj06Jj/vCLR+9ZAFYny65FqXEqfELGYPVE9SSme8n68P2224MuOlG+FxaEjREzVGMPtoIS1uCLlsJHUie1DSYf/oeK5UBsYqaZU6NzccWhkBWsnZdl5gm2Wmtu2lnHCJZFc0HFXx888AgyFkQlvq/GxsTzhAsn+M9FshAOXQl4olDRrYFq9yUy4d25Vnuh27k0OvwBJqLVVZyjCCZ0lwlFJnExQnN5GQQpHMPU8Su9zlAOjECmCyFW9Vdtp/ifwSK2BCk5nz8HeOiDTur2shTWbd1XeADhn4pnSXX0n2hsZJw6kfeWdCBSq868qQ/yPj4jSCLx/EQYMQyxq5bW1iBeRmaJ/i8F1q5aR+QUY/pS/29k5TxGrqXvcY1zTKfnDnCGBsLMSxLbs6OlkpnraHGvdniBl3OqJ1uM8cK9bntRCOAuWQVrakR8JELIUAETybIaQkhd3hgvH2X2ZAm3+utmTTtwebD2Mn+Y17XBp9pPzqlCW/kK9IgD 9wzKoROM qsIE/rbLKYUYDddVI2SlAWgKySlKJW9WjjxlB0awPZlNUxyB4TYWeoYw/Gm4cmYeG8Bag7CGBwRDcjF0FaMGR7XKodOdRkWq2MrkAGdQ13q9EFofqOfvHxbmNuytDry77zP8I+QYo5ZYOe2QisPFhJ8vXHDo0dcApBaLlfc3SMGnWis8pAtvCkQvgtU87tQZkMikvpcAc00xtb9XMRl8hAGA0wCca28QRcyHY6A/NnbKULeJmFuJghATap2Rv7GPAUHF5rgonCxcMAH/VpJdOAymUoBK/tTp5iooKkOCWLowjLBCLBkDJtOVJffhcn7rNulDsCGd8RX0yOiyC5pRKBXhrSryzQgrW78ge 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 Fri, Sep 13, 2024 at 10:15=E2=80=AFPM Simona Vetter wrote: > > On Thu, Sep 12, 2024 at 09:54:01AM +0000, Alice Ryhl wrote: > > Rust Binder holds incoming transactions in a read-only mmap'd region > > where it manually manages the pages. These pages are only in use until > > the incoming transaction is consumed by userspace, but the kernel will > > keep the pages around for future transactions. Rust Binder registers a > > shrinker with the kernel so that it can give back these pages if the > > system comes under memory pressure. > > > > Separate types are provided for registered and unregistered shrinkers. > > The unregistered shrinker type can be used to configure the shrinker > > before registering it. Separating it into two types also enables the > > user to construct the private data between the calls to `shrinker_alloc= ` > > and `shrinker_register` and avoid constructing the private data if > > allocating the shrinker fails. > > > > The user specifies the callbacks in use by implementing the Shrinker > > trait for the type used for the private data. This requires specifying > > three things: implementations for count_objects and scan_objects, and > > the pointer type that the private data will be wrapped in. > > > > The return values of count_objects and scan_objects are provided using > > new types called CountObjects and ScanObjects respectively. These types > > prevent the user from e.g. returning SHRINK_STOP from count_objects or > > returning SHRINK_EMPTY from scan_objects. > > > > Signed-off-by: Alice Ryhl > > Scary given that drm has a few, but I learned a few new thinks about > shrinkers. Some comments below, but looks all really nice imo. > > Cheers, Sima > > > --- > > rust/bindings/bindings_helper.h | 2 + > > rust/kernel/lib.rs | 1 + > > rust/kernel/shrinker.rs | 324 ++++++++++++++++++++++++++++++++= ++++++++ > > 3 files changed, 327 insertions(+) > > > > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_h= elper.h > > index ae82e9c941af..7fc958e05dc5 100644 > > --- a/rust/bindings/bindings_helper.h > > +++ b/rust/bindings/bindings_helper.h > > @@ -18,6 +18,7 @@ > > #include > > #include > > #include > > +#include > > #include > > #include > > #include > > @@ -31,4 +32,5 @@ const gfp_t RUST_CONST_HELPER_GFP_KERNEL_ACCOUNT =3D = GFP_KERNEL_ACCOUNT; > > const gfp_t RUST_CONST_HELPER_GFP_NOWAIT =3D GFP_NOWAIT; > > const gfp_t RUST_CONST_HELPER___GFP_ZERO =3D __GFP_ZERO; > > const gfp_t RUST_CONST_HELPER___GFP_HIGHMEM =3D ___GFP_HIGHMEM; > > +const gfp_t RUST_CONST_HELPER___GFP_FS =3D ___GFP_FS; > > const blk_features_t RUST_CONST_HELPER_BLK_FEAT_ROTATIONAL =3D BLK_FEA= T_ROTATIONAL; > > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs > > index f10b06a78b9d..f35eb290f2e0 100644 > > --- a/rust/kernel/lib.rs > > +++ b/rust/kernel/lib.rs > > @@ -45,6 +45,7 @@ > > pub mod prelude; > > pub mod print; > > pub mod rbtree; > > +pub mod shrinker; > > mod static_assert; > > #[doc(hidden)] > > pub mod std_vendor; > > diff --git a/rust/kernel/shrinker.rs b/rust/kernel/shrinker.rs > > new file mode 100644 > > index 000000000000..9af726bfe0b1 > > --- /dev/null > > +++ b/rust/kernel/shrinker.rs > > @@ -0,0 +1,324 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > + > > +// Copyright (C) 2024 Google LLC. > > + > > +//! Shrinker for handling memory pressure. > > +//! > > +//! C header: [`include/linux/shrinker.h`](srctree/include/linux/shrin= ker.h) > > + > > +use crate::{alloc::AllocError, bindings, c_str, str::CStr, types::Fore= ignOwnable}; > > + > > +use core::{ > > + ffi::{c_int, c_ulong, c_void}, > > + marker::PhantomData, > > + ptr::NonNull, > > +}; > > + > > +const SHRINK_STOP: c_ulong =3D bindings::SHRINK_STOP as c_ulong; > > +const SHRINK_EMPTY: c_ulong =3D bindings::SHRINK_EMPTY as c_ulong; > > + > > +/// The default value for the number of seeks needed to recreate an ob= ject. > > +pub const DEFAULT_SEEKS: u32 =3D bindings::DEFAULT_SEEKS; > > + > > +/// An unregistered shrinker. > > +/// > > +/// This type can be used to modify the settings of the shrinker befor= e it is registered. > > +/// > > +/// # Invariants > > +/// > > +/// The `shrinker` pointer references an unregistered shrinker. > > +pub struct UnregisteredShrinker { > > + shrinker: NonNull, > > +} > > + > > +// SAFETY: Moving an unregistered shrinker between threads is okay. > > +unsafe impl Send for UnregisteredShrinker {} > > +// SAFETY: An unregistered shrinker is thread safe. > > +unsafe impl Sync for UnregisteredShrinker {} > > + > > +impl UnregisteredShrinker { > > + /// Create a new shrinker. > > + pub fn alloc(name: &CStr) -> Result { > > + // SAFETY: Passing `0` as flags is okay. Using `%s` as the for= mat string is okay when we > > I'd elaborate here that we have to pass 0 as the only valid value because > all the non-zero flags are for memcg and numa aware shrinkers, and we do > not support those because we don't expose the relevant data from > ShrinkControl. > > > + // pass a nul-terminated string as the string for `%s` to prin= t. > > + let ptr =3D > > + unsafe { bindings::shrinker_alloc(0, c_str!("%s").as_char_= ptr(), name.as_char_ptr()) }; > > + > > + let shrinker =3D NonNull::new(ptr).ok_or(AllocError)?; > > + > > + // INVARIANT: The creation of the shrinker was successful. > > + Ok(Self { shrinker }) > > + } > > + > > + /// Create a new shrinker using format arguments for the name. > > + pub fn alloc_fmt(name: core::fmt::Arguments<'_>) -> Result { > > + // SAFETY: Passing `0` as flags is okay. Using `%pA` as the fo= rmat string is okay when we > > + // pass a `fmt::Arguments` as the value to print. > > + let ptr =3D unsafe { > > + bindings::shrinker_alloc( > > + 0, > > + c_str!("%pA").as_char_ptr(), > > + &name as *const _ as *const c_void, > > + ) > > + }; > > + > > + let shrinker =3D NonNull::new(ptr).ok_or(AllocError)?; > > + > > + // INVARIANT: The creation of the shrinker was successful. > > + Ok(Self { shrinker }) > > + } > > + > > + /// Set the number of seeks needed to recreate an object. > > + pub fn set_seeks(&mut self, seeks: u32) { > > Not sure we want to expose this, with ssd seeks kinda don't matter and > it's just a bit about relative fairness. I think nowadays it's more > important that the count_object values are normalized to size, if not all > objects you shrink have the same fixed size. > > So not sure we need this, at least initially. Good point about keeping the objects the same fixed size. I'll incorporate that in the docs. > > + unsafe { (*self.shrinker.as_ptr()).seeks =3D seeks as c_int }; > > + } > > + > > + /// Register the shrinker. > > + /// > > + /// The provided pointer is used as the private data, and the type= `T` determines the callbacks > > + /// that the shrinker will use. > > + pub fn register(self, private_data: T::Ptr) -> Regist= eredShrinker { > > + let shrinker =3D self.shrinker; > > + let ptr =3D shrinker.as_ptr(); > > + > > + // The destructor of `self` calls `shrinker_free`, so skip the= destructor. > > + core::mem::forget(self); > > + > > + let private_data_ptr =3D ::into_fore= ign(private_data); > > + > > + // SAFETY: We own the private data, so we can assign to it. > > + unsafe { (*ptr).private_data =3D private_data_ptr.cast_mut() }= ; > > + // SAFETY: The shrinker is not yet registered, so we can updat= e this field. > > + unsafe { (*ptr).count_objects =3D Some(rust_count_objects::= ) }; > > + // SAFETY: The shrinker is not yet registered, so we can updat= e this field. > > + unsafe { (*ptr).scan_objects =3D Some(rust_scan_objects::) = }; > > + > > + // SAFETY: The shrinker is unregistered, so it's safe to regis= ter it. > > + unsafe { bindings::shrinker_register(ptr) }; > > + > > + RegisteredShrinker { > > + shrinker, > > + _phantom: PhantomData, > > + } > > + } > > +} > > + > > +impl Drop for UnregisteredShrinker { > > + fn drop(&mut self) { > > + // SAFETY: The shrinker is a valid but unregistered shrinker, = and we will not use it > > + // anymore. > > + unsafe { bindings::shrinker_free(self.shrinker.as_ptr()) }; > > + } > > +} > > + > > +/// A shrinker that is registered with the kernel. > > +/// > > +/// # Invariants > > +/// > > +/// The `shrinker` pointer refers to a registered shrinker using `T` a= s the private data. > > +pub struct RegisteredShrinker { > > + shrinker: NonNull, > > + _phantom: PhantomData, > > +} > > + > > +// SAFETY: This allows you to deregister the shrinker from a different= thread, which means that > > +// private data could be dropped from any thread. > > +unsafe impl Send for RegisteredShrinker where T::Ptr: = Send {} > > +// SAFETY: The only thing you can do with an immutable reference is ac= cess the private data, which > > +// is okay to access in parallel as the `Shrinker` trait requires the = private data to be `Sync`. > > +unsafe impl Sync for RegisteredShrinker {} > > + > > +impl RegisteredShrinker { > > + /// Access the private data in this shrinker. > > + pub fn private_data(&self) -> ::Borrowed= <'_> { > > + // SAFETY: We own the private data, so we can access it. > > + let private =3D unsafe { (*self.shrinker.as_ptr()).private_dat= a }; > > + // SAFETY: By the type invariants, the private data is `T`. Th= is access could happen in > > + // parallel with a shrinker callback, but that's okay as the `= Shrinker` trait ensures that > > + // `T::Ptr` is `Sync`. > > + unsafe { ::borrow(private) } > > + } > > +} > > + > > +impl Drop for RegisteredShrinker { > > + fn drop(&mut self) { > > + // SAFETY: We own the private data, so we can access it. > > + let private =3D unsafe { (*self.shrinker.as_ptr()).private_dat= a }; > > + // SAFETY: We will not access the shrinker after this call. > > + unsafe { bindings::shrinker_free(self.shrinker.as_ptr()) }; > > + // SAFETY: The above call blocked until the completion of any = shrinker callbacks, so there > > + // are no longer any users of the private data. > > + drop(unsafe { ::from_foreign(private= ) }); > > + } > > +} > > + > > +/// Callbacks for a shrinker. > > +pub trait Shrinker { > > + /// The pointer type used to store the private data of the shrinke= r. > > + /// > > + /// Needs to be `Sync` because the shrinker callback could access = this value immutably from > > + /// several thread in parallel. > > + type Ptr: ForeignOwnable + Sync; > > + > > + /// Count the number of freeable items in the cache. > > + /// > > + /// May be called from atomic context. > > That's wrong, reclaim is allowed to block. Or my understanding of how thi= s > works is very busted. We do run in a pseudo locking context, the core > code annotates that with fs_reclaim_acquire/release. Ah, ok. Every shrinker I've looked at uses try_lock everywhere so I assumed this could happen. Thanks for verifying that. > > + fn count_objects( > > + me: ::Borrowed<'_>, > > + sc: ShrinkControl<'_>, > > + ) -> CountObjects; > > + > > + /// Count the number of freeable items in the cache. > > + /// > > + /// May be called from atomic context. > > Same here. > > > + fn scan_objects( > > + me: ::Borrowed<'_>, > > + sc: ShrinkControl<'_>, > > + ) -> ScanObjects; > > +} > > + > > +/// How many objects are there in the cache? > > +/// > > +/// This is used as the return value of [`Shrinker::count_objects`]. > > +pub struct CountObjects { > > + inner: c_ulong, > > +} > > + > > +impl CountObjects { > > + /// Indicates that the number of objects is unknown. > > + pub const UNKNOWN: Self =3D Self { inner: 0 }; > > + > > + /// Indicates that the number of objects is zero. > > + pub const EMPTY: Self =3D Self { > > + inner: SHRINK_EMPTY, > > So I spend way too much time looking at all this, and I think overflows > don't matter to the core shrinker code (aside from maybe a bit of > unfairness), as long as we don't accidently return SHRINK_EMPTY here. But > that's only relevant for memcg aware shrinkers I think. Overflow is one thing. The current API automatically converts 0 to SHRINK_EMPTY, whereas many C shrinkers just return the size directly, which means they return UNKNOWN when it's really empty. Thoughts? > > + }; > > + > > + /// The maximum possible number of freeable objects. > > + pub const MAX: Self =3D Self { > > + // The shrinker code assumes that it can multiply this value b= y two without overflow. > > + inner: c_ulong::MAX / 2, > > I think the limits is actually ulong_max/4, since priority can be 0 from > drop_slabs() and we multiply by 4 is seeks are nonzero. But then I tried > to look at what the limit for nr_to_scan and hence ScanObjects, and I > think aside from the special values the mm/shrinker.c simply does not car= e > about overflows at all. Both unsigned and signed integer math is well > defined for overflow in linux, so no compiler license to do stupid stuff, > and worst case if you do overflow you just end up shrinking a bit > unfairly. But afaict nothing breaks. > > So not sure we should enforce this when core mm doesn't bother. > > Same for ScanObjects below. > > > + }; > > + > > + /// Creates a new `CountObjects` with the given value. > > + pub fn from_count(count: usize) -> Self { > > + if count =3D=3D 0 { > > + return Self::EMPTY; > > + } > > + > > + if count > Self::MAX.inner as usize { > > + return Self::MAX; > > + } > > + > > + Self { > > + inner: count as c_ulong, > > + } > > + } > > +} > > + > > +/// How many objects were freed? > > +/// > > +/// This is used as the return value of [`Shrinker::scan_objects`]. > > +pub struct ScanObjects { > > + inner: c_ulong, > > +} > > + > > +impl ScanObjects { > > + /// Indicates that the shrinker should stop trying to free objects= from this cache due to > > + /// potential deadlocks. > > + pub const STOP: Self =3D Self { inner: SHRINK_STOP }; > > + > > + /// The maximum possible number of freeable objects. > > + pub const MAX: Self =3D Self { > > + // The shrinker code assumes that it can multiply this value b= y two without overflow. > > + inner: c_ulong::MAX / 2, > > + }; > > + > > + /// Creates a new `CountObjects` with the given value. > > + pub fn from_count(count: usize) -> Self { > > + if count > Self::MAX.inner as usize { > > + return Self::MAX; > > + } > > + > > + Self { > > + inner: count as c_ulong, > > + } > > + } > > +} > > + > > +/// This struct is used to pass information from page reclaim to the s= hrinkers. > > +pub struct ShrinkControl<'a> { > > + ptr: NonNull, > > + _phantom: PhantomData<&'a bindings::shrink_control>, > > +} > > + > > +impl<'a> ShrinkControl<'a> { > > + /// Create a `ShrinkControl` from a raw pointer. > > + /// > > + /// # Safety > > + /// > > + /// The pointer should point at a valid `shrink_control` for the d= uration of 'a. > > + pub unsafe fn from_raw(ptr: *mut bindings::shrink_control) -> Self= { > > + Self { > > + // SAFETY: Caller promises that this pointer is valid. > > + ptr: unsafe { NonNull::new_unchecked(ptr) }, > > + _phantom: PhantomData, > > + } > > + } > > + > > + /// Determines whether it is safe to recurse into filesystem code. > > + pub fn gfp_fs(&self) -> bool { > > I guess you need this in your new binder code, because the current one > seems to side-step __GFP_FS recursion with just trylocking absolutely > everything aside from the lru spinlock. At least I haven't found any > gfp_fs tests, but I might be blind. Ah, yeah, I don't think C binder tests for that. Either way, it probably makes more sense to just expose a method to get the flags directly, rather than have a dedicated method for testing this particular flags. Or what do you think? > > + // SAFETY: Okay by type invariants. > > + let mask =3D unsafe { (*self.ptr.as_ptr()).gfp_mask }; > > + > > + (mask & bindings::__GFP_FS) !=3D 0 > > + } > > + > > + /// Returns the number of objects that `scan_objects` should try t= o reclaim. > > + pub fn nr_to_scan(&self) -> usize { > > + // SAFETY: Okay by type invariants. > > + unsafe { (*self.ptr.as_ptr()).nr_to_scan as usize } > > + } > > + > > + /// The callback should set this value to the number of objects pr= ocessed. > > + pub fn set_nr_scanned(&mut self, val: usize) { > > Unless my grep skills are really bad I think the drm/i915 shrinker is the > only one that bothers with this. Not sure we want to bother either? > > > + let mut val =3D val as c_ulong; > > + // SAFETY: Okay by type invariants. > > + let max =3D unsafe { (*self.ptr.as_ptr()).nr_to_scan }; > > + if val > max { > > + val =3D max; > > + } > > + > > + // SAFETY: Okay by type invariants. > > + unsafe { (*self.ptr.as_ptr()).nr_scanned =3D val }; > > + } > > +} > > + > > +unsafe extern "C" fn rust_count_objects( > > + shrink: *mut bindings::shrinker, > > + sc: *mut bindings::shrink_control, > > +) -> c_ulong { > > + // SAFETY: We own the private data, so we can access it. > > + let private =3D unsafe { (*shrink).private_data }; > > + // SAFETY: This function is only used with shrinkers where `T` is = the type of the private data. > > + let private =3D unsafe { ::borrow(privat= e) }; > > + // SAFETY: The caller passes a valid `sc` pointer. > > + let sc =3D unsafe { ShrinkControl::from_raw(sc) }; > > + > > + let ret =3D T::count_objects(private, sc); > > + ret.inner > > +} > > + > > +unsafe extern "C" fn rust_scan_objects( > > + shrink: *mut bindings::shrinker, > > + sc: *mut bindings::shrink_control, > > +) -> c_ulong { > > + // SAFETY: We own the private data, so we can access it. > > + let private =3D unsafe { (*shrink).private_data }; > > + // SAFETY: This function is only used with shrinkers where `T` is = the type of the private data. > > + let private =3D unsafe { ::borrow(privat= e) }; > > + // SAFETY: The caller passes a valid `sc` pointer. > > + let sc =3D unsafe { ShrinkControl::from_raw(sc) }; > > + > > + let ret =3D T::scan_objects(private, sc); > > + ret.inner > > +} > > > > --- > > base-commit: 93dc3be19450447a3a7090bd1dfb9f3daac3e8d2 > > change-id: 20240911-shrinker-f8371af00b68 > > > > Best regards, > > -- > > Alice Ryhl > > > > -- > Simona Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch