On Wed, Nov 4, 2020 at 1:34 PM Kent Overstreet
<kent.overstreet@xxxxxxxxx> wrote:
>
> On Wed, Nov 04, 2020 at 08:42:03PM +0000, Matthew Wilcox (Oracle) wrote:
> > Increasing the batch size runs into diminishing returns. It's probably
> > better to make, eg, three calls to filemap_get_pages() than it is to
> > call into kmalloc().
>
> I have to disagree. Working with PAGEVEC_SIZE pages is eventually going to be
> like working with 4k pages today, and have you actually read the slub code for
> the kmalloc fast path? It's _really_ fast, there's no atomic operations and it
> doesn't even have to disable preemption - which is why you never see it showing
> up in profiles ever since we switched to slub.
Yeah, kmalloc's fast path is extremely fast. Let's stay off
PAGEVEC_SIZE for now.
>
> It would however be better to have a standard abstraction for this rather than
> open coding it - perhaps adding it to the pagevec code. Please don't just drop
> it, though.
>
>
> > Signed-off-by: Matthew Wilcox (Oracle) <willy@xxxxxxxxxxxxx>
> > ---
> > mm/filemap.c | 15 ++-------------
> > 1 file changed, 2 insertions(+), 13 deletions(-)
> >
> > diff --git a/mm/filemap.c b/mm/filemap.c
> > index 23e3781b3aef..bb1c42d0223c 100644
> > --- a/mm/filemap.c
> > +++ b/mm/filemap.c
> > @@ -2429,8 +2429,8 @@ ssize_t generic_file_buffered_read(struct kiocb *iocb,
> > struct file_ra_state *ra = &filp->f_ra;
> > struct address_space *mapping = filp->f_mapping;
> > struct inode *inode = mapping->host;
> > - struct page *pages_onstack[PAGEVEC_SIZE], **pages = NULL;
> > - unsigned int nr_pages = min_t(unsigned int, 512,
> > + struct page *pages[PAGEVEC_SIZE];
> > + unsigned int nr_pages = min_t(unsigned int, PAGEVEC_SIZE,
> > ((iocb->ki_pos + iter->count + PAGE_SIZE - 1) >> PAGE_SHIFT) -
> > (iocb->ki_pos >> PAGE_SHIFT));
> > int i, pg_nr, error = 0;
> > @@ -2441,14 +2441,6 @@ ssize_t generic_file_buffered_read(struct kiocb *iocb,
> > return 0;
> > iov_iter_truncate(iter, inode->i_sb->s_maxbytes);
> >
> > - if (nr_pages > ARRAY_SIZE(pages_onstack))
> > - pages = kmalloc_array(nr_pages, sizeof(void *), GFP_KERNEL);
> > -
> > - if (!pages) {
> > - pages = pages_onstack;
> > - nr_pages = min_t(unsigned int, nr_pages, ARRAY_SIZE(pages_onstack));
> > - }
> > -
> > do {
> > cond_resched();
> >
> > @@ -2533,9 +2525,6 @@ ssize_t generic_file_buffered_read(struct kiocb *iocb,
> >
> > file_accessed(filp);
> >
> > - if (pages != pages_onstack)
> > - kfree(pages);
> > -
> > return written ? written : error;
> > }
> > EXPORT_SYMBOL_GPL(generic_file_buffered_read);
> > --
> > 2.28.0
> >
Best regards,
Amy Parker
(they/them)
