Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

13 participants
3240 discussions

Re: [RFC v2 04/13] dma-fence: Move array and chain checks to flags

by Christian König

On 5/9/25 17:33, Tvrtko Ursulin wrote: > With the goal of reducing the need for drivers to touch fence->ops, we > add explicit flags for struct dma_fence_array and struct dma_fence_chain > and make the respective helpers (dma_fence_is_array() and > dma_fence_is_chain()) use them. > > This also allows us to remove the exported symbols for the respective > operation tables. That looks like overkill to me. We don't de-reference the ops for the check, instead just the values are compared. Since the array and chain are always build in that should be completely unproblematic for driver unload. Regards, Christian. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin(a)igalia.com> > --- > drivers/dma-buf/dma-fence-array.c | 2 +- > drivers/dma-buf/dma-fence-chain.c | 2 +- > include/linux/dma-fence.h | 9 ++++----- > 3 files changed, 6 insertions(+), 7 deletions(-) > > diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c > index 6657d4b30af9..daf444f5d228 100644 > --- a/drivers/dma-buf/dma-fence-array.c > +++ b/drivers/dma-buf/dma-fence-array.c > @@ -167,7 +167,6 @@ const struct dma_fence_ops dma_fence_array_ops = { > .release = dma_fence_array_release, > .set_deadline = dma_fence_array_set_deadline, > }; > -EXPORT_SYMBOL(dma_fence_array_ops); > > /** > * dma_fence_array_alloc - Allocate a custom fence array > @@ -207,6 +206,7 @@ void dma_fence_array_init(struct dma_fence_array *array, > spin_lock_init(&array->lock); > dma_fence_init(&array->base, &dma_fence_array_ops, &array->lock, > context, seqno); > + __set_bit(DMA_FENCE_FLAG_ARRAY_BIT, &array->base.flags); > init_irq_work(&array->work, irq_dma_fence_array_work); > > atomic_set(&array->num_pending, signal_on_any ? 1 : num_fences); > diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c > index a8a90acf4f34..f4abe41fb092 100644 > --- a/drivers/dma-buf/dma-fence-chain.c > +++ b/drivers/dma-buf/dma-fence-chain.c > @@ -225,7 +225,6 @@ const struct dma_fence_ops dma_fence_chain_ops = { > .release = dma_fence_chain_release, > .set_deadline = dma_fence_chain_set_deadline, > }; > -EXPORT_SYMBOL(dma_fence_chain_ops); > > /** > * dma_fence_chain_init - initialize a fence chain > @@ -263,6 +262,7 @@ void dma_fence_chain_init(struct dma_fence_chain *chain, > > dma_fence_init64(&chain->base, &dma_fence_chain_ops, &chain->lock, > context, seqno); > + __set_bit(DMA_FENCE_FLAG_CHAIN_BIT, &chain->base.flags); > > /* > * Chaining dma_fence_chain container together is only allowed through > diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h > index ac6535716dbe..5bafd0a5f1f1 100644 > --- a/include/linux/dma-fence.h > +++ b/include/linux/dma-fence.h > @@ -98,6 +98,8 @@ struct dma_fence { > > enum dma_fence_flag_bits { > DMA_FENCE_FLAG_SEQNO64_BIT, > + DMA_FENCE_FLAG_ARRAY_BIT, > + DMA_FENCE_FLAG_CHAIN_BIT, > DMA_FENCE_FLAG_SIGNALED_BIT, > DMA_FENCE_FLAG_TIMESTAMP_BIT, > DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, > @@ -632,9 +634,6 @@ struct dma_fence *dma_fence_get_stub(void); > struct dma_fence *dma_fence_allocate_private_stub(ktime_t timestamp); > u64 dma_fence_context_alloc(unsigned num); > > -extern const struct dma_fence_ops dma_fence_array_ops; > -extern const struct dma_fence_ops dma_fence_chain_ops; > - > /** > * dma_fence_is_array - check if a fence is from the array subclass > * @fence: the fence to test > @@ -643,7 +642,7 @@ extern const struct dma_fence_ops dma_fence_chain_ops; > */ > static inline bool dma_fence_is_array(struct dma_fence *fence) > { > - return fence->ops == &dma_fence_array_ops; > + return test_bit(DMA_FENCE_FLAG_ARRAY_BIT, &fence->flags); > } > > /** > @@ -654,7 +653,7 @@ static inline bool dma_fence_is_array(struct dma_fence *fence) > */ > static inline bool dma_fence_is_chain(struct dma_fence *fence) > { > - return fence->ops == &dma_fence_chain_ops; > + return test_bit(DMA_FENCE_FLAG_CHAIN_BIT, &fence->flags); > } > > /**

7 months, 1 week

Re: [RFC v2 03/13] dma-fence: Use a flag for 64-bit seqnos

by Christian König

