linux-kernel - Re: [PATCH V7 2/2] mm: shmem: implement POSIX_FADV

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8ba3f700-19a6-e7fd-c51f-cd277ff7a439@quicinc.com>
Date:   Mon, 10 Apr 2023 19:22:22 +0530
From:   Charan Teja Kalla <quic_charante@...cinc.com>
To:     Minchan Kim <minchan@...nel.org>
CC:     <akpm@...ux-foundation.org>, <hughd@...gle.com>,
        <willy@...radead.org>, <markhemm@...glemail.com>,
        <rientjes@...gle.com>, <surenb@...gle.com>, <shakeelb@...gle.com>,
        <quic_pkondeti@...cinc.com>, <linux-mm@...ck.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V7 2/2] mm: shmem: implement POSIX_FADV_[WILL|DONT]NEED
 for shmem

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@...cinc.com>
> 
> 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