Re: [PATCH] hugetlbfs: add MTE support

[Catalin Marinas <catalin.marinas@arm.com>]

On Wed, Jul 03, 2024 at 12:24:40PM +0200, David Hildenbrand wrote:
> On 03.07.24 02:20, Yang Shi wrote:
> > On 7/2/24 6:09 AM, David Hildenbrand wrote:
> > > On 02.07.24 14:34, Catalin Marinas wrote:
> > > > On Tue, Jun 25, 2024 at 04:37:17PM -0700, Yang Shi wrote:
> > > > > MTE can be supported on ram based filesystem. It is supported on tmpfs.
> > > > > There is use case to use MTE on hugetlbfs as well, adding MTE support.
> > > > > 
> > > > > Signed-off-by: Yang Shi <yang@os.amperecomputing.com>
> > > > > ---
> > > > >    fs/hugetlbfs/inode.c | 2 +-
> > > > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> > > > > index ecad73a4f713..c34faef62daf 100644
> > > > > --- a/fs/hugetlbfs/inode.c
> > > > > +++ b/fs/hugetlbfs/inode.c
> > > > > @@ -110,7 +110,7 @@ static int hugetlbfs_file_mmap(struct file
> > > > > *file, struct vm_area_struct *vma)
> > > > >         * way when do_mmap unwinds (may be important on powerpc
> > > > >         * and ia64).
> > > > >         */
> > > > > -    vm_flags_set(vma, VM_HUGETLB | VM_DONTEXPAND);
> > > > > +    vm_flags_set(vma, VM_HUGETLB | VM_DONTEXPAND | VM_MTE_ALLOWED);
> > > > >        vma->vm_ops = &hugetlb_vm_ops;
> > > > 
> > > > Last time I checked, about a year ago, this was not sufficient. One
> > > > issue is that there's no arch_clear_hugetlb_flags() implemented by your
> > > > patch, leaving PG_arch_{2,3} set on a page. The other issue was that I
> > > > initially tried to do this only on the head page but this did not go
> > > > well with the folio_copy() -> copy_highpage() which expects the
> > > > PG_arch_* flags on each individual page. The alternative was for
> > > > arch_clear_hugetlb_flags() to iterate over all the pages in a folio.
> > > 
> > > This would likely also add a blocker for
> > > ARCH_WANT_OPTIMIZE_HUGETLB_VMEMMAP on arm64 (no idea if there are now
> > > ways to move forward with that now, or if we are still not sure if we
> > > can actually add support), correct?
> > 
> > IIUC, it is not. We just need to guarantee each subpage has
> > PG_mte_tagged flag and allocated tags. The HVO just maps the 7 vmemmap
> > pages for sub pages to the first page, they still see the flag and the
> > space for tag is not impacted, right? Did I miss something?
> 
> In the R/O vmemmap optimization we won't be able to modify the flags of the
> double-mapped vmemmap pages via the double mappings.
> 
> Of course, we could find HVO-specific ways to only modify the flags of the
> first vmemmap page, but it does sound wrong ...
> 
> Really, the question is if we can have a per-folio flag for hugetlb instead
> and avoid all that?

I think it is possible and I have some half-baked changes but got
distracted and never completed. The only issue I came across was
folio_copy() calling copy_highpage() on individual pages that did not
have the original PG_mte_tagged (PG_arch_2) flag. To avoid some races,
we also use PG_mte_lock (PG_arch_3) as some form of locking but for
optimisation we don't clear this flag after copying the tags and setting
PG_mte_tagged. So doing the checks on the head page only confuses the
tail page copying.

Even if we use PG_arch_3 as a proper lock bit and clear it after
tag copying, I'll need to check whether this can race with any
mprotect(PROT_MTE) that could cause losing tags or leaking tags (not
initialising the pages). set_pte_at() relies on the PG_mte_tagged flag
to decide whether to initialise the tags. The arm64 hugetlbfs supports
contiguous ptes, so we'd get multiple set_pte_at() calls.

Anyway, I think with some care it is doable, I just did not have the
time, nor did I see anyone asking for such feature until now.

-- 
Catalin