On Mon, Jul 31, 2023 at 11:02 AM Linus Torvalds torvalds@linux-foundation.org wrote:
On Mon, 31 Jul 2023 at 10:12, Suren Baghdasaryan surenb@google.com wrote:
walk_page_vma(vma, &subpage_walk_ops, NULL);
walk_page_vma(vma, &subpage_walk_ops, true, NULL);Rather than add a new argument to the walk_page_*() functions, I really think you should just add the locking rule to the 'const struct mm_walk_ops' structure.
The locking rule goes along with the rules for what you are actually doing, after all. Plus it would actually make it all much more legible when it's not just some random "true/false" argument, but a an actual
.write_lock = 1in the ops definition.
Yeah, I was thinking about that but thought a flag like this in a pure "ops" struct would be frowned upon. If this is acceptable then it makes it much cleaner.
Yes, yes, that might mean that some ops might need duplicating in case you really have a walk that sometimes takes the lock, and sometimes doesn't, but that is odd to begin with.
The only such case I found from a quick look was the very strange queue_pages_range() case. Is it really true that do_mbind() needs the write-lock, but do_migrate_pages() does not?
And if they really are that different maybe they should have different walk_ops?
Makes sense to me.
Maybe there were other cases that I didn't notice.
error = walk_page_range(current->mm, start, end,
&prot_none_walk_ops, &new_pgprot);
&prot_none_walk_ops, true, &new_pgprot);This looks odd. You're adding vma locking to a place that didn't do it before.
Yes, the mmap semaphore is held for writing, but this particular walk doesn't need it as far as I can tell.
Yes you are correct. Locking a vma in this case seems unnecessary.
In fact, this feels like that walker should maybe *verify* that it's held for writing, but not try to write it again?
In this particular case, does this walk even require the vma to be write locked? Looks like it's simply checking the ptes. And if so, walk_page_range() already has mmap_assert_locked(walk.mm) at the beginning to ensure the tree is stable. Do we need anything else here?
Maybe the "lock_vma" flag should be a tri-state:
- lock for reading (no-op per vma), verify that the mmap sem is held
for reading
- lock for reading (no-op per vma), but with mmap sem held for
writing (this kind of "check before doing changes" walker)
- lock for writing (with mmap sem obviously needs to be held for writing)
mmap_assert_locked(walk.mm);
if (lock_vma)vma_start_write(vma);So I think this should also be tightened up, and something like
switch (ops->locking) { case WRLOCK: vma_start_write(vma); fallthrough; case WRLOCK_VERIFY: mmap_assert_write_locked(mm); break; case RDLOCK: mmap_assert_locked(walk.mm); }
I got the idea but a couple of modifications, if I may. walk_page_range() already does mmap_assert_locked() at the beginning, so we can change it to:
if (ops->locking == RDLOCK) mmap_assert_locked(walk.mm); else mmap_assert_write_locked(mm);
and during the walk:
switch (ops->locking) { case WRLOCK: vma_start_write(vma); break; #ifdef CONFIG_PER_VMA_LOCK case WRLOCK_VERIFY: vma_assert_write_locked(vma); break; #endif }
The above CONFIG_PER_VMA_LOCK is ugly but with !CONFIG_PER_VMA_LOCK vma_assert_write_locked() becomes mmap_assert_write_locked() and we already checked that, so it's unnecessary.
because we shouldn't have a 'vma_start_write()' without holding the mmap sem for *writing*, and the above would also allow that mprotect_fixup() "walk to see if we can merge, verify that it was already locked" thing.
Hmm?
NOTE! The above names are just completely made up. I dcon't think it should actually be some "WRLOCK" enum. There are probably much better names. Take the above as a "maybe something kind of in this direction" rather than "do it exactly like this".
I'm not great with names... Maybe just add a PGWALK_ prefix like this:
PGWALK_RDLOCK PGWALK_WRLOCK PGWALK_WRLOCK_VERIFY
?
Linus