Re: [PATCH 00/10] perf/uprobe: Optimize uprobes

[Peter Zijlstra <peterz@infradead.org>]

On Mon, Jul 22, 2024 at 12:09:21PM -0700, Suren Baghdasaryan wrote:
> On Wed, Jul 10, 2024 at 2:40 AM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > On Wed, Jul 10, 2024 at 11:16:31AM +0200, Peter Zijlstra wrote:
> >
> > > If it were an actual sequence count, I could make it work, but sadly,
> > > not. Also, vma_end_write() seems to be missing :-( If anything it could
> > > be used to lockdep annotate the thing.
> 
> Thanks Matthew for forwarding me this discussion!
> 
> > >
> > > Mooo.. I need to stare more at this to see if perhaps it can be made to
> > > work, but so far, no joy :/
> >
> > See, this is what I want, except I can't close the race against VMA
> > modification because of that crazy locking scheme :/
> 
> Happy to explain more about this crazy locking scheme. The catch is
> that we can write-lock a VMA only while holding mmap_lock for write
> and we unlock all write-locked VMAs together when we drop that
> mmap_lock:
> 
> mmap_write_lock(mm);
> vma_start_write(vma1);
> vma_start_write(vma2);
> ...
> mmap_write_unlock(mm); -> vma_end_write_all(mm); // unlocks all locked vmas
> 
> This is done because oftentimes we need to lock multiple VMAs when
> modifying the address space (vma merge/split) and unlocking them
> individually would be more expensive than unlocking them in bulk by
> incrementing mm->mm_lock_seq.

Right, but you can do that without having it quite this insane.

You can still make mm_lock_seq a proper seqcount, and still have
vma_end_write() -- even if its an empty stub only used for validation.

That is, something like the below, which adds a light barrier, ensures
that mm_lock_seq is a proper sequence count.

diff --git a/include/linux/mmap_lock.h b/include/linux/mmap_lock.h
index de9dc20b01ba..daa19d1a3022 100644
--- a/include/linux/mmap_lock.h
+++ b/include/linux/mmap_lock.h
@@ -104,6 +104,8 @@ static inline void mmap_write_lock(struct mm_struct *mm)
 {
 	__mmap_lock_trace_start_locking(mm, true);
 	down_write(&mm->mmap_lock);
+	WRITE_ONCE(mm->mm_lock_seq, mm->mm_lock_seq+1);
+	smp_wmb();
 	__mmap_lock_trace_acquire_returned(mm, true, true);
 }
 

With the above addition we could write (although I think we still need
the RCU_SLAB thing on files_cachep):

static struct uprobe *__find_active_uprobe(unsigned long bp_vaddr)
{
	struct mm_struct *mm = current->mm;
	struct uprobe *uprobe = NULL;
	struct vm_area_struct *vma;
	struct inode *inode;
	loff_t offset;
	int seq;

	guard(rcu)();

	seq = READ_ONCE(mm->mm_lock_seq);
	smp_rmb();
	do {
		vma = find_vma(mm, bp_vaddr);
		if (!vma)
			return NULL;

		if (!valid_vma(vma, false))
			return NULL;

		inode = file_inode(vma->vm_file);
		offset = vaddr_to_offset(vma, bp_vaddr);

	} while (smp_rmb(), seq != READ_ONCE(mm->mm_lock_seq));

	return find_uprobe(inode, offset);
}