Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

15 participants
5851 discussions

Re: [PATCH v19 00/40] DEPT(DEPendency Tracker)

by Byungchul Park

On Fri, Aug 21, 2026 at 07:48:40PM +1000, NeilBrown wrote: > On Fri, 21 Aug 2026, David Hildenbrand (Arm) wrote: > > [...] > > > > > Exactly. That's why we use classification e.g. lock class - DEPT also > > > makes use of the concept. > > > > > > DEPT doesn't use a full map in each page but uses a minimum space for a > > > timestamp in each to track when each starts to wait so as to use the > > > recorded timestamp when the event occurs e.g. folio_unlock(). > > > > Thanks for that information! > > > > > > > >> Given that lockdep is a debug feature, and we will at some point allocate struct > > >> folio separately, I assume we could just squeeze a "struct lockdep_map" in there > > >> in such debug configs and the world would not collapse. > > > > > > That's a good news for lockdep. (And even for DEPT :) > > > > He :) Where do you currently store the additional per-page information? > > lockdep doesn't need to store per-page information. Possibly DEPT > doesn't either. Class can be stored in global map, but DEPT needs to keep a timestamp in each page to track when a potential-wait e.g. folio_lock() has been started and to refer to the information on its event e.g. folio_unlock(). > lockdep needs one lockdep_map for each lock class. It would make sense > for all folio locks to use the same global lockdep_map. Exactly. Only considering classes, you are right. > Each specific lock is known to lockdep as a task which holds the lock, a > lockdep_map which represents the class of locks, and subclass number > which allows a given task to hold multiple locks of the same class > providing it declare (e.g. with spin_lock_nested() etc). Right, lockdep works that way. > > >> Doing that today (one "struct lockdep_map" in each "struct page") wouldn't work > > >> as mm_zero_struct_page() would not expect such large "struct page". But > > >> conceptually, for a debug kernel with a special CONFIG_LOCKDEP_PAGE_LOCK, maybe > > >> that would already be ok and we could just do that (and optimize it as we > > >> allocate folios separately). > > > > > > Sounds great. > > > > > >> Not that it's ideal, but for a debug feature to at least check PG_lock, probably > > >> an easier way to achieve it than some completely new infrastructure. > > > > > > I understand what you are going to tell. > > > > > > However, it's worth noting that lockdep tracks dependencies basically > > > based on **lock acqusition orders** in the system. To make it track > > > even rwlock and general synchronization mechanism as well, lockdep has > > > no choice but to get more complicated. > > > > Well, yes, sure :) > > > > > > > > Focusing on only the dependency checking, the most parts of lockdep are > > > for the tricky things, so the reusable parts are not that big. > > > > > >> Now, Willy said "locking rules don't really apply to individual folios", I > > >> wonder if that could just help to also let lockdep check PG_lock with less > > >> metadata? (didn't fully wrap my head around the implications) > > > > > > That's what DEPT did and what brought external wgen introduced in DEPT. > > > I was considering the exactly same thing :) > > > > > > Again, lockdep that tracks lock acquisition orders can't do that. > > > > > >> [1] > > >> https://lore.kernel.org/all/aR3WHf9QZ_dizNun@casper.infradead.org/?utm_sour… > > >> > > >> > > >> It's your guiding example, that's why I mention it. You do mention other wait > > >> cases here, I don't know anything about them, but for folios it's really just > > >> "we used a single bit so far" AFAIKs. > > > > > > It doesn't matter whether it's implemented using bit or not. folio lock > > > is quite special since it's allowed to be released other than the > > > acquisition context that makes lockdep impossible to track them. > > > > Does that really make lockdep *impossible* to track them? IOW, there is no way > > to extend lockdep to support lock release in different context? > > Yes and no.... > > lockdep has no knowledge of control flows moving across threads in the > way that I assume DEPT does. But it should be possible to tell it. > > If you have some code that takes a lock and then hands it off to > another thread, at the hand-off point you call > lock_map_release(&the_lock_map) > > This says "no task owns this lock any more". > > maybe you put the folio which is locked on a queue or an lru or > whatever. > > There is no way to say "that queue owns this lock". Maybe that could > usefully be added - assuming coherent semantics can be designed. It's certainly useful to track who owns the lock, but the essence, when it comes to deadlock detection, lies elsewhere. DEPT is based on the essence, that is, a deadlock comes from waits that are never awakened. > Somewhere else some other task takes responsibility for that folio and > the lock. maybe it dequeues a page, or maybe an lru callback gives the > locked page to some code. > That code then calls > lock_map_acquire_try(&the_lock_map) > > This says "this task is now holding this lock" (or more accurately "now > holding a lock of this class"). > Note the "_try" - that says that the task didn't have to wait for the > lock, it just got it for free, which in fact it did. > > Now if that task takes some other lock, lockdep will see a dependency > between the page lock and the new lock, and will accept or reject it as > you would expect. It's an interesting approach if your goal is to track the owenership, but if the goal is for tracking dependencies.. well.. I'm not sure. Byungchul > So you definitely *can* send lock dependency information between tasks > with lockdep. I have only tried it in extremely simple cases where a > single object is being locked by one task and unlocked by another - no > queues or lists. > There may be - and probably are - more complexities involved with > locking folios and passing them around. Maybe it is so complex that you > need all the support that DEPT provides. But I'd like to see a coherent > explanation of how the functionality offered by DEPT is clearly better. > > NeilBrown > > > > > I guess there is a way, but the question is at which price (I seriously have no > > idea, maybe this was already discussed and people have a pointer for me). > > > > > > > >> [...] > > >> > > >>> > > >>> Q. Why not build DEPT into lockdep? > > >>> > > >>> A. Lockdep is stable, battle-tested code. I chose separation because > > >>> while DEPT borrows BFS and hashing ideas, the wait/event model > > >>> requires rebuilding from scratch. Lockdep was designed for lock > > >>> acquisition order — retrofitting it would risk its stability. > > >> > > >> Why can't this just be some configurable extension to lockdep > > >> (CONFIG_LOCKDEP_XYZ) until the feature is stable and can unconditionally be > > >> enabled along with it? > > > > > > Answered? > > > > Not quite. I don't understand why this must be a completely separate machinery, > > even if, conceptually, it would do more than traditional lockdep. > > > > Is it either DEPT or LOCKDEP in current configurations? Can both run at the same > > time? > > > > > > > >> I don't quite buy the "would risk its stability" argument. A lot of stuff we do > > >> "risks stability", every day :) > > > > > > That's awsome anyway :) > > > > > >> Is there another good reason (incompatible with X, dangerous with Y, cinfusing > > >> Z) why this really must be a separate thing? > > > > > > Roughly: > > > > > > 1. Similar or less effort is needed for the new one - retrofitting > > > lockdep is not easy and big changes are required since the > > > reusable parts are not that big. > > > > > > 2. Even though you didn't agree, retrofitting it would risk its > > > stability. > > > > Well, I don't buy the stability argument, really :) > > > > Retrofitting effort for lockdep is an interesting point, though. Lockdep > > maintainers would have to make the call here regarding direction and feasibility. > > > > [...] > > > > >> But I am not a locking maintainer. I think there was plenty of discussion in the > > >> past, so I might just be raising points that were already discussed in the past, > > >> but I really just read some random pieces of earlier discussions. (ideally > > >> previous discussions would be summarized here) > > >> > > >> Long story short: we are now in v19 and I think there was pushback in the past. > > >> Did the opinion of locking maintainers change, or is there a way forward to > > >> integrate this in a way that would make locking maintainers accept this? > > > > > > One of locking maintainers who I met in an LPC told me that he agrees > > > with the direction of DEPT and supports DEPT, not officially tho. > > > > Hm. > > > > > > > > What he and other people are concerning w.r.t DEPT the most is, false > > > positives, which is the most important issue for now. > > > > Thanks for highlighting that. What's the main reason for false positives? Is it > > something conceptual that is mostly impossible to solve, or rather just > > implementation work to cover all edge cases? > > > > > > > > At the same time, I think the most important thing is to make DEPT > > > useful in practice especially with folio locks involved. Actually, I'm > > > planning to share DEPT's true reports periodically to LKML and work with > > > people who believe DEPT can make things better. > > > > > > Any advices will be welcome. Thanks for your opinions. > > > > I think we must come to some conclusion on how to proceed with DEPT. I see the > > following options: > > > > (1) Don't merge it and carry it OOT. Shame if it delivers real value. > > > > (2) Merge it (after proper review and acks from relevant maintainers ;) ), > > keeping it entirely separate from lockdep. > > > > (3) Integrate it with lockdep on a high level, giving us a single locking > > dependency checker, but mostly letting dept have a separate implementation. > > Look into possible merging afterwards. > > > > (4) Retrofit and extend lockdep to really have one mechanism. > > > > As the saying goes, it's hard to teach old dogs new tricks, but in the end > > taking care of two dogs is likely harder than only a single dog? :) > > > > I tend to favor (4) (or 3 with possible future work to achieve 4), but I am not > > a locking maintainer, so really they have to voice what to do. > > > > I do see value in DEPT (even if the folio lock might be handled differently). > > > > -- > > Cheers, > > > > David > > >

