[PATCH V7 0/2] mm: shmem: support POSIX_FADV_[WILL|DONT]NEED for shmem files

[PATCH V7 0/2] mm: shmem: support POSIX_FADV_[WILL|DONT]NEED for shmem files

[Charan Teja Kalla]

This patch aims to implement POSIX_FADV_WILLNEED and POSIX_FADV_DONTNEED
advices to shmem files which can be helpful for the drivers who may want
to manage the pages of shmem files on their own, like, that are created
through shmem_file_setup[_with_mnt]().

Changes in V7:
 -- Use folio based interface, shmem_read_folio(), for FADV_WILLNEED.
 -- Don't swap the SHM_LOCK'ed pages. 

Changes in V6:
 -- Replaced the pages with folio's for shmem changes.
 -- https://lore.kernel.org/all/cover.1675690847.git.quic_charante@quicinc.com/

Changes in V5:
 -- Moved the 'endbyte' calculations to a header function for use by shmem_fadvise().
 -- Addressed comments from suren.
 -- No changes in resend. Retested on the latest tip.
 -- https://lore.kernel.org/all/cover.1648706231.git.quic_charante@quicinc.com/

Changes in V4:
  -- Changed the code to use reclaim_pages() to writeout the shmem pages to swap and then reclaim.
  -- Addressed comments from Mark Hemment and Matthew.
  -- fadvise() on shmem file may even unmap a page.
  -- https://patchwork.kernel.org/project/linux-mm/patch/1644572051-24091-1-git-send-email-quic_charante@quicinc.com/

Changes in V3:
  -- Considered THP pages while doing FADVISE_[DONT|WILL]NEED, identified by Matthew.
  -- xarray used properly, as identified by Matthew.
  -- Excluded mapped pages as it requires unmapping and the man pages of fadvise don't talk about them.
  -- RESEND: Fixed the compilation issue when CONFIG_TMPFS is not defined.
  -- https://patchwork.kernel.org/project/linux-mm/patch/1641488717-13865-1-git-send-email-quic_charante@quicinc.com/

Changes in V2:
  -- Rearranged the code to not to sleep with rcu_lock while using xas_() functionality.
  -- Addressed the comments from Suren.
  -- https://patchwork.kernel.org/project/linux-mm/patch/1638442253-1591-1-git-send-email-quic_charante@quicinc.com/

changes in V1:
  -- Created the interface for fadvise(2) to work on shmem files.
  -- https://patchwork.kernel.org/project/linux-mm/patch/1633701982-22302-1-git-send-email-charante@codeaurora.org/


Charan Teja Kalla (2):
  mm: fadvise: move 'endbyte' calculations to helper function
  mm: shmem: implement POSIX_FADV_[WILL|DONT]NEED for shmem

 mm/fadvise.c  |  11 +-----
 mm/internal.h |  21 +++++++++++
 mm/shmem.c    | 116 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 138 insertions(+), 10 deletions(-)

-- 
2.7.4

[PATCH V7 1/2] mm: fadvise: move 'endbyte' calculations to helper function

[Charan Teja Kalla]

Move the 'endbyte' calculations that determines last byte that fadvise
can to a helper function. This is a preparatory change made for
shmem_fadvise() functionality in the next patch. No functional changes
in this patch.

Signed-off-by: Charan Teja Kalla <quic_charante@quicinc.com>
---
 mm/fadvise.c  | 11 +----------
 mm/internal.h | 21 +++++++++++++++++++++
 2 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/mm/fadvise.c b/mm/fadvise.c
index fb7c5f4..1b3cac5 100644
--- a/mm/fadvise.c
+++ b/mm/fadvise.c
@@ -65,16 +65,7 @@ int generic_fadvise(struct file *file, loff_t offset, loff_t len, int advice)
 		return 0;
 	}
 
-	/*
-	 * Careful about overflows. Len == 0 means "as much as possible".  Use
-	 * unsigned math because signed overflows are undefined and UBSan
-	 * complains.
-	 */
-	endbyte = (u64)offset + (u64)len;
-	if (!len || endbyte < len)
-		endbyte = LLONG_MAX;
-	else
-		endbyte--;		/* inclusive */
+	endbyte = fadvise_calc_endbyte(offset, len);
 
 	switch (advice) {
 	case POSIX_FADV_NORMAL:
diff --git a/mm/internal.h b/mm/internal.h
index dfb37e9..4445e48 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -657,6 +657,27 @@ static inline void vunmap_range_noflush(unsigned long start, unsigned long end)
 }
 #endif /* !CONFIG_MMU */
 
+/*
+ * Helper function to get the endbyte of a file that fadvise can operate on.
+ */
+static inline loff_t fadvise_calc_endbyte(loff_t offset, loff_t len)
+{
+	loff_t endbyte;
+
+	/*
+	 * Careful about overflows. Len == 0 means "as much as possible".  Use
+	 * unsigned math because signed overflows are undefined and UBSan
+	 * complains.
+	 */
+	endbyte = (u64)offset + (u64)len;
+	if (!len || endbyte < len)
+		endbyte = LLONG_MAX;
+	else
+		endbyte--;		/* inclusive */
+
+	return endbyte;
+}
+
 /* Memory initialisation debug and verification */
 enum mminit_level {
 	MMINIT_WARNING,
-- 
2.7.4

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

[Charan Teja Kalla]

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@quicinc.com>
---
 mm/shmem.c | 116 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 116 insertions(+)

diff --git a/mm/shmem.c b/mm/shmem.c
index 448f393..1af8525 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -40,6 +40,9 @@
 #include <linux/fs_parser.h>
 #include <linux/swapfile.h>
 #include <linux/iversion.h>
+#include <linux/mm_inline.h>
+#include <linux/fadvise.h>
+#include <linux/page_idle.h>
 #include "swap.h"
 
 static struct vfsmount *shm_mnt;
@@ -2344,6 +2347,118 @@ static void shmem_set_inode_flags(struct inode *inode, unsigned int fsflags)
 #define shmem_initxattrs NULL
 #endif
 
+static void shmem_isolate_pages_range(struct address_space *mapping, loff_t start,
+				loff_t end, struct list_head *list)
+{
+	XA_STATE(xas, &mapping->i_pages, start);
+	struct folio *folio;
+
+	rcu_read_lock();
+	xas_for_each(&xas, folio, end) {
+		if (xas_retry(&xas, folio))
+			continue;
+		if (xa_is_value(folio))
+			continue;
+
+		if (!folio_try_get(folio))
+			continue;
+		if (folio_test_unevictable(folio) || folio_mapped(folio) ||
+				folio_isolate_lru(folio)) {
+			folio_put(folio);
+			continue;
+		}
+		folio_put(folio);
+
+		/*
+		 * Prepare the folios to be passed to reclaim_pages().
+		 * VM can't reclaim a folio unless young bit is
+		 * cleared in its flags.
+		 */
+		folio_clear_referenced(folio);
+		folio_test_clear_young(folio);
+		list_add(&folio->lru, list);
+		if (need_resched()) {
+			xas_pause(&xas);
+			cond_resched_rcu();
+		}
+	}
+	rcu_read_unlock();
+}
+
+static int shmem_fadvise_dontneed(struct address_space *mapping, loff_t start,
+				loff_t end)
+{
+	LIST_HEAD(folio_list);
+
+	if (!total_swap_pages || mapping_unevictable(mapping))
+		return 0;
+
+	lru_add_drain();
+	shmem_isolate_pages_range(mapping, start, end, &folio_list);
+	reclaim_pages(&folio_list);
+
+	return 0;
+}
+
+static int shmem_fadvise_willneed(struct address_space *mapping,
+				 pgoff_t start, pgoff_t long end)
+{
+	struct folio *folio;
+	pgoff_t index;
+
+	xa_for_each_range(&mapping->i_pages, index, folio, start, end) {
+		if (!xa_is_value(folio))
+			continue;
+		folio = shmem_read_folio(mapping, index);
+		if (!IS_ERR(folio))
+			folio_put(folio);
+	}
+
+	return 0;
+}
+
+static int shmem_fadvise(struct file *file, loff_t offset, loff_t len, int advice)
+{
+	loff_t endbyte;
+	pgoff_t start_index;
+	pgoff_t end_index;
+	struct address_space *mapping;
+	struct inode *inode = file_inode(file);
+	int ret = 0;
+
+	if (S_ISFIFO(inode->i_mode))
+		return -ESPIPE;
+
+	mapping = file->f_mapping;
+	if (!mapping || len < 0 || !shmem_mapping(mapping))
+		return -EINVAL;
+
+	endbyte = fadvise_calc_endbyte(offset, len);
+
+	start_index = offset >> PAGE_SHIFT;
+	end_index   = endbyte >> PAGE_SHIFT;
+	switch (advice) {
+	case POSIX_FADV_DONTNEED:
+		ret = shmem_fadvise_dontneed(mapping, start_index, end_index);
+		break;
+	case POSIX_FADV_WILLNEED:
+		ret = shmem_fadvise_willneed(mapping, start_index, end_index);
+		break;
+	case POSIX_FADV_NORMAL:
+	case POSIX_FADV_RANDOM:
+	case POSIX_FADV_SEQUENTIAL:
+	case POSIX_FADV_NOREUSE:
+		/*
+		 * No bad return value, but ignore advice.
+		 */
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return ret;
+}
+
 static struct inode *shmem_get_inode(struct mnt_idmap *idmap, struct super_block *sb,
 				     struct inode *dir, umode_t mode, dev_t dev,
 				     unsigned long flags)
@@ -3942,6 +4057,7 @@ static const struct file_operations shmem_file_operations = {
 	.splice_write	= iter_file_splice_write,
 	.fallocate	= shmem_fallocate,
 #endif
+	.fadvise	= shmem_fadvise,
 };
 
 static const struct inode_operations shmem_inode_operations = {
-- 
2.7.4

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

[Andrew Morton]

On Tue, 14 Feb 2023 18:21:48 +0530 Charan Teja Kalla <quic_charante@quicinc.com> wrote:

> This patch aims to implement POSIX_FADV_WILLNEED and POSIX_FADV_DONTNEED
> advices to shmem files which can be helpful for the drivers who may want
> to manage the pages of shmem files on their own, like, that are created
> through shmem_file_setup[_with_mnt]().

Could we please have some additional review of this series?

Thanks.

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

[Suren Baghdasaryan]

On Tue, Mar 28, 2023 at 3:50 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Tue, 14 Feb 2023 18:21:48 +0530 Charan Teja Kalla <quic_charante@quicinc.com> wrote:
>
> > This patch aims to implement POSIX_FADV_WILLNEED and POSIX_FADV_DONTNEED
> > advices to shmem files which can be helpful for the drivers who may want
> > to manage the pages of shmem files on their own, like, that are created
> > through shmem_file_setup[_with_mnt]().
>
> Could we please have some additional review of this series?

The series LGTM but unfortunately I'm not a shmem expert. I asked
Minchan to take a look as well.
Thanks!

>
> Thanks.

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

[Minchan Kim]

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@quicinc.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.

Only nit is the description covers mostly DONTNEED case but not
WILLNEED case.

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@quicinc.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

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

[Andrew Morton]

On Mon, 10 Apr 2023 19:22:22 +0530 Charan Teja Kalla <quic_charante@quicinc.com> wrote:

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

Please just send us the new text and I'll do the copy-paste.

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

[Frank van der Linden]

On Tue, Feb 14, 2023 at 4:53 AM Charan Teja Kalla
<quic_charante@quicinc.com> wrote:
>
> This patch aims to implement POSIX_FADV_WILLNEED and POSIX_FADV_DONTNEED
> advices to shmem files which can be helpful for the drivers who may want
> to manage the pages of shmem files on their own, like, that are created
> through shmem_file_setup[_with_mnt]().
>
> Changes in V7:
>  -- Use folio based interface, shmem_read_folio(), for FADV_WILLNEED.
>  -- Don't swap the SHM_LOCK'ed pages.
>
> Changes in V6:
>  -- Replaced the pages with folio's for shmem changes.
>  -- https://lore.kernel.org/all/cover.1675690847.git.quic_charante@quicinc.com/
>
> Changes in V5:
>  -- Moved the 'endbyte' calculations to a header function for use by shmem_fadvise().
>  -- Addressed comments from suren.
>  -- No changes in resend. Retested on the latest tip.
>  -- https://lore.kernel.org/all/cover.1648706231.git.quic_charante@quicinc.com/
>
> Changes in V4:
>   -- Changed the code to use reclaim_pages() to writeout the shmem pages to swap and then reclaim.
>   -- Addressed comments from Mark Hemment and Matthew.
>   -- fadvise() on shmem file may even unmap a page.
>   -- https://patchwork.kernel.org/project/linux-mm/patch/1644572051-24091-1-git-send-email-quic_charante@quicinc.com/
>
> Changes in V3:
>   -- Considered THP pages while doing FADVISE_[DONT|WILL]NEED, identified by Matthew.
>   -- xarray used properly, as identified by Matthew.
>   -- Excluded mapped pages as it requires unmapping and the man pages of fadvise don't talk about them.
>   -- RESEND: Fixed the compilation issue when CONFIG_TMPFS is not defined.
>   -- https://patchwork.kernel.org/project/linux-mm/patch/1641488717-13865-1-git-send-email-quic_charante@quicinc.com/
>
> Changes in V2:
>   -- Rearranged the code to not to sleep with rcu_lock while using xas_() functionality.
>   -- Addressed the comments from Suren.
>   -- https://patchwork.kernel.org/project/linux-mm/patch/1638442253-1591-1-git-send-email-quic_charante@quicinc.com/
>
> changes in V1:
>   -- Created the interface for fadvise(2) to work on shmem files.
>   -- https://patchwork.kernel.org/project/linux-mm/patch/1633701982-22302-1-git-send-email-charante@codeaurora.org/
>
>
> Charan Teja Kalla (2):
>   mm: fadvise: move 'endbyte' calculations to helper function
>   mm: shmem: implement POSIX_FADV_[WILL|DONT]NEED for shmem
>
>  mm/fadvise.c  |  11 +-----
>  mm/internal.h |  21 +++++++++++
>  mm/shmem.c    | 116 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 138 insertions(+), 10 deletions(-)
>
> --
> 2.7.4
>
>

I didn't see this patch before, so I looked a bit at the history. At
some point, in v3, dealing with mapped pages for DONTNEED was left
out, they are now skipped. Unfortunately, that makes this patch no
longer usable for a case that we have: restoring the (approximate)
swap state of a tmpfs file. This involves walking a potentially large
number of regions, and explicitly pushing them out to swap. This can
be used to e.g. restore the state VM memory that is backed by a tmpfs
file, avoiding memory usage by cold VM pages after resume.

If DONTNEED also reclaims mapped pages (e.g. they get pushed out to
swap, if any), implementing the above use case efficiently is simple:
use io_uring with a vector that contains each region and the fadvise
method.

Without DONTNEED reclaiming mapped pages, you'd have to do mmap +
madvise(MADV_PAGEOUT) for each region that you want swapped out, which
is rather inefficient.

I understand that the semantics for POSIX_FADV_DONTNEED on shmem/tmpfs
files are open to interpretation, as it is a special case. And you can
certainly make the argument that relying on behavior caused by what
can be considered an implementation detail is bad.

So, is there any way we can make this use case work efficiently using
this patch?

You state in the commit message:

> So, FADV_DONTNEED also covers the semantics of MADV_PAGEOUT for file pages
> and there is no purpose of PAGEOUT for file pages.

But that doesn't seem correct: for shmem file pages, there actually
can be a purpose, and the FADV_DONTNEED as implemented for shmem in
this patch set does not cover the semantics.

You can say that it doesn't need to cover the pageout case of mapped
shmem pages, and that's fair. But I don't think you can claim that it
covers the case as currently implemented.

I suppose there are three options here:

1) Do nothing, this use case will just have to spend more time doing
mmap+madvise
2) Don't skip mapped pages for POSIX_FADV_DONTNEED in shmem_fadvise
3) Implement something like POSIX_FADV_PAGEOUT_NP, which would include
mapped pages.

