[PATCH 2/5] drm/xe: Add ref counting for xe_file
Lucas De Marchi
lucas.demarchi at intel.com
Thu Jul 11 13:46:23 UTC 2024
On Tue, Jul 09, 2024 at 08:37:10PM GMT, Matthew Brost wrote:
>On Tue, Jul 09, 2024 at 09:37:54AM -0700, Umesh Nerlige Ramappa wrote:
>> On Tue, Jul 09, 2024 at 10:11:47AM -0500, Lucas De Marchi wrote:
>> > On Tue, Jul 09, 2024 at 12:28:43AM GMT, Matthew Brost wrote:
>> > > On Mon, Jul 08, 2024 at 04:26:00PM -0700, Umesh Nerlige Ramappa wrote:
>> > > > On Mon, Jul 08, 2024 at 05:52:00PM -0500, Lucas De Marchi wrote:
>> > > > > On Mon, Jul 08, 2024 at 01:21:00PM GMT, Umesh Nerlige Ramappa wrote:
>> > > > > > Add ref counting for xe_file.
>> > > > > >
>> > > > > > Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa at intel.com>
>> > > > > > ---
>> > > > > > drivers/gpu/drm/xe/xe_device.c | 7 +++++--
>> > > > > > drivers/gpu/drm/xe/xe_device.h | 12 ++++++++++++
>> > > > > > drivers/gpu/drm/xe/xe_device_types.h | 3 +++
>> > > > > > 3 files changed, 20 insertions(+), 2 deletions(-)
>> > > > > >
>> > > > > > diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
>> > > > > > index babb697652d5..e6eacf1bcce0 100644
>> > > > > > --- a/drivers/gpu/drm/xe/xe_device.c
>> > > > > > +++ b/drivers/gpu/drm/xe/xe_device.c
>> > > > > > @@ -87,11 +87,14 @@ static int xe_file_open(struct drm_device *dev, struct drm_file *file)
>> > > > > > spin_unlock(&xe->clients.lock);
>> > > > > >
>> > > > > > file->driver_priv = xef;
>> > > > > > + kref_init(&xef->refcount);
>> > > > > > +
>> > > > > > return 0;
>> > > > > > }
>> > > > > >
>> > > > > > -static void xe_file_destroy(struct xe_file *xef)
>> > > > > > +void xe_file_destroy(struct kref *ref)
>> > > > >
>> > > > > why do you export this? I don't think anybody should be calling
>> > > > > xe_file_destroy() directly and this function being executed should only
>> > > > > be the outcome of kref being 0. Also, if it's exported, it shouldn't
>> > > > > really have struct kref as argument, but rather a struct xe_file
>> > > > >
>> > > > >
>> > > > > > {
>> > > > > > + struct xe_file *xef = container_of(ref, struct xe_file, refcount);
>> > > > > > struct xe_device *xe = xef->xe;
>> > > > > > struct xe_vm *vm;
>> > > > > > unsigned long idx;
>> > > > > > @@ -130,7 +133,7 @@ static void xe_file_close(struct drm_device *dev, struct drm_file *file)
>> > > > > > xe_exec_queue_put(q);
>> > > > > > }
>> > > > > >
>> > > > > > - xe_file_destroy(xef);
>> > > > > > + xe_file_put(xef);
>> > > > > > }
>> > > > > >
>> > > > > > static const struct drm_ioctl_desc xe_ioctls[] = {
>> > > > > > diff --git a/drivers/gpu/drm/xe/xe_device.h b/drivers/gpu/drm/xe/xe_device.h
>> > > > > > index bb07f5669dbb..2a8b370b1fda 100644
>> > > > > > --- a/drivers/gpu/drm/xe/xe_device.h
>> > > > > > +++ b/drivers/gpu/drm/xe/xe_device.h
>> > > > > > @@ -169,5 +169,17 @@ static inline bool xe_device_wedged(struct xe_device *xe)
>> > > > > > }
>> > > > > >
>> > > > > > void xe_device_declare_wedged(struct xe_device *xe);
>> > > > > > +void xe_file_destroy(struct kref *ref);
>> > > > > > +
>> > > > > > +static inline struct xe_file *xe_file_get(struct xe_file *xef)
>> > > > > > +{
>> > > > > > + kref_get(&xef->refcount);
>> > > > > > + return xef;
>> > > > > > +}
>> > > > > > +
>> > > > > > +static inline void xe_file_put(struct xe_file *xef)
>> > > > >
>> > > > > I think if you just make this non-inline, passing file_destroy and
>> > > > > leave it in the same place, it should work.
>> > > >
>> > > > the put is called from xe_exec_queue.c and xe_vm.c also, so passing the
>> > > > destroy may not work.
>> > > >
>> > > > Based on the patterns below, xe_file_put/xe_file_get will need to be moved
>> > > > to xe_device.c and then exported. It looks cleaner, hoping that'w what you
>> > > > mean.
>> > > >
>> > > > In xe_device.h:
>> > > >
>> > > > void xe_file_put(struct xe_file *xef);
>> > > > struct xe_file *xe_file_get(struct xe_file *xef);
>> >
>> > yes
>>
>> ok, posted v2 with this change.
>>
>> >
>> > > >
>> > >
>> > > My opinion is the way you have it is correct.
>> > >
>> > > Most of Xe is coded this way...
>> > >
>> > > mbrost at lstrano-desk:xe$ grep _get\( *.h | grep inline
>> > > xe_bo.h:static inline struct xe_bo *xe_bo_get(struct xe_bo *bo)
>> > > xe_device.h:static inline struct xe_file *xe_file_get(struct xe_file *xef)
>> > > xe_exec_queue.h:static inline struct xe_exec_queue *xe_exec_queue_get(struct xe_exec_queue *q)
>> > > xe_lrc.h:static inline struct xe_lrc *xe_lrc_get(struct xe_lrc *lrc)
>> > > xe_sched_job.h:static inline struct xe_sched_job *xe_sched_job_get(struct xe_sched_job *job)
>> > > xe_vm.h:static inline struct xe_vm *xe_vm_get(struct xe_vm *vm)
>> > >
>> > > I'm kinda confused by what Lucas is suggesting.
>> >
>> > just don't make xe_file_destroy() non-static, which would basically
>> > ignore the refcounting. xe_file_destroy() should rather be local to the
>> > .c implementing xe_file_put().
>> >
>>
>> In the above instances from grep, most of the destroy helpers are indeed
>> exported. Those might need to be changed as per this review comment.
>>
>
>This really just bikeshedding, I prefer static inlines so these being
>exported to make get/put as fast as possible. Perhaps this doesn't even
>matter with how compilers work these days (e.g. even if exported
>functions they get inlined by the compiler?). I don't care enough so
>push back though.
my issue is having a xe_file_destroy() with no indication whatsoever
that it should never be called... the only valid caller is by giving
it as callback to kref_put().
Some parts of the kernel uses a __ to make this clear (we follow this
approach with __xe_bo_release_dummy, __xe_drm_client_free) or don't
expose the release/destroy.
Another option to make it even clearer would be, e.g.:
// diff --git a/drivers/gpu/drm/xe/xe_exec_queue.h b/drivers/gpu/drm/xe/xe_exec_queue.h
// index ded77b0f3b90..e7111e2aa7ba 100644
// --- a/drivers/gpu/drm/xe/xe_exec_queue.h
// +++ b/drivers/gpu/drm/xe/xe_exec_queue.h
// @@ -23,7 +23,6 @@ struct xe_exec_queue *xe_exec_queue_create_class(struct xe_device *xe, struct xe
// enum xe_engine_class class, u32 flags);
//
// void xe_exec_queue_fini(struct xe_exec_queue *q);
// -void xe_exec_queue_destroy(struct kref *ref);
// void xe_exec_queue_assign_name(struct xe_exec_queue *q, u32 instance);
//
// static inline struct xe_exec_queue *
// @@ -43,6 +42,7 @@ static inline struct xe_exec_queue *xe_exec_queue_get(struct xe_exec_queue *q)
// return q;
// }
//
// +extern void __xe_exec_queue_destroy(struct kref *ref);
// static inline void xe_exec_queue_put(struct xe_exec_queue *q)
// {
// kref_put(&q->refcount, __xe_exec_queue_destroy);
... And yet another option is to add a warning in the destroy() function
if kref is not 0.
>
>I do agree we should be uniform in this though. Maybe the maintainers
>set a policy and we follow that everywhere in Xe?
Cc'ing them... but I'm not sure how uniform we can get.
Lucas De Marhci
>
>Matt
>
>
>> Thanks,
>> Umesh
>> >
>> > Lucas De Marchi
More information about the Intel-xe
mailing list