Re: [PATCH 2/5] drm/gem: Add a mountpoint parameter to drm_gem_object_init()

["Maíra Canal" <mcanal@igalia.com>]

Not that the CC list wasn't big enough, but I'm adding MM folks
in the CC list.

On 3/18/24 11:04, Christian König wrote:
> Am 18.03.24 um 14:28 schrieb Maíra Canal:
>> Hi Christian,
>>
>> On 3/18/24 10:10, Christian König wrote:
>>> Am 18.03.24 um 13:42 schrieb Maíra Canal:
>>>> Hi Christian,
>>>>
>>>> On 3/12/24 10:48, Christian König wrote:
>>>>> Am 12.03.24 um 14:09 schrieb Tvrtko Ursulin:
>>>>>>
>>>>>> On 12/03/2024 10:37, Christian König wrote:
>>>>>>> Am 12.03.24 um 11:31 schrieb Tvrtko Ursulin:
>>>>>>>>
>>>>>>>> On 12/03/2024 10:23, Christian König wrote:
>>>>>>>>> Am 12.03.24 um 10:30 schrieb Tvrtko Ursulin:
>>>>>>>>>>
>>>>>>>>>> On 12/03/2024 08:59, Christian König wrote:
>>>>>>>>>>> Am 12.03.24 um 09:51 schrieb Tvrtko Ursulin:
>>>>>>>>>>>>
>>>>>>>>>>>> Hi Maira,
>>>>>>>>>>>>
>>>>>>>>>>>> On 11/03/2024 10:05, Maíra Canal wrote:
>>>>>>>>>>>>> For some applications, such as using huge pages, we might 
>>>>>>>>>>>>> want to have a
>>>>>>>>>>>>> different mountpoint, for which we pass in mount flags that 
>>>>>>>>>>>>> better match
>>>>>>>>>>>>> our usecase.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Therefore, add a new parameter to drm_gem_object_init() 
>>>>>>>>>>>>> that allow us to
>>>>>>>>>>>>> define the tmpfs mountpoint where the GEM object will be 
>>>>>>>>>>>>> created. If
>>>>>>>>>>>>> this parameter is NULL, then we fallback to 
>>>>>>>>>>>>> shmem_file_setup().
>>>>>>>>>>>>
>>>>>>>>>>>> One strategy for reducing churn, and so the number of 
>>>>>>>>>>>> drivers this patch touches, could be to add a lower level 
>>>>>>>>>>>> drm_gem_object_init() (which takes vfsmount, call it 
>>>>>>>>>>>> __drm_gem_object_init(), or drm__gem_object_init_mnt(), and 
>>>>>>>>>>>> make drm_gem_object_init() call that one with a NULL argument.
>>>>>>>>>>>
>>>>>>>>>>> I would even go a step further into the other direction. The 
>>>>>>>>>>> shmem backed GEM object is just some special handling as far 
>>>>>>>>>>> as I can see.
>>>>>>>>>>>
>>>>>>>>>>> So I would rather suggest to rename all drm_gem_* function 
>>>>>>>>>>> which only deal with the shmem backed GEM object into 
>>>>>>>>>>> drm_gem_shmem_*.
>>>>>>>>>>
>>>>>>>>>> That makes sense although it would be very churny. I at least 
>>>>>>>>>> would be on the fence regarding the cost vs benefit.
>>>>>>>>>
>>>>>>>>> Yeah, it should clearly not be part of this patch here.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> Also the explanation why a different mount point helps with 
>>>>>>>>>>> something isn't very satisfying.
>>>>>>>>>>
>>>>>>>>>> Not satisfying as you think it is not detailed enough to say 
>>>>>>>>>> driver wants to use huge pages for performance? Or not 
>>>>>>>>>> satisying as you question why huge pages would help?
>>>>>>>>>
>>>>>>>>> That huge pages are beneficial is clear to me, but I'm missing 
>>>>>>>>> the connection why a different mount point helps with using 
>>>>>>>>> huge pages.
>>>>>>>>
>>>>>>>> Ah right, same as in i915, one needs to mount a tmpfs instance 
>>>>>>>> passing huge=within_size or huge=always option. Default is 
>>>>>>>> 'never', see man 5 tmpfs.
>>>>>>>
>>>>>>> Thanks for the explanation, I wasn't aware of that.
>>>>>>>
>>>>>>> Mhm, shouldn't we always use huge pages? Is there a reason for a 
>>>>>>> DRM device to not use huge pages with the shmem backend?
>>>>>>
>>>>>> AFAIU, according to b901bb89324a ("drm/i915/gemfs: enable THP"), 
>>>>>> back then the understanding was within_size may overallocate, 
>>>>>> meaning there would be some space wastage, until the memory 
>>>>>> pressure makes the thp code split the trailing huge page. I 
>>>>>> haven't checked if that still applies.
>>>>>>
>>>>>> Other than that I don't know if some drivers/platforms could have 
>>>>>> problems if they have some limitations or hardcoded assumptions 
>>>>>> when they iterate the sg list.
>>>>>
>>>>> Yeah, that was the whole point behind my question. As far as I can 
>>>>> see this isn't driver specific, but platform specific.
>>>>>
>>>>> I might be wrong here, but I think we should then probably not have 
>>>>> that handling in each individual driver, but rather centralized in 
>>>>> the DRM code.
>>>>
>>>> I don't see a point in enabling THP for all shmem drivers. A huge page
>>>> is only useful if the driver is going to use it. On V3D, for example,
>>>> I only need huge pages because I need the memory contiguously allocated
>>>> to implement Super Pages. Otherwise, if we don't have the Super Pages
>>>> support implemented in the driver, I would be creating memory pressure
>>>> without any performance gain.
>>>
>>> Well that's the point I'm disagreeing with. THP doesn't seem to 
>>> create much extra memory pressure for this use case.
>>>
>>> As far as I can see background for the option is that files in tmpfs 
>>> usually have a varying size, so it usually isn't beneficial to 
>>> allocate a huge page just to find that the shmem file is much smaller 
>>> than what's needed.
>>>
>>> But GEM objects have a fixed size. So we of hand knew if we need 4KiB 
>>> or 1GiB and can therefore directly allocate huge pages if they are 
>>> available and object large enough to back them with.
>>>
>>> If the memory pressure is so high that we don't have huge pages 
>>> available the shmem code falls back to standard pages anyway.
>>
>> The matter is: how do we define the point where the memory pressure is 
>> high?
> 
> Well as driver developers/maintainers we simply don't do that. This is 
> the job of the shmem code.
> 
>> For example, notice that in this implementation of Super Pages
>> for the V3D driver, I only use a Super Page if the BO is bigger than 
>> 2MB. I'm doing that because the Raspberry Pi only has 4GB of RAM 
>> available for the GPU. If I created huge pages for every BO allocation 
>> (and initially, I tried that), I would end up with hangs in some 
>> applications.
> 
> Yeah, that is what I meant with the trivial optimisation to the shmem 
> code. Essentially when you have 2MiB+4KiB as BO size then the shmem code 
> should use a 2MiB and a 4KiB page to back them, but what it currently 
> does is to use two 2MiB pages and then split up the second page when it 
> find that it isn't needed.
> 
> That is wasteful and leads to fragmentation, but as soon as we stop 
> doing that we should be fine to enable it unconditionally for all drivers.

I see your point, but I believe that it would be tangent to the goal of
this series. As you mentioned, currently, we have a lot of memory
fragmentation when using THP and while we don't solve that (at the tmpfs
level), I believe we could move on with the current implementation (with
improvements proposed by Tvrtko).

