Re: [PATCH v2 00/28] Convert GUP to folios

[Matthew Wilcox]

On Mon, Jan 10, 2022 at 11:31:03AM -0400, Jason Gunthorpe wrote:
> 
> On Mon, Jan 10, 2022 at 04:23:38AM +0000, Matthew Wilcox (Oracle) wrote:
> > This patch series is against my current folio for-next branch.  I know
> > it won't apply to sfr's next tree, and it's not for-next material yet.
> > I intend to submit it for 5.18 after I've rebased it to one of the
> > 5.17-rc releases.
> > 
> > The overall effect of this (ignoring the primary "preparing for folios
> > that are not PAGE or PMD sized" purpose) is to reduce the size of gup.o
> > by ~700 bytes in the config I normally test with.
> > 
> > This patchset just converts existing implementations to use folios.
> > There's no new API for consumers here to provide information in a more
> > efficient (address, length) format.  That will be a separate patchset.
> > 
> > In v2, I've tried to address all the comments from the reviews I got
> > on v1.  Apologies if I missed anything; I got a lot of good feedback.
> > Primarily I separated out the folio changes (later) from the "While
> > I'm looking at this ..." changes (earlier).  I'm not sure the story
> > of the patchset is necessarily coherent this way, but it should be
> > easier to review.
> > 
> > Another big change is that compound_pincount is now available to all
> > compound pages, not just those that are order-2-or-higher.  Patch 11.
> > 
> > I did notice one bug in my original patchset which I'm disappointed GCC
> > didn't diagnose:
> > 
> > 		pages[nr++] = nth_page(page, nr);
> > 
> > Given the massive reorg of the patchset, I didn't feel right using
> > anyone's SoB from v1 on any of the patches.  But, despite the increased
> > number of patches, I hope it's easier to review than v1.
> > 
> > And I'd dearly love a better name than 'folio_nth'.  page_nth() is
> > a temporary construct, so doesn't need a better name.  If you need
> > context, it's in the gup_folio_range_next() patch and its job
> > is to tell you, given a page and a folio, what # page it is within
> > a folio (so a number between [0 and folio_nr_pages())).
> 
> folio_tail_index() ?

I'm a little wary of "index" because folios are used to cache file data,
and folio->index means offset of the folio within the file.  We could
make the argument for folio_page_index() (since it might be the head
page, not a tail page), and argue this is the index into the folio,
not the index into the file.

It's better than folio_nth ;-)

> Series still looks good to me, though I checked each patch
> carefully than the prior series:
> 
> Reviewed-by: Jason Gunthorpe <jgg@xxxxxxxxxx>

Thanks!