Received-SPF: pass (google.com: domain of linux-kernel+bounces-10877-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <a77781a4-2d3c-403a-88a6-57242c7f0246@intel.com>
Date: Mon, 25 Dec 2023 10:59:16 +0800
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC 1/3] iommu/intel: Use page->refcount to count number of
 entries in IOMMU
To: Pasha Tatashin <pasha.tatashin@soleen.com>, <akpm@linux-foundation.org>,
	<linux-mm@kvack.org>, <linux-kernel@vger.kernel.org>, <rientjes@google.com>,
	<dwmw2@infradead.org>, <baolu.lu@linux.intel.com>, <joro@8bytes.org>,
	<will@kernel.org>, <robin.murphy@arm.com>, <iommu@lists.linux.dev>
References: <20231221031915.619337-1-pasha.tatashin@soleen.com>
 <20231221031915.619337-2-pasha.tatashin@soleen.com>
Content-Language: en-US
From: "Liu, Jingqi" <jingqi.liu@intel.com>
In-Reply-To: <20231221031915.619337-2-pasha.tatashin@soleen.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?czBUS0ZCMEM4Zzc2Yk9CMXJDanhsQkU4K01PZGcwQWxYV3JTeENrbWlUY2Z1?=
 =?utf-8?B?ak1XdGhSTjRhVnBZUUlWQ2x0L1dGUXJIM0xqWDUvemNMK2RGclkwT25hUTVy?=
 =?utf-8?B?dFB5WGlHYWF2Vjh0TU5qeTBncld2Um5sVW4zeWpJMXRydXZ6aWJha3h5aFJs?=
 =?utf-8?B?NXl6NUEwZmRlbEVlSzJBcU1DV005bGcrSFpXMmp1VWRWQVBVTGpjU2JKcndJ?=
 =?utf-8?B?RDhsRnpGN2pTUWoxODZUUzFJZ2JuaDc3cGxzWkY5T2F4SmovU1NoSnMzWHRP?=
 =?utf-8?B?QUR5eG9iQnBZZ2lXWncvdU8yaHBDa3YwYjNxcmkwNTUxbjViWFhXdjZ1Nnlz?=
 =?utf-8?B?N00xYjQ3d2ZWelcrUGNISUZjM1ZFZlpHOU84S2p3c1dmK2thVCtKNFlJV1B6?=
 =?utf-8?B?L1J1RGhYYW11SUFSaWFxL3VpYWhLMUkvNlVuaElQTXVKcXBUWVpGV1p1YmJF?=
 =?utf-8?B?MEpGU2xZWVREbDF3RWJnS3RmcHdxaHNkRi9XelcwMXdYSmU0STdrajJlTlI3?=
 =?utf-8?B?UitUSDdQQ3I3dHd3SkhzZkJ0NkpIUmZjSHJSR2xHeUFaUkZGalUvNjhINEd4?=
 =?utf-8?B?ems5NHk1NWVkUnpSK0doRS8vaGRtc0YwOXVML3pNNExPVjJiRDN3c2Q1TG9k?=
 =?utf-8?B?SFFZamw5bjJxMHlzSFp6QXUySlJkNldUY1UxTnd6bkIrT29KUWVMcnBFSWV4?=
 =?utf-8?B?VGNhelpBNW4ydWRXZExQUjZzcHZMQ0hvaDZRZkxLa1pCb3d6RU1HYVNRYUpy?=
 =?utf-8?B?OEtjQTZjblN2d0IraVJHR1ZXTXh5cUdkR3duN3VuMUsxcUUveDhwUGRoQWNF?=
 =?utf-8?B?WEtESW1FdVQ3a0xEMUpIN3pqbGUwcnFvN3FiOCtYaXltTE1vWHN0NXJTdTkv?=
 =?utf-8?B?VzY4V25XSHlrRVNDN1FRdDJmVDJ1aTNsMCtXYndEUWdsYzJpVzBUSHFwTzcz?=
 =?utf-8?B?OXB0YlJKaTlpdWVPdGVaMisrclFPRnJaclRZckxoVTkxeE82WThkcTVzcGVz?=
 =?utf-8?B?SDJNWEVPNnU4TEVDSVBQL1RTQmtidER0TWczZ05QM29QSndVbkhHb1pxOEZ0?=
 =?utf-8?B?cWpEenlOdE9LY0lSRUVrd1JHQlFWazlCbFJrcEhlcWNiZ1dxN2NNTHlNQ0px?=
 =?utf-8?B?VVRWTHZUcXJ4ZzNFY0VjSUMrTkdEb0pISndhaGllNHMwUG9WY2MzbCswRnN4?=
 =?utf-8?B?VkNIRndYSE55ODV6K0xXSE1CRE95R0dzU1llNlhDMHVqeUJUWXB3d3R1azR0?=
 =?utf-8?B?VUNlc0hjTFJQWG1Ra0dod3JRZzJ3OVhBYk9mdHYrUitOcGU0dGpxa3hSZmZa?=
 =?utf-8?B?aU80V21vL1JvRmREOUtpN09DM3psejNpcVFEWVY1akEzSXdTQnZqQ2dzNC84?=
 =?utf-8?B?YmpDYlFRbFhkS3dSWW9BUUNLeGVjR1lxaU8zOFpqeldRS1kybStic3Q5V0cy?=
 =?utf-8?B?Y0x5VjB4NkxXZnpUcVF1OFVjUWhBWjBNN1NyZWZNWHVlYUl6MHJlakFoVmlh?=
 =?utf-8?B?UDBHTmRVRzR0dHVYUkRvUEdkMmNhQVZibFh4eU0wRTNRdCt2U0tMZ1dhL2t0?=
 =?utf-8?B?YUMwYTJ5ZXpIUWl6NEdlYUVGTXNhdk94dkxiSkg3WEJHcGhHRlhNZ1VCSXNP?=
 =?utf-8?B?eDc5RXk2aVZWYUV5a05zdkN1Y0tza3Mvek9KQUY3MGsxd3pSTXAxYTNmOEpQ?=
 =?utf-8?B?a2FyWWs1UFlUOGhKeW1HdGMyMWFSQkRaTEFuOE1FeFVhY3lIOG02OGlCRVVV?=
 =?utf-8?B?MjI2ekRockd1OTlha0pKZkNxdjRqb01LaHNGVjlub1l5dkJoQjVSbXNVSFdC?=
 =?utf-8?B?MkxEaXUreklkM0JiQTVGUVR0bkhRSk0xRnlaZkYyaE1maXphZzR5TnBOTDls?=
 =?utf-8?B?THh1bExyK24wSkdGQ1QvcW9GNUkvS3RGYXJYTDNnRGNTSUszcWY5bitwTmho?=
 =?utf-8?B?VUVuTForRzFIWDlieWJYMUhrNzNxb1h5SXROVktobUdtc2FuUXI4M0QyV05j?=
 =?utf-8?B?NXV3Zldsc1VnRExoSGdoU1lLc1pwamUrZm1YMjFMQ0dvYnpTWkRYb1A3aXZM?=
 =?utf-8?B?N2liSk1RN1JRanlOaU5ndlBwY1VzSmZ5OFl0bVdVVnQ5RnR2YUpab3V5QVBu?=
 =?utf-8?Q?UNNYGEH3t6Umn/PUoznQxPZ77?=
