Re: [Linaro-mm-sig] Re: [PATCH RFC 07/18] rust: drm: mm: Add DRM MM Range Allocator abstraction
On 07/04/2023 01.13, Daniel Vetter wrote: 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 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 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 , a locked Mutex upgrades that to 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. No no, I mean you don't have to supply the lock at all. The idea is that if you have a mutable reference to the object *at all* then Rust says that's effectively interlocked, whether you achieve that with an actual lock or just by not sharing the object to begin with. This is the standard pattern in Rust. Thread-safe methods take , and you can call those from multiple threads at once. Thread-unsafe methods take self, and you can only call them from one thread at once. Mutex is one mechanism that allows you to upgrade a shared to a self (while holding the lock). The actual object doesn't know anything about mutexes or locking, it just relies on the more fundamental property that Rust says that if you have a obj, absolutely nobody else does, at any given time, by definition. (And then everything also needs to impl Send + Sync for this to work across threads, but that's usually what you want) Basically if there were to exist a Rust abstraction/object/anything that allows two threads to get ahold of a to the same object at the same time without using unsafe, that abstraction/etc would be broken and unsound (and necessarily have to involve bad unsafe code within, because you can't do that with just safe code, the borrow checker stops you), and the state of affairs of two threads having such a reference is outright undefined behavior in the language model. 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). Ah, PhantomData is just a hack because Rust wants you to use all type parameters inside structs even when you don't actually need them for anything because they only have meaning to the abstraction itself. Without it it won't compile. Something something deep type system black magic rules (I'm pretty sure this requirement isn't gratuitous but I don't know what the whole story is here). The lock does give you a Guard you could pass somewhere as proof, which itself contains a lifetime that ties it to the Mutex, but more importantly that Guard implements DerefMut to give you a to whatever is inside the Mutex, and *that* mutable reference is the proof that you are the sole execution context with the right to access that one particular object. At that point the Guard doesn't matter, and lifetimes tie everything together so you can't stash that somewhere else or anything like that to break the rules (modulo unsafe code, of course!). 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 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
Re: [Linaro-mm-sig] Re: [PATCH RFC 07/18] rust: drm: mm: Add DRM MM Range Allocator abstraction
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 > > > > 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 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 , a locked Mutex upgrades that to 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 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