3 weeks, 1 day

Re: [PATCH v5 05/10] mm/cma: Introduce cma_alloc_at() API

by Marek Szyprowski

On 20.08.2026 20:04, David Hildenbrand (Arm) wrote: >> /* This part must be outside protection */> diff --git a/mm/cma.c b/mm/cma.c >> index a10ea37a261d..1e1ebae79090 100644 >> --- a/mm/cma.c >> +++ b/mm/cma.c >> @@ -936,6 +936,141 @@ struct page *cma_alloc_frozen_compound(struct cma *cma, unsigned int order) >> return __cma_alloc_frozen(cma, 1 << order, order, gfp); >> } >> +static int cma_range_alloc_at(struct cma *cma, struct cma_memrange *cmr, >> + unsigned long offset, unsigned long count, >> + struct page **pagep, gfp_t gfp) >> +{ >> + struct page *page = NULL; >> + unsigned long pfn; >> + int ret = -EBUSY; >> + >> + spin_lock_irq(&cma->lock); >> + >> + /* >> + * If the request is larger than the available number of pages, stop >> + * right away. >> + */ >> + if (count > cma->available_count) >> + goto unlock; >> + >> + ret = bitmap_allocate(cmr->bitmap, offset, count); >> + if (ret < 0) >> + goto unlock; >> + >> + pfn = cmr->base_pfn + offset; >> + page = pfn_to_page(pfn); >> + >> + /* >> + * Do not hand out page ranges that are not contiguous, so >> + * callers can just iterate the pages without having to worry >> + * about these corner cases. >> + */ >> + if (!page_range_contiguous(page, count)) { >> + pr_warn_ratelimited("%s: %s: skipping non-contiguous area [0x%lx-0x%lx]", >> + __func__, cma->name, pfn, pfn + count - 1); >> + ret = -EBUSY; >> + goto clear; >> + } >> + >> + cma->available_count -= count; >> + >> + /* >> + * It's safe to drop the lock here. We've marked this region for >> + * our exclusive use. If the migration fails we will take the >> + * lock again and unmark it. >> + */ >> + spin_unlock_irq(&cma->lock); >> + >> + mutex_lock(&cma->alloc_mutex); >> + ret = alloc_contig_frozen_range(pfn, pfn + count, ACR_FLAGS_CMA, gfp); >> + mutex_unlock(&cma->alloc_mutex); >> + > There is quite some code duplication with cma_range_alloc(). Please try harder > to factor common code out and reuse it. > > > > ... > Also here, way too much code duplication with __cma_alloc_frozen(). > > There must be a better way :) Maybe the existing 'alloc' functions could call 'alloc_at' variant internally (with locks adjusted and moved out). > (I really prefer this direction of the patch set) Definitely, I also like this approach much more than previous attempt with 'dynamically' instantiated cma regions. Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland

3 weeks, 1 day

Re: [RFC PATCH] dma-buf: add a generic reclaim-priority hint

by Christian König

