| Message ID | 631e42b6dffdcc4b4b24f5be715c37f78bf903db.1676378702.git.quic_charante@quicinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: shmem: support POSIX_FADV_[WILL|DONT]NEED for shmem files | expand |
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.
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
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.
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
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
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
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 = {
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(+)