Re: [PATCH v3] mm/hugetlb: support FOLL_FORCE|FOLL_WRITE

[Guillaume Morin <guillaume@morinfr.org>]

On 06 Dec 10:29, David Hildenbrand wrote:
>
> On 06.12.24 10:24, Heiko Carstens wrote:
> > On Fri, Dec 06, 2024 at 06:27:09AM +0100, Guillaume Morin wrote:
> > > On 05 Dec 21:50, Nathan Chancellor wrote:
> > > > 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?
> 
> As hugetlb does not support softdirty, and PUDs are currently only possible
> (weird DAX thing put aside) with hugetlb, it makes sense to drop the pud
> softdirty thingy.

Thanks all. I dropped the check and the dummy definition I had to add
for i386 in v4 [1]

[1] https://lore.kernel.org/linux-mm/Z1MO5slZh8uWl8LH@bender.morinfr.org/T/#u

-- 
Guillaume Morin <guillaume@morinfr.org>