Re: [PATCH V7 2/2] mm: shmem: implement POSIX_FADV_[WILL|DONT]NEED for shmem

[Charan Teja Kalla]

Thanks Minchan for the review!!

On 4/7/2023 5:14 AM, Minchan Kim wrote:
> On Tue, Feb 14, 2023 at 06:21:50PM +0530, Charan Teja Kalla wrote:
>> Currently fadvise(2) is supported only for the files that doesn't
>> associated with noop_backing_dev_info thus for the files, like shmem,
>> fadvise results into NOP. But then there is file_operations->fadvise()
>> that lets the file systems to implement their own fadvise
>> implementation. Use this support to implement some of the POSIX_FADV_XXX
>> functionality for shmem files.
>>
>> This patch aims to implement POSIX_FADV_WILLNEED and POSIX_FADV_DONTNEED
>> advices to shmem files which can be helpful for the clients who may want
>> to manage the shmem pages of the files that are created through
>> shmem_file_setup[_with_mnt](). One usecase is implemented on the
>> Snapdragon SoC's running Android where the graphics client is allocating
>> lot of shmem pages per process and pinning them. When this process is
>> put to background, the instantaneous reclaim is performed on those shmem
>> pages using the logic implemented downstream[3][4]. With this patch, the
>> client can now issue the fadvise calls on the shmem files that does the
>> instantaneous reclaim which can aid the use cases like mentioned above.
>>
>> This usecase lead to ~2% reduction in average launch latencies of the
>> apps and 10% in total number of kills by the low memory killer running
>> on Android.
>>
>> Some questions asked while reviewing this patch:
>> Q) Can the same thing be achieved with FD mapped to user and use
>> madvise?
>> A) All drivers are not mapping all the shmem fd's to user space and want
>> to manage them with in the kernel. Ex: shmem memory can be mapped to the
>> other subsystems and they fill in the data and then give it to other
>> subsystem for further processing, where, the user mapping is not at all
>> required.  A simple example, memory that is given for gpu subsystem
>> which can be filled directly and give to display subsystem. And the
>> respective drivers know well about when to keep that memory in ram or
>> swap based on may be a user activity.
>>
>> Q) Should we add the documentation section in Manual pages?
>> A) The man[1] pages for the fadvise() whatever says is also applicable
>> for shmem files. so couldn't feel it correct to add specific to shmem
>> files separately.
>>
>> Q) The proposed semantics of POSIX_FADV_DONTNEED is actually similar to
>> MADV_PAGEOUT and different from MADV_DONTNEED. This is a user facing API
>> and this difference will cause confusion?
>> A) man pages [2] says that "POSIX_FADV_DONTNEED attempts to free cached
>> pages associated with the specified region." This means on issuing this
>> FADV, it is expected to free the file cache pages. And it is
>> implementation defined If the dirty pages may be attempted to writeback.
>> And the unwritten dirty pages will not be freed. So, FADV_DONTNEED also
>> covers the semantics of MADV_PAGEOUT for file pages and there is no
>> purpose of PAGEOUT for file pages.
>>
>> [1] https://linux.die.net/man/2/fadvise
>> [2] https://man7.org/linux/man-pages/man2/posix_fadvise.2.html
>> [3] https://git.codelinaro.org/clo/la/platform/vendor/qcom/opensource/graphics-kernel/-/blob/gfx-kernel.lnx.1.0.r3-rel/kgsl_reclaim.c#L289
>> [4] https://android.googlesource.com/kernel/common/+/refs/heads/android12-5.10/mm/shmem.c#4310
>>
>> Signed-off-by: Charan Teja Kalla <quic_charante@xxxxxxxxxxx>
> 
> I am not familar with why the shmem has noop_backing_dev_info
> but the below code to reclaim shmem pages and POXIS_FADV_DONTNEED
> semantic looks correct for me.
> 
Thanks!!
> Only nit is the description covers mostly DONTNEED case but not
> WILLNEED case.Okay. How about adding the below to the end of the 2nd paragraph of the
commit message:
Application that does require the reclaimed pages, say when the app put
to foreground, can issue the POSIX_FADV_WILLNEED to bring back them from
the swap area. Alternatively the drivers can also use
shmem_read_mapping_page_gfp() to bring back the reclaimed shmem pages.

@Andrew: I am not sure If this update to commit message requires respin
of the patchset. Please let me know If it required so.

Thanks,
Charan