Re: [PATCH 2/7] dma-buf: add dma_fence_chain_alloc/free

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Jun 11, 2021 at 1:48 PM Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
>
> Am 11.06.21 um 09:54 schrieb Daniel Vetter:
> > On Thu, Jun 10, 2021 at 11:17:55AM +0200, Christian König wrote:
> >> Add a common allocation helper. Cleaning up the mix of kzalloc/kmalloc
> >> and some unused code in the selftest.
> >>
> >> Signed-off-by: Christian König <christian.koenig@amd.com>
> >> ---
> >>   drivers/dma-buf/st-dma-fence-chain.c          | 16 ++++----------
> >>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c        |  4 ++--
> >>   drivers/gpu/drm/drm_syncobj.c                 |  6 ++---
> >>   .../gpu/drm/i915/gem/i915_gem_execbuffer.c    |  6 ++---
> >>   drivers/gpu/drm/msm/msm_gem_submit.c          |  6 ++---
> >>   include/linux/dma-fence-chain.h               | 22 +++++++++++++++++++
> >>   6 files changed, 35 insertions(+), 25 deletions(-)
> >>
> >> diff --git a/drivers/dma-buf/st-dma-fence-chain.c b/drivers/dma-buf/st-dma-fence-chain.c
> >> index 9525f7f56119..8ce1ea59d31b 100644
> >> --- a/drivers/dma-buf/st-dma-fence-chain.c
> >> +++ b/drivers/dma-buf/st-dma-fence-chain.c
> >> @@ -58,28 +58,20 @@ static struct dma_fence *mock_fence(void)
> >>      return &f->base;
> >>   }
> >>
> >> -static inline struct mock_chain {
> >> -    struct dma_fence_chain base;
> >> -} *to_mock_chain(struct dma_fence *f) {
> >> -    return container_of(f, struct mock_chain, base.base);
> >> -}
> >> -
> >>   static struct dma_fence *mock_chain(struct dma_fence *prev,
> >>                                  struct dma_fence *fence,
> >>                                  u64 seqno)
> >>   {
> >> -    struct mock_chain *f;
> >> +    struct dma_fence_chain *f;
> >>
> >> -    f = kmalloc(sizeof(*f), GFP_KERNEL);
> >> +    f = dma_fence_chain_alloc();
> >>      if (!f)
> >>              return NULL;
> >>
> >> -    dma_fence_chain_init(&f->base,
> >> -                         dma_fence_get(prev),
> >> -                         dma_fence_get(fence),
> >> +    dma_fence_chain_init(f, dma_fence_get(prev), dma_fence_get(fence),
> >>                           seqno);
> >>
> >> -    return &f->base.base;
> >> +    return &f->base;
> >>   }
> >>
> >>   static int sanitycheck(void *arg)
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> >> index 90136f9dedd6..325e82621467 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> >> @@ -1124,7 +1124,7 @@ static int amdgpu_cs_process_syncobj_timeline_out_dep(struct amdgpu_cs_parser *p
> >>
> >>              dep->chain = NULL;
> >>              if (syncobj_deps[i].point) {
> >> -                    dep->chain = kmalloc(sizeof(*dep->chain), GFP_KERNEL);
> >> +                    dep->chain = dma_fence_chain_alloc();
> >>                      if (!dep->chain)
> >>                              return -ENOMEM;
> >>              }
> >> @@ -1132,7 +1132,7 @@ static int amdgpu_cs_process_syncobj_timeline_out_dep(struct amdgpu_cs_parser *p
> >>              dep->syncobj = drm_syncobj_find(p->filp,
> >>                                              syncobj_deps[i].handle);
> >>              if (!dep->syncobj) {
> >> -                    kfree(dep->chain);
> >> +                    dma_fence_chain_free(dep->chain);
> >>                      return -EINVAL;
> >>              }
> >>              dep->point = syncobj_deps[i].point;
> >> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> >> index fdd2ec87cdd1..1c5b9ef6da37 100644
> >> --- a/drivers/gpu/drm/drm_syncobj.c
> >> +++ b/drivers/gpu/drm/drm_syncobj.c
> >> @@ -861,7 +861,7 @@ static int drm_syncobj_transfer_to_timeline(struct drm_file *file_private,
> >>                                   &fence);
> >>      if (ret)
> >>              goto err;
> >> -    chain = kzalloc(sizeof(struct dma_fence_chain), GFP_KERNEL);
> >> +    chain = dma_fence_chain_alloc();
> >>      if (!chain) {
> >>              ret = -ENOMEM;
> >>              goto err1;
> >> @@ -1402,10 +1402,10 @@ drm_syncobj_timeline_signal_ioctl(struct drm_device *dev, void *data,
> >>              goto err_points;
> >>      }
> >>      for (i = 0; i < args->count_handles; i++) {
> >> -            chains[i] = kzalloc(sizeof(struct dma_fence_chain), GFP_KERNEL);
> >> +            chains[i] = dma_fence_chain_alloc();
> >>              if (!chains[i]) {
> >>                      for (j = 0; j < i; j++)
> >> -                            kfree(chains[j]);
> >> +                            dma_fence_chain_free(chains[j]);
> >>                      ret = -ENOMEM;
> >>                      goto err_chains;
> >>              }
> >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> >> index 66789111a24b..a22cb86730b3 100644
> >> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> >> @@ -2983,7 +2983,7 @@ __free_fence_array(struct eb_fence *fences, unsigned int n)
> >>      while (n--) {
> >>              drm_syncobj_put(ptr_mask_bits(fences[n].syncobj, 2));
> >>              dma_fence_put(fences[n].dma_fence);
> >> -            kfree(fences[n].chain_fence);
> >> +            dma_fence_chain_free(fences[n].chain_fence);
> >>      }
> >>      kvfree(fences);
> >>   }
> >> @@ -3097,9 +3097,7 @@ add_timeline_fence_array(struct i915_execbuffer *eb,
> >>                              return -EINVAL;
> >>                      }
> >>
> >> -                    f->chain_fence =
> >> -                            kmalloc(sizeof(*f->chain_fence),
> >> -                                    GFP_KERNEL);
> >> +                    f->chain_fence = dma_fence_chain_alloc();
> >>                      if (!f->chain_fence) {
> >>                              drm_syncobj_put(syncobj);
> >>                              dma_fence_put(fence);
> >> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
> >> index 5480852bdeda..6ff6df6c4791 100644
> >> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> >> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> >> @@ -586,9 +586,7 @@ static struct msm_submit_post_dep *msm_parse_post_deps(struct drm_device *dev,
> >>                              break;
> >>                      }
> >>
> >> -                    post_deps[i].chain =
> >> -                            kmalloc(sizeof(*post_deps[i].chain),
> >> -                                    GFP_KERNEL);
> >> +                    post_deps[i].chain = dma_fence_chain_alloc();
> >>                      if (!post_deps[i].chain) {
> >>                              ret = -ENOMEM;
> >>                              break;
> >> @@ -605,7 +603,7 @@ static struct msm_submit_post_dep *msm_parse_post_deps(struct drm_device *dev,
> >>
> >>      if (ret) {
> >>              for (j = 0; j <= i; ++j) {
> >> -                    kfree(post_deps[j].chain);
> >> +                    dma_fence_chain_free(post_deps[j].chain);
> >>                      if (post_deps[j].syncobj)
> >>                              drm_syncobj_put(post_deps[j].syncobj);
> >>              }
> >> diff --git a/include/linux/dma-fence-chain.h b/include/linux/dma-fence-chain.h
> >> index 9d6a062be640..5f45689a6d2e 100644
> >> --- a/include/linux/dma-fence-chain.h
> >> +++ b/include/linux/dma-fence-chain.h
> >> @@ -12,6 +12,7 @@
> >>
> >>   #include <linux/dma-fence.h>
> >>   #include <linux/irq_work.h>
> >> +#include <linux/slab.h>
> >>
> >>   /**
> >>    * struct dma_fence_chain - fence to represent an node of a fence chain
> >> @@ -53,6 +54,27 @@ to_dma_fence_chain(struct dma_fence *fence)
> >>      return container_of(fence, struct dma_fence_chain, base);
> >>   }
> >>
> >> +/**
> >> + * dma_fence_chain_alloc
> >> + *
> >> + * Returns a new dma_fence_chain object
> > ... or NULL on failure.
> >
> >> + */
> >> +static inline struct dma_fence_chain *dma_fence_chain_alloc(void)
> >> +{
> >> +    return kmalloc(sizeof(struct dma_fence_chain), GFP_KERNEL);
> >> +};
> >> +
> >> +/**
> >> + * dma_fence_chain_free
> >> + * @chain: chain node to free
> >> + *
> >> + * Frees up an allocated but not used dma_fence_chain node.
> >> + */
> >> +static inline void dma_fence_chain_free(struct dma_fence_chain *chain)
> >> +{
> >> +    kfree(chain);
> > kfree_rcu, and I guess this means this patch here should be cc: stable.
>
> Nope, kfree() is correct here.
>
> This is to free up fences which never been initialized, so an RCU grace
> period isn't necessary because nobody could potentially have a reference.

Hah I got tricked. I see you have the next revision out there already,
I'll drop some suggestions on the kerneldoc so it's clearer what this
does and why.
-Daniel
> Christian.
>
> > This is kinda why I'm questioning whether this "dma_fence are protected by
> > rcu" cross driver api is really a good idea. We largely get it wrong in
> > the details in a _ton_ of places.
> >
> > With the details fixed:
> >
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >
> >> +
> >>   /**
> >>    * dma_fence_chain_for_each - iterate over all fences in chain
> >>    * @iter: current fence
> >> --
> >> 2.25.1
> >>
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch