linux-sgx.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Daniel Vetter <daniel@ffwll.ch>
To: Asahi Lina <lina@asahilina.net>
Cc: "Maarten Lankhorst" <maarten.lankhorst@linux.intel.com>,
	"Maxime Ripard" <mripard@kernel.org>,
	"Thomas Zimmermann" <tzimmermann@suse.de>,
	"David Airlie" <airlied@gmail.com>,
	"Miguel Ojeda" <ojeda@kernel.org>,
	"Alex Gaynor" <alex.gaynor@gmail.com>,
	"Wedson Almeida Filho" <wedsonaf@gmail.com>,
	"Boqun Feng" <boqun.feng@gmail.com>,
	"Gary Guo" <gary@garyguo.net>,
	"Björn Roy Baron" <bjorn3_gh@protonmail.com>,
	"Sumit Semwal" <sumit.semwal@linaro.org>,
	"Christian König" <christian.koenig@amd.com>,
	"Luben Tuikov" <luben.tuikov@amd.com>,
	"Jarkko Sakkinen" <jarkko@kernel.org>,
	"Dave Hansen" <dave.hansen@linux.intel.com>,
	"Alyssa Rosenzweig" <alyssa@rosenzweig.io>,
	"Karol Herbst" <kherbst@redhat.com>,
	"Ella Stanforth" <ella@iglunix.org>,
	"Faith Ekstrand" <faith.ekstrand@collabora.com>,
	Mary <mary@mary.zone>,
	linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
	rust-for-linux@vger.kernel.org, linux-media@vger.kernel.org,
	linaro-mm-sig@lists.linaro.org, linux-sgx@vger.kernel.org,
	asahi@lists.linux.dev
Subject: Re: [Linaro-mm-sig] Re: [PATCH RFC 07/18] rust: drm: mm: Add DRM MM Range Allocator abstraction
Date: Thu, 6 Apr 2023 18:13:01 +0200	[thread overview]
Message-ID: <ZC7vjYUnLZqb0FV4@phenom.ffwll.local> (raw)
In-Reply-To: <2a419eff-3566-0aa3-7185-951edde22abb@asahilina.net>

On Fri, Apr 07, 2023 at 12:53:47AM +0900, Asahi Lina wrote:
> On 06/04/2023 23.15, Daniel Vetter wrote:
> > On Tue, Mar 07, 2023 at 11:25:32PM +0900, Asahi Lina wrote:
> > > drm_mm provides a simple range allocator, useful for managing virtual
> > > address ranges. Add a Rust abstraction to expose this module to Rust
> > > drivers.
> > > 
> > > Signed-off-by: Asahi Lina <lina@asahilina.net>
> > 
> > In the cover letter you mentioned the design open about embedded the lock
> > into the rust wrappers.
> > 
> > I think for a first step that's perfectly fine.
> > 
> > Longer term we might want to ramp up some "proof of locking"
> > infrastructure in Rust, where callers can supply a lock guard and ideally
> > rust validates at compile time that it's for the right type, and at
> > runtime (like lockdep) that it's consistent and the callers don't mix up
> > locks (like using different locks for the same drm_mm allocator).
> 
> That proof-of-lock tuff works in Rust too as far as I know.
> 
> But the general thread safety story in Rust is much simpler, you just use
> methods that take &mut self when locking is the caller's responsibility.
> That effectively implies that there can only be one reference that can call
> those methods at any given time, thanks to the borrow checker. Shared
> references only give you &self, a locked Mutex upgrades that to &mut self,
> and that's how you get proof of locking at compile time, through and
> through, not just for the type but for the specific object.

Hm that still has the problem of making sure that you supply the right
lock (for generic abstractions like drm_mm or drm/sched where the lock is
supplied by the driver.

Once we have the lock then yeah borrow checker makes sure you can't screw
up, worst case needs a PhantomData (I guess) as toke of proof to pass
around the borrowed lifetime (If I got that right from your use of
PhantomData in the sched wrappers).

> > There's a lot of libraries in the kernel that have this "caller ensures
> > locking" pattern. drm/sched also has these requirements.
> 
> Yup, that all usually maps nicely to &mut self in Rust... except for the
> issue below.
> 
> > There's two other things I'd like to bring up on this patch though, just
> > because it's a good example. But they're both really general points that
> > apply for all the rust wrappers.
> > 
> > Documentation:
> > 
> > In drm we try to document all the interfaces that drivers use with formal
> > docs. Yes there's some areas that are not great for historical reasons,
> > but for new stuff and new wrappers we're really trying:
> > 
> > - This helps in telling internal (even across .c files or in rust across
> >    modules within a crate) from stuff drivers access. Sure you have static
> >    in C or pub in rust, but that doesn't tell you whether it's public all
> >    the way to drivers.
> > 
> > - ideally docs have a short intro section that explains the main concepts
> >    and links to the main data structures and functions. Just to give
> >    readers a good starting point to explore.
> > 
> > - Linking all the things, so that readers can connect the different parts.
> >    This is really important in C where e.g. get/put() or any such function
> >    pairs all needed to be linked together. With rust I'm hoping that
> >    rustdoc liberally sprinkles links already and we don't have to do this
> >    as much.
> > 
> > - Short explainers for parameters. For rust this also means type
> >    parameters, for those even simplified examples of how drivers are
> >    supposed to use them would help a lot in reading docs & understanding
> >    concepts.
> > 
> > - Ideally links from the rust to the sphinx side to linke relevant
> >    chapters together. Often the bigger explanations are in .rst files with
> >    DOT graphs (kms has a bunch I've added) or similar, and it doesn't make
> >    that much sense to duplicate all that on the rust side I guess. But it
> >    needs to be discoverable.
> > 
> > This might be more a discussion topic for the rust people than you
> > directly. Still needed for the merge-ready patches eventually.
> 
> I don't know much about the doc gen stuff on the Rust side so yeah, this is
> something I need to look into to make it pretty and complete...

From what Miguel has shown I think it's all there already, and the only
missing pieces are the cross-linking at a chapter level from rustdoc to
rst and sphinx to rstdoc too ideally. But I think for most rust wrappers
that will be one link each direction only (e.g. C drm_mm linking to
kernel::drm::MM and other way round and done). So absolutely no problem if
that one item is sorted out post merge once rustdoc/kernel-sphinx are
ready.

> > Refcounting vs borrowing:
> > 
> > This is honestly much more the eyebrow raising one than the locking. Very
> > often on the C side these datastructures all work with borrow semantics,
> > and you need to explicitly upgrade to a full reference (kref_get or
> > kref_get_unless_zero, depending whether it's a strong or weak reference)
> > if you need the object outside of the mutex/lock guard section.
> > 
> > Again I think for now it's ok, but the sales pitch of rust is that it
> > enables borrow lifetime checking with no runtime cost. Plus viz the vm
> > cleanup example, if you have too many strong backreferences the cleanup
> > flow gets complicated. And it would suck if rust drivers have to add
> > complexity like the openrefcount for the vm example simply because we
> > can't model the borrow semantics well enough to be safe.
> > 
> > So not something that's really bad here, but if we need to resort to full
> > refcounting already for simple datastructures then I'm getting a bit
> > worried about how well rust will cope with the really nasty borrowed
> > reference tricks we're playing in other areas.
> > 
> > Again more a topic for the rust folks I think than specifically here about
> > drm_mm wrapping. Just to get things going I think this is fine.
> 
> Yeeeeah... this is a *specific* problem. Drop.
> 
> The Allocator<T> itself is perfectly safe to implement without any locking,
> refcounting, or anything. You just make the methods take &mut self (as they
> already do), the caller can use it with a single reference or wrap it in an
> Arc<Mutex<T>> and share it, or whatever.
> 
> The problem is the Node<A, T>. When you Drop that, it has to go back to the
> Allocator. But now you're a different object, so no thread safety
> guarantees. And you need to keep the Allocator alive. So now to make a safe
> abstraction, you need refcounting and a mutex.
> 
> Lifetimes just don't work here, sadly. Not for a useful abstraction.
> 
> I'd love to hear from the other Rust folks whether they have any better
> ideas...

Hm yeah I think I get the gist of the issue. At time of Drop there's no
allocator reference you can borrow and so you're screwed.

In C we tend to solve that by passing both to the unlink/drop stuff (and
rust could then ensure that we have legit borrows for both), but I guess
that just totally wreaks entire wrapper and makes it really rough to use.

> One thing that *can* be done is making the Drop illegal (Rust can't do this
> "natively" but Linux already has hacks for that, we can make it fail to link
> if the Drop is ever called). Then you'd have to actively return the Node to
> the Allocator with a free function. Since Drop is forbidden, and Node is
> pinned, you'd always have to either return Node objects to the Allocator or
> leak them. You could drop the Allocator before its nodes, but as far as I
> know drm_mm can safely handle that (though it will complain), and then due
> to the previous guarantees the *only* thing you could do with orphan nodes
> is leak their memory, which is safe.
> 
> It would work... but it breaks the whole Rust automagic Drop stuff.

Yeah I think I see the challenge ...

> Thinking about this a bit, I think I want the current mutex/arc semantics
> for something like a memory allocator (which is one of my primary use cases
> for drm_mm), since I definitely don't want to be manually returning objects
> to their allocator all over the place, nor have overarching lifetime
> requirements that the allocator outlive its objects for safety (that sounds
> like a can of worms I don't want to open, I'd much rather use a refcount
> even if I "think" I can prove the lifetime bounds ad-hoc). But for something
> like a drm_mm that is tracking VA ranges within a VM with all Nodes held
> internally, maybe I could manage it all internally and have all node
> destruction be handled via an explicit call into the Allocator.

Yeah I think for gpuva we need to do better, but assuming the gpuva
library is in C then rust would just need to encode the safety properties
that (hopefully) the C library guarantees ...

And for any driver that just wants to use some range manager the standard
wrapping leans heavily on the side of "easy to use".

> Maybe the mm abstraction should offer both options? The extra locking can be
> implemented in terms of the base unlocked version I think (perhaps with some
> Deref abuse for ergonomics)... I definitely want to hear more opinions about
> this from other Rust folks, since there are probably other options I haven't
> considered...

I don't think we need the more raw/tricky one, at least not until we have
some serious libraries like gpuva implemented in rust. Or drivers
reimplementing the gpuva stuff in their driver :-)

> Aside: This, and all the other DRM abstractions, were written before the
> pin_init stuff from y86 that is in review right now was ready. That may open
> up more interesting/ergonomic/efficient APIs for some cases, especially
> where Pin and embedding C types into user objects in some way are involved.
> So maybe there's room for improvement here. Just a sidenote.

Ah good to know, and yeah that make open some interesting options.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

  reply	other threads:[~2023-04-06 16:13 UTC|newest]

Thread overview: 122+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-03-07 14:25 [PATCH RFC 00/18] Rust DRM subsystem abstractions (& preview AGX driver) Asahi Lina
2023-03-07 14:25 ` [PATCH RFC 01/18] rust: drm: ioctl: Add DRM ioctl abstraction Asahi Lina
2023-03-07 14:48   ` Karol Herbst
2023-03-07 14:51     ` Karol Herbst
2023-03-07 15:32   ` Maíra Canal
2023-03-09  5:32     ` Asahi Lina
2023-03-09  6:15       ` Dave Airlie
2023-03-09 12:09         ` Maíra Canal
2023-03-07 17:34   ` Björn Roy Baron
2023-03-09  6:04     ` Asahi Lina
2023-03-09 20:24       ` Faith Ekstrand
2023-03-09 20:39         ` Karol Herbst
2023-03-10  6:21           ` Asahi Lina
2023-04-13  9:23   ` Daniel Vetter
2023-03-07 14:25 ` [PATCH RFC 02/18] rust: drm: Add Device and Driver abstractions Asahi Lina
2023-03-07 18:19   ` Björn Roy Baron
2023-03-09  6:10     ` Asahi Lina
2023-03-10 18:56   ` Boqun Feng
2023-03-11  5:41   ` Boqun Feng
2023-04-05 17:10   ` Daniel Vetter
2023-03-07 14:25 ` [PATCH RFC 03/18] rust: drm: file: Add File abstraction Asahi Lina
2023-03-09 21:16   ` Faith Ekstrand
2023-03-09 22:16     ` Asahi Lina
2023-03-13 17:49       ` Faith Ekstrand
2023-03-14  2:07         ` Boqun Feng
2023-04-05 11:25           ` Daniel Vetter
2023-03-07 14:25 ` [PATCH RFC 04/18] rust: drm: gem: Add GEM object abstraction Asahi Lina
2023-04-05 11:08   ` Daniel Vetter
2023-04-05 11:19     ` Miguel Ojeda
2023-04-05 11:22       ` Daniel Vetter
2023-04-05 12:32         ` Miguel Ojeda
2023-04-05 12:36           ` Daniel Vetter
2023-03-07 14:25 ` [PATCH RFC 05/18] drm/gem-shmem: Export VM ops functions Asahi Lina
2023-03-07 14:25 ` [PATCH RFC 06/18] rust: drm: gem: shmem: Add DRM shmem helper abstraction Asahi Lina
2023-03-08 13:38   ` Maíra Canal
2023-03-09  5:25     ` Asahi Lina
2023-03-09 11:47       ` Maíra Canal
2023-03-09 14:16         ` Asahi Lina
2023-03-07 14:25 ` [PATCH RFC 07/18] rust: drm: mm: Add DRM MM Range Allocator abstraction Asahi Lina
2023-04-06 14:15   ` Daniel Vetter
2023-04-06 15:28     ` Miguel Ojeda
2023-04-06 15:45       ` Daniel Vetter
2023-04-06 17:19         ` Miguel Ojeda
2023-04-06 15:53     ` Asahi Lina
2023-04-06 16:13       ` Daniel Vetter [this message]
2023-04-06 16:39         ` [Linaro-mm-sig] " Asahi Lina
2023-03-07 14:25 ` [PATCH RFC 08/18] rust: dma_fence: Add DMA Fence abstraction Asahi Lina
2023-04-05 11:10   ` Daniel Vetter
2023-03-07 14:25 ` [PATCH RFC 09/18] rust: drm: syncobj: Add DRM Sync Object abstraction Asahi Lina
2023-04-05 12:33   ` Daniel Vetter
2023-04-06 16:04     ` Asahi Lina
2023-03-07 14:25 ` [PATCH RFC 10/18] drm/scheduler: Add can_run_job callback Asahi Lina
2023-03-08  8:46   ` Christian König
2023-03-08  9:41     ` Asahi Lina
2023-03-08 10:00       ` Christian König
2023-03-08 14:53         ` Asahi Lina
2023-03-08 15:30           ` Christian König
2023-03-08 16:44             ` Asahi Lina
2023-03-08 17:57               ` Christian König
2023-03-08 19:05                 ` Asahi Lina
2023-03-08 19:12                   ` Christian König
2023-03-08 19:45                     ` Asahi Lina
2023-03-08 20:14                       ` Christian König
2023-03-09  6:30                         ` Asahi Lina
2023-03-09  8:05                           ` Christian König
2023-03-09  9:14                             ` Asahi Lina
2023-03-09 18:50                               ` Faith Ekstrand
2023-03-10  9:16                                 ` Asahi Lina
2023-03-08 12:39     ` Karol Herbst
2023-03-08 13:47       ` Christian König
2023-03-08 14:43         ` Karol Herbst
2023-03-08 15:02           ` Christian König
2023-03-08 15:19             ` Karol Herbst
2023-03-16 13:40               ` Daniel Vetter
2023-04-05 13:40   ` Daniel Vetter
2023-04-05 14:14     ` Christian König
2023-04-05 14:21       ` Daniel Vetter
2023-03-07 14:25 ` [PATCH RFC 11/18] drm/scheduler: Clean up jobs when the scheduler is torn down Asahi Lina
2023-03-08  9:57   ` Maarten Lankhorst
2023-03-08 10:03     ` Christian König
2023-03-08 15:18       ` Asahi Lina
2023-03-08 15:42         ` Christian König
2023-03-08 17:32           ` Asahi Lina
2023-03-08 18:12             ` Christian König
2023-03-08 19:37               ` Asahi Lina
2023-03-09  8:42                 ` Christian König
2023-03-09  9:43                   ` Asahi Lina
2023-03-09 11:47                     ` Christian König
2023-03-09 13:48                       ` Asahi Lina
2023-03-09 19:59                     ` Faith Ekstrand
2023-03-10  9:58                       ` Asahi Lina
2023-03-13 20:11                         ` Faith Ekstrand
2023-03-08 17:39           ` alyssa
2023-03-08 17:44             ` Asahi Lina
2023-03-08 18:13             ` Christian König
2023-04-05 13:52   ` Daniel Vetter
2023-03-07 14:25 ` [PATCH RFC 12/18] rust: drm: sched: Add GPU scheduler abstraction Asahi Lina
2023-04-05 15:43   ` Daniel Vetter
2023-04-05 19:29     ` Daniel Vetter
2023-04-18  8:45       ` Daniel Vetter
2023-03-07 14:25 ` [PATCH RFC 13/18] drm/gem: Add a flag to control whether objects can be exported Asahi Lina
2023-04-05 14:55   ` Daniel Vetter
2023-03-07 14:25 ` [PATCH RFC 14/18] rust: drm: gem: Add set_exportable() method Asahi Lina
2023-03-07 14:25 ` [PATCH RFC 15/18] drm/asahi: Add the Asahi driver UAPI [DO NOT MERGE] Asahi Lina
2023-03-07 15:28   ` Karol Herbst
2023-03-07 14:25 ` [PATCH RFC 16/18] rust: bindings: Bind the Asahi DRM UAPI Asahi Lina
2023-03-07 14:25 ` [PATCH RFC 17/18] rust: macros: Add versions macro Asahi Lina
2023-03-07 16:17 ` [PATCH RFC 00/18] Rust DRM subsystem abstractions (& preview AGX driver) Asahi Lina
     [not found] ` <20230307-rust-drm-v1-18-917ff5bc80a8@asahilina.net>
2023-04-05 14:44   ` [PATCH RFC 18/18] drm/asahi: Add the Asahi driver for Apple AGX GPUs Daniel Vetter
2023-04-06  5:02     ` Asahi Lina
2023-04-06  5:09       ` Asahi Lina
2023-04-06 11:25       ` [Linaro-mm-sig] " Daniel Vetter
2023-04-06 13:32         ` Asahi Lina
2023-04-06 13:54           ` Daniel Vetter
     [not found]   ` <ZC2HtBOaoUAzVCVH@phenom.ffwll.local>
2023-04-06  4:44     ` Asahi Lina
2023-04-06  5:09       ` Asahi Lina
2023-04-06 11:26         ` Daniel Vetter
2023-04-06 10:42       ` [Linaro-mm-sig] " Daniel Vetter
2023-04-06 11:55       ` Daniel Vetter
2023-04-06 13:15         ` Asahi Lina
2023-04-06 13:48           ` Daniel Vetter
2023-04-06 15:19             ` Asahi Lina

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ZC7vjYUnLZqb0FV4@phenom.ffwll.local \
    --to=daniel@ffwll.ch \
    --cc=airlied@gmail.com \
    --cc=alex.gaynor@gmail.com \
    --cc=alyssa@rosenzweig.io \
    --cc=asahi@lists.linux.dev \
    --cc=bjorn3_gh@protonmail.com \
    --cc=boqun.feng@gmail.com \
    --cc=christian.koenig@amd.com \
    --cc=dave.hansen@linux.intel.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=ella@iglunix.org \
    --cc=faith.ekstrand@collabora.com \
    --cc=gary@garyguo.net \
    --cc=jarkko@kernel.org \
    --cc=kherbst@redhat.com \
    --cc=lina@asahilina.net \
    --cc=linaro-mm-sig@lists.linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-sgx@vger.kernel.org \
    --cc=luben.tuikov@amd.com \
    --cc=maarten.lankhorst@linux.intel.com \
    --cc=mary@mary.zone \
    --cc=mripard@kernel.org \
    --cc=ojeda@kernel.org \
    --cc=rust-for-linux@vger.kernel.org \
    --cc=sumit.semwal@linaro.org \
    --cc=tzimmermann@suse.de \
    --cc=wedsonaf@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).