On 5/9/25 17:33, Tvrtko Ursulin wrote: > With the goal of reducing the need for drivers to touch (and dereference) > fence->ops, we move the 64-bit seqnos flag from struct dma_fence_ops to > the fence->flags. > > Drivers which were setting this flag are changed to use new > dma_fence_init64() instead of dma_fence_init(). > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin(a)igalia.com> My idea was to copy the flag from the ops during init, but that should work as well. Reviewed-by: Christian König <christian.koenig(a)amd.com> > --- > drivers/dma-buf/dma-fence-chain.c | 5 ++--- > drivers/dma-buf/dma-fence.c | 9 +++++++++ > drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c | 5 ++--- > include/linux/dma-fence.h | 6 +++++- > 4 files changed, 18 insertions(+), 7 deletions(-) > > diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c > index 90424f23fd73..a8a90acf4f34 100644 > --- a/drivers/dma-buf/dma-fence-chain.c > +++ b/drivers/dma-buf/dma-fence-chain.c > @@ -218,7 +218,6 @@ static void dma_fence_chain_set_deadline(struct dma_fence *fence, > } > > const struct dma_fence_ops dma_fence_chain_ops = { > - .use_64bit_seqno = true, > .get_driver_name = dma_fence_chain_get_driver_name, > .get_timeline_name = dma_fence_chain_get_timeline_name, > .enable_signaling = dma_fence_chain_enable_signaling, > @@ -262,8 +261,8 @@ void dma_fence_chain_init(struct dma_fence_chain *chain, > seqno = max(prev->seqno, seqno); > } > > - dma_fence_init(&chain->base, &dma_fence_chain_ops, > - &chain->lock, context, seqno); > + dma_fence_init64(&chain->base, &dma_fence_chain_ops, &chain->lock, > + context, seqno); > > /* > * Chaining dma_fence_chain container together is only allowed through > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > index f0cdd3e99d36..33661658f684 100644 > --- a/drivers/dma-buf/dma-fence.c > +++ b/drivers/dma-buf/dma-fence.c > @@ -1023,3 +1023,12 @@ dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, > trace_dma_fence_init(fence); > } > EXPORT_SYMBOL(dma_fence_init); > + > +void > +dma_fence_init64(struct dma_fence *fence, const struct dma_fence_ops *ops, > + spinlock_t *lock, u64 context, u64 seqno) > +{ > + dma_fence_init(fence, ops, lock, context, seqno); > + __set_bit(DMA_FENCE_FLAG_SEQNO64_BIT, &fence->flags); > +} > +EXPORT_SYMBOL(dma_fence_init64); > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c > index 51cddfa3f1e8..5d26797356a3 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c > @@ -71,7 +71,6 @@ static void amdgpu_tlb_fence_work(struct work_struct *work) > } > > static const struct dma_fence_ops amdgpu_tlb_fence_ops = { > - .use_64bit_seqno = true, > .get_driver_name = amdgpu_tlb_fence_get_driver_name, > .get_timeline_name = amdgpu_tlb_fence_get_timeline_name > }; > @@ -101,8 +100,8 @@ void amdgpu_vm_tlb_fence_create(struct amdgpu_device *adev, struct amdgpu_vm *vm > INIT_WORK(&f->work, amdgpu_tlb_fence_work); > spin_lock_init(&f->lock); > > - dma_fence_init(&f->base, &amdgpu_tlb_fence_ops, &f->lock, > - vm->tlb_fence_context, atomic64_read(&vm->tlb_seq)); > + dma_fence_init64(&f->base, &amdgpu_tlb_fence_ops, &f->lock, > + vm->tlb_fence_context, atomic64_read(&vm->tlb_seq)); > > /* TODO: We probably need a separate wq here */ > dma_fence_get(&f->base); > diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h > index 48b5202c531d..ac6535716dbe 100644 > --- a/include/linux/dma-fence.h > +++ b/include/linux/dma-fence.h > @@ -97,6 +97,7 @@ struct dma_fence { > }; > > enum dma_fence_flag_bits { > + DMA_FENCE_FLAG_SEQNO64_BIT, > DMA_FENCE_FLAG_SIGNALED_BIT, > DMA_FENCE_FLAG_TIMESTAMP_BIT, > DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, > @@ -262,6 +263,9 @@ struct dma_fence_ops { > void dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, > spinlock_t *lock, u64 context, u64 seqno); > > +void dma_fence_init64(struct dma_fence *fence, const struct dma_fence_ops *ops, > + spinlock_t *lock, u64 context, u64 seqno); > + > void dma_fence_release(struct kref *kref); > void dma_fence_free(struct dma_fence *fence); > void dma_fence_describe(struct dma_fence *fence, struct seq_file *seq); > @@ -454,7 +458,7 @@ static inline bool __dma_fence_is_later(struct dma_fence *fence, u64 f1, u64 f2) > * 32bit sequence numbers. Use a 64bit compare when the driver says to > * do so. > */ > - if (fence->ops->use_64bit_seqno) > + if (test_bit(DMA_FENCE_FLAG_SEQNO64_BIT, &fence->flags)) > return f1 > f2; > > return (int)(lower_32_bits(f1) - lower_32_bits(f2)) > 0;

7 months, 1 week

Re: [RFC v2 02/13] dma-fence: Change signature of __dma_fence_is_later

by Christian König

On 5/9/25 17:33, Tvrtko Ursulin wrote: > With the goal of reducing the need for drivers to touch (and dereference) > fence->ops, we change the prototype of __dma_fence_is_later() to take > fence instead of fence->ops. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin(a)igalia.com> Reviewed-by: Christian König <christian.koenig(a)amd.com> > --- > drivers/dma-buf/dma-fence-chain.c | 2 +- > drivers/dma-buf/sw_sync.c | 2 +- > drivers/gpu/drm/xe/xe_hw_fence.c | 2 +- > drivers/gpu/drm/xe/xe_sched_job.c | 14 ++++++++------ > include/linux/dma-fence.h | 9 ++++----- > 5 files changed, 15 insertions(+), 14 deletions(-) > > diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c > index 9663ba1bb6ac..90424f23fd73 100644 > --- a/drivers/dma-buf/dma-fence-chain.c > +++ b/drivers/dma-buf/dma-fence-chain.c > @@ -252,7 +252,7 @@ void dma_fence_chain_init(struct dma_fence_chain *chain, > chain->prev_seqno = 0; > > /* Try to reuse the context of the previous chain node. */ > - if (prev_chain && __dma_fence_is_later(seqno, prev->seqno, prev->ops)) { > + if (prev_chain && __dma_fence_is_later(prev, seqno, prev->seqno)) { > context = prev->context; > chain->prev_seqno = prev->seqno; > } else { > diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c > index 4f27ee93a00c..3c20f1d31cf5 100644 > --- a/drivers/dma-buf/sw_sync.c > +++ b/drivers/dma-buf/sw_sync.c > @@ -170,7 +170,7 @@ static bool timeline_fence_signaled(struct dma_fence *fence) > { > struct sync_timeline *parent = dma_fence_parent(fence); > > - return !__dma_fence_is_later(fence->seqno, parent->value, fence->ops); > + return !__dma_fence_is_later(fence, fence->seqno, parent->value); > } > > static void timeline_fence_set_deadline(struct dma_fence *fence, ktime_t deadline) > diff --git a/drivers/gpu/drm/xe/xe_hw_fence.c b/drivers/gpu/drm/xe/xe_hw_fence.c > index 0b4f12be3692..03eb8c6d1616 100644 > --- a/drivers/gpu/drm/xe/xe_hw_fence.c > +++ b/drivers/gpu/drm/xe/xe_hw_fence.c > @@ -165,7 +165,7 @@ static bool xe_hw_fence_signaled(struct dma_fence *dma_fence) > u32 seqno = xe_map_rd(xe, &fence->seqno_map, 0, u32); > > return dma_fence->error || > - !__dma_fence_is_later(dma_fence->seqno, seqno, dma_fence->ops); > + !__dma_fence_is_later(dma_fence, dma_fence->seqno, seqno); > } > > static bool xe_hw_fence_enable_signaling(struct dma_fence *dma_fence) > diff --git a/drivers/gpu/drm/xe/xe_sched_job.c b/drivers/gpu/drm/xe/xe_sched_job.c > index 1905ca590965..f0a6ce610948 100644 > --- a/drivers/gpu/drm/xe/xe_sched_job.c > +++ b/drivers/gpu/drm/xe/xe_sched_job.c > @@ -216,15 +216,17 @@ void xe_sched_job_set_error(struct xe_sched_job *job, int error) > > bool xe_sched_job_started(struct xe_sched_job *job) > { > + struct dma_fence *fence = dma_fence_chain_contained(job->fence); > struct xe_lrc *lrc = job->q->lrc[0]; > > - return !__dma_fence_is_later(xe_sched_job_lrc_seqno(job), > - xe_lrc_start_seqno(lrc), > - dma_fence_chain_contained(job->fence)->ops); > + return !__dma_fence_is_later(fence, > + xe_sched_job_lrc_seqno(job), > + xe_lrc_start_seqno(lrc)); > } > > bool xe_sched_job_completed(struct xe_sched_job *job) > { > + struct dma_fence *fence = dma_fence_chain_contained(job->fence); > struct xe_lrc *lrc = job->q->lrc[0]; > > /* > @@ -232,9 +234,9 @@ bool xe_sched_job_completed(struct xe_sched_job *job) > * parallel handshake is done. > */ > > - return !__dma_fence_is_later(xe_sched_job_lrc_seqno(job), > - xe_lrc_seqno(lrc), > - dma_fence_chain_contained(job->fence)->ops); > + return !__dma_fence_is_later(fence, > + xe_sched_job_lrc_seqno(job), > + xe_lrc_seqno(lrc)); > } > > void xe_sched_job_arm(struct xe_sched_job *job) > diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h > index b12776883d14..48b5202c531d 100644 > --- a/include/linux/dma-fence.h > +++ b/include/linux/dma-fence.h > @@ -441,21 +441,20 @@ dma_fence_is_signaled(struct dma_fence *fence) > > /** > * __dma_fence_is_later - return if f1 is chronologically later than f2 > + * @fence: fence in whose context to do the comparison > * @f1: the first fence's seqno > * @f2: the second fence's seqno from the same context > - * @ops: dma_fence_ops associated with the seqno > * > * Returns true if f1 is chronologically later than f2. Both fences must be > * from the same context, since a seqno is not common across contexts. > */ > -static inline bool __dma_fence_is_later(u64 f1, u64 f2, > - const struct dma_fence_ops *ops) > +static inline bool __dma_fence_is_later(struct dma_fence *fence, u64 f1, u64 f2) > { > /* This is for backward compatibility with drivers which can only handle > * 32bit sequence numbers. Use a 64bit compare when the driver says to > * do so. > */ > - if (ops->use_64bit_seqno) > + if (fence->ops->use_64bit_seqno) > return f1 > f2; > > return (int)(lower_32_bits(f1) - lower_32_bits(f2)) > 0; > @@ -475,7 +474,7 @@ static inline bool dma_fence_is_later(struct dma_fence *f1, > if (WARN_ON(f1->context != f2->context)) > return false; > > - return __dma_fence_is_later(f1->seqno, f2->seqno, f1->ops); > + return __dma_fence_is_later(f1, f1->seqno, f2->seqno); > } > > /**

7 months, 1 week

Re: [RFC v2 01/13] drm/i915: Use provided dma_fence_is_chain

by Christian König

On 5/9/25 17:47, Matthew Brost wrote: > On Fri, May 09, 2025 at 04:33:40PM +0100, Tvrtko Ursulin wrote: >> Replace open-coded helper with the subsystem one. >> > > You probably can just send this one by itself as it good cleanup and > independent. > > Reviewed-by: Matthew Brost <matthew.brost(a)intel.com> Reviewed-by: Christian König <christian.koenig(a)amd.com> Any objections that I start to push those patches to drm-misc-next or do you want to take this one through the i915 branch? Regards, Christian. > >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin(a)igalia.com> >> --- >> drivers/gpu/drm/i915/gem/i915_gem_wait.c | 7 +------ >> 1 file changed, 1 insertion(+), 6 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c b/drivers/gpu/drm/i915/gem/i915_gem_wait.c >> index 7127e90c1a8f..991666fd9f85 100644 >> --- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c >> @@ -106,11 +106,6 @@ static void fence_set_priority(struct dma_fence *fence, >> rcu_read_unlock(); >> } >> >> -static inline bool __dma_fence_is_chain(const struct dma_fence *fence) >> -{ >> - return fence->ops == &dma_fence_chain_ops; >> -} >> - >> void i915_gem_fence_wait_priority(struct dma_fence *fence, >> const struct i915_sched_attr *attr) >> { >> @@ -126,7 +121,7 @@ void i915_gem_fence_wait_priority(struct dma_fence *fence, >> >> for (i = 0; i < array->num_fences; i++) >> fence_set_priority(array->fences[i], attr); >> - } else if (__dma_fence_is_chain(fence)) { >> + } else if (dma_fence_is_chain(fence)) { >> struct dma_fence *iter; >> >> /* The chain is ordered; if we boost the last, we boost all */ >> -- >> 2.48.0 >>

7 months, 1 week

Re: [PATCH bpf-next v4 5/5] selftests/bpf: Add test for open coded dmabuf_iter

by T.J. Mercier

On Fri, May 9, 2025 at 2:58 PM Song Liu <song(a)kernel.org> wrote: > > On Fri, May 9, 2025 at 2:43 PM T.J. Mercier <tjmercier(a)google.com> wrote: > > > [...] > > > > > > Personally, I would prefer we just merge all the logic of > > > create_udmabuf() and create_sys_heap_dmabuf() > > > into create_test_buffers(). > > > > That's a lot of different stuff to put in one place. How about > > returning file descriptors from the buffer create functions while > > having them clean up after themselves: > > I do like this version better. Some nitpicks though. > > > > > -static int memfd, udmabuf; > > +static int udmabuf; > > About this, and ... > > > static const char udmabuf_test_buffer_name[DMA_BUF_NAME_LEN] = > > "udmabuf_test_buffer_for_iter"; > > static size_t udmabuf_test_buffer_size; > > static int sysheap_dmabuf; > > static const char sysheap_test_buffer_name[DMA_BUF_NAME_LEN] = > > "sysheap_test_buffer_for_iter"; > > static size_t sysheap_test_buffer_size; > > > > -static int create_udmabuf(int map_fd) > > +static int create_udmabuf(void) > > { > > struct udmabuf_create create; > > - int dev_udmabuf; > > - bool f = false; > > + int dev_udmabuf, memfd, udmabuf; > .. here. > > It is not ideal to have a global udmabuf and a local udmabuf. > If we want the global version, let's rename the local one. Ok let me change up the name of the aliasing variable to local_udmabuf. > [...] > > > > > static int create_test_buffers(int map_fd) > > { > > - int ret; > > + bool f = false; > > + > > + udmabuf = create_udmabuf(); > > + sysheap_dmabuf = create_sys_heap_dmabuf(); > > > > - ret = create_udmabuf(map_fd); > > - if (ret) > > - return ret; > > + if (udmabuf < 0 || sysheap_dmabuf < 0) > > + return -1; > > We also need destroy_test_buffers() on the error path here, > or at the caller. The caller does currently check to decide if it should bother running the tests or not, and calls destroy_test_buffers() if not. > > - return create_sys_heap_dmabuf(map_fd); > > + return bpf_map_update_elem(map_fd, udmabuf_test_buffer_name, > > &f, BPF_ANY) || > > + bpf_map_update_elem(map_fd, sysheap_test_buffer_name, > > &f, BPF_ANY); > > } > > > > static void destroy_test_buffers(void) > > { > > close(udmabuf); > > - close(memfd); > > close(sysheap_dmabuf); > > For the two global fds, let's reset them to -1 right after close(). > > Thanks, > Song Will do, thanks.

7 months, 1 week

Re: [PATCH bpf-next v4 5/5] selftests/bpf: Add test for open coded dmabuf_iter

by T.J. Mercier

On Fri, May 9, 2025 at 11:46 AM Song Liu <song(a)kernel.org> wrote: > > On Thu, May 8, 2025 at 11:21 AM T.J. Mercier <tjmercier(a)google.com> wrote: > > > > Use the same test buffers as the traditional iterator and a new BPF map > > to verify the test buffers can be found with the open coded dmabuf > > iterator. > > The way we split 4/5 and 5/5 makes the code tricker to follow. I guess > the motivation is to back port default iter along to older kernels. But I > think we can still make the code cleaner. > > > > > Signed-off-by: T.J. Mercier <tjmercier(a)google.com> > > --- > [...] > > > > > -static int create_udmabuf(void) > > +static int create_udmabuf(int map_fd) > > { > > struct udmabuf_create create; > > int dev_udmabuf; > > + bool f = false; > > > > udmabuf_test_buffer_size = 10 * getpagesize(); > > > > @@ -63,10 +64,10 @@ static int create_udmabuf(void) > > if (!ASSERT_OK(ioctl(udmabuf, DMA_BUF_SET_NAME_B, udmabuf_test_buffer_name), "name")) > > return 1; > > > > - return 0; > > + return bpf_map_update_elem(map_fd, udmabuf_test_buffer_name, &f, BPF_ANY); > > We don't really need this bpf_map_update_elem() inside > create_udmabuf(), right? > > > } > > > > -static int create_sys_heap_dmabuf(void) > > +static int create_sys_heap_dmabuf(int map_fd) > > { > > sysheap_test_buffer_size = 20 * getpagesize(); > > > > @@ -77,6 +78,7 @@ static int create_sys_heap_dmabuf(void) > > .heap_flags = 0, > > }; > > int heap_fd, ret; > > + bool f = false; > > > > if (!ASSERT_LE(sizeof(sysheap_test_buffer_name), DMA_BUF_NAME_LEN, "NAMETOOLONG")) > > return 1; > > @@ -95,18 +97,18 @@ static int create_sys_heap_dmabuf(void) > > if (!ASSERT_OK(ioctl(sysheap_dmabuf, DMA_BUF_SET_NAME_B, sysheap_test_buffer_name), "name")) > > return 1; > > > > - return 0; > > + return bpf_map_update_elem(map_fd, sysheap_test_buffer_name, &f, BPF_ANY); > > Same for this bpf_map_update_elem(), we can call this directly from > create_test_buffers(). > > > } > > > > -static int create_test_buffers(void) > > +static int create_test_buffers(int map_fd) > > { > > int ret; > > > > - ret = create_udmabuf(); > > + ret = create_udmabuf(map_fd); > > if (ret) > > return ret; > > > > - return create_sys_heap_dmabuf(); > > + return create_sys_heap_dmabuf(map_fd); > > Personally, I would prefer we just merge all the logic of > create_udmabuf() and create_sys_heap_dmabuf() > into create_test_buffers(). That's a lot of different stuff to put in one place. How about returning file descriptors from the buffer create functions while having them clean up after themselves: -static int memfd, udmabuf; +static int udmabuf; static const char udmabuf_test_buffer_name[DMA_BUF_NAME_LEN] = "udmabuf_test_buffer_for_iter"; static size_t udmabuf_test_buffer_size; static int sysheap_dmabuf; static const char sysheap_test_buffer_name[DMA_BUF_NAME_LEN] = "sysheap_test_buffer_for_iter"; static size_t sysheap_test_buffer_size; -static int create_udmabuf(int map_fd) +static int create_udmabuf(void) { struct udmabuf_create create; - int dev_udmabuf; - bool f = false; + int dev_udmabuf, memfd, udmabuf; udmabuf_test_buffer_size = 10 * getpagesize(); if (!ASSERT_LE(sizeof(udmabuf_test_buffer_name), DMA_BUF_NAME_LEN, "NAMETOOLONG")) - return 1; + return -1; memfd = memfd_create("memfd_test", MFD_ALLOW_SEALING); if (!ASSERT_OK_FD(memfd, "memfd_create")) - return 1; + return -1; if (!ASSERT_OK(ftruncate(memfd, udmabuf_test_buffer_size), "ftruncate")) - return 1; + goto close_memfd; if (!ASSERT_OK(fcntl(memfd, F_ADD_SEALS, F_SEAL_SHRINK), "seal")) - return 1; + goto close_memfd; dev_udmabuf = open("/dev/udmabuf", O_RDONLY); if (!ASSERT_OK_FD(dev_udmabuf, "open udmabuf")) - return 1; + goto close_memfd; create.memfd = memfd; create.flags = UDMABUF_FLAGS_CLOEXEC; @@ -59,15 +58,21 @@ static int create_udmabuf(int map_fd) udmabuf = ioctl(dev_udmabuf, UDMABUF_CREATE, &create); close(dev_udmabuf); if (!ASSERT_OK_FD(udmabuf, "udmabuf_create")) - return 1; + goto close_memfd; if (!ASSERT_OK(ioctl(udmabuf, DMA_BUF_SET_NAME_B, udmabuf_test_buffer_name), "name")) - return 1; + goto close_udmabuf; + + return udmabuf; - return bpf_map_update_elem(map_fd, udmabuf_test_buffer_name, &f, BPF_ANY); +close_udmabuf: + close(udmabuf); +close_memfd: + close(memfd); + return -1; } -static int create_sys_heap_dmabuf(int map_fd) +static int create_sys_heap_dmabuf(void) { sysheap_test_buffer_size = 20 * getpagesize(); @@ -78,43 +83,46 @@ static int create_sys_heap_dmabuf(int map_fd) .heap_flags = 0, }; int heap_fd, ret; - bool f = false; if (!ASSERT_LE(sizeof(sysheap_test_buffer_name), DMA_BUF_NAME_LEN, "NAMETOOLONG")) - return 1; + return -1; heap_fd = open("/dev/dma_heap/system", O_RDONLY); if (!ASSERT_OK_FD(heap_fd, "open dma heap")) - return 1; + return -1; ret = ioctl(heap_fd, DMA_HEAP_IOCTL_ALLOC, &data); close(heap_fd); if (!ASSERT_OK(ret, "syheap alloc")) - return 1; + return -1; - sysheap_dmabuf = data.fd; + if (!ASSERT_OK(ioctl(data.fd, DMA_BUF_SET_NAME_B, sysheap_test_buffer_name), "name")) + goto close_sysheap_dmabuf; - if (!ASSERT_OK(ioctl(sysheap_dmabuf, DMA_BUF_SET_NAME_B, sysheap_test_buffer_name), "name")) - return 1; + return data.fd; - return bpf_map_update_elem(map_fd, sysheap_test_buffer_name, &f, BPF_ANY); +close_sysheap_dmabuf: + close(data.fd); + return -1; } static int create_test_buffers(int map_fd) { - int ret; + bool f = false; + + udmabuf = create_udmabuf(); + sysheap_dmabuf = create_sys_heap_dmabuf(); - ret = create_udmabuf(map_fd); - if (ret) - return ret; + if (udmabuf < 0 || sysheap_dmabuf < 0) + return -1; - return create_sys_heap_dmabuf(map_fd); + return bpf_map_update_elem(map_fd, udmabuf_test_buffer_name, &f, BPF_ANY) || + bpf_map_update_elem(map_fd, sysheap_test_buffer_name, &f, BPF_ANY); } static void destroy_test_buffers(void) { close(udmabuf); - close(memfd); close(sysheap_dmabuf); }

7 months, 1 week

Re: [RFC PATCH 00/12] Private MMIO support for private assigned dev

by Jason Gunthorpe

On Sat, May 10, 2025 at 12:28:48AM +0800, Xu Yilun wrote: > On Fri, May 09, 2025 at 07:12:46PM +0800, Xu Yilun wrote: > > On Fri, May 09, 2025 at 01:04:58PM +1000, Alexey Kardashevskiy wrote: > > > Ping? > > > > Sorry for late reply from vacation. > > > > > Also, since there is pushback on 01/12 "dma-buf: Introduce dma_buf_get_pfn_unlocked() kAPI", what is the plan now? Thanks, > > > > As disscussed in the thread, this kAPI is not well considered but IIUC > > the concept of "importer mapping" is still valid. We need more > > investigation about all the needs - P2P, CC memory, private bus > > channel, and work out a formal API. > > > > However in last few months I'm focusing on high level TIO flow - TSM > > framework, IOMMUFD based bind/unbind, so no much progress here and is > > still using this temporary kAPI. But as long as "importer mapping" is > > alive, the dmabuf fd for KVM is still valid and we could enable TIO > > based on that. > > Oh I forgot to mention I moved the dmabuf creation from VFIO to IOMMUFD > recently, the IOCTL is against iommufd_device. I'm surprised by this.. iommufd shouldn't be doing PCI stuff, it is just about managing the translation control of the device. > According to Jason's > opinion [1], TSM bind/unbind should be called against iommufd_device, > then I need to do the same for dmabuf. This is because Intel TDX > Connect enforces a specific operation sequence between TSM unbind & MMIO > unmap: > > 1. STOP TDI via TDISP message STOP_INTERFACE > 2. Private MMIO unmap from Secure EPT > 3. Trusted Device Context Table cleanup for the TDI > 4. TDI ownership reclaim and metadata free So your issue is you need to shoot down the dmabuf during vPCI device destruction? VFIO also needs to shoot down the MMIO during things like FLR I don't think moving to iommufd really fixes it, it sounds like you need more coordination between the two parts?? Jason

7 months, 1 week

Re: [PATCH bpf-next v4 4/5] selftests/bpf: Add test for dmabuf_iter

by T.J. Mercier

On Thu, May 8, 2025 at 5:36 PM Song Liu <song(a)kernel.org> wrote: > > On Thu, May 8, 2025 at 11:20 AM T.J. Mercier <tjmercier(a)google.com> wrote: > [...] > > diff --git a/tools/testing/selftests/bpf/prog_tests/dmabuf_iter.c b/tools/testing/selftests/bpf/prog_tests/dmabuf_iter.c > > new file mode 100644 > > index 000000000000..35745f4ce0f8 > > --- /dev/null > > +++ b/tools/testing/selftests/bpf/prog_tests/dmabuf_iter.c > > @@ -0,0 +1,224 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* Copyright (c) 2025 Google */ > > + > > +#include <test_progs.h> > > +#include <bpf/libbpf.h> > > +#include <bpf/btf.h> > > +#include "dmabuf_iter.skel.h" > > + > > +#include <fcntl.h> > > +#include <stdbool.h> > > +#include <stdio.h> > > +#include <stdlib.h> > > +#include <string.h> > > +#include <sys/ioctl.h> > > +#include <sys/mman.h> > > +#include <unistd.h> > > + > > +#include <linux/dma-buf.h> > > +#include <linux/dma-heap.h> > > +#include <linux/udmabuf.h> > > + > > +static int memfd, udmabuf; > > Global fds are weird. AFAICT, we don't really need them > to be global? If we really need them to be global, please > initialize them to -1, just in case we close(0) by accident. Hmm, no we don't really need them to be global but I didn't really want to pass all these variables around to all the setup and test functions. The fd lifetimes are nearly the whole program lifetime anyways, and just need to exist without actually being used for anything. I'll add the -1 initialization as you suggest. If udmabuf creation failed, we would have done a close(0) in destroy_test_buffers() on the sysheap_dmabuf fd. > > +static const char udmabuf_test_buffer_name[DMA_BUF_NAME_LEN] = "udmabuf_test_buffer_for_iter"; > > +static size_t udmabuf_test_buffer_size; > > +static int sysheap_dmabuf; > > +static const char sysheap_test_buffer_name[DMA_BUF_NAME_LEN] = "sysheap_test_buffer_for_iter"; > > +static size_t sysheap_test_buffer_size;

7 months, 1 week

Re: [PATCH bpf-next v4 3/5] bpf: Add open coded dmabuf iterator

by T.J. Mercier

On Thu, May 8, 2025 at 5:28 PM Song Liu <song(a)kernel.org> wrote: > > On Thu, May 8, 2025 at 11:20 AM T.J. Mercier <tjmercier(a)google.com> wrote: > > > > This open coded iterator allows for more flexibility when creating BPF > > programs. It can support output in formats other than text. With an open > > coded iterator, a single BPF program can traverse multiple kernel data > > structures (now including dmabufs), allowing for more efficient analysis > > of kernel data compared to multiple reads from procfs, sysfs, or > > multiple traditional BPF iterator invocations. > > > > Signed-off-by: T.J. Mercier <tjmercier(a)google.com> > > Acked-by: Song Liu <song(a)kernel.org> > > With one nitpick below: > > > --- > > kernel/bpf/dmabuf_iter.c | 47 ++++++++++++++++++++++++++++++++++++++++ > > kernel/bpf/helpers.c | 5 +++++ > > 2 files changed, 52 insertions(+) > > > > diff --git a/kernel/bpf/dmabuf_iter.c b/kernel/bpf/dmabuf_iter.c > > index 96b4ba7f0b2c..8049bdbc9efc 100644 > > --- a/kernel/bpf/dmabuf_iter.c > > +++ b/kernel/bpf/dmabuf_iter.c > > @@ -100,3 +100,50 @@ static int __init dmabuf_iter_init(void) > > } > > > > late_initcall(dmabuf_iter_init); > > + > > +struct bpf_iter_dmabuf { > > + /* opaque iterator state; having __u64 here allows to preserve correct > > + * alignment requirements in vmlinux.h, generated from BTF > > + */ > > nit: comment style. Added a leading /* (This is copied from task_iter.c, which currently has the same style.) > > + __u64 __opaque[1]; > > +} __aligned(8); > > + > > +/* Non-opaque version of bpf_iter_dmabuf */ > > +struct bpf_iter_dmabuf_kern { > > + struct dma_buf *dmabuf; > > +} __aligned(8); > > + > [...]

7 months, 1 week

Re: [PATCH bpf-next v4 2/5] bpf: Add dmabuf iterator

by T.J. Mercier

On Thu, May 8, 2025 at 5:27 PM Song Liu <song(a)kernel.org> wrote: > > On Thu, May 8, 2025 at 11:20 AM T.J. Mercier <tjmercier(a)google.com> wrote: > > > > The dmabuf iterator traverses the list of all DMA buffers. > > > > DMA buffers are refcounted through their associated struct file. A > > reference is taken on each buffer as the list is iterated to ensure each > > buffer persists for the duration of the bpf program execution without > > holding the list mutex. > > > > Signed-off-by: T.J. Mercier <tjmercier(a)google.com> > > Reviewed-by: Christian König <christian.koenig(a)amd.com> > > Acked-by: Song Liu <song(a)kernel.org> > > With one nitpick below. Thanks! > > --- > [...] > > diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h > > index 8ff4add71f88..7af2ea839f58 100644 > > --- a/include/linux/dma-buf.h > > +++ b/include/linux/dma-buf.h > > @@ -634,4 +634,6 @@ int dma_buf_vmap(struct dma_buf *dmabuf, struct iosys_map *map); > > void dma_buf_vunmap(struct dma_buf *dmabuf, struct iosys_map *map); > > int dma_buf_vmap_unlocked(struct dma_buf *dmabuf, struct iosys_map *map); > > void dma_buf_vunmap_unlocked(struct dma_buf *dmabuf, struct iosys_map *map); > > +struct dma_buf *dma_buf_iter_begin(void); > > +struct dma_buf *dma_buf_iter_next(struct dma_buf *dmbuf); > > #endif /* __DMA_BUF_H__ */ > > diff --git a/kernel/bpf/Makefile b/kernel/bpf/Makefile > > index 70502f038b92..3a335c50e6e3 100644 > > --- a/kernel/bpf/Makefile > > +++ b/kernel/bpf/Makefile > > @@ -53,6 +53,9 @@ obj-$(CONFIG_BPF_SYSCALL) += relo_core.o > > obj-$(CONFIG_BPF_SYSCALL) += btf_iter.o > > obj-$(CONFIG_BPF_SYSCALL) += btf_relocate.o > > obj-$(CONFIG_BPF_SYSCALL) += kmem_cache_iter.o > > +ifeq ($(CONFIG_DMA_SHARED_BUFFER),y) > > +obj-$(CONFIG_BPF_SYSCALL) += dmabuf_iter.o > > +endif > > > > CFLAGS_REMOVE_percpu_freelist.o = $(CC_FLAGS_FTRACE) > > CFLAGS_REMOVE_bpf_lru_list.o = $(CC_FLAGS_FTRACE) > > diff --git a/kernel/bpf/dmabuf_iter.c b/kernel/bpf/dmabuf_iter.c > > new file mode 100644 > > index 000000000000..96b4ba7f0b2c > > --- /dev/null > > +++ b/kernel/bpf/dmabuf_iter.c > > @@ -0,0 +1,102 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* Copyright (c) 2025 Google LLC */ > > +#include <linux/bpf.h> > > +#include <linux/btf_ids.h> > > +#include <linux/dma-buf.h> > > +#include <linux/kernel.h> > > +#include <linux/seq_file.h> > > + > > +BTF_ID_LIST_SINGLE(bpf_dmabuf_btf_id, struct, dma_buf) > > +DEFINE_BPF_ITER_FUNC(dmabuf, struct bpf_iter_meta *meta, struct dma_buf *dmabuf) > > nit: It is better to move these two lines later, to where they > are about to be used. I've moved them both to just before dmabuf_iter_init() farther down.

7 months, 1 week

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig