Re: [Intel-xe] [v2 1/2] drm/xe: Add a new memory directory under tile

["Sundaresan, Sujaritha" <sujaritha.sundaresan@intel.com>]

On 12/7/2023 3:25 PM, Sundaresan, Sujaritha wrote:
>
> On 12/7/2023 2:00 PM, Upadhyay, Tejas wrote:
>>
>>> -----Original Message-----
>>> From: Sundaresan, Sujaritha <sujaritha.sundaresan@intel.com>
>>> Sent: Thursday, December 7, 2023 12:58 PM
>>> To: Tauro, Riana <riana.tauro@intel.com>; Upadhyay, Tejas
>>> <tejas.upadhyay@intel.com>; Gupta, Anshuman
>>> <anshuman.gupta@intel.com>; intel-xe@lists.freedesktop.org
>>> Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>
>>> Subject: Re: [Intel-xe] [v2 1/2] drm/xe: Add a new memory directory 
>>> under
>>> tile
>>>
>>>
>>> On 12/7/2023 12:08 PM, Sundaresan, Sujaritha wrote:
>>>> On 12/7/2023 11:36 AM, Riana Tauro wrote:
>>>>>
>>>>> On 12/7/2023 10:51 AM, Sundaresan, Sujaritha wrote:
>>>>>> On 12/7/2023 10:42 AM, Upadhyay, Tejas wrote:
>>>>>>>> -----Original Message-----
>>>>>>>> From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf
>>>>>>>> Of Sundaresan, Sujaritha
>>>>>>>> Sent: Wednesday, December 6, 2023 5:44 PM
>>>>>>>> To: Gupta, Anshuman <anshuman.gupta@intel.com>; intel-
>>>>>>>> xe@lists.freedesktop.org
>>>>>>>> Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>
>>>>>>>> Subject: Re: [Intel-xe] [v2 1/2] drm/xe: Add a new memory
>>>>>>>> directory under tile
>>>>>>>>
>>>>>>>>
>>>>>>>> On 12/6/2023 5:38 PM, Sundaresan, Sujaritha wrote:
>>>>>>>>> On 12/6/2023 5:23 PM, Gupta, Anshuman wrote:
>>>>>>>>>>> -----Original Message-----
>>>>>>>>>>> From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On
>>>>>>>>>>> Behalf Of Sujaritha Sundaresan
>>>>>>>>>>> Sent: Wednesday, December 6, 2023 10:18 AM
>>>>>>>>>>> To: intel-xe@lists.freedesktop.org
>>>>>>>>>>> Cc: Sundaresan, Sujaritha <sujaritha.sundaresan@intel.com>;
>>>>>>>>>>> Vivi, Rodrigo <rodrigo.vivi@intel.com>
>>>>>>>>>>> Subject: [Intel-xe] [v2 1/2] drm/xe: Add a new memory directory
>>>>>>>>>>> under tile
>>>>>>>>>>>
>>>>>>>>>>> Add a new memory directory under /device/tile<n> and move
>>>>>>>>>>> physical_vram_size attribute to the new directory.
>>>>>>>>>>>
>>>>>>>>>>> New hierarchy:
>>>>>>>>>>>
>>>>>>>>>>> /device/tile<n>/memory/physical_vram_size_bytes
>>>>>>>>>>>
>>>>>>>>>>> v2: Fix heading typo (Riana)
>>>>>>>>>>>        Fix cleanup error on unload/reload cycle
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Sujaritha Sundaresan
>>>>>>>>>>> <sujaritha.sundaresan@intel.com>
>>>>>>>>>>> ---
>>>>>>>>>>>     drivers/gpu/drm/xe/xe_tile_sysfs.c | 15 ++++++++++++---
>>>>>>>>>>>     1 file changed, 12 insertions(+), 3 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/drivers/gpu/drm/xe/xe_tile_sysfs.c
>>>>>>>>>>> b/drivers/gpu/drm/xe/xe_tile_sysfs.c
>>>>>>>>>>> index 16376607c68f..e8ce4d9270e6 100644
>>>>>>>>>>> --- a/drivers/gpu/drm/xe/xe_tile_sysfs.c
>>>>>>>>>>> +++ b/drivers/gpu/drm/xe/xe_tile_sysfs.c
>>>>>>>>>>> @@ -24,7 +24,8 @@ static ssize_t
>>>>>>>>>>>     physical_vram_size_bytes_show(struct device *kdev, struct
>>>>>>>>>>> device_attribute *attr,
>>>>>>>>>>>                       char *buf)
>>>>>>>>>>>     {
>>>>>>>>>>> -    struct xe_tile *tile = kobj_to_tile(&kdev->kobj);
>>>>>>>>>>> +    struct kobject *kobj = &kdev->kobj;
>>>>>>>>>>> +    struct xe_tile *tile = kobj_to_tile(kobj->parent);
>>>>>>>>>>>
>>>>>>>>>>>         return sysfs_emit(buf, "%llu\n",
>>>>>>>>>>> tile->mem.vram.actual_physical_size);
>>>>>>>>>>>     }
>>>>>>>>>>> @@ -38,7 +39,7 @@ static void tile_sysfs_fini(struct drm_device
>>>>>>>>>>> *drm, void
>>>>>>>>>>> *arg)  {
>>>>>>>>>>>         struct xe_tile *tile = arg;
>>>>>>>>>>>
>>>>>>>>>>> -    kobject_put(tile->sysfs);
>>>>>>>>>>> +    kobject_del(tile->sysfs);
>>>>>>>>>> Why kobekct_del instead of kobject_put?
>>>>>>>>>> Thanks,
>>>>>>>>>> Anshuman Gupta.
>>>>>>>>> Hi Anshuman,
>>>>>>>>>
>>>>>>>>> Basically when sanity checking, after reload we see that we are
>>>>>>>>> not doing a proper cleanup.
>>>>>>>>>
>>>>>>>>> kobject_put will only decrement the ref count and possibly free
>>>>>>>>> the kobject.
>>>>>>>>>
>>>>>>>>> But that is not happening in this case. There is a duplicate
>>>>>>>>> remaining of the tile directory.
>>>>>>>>>
>>>>>>>>> This required a clean unregister of the parent from sysfs hence
>>>>>>>>> the use of kobject_del.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>>
>>>>>>>>> Suja
>>>>>>>> As a continuation of the above response;
>>>>>>>>
>>>>>>>> I can probably add a kobject_put call as well to ensure that we
>>>>>>>> are cleaning up the memory side of
>>>>>>>>
>>>>>>>> things as well. Will add.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>> Suja
>>>>>>>>
>>>>>>>>>>>     }
>>>>>>>>>>>
>>>>>>>>>>>     void xe_tile_sysfs_init(struct xe_tile *tile) @@ -46,6 
>>>>>>>>>>> +47,7
>>>>>>>>>>> @@ void xe_tile_sysfs_init(struct xe_tile *tile)
>>>>>>>>>>>         struct xe_device *xe = tile_to_xe(tile);
>>>>>>>>>>>         struct device *dev = xe->drm.dev;
>>>>>>>>>>>         struct kobj_tile *kt;
>>>>>>>>>>> +    struct kobject *kobj;
>>>>>>>>>>>         int err;
>>>>>>>>>>>
>>>>>>>>>>>         kt = kzalloc(sizeof(*kt), GFP_KERNEL); @@ -64,8 +66,15
>>>>>>>>>>> @@ void xe_tile_sysfs_init(struct xe_tile *tile)
>>>>>>>>>>>
>>>>>>>>>>>         tile->sysfs = &kt->base;
>>>>>>>>>>>
>>>>>>>>>>> +    kobj = kobject_create_and_add("memory", tile->sysfs);
>>>>>>>>>>> +    if (!kobj) {
>>>>>>>>>>> +        kobject_put(kobj);
>>>>>>> Do you mean to put kobject_put(tile->sysfs) instead of
>>>>>>> kobject_put(kobj) ? as there was no Kobj created by the time you
>>>>>>> reached here!
>>>>>>>
>>>>>>> Tejas
>>>>>> Yup this should be fixed.
>>>>> Hi Suja
>>>>>
>>>>> Removing tile won't be right, as there are other directories (gt#)
>>>>> dependent on it. Simple return should be good with a warn?
>>>>>
>>>>> Thanks
>>>>> Riana
>>>> Sure. We can probably have the original cleanup in fini.
>> If you just give warn and return then you will never register 
>> tile_fini function and it will never be called on driver 
>> unload/reload. So either you remove tile using kobject_put() before 
>> return or don’t return and check kobj before creating files under it 
>> to avoid crash.
>>
>> Tejas
>
> Let me test out everything and see which approach will work the best.
>
> Thanks,
>
> Suja

After a round of debug with Tejas, was able to fix the cleanup cycle. 
Will send out v3 asap.

Thanks,

Suja

>
>>>>>> Thanks.
>>>>>>
>>>>>> Suja
>>>>>>
>>>>>>>>>>> + drm_warn(&xe->drm, "%s failed, err: %d\n", __func__, -
>>>>>>>>>>> ENOMEM);
>>>>>>>>>>> +        return;
>>>>>>>>>>> +    }
>>>>>>>>>>> +
>>>>>>>>>>>         if (IS_DGFX(xe) && xe->info.platform != XE_DG1 &&
>>>>>>>>>>> -        sysfs_create_file(tile->sysfs, physical_memsize_attr))
>>>>>>>>>>> +        sysfs_create_file(kobj, physical_memsize_attr))
>>>>>>>>>>>             drm_warn(&xe->drm,
>>>>>>>>>>>                  "Sysfs creation to read addr_range per tile
>>>>>>>>>>> failed\n");
>>>>>>>>>>>
>>>>>>>>>>> -- 
>>>>>>>>>>> 2.25.1
>>> Hi all,
>>>
>>> So after all of this discussion, here's the final results after 
>>> testing the cleanup.
>>>
>>> Regardless of anything we do in the init function, it looks like 
>>> without having
>>> the
>>>
>>> two-step kobject_del and kobject_put cleanup in the sysfs_fini 
>>> function we
>>> will
>>>
>>> see an error on reload with the tile directory not being fully 
>>> cleaned up and
>>> reporting
>>>
>>> duplicate creation.
>>>
>>> The solution seems to be just to add a kobject_del before 
>>> kobject_put in the
>>> fini
>>>
>>> function.
>>>
>>> I am only having warnings and simple returns on the init side.
>>>
>>> If there can be a consensus about this from the reviewers, I can 
>>> float the next
>>> version
>>>
>>> accordingly.
>>>
>>> Thanks,
>>>
>>> Suja