X-MS-Exchange-CrossTenant-Network-Message-Id: e428e1ca-434f-440e-4f1a-08dc04f58705
X-MS-Exchange-CrossTenant-AuthSource: DM4PR11MB5469.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 25 Dec 2023 02:59:36.2106
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: KlSE2cWH3UcIMIEvrl6j4gnLbrtT2ufchjB7Yzr6oZnzWFWmjMARjKorypyLp3WwwjLoFtZZ/hIzVnTnCiKIsg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR11MB5804
X-OriginatorOrg: intel.com

On 12/21/2023 11:19 AM, Pasha Tatashin wrote:
> In order to be able to efficiently free empty page table levels, count the
> number of entries in each page table my incremeanting and decremeanting
s/incremeanting/incrementing
s/decremeanting/decrementing

> refcount every time a PTE is inserted or removed form the page table.
s/form/from

> 
> For this to work correctly, add two helper function:
> dma_clear_pte and dma_set_pte where counting is performed,
> 
> Also, modify the code so every page table entry is always updated using the
> two new functions.
> 
> Signed-off-by: Pasha Tatashin <pasha.tatashin@soleen.com>
> ---
>   drivers/iommu/intel/iommu.c | 40 +++++++++++++++++++++---------------
>   drivers/iommu/intel/iommu.h | 41 +++++++++++++++++++++++++++++++------
>   2 files changed, 58 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 897159dba47d..4688ef797161 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -949,7 +949,7 @@ static struct dma_pte *pfn_to_dma_pte(struct dmar_domain *domain,
>   			if (domain->use_first_level)
>   				pteval |= DMA_FL_PTE_XD | DMA_FL_PTE_US | DMA_FL_PTE_ACCESS;
>   
> -			if (cmpxchg64(&pte->val, 0ULL, pteval))
> +			if (dma_set_pte(pte, pteval))
>   				/* Someone else set it while we were thinking; use theirs. */
>   				free_pgtable_page(tmp_page);
>   			else
> @@ -1021,7 +1021,8 @@ static void dma_pte_clear_range(struct dmar_domain *domain,
>   			continue;
>   		}
>   		do {
> -			dma_clear_pte(pte);
> +			if (dma_pte_present(pte))
> +				dma_clear_pte(pte);
>   			start_pfn += lvl_to_nr_pages(large_page);
>   			pte++;
>   		} while (start_pfn <= last_pfn && !first_pte_in_page(pte));
> @@ -1062,7 +1063,8 @@ static void dma_pte_free_level(struct dmar_domain *domain, int level,
>   		 */
>   		if (level < retain_level && !(start_pfn > level_pfn ||
>   		      last_pfn < level_pfn + level_size(level) - 1)) {
> -			dma_clear_pte(pte);
> +			if (dma_pte_present(pte))
> +				dma_clear_pte(pte);
>   			domain_flush_cache(domain, pte, sizeof(*pte));
>   			free_pgtable_page(level_pte);
>   		}
> @@ -1093,12 +1095,13 @@ static void dma_pte_free_pagetable(struct dmar_domain *domain,
>   	}
>   }
>   
> -/* When a page at a given level is being unlinked from its parent, we don't
> -   need to *modify* it at all. All we need to do is make a list of all the
> -   pages which can be freed just as soon as we've flushed the IOTLB and we
> -   know the hardware page-walk will no longer touch them.
> -   The 'pte' argument is the *parent* PTE, pointing to the page that is to
> -   be freed. */
> +/*
> + * A given page at a given level is being unlinked from its parent.
> + * We need to make a list of all the pages which can be freed just as soon as
> + * we've flushed the IOTLB and we know the hardware page-walk will no longer
> + * touch them. The 'pte' argument is the *parent* PTE, pointing to the page
> + * that is to be freed.
> + */
>   static void dma_pte_list_pagetables(struct dmar_domain *domain,
>   				    int level, struct dma_pte *pte,
>   				    struct list_head *freelist)
> @@ -1106,17 +1109,20 @@ static void dma_pte_list_pagetables(struct dmar_domain *domain,
>   	struct page *pg;
>   
>   	pg = pfn_to_page(dma_pte_addr(pte) >> PAGE_SHIFT);
> -	list_add_tail(&pg->lru, freelist);
> -
> -	if (level == 1)
> -		return;
> -
>   	pte = page_address(pg);
> +
>   	do {
> -		if (dma_pte_present(pte) && !dma_pte_superpage(pte))
> -			dma_pte_list_pagetables(domain, level - 1, pte, freelist);
> +		if (dma_pte_present(pte)) {
> +			if (level > 1 && !dma_pte_superpage(pte)) {
> +				dma_pte_list_pagetables(domain, level - 1, pte,
> +							freelist);
> +			}
> +			dma_clear_pte(pte);
> +		}
>   		pte++;
>   	} while (!first_pte_in_page(pte));
> +
> +	list_add_tail(&pg->lru, freelist);
>   }
>   
How about calculating the page decrement when the pages in the freelist 
are really freed in iommu_free_pgtbl_pages() ?

Thanks,
Jingqi