Re: [Intel-gfx] [PATCH 7/9] drm/i915/gt: Fix memory leaks in per-gt sysfs

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Wed, 20 Apr 2022 05:17:57 -0700, Andrzej Hajda wrote:
>
> Hi Ashutosh,

Hi Andrzej,

> On 20.04.2022 07:21, Ashutosh Dixit wrote:
> > All kmalloc'd kobjects need a kobject_put() to free memory. For example in
> > previous code, kobj_gt_release() never gets called. The requirement of
> > kobject_put() now results in a slightly different code organization.
> >
> > Cc: Andi Shyti <andi.shyti@intel.com>
> > Cc: Andrzej Hajda <andrzej.hajda@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Fixes: b770bcfae9ad ("drm/i915/gt: create per-tile sysfs interface")

/snip/

> > +void intel_gt_sysfs_unregister(struct intel_gt *gt)
> > +{
> > +	kobject_put(&gt->sysfs_gtn);
> > +}
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h
> > index 9471b26752cf..a99aa7e8b01a 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h
> > @@ -13,11 +13,6 @@
> >     struct intel_gt;
> >   -struct kobj_gt {
> > -	struct kobject base;
> > -	struct intel_gt *gt;
> > -};
> > -
> >   bool is_object_gt(struct kobject *kobj);
> >     struct drm_i915_private *kobj_to_i915(struct kobject *kobj);
> > @@ -28,6 +23,7 @@ intel_gt_create_kobj(struct intel_gt *gt,
> >		     const char *name);
> >     void intel_gt_sysfs_register(struct intel_gt *gt);
> > +void intel_gt_sysfs_unregister(struct intel_gt *gt);
> >   struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev,
> >					    const char *name);
> >   diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > index 937b2e1a305e..4c72b4f983a6 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > @@ -222,6 +222,9 @@ struct intel_gt {
> >	} mocs;
> >		struct intel_pxp pxp;
> > +
> > +	/* gt/gtN sysfs */
> > +	struct kobject sysfs_gtn;
>
> If you put kobject as a part of intel_gt what assures you that lifetime of
> kobject is shorter than intel_gt? Ie its refcounter is 0 on removal of
> intel_gt?

Because we are explicitly doing a kobject_put() in
intel_gt_sysfs_unregister(). Which is exactly what we are *not* doing in
the previous code.

Let me explain a bit about the previous code (but feel free to skip since
the patch should speak for itself):
* Previously we kzalloc a 'struct kobj_gt'
* But we don't save a pointer to the 'struct kobj_gt' so we don't have the
  pointer to the kobject to be able to do a kobject_put() on it later
* Therefore we need to store the pointer in 'struct intel_gt'
* But if we have to put the pointer in 'struct intel_gt' we might as well
  put the kobject as part of 'struct intel_gt' and that also removes the
  need to have a 'struct kobj_gt' (kobj_to_gt() can just use container_of()
  to get gt from kobj).
* So I think this patch simpler/cleaner than the original code if you take
  the requirement for kobject_put() into account.

Thanks.
--
Ashutosh