On 4/19/23 10:35?AM, Jens Axboe wrote: > On 4/18/23 9:49?AM, Lorenzo Stoakes wrote: >> We are shortly to remove pin_user_pages(), and instead perform the required >> VMA checks ourselves. In most cases there will be a single VMA so this >> should caues no undue impact on an already slow path. >> >> Doing this eliminates the one instance of vmas being used by >> pin_user_pages(). > > First up, please don't just send single patches from a series. It's > really annoying when you are trying to get the full picture. Just CC the > whole series, so reviews don't have to look it up separately. > > So when you're doing a respin for what I'll mention below and the issue > that David found, please don't just show us patch 4+5 of the series. I'll reply here too rather than keep some of this conversaion out-of-band. I don't necessarily think that making io buffer registration dumber and less efficient by needing a separate vma lookup after the fact is a huge deal, as I would imagine most workloads register buffers at setup time and then don't change them. But if people do switch sets at runtime, it's not necessarily a slow path. That said, I suspect the other bits that we do in here, like the GUP, is going to dominate the overhead anyway. My main question is, why don't we just have a __pin_user_pages or something helper that still takes the vmas argument, and drop it from pin_user_pages() only? That'd still allow the cleanup of the other users that don't care about the vma at all, while retaining the bundled functionality for the case/cases that do? That would avoid needing explicit vma iteration in io_uring. -- Jens Axboe