What do people think?

- Frank

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

[Frank van der Linden]

On Thu, Apr 13, 2023 at 12:45 PM Frank van der Linden <fvdl@google.com> wrote:
>
> On Tue, Feb 14, 2023 at 4:53 AM Charan Teja Kalla
> <quic_charante@quicinc.com> wrote:
> >
> > This patch aims to implement POSIX_FADV_WILLNEED and POSIX_FADV_DONTNEED
> > advices to shmem files which can be helpful for the drivers who may want
> > to manage the pages of shmem files on their own, like, that are created
> > through shmem_file_setup[_with_mnt]().
> >
> > Changes in V7:
> >  -- Use folio based interface, shmem_read_folio(), for FADV_WILLNEED.
> >  -- Don't swap the SHM_LOCK'ed pages.
> >
> > Changes in V6:
> >  -- Replaced the pages with folio's for shmem changes.
> >  -- https://lore.kernel.org/all/cover.1675690847.git.quic_charante@quicinc.com/
> >
> > Changes in V5:
> >  -- Moved the 'endbyte' calculations to a header function for use by shmem_fadvise().
> >  -- Addressed comments from suren.
> >  -- No changes in resend. Retested on the latest tip.
> >  -- https://lore.kernel.org/all/cover.1648706231.git.quic_charante@quicinc.com/
> >
> > Changes in V4:
> >   -- Changed the code to use reclaim_pages() to writeout the shmem pages to swap and then reclaim.
> >   -- Addressed comments from Mark Hemment and Matthew.
> >   -- fadvise() on shmem file may even unmap a page.
> >   -- https://patchwork.kernel.org/project/linux-mm/patch/1644572051-24091-1-git-send-email-quic_charante@quicinc.com/
> >
> > Changes in V3:
> >   -- Considered THP pages while doing FADVISE_[DONT|WILL]NEED, identified by Matthew.
> >   -- xarray used properly, as identified by Matthew.
> >   -- Excluded mapped pages as it requires unmapping and the man pages of fadvise don't talk about them.
> >   -- RESEND: Fixed the compilation issue when CONFIG_TMPFS is not defined.
> >   -- https://patchwork.kernel.org/project/linux-mm/patch/1641488717-13865-1-git-send-email-quic_charante@quicinc.com/
> >
> > Changes in V2:
> >   -- Rearranged the code to not to sleep with rcu_lock while using xas_() functionality.
> >   -- Addressed the comments from Suren.
> >   -- https://patchwork.kernel.org/project/linux-mm/patch/1638442253-1591-1-git-send-email-quic_charante@quicinc.com/
> >
> > changes in V1:
> >   -- Created the interface for fadvise(2) to work on shmem files.
> >   -- https://patchwork.kernel.org/project/linux-mm/patch/1633701982-22302-1-git-send-email-charante@codeaurora.org/
> >
> >
> > Charan Teja Kalla (2):
> >   mm: fadvise: move 'endbyte' calculations to helper function
> >   mm: shmem: implement POSIX_FADV_[WILL|DONT]NEED for shmem
> >
> >  mm/fadvise.c  |  11 +-----
> >  mm/internal.h |  21 +++++++++++
> >  mm/shmem.c    | 116 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 138 insertions(+), 10 deletions(-)
> >
> > --
> > 2.7.4
> >
> >
>
> I didn't see this patch before, so I looked a bit at the history. At
> some point, in v3, dealing with mapped pages for DONTNEED was left
> out, they are now skipped. Unfortunately, that makes this patch no
> longer usable for a case that we have: restoring the (approximate)
> swap state of a tmpfs file. This involves walking a potentially large
> number of regions, and explicitly pushing them out to swap. This can
> be used to e.g. restore the state VM memory that is backed by a tmpfs
> file, avoiding memory usage by cold VM pages after resume.
>
> If DONTNEED also reclaims mapped pages (e.g. they get pushed out to
> swap, if any), implementing the above use case efficiently is simple:
> use io_uring with a vector that contains each region and the fadvise
> method.
>
> Without DONTNEED reclaiming mapped pages, you'd have to do mmap +
> madvise(MADV_PAGEOUT) for each region that you want swapped out, which
> is rather inefficient.
>
> I understand that the semantics for POSIX_FADV_DONTNEED on shmem/tmpfs
> files are open to interpretation, as it is a special case. And you can
> certainly make the argument that relying on behavior caused by what
> can be considered an implementation detail is bad.
>
> So, is there any way we can make this use case work efficiently using
> this patch?
>
> You state in the commit message:
>
> > So, FADV_DONTNEED also covers the semantics of MADV_PAGEOUT for file pages
> > and there is no purpose of PAGEOUT for file pages.
>
> But that doesn't seem correct: for shmem file pages, there actually
> can be a purpose, and the FADV_DONTNEED as implemented for shmem in
> this patch set does not cover the semantics.
>
> You can say that it doesn't need to cover the pageout case of mapped
> shmem pages, and that's fair. But I don't think you can claim that it
> covers the case as currently implemented.
>
> I suppose there are three options here:
>
> 1) Do nothing, this use case will just have to spend more time doing
> mmap+madvise
> 2) Don't skip mapped pages for POSIX_FADV_DONTNEED in shmem_fadvise
> 3) Implement something like POSIX_FADV_PAGEOUT_NP, which would include
> mapped pages.
>
> What do people think?
>
> - Frank

