Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Received-SPF: Pass (protection.outlook.com: domain of nvidia.com designates
 12.22.5.235 as permitted sender) receiver=protection.outlook.com;
 client-ip=12.22.5.235; helo=mail.nvidia.com;
From:   Alistair Popple <apopple@nvidia.com>
To:     <akpm@linux-foundation.org>, <Felix.Kuehling@amd.com>,
        <linux-mm@kvack.org>, <rcampbell@nvidia.com>,
        <linux-ext4@vger.kernel.org>, <linux-xfs@vger.kernel.org>,
        Alex Sierra <alex.sierra@amd.com>
CC:     <amd-gfx@lists.freedesktop.org>, <dri-devel@lists.freedesktop.org>,
        <hch@lst.de>, <jgg@nvidia.com>, <jglisse@redhat.com>,
        <willy@infradead.org>
Subject: Re: [PATCH v4 01/10] mm: add zone device coherent type memory support
Date:   Fri, 28 Jan 2022 12:57:40 +1100
Message-ID: <3321052.0DCqWhyCST@nvdebian>
In-Reply-To: <20220127030949.19396-2-alex.sierra@amd.com>
References: <20220127030949.19396-1-alex.sierra@amd.com> <20220127030949.19396-2-alex.sierra@amd.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Jan 2022 01:57:46.3103
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: e79e3e1a-b0c3-4024-f6f7-08d9e2019486
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=43083d15-7273-40c1-b7db-39efd9ccc17a;Ip=[12.22.5.235];Helo=[mail.nvidia.com]
X-MS-Exchange-CrossTenant-AuthSource: CO1NAM11FT037.eop-nam11.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR12MB3198
Precedence: bulk
List-ID: <linux-ext4.vger.kernel.org>
X-Mailing-List: linux-ext4@vger.kernel.org

On Thursday, 27 January 2022 2:09:40 PM AEDT Alex Sierra wrote:

[...]

> diff --git a/mm/migrate.c b/mm/migrate.c
> index 1852d787e6ab..277562cd4cf5 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -362,7 +362,7 @@ static int expected_page_refs(struct address_space
> *mapping, struct page *page)> 
>          * Device private pages have an extra refcount as they are
>          * ZONE_DEVICE pages.
>          */
> 
> -       expected_count += is_device_private_page(page);
> +       expected_count += is_dev_private_or_coherent_page(page);
> 
>         if (mapping)
>         
>                 expected_count += thp_nr_pages(page) +
>                 page_has_private(page);
> 
> @@ -2503,7 +2503,7 @@ static bool migrate_vma_check_page(struct page *page)
> 
>                  * FIXME proper solution is to rework migration_entry_wait()
>                  so
>                  * it does not need to take a reference on page.
>                  */
> 
> -               return is_device_private_page(page);
> +               return is_dev_private_or_coherent_page(page);

As Andrew points out this no longer applies due to changes here. I think you
can just drop this hunk though.

[...]

> diff --git a/mm/rmap.c b/mm/rmap.c
> index 6aebd1747251..32dae6839403 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1823,10 +1823,17 @@ static bool try_to_migrate_one(struct page *page, struct vm_area_struct *vma,
>  			 * pteval maps a zone device page and is therefore
>  			 * a swap pte.
>  			 */
> -			if (pte_swp_soft_dirty(pteval))
> -				swp_pte = pte_swp_mksoft_dirty(swp_pte);
> -			if (pte_swp_uffd_wp(pteval))
> -				swp_pte = pte_swp_mkuffd_wp(swp_pte);
> +			if (is_device_coherent_page(page)) {
> +				if (pte_soft_dirty(pteval))
> +					swp_pte = pte_swp_mksoft_dirty(swp_pte);
> +				if (pte_uffd_wp(pteval))
> +					swp_pte = pte_swp_mkuffd_wp(swp_pte);
> +			} else {
> +				if (pte_swp_soft_dirty(pteval))
> +					swp_pte = pte_swp_mksoft_dirty(swp_pte);
> +				if (pte_swp_uffd_wp(pteval))
> +					swp_pte = pte_swp_mkuffd_wp(swp_pte);
> +			}

As I understand things ptes for device coherent pages don't need special
treatment, therefore rather than special casing here it should just fall
through to the same path as normal pages. For that I think all you need is
something like:

-                if (is_zone_device_page(page)) {
+                if (is_device_private_page(page)) {

Noting that device private pages are the only zone device pages that could
have been encountered here anyway.

>  			set_pte_at(mm, pvmw.address, pvmw.pte, swp_pte);
>  			/*
>  			 * No need to invalidate here it will synchronize on
> @@ -1837,7 +1844,7 @@ static bool try_to_migrate_one(struct page *page, struct vm_area_struct *vma,
>  			 * Since only PAGE_SIZE pages can currently be
>  			 * migrated, just set it to page. This will need to be
>  			 * changed when hugepage migrations to device private
> -			 * memory are supported.
> +			 * or coherent memory are supported.
>  			 */
>  			subpage = page;
>  		} else if (PageHWPoison(page)) {
> @@ -1943,7 +1950,8 @@ void try_to_migrate(struct page *page, enum ttu_flags flags)
>  					TTU_SYNC)))
>  		return;
>  
> -	if (is_zone_device_page(page) && !is_device_private_page(page))
> +	if (is_zone_device_page(page) &&
> +	    !is_dev_private_or_coherent_page(page))
>  		return;
>  
>  	/*
>