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

  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(+)
  

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.
  

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
>
  

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
  

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.
  

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
  

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