Hmm, actually, looking at it a bit more, there are several issues
here. One is that with fadvise, you can't be sure if you are the only
one dealing with the page in a mapped way(with madvise, if mapcount ==
1, that mean's it's just you, but you don't know that for fadvise, so
that makes correctly dealing with mapped pages harder).

Also, the madvise loop issue can probably also be dealt with via io_uring.

So, I think we can deal with the use case I mentioned in a different
way, that is mostly unrelated to this patchset, so basically:
disregard my previous email.

- Frank

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

[Charan Teja Kalla]

Thanks Frank!!

On 4/14/2023 11:14 PM, Frank van der Linden wrote:
>> I didn't see this patch before, so I looked a bit at the history. At
>> some point, in v3, dealing with mapped pages for DONTNEED was left
>> out, they are now skipped. Unfortunately, that makes this patch no
>> longer usable for a case that we have: restoring the (approximate)
>> swap state of a tmpfs file. This involves walking a potentially large
>> number of regions, and explicitly pushing them out to swap. This can
>> be used to e.g. restore the state VM memory that is backed by a tmpfs
>> file, avoiding memory usage by cold VM pages after resume.
>>

This is an interesting use case and I feel this really supports this
patchset. IIUC, supporting the reclaim of mapped file pages through
fadvise() helps this usecase where you can avoid traversing the large
number of vma regions as you can directly issue the fadvise() on the
shmem file 'fd' and it takes care. Am I correct?

> Hmm, actually, looking at it a bit more, there are several issues
> here. One is that with fadvise, you can't be sure if you are the only
> one dealing with the page in a mapped way(with madvise, if mapcount ==
> 1, that mean's it's just you, but you don't know that for fadvise, so
> that makes correctly dealing with mapped pages harder).
> 
Sorry, Why not for fadvise()? I can still attempt only if the page is
mapped and its mapcount == 1, but then we already have madvise() for
such pages and why not we simply use it.

> Also, the madvise loop issue can probably also be dealt with via io_uring.
> 
> So, I think we can deal with the use case I mentioned in a different
> way, that is mostly unrelated to this patchset, so basically:
> disregard my previous email.
Sure! If this patch looks good for you, Reviewed/Ack tags can help me..

Thanks,
Charan

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

[Frank van der Linden]

On Fri, Apr 14, 2023 at 12:10 PM Charan Teja Kalla
<quic_charante@quicinc.com> wrote:
>
> Thanks Frank!!
>
> On 4/14/2023 11:14 PM, Frank van der Linden wrote:
> >> I didn't see this patch before, so I looked a bit at the history. At
> >> some point, in v3, dealing with mapped pages for DONTNEED was left
> >> out, they are now skipped. Unfortunately, that makes this patch no
> >> longer usable for a case that we have: restoring the (approximate)
> >> swap state of a tmpfs file. This involves walking a potentially large
> >> number of regions, and explicitly pushing them out to swap. This can
> >> be used to e.g. restore the state VM memory that is backed by a tmpfs
> >> file, avoiding memory usage by cold VM pages after resume.
> >>
>
> This is an interesting use case and I feel this really supports this
> patchset. IIUC, supporting the reclaim of mapped file pages through
> fadvise() helps this usecase where you can avoid traversing the large
> number of vma regions as you can directly issue the fadvise() on the
> shmem file 'fd' and it takes care. Am I correct?

Right, that's correct. The only snag here is that fadvise, with your
patch set, will skip mapped pages, which might be an issue for this
case.

>
> > Hmm, actually, looking at it a bit more, there are several issues
> > here. One is that with fadvise, you can't be sure if you are the only
> > one dealing with the page in a mapped way(with madvise, if mapcount ==
> > 1, that mean's it's just you, but you don't know that for fadvise, so
> > that makes correctly dealing with mapped pages harder).
> >
> Sorry, Why not for fadvise()? I can still attempt only if the page is
> mapped and its mapcount == 1, but then we already have madvise() for
> such pages and why not we simply use it.

Yes, you could use madvise (as I was thinking). One issue with that
is, though, that madvise(PAGEOUT) is based on a page table walk of
present PTEs. So you actually need to map and touch the page before
madvise will work, which is suboptimal. A direct fadvise solution
would be nicer, since that does a file mapping walk. However, that can
be addressed later, my comments weren't intended to raise an objection
- merely that there is a chance here to address this usecase. But that
shouldn't block anything. It's something to keep in mind, though.

I'll do some experiments to see what the best solution is here. But,
any follow-ups would be on top of your patch, so I'm not raising
objections, merely hoping that this, and possible extra work, could
solve this case.


- Frank

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

[Charan Teja Kalla]

On 4/15/2023 3:32 AM, Frank van der Linden wrote:
> However, that can
> be addressed later, my comments weren't intended to raise an objection
> - merely that there is a chance here to address this usecase. But that
> shouldn't block anything. It's something to keep in mind, though.
>
Noted. Thanks!!
> I'll do some experiments to see what the best solution is here
Sure. We can work and add the support for mapped pages too which will go
on top of these patches.

Thanks,
Charan

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

[Frank van der Linden]

Below is a quick patch to allow FADVISE_DONTNEED for shmem to reclaim
mapped pages too. This would fit our usecase, and matches MADV_PAGEOUT
more closely.

The patch series as posted skips mapped pages even if you remove
the folio_mapped() check, because page_referenced() in
shrink_page_list() will find page tables with the page mapped,
and ignore_references is not set when called from reclaim_pages().

You can make this work in a similar fashion to MADV_PAGEOUT by
first unmapping a page, but only if the mapping belongs to
the caller. You just have to change the check for "is there
only one mapping and am I the owner". To do that, change a few
lines in try_to_unmap to allow for checking the mm the mapping
belongs to, and pass in current->mm (NULL still unmaps all mappings).

I lightly tested this in a somewhat older codebase, so the diff
below isn't fully tested. But if there are no objections to
this approach, we could add it on top of your patchset after
better testing.

- Frank

