Re: [PATCH v2 2/6] drm/shmem_helper: use vmf_insert_pfn_mkwrite()

[Boris Brezillon <boris.brezillon@collabora.com>]

On Wed, 5 Aug 2026 08:08:17 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On Tue, Aug 4, 2026 at 4:42 PM Boris Brezillon
> <boris.brezillon@collabora.com> wrote:
> > Okay, I thought I'd ask to be sure, because of all the implementations
> > of pfn_mkwrite listed here [1], only drm_gem_shmem_helper.c and
> > kernel/events/core.c do that.  
> 
> Also the vmwgfx one in patch 3; and for the !DAX case ext4 and XFS too
> (e.g. xfs_filemap_pfn_mkwrite gets to iomap_page_mkwrite, not to
> filemap_fault).

Right, iomap_page_mkwrite() doesn't seem to update the PTE entry, it
just records the access. But there's quite a bit of locking taking place
before this recording is done, which we don't do in gem_shmem's
pkf_mkwrite implementation. This brings me back to some question I asked
in another thread where we were discussing another regression
introduced by pfn_mkwrite addition to gem_shmem [1]: do we need to have
the folio locked when recording the mkwrite? If we do, we probably need
to surround the drm_gem_shmem_record_mkwrite() call in
drm_gem_shmem_pfn_mkwrite() with a folio_lock/unlock() sequence.

For the record, the folio_mark_dirty() doc says:

 * The folio may not be truncated while this function is running.
 * Holding the folio lock is sufficient to prevent truncation, but some
 * callers cannot acquire a sleeping lock.  These callers instead hold
 * the page table lock for a page table which contains at least one page
 * in this folio.  Truncation will block on the page table lock as it
 * unmaps pages before removing the folio from its mapping.

I'm really sorry to hijack this thread like that, but now that I have
people with a bit more MM knowledge looking at this stuff, I'm taking
the opportunity to ask all the questions that were left unanswered back
then :-/.

> 
> > The rest have their "generic" fault
> > handler (by generic I mean a fault handler helper that covers all the
> > order/WRITE_FLAG combinations) called from pfn_mkwrite(), and return a
> > non-zero vm_fault_t.  
> 
> I see; I think you can do that, it is handled at
> https://elixir.bootlin.com/linux/v7.2-rc5/source/mm/memory.c#L2681 and
> indeed it returns VM_FAULT_NOPAGE. But it's more or work for no real
> reason. Your .pfn_mkwrite is a single line of code and it is clearer
> IMO if you can see that try_insert_pfn() is calling the same helper as
> .pfn_mkwrite().

Sure. As long as what we're doing is safe, I'm fine keeping
drm_gem_shmem_pfn_mkwrite() as is and letting the core update the PTE
props. It's just that, after reading all these implementations, I was
skeptical (see the question around folio locking before mark_dirty(),
for instance).

Anyway, thanks for chiming in.

[1]https://lore.kernel.org/dri-devel/20260313111851.4c1f89f3@fedora/