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 B2F37D7876C for ; Thu, 21 Nov 2024 14:36:02 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 2726F6B007B; Thu, 21 Nov 2024 09:36:02 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 221966B0088; Thu, 21 Nov 2024 09:36:02 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 0E9756B0089; Thu, 21 Nov 2024 09:36:02 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id E2FFA6B007B for ; Thu, 21 Nov 2024 09:36:01 -0500 (EST) Received: from smtpin30.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 86314A1368 for ; Thu, 21 Nov 2024 14:36:01 +0000 (UTC) X-FDA: 82810350126.30.DBD7900 Received: from mail-wr1-f51.google.com (mail-wr1-f51.google.com [209.85.221.51]) by imf05.hostedemail.com (Postfix) with ESMTP id 69D83100003 for ; Thu, 21 Nov 2024 14:34:19 +0000 (UTC) Authentication-Results: imf05.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=06ya6xFi; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf05.hostedemail.com: domain of aliceryhl@google.com designates 209.85.221.51 as permitted sender) smtp.mailfrom=aliceryhl@google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1732199573; 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=ZsVy0uy+Xfrlo3kE7Fo9CxxMEZ/R7tSBkr5imkd6cwo=; b=FEzsMk6WH2fr+QtWIaf8A95/6iGuBpn3+etGZ4SSr6u5bpGzY0vdSwo7OFgejtxG9pLM32 60pHg9TOzxHiRSxOsm/g0R0UcrBFwFZwbWAw3752Rbf1BjfIOzcWq4t62yJC/ds2hNOWad yolZkeCnbXMJTD3lG1rNGio+U9DbGSU= ARC-Authentication-Results: i=1; imf05.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=06ya6xFi; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf05.hostedemail.com: domain of aliceryhl@google.com designates 209.85.221.51 as permitted sender) smtp.mailfrom=aliceryhl@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1732199573; a=rsa-sha256; cv=none; b=TEWPhilBp6c7XbH/ZyJfWjshEYVkjUm5jNh7/BM2JIFuceusSnlPx84b13UVfQG5PWFlHY hWxFOsc4wday5qcPK/dNZV976GwZ/Ew93MHy9YHmURKx+B/6Vdxm9aP1WeL0IAskOuayFJ JDgLbriYhHm2bPkW1oFm9UfFbvDa9Ps= Received: by mail-wr1-f51.google.com with SMTP id ffacd0b85a97d-38246333e12so896969f8f.1 for ; Thu, 21 Nov 2024 06:35:59 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1732199758; x=1732804558; 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=ZsVy0uy+Xfrlo3kE7Fo9CxxMEZ/R7tSBkr5imkd6cwo=; b=06ya6xFiIjk96/CqE7u8DB+KagnlnNMnmPywEMAzyE5A8OuREAGlvOAJfV7fH84bH3 rUkh36VLieFimRK3bEl6uBeWwwa6X+YbYYFKmX06/InN4D/lBqFC1ST30AE109Hi7P0+ zecGj3H4XJ7w8Z/zcN4Hh/m/oHXj0oBP+o5gBx9lN7zTUeFXp7jWPIXsScK7xiL5LlsR xtA7bO7JPEkAeh2r8IjexEBYI7FenkB0ZRoOjKYMjnXVSYytY7jvQzlKjMSdBoDFxpr4 j88EYx53PheeoAyjQ/WcjM+EjmZNAAPwdS/a7GjlCSWDhUCvsX1S2fF97mRmoXvqcHg9 Hs2w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1732199758; x=1732804558; 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=ZsVy0uy+Xfrlo3kE7Fo9CxxMEZ/R7tSBkr5imkd6cwo=; b=MEwWUjNa4braOto0dkzhApSH337xhQMJjFg5Lrg8OnzTuqaqsYpYOauyb88mf61M5R 111+2uOg37jmKoyrSuDvINbc9BSjuUxUGInPV5TUisSv/fnw1oKfxHe2vla5TyAF07dF VEvQMg/Oym+zq+oHOZFl/rmyEN9TYH9RYHytFpVWVhAPBk+ns10LoHm9EuceWPY4bqxU qFhUwZEz4jZNFV+GyNOQL8xzloqK5stqRJ/VVn7K6VXeSLtvRAIn9TvIljFtf78z5hFh Rul7x0WD3nGbicHC9/K4Iwd4vKqHHUIXlCaC00sZmEdtcv+RLaHbDC26HAlOJVsNleX+ Neww== X-Forwarded-Encrypted: i=1; AJvYcCUoXUp4ARNs3lkHfeB9wpdISdoQOsnSfK+ST1CrxIT185w6Aq2PpmcD3K4CiAiE5Iu0DEFo+L0SWA==@kvack.org X-Gm-Message-State: AOJu0Ywh/hmWzzz1A/DQnD+bhOYVhomJXmQTuXBq9O6mYVn8h6sqW9oO qvzVFGvTrMOIxgjzU07lYgZrmaSS6Jxa2CnEHwn8URJC1lorTn0AVGF1FqQhWR8IB6yR0HSdTki lns7mnxnoMP0sxvBdSFgmxl6TV+c+U2Js0c0w X-Gm-Gg: ASbGncszDyXiFqgkf2IWHD1piq/G90wa3hCwnSL/NrNC1W405+ateOtvzQfCaqxfl6F OaGXnZPqdpqF2cXspUmtBOZvextbmixoRodwwTbO57iihmAzvFJ0+rlgI5fgZ0A== X-Google-Smtp-Source: AGHT+IHJLA9duB6XIUDG0QrZybh6MZxkzXN8hB3iai9ICokuoepRojYUjEIhCq5VjfNQLrg3URF3G8FMaA2ucsNyiZE= X-Received: by 2002:a5d:6487:0:b0:382:46a2:3788 with SMTP id ffacd0b85a97d-38254af9fd3mr5469204f8f.25.1732199757644; Thu, 21 Nov 2024 06:35:57 -0800 (PST) MIME-Version: 1.0 References: <20241120-vma-v8-0-eb31425da66b@google.com> <20241120-vma-v8-1-eb31425da66b@google.com> <0c6f4dbb-ff09-439c-b736-35568c1450cc@lucifer.local> <5a06280b-955d-41e1-8ab5-0b766ec7d7ac@lucifer.local> In-Reply-To: <5a06280b-955d-41e1-8ab5-0b766ec7d7ac@lucifer.local> From: Alice Ryhl Date: Thu, 21 Nov 2024 15:35:45 +0100 Message-ID: Subject: Re: [PATCH v8 1/7] mm: rust: add abstraction for struct mm_struct To: Lorenzo Stoakes Cc: Miguel Ojeda , Matthew Wilcox , Vlastimil Babka , John Hubbard , "Liam R. Howlett" , Andrew Morton , Greg Kroah-Hartman , Arnd Bergmann , Christian Brauner , Alex Gaynor , Boqun Feng , Gary Guo , =?UTF-8?Q?Bj=C3=B6rn_Roy_Baron?= , Benno Lossin , linux-kernel@vger.kernel.org, linux-mm@kvack.org, rust-for-linux@vger.kernel.org, Andreas Hindborg Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: 69D83100003 X-Stat-Signature: qmczjubndzy31hcr6a9aphn5k6r6gmbk X-Rspam-User: X-HE-Tag: 1732199659-942429 X-HE-Meta: U2FsdGVkX1+nbkd3nNMY2qXs95J9/s7S+U5LBfPa4zIUQGoXgCZ8xy5UePOe8JhEOpIHBGeuTJ67FFAyh3l+a9ecxH1THFsCmsZkGsZ4KtGVPAl4JwRrKpX3cvCi3rkJppIttXodGrqXioFxzIGioaaiMjjV/vf6SO4HjlZb2GfwsPWZ31glpZ7kcEP+7IYJZSy9NcKxc2fbSX6sVcuMrATn2c0IXTIZjGXdl31oLca3mc7K6lefkqfuHj5LmVOHsXqSj2BNU2MJXroWfyA4kjD8i5X2eGTeZwQdjaf1eIjBobc+15ZPB2nzKnVeFSWM48E9zUKAj7SSWjCTVm/1rdpeRAI6kH9rkNjCyGw7Jt+qlTcaAP8FweO2fPYGaf4M6JQPYMvOz2ucO4Oq4A1GOkHPl5fZ+BL83WTsqXLoi83g9sZtKlt3oE9aBqnSxQriiowd0OVvy9giLrt8GaH2+OEpwa2k1DxQDKy3eogrQkZhMMtpYEfNGaZFf9XglBlSULrZoF/v8wWd1/StQVBDop0EegLMthF2PBswUqP4iSo+DXC7znv/gti2oRHPmH2CscSWQIyovPV4g3atiiMmv31Q09VlOyWN4iCRD/7Js0ZIWv81iWAJeoIPl6PDn0+TFs7mTgwn8o/DyxAEHLFSXav+ery5wIzP6yU4x9N8G0eqE3YrE/x9ds6KMHXrCd3bpDX/JjBBTRWTsp8OO0PUp0fWrytkgAKfMUwRsI2A9RPe2hWp7+zRehjh+ficBGiMQOO1n1qqpJXYtqsAdOyuXb51mM/tUQbDa2cOBNfcbZ9VLfQWCZ6/X1hU1g1GziB2QDZHWb5G4bhcZC/hZNmwWFJD183XoTtCkGMSmepHtfz5oQ5f5TEq5ETY1UYrMpLTk0RHzKFkmr/+VvjgN7q8NbNJ14Ad5a1mnukMFzaKYMKMeuPiIST/VB4iD+l7Wd/2RO5aERv3sG0AvMseRH/ Mdk6wdRq HTnSUNnuzXBFunWHeE/0LR4NjWQ+xrHMNa6OT2zvItfAWM9JR1vDmneije6STnUJlAVV3uukwHohfYjqsiKm2lUpQ/te2G7GgvoKK24GM8V40GvAM8Xn9qL70mUJbtiD85SP4UJFgxQI0v3xQyh2L4Fr80Mdme+8ogG6xCCZHebpaS/o76B1Ftl8kkA+wkIvpqWec0usUtwqi1qDl9QHtkDHXf9HLIITTH6DDqZn+WcHCbIYEcOLjPQ33En8XmEafZjG1l8jr0WtWof+Fxn0zLDybVEyJfcywSxv2K1DeZo/ZWmqQfLTxM2Gib2irO/kkMwQHhCD/IQFL4f13O+sv92ZLAW4sM5Bc+GHk5acbHnn+3Kc= 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, Nov 21, 2024 at 1:37=E2=80=AFPM Lorenzo Stoakes wrote: > > I'm generally fine with this patch (other than rust specifics which I lea= ve > to the rust experts), however I'm a little concerned about us taking > reference counts when we don't need to so this is something I'd like to s= ee > addressed for v9 or, at least to be confident we're not doing this in the > binder code unnecessarily. > > Thanks! The refcount increment is *not* unnecessary in Binder. For the C equivalent, see the implementation of `binder_alloc_init`. It's used because Binder will access the mm of the recipient from the sender's scope, so it must hold on to an mm_struct reference. > > > > +/// [`mmget_not_zero`]: Mm::mmget_not_zero > > > > +#[repr(transparent)] > > > > +pub struct Mm { > > > > + mm: Opaque, > > > > +} > > > > > > Does this tie this type to the C struct mm_struct type? > > > > > > Does 'Opaque' mean it is a pointer to a type which is 'opaque' in the= sense > > > that rust can't see into its internals? > > > > This declaration defines a Rust type called Mm which has the same > > size, alignment, and contents as `struct mm_struct`. The purpose of > > Opaque is to tell Rust that it can't assume anything about the > > contents at all; we do that to leave it up to C. > > > > For example, normally if you have an immutable reference &i32, then > > Rust is going to assume that the contents behind the reference are in > > fact immutable. Opaque turns that off, meaning that an `&Opaque` > > is allowed to reference an integer as it gets modified. It makes all > > access to the contents unsafe, though. > > > > Note that Opaque is *not* a pointer type. We're going to be dealing > > with types like &Mm or ARef where &_ and ARef<_> are two different > > kinds of pointers. > > OK so when you reference Mm.mm that is in effect referencing the struct > mm_struct object rather than a pointer to a struct mm_struct? and Yes. > > > > +// SAFETY: By the type invariants, this type is always refcounted. > > > > +unsafe impl AlwaysRefCounted for Mm { > > > > + #[inline] > > > > + fn inc_ref(&self) { > > > > + // SAFETY: The pointer is valid since self is a reference. > > > > + unsafe { bindings::mmgrab(self.as_raw()) }; > > > > + } > > > > + > > > > + #[inline] > > > > + unsafe fn dec_ref(obj: NonNull) { > > > > + // SAFETY: The caller is giving up their refcount. > > > > + unsafe { bindings::mmdrop(obj.cast().as_ptr()) }; > > > > + } > > > > +} > > > > > > Under what circumstances would these be taken? Same question for MmWt= hUser. > > > > This makes `Mm` compatible with the pointer type called ARef<_>. This > > pointer type is used to represent ownership of a refcount. So whenever > > a variable of type ARef<_> goes out of scope, the refcount is > > decremented, and whenever an ARef<_> is cloned, the refcount is > > incremented. > > > > The way this works is that ARef<_> is implemented to use the > > AlwaysRefCounted trait to understand how to manipulate the count. Only > > types that implement the trait with an impl block like above can be > > used with ARef<_>. > > OK so when you first instantiate an ARef<_> you don't increment the > reference count, only if it is cloned from there on in? Well it depends on which ARef constructor you are using. But the uses of ARef::from_raw do not increment the count. > > > > +// These methods are safe to call even if `mm_users` is zero. > > > > +impl Mm { > > > > + /// Call `mmgrab` on `current.mm`. > > > > + #[inline] > > > > + pub fn mmgrab_current() -> Option> { > > > > + // SAFETY: It's safe to get the `mm` field from current. > > > > + let mm =3D unsafe { > > > > + let current =3D bindings::get_current(); > > > > + (*current).mm > > > > + }; > > > > > > I don't see an equivalent means of obtaining mm from current for > > > MmWithUser, is that intentional, would there be another means of obta= ining > > > an mm? (perhaps via vma->vm_mm I guess?) > > > > > > An aside --> > > > > > > If we're grabbing from current, and this is non-NULL (i.e. not a kern= el > > > thread), this is kinda MmWithUser to _start out_ with, but I guess if > > > you're grabbing the current one you might not expect it. > > > > > > I guess one thing I want to point out (maybe here is wrong place) is = that > > > the usual way of interacting with current->mm is that we do _not_ inc= rement > > > mm->mm_count, mm->mm_users or any refernce count, as while we are in = the > > > kernel portion of the task, we are guaranteed the mm and the backing > > > virtual address space will stick around. > > > > > > With reference to MmWithUser, in fact, if you look up users of > > > mmget()/mmput() it is pretty rare to do that. > > > > > > So ideally we'd avoid doing this if we could (though having the seman= tics > > > of grabbing a reference if we were to copy the object somehow again o= r hold > > > its state or something would be nice). > > > > > > I guess this might actually be tricky in rust, because we'd probably = need > > > to somehow express the current task's lifetime and tie this to that > > > and... yeah. > > > > > > <-- aside > > > > Ah, yeah, I guess this API is incomplete. We could have an API that > > lets you obtain an &MmWithUser instead. Then, if the user wants to > > increment the refcount, they can manually convert that into an > > ARef or ARef. > > > > It's true that it's slightly tricky to express in Rust, but it's > > possible. We already have a way to get a &Task pointing at current. > > Yeah, but I think we should do this incrementally as I want this initial > series to merge first, after which we can tweak things. Rest assured that the API can be written to not automatically increment the refcount when accessing current. That's just binder's case. > > > > + unsafe { &*ptr.cast() } > > > > + } > > > > + > > > > + /// Calls `mmget_not_zero` and returns a handle if it succeeds= . > > > > + #[inline] > > > > + pub fn mmget_not_zero(&self) -> Option> { > > > > > > I actually kinda love that this takes an mm and guarantees that you g= et an > > > MmWithUser out of it which is implied by the fact this succeeds. > > > > > > However as to the point above, I'm concerned that this might be seen = as > > > 'the way' to access an mm, i.e. mm.mmgrab_current().mmget_not_zero() = or > > > something. > > > > > > Whereas, the usual way of referencing current->mm is to not increment= any > > > reference counts at all (assuming what you are doing resides in the s= ame > > > lifetime as the task). > > > > > > Obviously if you step outside of that lifetime, then you _do_ have to= pin > > > the mm (nearly always you want to grab rather than get though in that > > > circumstance). > > > > I can add a way to obtain an &MmWithUser from current without > > incrementing the refcount. > > Yeah, I feel like we definitely need this. > > However we'd need to _not_ drop the refcount when it goes out of scope to= o > in this case. > > I'm not sure how you'd express that, however. The way you express that is by giving the user a &Mm or &MmWithUser instead of giving the user an ARef or ARef. Using a normal reference implies that you don't have ownership over the refcount, and the reference has no destructor when it goes out of scope. The only slightly tricky piece is tying the lifetime of that reference to the scope of the current task, but this is a problem with a known solution. For more on this, see the discussion on the various versions of Christian's PidNamespace series: https://lore.kernel.org/rust-for-linux/20241002-brauner-rust-pid_namespace-= v5-1-a90e70d44fde@kernel.org/ Alice