diff --git a/include/linux/rmap.h b/include/linux/rmap.h
index b87d01660412..4403cc2ccc4c 100644
--- a/include/linux/rmap.h
+++ b/include/linux/rmap.h
@@ -368,6 +368,8 @@ int folio_referenced(struct folio *, int is_locked,
 
 void try_to_migrate(struct folio *folio, enum ttu_flags flags);
 void try_to_unmap(struct folio *, enum ttu_flags flags);
+void try_to_unmap_mm(struct mm_struct *mm, struct folio *folio,
+			enum ttu_flags flags);
 
 int make_device_exclusive_range(struct mm_struct *mm, unsigned long start,
 				unsigned long end, struct page **pages,
diff --git a/mm/rmap.c b/mm/rmap.c
index 8632e02661ac..4d30e8f5afe2 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1443,6 +1443,11 @@ void page_remove_rmap(struct page *page, struct vm_area_struct *vma,
 	munlock_vma_folio(folio, vma, compound);
 }
 
+struct unmap_arg {
+	enum ttu_flags flags;
+	struct mm_struct *mm;
+};
+
 /*
  * @arg: enum ttu_flags will be passed to this argument
  */
@@ -1455,7 +1460,11 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
 	struct page *subpage;
 	bool anon_exclusive, ret = true;
 	struct mmu_notifier_range range;
-	enum ttu_flags flags = (enum ttu_flags)(long)arg;
+	struct unmap_arg *uap = (struct unmap_arg *)arg;
+	enum ttu_flags flags = uap->flags;
+
+	if (uap->mm && uap->mm != mm)
+		return true;
 
 	/*
 	 * When racing against e.g. zap_pte_range() on another cpu,
@@ -1776,6 +1785,7 @@ static int folio_not_mapped(struct folio *folio)
 
 /**
  * try_to_unmap - Try to remove all page table mappings to a folio.
+ * @mm: mm to unmap from (NULL to unmap from all)
  * @folio: The folio to unmap.
  * @flags: action and flags
  *
@@ -1785,11 +1795,16 @@ static int folio_not_mapped(struct folio *folio)
  *
  * Context: Caller must hold the folio lock.
  */
-void try_to_unmap(struct folio *folio, enum ttu_flags flags)
+void try_to_unmap_mm(struct mm_struct *mm, struct folio *folio,
+		enum ttu_flags flags)
 {
+	struct unmap_arg ua = {
+		.flags = flags,
+		.mm = mm,
+	};
 	struct rmap_walk_control rwc = {
 		.rmap_one = try_to_unmap_one,
-		.arg = (void *)flags,
+		.arg = (void *)&ua,
 		.done = folio_not_mapped,
 		.anon_lock = folio_lock_anon_vma_read,
 	};
@@ -1800,6 +1815,11 @@ void try_to_unmap(struct folio *folio, enum ttu_flags flags)
 		rmap_walk(folio, &rwc);
 }
 
+void try_to_unmap(struct folio *folio, enum ttu_flags flags)
+{
+	try_to_unmap_mm(NULL, folio, flags);
+}
+
 /*
  * @arg: enum ttu_flags will be passed to this argument.
  *
diff --git a/mm/shmem.c b/mm/shmem.c
index 1af85259b6fc..b24af2fb3378 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -2362,8 +2362,24 @@ static void shmem_isolate_pages_range(struct address_space *mapping, loff_t star
 
 		if (!folio_try_get(folio))
 			continue;
-		if (folio_test_unevictable(folio) || folio_mapped(folio) ||
-				folio_isolate_lru(folio)) {
+
+		if (folio_test_unevictable(folio)) {
+			folio_put(folio);
+			continue;
+		}
+
+		/*
+		 * If the folio is mapped once, try to unmap it from the
+		 * caller's page table. If it's still mapped afterwards,
+		 * it belongs to someone else, and we're not going to
+		 * change someone else's mapping.
+		 */
+		if (folio_mapcount(folio) == 1 && folio_trylock(folio)) {
+			try_to_unmap_mm(current->mm, folio, TTU_BATCH_FLUSH);
+			folio_unlock(folio);
+		}
+
+		if (folio_mapped(folio) || folio_isolate_lru(folio)) {
 			folio_put(folio);
 			continue;
 		}
@@ -2383,6 +2399,8 @@ static void shmem_isolate_pages_range(struct address_space *mapping, loff_t star
 		}
 	}
 	rcu_read_unlock();
+
+	try_to_unmap_flush();
 }
 
 static int shmem_fadvise_dontneed(struct address_space *mapping, loff_t start,

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

[Pavan Kondeti]

On Tue, Apr 18, 2023 at 05:29:42PM +0000, Frank van der Linden wrote:
> Below is a quick patch to allow FADVISE_DONTNEED for shmem to reclaim
> mapped pages too. This would fit our usecase, and matches MADV_PAGEOUT
> more closely.
> 
> The patch series as posted skips mapped pages even if you remove
> the folio_mapped() check, because page_referenced() in
> shrink_page_list() will find page tables with the page mapped,
> and ignore_references is not set when called from reclaim_pages().
> 
> You can make this work in a similar fashion to MADV_PAGEOUT by
> first unmapping a page, but only if the mapping belongs to
> the caller. You just have to change the check for "is there
> only one mapping and am I the owner". To do that, change a few
> lines in try_to_unmap to allow for checking the mm the mapping
> belongs to, and pass in current->mm (NULL still unmaps all mappings).
> 
> I lightly tested this in a somewhat older codebase, so the diff
> below isn't fully tested. But if there are no objections to
> this approach, we could add it on top of your patchset after
> better testing.
> 
> - Frank
> 
> diff --git a/include/linux/rmap.h b/include/linux/rmap.h
> index b87d01660412..4403cc2ccc4c 100644
> --- a/include/linux/rmap.h
> +++ b/include/linux/rmap.h
> @@ -368,6 +368,8 @@ int folio_referenced(struct folio *, int is_locked,
>  
>  void try_to_migrate(struct folio *folio, enum ttu_flags flags);
>  void try_to_unmap(struct folio *, enum ttu_flags flags);
> +void try_to_unmap_mm(struct mm_struct *mm, struct folio *folio,
> +			enum ttu_flags flags);
>  
>  int make_device_exclusive_range(struct mm_struct *mm, unsigned long start,
>  				unsigned long end, struct page **pages,
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 8632e02661ac..4d30e8f5afe2 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1443,6 +1443,11 @@ void page_remove_rmap(struct page *page, struct vm_area_struct *vma,
>  	munlock_vma_folio(folio, vma, compound);
>  }
>  
> +struct unmap_arg {
> +	enum ttu_flags flags;
> +	struct mm_struct *mm;
> +};
> +
>  /*
>   * @arg: enum ttu_flags will be passed to this argument
>   */
> @@ -1455,7 +1460,11 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
>  	struct page *subpage;
>  	bool anon_exclusive, ret = true;
>  	struct mmu_notifier_range range;
> -	enum ttu_flags flags = (enum ttu_flags)(long)arg;
> +	struct unmap_arg *uap = (struct unmap_arg *)arg;
> +	enum ttu_flags flags = uap->flags;
> +
> +	if (uap->mm && uap->mm != mm)
> +		return true;
>  
>  	/*
>  	 * When racing against e.g. zap_pte_range() on another cpu,
> @@ -1776,6 +1785,7 @@ static int folio_not_mapped(struct folio *folio)
>  
>  /**
>   * try_to_unmap - Try to remove all page table mappings to a folio.
> + * @mm: mm to unmap from (NULL to unmap from all)
>   * @folio: The folio to unmap.
>   * @flags: action and flags
>   *
> @@ -1785,11 +1795,16 @@ static int folio_not_mapped(struct folio *folio)
>   *
>   * Context: Caller must hold the folio lock.
>   */
> -void try_to_unmap(struct folio *folio, enum ttu_flags flags)
> +void try_to_unmap_mm(struct mm_struct *mm, struct folio *folio,
> +		enum ttu_flags flags)
>  {
> +	struct unmap_arg ua = {
> +		.flags = flags,
> +		.mm = mm,
> +	};
>  	struct rmap_walk_control rwc = {
>  		.rmap_one = try_to_unmap_one,
> -		.arg = (void *)flags,
> +		.arg = (void *)&ua,
>  		.done = folio_not_mapped,
>  		.anon_lock = folio_lock_anon_vma_read,
>  	};
> @@ -1800,6 +1815,11 @@ void try_to_unmap(struct folio *folio, enum ttu_flags flags)
>  		rmap_walk(folio, &rwc);
>  }
>  
> +void try_to_unmap(struct folio *folio, enum ttu_flags flags)
> +{
> +	try_to_unmap_mm(NULL, folio, flags);
> +}
> +
>  /*
>   * @arg: enum ttu_flags will be passed to this argument.
>   *
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 1af85259b6fc..b24af2fb3378 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -2362,8 +2362,24 @@ static void shmem_isolate_pages_range(struct address_space *mapping, loff_t star
>  
>  		if (!folio_try_get(folio))
>  			continue;
> -		if (folio_test_unevictable(folio) || folio_mapped(folio) ||
> -				folio_isolate_lru(folio)) {
> +
> +		if (folio_test_unevictable(folio)) {
> +			folio_put(folio);
> +			continue;
> +		}
> +
> +		/*
> +		 * If the folio is mapped once, try to unmap it from the
> +		 * caller's page table. If it's still mapped afterwards,
> +		 * it belongs to someone else, and we're not going to
> +		 * change someone else's mapping.
> +		 */
> +		if (folio_mapcount(folio) == 1 && folio_trylock(folio)) {
> +			try_to_unmap_mm(current->mm, folio, TTU_BATCH_FLUSH);
> +			folio_unlock(folio);
> +		}

Is rmap walk can be done from a RCU read critical section which does not
allow explicit blocking? 

Thanks,
Pavan

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

[Charan Teja Kalla]

Hi Frank,

Lets start a separate thread to add the support for mapped pages. I
think one question that can come while review is: "what is the overhead
an application has in collecting the memory regions and issuing the
MADV_PAGEOUT, that made to add this support?". Let me try to get details
for this from my side too.

BTW, thanks for this POC code!!

Thanks,
Charan

On 4/18/2023 10:59 PM, Frank van der Linden wrote:
> Below is a quick patch to allow FADVISE_DONTNEED for shmem to reclaim
> mapped pages too. This would fit our usecase, and matches MADV_PAGEOUT
> more closely.
> 
> The patch series as posted skips mapped pages even if you remove
> the folio_mapped() check, because page_referenced() in
> shrink_page_list() will find page tables with the page mapped,
> and ignore_references is not set when called from reclaim_pages().
> 
> You can make this work in a similar fashion to MADV_PAGEOUT by
> first unmapping a page, but only if the mapping belongs to
> the caller. You just have to change the check for "is there
> only one mapping and am I the owner". To do that, change a few
> lines in try_to_unmap to allow for checking the mm the mapping
> belongs to, and pass in current->mm (NULL still unmaps all mappings).
> 
> I lightly tested this in a somewhat older codebase, so the diff
> below isn't fully tested. But if there are no objections to
> this approach, we could add it on top of your patchset after
> better testing.
> 
> - Frank
> 
> diff --git a/include/linux/rmap.h b/include/linux/rmap.h
> index b87d01660412..4403cc2ccc4c 100644
> --- a/include/linux/rmap.h
> +++ b/include/linux/rmap.h
> @@ -368,6 +368,8 @@ int folio_referenced(struct folio *, int is_locked,
>  
>  void try_to_migrate(struct folio *folio, enum ttu_flags flags);
>  void try_to_unmap(struct folio *, enum ttu_flags flags);
> +void try_to_unmap_mm(struct mm_struct *mm, struct folio *folio,
> +			enum ttu_flags flags);
>  
>  int make_device_exclusive_range(struct mm_struct *mm, unsigned long start,
>  				unsigned long end, struct page **pages,
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 8632e02661ac..4d30e8f5afe2 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1443,6 +1443,11 @@ void page_remove_rmap(struct page *page, struct vm_area_struct *vma,
>  	munlock_vma_folio(folio, vma, compound);
>  }
>  
> +struct unmap_arg {
> +	enum ttu_flags flags;
> +	struct mm_struct *mm;
> +};
> +
>  /*
>   * @arg: enum ttu_flags will be passed to this argument
>   */
> @@ -1455,7 +1460,11 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
>  	struct page *subpage;
>  	bool anon_exclusive, ret = true;
>  	struct mmu_notifier_range range;
> -	enum ttu_flags flags = (enum ttu_flags)(long)arg;
> +	struct unmap_arg *uap = (struct unmap_arg *)arg;
> +	enum ttu_flags flags = uap->flags;
> +
> +	if (uap->mm && uap->mm != mm)
> +		return true;
>  
>  	/*
>  	 * When racing against e.g. zap_pte_range() on another cpu,
> @@ -1776,6 +1785,7 @@ static int folio_not_mapped(struct folio *folio)
>  
>  /**
>   * try_to_unmap - Try to remove all page table mappings to a folio.
> + * @mm: mm to unmap from (NULL to unmap from all)
>   * @folio: The folio to unmap.
>   * @flags: action and flags
>   *
> @@ -1785,11 +1795,16 @@ static int folio_not_mapped(struct folio *folio)
>   *
>   * Context: Caller must hold the folio lock.
>   */
> -void try_to_unmap(struct folio *folio, enum ttu_flags flags)
> +void try_to_unmap_mm(struct mm_struct *mm, struct folio *folio,
> +		enum ttu_flags flags)
>  {
> +	struct unmap_arg ua = {
> +		.flags = flags,
> +		.mm = mm,
> +	};
>  	struct rmap_walk_control rwc = {
>  		.rmap_one = try_to_unmap_one,
> -		.arg = (void *)flags,
> +		.arg = (void *)&ua,
>  		.done = folio_not_mapped,
>  		.anon_lock = folio_lock_anon_vma_read,
>  	};
> @@ -1800,6 +1815,11 @@ void try_to_unmap(struct folio *folio, enum ttu_flags flags)
>  		rmap_walk(folio, &rwc);
>  }
>  
> +void try_to_unmap(struct folio *folio, enum ttu_flags flags)
> +{
> +	try_to_unmap_mm(NULL, folio, flags);
> +}
> +
>  /*
>   * @arg: enum ttu_flags will be passed to this argument.
>   *
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 1af85259b6fc..b24af2fb3378 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -2362,8 +2362,24 @@ static void shmem_isolate_pages_range(struct address_space *mapping, loff_t star
>  
>  		if (!folio_try_get(folio))
>  			continue;
> -		if (folio_test_unevictable(folio) || folio_mapped(folio) ||
> -				folio_isolate_lru(folio)) {
> +
> +		if (folio_test_unevictable(folio)) {
> +			folio_put(folio);
> +			continue;
> +		}
> +
> +		/*
> +		 * If the folio is mapped once, try to unmap it from the
> +		 * caller's page table. If it's still mapped afterwards,
> +		 * it belongs to someone else, and we're not going to
> +		 * change someone else's mapping.
> +		 */
> +		if (folio_mapcount(folio) == 1 && folio_trylock(folio)) {
> +			try_to_unmap_mm(current->mm, folio, TTU_BATCH_FLUSH);
> +			folio_unlock(folio);
> +		}
> +
> +		if (folio_mapped(folio) || folio_isolate_lru(folio)) {
>  			folio_put(folio);
>  			continue;
>  		}
> @@ -2383,6 +2399,8 @@ static void shmem_isolate_pages_range(struct address_space *mapping, loff_t star
>  		}
>  	}
>  	rcu_read_unlock();
> +
> +	try_to_unmap_flush();
>  }
>  
>  static int shmem_fadvise_dontneed(struct address_space *mapping, loff_t start,

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

[Frank van der Linden]

On Tue, Apr 18, 2023 at 9:19 PM Pavan Kondeti <quic_pkondeti@quicinc.com> wrote:
>
> On Tue, Apr 18, 2023 at 05:29:42PM +0000, Frank van der Linden wrote:
> > Below is a quick patch to allow FADVISE_DONTNEED for shmem to reclaim
> > mapped pages too. This would fit our usecase, and matches MADV_PAGEOUT
> > more closely.
> >
> > The patch series as posted skips mapped pages even if you remove
> > the folio_mapped() check, because page_referenced() in
> > shrink_page_list() will find page tables with the page mapped,
> > and ignore_references is not set when called from reclaim_pages().
> >
> > You can make this work in a similar fashion to MADV_PAGEOUT by
> > first unmapping a page, but only if the mapping belongs to
> > the caller. You just have to change the check for "is there
> > only one mapping and am I the owner". To do that, change a few
> > lines in try_to_unmap to allow for checking the mm the mapping
> > belongs to, and pass in current->mm (NULL still unmaps all mappings).
> >
> > I lightly tested this in a somewhat older codebase, so the diff
> > below isn't fully tested. But if there are no objections to
> > this approach, we could add it on top of your patchset after
> > better testing.
> >
> > - Frank
> >
> > diff --git a/include/linux/rmap.h b/include/linux/rmap.h
> > index b87d01660412..4403cc2ccc4c 100644
> > --- a/include/linux/rmap.h
> > +++ b/include/linux/rmap.h
> > @@ -368,6 +368,8 @@ int folio_referenced(struct folio *, int is_locked,
> >
> >  void try_to_migrate(struct folio *folio, enum ttu_flags flags);
> >  void try_to_unmap(struct folio *, enum ttu_flags flags);
> > +void try_to_unmap_mm(struct mm_struct *mm, struct folio *folio,
> > +                     enum ttu_flags flags);
> >
> >  int make_device_exclusive_range(struct mm_struct *mm, unsigned long start,
> >                               unsigned long end, struct page **pages,
> > diff --git a/mm/rmap.c b/mm/rmap.c
> > index 8632e02661ac..4d30e8f5afe2 100644
> > --- a/mm/rmap.c
> > +++ b/mm/rmap.c
> > @@ -1443,6 +1443,11 @@ void page_remove_rmap(struct page *page, struct vm_area_struct *vma,
> >       munlock_vma_folio(folio, vma, compound);
> >  }
> >
> > +struct unmap_arg {
> > +     enum ttu_flags flags;
> > +     struct mm_struct *mm;
> > +};
> > +
> >  /*
> >   * @arg: enum ttu_flags will be passed to this argument
> >   */
> > @@ -1455,7 +1460,11 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
> >       struct page *subpage;
> >       bool anon_exclusive, ret = true;
> >       struct mmu_notifier_range range;
> > -     enum ttu_flags flags = (enum ttu_flags)(long)arg;
> > +     struct unmap_arg *uap = (struct unmap_arg *)arg;
> > +     enum ttu_flags flags = uap->flags;
> > +
> > +     if (uap->mm && uap->mm != mm)
> > +             return true;
> >
> >       /*
> >        * When racing against e.g. zap_pte_range() on another cpu,
> > @@ -1776,6 +1785,7 @@ static int folio_not_mapped(struct folio *folio)
> >
> >  /**
> >   * try_to_unmap - Try to remove all page table mappings to a folio.
> > + * @mm: mm to unmap from (NULL to unmap from all)
> >   * @folio: The folio to unmap.
> >   * @flags: action and flags
> >   *
> > @@ -1785,11 +1795,16 @@ static int folio_not_mapped(struct folio *folio)
> >   *
> >   * Context: Caller must hold the folio lock.
> >   */
> > -void try_to_unmap(struct folio *folio, enum ttu_flags flags)
> > +void try_to_unmap_mm(struct mm_struct *mm, struct folio *folio,
> > +             enum ttu_flags flags)
> >  {
> > +     struct unmap_arg ua = {
> > +             .flags = flags,
> > +             .mm = mm,
> > +     };
> >       struct rmap_walk_control rwc = {
> >               .rmap_one = try_to_unmap_one,
> > -             .arg = (void *)flags,
> > +             .arg = (void *)&ua,
> >               .done = folio_not_mapped,
> >               .anon_lock = folio_lock_anon_vma_read,
> >       };
> > @@ -1800,6 +1815,11 @@ void try_to_unmap(struct folio *folio, enum ttu_flags flags)
> >               rmap_walk(folio, &rwc);
> >  }
> >
> > +void try_to_unmap(struct folio *folio, enum ttu_flags flags)
> > +{
> > +     try_to_unmap_mm(NULL, folio, flags);
> > +}
> > +
> >  /*
> >   * @arg: enum ttu_flags will be passed to this argument.
> >   *
> > diff --git a/mm/shmem.c b/mm/shmem.c
> > index 1af85259b6fc..b24af2fb3378 100644
> > --- a/mm/shmem.c
> > +++ b/mm/shmem.c
> > @@ -2362,8 +2362,24 @@ static void shmem_isolate_pages_range(struct address_space *mapping, loff_t star
> >
> >               if (!folio_try_get(folio))
> >                       continue;
> > -             if (folio_test_unevictable(folio) || folio_mapped(folio) ||
> > -                             folio_isolate_lru(folio)) {
> > +
> > +             if (folio_test_unevictable(folio)) {
> > +                     folio_put(folio);
> > +                     continue;
> > +             }
> > +
> > +             /*
> > +              * If the folio is mapped once, try to unmap it from the
> > +              * caller's page table. If it's still mapped afterwards,
> > +              * it belongs to someone else, and we're not going to
> > +              * change someone else's mapping.
> > +              */
> > +             if (folio_mapcount(folio) == 1 && folio_trylock(folio)) {
> > +                     try_to_unmap_mm(current->mm, folio, TTU_BATCH_FLUSH);
> > +                     folio_unlock(folio);
> > +             }
>
> Is rmap walk can be done from a RCU read critical section which does not
> allow explicit blocking?
>
> Thanks,
> Pavan

True, yes, rmap_walk may block, so it the try_to_unmap calls should be
outside the loop. The easiest thing to do there is to add all mapped
pages to a separate list, walk that list outside of the rcu lock for
i_mapping, and add all pages that could be unmapped to the return
list.

- Frank

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

[Frank van der Linden]

Sure, we can add a separate thread. I can send a more formal version
of my quick diff (one that has actually been tested more thoroughly)
to kick it off.

Basically, using madvise() would work, but then I realized you'd have
to first explicitly map all regions and touch them to get them in your
page table, after which you then call MADV_PAGEOUT to get rid of them
again. Which is just .. silly :)

- Frank

On Wed, Apr 19, 2023 at 7:39 AM Charan Teja Kalla
<quic_charante@quicinc.com> wrote:
>
> Hi Frank,
>
> Lets start a separate thread to add the support for mapped pages. I
> think one question that can come while review is: "what is the overhead
> an application has in collecting the memory regions and issuing the
> MADV_PAGEOUT, that made to add this support?". Let me try to get details
> for this from my side too.
>
> BTW, thanks for this POC code!!
>
> Thanks,
> Charan
>
> On 4/18/2023 10:59 PM, Frank van der Linden wrote:
> > Below is a quick patch to allow FADVISE_DONTNEED for shmem to reclaim
> > mapped pages too. This would fit our usecase, and matches MADV_PAGEOUT
> > more closely.
> >
> > The patch series as posted skips mapped pages even if you remove
> > the folio_mapped() check, because page_referenced() in
> > shrink_page_list() will find page tables with the page mapped,
> > and ignore_references is not set when called from reclaim_pages().
> >
> > You can make this work in a similar fashion to MADV_PAGEOUT by
> > first unmapping a page, but only if the mapping belongs to
> > the caller. You just have to change the check for "is there
> > only one mapping and am I the owner". To do that, change a few
> > lines in try_to_unmap to allow for checking the mm the mapping
> > belongs to, and pass in current->mm (NULL still unmaps all mappings).
> >
> > I lightly tested this in a somewhat older codebase, so the diff
> > below isn't fully tested. But if there are no objections to
> > this approach, we could add it on top of your patchset after
> > better testing.
> >
> > - Frank
> >
> > diff --git a/include/linux/rmap.h b/include/linux/rmap.h
> > index b87d01660412..4403cc2ccc4c 100644
> > --- a/include/linux/rmap.h
> > +++ b/include/linux/rmap.h
> > @@ -368,6 +368,8 @@ int folio_referenced(struct folio *, int is_locked,
> >
> >  void try_to_migrate(struct folio *folio, enum ttu_flags flags);
> >  void try_to_unmap(struct folio *, enum ttu_flags flags);
> > +void try_to_unmap_mm(struct mm_struct *mm, struct folio *folio,
> > +                     enum ttu_flags flags);
> >
> >  int make_device_exclusive_range(struct mm_struct *mm, unsigned long start,
> >                               unsigned long end, struct page **pages,
> > diff --git a/mm/rmap.c b/mm/rmap.c
> > index 8632e02661ac..4d30e8f5afe2 100644
> > --- a/mm/rmap.c
> > +++ b/mm/rmap.c
> > @@ -1443,6 +1443,11 @@ void page_remove_rmap(struct page *page, struct vm_area_struct *vma,
> >       munlock_vma_folio(folio, vma, compound);
> >  }
> >
> > +struct unmap_arg {
> > +     enum ttu_flags flags;
> > +     struct mm_struct *mm;
> > +};
> > +
> >  /*
> >   * @arg: enum ttu_flags will be passed to this argument
> >   */
> > @@ -1455,7 +1460,11 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
> >       struct page *subpage;
> >       bool anon_exclusive, ret = true;
> >       struct mmu_notifier_range range;
> > -     enum ttu_flags flags = (enum ttu_flags)(long)arg;
> > +     struct unmap_arg *uap = (struct unmap_arg *)arg;
> > +     enum ttu_flags flags = uap->flags;
> > +
> > +     if (uap->mm && uap->mm != mm)
> > +             return true;
> >
> >       /*
> >        * When racing against e.g. zap_pte_range() on another cpu,
> > @@ -1776,6 +1785,7 @@ static int folio_not_mapped(struct folio *folio)
> >
> >  /**
> >   * try_to_unmap - Try to remove all page table mappings to a folio.
> > + * @mm: mm to unmap from (NULL to unmap from all)
> >   * @folio: The folio to unmap.
> >   * @flags: action and flags
> >   *
> > @@ -1785,11 +1795,16 @@ static int folio_not_mapped(struct folio *folio)
> >   *
> >   * Context: Caller must hold the folio lock.
> >   */
> > -void try_to_unmap(struct folio *folio, enum ttu_flags flags)
> > +void try_to_unmap_mm(struct mm_struct *mm, struct folio *folio,
> > +             enum ttu_flags flags)
> >  {
> > +     struct unmap_arg ua = {
> > +             .flags = flags,
> > +             .mm = mm,
> > +     };
> >       struct rmap_walk_control rwc = {
> >               .rmap_one = try_to_unmap_one,
> > -             .arg = (void *)flags,
> > +             .arg = (void *)&ua,
> >               .done = folio_not_mapped,
> >               .anon_lock = folio_lock_anon_vma_read,
> >       };
> > @@ -1800,6 +1815,11 @@ void try_to_unmap(struct folio *folio, enum ttu_flags flags)
> >               rmap_walk(folio, &rwc);
> >  }
> >
> > +void try_to_unmap(struct folio *folio, enum ttu_flags flags)
> > +{
> > +     try_to_unmap_mm(NULL, folio, flags);
> > +}
> > +
> >  /*
> >   * @arg: enum ttu_flags will be passed to this argument.
> >   *
> > diff --git a/mm/shmem.c b/mm/shmem.c
> > index 1af85259b6fc..b24af2fb3378 100644
> > --- a/mm/shmem.c
> > +++ b/mm/shmem.c
> > @@ -2362,8 +2362,24 @@ static void shmem_isolate_pages_range(struct address_space *mapping, loff_t star
> >
> >               if (!folio_try_get(folio))
> >                       continue;
> > -             if (folio_test_unevictable(folio) || folio_mapped(folio) ||
> > -                             folio_isolate_lru(folio)) {
> > +
> > +             if (folio_test_unevictable(folio)) {
> > +                     folio_put(folio);
> > +                     continue;
> > +             }
> > +
> > +             /*
> > +              * If the folio is mapped once, try to unmap it from the
> > +              * caller's page table. If it's still mapped afterwards,
> > +              * it belongs to someone else, and we're not going to
> > +              * change someone else's mapping.
> > +              */
> > +             if (folio_mapcount(folio) == 1 && folio_trylock(folio)) {
> > +                     try_to_unmap_mm(current->mm, folio, TTU_BATCH_FLUSH);
> > +                     folio_unlock(folio);
> > +             }
> > +
> > +             if (folio_mapped(folio) || folio_isolate_lru(folio)) {
> >                       folio_put(folio);
> >                       continue;
> >               }
> > @@ -2383,6 +2399,8 @@ static void shmem_isolate_pages_range(struct address_space *mapping, loff_t star
> >               }
> >       }
> >       rcu_read_unlock();
> > +
> > +     try_to_unmap_flush();
> >  }
> >
> >  static int shmem_fadvise_dontneed(struct address_space *mapping, loff_t start,

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

[Hugh Dickins]

On Tue, 14 Feb 2023, 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@quicinc.com>

I'm sorry, but no, this is not yet ready for primetime. I came here
expecting to be able just to add a patch on top with small fixes,
but see today that it needs more than that, and my time has run out.

Though if Andrew is keen to go ahead with it in 6.4, and add fixes
on top while it's in rc, that will be okay: except for one small bad
bug, which must be fixed immediately - "luckily" nobody appears to
be using or testing this since v5, but it cannot go further as is.

Willneed is probably fine, but dontneed is not.

> ---
>  mm/shmem.c | 116 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 116 insertions(+)
> 
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 448f393..1af8525 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -40,6 +40,9 @@
>  #include <linux/fs_parser.h>
>  #include <linux/swapfile.h>
>  #include <linux/iversion.h>
> +#include <linux/mm_inline.h>
> +#include <linux/fadvise.h>
> +#include <linux/page_idle.h>
>  #include "swap.h"
>  
>  static struct vfsmount *shm_mnt;
> @@ -2344,6 +2347,118 @@ static void shmem_set_inode_flags(struct inode *inode, unsigned int fsflags)
>  #define shmem_initxattrs NULL
>  #endif
>  
> +static void shmem_isolate_pages_range(struct address_space *mapping, loff_t start,
> +				loff_t end, struct list_head *list)

loff_t? They are pgoff_t.

> +{
> +	XA_STATE(xas, &mapping->i_pages, start);
> +	struct folio *folio;
> +
> +	rcu_read_lock();
> +	xas_for_each(&xas, folio, end) {
> +		if (xas_retry(&xas, folio))
> +			continue;
> +		if (xa_is_value(folio))
> +			continue;
> +
> +		if (!folio_try_get(folio))
> +			continue;
> +		if (folio_test_unevictable(folio) || folio_mapped(folio) ||
> +				folio_isolate_lru(folio)) {

There is the one small bad bug.  That should say !folio_isolate_lru(folio).
In v5, it was isolate_lru_page(page), because isolate_lru_page() returned
0 for success or -EBUSY for unavailable; whereas folio_isolate_lru(folio)
is a boolean, returning true if it successfully removed folio from LRU.

The effect of that bug is that in v6 and v7, it has skipped all the folios
it was expected to be reclaiming; except when one of them happened to be
off LRU for other reasons (being reclaimed elsewhere, being migrated,
whatever) - and precisely those folios which were not safe to touch,
which have often been transferred to a private worklist, are the ones
which the code below goes on to play with - corrupting either or both
lists.  (I haven't tried to reproduce that in practice, just saw it
in the code, and verified with a count that no pages were reclaimed.)

> +			folio_put(folio);
> +			continue;
> +		}
> +		folio_put(folio);
> +
> +		/*
> +		 * Prepare the folios to be passed to reclaim_pages().
> +		 * VM can't reclaim a folio unless young bit is
> +		 * cleared in its flags.
> +		 */
> +		folio_clear_referenced(folio);
> +		folio_test_clear_young(folio);
> +		list_add(&folio->lru, list);
> +		if (need_resched()) {
> +			xas_pause(&xas);
> +			cond_resched_rcu();
> +		}
> +	}
> +	rcu_read_unlock();
> +}
> +
> +static int shmem_fadvise_dontneed(struct address_space *mapping, loff_t start,
> +				loff_t end)

loff_t? They are pgoff_t. And why return an int which is always 0?

> +{
> +	LIST_HEAD(folio_list);
> +
> +	if (!total_swap_pages || mapping_unevictable(mapping))
> +		return 0;
> +
> +	lru_add_drain();
> +	shmem_isolate_pages_range(mapping, start, end, &folio_list);
> +	reclaim_pages(&folio_list);
> +
> +	return 0;
> +}
> +
> +static int shmem_fadvise_willneed(struct address_space *mapping,
> +				 pgoff_t start, pgoff_t long end)

pgoff_t long? That's a new type to me! Again, why return an int always 0?

> +{
> +	struct folio *folio;
> +	pgoff_t index;
> +
> +	xa_for_each_range(&mapping->i_pages, index, folio, start, end) {
> +		if (!xa_is_value(folio))
> +			continue;
> +		folio = shmem_read_folio(mapping, index);
> +		if (!IS_ERR(folio))
> +			folio_put(folio);
> +	}
> +
> +	return 0;
> +}
> +
> +static int shmem_fadvise(struct file *file, loff_t offset, loff_t len, int advice)
> +{
> +	loff_t endbyte;
> +	pgoff_t start_index;
> +	pgoff_t end_index;
> +	struct address_space *mapping;
> +	struct inode *inode = file_inode(file);
> +	int ret = 0;
> +
> +	if (S_ISFIFO(inode->i_mode))
> +		return -ESPIPE;
> +
> +	mapping = file->f_mapping;
> +	if (!mapping || len < 0 || !shmem_mapping(mapping))
> +		return -EINVAL;
> +
> +	endbyte = fadvise_calc_endbyte(offset, len);
> +
> +	start_index = offset >> PAGE_SHIFT;
> +	end_index   = endbyte >> PAGE_SHIFT;
> +	switch (advice) {
> +	case POSIX_FADV_DONTNEED:

This is where I ran out of time.  I'm afraid all the focus on
fadvise_calc_endbyte() has distracted you from looking at the DONTNEED
in mm/fadvise.c: where there are detailed comments on why and how it
then narrows the DONTNEED range.  And aside from needing to duplicate
that here for shmem (or put it into another or combined helper), it
implies to me that shmem_isolate_pages_range() needs to do a similar
narrowing, when it finds that the range overlaps part of a large folio.

Something that has crossed my mind as a worry, but I've not had time
to look further into (maybe it's no concern at all) is the question
of this syscall temporarily isolating a very large number of folios,
whether they need to be (or perhaps already are) counted in
NR_ISOLATED_ANON, whether too many isolated needs to be limited.

> +		ret = shmem_fadvise_dontneed(mapping, start_index, end_index);
> +		break;
> +	case POSIX_FADV_WILLNEED:
> +		ret = shmem_fadvise_willneed(mapping, start_index, end_index);
> +		break;
> +	case POSIX_FADV_NORMAL:
> +	case POSIX_FADV_RANDOM:
> +	case POSIX_FADV_SEQUENTIAL:
> +	case POSIX_FADV_NOREUSE:
> +		/*
> +		 * No bad return value, but ignore advice.
> +		 */
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
>  static struct inode *shmem_get_inode(struct mnt_idmap *idmap, struct super_block *sb,
>  				     struct inode *dir, umode_t mode, dev_t dev,
>  				     unsigned long flags)
> @@ -3942,6 +4057,7 @@ static const struct file_operations shmem_file_operations = {
>  	.splice_write	= iter_file_splice_write,
>  	.fallocate	= shmem_fallocate,
>  #endif
> +	.fadvise	= shmem_fadvise,

I'd say posix_fadvise() is an operation on an fd, and shmem_fadvise() and
all its helpers should be under CONFIG_TMPFS (but oftentimes I do think
CONFIG_TMPFS and CONFIG_SHMEM are more trouble than they are worth).

Hugh

>  };
>  
>  static const struct inode_operations shmem_inode_operations = {
> -- 
> 2.7.4

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

[Charan Teja Kalla]

Thanks Hugh for the valuable comments!!

On 4/21/2023 5:37 AM, Hugh Dickins wrote:
>> Signed-off-by: Charan Teja Kalla <quic_charante@quicinc.com>
> I'm sorry, but no, this is not yet ready for primetime. I came here
> expecting to be able just to add a patch on top with small fixes,
> but see today that it needs more than that, and my time has run out.
> 
> Though if Andrew is keen to go ahead with it in 6.4, and add fixes
> on top while it's in rc, that will be okay: except for one small bad
@Andrew: I should resend the patch soon with all these comments addressed.
> bug, which must be fixed immediately - "luckily" nobody appears to
> be using or testing this since v5, but it cannot go further as is>
> Willneed is probably fine, but dontneed is not.
> 
>> ---
>>  mm/shmem.c | 116 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 116 insertions(+)
>>
>> diff --git a/mm/shmem.c b/mm/shmem.c
>> index 448f393..1af8525 100644
>> --- a/mm/shmem.c
>> +++ b/mm/shmem.c
>> @@ -40,6 +40,9 @@
>>  #include <linux/fs_parser.h>
>>  #include <linux/swapfile.h>
>>  #include <linux/iversion.h>
>> +#include <linux/mm_inline.h>
>> +#include <linux/fadvise.h>
>> +#include <linux/page_idle.h>
>>  #include "swap.h"
>>  
>>  static struct vfsmount *shm_mnt;
>> @@ -2344,6 +2347,118 @@ static void shmem_set_inode_flags(struct inode *inode, unsigned int fsflags)
>>  #define shmem_initxattrs NULL
>>  #endif
>>  
>> +static void shmem_isolate_pages_range(struct address_space *mapping, loff_t start,
>> +				loff_t end, struct list_head *list)
> loff_t? They are pgoff_t.
> 
>> +{
>> +	XA_STATE(xas, &mapping->i_pages, start);
>> +	struct folio *folio;
>> +
>> +	rcu_read_lock();
>> +	xas_for_each(&xas, folio, end) {
>> +		if (xas_retry(&xas, folio))
>> +			continue;
>> +		if (xa_is_value(folio))
>> +			continue;
>> +
>> +		if (!folio_try_get(folio))
>> +			continue;
>> +		if (folio_test_unevictable(folio) || folio_mapped(folio) ||
>> +				folio_isolate_lru(folio)) {
> There is the one small bad bug.  That should say !folio_isolate_lru(folio).
> In v5, it was isolate_lru_page(page), because isolate_lru_page() returned
> 0 for success or -EBUSY for unavailable; whereas folio_isolate_lru(folio)
> is a boolean, returning true if it successfully removed folio from LRU.
> 

Looks bad thing from my side:(.  Thanks a lot for catching it. This time
I will update the patch with unit tests too.

> The effect of that bug is that in v6 and v7, it has skipped all the folios
> it was expected to be reclaiming; except when one of them happened to be
> off LRU for other reasons (being reclaimed elsewhere, being migrated,
> whatever) - and precisely those folios which were not safe to touch,
> which have often been transferred to a private worklist, are the ones
> which the code below goes on to play with - corrupting either or both
> lists.  (I haven't tried to reproduce that in practice, just saw it
> in the code, and verified with a count that no pages were reclaimed.)

True.
> 
>> +			folio_put(folio);
>> +			continue;
>> +		}
>> +		folio_put(folio);
>> +
>> +		/*
>> +		 * Prepare the folios to be passed to reclaim_pages().
>> +		 * VM can't reclaim a folio unless young bit is
>> +		 * cleared in its flags.
>> +		 */
>> +		folio_clear_referenced(folio);
>> +		folio_test_clear_young(folio);
>> +		list_add(&folio->lru, list);
>> +		if (need_resched()) {
>> +			xas_pause(&xas);
>> +			cond_resched_rcu();
>> +		}
>> +	}
>> +	rcu_read_unlock();
>> +}
>> +
>> +static int shmem_fadvise_dontneed(struct address_space *mapping, loff_t start,
>> +				loff_t end)
> loff_t? They are pgoff_t. And why return an int which is always 0?
> 
>> +{
>> +	LIST_HEAD(folio_list);
>> +
>> +	if (!total_swap_pages || mapping_unevictable(mapping))
>> +		return 0;
>> +
>> +	lru_add_drain();
>> +	shmem_isolate_pages_range(mapping, start, end, &folio_list);
>> +	reclaim_pages(&folio_list);
>> +
>> +	return 0;
>> +}
>> +
>> +static int shmem_fadvise_willneed(struct address_space *mapping,
>> +				 pgoff_t start, pgoff_t long end)
> pgoff_t long? That's a new type to me! Again, why return an int always 0?
> 
Will remove this in the next patch.
>> +{
>> +	struct folio *folio;
>> +	pgoff_t index;
>> +
>> +	xa_for_each_range(&mapping->i_pages, index, folio, start, end) {
>> +		if (!xa_is_value(folio))
>> +			continue;
>> +		folio = shmem_read_folio(mapping, index);
>> +		if (!IS_ERR(folio))
>> +			folio_put(folio);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int shmem_fadvise(struct file *file, loff_t offset, loff_t len, int advice)
>> +{
>> +	loff_t endbyte;
>> +	pgoff_t start_index;
>> +	pgoff_t end_index;
>> +	struct address_space *mapping;
>> +	struct inode *inode = file_inode(file);
>> +	int ret = 0;
>> +
>> +	if (S_ISFIFO(inode->i_mode))
>> +		return -ESPIPE;
>> +
>> +	mapping = file->f_mapping;
>> +	if (!mapping || len < 0 || !shmem_mapping(mapping))
>> +		return -EINVAL;
>> +
>> +	endbyte = fadvise_calc_endbyte(offset, len);
>> +
>> +	start_index = offset >> PAGE_SHIFT;
>> +	end_index   = endbyte >> PAGE_SHIFT;
>> +	switch (advice) {
>> +	case POSIX_FADV_DONTNEED:
> This is where I ran out of time.  I'm afraid all the focus on
> fadvise_calc_endbyte() has distracted you from looking at the DONTNEED
> in mm/fadvise.c: where there are detailed comments on why and how it
> then narrows the DONTNEED range.  And aside from needing to duplicate
> that here for shmem (or put it into another or combined helper), it
> implies to me that shmem_isolate_pages_range() needs to do a similar
> narrowing, when it finds that the range overlaps part of a large folio.
> 
Sure, will include those range calculations for shmem pages too.

> Something that has crossed my mind as a worry, but I've not had time
> to look further into (maybe it's no concern at all) is the question
> of this syscall temporarily isolating a very large number of folios,
> whether they need to be (or perhaps already are) counted in
> NR_ISOLATED_ANON, whether too many isolated needs to be limited.

They are _not_ counted as ISOLATED_ANON now as this operation is for a
small duration. I do see there exists too_many_isolated() checks in
direct reclaim/compaction logic where it is necessary to stop the
multiple processes in the direct reclaim from isolating too many pages.

I am not able to envisage such problem here, where usually single
process doing the fadvise operation on a file. Even If the file is
opened by multiple processes and do fadvise, the operation is limited
only to the pages of this file and doesn't impact the system.

Please let me know if I'm missing something where I should be counting
these as NR_ISOLATED.

> 
>> +		ret = shmem_fadvise_dontneed(mapping, start_index, end_index);
>> +		break;
>> +	case POSIX_FADV_WILLNEED:
>> +		ret = shmem_fadvise_willneed(mapping, start_index, end_index);
>> +		break;
>> +	case POSIX_FADV_NORMAL:
>> +	case POSIX_FADV_RANDOM:
>> +	case POSIX_FADV_SEQUENTIAL:
>> +	case POSIX_FADV_NOREUSE:
>> +		/*
>> +		 * No bad return value, but ignore advice.
>> +		 */
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>>  static struct inode *shmem_get_inode(struct mnt_idmap *idmap, struct super_block *sb,
>>  				     struct inode *dir, umode_t mode, dev_t dev,
>>  				     unsigned long flags)
>> @@ -3942,6 +4057,7 @@ static const struct file_operations shmem_file_operations = {
>>  	.splice_write	= iter_file_splice_write,
>>  	.fallocate	= shmem_fallocate,
>>  #endif
>> +	.fadvise	= shmem_fadvise,
> I'd say posix_fadvise() is an operation on an fd, and shmem_fadvise() and
> all its helpers should be under CONFIG_TMPFS (but oftentimes I do think
Sure.
> CONFIG_TMPFS and CONFIG_SHMEM are more trouble than they are worth).
> 
> Hugh
> 

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

[Hugh Dickins]

On Mon, 24 Apr 2023, Charan Teja Kalla wrote:
> On 4/21/2023 5:37 AM, Hugh Dickins wrote:
> > This is where I ran out of time.  I'm afraid all the focus on
> > fadvise_calc_endbyte() has distracted you from looking at the DONTNEED
> > in mm/fadvise.c: where there are detailed comments on why and how it
> > then narrows the DONTNEED range.  And aside from needing to duplicate
> > that here for shmem (or put it into another or combined helper), it
> > implies to me that shmem_isolate_pages_range() needs to do a similar
> > narrowing, when it finds that the range overlaps part of a large folio.
> > 
> Sure, will include those range calculations for shmem pages too.

Oh, I forgot this issue, you would have liked me to look at V8 by now,
to see whether I agree with your resolution there.  Sorry, no, I've
not been able to divert my concentration to it yet.

And it's quite likely that I shall disagree, because I've a history of
disagreeing even with myself on such range widening/narrowing issues -
reconciling conflicting precedents is difficult :(

> 
> > Something that has crossed my mind as a worry, but I've not had time
> > to look further into (maybe it's no concern at all) is the question
> > of this syscall temporarily isolating a very large number of folios,
> > whether they need to be (or perhaps already are) counted in
> > NR_ISOLATED_ANON, whether too many isolated needs to be limited.
> 
> They are _not_ counted as ISOLATED_ANON now as this operation is for a
> small duration. I do see there exists too_many_isolated() checks in
> direct reclaim/compaction logic where it is necessary to stop the
> multiple processes in the direct reclaim from isolating too many pages.
> 
> I am not able to envisage such problem here, where usually single
> process doing the fadvise operation on a file. Even If the file is
> opened by multiple processes and do fadvise, the operation is limited
> only to the pages of this file and doesn't impact the system.
> 
> Please let me know if I'm missing something where I should be counting
> these as NR_ISOLATED.

Please grep for NR_ISOLATED, to see where and how they get manipulated
already, and follow the existing examples.  The case that sticks in my
mind is in mm/mempolicy.c, where the migrate_pages() syscall can build
up a gigantic quantity of transiently isolated pages: your syscall can
do the same, so should account for itself in the same way.

I'm not claiming that mm/vmscan.c's too_many_isolated(), and the way it
gets used by shrink_inactive_list(), is perfect: not at all.  But please
follow existing convention.

Sorry, that's all for now.
Hugh

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

[Charan Teja Kalla]

Hi Hugh, Thanks for the time and comments on this patch.

On 5/17/2023 5:02 PM, Hugh Dickins wrote:
>> Sure, will include those range calculations for shmem pages too.
> Oh, I forgot this issue, you would have liked me to look at V8 by now,
> to see whether I agree with your resolution there.  Sorry, no, I've
> not been able to divert my concentration to it yet.
> 
> And it's quite likely that I shall disagree, because I've a history of
> disagreeing even with myself on such range widening/narrowing issues -
> reconciling conflicting precedents is difficult :(
> 
If you can at least help by commenting which part of the patch you
disagree with, I can try hard to convince you there:) .

>> Please let me know if I'm missing something where I should be counting
>> these as NR_ISOLATED.
> Please grep for NR_ISOLATED, to see where and how they get manipulated
> already, and follow the existing examples.  The case that sticks in my
> mind is in mm/mempolicy.c, where the migrate_pages() syscall can build
> up a gigantic quantity of transiently isolated pages: your syscall can
> do the same, so should account for itself in the same way.

I had a V8 posted without this into accounting. Let me make the changes
to account for the NR_ISOLATED too.
> 
> I'm not claiming that mm/vmscan.c's too_many_isolated(), and the way it
> gets used by shrink_inactive_list(), is perfect: not at all.  But please
> follow existing convention.
> 
> Sorry, that's all for now.

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

[Charan Teja Kalla]

Hello Hugh,

Based on offline discussion with some folks in the list, it seems that
this syscall can be helpful. This patch might have forgotten and I hope
this ping helps in resurrecting this thread.

On 5/18/2023 6:16 PM, Charan Teja Kalla wrote:
> On 5/17/2023 5:02 PM, Hugh Dickins wrote:
>>> Sure, will include those range calculations for shmem pages too.
>> Oh, I forgot this issue, you would have liked me to look at V8 by now,
>> to see whether I agree with your resolution there.  Sorry, no, I've
>> not been able to divert my concentration to it yet.
>>
>> And it's quite likely that I shall disagree, because I've a history of
>> disagreeing even with myself on such range widening/narrowing issues -
>> reconciling conflicting precedents is difficult 🙁
>>
> If you can at least help by commenting which part of the patch you
> disagree with, I can try hard to convince you there:) .
> 
>>> Please let me know if I'm missing something where I should be counting
>>> these as NR_ISOLATED.
>> Please grep for NR_ISOLATED, to see where and how they get manipulated
>> already, and follow the existing examples.  The case that sticks in my
>> mind is in mm/mempolicy.c, where the migrate_pages() syscall can build
>> up a gigantic quantity of transiently isolated pages: your syscall can
>> do the same, so should account for itself in the same way.

Based on the grep, it seems almost all the call stacks that isolates the
folios is for migrating the pages where after migration the NR_ISOLATED
is decremented (in migrate_folio_done()). The call paths are(compaction,
memory hotplug, mempolicy).

The another call path is reclaim where we isolate 'nr' pages belongs to
a pgdat, account/unaccount them in NR_ISOLATED across the reclaim.

I think it is easy to account for the above call paths as we know "which
folio corresponds to which pgdat".

Where as in this patch, we are isolating a set of folios(can corresponds
to different nodes) and relying on the reclaim_pages() to do the swap
out. It is straightforward to account NR_ISOLATED while isolating, but
it requires unaccounting changes in the shrink_folio_list() where folio
is being freed after swap out.  Doing so requires changes in all the
code places(eg: shrink_inactive_list()), where it now requires to
account NR_ISOLATED while isolating and the shrink_folio_list()
unaccounts it.

So, accounting NR_ISOLATED requires changes in other code places where
this patch has not touched.

If isolating a large amount of pages and not being recorded in
NR_ISOLATED is really a problem, then may I please know your opinion on
isolating(with out accounting) and reclaiming in small batches? The
batch size can be considered as SWAP_CLUSTER_MAX of pages.

> I had a V8 posted without this into accounting. Let me make the changes
> to account for the NR_ISOLATED too.

Thanks,
Charan

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

[Hugh Dickins]

[-- Attachment #1: Type: text/plain, Size: 4571 bytes --]

On Wed, 14 Feb 2024, Charan Teja Kalla wrote:

> Hello Hugh,
> 
> Based on offline discussion with some folks in the list, it seems that
> this syscall can be helpful. This patch might have forgotten and I hope
> this ping helps in resurrecting this thread.

Charan, it's not forgotten, but it was relayed to you through another
channel a month ago, that I did not expect to have time to think about
this for 3 months.  Countdown says 2 months to go now.

I realize that it's frustrating for you; it's unpleasant for me too.

> 
> On 5/18/2023 6:16 PM, Charan Teja Kalla wrote:
> > On 5/17/2023 5:02 PM, Hugh Dickins wrote:
> >>> Sure, will include those range calculations for shmem pages too.
> >> Oh, I forgot this issue, you would have liked me to look at V8 by now,
> >> to see whether I agree with your resolution there.  Sorry, no, I've
> >> not been able to divert my concentration to it yet.
> >>
> >> And it's quite likely that I shall disagree, because I've a history of
> >> disagreeing even with myself on such range widening/narrowing issues -
> >> reconciling conflicting precedents is difficult 🙁
> >>
> > If you can at least help by commenting which part of the patch you
> > disagree with, I can try hard to convince you there:) .
> > 
> >>> Please let me know if I'm missing something where I should be counting
> >>> these as NR_ISOLATED.
> >> Please grep for NR_ISOLATED, to see where and how they get manipulated
> >> already, and follow the existing examples.  The case that sticks in my
> >> mind is in mm/mempolicy.c, where the migrate_pages() syscall can build
> >> up a gigantic quantity of transiently isolated pages: your syscall can
> >> do the same, so should account for itself in the same way.
> 
> Based on the grep, it seems almost all the call stacks that isolates the
> folios is for migrating the pages where after migration the NR_ISOLATED
> is decremented (in migrate_folio_done()). The call paths are(compaction,
> memory hotplug, mempolicy).
> 
> The another call path is reclaim where we isolate 'nr' pages belongs to
> a pgdat, account/unaccount them in NR_ISOLATED across the reclaim.
> 
> I think it is easy to account for the above call paths as we know "which
> folio corresponds to which pgdat".
> 
> Where as in this patch, we are isolating a set of folios(can corresponds
> to different nodes) and relying on the reclaim_pages() to do the swap
> out. It is straightforward to account NR_ISOLATED while isolating, but
> it requires unaccounting changes in the shrink_folio_list() where folio
> is being freed after swap out.  Doing so requires changes in all the
> code places(eg: shrink_inactive_list()), where it now requires to
> account NR_ISOLATED while isolating and the shrink_folio_list()
> unaccounts it.
> 
> So, accounting NR_ISOLATED requires changes in other code places where
> this patch has not touched.

That surprises me, though I do recall that there's an irritating
asymmetry in where NR_ISOLATED is accounted and unaccounted.
I have not checked what you say there, may do so in 2 months.

> 
> If isolating a large amount of pages and not being recorded in
> NR_ISOLATED is really a problem, then may I please know your opinion on
> isolating(with out accounting) and reclaiming in small batches? The
> batch size can be considered as SWAP_CLUSTER_MAX of pages.

In most circumstances, omitting to account NR_ISOLATED wouldn't show
up as a problem; in low memory it would.  Splitting into small batches
without accounting might be an easier and better way; but whatever I
say in a hurried unthoughtful reply is likely to be wrong.

I am not convinced that isolating is even appropriate: I think I
hinted before that I would want to compare what you do here with what
shmem_swapin_range() does in mm/madvise.c, and the shmem_collapse_swapin()
I'll be proposing to avoid swapin while building up THP in collapse_file().

But it may well be that you've found the switching of LRUs essential:
I'm not prejudging, just saying I cannot rush to judgment.  And this
is also a new UAPI for tmpfs, so should not be rushed then regretted.

But if you can find another champion to force this into mm/shmem.c
for you faster than I can manage, well, I don't own any Linux source.
It's not unusual for me to limp along later and rearrange things to
suit my preference.

Hugh

> 
> > I had a V8 posted without this into accounting. Let me make the changes
> > to account for the NR_ISOLATED too.
> 
> Thanks,
> Charan