On Mon, 16 Mar 2026 09:45:49 +0100 Thomas Zimmermann <[email protected]> wrote:
> Hi Boris, > > thanks for investigating this problem. > > Am 13.03.26 um 18:45 schrieb Boris Brezillon: > > On Fri, 13 Mar 2026 13:55:21 +0100 > > Boris Brezillon <[email protected]> wrote: > > > >> On Fri, 13 Mar 2026 13:43:28 +0100 > >> Boris Brezillon <[email protected]> wrote: > >> > >>> On Fri, 13 Mar 2026 13:18:35 +0100 > >>> Boris Brezillon <[email protected]> wrote: > >>> > >>>> On Fri, 13 Mar 2026 12:04:25 +0000 > >>>> Biju Das <[email protected]> wrote: > >>>> > >>>>>> -----Original Message----- > >>>>>> From: dri-devel <[email protected]> On Behalf Of > >>>>>> Boris Brezillon > >>>>>> Sent: 13 March 2026 11:57 > >>>>>> Subject: Re: [PATCH v4 5/6] drm/gem-shmem: Track folio accessed/dirty > >>>>>> status in mmap > >>>>>> > >>>>>> On Fri, 13 Mar 2026 11:29:47 +0100 > >>>>>> Thomas Zimmermann <[email protected]> wrote: > >>>>>> > >>>>>>> Hi > >>>>>>> > >>>>>>> Am 13.03.26 um 11:18 schrieb Boris Brezillon: > >>>>>>> [...] > >>>>>>>>>>>> + if (drm_WARN_ON(obj->dev, !shmem->pages || page_offset > >>>>>>>>>>>> >= num_pages)) > >>>>>>>>>>>> + return VM_FAULT_SIGBUS; > >>>>>>>>>>>> + > >>>>>>>>>>>> + file_update_time(vma->vm_file); > >>>>>>>>>>>> + > >>>>>>>>>>>> + > >>>>>>>>>>>> folio_mark_dirty(page_folio(shmem->pages[page_offset])); > >>>>>>>> Do we need a folio_mark_dirty_lock() here? > >>>>>>> There is a helper for that with some documentation. [1] > >>>>>> This [1] seems to solve the problem for me. Still unsure about the > >>>>>> folio_mark_dirty_lock vs > >>>>>> folio_mark_dirty though. > >>>>>> > >>>>>> [1]https://yhbt.net/lore/dri-devel/[email protected]/ > >>>>>> > >>>>> FYI, I used folio_mark_dirty_lock() still it does not solve the issue > >>>>> with weston hang. > >>>> The patch I pointed to has nothing to do with folio_mark_dirty_lock(), > >>>> It's a bug caused by huge page mapping changes. > >>> Scratch that. I had a bunch of other changes on top, and it hangs again > >>> now that I dropped those. > >> Seems like it's the combination of huge pages and mkwrite that's > >> causing issues, if I disable huge pages, it doesn't hang... > > I managed to have it working with the following diff. I still need to > > check why the "map-RO-split+RW-on-demand" approach doesn't work (races > > between huge_fault and pfn_mkwrite?), but I think it's okay to map the > > real thing writable on the first attempt anyway (we're not trying to do > > CoW here, since we're always pointing to the same page, it's just the > > permissions that change). Note that there's still the race fixed by > > https://yhbt.net/lore/dri-devel/[email protected]/ > > in this diff, I just tried to keep the diffstat minimal. > > > > --->8--- > > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c > > b/drivers/gpu/drm/drm_gem_shmem_helper.c > > index 4500deef4127..4efdce5a60f0 100644 > > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c > > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c > > @@ -561,9 +561,8 @@ static vm_fault_t > > drm_gem_shmem_try_insert_pfn_pmd(struct vm_fault *vmf, unsigne > > bool aligned = (vmf->address & ~PMD_MASK) == (paddr & ~PMD_MASK); > > > > if (aligned && pmd_none(*vmf->pmd)) { > > - /* Read-only mapping; split upon write fault */ > > pfn &= PMD_MASK >> PAGE_SHIFT; > > - return vmf_insert_pfn_pmd(vmf, pfn, false); > > + return vmf_insert_pfn_pmd(vmf, pfn, vmf->flags & > > FAULT_FLAG_WRITE); > > } > > #endif > > > > @@ -597,8 +596,12 @@ static vm_fault_t drm_gem_shmem_fault(struct vm_fault > > *vmf) > > > > pfn = page_to_pfn(page); > > > > - if (folio_test_pmd_mappable(folio)) > > + if (folio_test_pmd_mappable(folio)) { > > ret = drm_gem_shmem_try_insert_pfn_pmd(vmf, pfn); > > + if (ret == VM_FAULT_NOPAGE && vmf->flags & FAULT_FLAG_WRITE) > > + folio_mark_dirty(folio); > > + } > > + > > if (ret != VM_FAULT_NOPAGE) > > ret = vmf_insert_pfn(vma, vmf->address, pfn); > > All these branches with NOPAGE seem confusing. Can we change the overall > logic here? Something like: > > if (folio_test_pmd_mappable()) { > ret = try_insert_pfn_pmd() > if (ret == VM_FAULT_NOPAGE) { > folio_mark_accessed() > if (flags & FLAG_WRITE) > folio_mark_dirty() > goto out; > } > } > > ret = vmf_insert_pfn() > if (ret == NOPAGE) > folio_mark_accesed() > > out: > ... > > > This would keep the huge-page code within the first branch. And if it > fails, it still does the regular page fault. Well, in practice that's not what we want anyway (see the other fix for the huge_fault vs regular fault race), so I'd still be inclined to have the folio_mark_accessed() handled in the common path and have the pmd vs regular pfn insertion in some if/else branches. Something like that: if (pmd_insert) ret = drm_gem_shmem_try_insert_pfn_pmd(vmf, pfn); else ret = vmf_insert_pfn(vma, vmf->address, pfn); if (ret == VM_FAULT_NOPAGE) { folio_mark_accesed(folio); /* Normal pages are mapped RO, and remapped RW afterwards. */ if (pmd_insert && vmf->flags & FAULT_FLAG_WRITE) folio_mark_dirty(folio); }
