| Message ID | 20240202022029.1903629-1-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/madvise: set ra_pages as device max request size during ADV_POPULATE_READ | expand |
On Fri, Feb 02, 2024 at 10:20:29AM +0800, Ming Lei wrote: > +static struct file *madvise_override_ra_win(struct file *f, > + unsigned long start, unsigned long end, > + unsigned int *old_ra_pages) > +{ > + unsigned int io_pages; > + > + if (!f || !f->f_mapping || !f->f_mapping->host) > + return NULL; How can ->f_mapping be NULL? How can f_mapping->host be NULL?
On Thu, Feb 01 2024 at 9:20P -0500, Ming Lei <ming.lei@redhat.com> wrote: > madvise(MADV_POPULATE_READ) tries to populate all page tables in the > specific range, so it is usually sequential IO if VMA is backed by > file. > > Set ra_pages as device max request size for the involved readahead in > the ADV_POPULATE_READ, this way reduces latency of madvise(MADV_POPULATE_READ) > to 1/10 when running madvise(MADV_POPULATE_READ) over one 1GB file with > usual(default) 128KB of read_ahead_kb. > > Cc: David Hildenbrand <david@redhat.com> > Cc: Matthew Wilcox <willy@infradead.org> > Cc: Alexander Viro <viro@zeniv.linux.org.uk> > Cc: Christian Brauner <brauner@kernel.org> > Cc: Don Dutile <ddutile@redhat.com> > Cc: Rafael Aquini <raquini@redhat.com> > Cc: Dave Chinner <david@fromorbit.com> > Cc: Mike Snitzer <snitzer@kernel.org> > Cc: Andrew Morton <akpm@linux-foundation.org> > Signed-off-by: Ming Lei <ming.lei@redhat.com> > --- > mm/madvise.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 51 insertions(+), 1 deletion(-) > > diff --git a/mm/madvise.c b/mm/madvise.c > index 912155a94ed5..db5452c8abdd 100644 > --- a/mm/madvise.c > +++ b/mm/madvise.c > @@ -900,6 +900,37 @@ static long madvise_dontneed_free(struct vm_area_struct *vma, > return -EINVAL; > } > > +static void madvise_restore_ra_win(struct file **file, unsigned int ra_pages) > +{ > + if (*file) { > + struct file *f = *file; > + > + f->f_ra.ra_pages = ra_pages; > + fput(f); > + *file = NULL; > + } > +} > + > +static struct file *madvise_override_ra_win(struct file *f, > + unsigned long start, unsigned long end, > + unsigned int *old_ra_pages) > +{ > + unsigned int io_pages; > + > + if (!f || !f->f_mapping || !f->f_mapping->host) > + return NULL; > + > + io_pages = inode_to_bdi(f->f_mapping->host)->io_pages; > + if (((end - start) >> PAGE_SHIFT) < io_pages) > + return NULL; > + > + f = get_file(f); > + *old_ra_pages = f->f_ra.ra_pages; > + f->f_ra.ra_pages = io_pages; > + > + return f; > +} > + Does this override imply that madvise_populate resorts to calling filemap_fault() and here you're just arming it to use the larger ->io_pages for the duration of all associated faulting? Wouldn't it be better to avoid faulting and build up larger page vectors that get sent down to the block layer in one go and let the block layer split using the device's limits? (like happens with force_page_cache_ra) I'm concerned that madvise_populate isn't so efficient with filemap due to excessive faulting (*BUT* I haven't traced to know, I'm just inferring that is why twiddling f->f_ra.ra_pages helps improve madvise_populate by having it issue larger IO. Apologies if I'm way off base) Mike
On Fri, Feb 02, 2024 at 04:15:39AM +0000, Matthew Wilcox wrote: > On Fri, Feb 02, 2024 at 10:20:29AM +0800, Ming Lei wrote: > > +static struct file *madvise_override_ra_win(struct file *f, > > + unsigned long start, unsigned long end, > > + unsigned int *old_ra_pages) > > +{ > > + unsigned int io_pages; > > + > > + if (!f || !f->f_mapping || !f->f_mapping->host) > > + return NULL; > > How can ->f_mapping be NULL? How can f_mapping->host be NULL? You are right, the two checks can be removed because both two won't be NULL for opened file, and .f_ra is initialized with f->f_mapping->host->i_mapping directly too. I will drop the checks in next version. BTW, looks the same check in madvise_remove() can removed too. Thanks, Ming
On Thu, Feb 01, 2024 at 11:43:11PM -0500, Mike Snitzer wrote: > On Thu, Feb 01 2024 at 9:20P -0500, > Ming Lei <ming.lei@redhat.com> wrote: > > > madvise(MADV_POPULATE_READ) tries to populate all page tables in the > > specific range, so it is usually sequential IO if VMA is backed by > > file. > > > > Set ra_pages as device max request size for the involved readahead in > > the ADV_POPULATE_READ, this way reduces latency of madvise(MADV_POPULATE_READ) > > to 1/10 when running madvise(MADV_POPULATE_READ) over one 1GB file with > > usual(default) 128KB of read_ahead_kb. > > > > Cc: David Hildenbrand <david@redhat.com> > > Cc: Matthew Wilcox <willy@infradead.org> > > Cc: Alexander Viro <viro@zeniv.linux.org.uk> > > Cc: Christian Brauner <brauner@kernel.org> > > Cc: Don Dutile <ddutile@redhat.com> > > Cc: Rafael Aquini <raquini@redhat.com> > > Cc: Dave Chinner <david@fromorbit.com> > > Cc: Mike Snitzer <snitzer@kernel.org> > > Cc: Andrew Morton <akpm@linux-foundation.org> > > Signed-off-by: Ming Lei <ming.lei@redhat.com> > > --- > > mm/madvise.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 51 insertions(+), 1 deletion(-) > > > > diff --git a/mm/madvise.c b/mm/madvise.c > > index 912155a94ed5..db5452c8abdd 100644 > > --- a/mm/madvise.c > > +++ b/mm/madvise.c > > @@ -900,6 +900,37 @@ static long madvise_dontneed_free(struct vm_area_struct *vma, > > return -EINVAL; > > } > > > > +static void madvise_restore_ra_win(struct file **file, unsigned int ra_pages) > > +{ > > + if (*file) { > > + struct file *f = *file; > > + > > + f->f_ra.ra_pages = ra_pages; > > + fput(f); > > + *file = NULL; > > + } > > +} > > + > > +static struct file *madvise_override_ra_win(struct file *f, > > + unsigned long start, unsigned long end, > > + unsigned int *old_ra_pages) > > +{ > > + unsigned int io_pages; > > + > > + if (!f || !f->f_mapping || !f->f_mapping->host) > > + return NULL; > > + > > + io_pages = inode_to_bdi(f->f_mapping->host)->io_pages; > > + if (((end - start) >> PAGE_SHIFT) < io_pages) > > + return NULL; > > + > > + f = get_file(f); > > + *old_ra_pages = f->f_ra.ra_pages; > > + f->f_ra.ra_pages = io_pages; > > + > > + return f; > > +} > > + > > Does this override imply that madvise_populate resorts to calling > filemap_fault() and here you're just arming it to use the larger > ->io_pages for the duration of all associated faulting? Yes. > > Wouldn't it be better to avoid faulting and build up larger page How can we avoid the fault handling? which is needed to build VA->PA mapping. > vectors that get sent down to the block layer in one go and let the filemap_fault() already tries to allocate folio in big size(max order is MAX_PAGECACHE_ORDER), see page_cache_ra_order() and ra_alloc_folio(). > block layer split using the device's limits? (like happens with > force_page_cache_ra) Here filemap code won't deal with block directly because there is VFS & FS and io mapping is required, and it just calls aops->readahead() or aops->read_folio(), but block plug & readahead_control are applied for handling everything in batch. > > I'm concerned that madvise_populate isn't so efficient with filemap That is why this patch increases readahead window, then madvise_populate() performance can be improved by X10 in big file-backed popluate read. > due to excessive faulting (*BUT* I haven't traced to know, I'm just > inferring that is why twiddling f->f_ra.ra_pages helps improve > madvise_populate by having it issue larger IO. Apologies if I'm way > off base) As mentioned, fault handling can't be avoided, but we can improve involved readahead IO perf. Thanks, Ming
On Fri, Feb 02 2024 at 5:52P -0500, Ming Lei <ming.lei@redhat.com> wrote: > On Thu, Feb 01, 2024 at 11:43:11PM -0500, Mike Snitzer wrote: > > On Thu, Feb 01 2024 at 9:20P -0500, > > Ming Lei <ming.lei@redhat.com> wrote: > > > > > madvise(MADV_POPULATE_READ) tries to populate all page tables in the > > > specific range, so it is usually sequential IO if VMA is backed by > > > file. > > > > > > Set ra_pages as device max request size for the involved readahead in > > > the ADV_POPULATE_READ, this way reduces latency of madvise(MADV_POPULATE_READ) > > > to 1/10 when running madvise(MADV_POPULATE_READ) over one 1GB file with > > > usual(default) 128KB of read_ahead_kb. > > > > > > Cc: David Hildenbrand <david@redhat.com> > > > Cc: Matthew Wilcox <willy@infradead.org> > > > Cc: Alexander Viro <viro@zeniv.linux.org.uk> > > > Cc: Christian Brauner <brauner@kernel.org> > > > Cc: Don Dutile <ddutile@redhat.com> > > > Cc: Rafael Aquini <raquini@redhat.com> > > > Cc: Dave Chinner <david@fromorbit.com> > > > Cc: Mike Snitzer <snitzer@kernel.org> > > > Cc: Andrew Morton <akpm@linux-foundation.org> > > > Signed-off-by: Ming Lei <ming.lei@redhat.com> > > > --- > > > mm/madvise.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++++++- > > > 1 file changed, 51 insertions(+), 1 deletion(-) > > > > > > diff --git a/mm/madvise.c b/mm/madvise.c > > > index 912155a94ed5..db5452c8abdd 100644 > > > --- a/mm/madvise.c > > > +++ b/mm/madvise.c > > > @@ -900,6 +900,37 @@ static long madvise_dontneed_free(struct vm_area_struct *vma, > > > return -EINVAL; > > > } > > > > > > +static void madvise_restore_ra_win(struct file **file, unsigned int ra_pages) > > > +{ > > > + if (*file) { > > > + struct file *f = *file; > > > + > > > + f->f_ra.ra_pages = ra_pages; > > > + fput(f); > > > + *file = NULL; > > > + } > > > +} > > > + > > > +static struct file *madvise_override_ra_win(struct file *f, > > > + unsigned long start, unsigned long end, > > > + unsigned int *old_ra_pages) > > > +{ > > > + unsigned int io_pages; > > > + > > > + if (!f || !f->f_mapping || !f->f_mapping->host) > > > + return NULL; > > > + > > > + io_pages = inode_to_bdi(f->f_mapping->host)->io_pages; > > > + if (((end - start) >> PAGE_SHIFT) < io_pages) > > > + return NULL; > > > + > > > + f = get_file(f); > > > + *old_ra_pages = f->f_ra.ra_pages; > > > + f->f_ra.ra_pages = io_pages; > > > + > > > + return f; > > > +} > > > + > > > > Does this override imply that madvise_populate resorts to calling > > filemap_fault() and here you're just arming it to use the larger > > ->io_pages for the duration of all associated faulting? > > Yes. > > > > > Wouldn't it be better to avoid faulting and build up larger page > > How can we avoid the fault handling? which is needed to build VA->PA mapping. I was wondering if it made sense to add fadvise_populate -- but given my lack of experience with MM I then get handwavvy quick -- I have more work ahead to round out my MM understanding so that I'm more informed. > > vectors that get sent down to the block layer in one go and let the > > filemap_fault() already tries to allocate folio in big size(max order > is MAX_PAGECACHE_ORDER), see page_cache_ra_order() and ra_alloc_folio(). > > > block layer split using the device's limits? (like happens with > > force_page_cache_ra) > > Here filemap code won't deal with block directly because there is VFS & > FS and io mapping is required, and it just calls aops->readahead() or > aops->read_folio(), but block plug & readahead_control are applied for > handling everything in batch. > > > > > I'm concerned that madvise_populate isn't so efficient with filemap > > That is why this patch increases readahead window, then > madvise_populate() performance can be improved by X10 in big file-backed > popluate read. Right, as you know I've tested your patch, the larger readahead window certainly did provide the much more desirable performance. I'll reply to your v2 (with reduced negative checks) with my Reviewed-by and Tested-by. I was just wondering if there an opportunity to plumb in more a specific (and potentially better) fadvise_populate for dealing with file backed pages. > > due to excessive faulting (*BUT* I haven't traced to know, I'm just > > inferring that is why twiddling f->f_ra.ra_pages helps improve > > madvise_populate by having it issue larger IO. Apologies if I'm way > > off base) > > As mentioned, fault handling can't be avoided, but we can improve > involved readahead IO perf. Thanks, and sorry for asking such a naive question (put more pressure on you to educate than I should have). Mike
On Fri, Feb 02, 2024 at 10:20:29AM +0800, Ming Lei wrote: > madvise(MADV_POPULATE_READ) tries to populate all page tables in the > specific range, so it is usually sequential IO if VMA is backed by > file. > > Set ra_pages as device max request size for the involved readahead in > the ADV_POPULATE_READ, this way reduces latency of madvise(MADV_POPULATE_READ) > to 1/10 when running madvise(MADV_POPULATE_READ) over one 1GB file with > usual(default) 128KB of read_ahead_kb. > > Cc: David Hildenbrand <david@redhat.com> > Cc: Matthew Wilcox <willy@infradead.org> > Cc: Alexander Viro <viro@zeniv.linux.org.uk> > Cc: Christian Brauner <brauner@kernel.org> > Cc: Don Dutile <ddutile@redhat.com> > Cc: Rafael Aquini <raquini@redhat.com> > Cc: Dave Chinner <david@fromorbit.com> > Cc: Mike Snitzer <snitzer@kernel.org> > Cc: Andrew Morton <akpm@linux-foundation.org> > Signed-off-by: Ming Lei <ming.lei@redhat.com> > --- > mm/madvise.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 51 insertions(+), 1 deletion(-) > > diff --git a/mm/madvise.c b/mm/madvise.c > index 912155a94ed5..db5452c8abdd 100644 > --- a/mm/madvise.c > +++ b/mm/madvise.c > @@ -900,6 +900,37 @@ static long madvise_dontneed_free(struct vm_area_struct *vma, > return -EINVAL; > } > > +static void madvise_restore_ra_win(struct file **file, unsigned int ra_pages) > +{ > + if (*file) { > + struct file *f = *file; > + > + f->f_ra.ra_pages = ra_pages; > + fput(f); > + *file = NULL; > + } > +} > + > +static struct file *madvise_override_ra_win(struct file *f, > + unsigned long start, unsigned long end, > + unsigned int *old_ra_pages) > +{ > + unsigned int io_pages; > + > + if (!f || !f->f_mapping || !f->f_mapping->host) > + return NULL; > + > + io_pages = inode_to_bdi(f->f_mapping->host)->io_pages; > + if (((end - start) >> PAGE_SHIFT) < io_pages) > + return NULL; > + > + f = get_file(f); > + *old_ra_pages = f->f_ra.ra_pages; > + f->f_ra.ra_pages = io_pages; > + > + return f; > +} This won't do what you think if the file has been marked FMODE_RANDOM before this populate call. IOWs, I don't think madvise should be digging in the struct file readahead stuff here. It should call vfs_fadvise(FADV_SEQUENTIAL) to do the set the readahead mode, rather that try to duplicate FADV_SEQUENTIAL (badly). We already do this for WILLNEED to make it do the right thing, we should be doing the same thing here. Also, AFAICT, there is no need for get_file()/fput() here - the vma already has a reference to the struct file, and the vma should not be going away whilst the madvise() operation is in progress. -Dave.
On Mon, Feb 05, 2024 at 10:34:47AM +1100, Dave Chinner wrote: > On Fri, Feb 02, 2024 at 10:20:29AM +0800, Ming Lei wrote: > > madvise(MADV_POPULATE_READ) tries to populate all page tables in the > > specific range, so it is usually sequential IO if VMA is backed by > > file. > > > > Set ra_pages as device max request size for the involved readahead in > > the ADV_POPULATE_READ, this way reduces latency of madvise(MADV_POPULATE_READ) > > to 1/10 when running madvise(MADV_POPULATE_READ) over one 1GB file with > > usual(default) 128KB of read_ahead_kb. > > > > Cc: David Hildenbrand <david@redhat.com> > > Cc: Matthew Wilcox <willy@infradead.org> > > Cc: Alexander Viro <viro@zeniv.linux.org.uk> > > Cc: Christian Brauner <brauner@kernel.org> > > Cc: Don Dutile <ddutile@redhat.com> > > Cc: Rafael Aquini <raquini@redhat.com> > > Cc: Dave Chinner <david@fromorbit.com> > > Cc: Mike Snitzer <snitzer@kernel.org> > > Cc: Andrew Morton <akpm@linux-foundation.org> > > Signed-off-by: Ming Lei <ming.lei@redhat.com> > > --- > > mm/madvise.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 51 insertions(+), 1 deletion(-) > > > > diff --git a/mm/madvise.c b/mm/madvise.c > > index 912155a94ed5..db5452c8abdd 100644 > > --- a/mm/madvise.c > > +++ b/mm/madvise.c > > @@ -900,6 +900,37 @@ static long madvise_dontneed_free(struct vm_area_struct *vma, > > return -EINVAL; > > } > > > > +static void madvise_restore_ra_win(struct file **file, unsigned int ra_pages) > > +{ > > + if (*file) { > > + struct file *f = *file; > > + > > + f->f_ra.ra_pages = ra_pages; > > + fput(f); > > + *file = NULL; > > + } > > +} > > + > > +static struct file *madvise_override_ra_win(struct file *f, > > + unsigned long start, unsigned long end, > > + unsigned int *old_ra_pages) > > +{ > > + unsigned int io_pages; > > + > > + if (!f || !f->f_mapping || !f->f_mapping->host) > > + return NULL; > > + > > + io_pages = inode_to_bdi(f->f_mapping->host)->io_pages; > > + if (((end - start) >> PAGE_SHIFT) < io_pages) > > + return NULL; > > + > > + f = get_file(f); > > + *old_ra_pages = f->f_ra.ra_pages; > > + f->f_ra.ra_pages = io_pages; > > + > > + return f; > > +} > > This won't do what you think if the file has been marked > FMODE_RANDOM before this populate call. Yeah. But madvise(POPULATE_READ) is actually one action, so userspace can call fadvise(POSIX_FADV_NORMAL) or fadvise(POSIX_FADV_SEQUENTIAL) before madvise(POPULATE_READ), and set RANDOM advise back after madvise(POPULATE_READ) returns, so looks not big issue in reality. > > IOWs, I don't think madvise should be digging in the struct file > readahead stuff here. It should call vfs_fadvise(FADV_SEQUENTIAL) to > do the set the readahead mode, rather that try to duplicate > FADV_SEQUENTIAL (badly). We already do this for WILLNEED to make it > do the right thing, we should be doing the same thing here. FADV_SEQUENTIAL doubles current readahead window, which is far from enough to get top performance, such as, latency of doubling (default) ra window is still 2X of setting ra windows as bdi->io_pages. If application sets small 'bdi/read_ahead_kb' just like this report, the gap can be very big. Or can we add one API/helper in fs code to set file readahead ra_pages for this use case? > > Also, AFAICT, there is no need for get_file()/fput() here - the vma > already has a reference to the struct file, and the vma should not > be going away whilst the madvise() operation is in progress. You are right, get_file() is only needed in case of dropping mm lock. Thanks, Ming
diff --git a/mm/madvise.c b/mm/madvise.c index 912155a94ed5..db5452c8abdd 100644 --- a/mm/madvise.c +++ b/mm/madvise.c @@ -900,6 +900,37 @@ static long madvise_dontneed_free(struct vm_area_struct *vma, return -EINVAL; } +static void madvise_restore_ra_win(struct file **file, unsigned int ra_pages) +{ + if (*file) { + struct file *f = *file; + + f->f_ra.ra_pages = ra_pages; + fput(f); + *file = NULL; + } +} + +static struct file *madvise_override_ra_win(struct file *f, + unsigned long start, unsigned long end, + unsigned int *old_ra_pages) +{ + unsigned int io_pages; + + if (!f || !f->f_mapping || !f->f_mapping->host) + return NULL; + + io_pages = inode_to_bdi(f->f_mapping->host)->io_pages; + if (((end - start) >> PAGE_SHIFT) < io_pages) + return NULL; + + f = get_file(f); + *old_ra_pages = f->f_ra.ra_pages; + f->f_ra.ra_pages = io_pages; + + return f; +} + static long madvise_populate(struct vm_area_struct *vma, struct vm_area_struct **prev, unsigned long start, unsigned long end, @@ -908,9 +939,21 @@ static long madvise_populate(struct vm_area_struct *vma, const bool write = behavior == MADV_POPULATE_WRITE; struct mm_struct *mm = vma->vm_mm; unsigned long tmp_end; + unsigned int ra_pages; + struct file *file; int locked = 1; long pages; + /* + * In case of file backing mapping, increase readahead window + * for reducing the whole populate latency, and restore it + * after the populate is done + */ + if (behavior == MADV_POPULATE_READ) + file = madvise_override_ra_win(vma->vm_file, start, end, + &ra_pages); + else + file = NULL; *prev = vma; while (start < end) { @@ -920,8 +963,10 @@ static long madvise_populate(struct vm_area_struct *vma, */ if (!vma || start >= vma->vm_end) { vma = vma_lookup(mm, start); - if (!vma) + if (!vma) { + madvise_restore_ra_win(&file, ra_pages); return -ENOMEM; + } } tmp_end = min_t(unsigned long, end, vma->vm_end); @@ -935,6 +980,9 @@ static long madvise_populate(struct vm_area_struct *vma, vma = NULL; } if (pages < 0) { + /* restore ra pages back in case of any failure */ + madvise_restore_ra_win(&file, ra_pages); + switch (pages) { case -EINTR: return -EINTR; @@ -954,6 +1002,8 @@ static long madvise_populate(struct vm_area_struct *vma, } start += pages * PAGE_SIZE; } + + madvise_restore_ra_win(&file, ra_pages); return 0; }
madvise(MADV_POPULATE_READ) tries to populate all page tables in the specific range, so it is usually sequential IO if VMA is backed by file. Set ra_pages as device max request size for the involved readahead in the ADV_POPULATE_READ, this way reduces latency of madvise(MADV_POPULATE_READ) to 1/10 when running madvise(MADV_POPULATE_READ) over one 1GB file with usual(default) 128KB of read_ahead_kb. Cc: David Hildenbrand <david@redhat.com> Cc: Matthew Wilcox <willy@infradead.org> Cc: Alexander Viro <viro@zeniv.linux.org.uk> Cc: Christian Brauner <brauner@kernel.org> Cc: Don Dutile <ddutile@redhat.com> Cc: Rafael Aquini <raquini@redhat.com> Cc: Dave Chinner <david@fromorbit.com> Cc: Mike Snitzer <snitzer@kernel.org> Cc: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Ming Lei <ming.lei@redhat.com> --- mm/madvise.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 51 insertions(+), 1 deletion(-)