On 8/20/26 21:08, Ferran Duarri wrote: > dma-buf has no generic mechanism for a buffer's exporter, importer, or > a cooperating userspace agent to signal how eagerly a given buffer's > backing memory should be given up relative to other buffers when the > system is under memory pressure. Pages pinned via pin_user_pages()/ > FOLL_LONGTERM sit outside the normal reclaim path by design, so any > subsystem that wants a "prefer to keep A over B under pressure" policy > for its pinned dma-buf allocations currently has to invent its own > private, driver-specific side channel to express it. Well to start exporting pages pinned with FOLL_LONGTERM as DMA-buf is absolutely *STRICTLY* forbidden. I hope that you know this. What can be done is for exporters to migrate a DMA-buf between local memory, system memory and swap, but even that usually doesn't happen depending on the normal reclaim mechanism. It could be interesting to implement something like that for backing some Vulkan extension, but IIRC that is currently not even supported by anybody. Anyway as long as you can demonstrate any of this with an in tree driver the whole approach is not something we would discuss in the first place. Regards, Christian. > > This is a real, recurring pattern: GPU drivers juggling foreground and > background clients want it, and it shows up concretely in large-model > AI/ML inference stacks that tier working sets across VRAM/system-RAM/ > NVMe using their own dma-buf exporters -- e.g. an out-of-tree memory > tiering driver we've been using has its own gaming_mode sysfs flag and > a per-buffer "heat" score, moving its own DMA-BUF-backed allocations to > its own private LRU tail under a private IOCTL, purely because there is > nowhere generic to express "this buffer can go first." > > Add one small, opt-in hint instead of another private channel: > > - struct dma_buf gains a `priority` field (atomic_t, plain hint, no > new locking), defaulting to DMA_BUF_PRIORITY_DEFAULT (128) and > ranging DMA_BUF_PRIORITY_MIN (0) to DMA_BUF_PRIORITY_MAX (255). > Lower values should be reclaimed EARLIER under memory pressure. > > - dma_buf_set_priority()/dma_buf_get_priority(), exported under the > DMA_BUF symbol namespace, so any dma-buf exporter (in-tree or an > out-of-tree module) can set/read it directly. > > - DMA_BUF_IOCTL_SET_PRIORITY / DMA_BUF_IOCTL_GET_PRIORITY so > userspace holding an fd can do the same, without needing a private > driver ioctl. > > - The current value is reported via fdinfo (`priority:`) for > debugging/accounting, next to the existing size/name/exp_name > fields. > > This is deliberately a hint only: dma-buf core stores and reports the > value, it implements no eviction policy of its own, and it changes no > behavior for any existing exporter or importer that doesn't opt in. > No existing dma_buf_ops callback is touched. Compile-tested: drivers/ > dma-buf/ (incl. the selftest module) and drivers/gpu/drm/drm_prime.o > both build clean against this change. > > Signed-off-by: Ferran Duarri <ferran.duarri(a)me.com> > --- > drivers/dma-buf/dma-buf.c | 51 ++++++++++++++++++++++++++++++++++++ > include/linux/dma-buf.h | 26 ++++++++++++++++++ > include/uapi/linux/dma-buf.h | 33 +++++++++++++++++++++++ > 3 files changed, 110 insertions(+) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index d504c636dc29..1b1d3ee77f47 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -432,6 +432,39 @@ static long dma_buf_set_name(struct dma_buf *dmabuf, const char __user *buf) > return 0; > } > > +/** > + * dma_buf_set_priority - Set the reclaim-priority hint on a dma_buf. > + * @dmabuf: [in] buffer to update. > + * @priority: [in] new priority, clamped to > + * [DMA_BUF_PRIORITY_MIN, DMA_BUF_PRIORITY_MAX]. > + * > + * Lower values should be reclaimed EARLIER under memory pressure. This is > + * a hint only , dma-buf core stores and reports it, it implements no > + * eviction policy itself. Safe to call at any time, from any context that > + * can call dma_buf_get()/hold a reference (no locking required beyond > + * that reference). > + */ > +void dma_buf_set_priority(struct dma_buf *dmabuf, unsigned int priority) > +{ > + if (priority > DMA_BUF_PRIORITY_MAX) > + priority = DMA_BUF_PRIORITY_MAX; > + atomic_set(&dmabuf->priority, priority); > +} > +EXPORT_SYMBOL_NS_GPL(dma_buf_set_priority, "DMA_BUF"); > + > +/** > + * dma_buf_get_priority - Read back the current reclaim-priority hint. > + * @dmabuf: [in] buffer to query. > + * > + * Returns the value most recently set via dma_buf_set_priority() (or > + * DMA_BUF_PRIORITY_DEFAULT if never set). > + */ > +unsigned int dma_buf_get_priority(struct dma_buf *dmabuf) > +{ > + return atomic_read(&dmabuf->priority); > +} > +EXPORT_SYMBOL_NS_GPL(dma_buf_get_priority, "DMA_BUF"); > + > #if IS_ENABLED(CONFIG_SYNC_FILE) > static long dma_buf_export_sync_file(struct dma_buf *dmabuf, > void __user *user_data) > @@ -542,6 +575,7 @@ static long dma_buf_ioctl(struct file *file, > { > struct dma_buf *dmabuf; > struct dma_buf_sync sync; > + struct dma_buf_priority prio; > enum dma_data_direction direction; > int ret; > > @@ -580,6 +614,21 @@ static long dma_buf_ioctl(struct file *file, > case DMA_BUF_SET_NAME_B: > return dma_buf_set_name(dmabuf, (const char __user *)arg); > > + case DMA_BUF_IOCTL_SET_PRIORITY: > + if (copy_from_user(&prio, (void __user *)arg, sizeof(prio))) > + return -EFAULT; > + if (prio.pad || prio.priority > DMA_BUF_PRIORITY_MAX) > + return -EINVAL; > + dma_buf_set_priority(dmabuf, prio.priority); > + return 0; > + > + case DMA_BUF_IOCTL_GET_PRIORITY: > + memset(&prio, 0, sizeof(prio)); > + prio.priority = dma_buf_get_priority(dmabuf); > + if (copy_to_user((void __user *)arg, &prio, sizeof(prio))) > + return -EFAULT; > + return 0; > + > #if IS_ENABLED(CONFIG_SYNC_FILE) > case DMA_BUF_IOCTL_EXPORT_SYNC_FILE: > return dma_buf_export_sync_file(dmabuf, (void __user *)arg); > @@ -604,6 +653,7 @@ static void dma_buf_show_fdinfo(struct seq_file *m, struct file *file) > if (dmabuf->name) > seq_printf(m, "name:\t%s\n", dmabuf->name); > spin_unlock(&dmabuf->name_lock); > + seq_printf(m, "priority:\t%u\n", dma_buf_get_priority(dmabuf)); > } > > static const struct file_operations dma_buf_fops = { > @@ -748,6 +798,7 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info) > dmabuf->exp_name = exp_info->exp_name; > dmabuf->owner = exp_info->owner; > spin_lock_init(&dmabuf->name_lock); > + atomic_set(&dmabuf->priority, DMA_BUF_PRIORITY_DEFAULT); > init_waitqueue_head(&dmabuf->poll); > dmabuf->cb_in.poll = dmabuf->cb_out.poll = &dmabuf->poll; > dmabuf->cb_in.active = dmabuf->cb_out.active = 0; > diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h > index d1203da56fc5..b3d3f2858704 100644 > --- a/include/linux/dma-buf.h > +++ b/include/linux/dma-buf.h > @@ -354,6 +354,29 @@ struct dma_buf { > /** @name_lock: Spinlock to protect name access for read access. */ > spinlock_t name_lock; > > + /** > + * @priority: > + * > + * Reclaim-priority hint for this buffer's backing memory, in the > + * range DMA_BUF_PRIORITY_MIN..DMA_BUF_PRIORITY_MAX (see > + * include/uapi/linux/dma-buf.h). Lower values should be reclaimed > + * EARLIER under memory pressure. Defaults to > + * DMA_BUF_PRIORITY_DEFAULT. > + * > + * This is a hint only: dma-buf core implements no eviction policy > + * of its own, it merely stores and reports the value so exporters > + * (and cooperating shrinkers) have one shared, generic place to > + * look instead of each inventing a private side channel. Read with > + * dma_buf_get_priority(), set with dma_buf_set_priority() , also > + * reachable from userspace via DMA_BUF_IOCTL_SET_PRIORITY / > + * DMA_BUF_IOCTL_GET_PRIORITY. > + * > + * Plain atomic_t rather than a lock: this is a coarse, racy-by- > + * design hint consulted opportunistically, not a value anything > + * synchronizes correctness on. > + */ > + atomic_t priority; > + > /** > * @owner: > * > @@ -566,6 +589,9 @@ void dma_buf_unpin(struct dma_buf_attachment *attach); > > struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info); > > +void dma_buf_set_priority(struct dma_buf *dmabuf, unsigned int priority); > +unsigned int dma_buf_get_priority(struct dma_buf *dmabuf); > + > int dma_buf_fd(struct dma_buf *dmabuf, int flags); > struct dma_buf *dma_buf_get(int fd); > void dma_buf_put(struct dma_buf *dmabuf); > diff --git a/include/uapi/linux/dma-buf.h b/include/uapi/linux/dma-buf.h > index e827c9d20c5d..4a1d26e0b0a0 100644 > --- a/include/uapi/linux/dma-buf.h > +++ b/include/uapi/linux/dma-buf.h > @@ -168,6 +168,37 @@ struct dma_buf_import_sync_file { > __s32 fd; > }; > > +/** > + * struct dma_buf_priority - Reclaim-priority hint for a dma-buf > + * > + * dma-buf has no generic mechanism for a buffer's exporter, importer, or > + * a cooperating userspace agent to signal how eagerly this buffer's > + * backing memory should be given up relative to other buffers when the > + * system is under memory pressure. Every subsystem that wants this today > + * (GPU drivers juggling foreground/background clients, memory-tiering > + * allocators for large ML/AI working sets, ...) has to build its own > + * private, driver-specific side channel to express it. > + * > + * DMA_BUF_IOCTL_SET_PRIORITY / DMA_BUF_IOCTL_GET_PRIORITY add one shared, > + * generic hint that any dma-buf exporter MAY consult. This is a hint > + * only: the dma-buf core stores and reports the value, it implements no > + * eviction policy of its own and by itself changes no behavior for any > + * existing exporter. > + */ > +struct dma_buf_priority { > + /** > + * @priority: DMA_BUF_PRIORITY_MIN..DMA_BUF_PRIORITY_MAX. Lower > + * values should be reclaimed EARLIER under memory pressure. > + */ > + __u32 priority; > + /** @pad: must be zero, reserved for future use. */ > + __u32 pad; > +}; > + > +#define DMA_BUF_PRIORITY_MIN 0 > +#define DMA_BUF_PRIORITY_DEFAULT 128 > +#define DMA_BUF_PRIORITY_MAX 255 > + > #define DMA_BUF_BASE 'b' > #define DMA_BUF_IOCTL_SYNC _IOW(DMA_BUF_BASE, 0, struct dma_buf_sync) > > @@ -179,5 +210,7 @@ struct dma_buf_import_sync_file { > #define DMA_BUF_SET_NAME_B _IOW(DMA_BUF_BASE, 1, __u64) > #define DMA_BUF_IOCTL_EXPORT_SYNC_FILE _IOWR(DMA_BUF_BASE, 2, struct dma_buf_export_sync_file) > #define DMA_BUF_IOCTL_IMPORT_SYNC_FILE _IOW(DMA_BUF_BASE, 3, struct dma_buf_import_sync_file) > +#define DMA_BUF_IOCTL_SET_PRIORITY _IOW(DMA_BUF_BASE, 4, struct dma_buf_priority) > +#define DMA_BUF_IOCTL_GET_PRIORITY _IOR(DMA_BUF_BASE, 5, struct dma_buf_priority) > > #endif > -- > 2.53.0 >

3 weeks, 1 day

Re: [PATCH v19 00/40] DEPT(DEPendency Tracker)

by Byungchul Park

On Fri, Aug 21, 2026 at 09:56:05AM +0200, David Hildenbrand (Arm) wrote: [...] > >>> Q. Why not build DEPT into lockdep? > >>> > >>> A. Lockdep is stable, battle-tested code. I chose separation because > >>> while DEPT borrows BFS and hashing ideas, the wait/event model > >>> requires rebuilding from scratch. Lockdep was designed for lock > >>> acquisition order — retrofitting it would risk its stability. > >> > >> Why can't this just be some configurable extension to lockdep > >> (CONFIG_LOCKDEP_XYZ) until the feature is stable and can unconditionally be > >> enabled along with it? > > > > Answered? > > Not quite. I don't understand why this must be a completely separate machinery, > even if, conceptually, it would do more than traditional lockdep. > > Is it either DEPT or LOCKDEP in current configurations? Can both run at the same > time? Yes. > > > >> I don't quite buy the "would risk its stability" argument. A lot of stuff we do > >> "risks stability", every day :) > > > > That's awsome anyway :) > > > >> Is there another good reason (incompatible with X, dangerous with Y, cinfusing > >> Z) why this really must be a separate thing? > > > > Roughly: > > > > 1. Similar or less effort is needed for the new one - retrofitting > > lockdep is not easy and big changes are required since the > > reusable parts are not that big. > > > > 2. Even though you didn't agree, retrofitting it would risk its > > stability. > > Well, I don't buy the stability argument, really :) > > Retrofitting effort for lockdep is an interesting point, though. Lockdep > maintainers would have to make the call here regarding direction and feasibility. > > [...] > > >> But I am not a locking maintainer. I think there was plenty of discussion in the > >> past, so I might just be raising points that were already discussed in the past, > >> but I really just read some random pieces of earlier discussions. (ideally > >> previous discussions would be summarized here) > >> > >> Long story short: we are now in v19 and I think there was pushback in the past. > >> Did the opinion of locking maintainers change, or is there a way forward to > >> integrate this in a way that would make locking maintainers accept this? > > > > One of locking maintainers who I met in an LPC told me that he agrees > > with the direction of DEPT and supports DEPT, not officially tho. > > Hm. > > > > > What he and other people are concerning w.r.t DEPT the most is, false > > positives, which is the most important issue for now. > > Thanks for highlighting that. What's the main reason for false positives? Is it > something conceptual that is mostly impossible to solve, or rather just > implementation work to cover all edge cases? It's because of lack of annotations for proper classification. As Willy mentioned, different call paths for various usages should be annotated with different classes throughout the kernel. > > At the same time, I think the most important thing is to make DEPT > > useful in practice especially with folio locks involved. Actually, I'm > > planning to share DEPT's true reports periodically to LKML and work with > > people who believe DEPT can make things better. > > > > Any advices will be welcome. Thanks for your opinions. > > I think we must come to some conclusion on how to proceed with DEPT. I see the > following options: > > (1) Don't merge it and carry it OOT. Shame if it delivers real value. > > (2) Merge it (after proper review and acks from relevant maintainers ;) ), > keeping it entirely separate from lockdep. > > (3) Integrate it with lockdep on a high level, giving us a single locking > dependency checker, but mostly letting dept have a separate implementation. > Look into possible merging afterwards. > > (4) Retrofit and extend lockdep to really have one mechanism. > > As the saying goes, it's hard to teach old dogs new tricks, but in the end > taking care of two dogs is likely harder than only a single dog? :) Or it's better to train a unmatured hunting dog than to teach a pet dog, how to hunt, for better hunting, even though the pet dog might eventually become able to. > I tend to favor (4) (or 3 with possible future work to achieve 4), but I am not > a locking maintainer, so really they have to voice what to do. Right. However, thanks for your opinion. > I do see value in DEPT (even if the folio lock might be handled differently). Thanks! Byungchul > > -- > Cheers, > > David

