On 02.05.23 20:17, Lorenzo Stoakes wrote:
On Tue, May 02, 2023 at 07:34:06PM +0200, David Hildenbrand wrote:
On 02.05.23 19:22, Peter Zijlstra wrote:
On Tue, May 02, 2023 at 07:13:49PM +0200, David Hildenbrand wrote:
[...]
+{
+ struct address_space *mapping;
+
+ /*
+ * GUP-fast disables IRQs - this prevents IPIs from causing page tables
+ * to disappear from under us, as well as preventing RCU grace periods
+ * from making progress (i.e. implying rcu_read_lock()).
+ *
+ * This means we can rely on the folio remaining stable for all
+ * architectures, both those that set CONFIG_MMU_GATHER_RCU_TABLE_FREE
+ * and those that do not.
+ *
+ * We get the added benefit that given inodes, and thus address_space,
+ * objects are RCU freed, we can rely on the mapping remaining stable
+ * here with no risk of a truncation or similar race.
+ */
+ lockdep_assert_irqs_disabled();
+
+ /*
+ * If no mapping can be found, this implies an anonymous or otherwise
+ * non-file backed folio so in this instance we permit the pin.
+ *
+ * shmem and hugetlb mappings do not require dirty-tracking so we
+ * explicitly whitelist these.
+ *
+ * Other non dirty-tracked folios will be picked up on the slow path.
+ */
+ mapping = folio_mapping(folio);
+ return !mapping || shmem_mapping(mapping) || folio_test_hugetlb(folio);
"Folios in the swap cache return the swap mapping" -- you might disallow
pinning anonymous pages that are in the swap cache.
I recall that there are corner cases where we can end up with an anon page
that's mapped writable but still in the swap cache ... so you'd fallback to
the GUP slow path (acceptable for these corner cases, I guess), however
especially the comment is a bit misleading then.
So I'd suggest not dropping the folio_test_anon() check, or open-coding it
... which will make this piece of code most certainly easier to get when
staring at folio_mapping(). Or to spell it out in the comment (usually I
prefer code over comments).
So how stable is folio->mapping at this point? Can two subsequent reads
get different values? (eg. an actual mapping and NULL)
If so, folio_mapping() itself seems to be missing a READ_ONCE() to avoid
the compiler from emitting the load multiple times.
I can only talk about anon pages in this specific call order here (check
first, then test if the PTE changed in the meantime): we don't care if we
get two different values. If we get a different value the second time,
surely we (temporarily) pinned an anon page that is no longer mapped (freed
in the meantime). But in that case (even if we read garbage folio->mapping
and made the wrong call here), we'll detect afterwards that the PTE changed,
and unpin what we (temporarily) pinned. As folio_test_anon() only checks two
bits in folio->mapping it's fine, because we won't dereference garbage
folio->mapping.
With folio_mapping() on !anon and READ_ONCE() ... good question. Kirill said
it would be fairly stable, but I suspect that it could change (especially if
we call it before validating if the PTE changed as I described further
below).
Now, if we read folio->mapping after checking if the page we pinned is still
mapped (PTE unchanged), at least the page we pinned cannot be reused in the
meantime. I suspect that we can still read "NULL" on the second read. But
whatever we dereference from the first read should still be valid, even if
the second read would have returned NULL ("rcu freeing").
On a specific point - if mapping turns out to be NULL after we confirm
stable PTE, I'd be inclined to reject and let the slow path take care of
it, would you agree that that's the correct approach?
If it's not an anon page and the mapping is NULL, I'd say simply
fallback to the slow path.
--
Thanks,
David / dhildenb