Best Regards,
- Maíra

> 
> TTM does essentially the same thing for years.
> 
> Regards,
> Christian.
> 
>>
>>
>> At least, for V3D, I wouldn't like to see THP being used for all the 
>> allocations. But, we have maintainers of other drivers in the CC.
>>
>> Best Regards,
>> - Maíra
>>
>>>
>>> So THP is almost always beneficial for GEM even if the driver doesn't 
>>> actually need it. The only potential case I can think of which might 
>>> not be handled gracefully is the tail pages, e.g. huge + 4kib.
>>>
>>> But that is trivial to optimize in the shmem code when the final size 
>>> of the file is known beforehand.
>>>
>>> Regards,
>>> Christian.
>>>
>>>>
>>>> Best Regards,
>>>> - Maíra
>>>>
>>>>>
>>>>> Regards,
>>>>> Christian.
>>>>>
>>>>>
>>>>>>
>>>>>> Te Cc is plenty large so perhaps someone else will have additional 
>>>>>> information. :)
>>>>>>
>>>>>> Regards,
>>>>>>
>>>>>> Tvrtko
>>>>>>
>>>>>>>
>>>>>>> I mean it would make this patch here even smaller.
>>>>>>>
>>>>>>> Regards,
>>>>>>> Christian.
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>>
>>>>>>>> Tvrtko
>>>>>>>
>>>>>
>>>
>