3 weeks, 1 day

[PATCH 0/2] Clear shared pages after private-to-shared conversion

by Steven Price

Arm CCA includes "Memory Encryption Contexts" (MEC) which allows the private and shared data accessible to a guest to have different memory encryption keys. Consequently when converting memory to shared, the memory encryption key used to access the physical page will change. Both the GICv3 ITS driver and the system_cc_shared dma-buf heap currently allocate memory with __GFP_ZERO and then decrypt it. With MEC the zeroing is done with the wrong encryption key and the data visible after decryption may be ciphertext. The RMM is required to scrub the data, but may perform this scrub with a different encryption key to the eventual key that will be used for shared access. Fix these two sites by avoiding the __GFP_ZERO during the allocation and performing a clear_pages() call after the decryption. Steven Price (2): irqchip/gic-v3-its: Zero shared pages after conversion dma-buf: heaps: Zero system shared heap pages after conversion drivers/dma-buf/heaps/system_heap.c | 14 +++++++++++--- drivers/irqchip/irq-gic-v3-its.c | 7 ++++++- 2 files changed, 17 insertions(+), 4 deletions(-) -- 2.43.0

3 weeks, 1 day

Re: [PATCH v2 09/15] accel/qda: Add DMA-backed GEM objects and memory manager integration

by Christian König

