Re: [GIT PULL] arm64 updates for 6.13-rc1

[David Hildenbrand]

On 28.11.24 15:12, Catalin Marinas wrote:
> On Wed, Nov 27, 2024 at 05:21:37PM -0800, Yang Shi wrote:
> > > > diff --git a/arch/arm64/mm/copypage.c b/arch/arm64/mm/copypage.c
> > > > index 87b3f1a25535..ef303a2262c5 100644
> > > > --- a/arch/arm64/mm/copypage.c
> > > > +++ b/arch/arm64/mm/copypage.c
> > > > @@ -30,9 +30,9 @@ void copy_highpage(struct page *to, struct page *from)
> > > >     	if (!system_supports_mte())
> > > >     		return;
> > > > -	if (folio_test_hugetlb(src) &&
> > > > -	    folio_test_hugetlb_mte_tagged(src)) {
> > > > -		if (!folio_try_hugetlb_mte_tagging(dst))
> > > > +	if (folio_test_hugetlb(src)) {
> > > > +		if (!folio_test_hugetlb_mte_tagged(src) ||
> > > > +		    !folio_try_hugetlb_mte_tagging(dst))
> > > >     			return;
> > > >     		/*
> > > I wonder why we had a 'return' here originally rather than a
> > > WARN_ON_ONCE() as we do further down for the page case. Do you seen any
> > > issue with the hunk below? Destination should be a new folio and not
> > > tagged yet:
> >
> > Yes, I did see problem. Because we copy tags for all sub pages then set
> > folio mte tagged when copying the data for the first subpage. The warning
> > will be triggered when we copy the second subpage.
>
> Ah, good point, copy_highpage() will be called multiple times for each
> subpage but we only do the copying once for the folio.

It makes me still a bit nervous that we assume both the src and the 
destination folio have the same #pages (and in particular, that both are 
hugetlb folios :) ).

Hopefully that's an invariant that will always hold :)

> Now, I wonder whether we should actually defer the tag copying until
> copy_page() is called on the head page. This way we can keep the warning
> for consistency with the non-compound page case:
>
> diff --git a/arch/arm64/mm/copypage.c b/arch/arm64/mm/copypage.c
> index 87b3f1a25535..a86c897017df 100644
> --- a/arch/arm64/mm/copypage.c
> +++ b/arch/arm64/mm/copypage.c
> @@ -30,11 +30,13 @@ void copy_highpage(struct page *to, struct page *from)
>   	if (!system_supports_mte())
>   		return;
>   
> -	if (folio_test_hugetlb(src) &&
> -	    folio_test_hugetlb_mte_tagged(src)) {
> -		if (!folio_try_hugetlb_mte_tagging(dst))
> +	if (folio_test_hugetlb(src)) {
> +		if (!folio_test_hugetlb_mte_tagged(src) ||
> +		    from != folio_page(src, 0))
>   			return;
>   
> +		WARN_ON_ONCE(!folio_try_hugetlb_mte_tagging(dst));
> +
>   		/*
>   		 * Populate tags for all subpages.
>   		 *

Yes, looks better. A comment describing the oddity of "copy single page 
but copy all tags on head page access" might be reasonable.

--
Cheers,

David / dhildenb