On 05/04/2023 23.37, Daniel Vetter wrote:
On Tue, Mar 07, 2023 at 11:25:43PM +0900, Asahi Lina wrote:+/// A generic monotonically incrementing ID used to uniquely identify object instances within the +/// driver. +pub(crate) struct ID(AtomicU64); + +impl ID { + /// Create a new ID counter with a given value. + fn new(val: u64) -> ID { + ID(AtomicU64::new(val)) + } + + /// Fetch the next unique ID. + pub(crate) fn next(&self) -> u64 { + self.0.fetch_add(1, Ordering::Relaxed) + } +}Continuing the theme of me commenting on individual things, I stumbled over this because I noticed that there's a lot of id based lookups where I don't expect them, and started chasing. - For ids use xarray, not atomic counters. Yes I know dma_fence timelines gets this wrong, this goes back to an innocent time where we didn't allocate more than one timeline per engine, and no one fixed it since then. Yes u64 should be big enough for everyone :-/ - Attaching ID spaces to drm_device is also not great. drm is full of these mistakes. Much better if their per drm_file and so private to each client. - They shouldn't be used for anything else than uapi id -> kernel object lookup at the beginning of ioctl code, and nowhere else. At least from skimming it seems like these are used all over the driver codebase, which does freak me out. At least on the C side that's a clear indicator for a refcount/lockin/data structure model that's not thought out at all. What's going on here, what do I miss?
These aren't UAPI IDs, they are driver-internal IDs (the UAPI IDs do use xarray and are per-File). Most of them are just for debugging, so that when I enable full debug spam I have some way to correlate different things that are happening together (this subset of interleaved log lines relate to the same submission). Basically just object names that are easier to read (and less of a security leak) than pointers and guaranteed not to repeat. You could get rid of most of them and it wouldn't affect the driver design, it just makes it very hard to see what's going on with debug logs ^^;
There are only two that are ever used for non-debugging purposes: the VM ID, and the File ID. Both are per-device global IDs attached to the VMs (not the UAPI VM objects, but rather the underlyng MMU address space managers they represent, including the kernel-internal ones) and to Files themselves. They are used for destroying GEM objects: since the objects are also device-global across multiple clients, I need a way to do things like "clean up all mappings for this File" or "clean up all mappings for this VM". There's an annoying circular reference between GEM objects and their mappings, which is why this is explicitly coded out in destroy paths instead of naturally happening via Drop semantics (without that cleanup code, the circular reference leaks it).
So e.g. when a File does a GEM close or explicitly asks for all mappings of an object to be removed, it goes out to the (possibly shared) GEM object and tells it to drop all mappings marked as owned by that unique File ID. When an explicit "unmap all in VM" op happens, it asks the GEM object to drop all mappings for that underlying VM ID. Similarly, when a UAPI VM object is dropped (in the Drop impl, so both explicitly and when the whole File/xarray is dropped and such), that does an explicit unmap of a special dummy object it owns which would otherwise leak since it is not tracked as a GEM object owned by that File and therefore not handled by GEM closing. And again along the same lines, the allocators in alloc.rs explicitly destroy the mappings for their backing GEM objects on Drop. All this is due to that annoying circular reference between VMs and GEM objects that I'm not sure how to fix.
Note that if I *don't* do this (or forget to do it somewhere) the consequence is just that we leak memory, and if you try to destroy the wrong IDs somehow the worst that can happen is you unmap things you shouldn't and fault the GPU (or, in the kernel or kernel-managed user VM cases, potentially the firmware). Rust safety guarantees still keep things from going entirely off the rails within the kernel, since everything that matters is reference counted (which is why these reference cycles are possible at all).
This all started when I was looking at the panfrost driver for reference. It does the same thing except it uses actual pointers to the owning entities instead of IDs, and pointer comparison (see panfrost_gem_close). Of course you could try do that in Rust too (literally storing and comparing raw pointers that aren't owned references), but then you're introducing a Pin<> requirement on those objects to make their addresses stable and it feels way more icky and error-prone than unique IDs (since addresses can be reused). panfrost only has a single mmu (what I call the raw VM) per File while I have an arbitrary number, which is why I end up with the extra distinction/complexity of both File and VM IDs, but the concept is the same.
Some of this is going to be refactored when I implement arbitrary VM range mapping/unmapping, which would be a good time to improve this... but is there something particularly wrong/broken about the way I'm doing it now that I missed? I figured unique u64 IDs would be a pretty safe way to identify entities and cleanup the mappings when needed.
~~ Lina