On 8/17/26 06:47, Ekansh Gupta wrote: > Introduce DMA-coherent buffer management for the QDA driver, wiring > together the GEM subsystem, the IOMMU memory manager, and a DMA > allocation backend. > > qda_gem.c / qda_gem.h > Implements the GEM object lifecycle for QDA buffers. Each buffer is > represented by a qda_gem_obj which embeds a drm_gem_object and > carries the kernel virtual address, DMA address, and a pointer to > the IOMMU device that performed the allocation. The .free callback > delegates to the memory manager, and the .mmap callback uses > dma_mmap_coherent() via the DMA backend. > > qda_memory_dma.c / qda_memory_dma.h > DMA coherent allocation backend. qda_dma_alloc() calls > dma_alloc_coherent() on the CB device and encodes the stream ID > (SID) in the upper 32 bits of the returned DMA address, following > the Qualcomm FastRPC convention for IOMMU address space tagging. > qda_dma_free() strips the SID prefix before calling > dma_free_coherent(). > > qda_memory_manager.c > Adds process-to-device assignment: each DRM file (process) is > assigned one IOMMU context bank device for the lifetime of the > session. qda_memory_manager_assign_device() first checks whether > the process already has a device (reusing it with a refcount > increment), then falls back to claiming an unassigned device. > qda_memory_manager_alloc() and qda_memory_manager_free() delegate > to the DMA backend after resolving the correct CB device for the > calling process. Oh, stuff like that is usually a pretty big NO-GO now. AMD has made the same mistake with KFD and it resulted in a massive chaos. The DRM file is the driver context your process uses and even if there are multiple DRM files for the same PID you should absolute *NOT* share anything between them. The general rule of thumb is to not attach anything to the process using the DRM file descriptor. Regards, Christian. > > qda_drv.c / qda_drv.h > qda_file_priv gains an assigned_iommu_dev pointer and a pid field. > The .postclose callback decrements the IOMMU device refcount and > clears the process assignment when the last reference is dropped. > > Assisted-by: Claude:claude-sonnet-5 > Signed-off-by: Ekansh Gupta <ekansh.gupta(a)oss.qualcomm.com> > --- > Changes in v2: > - Adapt to the dynamically-sized device array introduced in patch 07 > (kcalloc'd from DT node count, replaces fixed QDA_IOMMU_DEVICES_MAX) > - Protect register/unregister with the process_assignment_lock mutex so > the device-assignment and device-registration paths are serialised > - No functional changes requested by reviewers on this patch > --- > drivers/accel/qda/Makefile | 2 + > drivers/accel/qda/qda_drv.c | 4 + > drivers/accel/qda/qda_drv.h | 4 + > drivers/accel/qda/qda_gem.c | 134 ++++++++++++++++++ > drivers/accel/qda/qda_gem.h | 52 +++++++ > drivers/accel/qda/qda_memory_dma.c | 82 +++++++++++ > drivers/accel/qda/qda_memory_dma.h | 17 +++ > drivers/accel/qda/qda_memory_manager.c | 239 ++++++++++++++++++++++++++++++++- > drivers/accel/qda/qda_memory_manager.h | 30 +++++ > 9 files changed, 559 insertions(+), 5 deletions(-) > > diff --git a/drivers/accel/qda/Makefile b/drivers/accel/qda/Makefile > index b658dad35fee..a46ddceecfc5 100644 > --- a/drivers/accel/qda/Makefile > +++ b/drivers/accel/qda/Makefile > @@ -8,7 +8,9 @@ obj-$(CONFIG_DRM_ACCEL_QDA) := qda.o > qda-y := \ > qda_cb.o \ > qda_drv.o \ > + qda_gem.o \ > qda_ioctl.o \ > + qda_memory_dma.o \ > qda_memory_manager.o \ > qda_rpmsg.o > > diff --git a/drivers/accel/qda/qda_drv.c b/drivers/accel/qda/qda_drv.c > index e1fd8bfa12d7..2b14dab95507 100644 > --- a/drivers/accel/qda/qda_drv.c > +++ b/drivers/accel/qda/qda_drv.c > @@ -21,6 +21,7 @@ static int qda_open(struct drm_device *dev, struct drm_file *file) > if (!qda_file_priv) > return -ENOMEM; > > + qda_file_priv->pid = current->pid; > qda_file_priv->qda_dev = qda_dev_from_drm(dev); > file->driver_priv = qda_file_priv; > > @@ -31,6 +32,9 @@ static void qda_postclose(struct drm_device *dev, struct drm_file *file) > { > struct qda_file_priv *qda_file_priv = file->driver_priv; > > + if (qda_file_priv->assigned_iommu_dev) > + qda_memory_manager_release_device(qda_file_priv->assigned_iommu_dev); > + > kfree(qda_file_priv); > file->driver_priv = NULL; > } > diff --git a/drivers/accel/qda/qda_drv.h b/drivers/accel/qda/qda_drv.h > index 690a833d732b..2fe58f3efec0 100644 > --- a/drivers/accel/qda/qda_drv.h > +++ b/drivers/accel/qda/qda_drv.h > @@ -24,6 +24,10 @@ > struct qda_file_priv { > /** @qda_dev: Back-pointer to device structure */ > struct qda_dev *qda_dev; > + /** @assigned_iommu_dev: IOMMU device assigned to this process */ > + struct qda_iommu_device *assigned_iommu_dev; > + /** @pid: Process ID for tracking */ > + pid_t pid; > }; > > /** > diff --git a/drivers/accel/qda/qda_gem.c b/drivers/accel/qda/qda_gem.c > new file mode 100644 > index 000000000000..66e78013a726 > --- /dev/null > +++ b/drivers/accel/qda/qda_gem.c > @@ -0,0 +1,134 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +// Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries. > +#include <drm/drm_gem.h> > +#include <drm/drm_prime.h> > +#include <drm/drm_print.h> > +#include <linux/slab.h> > +#include <linux/dma-mapping.h> > +#include "qda_drv.h" > +#include "qda_gem.h" > +#include "qda_memory_manager.h" > +#include "qda_memory_dma.h" > + > +/** > + * qda_gem_free_object() - Free a GEM object and its associated resources > + * @gem_obj: DRM GEM object to free > + */ > +void qda_gem_free_object(struct drm_gem_object *gem_obj) > +{ > + struct qda_gem_obj *qda_gem_obj = to_qda_gem_obj(gem_obj); > + struct qda_dev *qdev = qda_dev_from_drm(gem_obj->dev); > + > + if (qda_gem_obj->virt && qdev->iommu_mgr) > + qda_memory_manager_free(qdev->iommu_mgr, qda_gem_obj); > + > + drm_gem_object_release(gem_obj); > + kfree(qda_gem_obj); > +} > + > +/** > + * qda_gem_mmap_obj() - Map a GEM object into userspace > + * @drm_obj: DRM GEM object to map > + * @vma: Virtual memory area to map into > + * > + * Return: 0 on success, negative error code on failure > + */ > +int qda_gem_mmap_obj(struct drm_gem_object *drm_obj, struct vm_area_struct *vma) > +{ > + struct qda_gem_obj *qda_gem_obj = to_qda_gem_obj(drm_obj); > + int ret; > + > + /* The fake offset is only used to find the object, not to index it */ > + vma->vm_pgoff = 0; > + > + ret = qda_dma_mmap(qda_gem_obj, vma); > + if (ret) > + return ret; > + > + vm_flags_set(vma, VM_DONTEXPAND | VM_DONTDUMP); > + > + return 0; > +} > + > +static const struct drm_gem_object_funcs qda_gem_object_funcs = { > + .free = qda_gem_free_object, > + .mmap = qda_gem_mmap_obj, > +}; > + > +/** > + * qda_gem_alloc_object() - Allocate a new QDA GEM object > + * @drm_dev: DRM device > + * @aligned_size: Size of the object in bytes (must be page-aligned) > + * > + * Return: Pointer to the new GEM object, or ERR_PTR on failure > + */ > +struct qda_gem_obj *qda_gem_alloc_object(struct drm_device *drm_dev, size_t aligned_size) > +{ > + struct qda_gem_obj *qda_gem_obj; > + int ret; > + > + qda_gem_obj = kzalloc_obj(*qda_gem_obj); > + if (!qda_gem_obj) > + return ERR_PTR(-ENOMEM); > + > + ret = drm_gem_object_init(drm_dev, &qda_gem_obj->base, aligned_size); > + if (ret) { > + kfree(qda_gem_obj); > + return ERR_PTR(ret); > + } > + > + qda_gem_obj->base.funcs = &qda_gem_object_funcs; > + qda_gem_obj->size = aligned_size; > + > + return qda_gem_obj; > +} > + > +void qda_gem_cleanup_object(struct qda_gem_obj *qda_gem_obj) > +{ > + drm_gem_object_release(&qda_gem_obj->base); > + kfree(qda_gem_obj); > +} > + > +int qda_gem_create_handle(struct drm_file *file_priv, struct drm_gem_object *gem_obj, u32 *handle) > +{ > + int ret; > + > + ret = drm_gem_handle_create(file_priv, gem_obj, handle); > + drm_gem_object_put(gem_obj); > + > + return ret; > +} > + > +/** > + * qda_gem_create_object() - Allocate and initialize a GEM object with DMA backing > + * @drm_dev: DRM device > + * @iommu_mgr: Memory manager to use for DMA allocation > + * @size: Requested size in bytes > + * @file_priv: DRM file private data for process association > + * > + * Return: Pointer to the base DRM GEM object on success, ERR_PTR on failure > + */ > +struct drm_gem_object *qda_gem_create_object(struct drm_device *drm_dev, > + struct qda_memory_manager *iommu_mgr, size_t size, > + struct drm_file *file_priv) > +{ > + struct qda_gem_obj *qda_gem_obj; > + size_t aligned_size; > + int ret; > + > + aligned_size = PAGE_ALIGN(size); > + if (!aligned_size) > + return ERR_PTR(-EINVAL); > + > + qda_gem_obj = qda_gem_alloc_object(drm_dev, aligned_size); > + if (IS_ERR(qda_gem_obj)) > + return ERR_CAST(qda_gem_obj); > + > + ret = qda_memory_manager_alloc(iommu_mgr, qda_gem_obj, file_priv); > + if (ret) { > + qda_gem_cleanup_object(qda_gem_obj); > + return ERR_PTR(ret); > + } > + > + return &qda_gem_obj->base; > +} > diff --git a/drivers/accel/qda/qda_gem.h b/drivers/accel/qda/qda_gem.h > new file mode 100644 > index 000000000000..afd7c9b49549 > --- /dev/null > +++ b/drivers/accel/qda/qda_gem.h > @@ -0,0 +1,52 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries. > + */ > +#ifndef __QDA_GEM_H__ > +#define __QDA_GEM_H__ > + > +#include <linux/dma-mapping.h> > +#include <drm/drm_device.h> > +#include <drm/drm_gem.h> > +#include "qda_memory_manager.h" > + > +/** > + * struct qda_gem_obj - QDA GEM buffer object > + * > + * Represents a GEM buffer object that can be allocated by the driver > + * or imported from another driver via DMA-BUF. > + */ > +struct qda_gem_obj { > + /** @base: DRM GEM object base — must be first member */ > + struct drm_gem_object base; > + /** @iommu_dev: IOMMU context bank device that performed the allocation */ > + struct qda_iommu_device *iommu_dev; > + /** @virt: Kernel virtual address of the allocated DMA memory */ > + void *virt; > + /** @dma_addr: DMA address (with SID encoded in upper 32 bits) */ > + dma_addr_t dma_addr; > + /** @size: Size of the buffer in bytes */ > + size_t size; > +}; > + > +/** > + * to_qda_gem_obj - Cast a drm_gem_object pointer to qda_gem_obj > + * @gem_obj: Pointer to the embedded drm_gem_object > + */ > +#define to_qda_gem_obj(gem_obj) container_of(gem_obj, struct qda_gem_obj, base) > + > +/* GEM object lifecycle */ > +struct drm_gem_object *qda_gem_create_object(struct drm_device *drm_dev, > + struct qda_memory_manager *iommu_mgr, > + size_t size, struct drm_file *file_priv); > +void qda_gem_free_object(struct drm_gem_object *gem_obj); > +int qda_gem_mmap_obj(struct drm_gem_object *gem_obj, struct vm_area_struct *vma); > + > +/* Internal helpers (also used by PRIME import) */ > +struct qda_gem_obj *qda_gem_alloc_object(struct drm_device *drm_dev, size_t aligned_size); > +void qda_gem_cleanup_object(struct qda_gem_obj *qda_gem_obj); > + > +/* Utility functions */ > +int qda_gem_create_handle(struct drm_file *file_priv, struct drm_gem_object *gem_obj, u32 *handle); > + > +#endif /* __QDA_GEM_H__ */ > diff --git a/drivers/accel/qda/qda_memory_dma.c b/drivers/accel/qda/qda_memory_dma.c > new file mode 100644 > index 000000000000..7072ed8edf0b > --- /dev/null > +++ b/drivers/accel/qda/qda_memory_dma.c > @@ -0,0 +1,82 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +// Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries. > +#include <linux/slab.h> > +#include <linux/dma-mapping.h> > +#include "qda_drv.h" > +#include "qda_gem.h" > +#include "qda_memory_dma.h" > + > +/* > + * The DSP identifies the IOMMU context bank from the upper bits of the address, > + * so the stream ID is folded into every address handed to it. The DMA API only > + * ever sees the plain 32-bit address, which is why the context bank devices are > + * created with a 32-bit DMA mask. > + */ > +static dma_addr_t get_actual_dma_addr(struct qda_gem_obj *gem_obj) > +{ > + return gem_obj->dma_addr - ((u64)gem_obj->iommu_dev->sid << 32); > +} > + > +static void setup_gem_object(struct qda_gem_obj *gem_obj, void *virt, > + dma_addr_t dma_addr, struct qda_iommu_device *iommu_dev) > +{ > + gem_obj->virt = virt; > + gem_obj->dma_addr = dma_addr; > + gem_obj->iommu_dev = iommu_dev; > +} > + > +static void cleanup_gem_object_fields(struct qda_gem_obj *gem_obj) > +{ > + gem_obj->virt = NULL; > + gem_obj->dma_addr = 0; > + gem_obj->iommu_dev = NULL; > +} > + > +/** > + * qda_dma_alloc() - Allocate DMA coherent memory for a GEM object > + * @iommu_dev: Pointer to the QDA IOMMU device structure > + * @gem_obj: Pointer to GEM object to allocate memory for > + * @size: Size of memory to allocate in bytes > + * > + * Return: 0 on success, negative error code on failure > + */ > +int qda_dma_alloc(struct qda_iommu_device *iommu_dev, > + struct qda_gem_obj *gem_obj, size_t size) > +{ > + dma_addr_t dma_addr; > + void *virt; > + > + virt = dma_alloc_coherent(iommu_dev->dev, size, &dma_addr, GFP_KERNEL); > + if (!virt) > + return -ENOMEM; > + > + dma_addr += ((u64)iommu_dev->sid << 32); > + setup_gem_object(gem_obj, virt, dma_addr, iommu_dev); > + > + return 0; > +} > + > +/** > + * qda_dma_free() - Free DMA coherent memory for a GEM object > + * @gem_obj: Pointer to GEM object to free memory for > + */ > +void qda_dma_free(struct qda_gem_obj *gem_obj) > +{ > + dma_free_coherent(gem_obj->iommu_dev->dev, gem_obj->size, > + gem_obj->virt, get_actual_dma_addr(gem_obj)); > + > + cleanup_gem_object_fields(gem_obj); > +} > + > +/** > + * qda_dma_mmap() - Map DMA memory into userspace > + * @gem_obj: Pointer to GEM object containing DMA memory > + * @vma: Virtual memory area to map into > + * > + * Return: 0 on success, negative error code on failure > + */ > +int qda_dma_mmap(struct qda_gem_obj *gem_obj, struct vm_area_struct *vma) > +{ > + return dma_mmap_coherent(gem_obj->iommu_dev->dev, vma, gem_obj->virt, > + get_actual_dma_addr(gem_obj), gem_obj->size); > +} > diff --git a/drivers/accel/qda/qda_memory_dma.h b/drivers/accel/qda/qda_memory_dma.h > new file mode 100644 > index 000000000000..99352a99dc33 > --- /dev/null > +++ b/drivers/accel/qda/qda_memory_dma.h > @@ -0,0 +1,17 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries. > + */ > + > +#ifndef __QDA_MEMORY_DMA_H__ > +#define __QDA_MEMORY_DMA_H__ > + > +#include <linux/dma-mapping.h> > +#include "qda_memory_manager.h" > + > +int qda_dma_alloc(struct qda_iommu_device *iommu_dev, > + struct qda_gem_obj *gem_obj, size_t size); > +void qda_dma_free(struct qda_gem_obj *gem_obj); > +int qda_dma_mmap(struct qda_gem_obj *gem_obj, struct vm_area_struct *vma); > + > +#endif /* __QDA_MEMORY_DMA_H__ */ > diff --git a/drivers/accel/qda/qda_memory_manager.c b/drivers/accel/qda/qda_memory_manager.c > index b1a80ee77c35..d47c7419e11d 100644 > --- a/drivers/accel/qda/qda_memory_manager.c > +++ b/drivers/accel/qda/qda_memory_manager.c > @@ -1,14 +1,26 @@ > // SPDX-License-Identifier: GPL-2.0-only > // Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries. > > +#include <linux/refcount.h> > #include <linux/slab.h> > +#include <linux/spinlock.h> > +#include <drm/drm_file.h> > +#include <drm/drm_print.h> > #include "qda_drv.h" > +#include "qda_gem.h" > #include "qda_memory_manager.h" > +#include "qda_memory_dma.h" > + > +static void iommu_device_free(struct kref *ref) > +{ > + kfree(container_of(ref, struct qda_iommu_device, ref)); > +} > > static void cleanup_all_memory_devices(struct qda_memory_manager *mem_mgr) > { > int i; > > + mutex_lock(&mem_mgr->process_assignment_lock); > for (i = 0; i < mem_mgr->num_devices; i++) { > struct qda_iommu_device *iommu_dev = mem_mgr->devices[i]; > > @@ -16,9 +28,215 @@ static void cleanup_all_memory_devices(struct qda_memory_manager *mem_mgr) > continue; > > mem_mgr->devices[i] = NULL; > - kfree(iommu_dev); > + kref_put(&iommu_dev->ref, iommu_device_free); > } > mem_mgr->num_devices = 0; > + mutex_unlock(&mem_mgr->process_assignment_lock); > +} > + > +static void init_iommu_device_fields(struct qda_iommu_device *iommu_dev) > +{ > + spin_lock_init(&iommu_dev->lock); > + refcount_set(&iommu_dev->users, 0); > + kref_init(&iommu_dev->ref); > + iommu_dev->assigned_pid = 0; > + iommu_dev->assigned_file_priv = NULL; > +} > + > +static struct qda_iommu_device *find_device_for_pid(struct qda_memory_manager *mem_mgr, > + pid_t pid) > +{ > + struct qda_iommu_device *found_dev = NULL; > + unsigned long flags; > + int i; > + > + for (i = 0; i < mem_mgr->num_devices; i++) { > + struct qda_iommu_device *iommu_dev = mem_mgr->devices[i]; > + > + if (!iommu_dev) > + continue; > + > + spin_lock_irqsave(&iommu_dev->lock, flags); > + if (iommu_dev->assigned_pid == pid) { > + found_dev = iommu_dev; > + refcount_inc(&found_dev->users); > + kref_get(&found_dev->ref); > + spin_unlock_irqrestore(&iommu_dev->lock, flags); > + break; > + } > + spin_unlock_irqrestore(&iommu_dev->lock, flags); > + } > + > + return found_dev; > +} > + > +static struct qda_iommu_device *assign_available_device_to_pid(struct qda_memory_manager *mem_mgr, > + pid_t pid, > + struct drm_file *file_priv) > +{ > + struct qda_iommu_device *selected_dev = NULL; > + unsigned long flags; > + int i; > + > + for (i = 0; i < mem_mgr->num_devices; i++) { > + struct qda_iommu_device *iommu_dev = mem_mgr->devices[i]; > + > + if (!iommu_dev) > + continue; > + > + spin_lock_irqsave(&iommu_dev->lock, flags); > + if (iommu_dev->assigned_pid == 0) { > + iommu_dev->assigned_pid = pid; > + iommu_dev->assigned_file_priv = file_priv; > + selected_dev = iommu_dev; > + refcount_set(&selected_dev->users, 1); > + kref_get(&selected_dev->ref); > + spin_unlock_irqrestore(&iommu_dev->lock, flags); > + break; > + } > + spin_unlock_irqrestore(&iommu_dev->lock, flags); > + } > + > + return selected_dev; > +} > + > +static struct qda_iommu_device *get_process_iommu_device(struct qda_memory_manager *mem_mgr, > + struct drm_file *file_priv) > +{ > + struct qda_file_priv *qda_priv; > + > + if (!file_priv || !file_priv->driver_priv) > + return NULL; > + > + qda_priv = (struct qda_file_priv *)file_priv->driver_priv; > + return qda_priv->assigned_iommu_dev; > +} > + > +/** > + * qda_memory_manager_assign_device() - Assign an IOMMU device to a process > + * @mem_mgr: Pointer to memory manager > + * @file_priv: DRM file private data for process association > + * > + * On success the caller owns a reference on the device and must release it > + * with qda_memory_manager_release_device(). > + * > + * Return: 0 on success, negative error code on failure > + */ > +int qda_memory_manager_assign_device(struct qda_memory_manager *mem_mgr, > + struct drm_file *file_priv) > +{ > + struct qda_file_priv *qda_priv; > + struct qda_iommu_device *selected_dev = NULL; > + int ret = 0; > + pid_t current_pid; > + > + if (!file_priv || !file_priv->driver_priv) > + return -EINVAL; > + > + qda_priv = (struct qda_file_priv *)file_priv->driver_priv; > + current_pid = qda_priv->pid; > + > + mutex_lock(&mem_mgr->process_assignment_lock); > + > + if (qda_priv->assigned_iommu_dev) { > + ret = 0; > + goto unlock_and_return; > + } > + > + selected_dev = find_device_for_pid(mem_mgr, current_pid); > + > + if (selected_dev) { > + qda_priv->assigned_iommu_dev = selected_dev; > + goto unlock_and_return; > + } > + > + selected_dev = assign_available_device_to_pid(mem_mgr, current_pid, file_priv); > + > + if (!selected_dev) { > + drm_dbg_driver(file_priv->minor->dev, "No context bank available for PID %d\n", > + current_pid); > + ret = -EBUSY; > + goto unlock_and_return; > + } > + > + qda_priv->assigned_iommu_dev = selected_dev; > + > +unlock_and_return: > + mutex_unlock(&mem_mgr->process_assignment_lock); > + > + return ret; > +} > + > +/** > + * qda_memory_manager_release_device() - Release a process assignment > + * @iommu_dev: Device previously obtained from qda_memory_manager_assign_device() > + * > + * Drops the caller's reference. Once the last process using the device is > + * gone the assignment is cleared so the device can serve another process. > + */ > +void qda_memory_manager_release_device(struct qda_iommu_device *iommu_dev) > +{ > + unsigned long flags; > + > + if (refcount_dec_and_test(&iommu_dev->users)) { > + spin_lock_irqsave(&iommu_dev->lock, flags); > + iommu_dev->assigned_pid = 0; > + iommu_dev->assigned_file_priv = NULL; > + spin_unlock_irqrestore(&iommu_dev->lock, flags); > + } > + > + kref_put(&iommu_dev->ref, iommu_device_free); > +} > + > +static struct qda_iommu_device *get_or_assign_iommu_device(struct qda_memory_manager *mem_mgr, > + struct drm_file *file_priv) > +{ > + struct qda_iommu_device *iommu_dev; > + int ret; > + > + iommu_dev = get_process_iommu_device(mem_mgr, file_priv); > + if (iommu_dev) > + return iommu_dev; > + > + ret = qda_memory_manager_assign_device(mem_mgr, file_priv); > + if (ret) > + return NULL; > + > + iommu_dev = get_process_iommu_device(mem_mgr, file_priv); > + if (iommu_dev) > + return iommu_dev; > + > + return NULL; > +} > + > +/** > + * qda_memory_manager_alloc() - Allocate memory for a GEM object > + * @mem_mgr: Pointer to memory manager > + * @gem_obj: Pointer to GEM object to allocate memory for > + * @file_priv: DRM file private data for process association > + * > + * Return: 0 on success, negative error code on failure > + */ > +int qda_memory_manager_alloc(struct qda_memory_manager *mem_mgr, struct qda_gem_obj *gem_obj, > + struct drm_file *file_priv) > +{ > + struct qda_iommu_device *selected_dev; > + > + selected_dev = get_or_assign_iommu_device(mem_mgr, file_priv); > + if (!selected_dev) > + return -EBUSY; > + > + return qda_dma_alloc(selected_dev, gem_obj, gem_obj->size); > +} > + > +/** > + * qda_memory_manager_free() - Free memory for a GEM object > + * @mem_mgr: Pointer to memory manager > + * @gem_obj: Pointer to GEM object to free memory for > + */ > +void qda_memory_manager_free(struct qda_memory_manager *mem_mgr, struct qda_gem_obj *gem_obj) > +{ > + qda_dma_free(gem_obj); > } > > /** > @@ -31,13 +249,21 @@ static void cleanup_all_memory_devices(struct qda_memory_manager *mem_mgr) > int qda_memory_manager_register_device(struct qda_memory_manager *mem_mgr, > struct qda_iommu_device *iommu_dev) > { > - if (mem_mgr->num_devices >= mem_mgr->max_devices) > - return -ENOSPC; > + int ret = 0; > > + mutex_lock(&mem_mgr->process_assignment_lock); > + if (mem_mgr->num_devices >= mem_mgr->max_devices) { > + ret = -ENOSPC; > + goto out; > + } > + > + init_iommu_device_fields(iommu_dev); > iommu_dev->id = mem_mgr->num_devices; > mem_mgr->devices[mem_mgr->num_devices++] = iommu_dev; > > - return 0; > +out: > + mutex_unlock(&mem_mgr->process_assignment_lock); > + return ret; > } > > /** > @@ -52,14 +278,16 @@ void qda_memory_manager_unregister_device(struct qda_memory_manager *mem_mgr, > { > int i; > > + mutex_lock(&mem_mgr->process_assignment_lock); > for (i = 0; i < mem_mgr->num_devices; i++) { > if (mem_mgr->devices[i] == iommu_dev) { > mem_mgr->devices[i] = NULL; > break; > } > } > + mutex_unlock(&mem_mgr->process_assignment_lock); > > - kfree(iommu_dev); > + kref_put(&iommu_dev->ref, iommu_device_free); > } > > /** > @@ -80,6 +308,7 @@ int qda_memory_manager_init(struct qda_memory_manager *mem_mgr, int max_devices) > > mem_mgr->num_devices = 0; > mem_mgr->max_devices = max_devices; > + mutex_init(&mem_mgr->process_assignment_lock); > > return 0; > } > diff --git a/drivers/accel/qda/qda_memory_manager.h b/drivers/accel/qda/qda_memory_manager.h > index 7e38c8a18284..0949c0c213f6 100644 > --- a/drivers/accel/qda/qda_memory_manager.h > +++ b/drivers/accel/qda/qda_memory_manager.h > @@ -7,25 +7,44 @@ > #define __QDA_MEMORY_MANAGER_H__ > > #include <linux/device.h> > +#include <linux/kref.h> > +#include <linux/mutex.h> > +#include <linux/refcount.h> > +#include <linux/spinlock.h> > +#include <drm/drm_file.h> > > /* Forward declarations */ > struct qda_dev; > +struct qda_gem_obj; > > /** > * struct qda_iommu_device - IOMMU device instance for memory management > * > * Represents a single IOMMU-enabled device managed by the memory manager. > * Each device can be assigned to a specific process session. > + * > + * The object outlives its registration: open files keep a reference through > + * @ref, so a device can be unregistered while a process still holds it. > */ > struct qda_iommu_device { > /** @dev: Pointer to the underlying device */ > struct device *dev; > /** @qdev: Back-pointer to the parent QDA device */ > struct qda_dev *qdev; > + /** @assigned_file_priv: DRM file private data for the assigned process */ > + struct drm_file *assigned_file_priv; > /** @id: Unique identifier assigned by the memory manager */ > u32 id; > /** @sid: Stream ID for IOMMU transactions */ > u32 sid; > + /** @assigned_pid: Process ID of the process assigned to this device */ > + pid_t assigned_pid; > + /** @users: Number of open files sharing this device */ > + refcount_t users; > + /** @ref: Reference count controlling when the object is freed */ > + struct kref ref; > + /** @lock: Spinlock protecting concurrent access to device */ > + spinlock_t lock; > }; > > /** > @@ -42,6 +61,8 @@ struct qda_memory_manager { > int num_devices; > /** @max_devices: Capacity of the @devices array */ > int max_devices; > + /** @process_assignment_lock: Mutex protecting process-to-device assignments */ > + struct mutex process_assignment_lock; > }; > > int qda_memory_manager_init(struct qda_memory_manager *mem_mgr, int max_devices); > @@ -51,5 +72,14 @@ int qda_memory_manager_register_device(struct qda_memory_manager *mem_mgr, > struct qda_iommu_device *iommu_dev); > void qda_memory_manager_unregister_device(struct qda_memory_manager *mem_mgr, > struct qda_iommu_device *iommu_dev); > +int qda_memory_manager_assign_device(struct qda_memory_manager *mem_mgr, > + struct drm_file *file_priv); > +void qda_memory_manager_release_device(struct qda_iommu_device *iommu_dev); > + > +int qda_memory_manager_alloc(struct qda_memory_manager *mem_mgr, > + struct qda_gem_obj *gem_obj, > + struct drm_file *file_priv); > +void qda_memory_manager_free(struct qda_memory_manager *mem_mgr, > + struct qda_gem_obj *gem_obj); > > #endif /* __QDA_MEMORY_MANAGER_H__ */ >

