On Mon, Apr 24, 2023 at 09:43:28PM -0400, Liam R. Howlett wrote: > * Carlos Llamas <cmllamas@xxxxxxxxxx> [230424 19:11]: > > > > The specifics are in the third patch of this patchset but the gist of it > > is that during ->mmap() handler, binder will complete the initialization > > of the binder_alloc structure. With the last step of this process being > > the caching of the vma pointer. Since the ordering is protected with a > > barrier we can then check alloc->vma to determine if the initialization > > has been completed. > > > > Since this check is part of the critical path for every single binder > > transaction, the performance plummeted when we started contending for > > the mmap_lock. In this particular case, binder doesn't actually use the > > vma. > > So why does binder_update_page_range() take the mmap_read_lock then use > the cached vma in the reverted patch? > > If you want to use it as a flag to see if the driver is initialized, why > not use the cached address != 0? > > Or better yet, > > >It only needs to know if the internal structure has been fully > > initialized and it is safe to use it. > > This seems like a good reason to use your own rwsem. This is, > essentially, rolling your own lock with > smp_store_release()/smp_load_acquire() and a pointer which should not be > cached. We can't use an rwsem to protect the initialization. We already have an alloc->mutex which would be an option. However, using it under ->mmap() would only lead to dead-locks with the mmap_lock. I agree with you that we could use some other flag instead of the vma pointer to signal the initialization. I've actually tried several times to come up with a scenario in which caching the vma pointer becomes an issue to stop doing this altogether. However, I can't find anything concrete. I don't think the current solution in which we do all these unnecessary vma lookups is correct. Instead, I'm currently working on a redesign of this section in which binder stops to allocate/insert pages manually. We should be making use of the page-fault handler and let the infra handle all the work. The overall idea is here: https://lore.kernel.org/all/ZEGh4mliGHvyWIvo@xxxxxxxxxx/ It's hard to make the case for just dropping the vma pointer after ~15 years and take the performance hit without having an actual issue to support this idea. So I'll revert this for now and keep working on the page-fault solution. Thanks Liam, I'll keep you in the loop.
