Re: [RFC PATCH v2 1/3] mm: Don't pin ZERO_PAGE in pin_user_pages()

[Lorenzo Stoakes <lstoakes@gmail.com>]

On Fri, May 26, 2023 at 09:43:35AM +0100, David Howells wrote:
> Lorenzo Stoakes <lstoakes@gmail.com> wrote:
>
> > I guess we're not quite as concerned about FOLL_GET because FOLL_GET should
> > be ephemeral and FOLL_PIN (horrifically) adds GUP_PIN_COUNTING_BIAS each
> > time?
>
> It's not that - it's that iov_iter_get_pages*() is a lot more commonly used at
> the moment, and we'd have to find *all* the places that things using that hand
> refs around.
>
> iov_iter_extract_pages(), on the other hand, is only used in two places with
> these patches and the pins are always released with unpin_user_page*() so it's
> a lot easier to audit.

Thanks for the clarification. I guess these are the cases where you're likely to
see zero page usage, but since this is changing all PUP*() callers don't you
need to audit all of those too?

>
> I could modify put_page(), folio_put(), etc. to ignore the zero pages, but
> that might have a larger performance impact.
>
> > > +		if (is_zero_page(page))
> > > +			return page_folio(page);
> > > +
> >
> > This will capture huge page cases too which have folio->_pincount and thus
> > don't suffer the GUP_PIN_COUNTING_BIAS issue, however it is equally logical
> > to simply skip these when pinning.
>
> I'm not sure I understand.  The zero page(s) is/are single-page folios?

I'm actually a little unsure of how huge zero pages are handled (not an area I'm
hugely familiar with) so this might just be mistaken, I mean the point was more
so that hugetlb calls into this, but seems then not an issue.

>
> > This does make me think that we should just skip pinning for FOLL_GET cases
> > too - there's literally no sane reason we should be pinning zero pages in
> > any case (unless I'm missing something!)
>
> As mentioned above, there's a code auditing issue and a potential performance
> issue, depending on how it's done.

Ack, makes sense. It'd be good to have this documented somewhere though in
commit msg or docs so this trade-off is clear.

>
> > Another nitty thing that I noticed is, in is_longterm_pinnable_page():-
> >
> > 	/* The zero page may always be pinned */
> > 	if (is_zero_pfn(page_to_pfn(page)))
> > 		return true;
> >
> > Which, strictly speaking I suppose we are 'pinning' it or rather allowing
> > the pin to succeed without actually pinning, but to be super pedantic
> > perhaps it's worth updating this comment too.
>
> Yeah.  It is "pinnable" but no pin will actually be added.

The comment striks me as misleading, previously it literally meant that you
could pin the zero page. Now it means that we just don't. I do think for the
sake of avoiding confusion this should be tweaked.

Obviously something of a nit, however!

I did dig into this change a fair bit and kept adding then deleting comments
since you cover all the bases well, so overall this is nice + I can but nit it
:) Nice to see further improvements to GUP which is crying out for that.

>
> David
>
>