3 weeks, 1 day

Re: [PATCH v7 0/4] dma-buf: fix fd leak when copy_to_user() fails after fd_install()

by Christian König

I've gone ahead, added one more rb and pushed the result to drm-misc-fixes. Thanks for the help, Christian. On 8/17/26 07:04, Baineng Shou wrote: > Several drivers call dma_buf_fd() — which internally calls fd_install() > — before copy_to_user() returns the fd number to userspace. If > copy_to_user() fails, the fd is already published in the caller's fd > table but the ioctl returns an error, so userspace never learns the fd > number. Worse, the window between fd_install() and copy_to_user() > allows other threads to observe and manipulate the fd (dup, close, > SCM_RIGHTS), making any "close it on the failure path" fix unsafe. > > The fix is to split the allocation into three steps: reserve an fd with > get_unused_fd_flags() (not yet visible to other threads), do > copy_to_user(), and only then publish the fd with fd_install() via the > new dma_buf_fd_install() helper. On copy_to_user() failure, > put_unused_fd() + dma_buf_put() cleanly unwind with no user-visible > side effects. > > Patch 1 introduces dma_buf_fd_install() in dma-buf.c (wrapping > fd_install() together with the DMA_BUF_TRACE call to preserve export > tracing) and applies the fix to dma-heap. > > Patch 2 applies the same fix to fastrpc, which even had a comment > acknowledging the problem could not be fixed before. > > Patch 3 replaces the bare fd_install() in drm_gem_prime_handle_to_fd() > with dma_buf_fd_install() to restore tracepoint coverage for DRM PRIME > exports (suggested by Christian König). > > Patch 4 adds a selftest to tools/testing/selftests/dmabuf-heaps/ that > reproduces the fd-leak scenario (mprotect flip before the ioctl) and > verifies the fd count is unchanged after a failed ioctl (suggested by > Sumit Semwal). > > v1: https://lore.kernel.org/dri-devel/20260703080922.1838362-1-shoubaineng@gmai… > v2: https://lore.kernel.org/dri-devel/20260710105740.3080070-1-shoubaineng@gmai… > v3: https://lore.kernel.org/dri-devel/20260714114654.3885457-1-shoubaineng@gmai… > v5: https://lore.kernel.org/dri-devel/20260730062645.233148-1-shoubaineng@gmail… > v6: https://lore.kernel.org/dri-devel/20260807101140.1357218-1-shoubaineng@gmai… > > Changes in v7: > - Add Reviewed-by: T.J. Mercier to patch 4 (selftest). > - Add Acked-by: Sumit Semwal to the whole series. > > Changes in v6: > - Rework the selftest (patch 4) per review: extract a count_open_fds() > helper, fix the copy_from_user() comment, fail (not skip) when the > ioctl does not return -1, drop the bogus mprotect-race mention, and > reword the result message. > > Changes in v5: > - Add selftest (patch 4) reproducing the fd-leak scenario (Sumit Semwal) > > Changes in v4: > - Add patch 3: drm/prime: use dma_buf_fd_install() (Christian König) > - Add Acked-by: Christian König to patches 1 and 2 > > Changes in v3: > - Split into two patches (dma-heap + fastrpc separately) > - Add dma_buf_fd_install() to preserve trace_dma_buf_fd tracepoint > - Add fastrpc fix using the new helper (T.J. Mercier) > > Baineng Shou (4): > dma-buf: dma-heap: don't publish fd before copy_to_user() succeeds > misc: fastrpc: don't publish fd before copy_to_user() succeeds > drm/prime: use dma_buf_fd_install() to preserve export tracing > selftests: dmabuf-heaps: add fd-leak-on-EFAULT regression test > > drivers/dma-buf/dma-buf.c | 20 ++++ > drivers/dma-buf/dma-heap.c | 80 ++++++------- > drivers/gpu/drm/drm_prime.c | 2 +- > drivers/misc/fastrpc.c | 16 +-- > include/linux/dma-buf.h | 1 + > .../selftests/dmabuf-heaps/dmabuf-heap.c | 113 +++++++++++++++++- > 6 files changed, 180 insertions(+), 52 deletions(-) >

