Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

18 participants
3296 discussions

Re: [PATCH] dma-buf: system_heap: account for system heap allocation in memcg

by T.J. Mercier

On Mon, Dec 15, 2025 at 7:51 PM Maxime Ripard <mripard(a)redhat.com> wrote: > > Hi TJ, Hi Maxime, > On Fri, Dec 12, 2025 at 08:25:19AM +0900, T.J. Mercier wrote: > > On Fri, Dec 12, 2025 at 4:31 AM Eric Chanudet <echanude(a)redhat.com> wrote: > > > > > > The system dma-buf heap lets userspace allocate buffers from the page > > > allocator. However, these allocations are not accounted for in memcg, > > > allowing processes to escape limits that may be configured. > > > > > > Pass the __GFP_ACCOUNT for our allocations to account them into memcg. > > > > We had a discussion just last night in the MM track at LPC about how > > shared memory accounted in memcg is pretty broken. Without a way to > > identify (and possibly transfer) ownership of a shared buffer, this > > makes the accounting of shared memory, and zombie memcg problems > > worse. :\ > > Are there notes or a report from that discussion anywhere? The LPC vids haven't been clipped yet, and actually I can't even find the recorded full live stream from Hall A2 on the first day. So I don't think there's anything to look at, but I bet there's probably nothing there you don't already know. > The way I see it, the dma-buf heaps *trivial* case is non-existent at > the moment and that's definitely broken. Any application can bypass its > cgroups limits trivially, and that's a pretty big hole in the system. Agree, but if we only charge the first allocator then limits can still easily be bypassed assuming an app can cause an allocation outside of its cgroup tree. I'm not sure using static memcg limits where a significant portion of the memory can be shared is really feasible. Even with just pagecache being charged to memcgs, we're having trouble defining a static memcg limit that is really useful since it has to be high enough to accomodate occasional spikes due to shared memory that might or might not be charged (since it can only be charged to one memcg - it may be spread around or it may all get charged to one memcg). So excessive anonymous use has to get really bad before it gets punished. What I've been hearing lately is that folks are polling memory.stat or PSI or other metrics and using that to take actions (memory.reclaim / killing / adjust memory.high) at runtime rather than relying on memory.high/max behavior with a static limit. > The shared ownership is indeed broken, but it's not more or less broken > than, say, memfd + udmabuf, and I'm sure plenty of others. One thing that's worse about system heap buffers is that unlike memfd the memory isn't reclaimable. So without killing all users there's currently no way to deal with the zombie issue. Harry's proposing reparenting, but I don't think our current interfaces support that because we'd have to mess with the page structs behind system heap dmabufs to change the memcg during reparenting. Ah... but udmabuf pins the memfd pages, so you're right that memfd + udmabuf isn't worse. > So we really improve the common case, but only make the "advanced" > slightly more broken than it already is. > > Would you disagree? I think memcg limits in this case just wouldn't be usable because of what I mentioned above. In our common case the allocator is in a different cgroup tree than the real users of the buffer. > Maxime

1 month, 1 week

Re: [PATCH 09/19] drm/sched: use inline locks for the drm-sched-fence

by Christian König

