On Fri, Dec 06, 2024 at 06:27:09AM +0100, Guillaume Morin wrote:
> On 05 Dec 21:50, Nathan Chancellor wrote:
> > > #ifdef CONFIG_PGTABLE_HAS_HUGE_LEAVES
> > > +/* FOLL_FORCE can write to even unwritable PUDs in COW mappings. */
> > > +static inline bool can_follow_write_pud(pud_t pud, struct page *page,
> > > + struct vm_area_struct *vma,
> > > + unsigned int flags)
> > > +{
> > > + /* If the pud is writable, we can write to the page. */
> > > + if (pud_write(pud))
> > > + return true;
> > > +
> > > + if (!can_follow_write_common(page, vma, flags))
> > > + return false;
> > > +
> > > + /* ... and a write-fault isn't required for other reasons. */
> > > + return !vma_soft_dirty_enabled(vma) || pud_soft_dirty(pud);
> >
> > This looks to be one of the first uses of pud_soft_dirty() in a generic
> > part of the tree from what I can tell, which shows that s390 is lacking
> > it despite setting CONFIG_HAVE_ARCH_SOFT_DIRTY:
> >
> > $ make -skj"$(nproc)" ARCH=s390 CROSS_COMPILE=s390-linux- mrproper defconfig mm/gup.o
> > mm/gup.c: In function 'can_follow_write_pud':
> > mm/gup.c:665:48: error: implicit declaration of function 'pud_soft_dirty'; did you mean 'pmd_soft_dirty'? [-Wimplicit-function-declaration]
> > 665 | return !vma_soft_dirty_enabled(vma) || pud_soft_dirty(pud);
> > | ^~~~~~~~~~~~~~
> > | pmd_soft_dirty
> >
> > Is this expected?
>
> Yikes! It does look like an oversight in the s390 code since as you said
> it has CONFIG_HAVE_ARCH_SOFT_DIRTY and pud_mkdirty seems to be setting
> _REGION3_ENTRY_SOFT_DIRTY. But I'll let the s390 folks opine.
>
> I don't mind dropping the pud part of the change (even if that's a bit
> of a shame) if it's causing too many issues.
It would be quite easy to add pud_soft_dirty() etc. helper functions
for s390, but I think that would be the wrong answer to this problem.
s390 implements pud_mkdirty(), but it is only used in the context of
HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD, which s390 doesn't support. So
this function should probably be removed from s390's pgtable.h.
Similar the pud_soft_dirty() and friends helper functions should only
be implemented if common code support for soft dirty would exist,
which is currently not the case. Otherwise similar fallbacks like for
pmd_soft_dirty() (-> include/linux/pgtable.h) would also need to be
implemented.
So IMHO the right fix (at this time) seems to be to remove the above
pud part of your patch, and in addition we should probably also drop
the partially implemented pud level soft dirty bits in s390 code,
since that is dead code and might cause even more confusion in future.
Does that make sense?