3 weeks, 2 days

Re: [PATCH 2/2] drm/amdgpu/userq: hold the doorbell xa lock during hang reset

by Christian König

Hi, On 8/23/26 08:02, Junrui Luo wrote: > Hi Christian, Alex, > > Sorry for the ping. No need to look at the patch itself. > > I would just be grateful for a quick word on whether the issue it describes > is a real one. If not, I will drop it; if it is, should I send a v2? as far as I can see it is completely nonsense what you try to do here. The fence_drv references an amdgpu_userq object holds are the ones which the queue potentially waits on. When the queue is freed up those references must be dropped, but that shouldn't affect fence_drv->fences in any way possible. What exactly is the leak you are seeing? Regards, Christian. > > Thanks for your time, > Junrui Luo

3 weeks, 2 days

Re: [PATCH 0/2] fpga: add dma-buf based FPGA programming interface

by Christian König

On 8/16/26 17:07, Aravind Thokala wrote: > Some systems need to load large FPGA configuration images. The FPGA > subsystem allows loading images from the filesystem, but this requires > the entire image to be loaded into kernel memory first. For drivers > that need a DMA-capable buffer for programming, the data is then > copied again into DMA memory. This creates needless memory pressure > and delays due to the extra copy. > > This series adds dma-buf support that allows userspace to allocate a > buffer directly from a DMA heap, write the FPGA image into it, and > pass the file descriptor to the kernel via ioctl — skipping the > intermediate kernel buffer entirely. Well when you have a device with limited DMA capabilitiesthen DMA buf heaps doesn't allocate DMA-capable memory for that device either. So the explanation you give above why this interface might be useful is clearly not correct. The DMA subsystem will still do an additional copy when you try to import the DMA-buf allocated from the heap into this device. Either you need to define a heap with specific allocation restrictions (e.g. GFP_DMA32) or you allocate the DMA-buf through your fpga device so that dma_alloc_attrs() knows that a certain device needs to access the pages beforehand. Regards, Christian. > > Userspace flow: > 1. Allocate buffer from /dev/dma_heap/ (e.g., CMA heap) > 2. mmap the buffer and write the FPGA image into it > 3. ioctl(/dev/fpgaX, FPGA_IOCTL_LOAD_DMA_BUF, &dmabuf_fd) > > The dma-buf logic is implemented as a separate layer on top of the > FPGA manager, keeping buffer management separate from the write path. > Individual FPGA drivers opt in by calling fpga_dmabuf_register(). > --- > This work is based on the approach discussed in [1]. > > [1] https://lore.kernel.org/all/20231122053035.3758124-1-nava.kishore.manne@amd… > --- > Aravind Thokala (2): > fpga: Add dma-buf interface for FPGA programming > fpga: versal: add dma-buf programming support > > .../userspace-api/ioctl/ioctl-number.rst | 1 + > MAINTAINERS | 1 + > drivers/fpga/Kconfig | 9 + > drivers/fpga/Makefile | 2 + > drivers/fpga/fpga-dmabuf.c | 198 ++++++++++++++++++ > drivers/fpga/versal-fpga.c | 33 ++- > include/linux/fpga/fpga-dmabuf.h | 22 ++ > include/linux/fpga/fpga-mgr.h | 1 + > include/uapi/linux/fpga.h | 15 ++ > 9 files changed, 281 insertions(+), 1 deletion(-) > create mode 100644 drivers/fpga/fpga-dmabuf.c > create mode 100644 include/linux/fpga/fpga-dmabuf.h > create mode 100644 include/uapi/linux/fpga.h >