On 12/15/25 16:53, Tvrtko Ursulin wrote: > > On 15/12/2025 15:38, Christian König wrote: >> On 12/15/25 10:20, Tvrtko Ursulin wrote: >>> >>> On 12/12/2025 15:50, Christian König wrote: >>>> On 12/11/25 16:13, Tvrtko Ursulin wrote: >>>>> >>>>> On 11/12/2025 13:16, Christian König wrote: >>>>>> Using the inline lock is now the recommended way for dma_fence implementations. >>>>>> >>>>>> So use this approach for the scheduler fences as well just in case if >>>>>> anybody uses this as blueprint for its own implementation. >>>>>> >>>>>> Also saves about 4 bytes for the external spinlock. >>>>>> >>>>>> Signed-off-by: Christian König <christian.koenig(a)amd.com> >>>>>> --- >>>>>> drivers/gpu/drm/scheduler/sched_fence.c | 7 +++---- >>>>>> include/drm/gpu_scheduler.h | 4 ---- >>>>>> 2 files changed, 3 insertions(+), 8 deletions(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c >>>>>> index 08ccbde8b2f5..47471b9e43f9 100644 >>>>>> --- a/drivers/gpu/drm/scheduler/sched_fence.c >>>>>> +++ b/drivers/gpu/drm/scheduler/sched_fence.c >>>>>> @@ -161,7 +161,7 @@ static void drm_sched_fence_set_deadline_finished(struct dma_fence *f, >>>>>> /* If we already have an earlier deadline, keep it: */ >>>>>> if (test_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags) && >>>>>> ktime_before(fence->deadline, deadline)) { >>>>>> - spin_unlock_irqrestore(&fence->lock, flags); >>>>>> + dma_fence_unlock_irqrestore(f, flags); >>>>> >>>>> Rebase error I guess. Pull into the locking helpers patch. >>>> >>>> No that is actually completely intentional here. >>>> >>>> Previously we had a separate lock which protected both the DMA-fences as well as the deadline state. >>>> >>>> Now we turn that upside down by dropping the separate lock and protecting the deadline state with the dma_fence lock instead. >>> >>> I don't follow. The code is currently like this: >>> >>> static void drm_sched_fence_set_deadline_finished(struct dma_fence *f, >>> ktime_t deadline) >>> { >>> struct drm_sched_fence *fence = to_drm_sched_fence(f); >>> struct dma_fence *parent; >>> unsigned long flags; >>> >>> spin_lock_irqsave(&fence->lock, flags); >>> >>> /* If we already have an earlier deadline, keep it: */ >>> if (test_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags) && >>> ktime_before(fence->deadline, deadline)) { >>> spin_unlock_irqrestore(&fence->lock, flags); >>> return; >>> } >>> >>> fence->deadline = deadline; >>> set_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags); >>> >>> spin_unlock_irqrestore(&fence->lock, flags);... >>> >>> The diff changes one out of the three lock/unlock operations. Other two are changed in 3/19. All three should surely be changed in the same patch. >> >> We could change those spin_lock/unlock calls in patch #3, but I don't think that this is clean. >> >> See the code here currently uses fence->lock and patch #3 would change it to use fence->finished->lock instead. That might be the pointer at the moment, but that is just by coincident and not design. >> >> Only this change here ontop makes it intentional that we use fence->finished->lock for everything. > > Sorry I still don't follow. After 3/19 and before this 9/19 the function looks like this: > > static void drm_sched_fence_set_deadline_finished(struct dma_fence *f, > ktime_t deadline) > { > struct drm_sched_fence *fence = to_drm_sched_fence(f); > struct dma_fence *parent; > unsigned long flags; > > dma_fence_lock_irqsave(f, flags); > > /* If we already have an earlier deadline, keep it: */ > if (test_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags) && > ktime_before(fence->deadline, deadline)) { > spin_unlock_irqrestore(&fence->lock, flags); > return; > } > > fence->deadline = deadline; > set_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags); > > dma_fence_unlock_irqrestore(f, flags); > > Notice the lonely spin_unlock_irqrestore on the early return path while other two use the dma_fence_(un)lock helpers. Am I blind or how is that clean? Oh, that's what you mean. Sorry I was blind! Yeah that is clearly unintentional. Thanks, Christian. > > Regards, > > Tvrtko > >> >> Regards, >> Christian. >> >>> >>> Regards, >>> >>> Tvrtko >>> >>>> >>>> Regards, >>>> Christian. >>>> >>>>> >>>>> Regards, >>>>> >>>>> Tvrtko >>>>> >>>>>> return; >>>>>> } >>>>>> @@ -217,7 +217,6 @@ struct drm_sched_fence *drm_sched_fence_alloc(struct drm_sched_entity *entity, >>>>>> fence->owner = owner; >>>>>> fence->drm_client_id = drm_client_id; >>>>>> - spin_lock_init(&fence->lock); >>>>>> return fence; >>>>>> } >>>>>> @@ -230,9 +229,9 @@ void drm_sched_fence_init(struct drm_sched_fence *fence, >>>>>> fence->sched = entity->rq->sched; >>>>>> seq = atomic_inc_return(&entity->fence_seq); >>>>>> dma_fence_init(&fence->scheduled, &drm_sched_fence_ops_scheduled, >>>>>> - &fence->lock, entity->fence_context, seq); >>>>>> + NULL, entity->fence_context, seq); >>>>>> dma_fence_init(&fence->finished, &drm_sched_fence_ops_finished, >>>>>> - &fence->lock, entity->fence_context + 1, seq); >>>>>> + NULL, entity->fence_context + 1, seq); >>>>>> } >>>>>> module_init(drm_sched_fence_slab_init); >>>>>> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h >>>>>> index fb88301b3c45..b77f24a783e3 100644 >>>>>> --- a/include/drm/gpu_scheduler.h >>>>>> +++ b/include/drm/gpu_scheduler.h >>>>>> @@ -297,10 +297,6 @@ struct drm_sched_fence { >>>>>> * belongs to. >>>>>> */ >>>>>> struct drm_gpu_scheduler *sched; >>>>>> - /** >>>>>> - * @lock: the lock used by the scheduled and the finished fences. >>>>>> - */ >>>>>> - spinlock_t lock; >>>>>> /** >>>>>> * @owner: job owner for debugging >>>>>> */ >>>>> >>>> >>> >> >

1 month, 1 week

Re: [PATCH 09/19] drm/sched: use inline locks for the drm-sched-fence

by Christian König

On 12/15/25 10:20, Tvrtko Ursulin wrote: > > On 12/12/2025 15:50, Christian König wrote: >> On 12/11/25 16:13, Tvrtko Ursulin wrote: >>> >>> On 11/12/2025 13:16, Christian König wrote: >>>> Using the inline lock is now the recommended way for dma_fence implementations. >>>> >>>> So use this approach for the scheduler fences as well just in case if >>>> anybody uses this as blueprint for its own implementation. >>>> >>>> Also saves about 4 bytes for the external spinlock. >>>> >>>> Signed-off-by: Christian König <christian.koenig(a)amd.com> >>>> --- >>>> drivers/gpu/drm/scheduler/sched_fence.c | 7 +++---- >>>> include/drm/gpu_scheduler.h | 4 ---- >>>> 2 files changed, 3 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c >>>> index 08ccbde8b2f5..47471b9e43f9 100644 >>>> --- a/drivers/gpu/drm/scheduler/sched_fence.c >>>> +++ b/drivers/gpu/drm/scheduler/sched_fence.c >>>> @@ -161,7 +161,7 @@ static void drm_sched_fence_set_deadline_finished(struct dma_fence *f, >>>> /* If we already have an earlier deadline, keep it: */ >>>> if (test_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags) && >>>> ktime_before(fence->deadline, deadline)) { >>>> - spin_unlock_irqrestore(&fence->lock, flags); >>>> + dma_fence_unlock_irqrestore(f, flags); >>> >>> Rebase error I guess. Pull into the locking helpers patch. >> >> No that is actually completely intentional here. >> >> Previously we had a separate lock which protected both the DMA-fences as well as the deadline state. >> >> Now we turn that upside down by dropping the separate lock and protecting the deadline state with the dma_fence lock instead. > > I don't follow. The code is currently like this: > > static void drm_sched_fence_set_deadline_finished(struct dma_fence *f, > ktime_t deadline) > { > struct drm_sched_fence *fence = to_drm_sched_fence(f); > struct dma_fence *parent; > unsigned long flags; > > spin_lock_irqsave(&fence->lock, flags); > > /* If we already have an earlier deadline, keep it: */ > if (test_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags) && > ktime_before(fence->deadline, deadline)) { > spin_unlock_irqrestore(&fence->lock, flags); > return; > } > > fence->deadline = deadline; > set_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags); > > spin_unlock_irqrestore(&fence->lock, flags);... > > The diff changes one out of the three lock/unlock operations. Other two are changed in 3/19. All three should surely be changed in the same patch. We could change those spin_lock/unlock calls in patch #3, but I don't think that this is clean. See the code here currently uses fence->lock and patch #3 would change it to use fence->finished->lock instead. That might be the pointer at the moment, but that is just by coincident and not design. Only this change here ontop makes it intentional that we use fence->finished->lock for everything. Regards, Christian. > > Regards, > > Tvrtko > >> >> Regards, >> Christian. >> >>> >>> Regards, >>> >>> Tvrtko >>> >>>> return; >>>> } >>>> @@ -217,7 +217,6 @@ struct drm_sched_fence *drm_sched_fence_alloc(struct drm_sched_entity *entity, >>>> fence->owner = owner; >>>> fence->drm_client_id = drm_client_id; >>>> - spin_lock_init(&fence->lock); >>>> return fence; >>>> } >>>> @@ -230,9 +229,9 @@ void drm_sched_fence_init(struct drm_sched_fence *fence, >>>> fence->sched = entity->rq->sched; >>>> seq = atomic_inc_return(&entity->fence_seq); >>>> dma_fence_init(&fence->scheduled, &drm_sched_fence_ops_scheduled, >>>> - &fence->lock, entity->fence_context, seq); >>>> + NULL, entity->fence_context, seq); >>>> dma_fence_init(&fence->finished, &drm_sched_fence_ops_finished, >>>> - &fence->lock, entity->fence_context + 1, seq); >>>> + NULL, entity->fence_context + 1, seq); >>>> } >>>> module_init(drm_sched_fence_slab_init); >>>> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h >>>> index fb88301b3c45..b77f24a783e3 100644 >>>> --- a/include/drm/gpu_scheduler.h >>>> +++ b/include/drm/gpu_scheduler.h >>>> @@ -297,10 +297,6 @@ struct drm_sched_fence { >>>> * belongs to. >>>> */ >>>> struct drm_gpu_scheduler *sched; >>>> - /** >>>> - * @lock: the lock used by the scheduled and the finished fences. >>>> - */ >>>> - spinlock_t lock; >>>> /** >>>> * @owner: job owner for debugging >>>> */ >>> >> >

1 month, 1 week

Re: [PATCH] dma-buf: system_heap: account for system heap allocation in memcg

by Christian König

On 12/15/25 11:51, Maxime Ripard wrote: > Hi TJ, > > On Fri, Dec 12, 2025 at 08:25:19AM +0900, T.J. Mercier wrote: >> On Fri, Dec 12, 2025 at 4:31 AM Eric Chanudet <echanude(a)redhat.com> wrote: >>> >>> The system dma-buf heap lets userspace allocate buffers from the page >>> allocator. However, these allocations are not accounted for in memcg, >>> allowing processes to escape limits that may be configured. >>> >>> Pass the __GFP_ACCOUNT for our allocations to account them into memcg. >> >> We had a discussion just last night in the MM track at LPC about how >> shared memory accounted in memcg is pretty broken. Without a way to >> identify (and possibly transfer) ownership of a shared buffer, this >> makes the accounting of shared memory, and zombie memcg problems >> worse. :\ > > Are there notes or a report from that discussion anywhere? > > The way I see it, the dma-buf heaps *trivial* case is non-existent at > the moment and that's definitely broken. Any application can bypass its > cgroups limits trivially, and that's a pretty big hole in the system. Well, that is just the tip of the iceberg. Pretty much all driver interfaces doesn't account to memcg at the moment, all the way from alsa, over GPUs (both TTM and SHM-GEM) to V4L2. > The shared ownership is indeed broken, but it's not more or less broken > than, say, memfd + udmabuf, and I'm sure plenty of others. > > So we really improve the common case, but only make the "advanced" > slightly more broken than it already is. > > Would you disagree? I strongly disagree. As far as I can see there is a huge chance we break existing use cases with that. There has been some work on TTM by Dave but I still haven't found time to wrap my head around all possible side effects such a change can have. The fundamental problem is that neither memcg nor the classic resource tracking (e.g. the OOM killer) has a good understanding of shared resources. For example you can use memfd to basically kill any process in the system because the OOM killer can't identify the process which holds the reference to the memory in question. And that is a *MUCH* bigger problem than just inaccurate memcg accounting. Regards, Christian. > > Maxime

1 month, 1 week

Re: [PATCH 14/19] drm/amdgpu: independence for the amdkfd_fence! v4

by Christian König

On 12/11/25 16:08, Philipp Stanner wrote: > On Thu, 2025-12-11 at 13:16 +0100, Christian König wrote: >> This allows amdkfd_fences to outlive the amdgpu module. >> >> v2: implement Felix suggestion to lock the fence while signaling it. >> v3: fix typos >> v4: fix return code in signal_eviction_fence >> >> Signed-off-by: Christian König <christian.koenig(a)amd.com> >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h | 7 +++ >> .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c | 44 +++++++++---------- >> drivers/gpu/drm/amd/amdkfd/kfd_process.c | 2 +- >> drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 4 +- >> 4 files changed, 31 insertions(+), 26 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h >> index 8bdfcde2029b..2f2b277cfaed 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h >> @@ -196,6 +196,7 @@ int kfd_debugfs_kfd_mem_limits(struct seq_file *m, void *data); >> #endif >> #if IS_ENABLED(CONFIG_HSA_AMD) >> bool amdkfd_fence_check_mm(struct dma_fence *f, struct mm_struct *mm); >> +bool amdkfd_fence_signal(struct dma_fence *f); >> struct amdgpu_amdkfd_fence *to_amdgpu_amdkfd_fence(struct dma_fence *f); >> void amdgpu_amdkfd_remove_all_eviction_fences(struct amdgpu_bo *bo); >> int amdgpu_amdkfd_evict_userptr(struct mmu_interval_notifier *mni, >> @@ -210,6 +211,12 @@ bool amdkfd_fence_check_mm(struct dma_fence *f, struct mm_struct *mm) >> return false; >> } >> >> +static inline >> +bool amdkfd_fence_signal(struct dma_fence *f) >> +{ >> + return false; >> +} > > Huh? What's that? > > That function seems to be just a NOP. It's return code is used nowhere, > is it? It's the dummy which is used when CONFIG_HSA_AMD isn't enabled. Not sure if it's actually used or not, but we have dummies for all functions declared in this file. > >> + >> static inline >> struct amdgpu_amdkfd_fence *to_amdgpu_amdkfd_fence(struct dma_fence *f) >> { >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c >> index 09c919f72b6c..9cd413e325f0 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c >> @@ -127,29 +127,9 @@ static bool amdkfd_fence_enable_signaling(struct dma_fence *f) >> if (!svm_range_schedule_evict_svm_bo(fence)) >> return true; >> } >> - return false; >> -} >> - >> -/** >> - * amdkfd_fence_release - callback that fence can be freed >> - * >> - * @f: dma_fence >> - * >> - * This function is called when the reference count becomes zero. >> - * Drops the mm_struct reference and RCU schedules freeing up the fence. >> - */ >> -static void amdkfd_fence_release(struct dma_fence *f) >> -{ >> - struct amdgpu_amdkfd_fence *fence = to_amdgpu_amdkfd_fence(f); >> - >> - /* Unconditionally signal the fence. The process is getting >> - * terminated. >> - */ >> - if (WARN_ON(!fence)) >> - return; /* Not an amdgpu_amdkfd_fence */ >> - >> mmdrop(fence->mm); >> - kfree_rcu(f, rcu); >> + fence->mm = NULL; >> + return false; >> } >> >> /** >> @@ -174,9 +154,27 @@ bool amdkfd_fence_check_mm(struct dma_fence *f, struct mm_struct *mm) >> return false; >> } >> >> +bool amdkfd_fence_signal(struct dma_fence *f) >> +{ >> + struct amdgpu_amdkfd_fence *fence = to_amdgpu_amdkfd_fence(f); >> + unsigned long flags; >> + bool was_signaled; >> + >> + dma_fence_lock_irqsave(f, flags); >> + if (fence->mm) { >> + mmdrop(fence->mm); >> + fence->mm = NULL; >> + } >> + was_signaled = dma_fence_is_signaled_locked(f); >> + if (!was_signaled) >> + dma_fence_signal_locked(f); >> + dma_fence_unlock_irqrestore(f, flags); >> + >> + return was_signaled; >> +} >> + >> static const struct dma_fence_ops amdkfd_fence_ops = { >> .get_driver_name = amdkfd_fence_get_driver_name, >> .get_timeline_name = amdkfd_fence_get_timeline_name, >> .enable_signaling = amdkfd_fence_enable_signaling, >> - .release = amdkfd_fence_release, >> }; >> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c >> index bb252ec43733..2cf39e3d3fae 100644 >> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c >> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c >> @@ -1173,7 +1173,7 @@ static void kfd_process_wq_release(struct work_struct *work) >> synchronize_rcu(); >> ef = rcu_access_pointer(p->ef); >> if (ef) >> - dma_fence_signal(ef); >> + amdkfd_fence_signal(ef); >> >> kfd_process_remove_sysfs(p); >> kfd_debugfs_remove_process(p); >> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c >> index 97c2270f278f..0e94f3a976b1 100644 >> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c >> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c >> @@ -428,7 +428,7 @@ static void svm_range_bo_release(struct kref *kref) >> >> if (!dma_fence_is_signaled(&svm_bo->eviction_fence->base)) >> /* We're not in the eviction worker. Signal the fence. */ >> - dma_fence_signal(&svm_bo->eviction_fence->base); >> + amdkfd_fence_signal(&svm_bo->eviction_fence->base); >> dma_fence_put(&svm_bo->eviction_fence->base); >> amdgpu_bo_unref(&svm_bo->bo); >> kfree(svm_bo); >> @@ -3628,7 +3628,7 @@ static void svm_range_evict_svm_bo_worker(struct work_struct *work) >> mmap_read_unlock(mm); >> mmput(mm); >> >> - dma_fence_signal(&svm_bo->eviction_fence->base); >> + amdkfd_fence_signal(&svm_bo->eviction_fence->base); > > > And why do you do those changes and why doesn't the commit message > explain it? > > You stop signalling those fences, after all. Hui? I don't stop signaling the fences. I just delegate signaling into a separate helper function which does some extra cleanup before signaling the fence. Regards, Christian. > > > P. > >> >> /* This is the last reference to svm_bo, after svm_range_vram_node_free >> * has been called in svm_migrate_vram_to_ram >

1 month, 1 week

Re: [PATCH 09/19] drm/sched: use inline locks for the drm-sched-fence

by Christian König

On 12/11/25 16:13, Tvrtko Ursulin wrote: > > On 11/12/2025 13:16, Christian König wrote: >> Using the inline lock is now the recommended way for dma_fence implementations. >> >> So use this approach for the scheduler fences as well just in case if >> anybody uses this as blueprint for its own implementation. >> >> Also saves about 4 bytes for the external spinlock. >> >> Signed-off-by: Christian König <christian.koenig(a)amd.com> >> --- >> drivers/gpu/drm/scheduler/sched_fence.c | 7 +++---- >> include/drm/gpu_scheduler.h | 4 ---- >> 2 files changed, 3 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c >> index 08ccbde8b2f5..47471b9e43f9 100644 >> --- a/drivers/gpu/drm/scheduler/sched_fence.c >> +++ b/drivers/gpu/drm/scheduler/sched_fence.c >> @@ -161,7 +161,7 @@ static void drm_sched_fence_set_deadline_finished(struct dma_fence *f, >> /* If we already have an earlier deadline, keep it: */ >> if (test_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags) && >> ktime_before(fence->deadline, deadline)) { >> - spin_unlock_irqrestore(&fence->lock, flags); >> + dma_fence_unlock_irqrestore(f, flags); > > Rebase error I guess. Pull into the locking helpers patch. No that is actually completely intentional here. Previously we had a separate lock which protected both the DMA-fences as well as the deadline state. Now we turn that upside down by dropping the separate lock and protecting the deadline state with the dma_fence lock instead. Regards, Christian. > > Regards, > > Tvrtko > >> return; >> } >> @@ -217,7 +217,6 @@ struct drm_sched_fence *drm_sched_fence_alloc(struct drm_sched_entity *entity, >> fence->owner = owner; >> fence->drm_client_id = drm_client_id; >> - spin_lock_init(&fence->lock); >> return fence; >> } >> @@ -230,9 +229,9 @@ void drm_sched_fence_init(struct drm_sched_fence *fence, >> fence->sched = entity->rq->sched; >> seq = atomic_inc_return(&entity->fence_seq); >> dma_fence_init(&fence->scheduled, &drm_sched_fence_ops_scheduled, >> - &fence->lock, entity->fence_context, seq); >> + NULL, entity->fence_context, seq); >> dma_fence_init(&fence->finished, &drm_sched_fence_ops_finished, >> - &fence->lock, entity->fence_context + 1, seq); >> + NULL, entity->fence_context + 1, seq); >> } >> module_init(drm_sched_fence_slab_init); >> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h >> index fb88301b3c45..b77f24a783e3 100644 >> --- a/include/drm/gpu_scheduler.h >> +++ b/include/drm/gpu_scheduler.h >> @@ -297,10 +297,6 @@ struct drm_sched_fence { >> * belongs to. >> */ >> struct drm_gpu_scheduler *sched; >> - /** >> - * @lock: the lock used by the scheduled and the finished fences. >> - */ >> - spinlock_t lock; >> /** >> * @owner: job owner for debugging >> */ >

1 month, 1 week

Re: [PATCH 11/19] drm/amdgpu: independence for the amdgpu_fence! v2

by Christian König

On 12/11/25 16:53, Tvrtko Ursulin wrote: > > On 11/12/2025 13:16, Christian König wrote: >> This allows amdgpu_fences to outlive the amdgpu module. >> >> v2: use dma_fence_get_rcu_safe to be NULL safe here. >> >> Signed-off-by: Christian König <christian.koenig(a)amd.com> >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 63 +++++++---------------- >> drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 1 - >> 2 files changed, 20 insertions(+), 44 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c >> index c7843e336310..c636347801c1 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c >> @@ -112,8 +112,7 @@ int amdgpu_fence_emit(struct amdgpu_ring *ring, struct amdgpu_fence *af, >> af->ring = ring; >> seq = ++ring->fence_drv.sync_seq; >> - dma_fence_init(fence, &amdgpu_fence_ops, >> - &ring->fence_drv.lock, >> + dma_fence_init(fence, &amdgpu_fence_ops, NULL, >> adev->fence_context + ring->idx, seq); >> amdgpu_ring_emit_fence(ring, ring->fence_drv.gpu_addr, >> @@ -467,7 +466,6 @@ int amdgpu_fence_driver_init_ring(struct amdgpu_ring *ring) >> timer_setup(&ring->fence_drv.fallback_timer, amdgpu_fence_fallback, 0); >> ring->fence_drv.num_fences_mask = ring->num_hw_submission * 2 - 1; >> - spin_lock_init(&ring->fence_drv.lock); >> ring->fence_drv.fences = kcalloc(ring->num_hw_submission * 2, sizeof(void *), >> GFP_KERNEL); >> @@ -654,16 +652,20 @@ void amdgpu_fence_driver_set_error(struct amdgpu_ring *ring, int error) >> struct amdgpu_fence_driver *drv = &ring->fence_drv; >> unsigned long flags; >> - spin_lock_irqsave(&drv->lock, flags); >> + rcu_read_lock(); >> for (unsigned int i = 0; i <= drv->num_fences_mask; ++i) { >> struct dma_fence *fence; >> - fence = rcu_dereference_protected(drv->fences[i], >> - lockdep_is_held(&drv->lock)); >> - if (fence && !dma_fence_is_signaled_locked(fence)) >> + fence = dma_fence_get_rcu(drv->fences[i]); > > dma_fence_get_rcu is not safe against passing a NULL fence in, while the existing code makes it look like drv->fence[] slot can contain NULL at this point? > > amdgpu_fence_process() is the place which can NULL the slots? Irq context? Why is that safe with no reference held from clearing the slot to operating on the fence? The slots are never cleared. It can only be that they are NULL when the driver is loaded. I've switched over to dma_fence_get_rcu_safe() where appropriated. Regards, Christian. > >> + if (!fence) >> + continue; >> + >> + dma_fence_lock_irqsave(fence, flags); >> + if (!dma_fence_is_signaled_locked(fence)) >> dma_fence_set_error(fence, error); >> + dma_fence_unlock_irqrestore(fence, flags); >> } >> - spin_unlock_irqrestore(&drv->lock, flags); >> + rcu_read_unlock(); >> } >> /** >> @@ -714,16 +716,19 @@ void amdgpu_fence_driver_guilty_force_completion(struct amdgpu_fence *af) >> seq = ring->fence_drv.sync_seq & ring->fence_drv.num_fences_mask; >> /* mark all fences from the guilty context with an error */ >> - spin_lock_irqsave(&ring->fence_drv.lock, flags); >> + rcu_read_lock(); >> do { >> last_seq++; >> last_seq &= ring->fence_drv.num_fences_mask; >> ptr = &ring->fence_drv.fences[last_seq]; >> - rcu_read_lock(); >> - unprocessed = rcu_dereference(*ptr); >> + unprocessed = dma_fence_get_rcu_safe(ptr); > > Similar concern like the above. > > Regards, > > Tvrtko >> + >> + if (!unprocessed) >> + continue; >> - if (unprocessed && !dma_fence_is_signaled_locked(unprocessed)) { >> + dma_fence_lock_irqsave(unprocessed, flags); >> + if (dma_fence_is_signaled_locked(unprocessed)) { >> fence = container_of(unprocessed, struct amdgpu_fence, base); >> if (fence == af) >> @@ -731,9 +736,10 @@ void amdgpu_fence_driver_guilty_force_completion(struct amdgpu_fence *af) >> else if (fence->context == af->context) >> dma_fence_set_error(&fence->base, -ECANCELED); >> } >> - rcu_read_unlock(); >> + dma_fence_unlock_irqrestore(unprocessed, flags); >> + dma_fence_put(unprocessed); >> } while (last_seq != seq); >> - spin_unlock_irqrestore(&ring->fence_drv.lock, flags); >> + rcu_read_unlock(); >> /* signal the guilty fence */ >> amdgpu_fence_write(ring, (u32)af->base.seqno); >> amdgpu_fence_process(ring); >> @@ -823,39 +829,10 @@ static bool amdgpu_fence_enable_signaling(struct dma_fence *f) >> return true; >> } >> -/** >> - * amdgpu_fence_free - free up the fence memory >> - * >> - * @rcu: RCU callback head >> - * >> - * Free up the fence memory after the RCU grace period. >> - */ >> -static void amdgpu_fence_free(struct rcu_head *rcu) >> -{ >> - struct dma_fence *f = container_of(rcu, struct dma_fence, rcu); >> - >> - /* free fence_slab if it's separated fence*/ >> - kfree(to_amdgpu_fence(f)); >> -} >> - >> -/** >> - * amdgpu_fence_release - callback that fence can be freed >> - * >> - * @f: fence >> - * >> - * This function is called when the reference count becomes zero. >> - * It just RCU schedules freeing up the fence. >> - */ >> -static void amdgpu_fence_release(struct dma_fence *f) >> -{ >> - call_rcu(&f->rcu, amdgpu_fence_free); >> -} >> - >> static const struct dma_fence_ops amdgpu_fence_ops = { >> .get_driver_name = amdgpu_fence_get_driver_name, >> .get_timeline_name = amdgpu_fence_get_timeline_name, >> .enable_signaling = amdgpu_fence_enable_signaling, >> - .release = amdgpu_fence_release, >> }; >> /* >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h >> index 7a27c6c4bb44..9cbf63454004 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h >> @@ -125,7 +125,6 @@ struct amdgpu_fence_driver { >> unsigned irq_type; >> struct timer_list fallback_timer; >> unsigned num_fences_mask; >> - spinlock_t lock; >> struct dma_fence **fences; >> }; >> >

1 month, 1 week

Re: [PATCH 15/19] drm/amdgpu: independence for the amdgpu_userq_fence!

by Christian König

On 12/11/25 17:12, Tvrtko Ursulin wrote: > > On 11/12/2025 13:16, Christian König wrote: >> This allows amdgpu_userq_fences to outlive the amdgpu module. >> >> Signed-off-by: Christian König <christian.koenig(a)amd.com> >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 13 +---- >> .../gpu/drm/amd/amdgpu/amdgpu_userq_fence.c | 54 ++++--------------- >> .../gpu/drm/amd/amdgpu/amdgpu_userq_fence.h | 8 --- >> 3 files changed, 11 insertions(+), 64 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c >> index 2dfbddcef9ab..f206297aae8b 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c >> @@ -3155,11 +3155,7 @@ static int __init amdgpu_init(void) >> r = amdgpu_sync_init(); >> if (r) >> - goto error_sync; >> - >> - r = amdgpu_userq_fence_slab_init(); >> - if (r) >> - goto error_fence; >> + return r; >> DRM_INFO("amdgpu kernel modesetting enabled.\n"); >> amdgpu_register_atpx_handler(); >> @@ -3176,12 +3172,6 @@ static int __init amdgpu_init(void) >> /* let modprobe override vga console setting */ >> return pci_register_driver(&amdgpu_kms_pci_driver); >> - >> -error_fence: >> - amdgpu_sync_fini(); >> - >> -error_sync: >> - return r; >> } >> static void __exit amdgpu_exit(void) >> @@ -3191,7 +3181,6 @@ static void __exit amdgpu_exit(void) >> amdgpu_unregister_atpx_handler(); >> amdgpu_acpi_release(); >> amdgpu_sync_fini(); >> - amdgpu_userq_fence_slab_fini(); >> mmu_notifier_synchronize(); >> amdgpu_xcp_drv_release(); >> } >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c >> index eba9fb359047..bb19f72770b0 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c >> @@ -33,26 +33,6 @@ >> #include "amdgpu_userq_fence.h" >> static const struct dma_fence_ops amdgpu_userq_fence_ops; >> -static struct kmem_cache *amdgpu_userq_fence_slab; >> - >> -int amdgpu_userq_fence_slab_init(void) >> -{ >> - amdgpu_userq_fence_slab = kmem_cache_create("amdgpu_userq_fence", >> - sizeof(struct amdgpu_userq_fence), >> - 0, >> - SLAB_HWCACHE_ALIGN, >> - NULL); >> - if (!amdgpu_userq_fence_slab) >> - return -ENOMEM; >> - >> - return 0; >> -} >> - >> -void amdgpu_userq_fence_slab_fini(void) >> -{ >> - rcu_barrier(); > > What was this rcu_barrier() for? Cargo culted or more to it? All dma_fences are RCU protected. When they are backed by a kmem_cache you need to make sure to wait for an RCU grace period to pass before destroying the kmem_cache. Since the dma_fence framework now uses kfree_rcu that shouldn't be problematic any more. >> - kmem_cache_destroy(amdgpu_userq_fence_slab); >> -} >> static inline struct amdgpu_userq_fence *to_amdgpu_userq_fence(struct dma_fence *f) >> { >> @@ -227,7 +207,7 @@ void amdgpu_userq_fence_driver_put(struct amdgpu_userq_fence_driver *fence_drv) >> static int amdgpu_userq_fence_alloc(struct amdgpu_userq_fence **userq_fence) >> { >> - *userq_fence = kmem_cache_alloc(amdgpu_userq_fence_slab, GFP_ATOMIC); >> + *userq_fence = kmalloc(sizeof(**userq_fence), GFP_ATOMIC); > This GFP_ATOMIC is suboptimal for sure being on the ioctl path. It is outside of the scope for this patch, but once my userq cleanup patches get reviewed next on my list was to try and understand this. >> return *userq_fence ? 0 : -ENOMEM; >> } >> @@ -243,12 +223,11 @@ static int amdgpu_userq_fence_create(struct amdgpu_usermode_queue *userq, >> if (!fence_drv) >> return -EINVAL; >> - spin_lock_init(&userq_fence->lock); >> INIT_LIST_HEAD(&userq_fence->link); >> fence = &userq_fence->base; >> userq_fence->fence_drv = fence_drv; >> - dma_fence_init64(fence, &amdgpu_userq_fence_ops, &userq_fence->lock, >> + dma_fence_init64(fence, &amdgpu_userq_fence_ops, NULL, >> fence_drv->context, seq); >> amdgpu_userq_fence_driver_get(fence_drv); >> @@ -318,35 +297,22 @@ static bool amdgpu_userq_fence_signaled(struct dma_fence *f) >> rptr = amdgpu_userq_fence_read(fence_drv); >> wptr = fence->base.seqno; >> - if (rptr >= wptr) >> + if (rptr >= wptr) { >> + amdgpu_userq_fence_driver_put(fence->fence_drv); > > fence_drv is in a local already. > >> + fence->fence_drv = NULL; > > amdgpu_userq_fence_get_timeline_name could now oops somehow? > >> + >> + kvfree(fence->fence_drv_array); >> + fence->fence_drv_array = NULL; > > Not sure if this is safe either. amdgpu_userq_fence_driver_process() drops its reference before it unlinks the fence from the list. Can someone external trigger the fence_is_signaled check, before the interrupt processing kicks in, which will clear fence_drv_array, and so amdgpu_userq_fence_driver_process() would oops? Oh, good question. I need to double check that. Thanks, Christian. > > Regards, > > Tvrtko > >> return true; >> + } >> return false; >> } >> -static void amdgpu_userq_fence_free(struct rcu_head *rcu) >> -{ >> - struct dma_fence *fence = container_of(rcu, struct dma_fence, rcu); >> - struct amdgpu_userq_fence *userq_fence = to_amdgpu_userq_fence(fence); >> - struct amdgpu_userq_fence_driver *fence_drv = userq_fence->fence_drv; >> - >> - /* Release the fence driver reference */ >> - amdgpu_userq_fence_driver_put(fence_drv); >> - >> - kvfree(userq_fence->fence_drv_array); >> - kmem_cache_free(amdgpu_userq_fence_slab, userq_fence); >> -} >> - >> -static void amdgpu_userq_fence_release(struct dma_fence *f) >> -{ >> - call_rcu(&f->rcu, amdgpu_userq_fence_free); >> -} >> - >> static const struct dma_fence_ops amdgpu_userq_fence_ops = { >> .get_driver_name = amdgpu_userq_fence_get_driver_name, >> .get_timeline_name = amdgpu_userq_fence_get_timeline_name, >> .signaled = amdgpu_userq_fence_signaled, >> - .release = amdgpu_userq_fence_release, >> }; >> /** >> @@ -560,7 +526,7 @@ int amdgpu_userq_signal_ioctl(struct drm_device *dev, void *data, >> r = amdgpu_userq_fence_create(queue, userq_fence, wptr, &fence); >> if (r) { >> mutex_unlock(&userq_mgr->userq_mutex); >> - kmem_cache_free(amdgpu_userq_fence_slab, userq_fence); >> + kfree(userq_fence); >> goto put_gobj_write; >> } >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.h >> index d76add2afc77..6f04782f3ea9 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.h >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.h >> @@ -31,11 +31,6 @@ >> struct amdgpu_userq_fence { >> struct dma_fence base; >> - /* >> - * This lock is necessary to synchronize the >> - * userqueue dma fence operations. >> - */ >> - spinlock_t lock; >> struct list_head link; >> unsigned long fence_drv_array_count; >> struct amdgpu_userq_fence_driver *fence_drv; >> @@ -58,9 +53,6 @@ struct amdgpu_userq_fence_driver { >> char timeline_name[TASK_COMM_LEN]; >> }; >> -int amdgpu_userq_fence_slab_init(void); >> -void amdgpu_userq_fence_slab_fini(void); >> - >> void amdgpu_userq_fence_driver_get(struct amdgpu_userq_fence_driver *fence_drv); >> void amdgpu_userq_fence_driver_put(struct amdgpu_userq_fence_driver *fence_drv); >> int amdgpu_userq_fence_driver_alloc(struct amdgpu_device *adev, >

1 month, 1 week

Re: [PATCH v2] drm: Fix object leak in DRM_IOCTL_GEM_CHANGE_HANDLE

by Christian König

On 12/12/25 14:20, Karol Wachowski wrote: > Add missing drm_gem_object_put() call when drm_gem_object_lookup() > successfully returns an object. This fixes a GEM object reference > leak that can prevent driver modules from unloading when using > prime buffers. > > Fixes: 53096728b891 ("drm: Add DRM prime interface to reassign GEM handle") > Signed-off-by: Karol Wachowski <karol.wachowski(a)linux.intel.com> > --- > Changes between v1 and v2: > - move setting ret value under if branch as suggested in review > - add Cc: stable 6.18+ Oh don't CC the stable list on the review mail directly, just add "CC: stable(a)vger.kernel.org # 6.18+" to the tags. Greg is going to complain about that :( With that done Reviewed-by: Christian König <christian.koenig(a)amd.com> and please push to drm-misc-fixes. If you don't have commit rights for drm-misc-fixes please ping me and I'm going to push that. Thanks, Christian. > --- > drivers/gpu/drm/drm_gem.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > index ca1956608261..bcc08a6aebf8 100644 > --- a/drivers/gpu/drm/drm_gem.c > +++ b/drivers/gpu/drm/drm_gem.c > @@ -1010,8 +1010,10 @@ int drm_gem_change_handle_ioctl(struct drm_device *dev, void *data, > if (!obj) > return -ENOENT; > > - if (args->handle == args->new_handle) > - return 0; > + if (args->handle == args->new_handle) { > + ret = 0; > + goto out; > + } > > mutex_lock(&file_priv->prime.lock); > > @@ -1043,6 +1045,8 @@ int drm_gem_change_handle_ioctl(struct drm_device *dev, void *data, > > out_unlock: > mutex_unlock(&file_priv->prime.lock); > +out: > + drm_gem_object_put(obj); > > return ret; > }

1 month, 1 week

Re: [PATCH] drm: Fix object leak in DRM_IOCTL_GEM_CHANGE_HANDLE

by Christian König

On 12/12/25 14:02, Karol Wachowski wrote: > Add missing drm_gem_object_put() call when drm_gem_object_lookup() > successfully returns an object. This fixes a GEM object reference > leak that can prevent driver modules from unloading when using > prime buffers. Good catch. > Fixes: 53096728b891 ("drm: Add DRM prime interface to reassign GEM handle") > Signed-off-by: Karol Wachowski <karol.wachowski(a)linux.intel.com> CC: stable 6.18? > --- > drivers/gpu/drm/drm_gem.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > index ca1956608261..e150bc1ce65a 100644 > --- a/drivers/gpu/drm/drm_gem.c > +++ b/drivers/gpu/drm/drm_gem.c > @@ -1001,7 +1001,7 @@ int drm_gem_change_handle_ioctl(struct drm_device *dev, void *data, > { > struct drm_gem_change_handle *args = data; > struct drm_gem_object *obj; > - int ret; > + int ret = 0; Please set ret explicitly in the if branch below. Always initializing return values is usually considered bad coding style. Apart from that looks good to me. Thanks, Christian. > > if (!drm_core_check_feature(dev, DRIVER_GEM)) > return -EOPNOTSUPP; > @@ -1011,7 +1011,7 @@ int drm_gem_change_handle_ioctl(struct drm_device *dev, void *data, > return -ENOENT; > > if (args->handle == args->new_handle) > - return 0; > + goto out; > > mutex_lock(&file_priv->prime.lock); > > @@ -1043,6 +1043,8 @@ int drm_gem_change_handle_ioctl(struct drm_device *dev, void *data, > > out_unlock: > mutex_unlock(&file_priv->prime.lock); > +out: > + drm_gem_object_put(obj); > > return ret; > }

1 month, 1 week

← Newer
1
...
4
5
6
7
8
9
10
...
330
Older →

Jump to page:

2026

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig