Lorenzo Stoakes <lstoakes@xxxxxxxxx> 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. 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? > 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. > 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. David