3 weeks, 2 days

Re: [PATCH v19 00/40] DEPT(DEPendency Tracker)

by Byungchul Park

On Thu, Aug 20, 2026 at 06:51:08PM +0100, Matthew Wilcox wrote: > On Thu, Aug 20, 2026 at 07:16:05PM +0200, David Hildenbrand (Arm) wrote: > > > Consider this real deadlock pattern that lockdep cannot detect: > > > > > > context X context Y context Z > > > > > > mutex_lock A > > > folio_lock B > > > folio_lock B <- DEADLOCK > > > mutex_lock A <- DEADLOCK > > > folio_unlock B > > > folio_unlock B > > > mutex_unlock A > > > mutex_unlock A > > > > But that really just boils down to folio lock being implemented as a PG_lock + > > some advanced wait mechanism. And we must do that because of lack of bits in > > struct page. > > > > Willy mentioned in a previous version [1]: "I don't think it makes sense to > > track lock state in the page (nor folio). Partly because there's just so many > > of them, but also because the locking rules don't really apply to individual > > folios so much as they do to the mappings (or anon_vmas) that contain folios." > > > > Given that lockdep is a debug feature, and we will at some point allocate struct > > folio separately, I assume we could just squeeze a "struct lockdep_map" in there > > in such debug configs and the world would not collapse. > > > > Doing that today (one "struct lockdep_map" in each "struct page") wouldn't work > > as mm_zero_struct_page() would not expect such large "struct page". But > > conceptually, for a debug kernel with a special CONFIG_LOCKDEP_PAGE_LOCK, maybe > > that would already be ok and we could just do that (and optimize it as we > > allocate folios separately). > > > > Not that it's ideal, but for a debug feature to at least check PG_lock, probably > > an easier way to achieve it than some completely new infrastructure. > > > > Now, Willy said "locking rules don't really apply to individual folios", I > > wonder if that could just help to also let lockdep check PG_lock with less > > metadata? (didn't fully wrap my head around the implications) > > > > [1] > > https://lore.kernel.org/all/aR3WHf9QZ_dizNun@casper.infradead.org/?utm_sour… > > There are a few things going on that make PG_lock special. Let me try > to explain again, only better this time. > > 1. The current lifetime of a struct page is the lifetime of the system. > But the semantics of its PG_lock bit change each time it is freed and > allocated. Yes, it's a classification issue that is very important. > 2. The position of PG_lock in the locking hierarchy only depend on > what the folio is currently being used for. That is, all folios in > a given xfs inode behave exactly the same from a locking perspective. You are exactly explaining what the classification means. Perfect. > There's no need to build up state about how each PG_lock is used; > they can all share. Arguably all xfs file inodes are the same as Right. That's why DEPT doesn't use a full map in each page but just uses a timestamp in each. For the classification, DEPT uses a few classes for folio, using global maps: 1. folios in mm paths 2. folios in block device buffer (meta data) 3. folios in regular file cache However, yes. I bet you could be a big help when classifying folios more presicely according to its usage. But the current classification is still a good start I think. > each other (directory inodes might be different from file inodes), > so we might want to go further than telling DEPT that "this folio > belongs to this inode" and go to "this folio belongs to this xfs file > inode". Totally agree. > 3. PG_lock can be taken in task context then released in interrupt > context. For full points, we need to mark the exact point at which > we submit the folio for read. Otherwise we can get into the situation > alluded to by f2c817bed58d and better discussed at > https://lore.kernel.org/linux-mm/20200127150024.GN1183@dhcp22.suse.cz/ > where we have the folio locked but haven't yet submitted it for I/O > so it doesn't matter how long we wait, it will never come unlocked. Interesting. The following abstraction might make DEPT work with it. For example: Annotate the point submitting IO as an event for the folio_lock() to be released. That way, the issue above can be detected by DEPT. Again, DEPT can do every thing we need w.r.t. deadlock. Byungchul

3 weeks, 5 days

← Newer
1
2
3
4
5
6
7
...
586
Older →

Jump to page:

2026

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig