[PATCH 1/3] iommu/amd: Use try_cmpxchg64() in v2_alloc_pte()

[PATCH 1/3] iommu/amd: Use try_cmpxchg64() in v2_alloc_pte()

[Uros Bizjak]

Use try_cmpxchg64() instead of cmpxchg64 (*ptr, old, new) != old in
v2_alloc_pte().  cmpxchg returns success in ZF flag, so this change
saves a compare after cmpxchg (and related move instruction
in front of cmpxchg).

This is the same improvement as implemented for alloc_pte() in:

  commit 0d10fe759117 ("iommu/amd: Use try_cmpxchg64 in alloc_pte and free_clear_pte")

Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Cc: Will Deacon <will@kernel.org>
Cc: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/amd/io_pgtable_v2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/amd/io_pgtable_v2.c b/drivers/iommu/amd/io_pgtable_v2.c
index 78ac37c5ccc1..664e91c88748 100644
--- a/drivers/iommu/amd/io_pgtable_v2.c
+++ b/drivers/iommu/amd/io_pgtable_v2.c
@@ -158,7 +158,7 @@ static u64 *v2_alloc_pte(int nid, u64 *pgd, unsigned long iova,
 
 			__npte = set_pgtable_attr(page);
 			/* pte could have been changed somewhere. */
-			if (cmpxchg64(pte, __pte, __npte) != __pte)
+			if (!try_cmpxchg64(pte, &__pte, __npte))
 				iommu_free_page(page);
 			else if (IOMMU_PTE_PRESENT(__pte))
 				*updated = true;
-- 
2.45.1

[PATCH 2/3] iommu/vt-d: Use try_cmpxchg64() in intel_pasid_get_entry()

[Uros Bizjak]

Use try_cmpxchg64() instead of cmpxchg64 (*ptr, old, new) != old in
intel_pasid_get_entry().  cmpxchg returns success in ZF flag, so
this change saves a compare after cmpxchg (and related move
instruction in front of cmpxchg).

Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Lu Baolu <baolu.lu@linux.intel.com>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: Will Deacon <will@kernel.org>
Cc: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/intel/pasid.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index abce19e2ad6f..9bf45bc4b967 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -146,6 +146,8 @@ static struct pasid_entry *intel_pasid_get_entry(struct device *dev, u32 pasid)
 retry:
 	entries = get_pasid_table_from_pde(&dir[dir_index]);
 	if (!entries) {
+		u64 tmp;
+
 		entries = iommu_alloc_page_node(info->iommu->node, GFP_ATOMIC);
 		if (!entries)
 			return NULL;
@@ -156,8 +158,9 @@ static struct pasid_entry *intel_pasid_get_entry(struct device *dev, u32 pasid)
 		 * clear. However, this entry might be populated by others
 		 * while we are preparing it. Use theirs with a retry.
 		 */
-		if (cmpxchg64(&dir[dir_index].val, 0ULL,
-			      (u64)virt_to_phys(entries) | PASID_PTE_PRESENT)) {
+		tmp = 0ULL;
+		if (!try_cmpxchg64(&dir[dir_index].val, &tmp,
+				   (u64)virt_to_phys(entries) | PASID_PTE_PRESENT)) {
 			iommu_free_page(entries);
 			goto retry;
 		}
-- 
2.45.1

Re: [PATCH 2/3] iommu/vt-d: Use try_cmpxchg64() in intel_pasid_get_entry()

[Baolu Lu]

On 2024/5/22 16:26, Uros Bizjak wrote:
> Use try_cmpxchg64() instead of cmpxchg64 (*ptr, old, new) != old in
> intel_pasid_get_entry().  cmpxchg returns success in ZF flag, so
> this change saves a compare after cmpxchg (and related move
> instruction in front of cmpxchg).
> 
> Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Lu Baolu <baolu.lu@linux.intel.com>
> Cc: Joerg Roedel <joro@8bytes.org>
> Cc: Will Deacon <will@kernel.org>
> Cc: Robin Murphy <robin.murphy@arm.com>
> ---
>   drivers/iommu/intel/pasid.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
> index abce19e2ad6f..9bf45bc4b967 100644
> --- a/drivers/iommu/intel/pasid.c
> +++ b/drivers/iommu/intel/pasid.c
> @@ -146,6 +146,8 @@ static struct pasid_entry *intel_pasid_get_entry(struct device *dev, u32 pasid)
>   retry:
>   	entries = get_pasid_table_from_pde(&dir[dir_index]);
>   	if (!entries) {
> +		u64 tmp;
> +
>   		entries = iommu_alloc_page_node(info->iommu->node, GFP_ATOMIC);
>   		if (!entries)
>   			return NULL;
> @@ -156,8 +158,9 @@ static struct pasid_entry *intel_pasid_get_entry(struct device *dev, u32 pasid)
>   		 * clear. However, this entry might be populated by others
>   		 * while we are preparing it. Use theirs with a retry.
>   		 */
> -		if (cmpxchg64(&dir[dir_index].val, 0ULL,
> -			      (u64)virt_to_phys(entries) | PASID_PTE_PRESENT)) {
> +		tmp = 0ULL;

nit: can this simply be
		tmp = 0;
?

> +		if (!try_cmpxchg64(&dir[dir_index].val, &tmp,
> +				   (u64)virt_to_phys(entries) | PASID_PTE_PRESENT)) {

Above change will cause a dead loop during boot. It should be

		if (try_cmpxchg64(&dir[dir_index].val, &tmp,
				  (u64)virt_to_phys(entries) | PASID_PTE_PRESENT)) {

>   			iommu_free_page(entries);
>   			goto retry;
>   		}

Best regards,
baolu

Re: [PATCH 2/3] iommu/vt-d: Use try_cmpxchg64() in intel_pasid_get_entry()

[Uros Bizjak]

On Thu, May 23, 2024 at 3:24 PM Baolu Lu <baolu.lu@linux.intel.com> wrote:
>
> On 2024/5/22 16:26, Uros Bizjak wrote:
> > Use try_cmpxchg64() instead of cmpxchg64 (*ptr, old, new) != old in
> > intel_pasid_get_entry().  cmpxchg returns success in ZF flag, so
> > this change saves a compare after cmpxchg (and related move
> > instruction in front of cmpxchg).
> >
> > Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
> > Cc: David Woodhouse <dwmw2@infradead.org>
> > Cc: Lu Baolu <baolu.lu@linux.intel.com>
> > Cc: Joerg Roedel <joro@8bytes.org>
> > Cc: Will Deacon <will@kernel.org>
> > Cc: Robin Murphy <robin.murphy@arm.com>
> > ---
> >   drivers/iommu/intel/pasid.c | 7 +++++--
> >   1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
> > index abce19e2ad6f..9bf45bc4b967 100644
> > --- a/drivers/iommu/intel/pasid.c
> > +++ b/drivers/iommu/intel/pasid.c
> > @@ -146,6 +146,8 @@ static struct pasid_entry *intel_pasid_get_entry(struct device *dev, u32 pasid)
> >   retry:
> >       entries = get_pasid_table_from_pde(&dir[dir_index]);
> >       if (!entries) {
> > +             u64 tmp;
> > +
> >               entries = iommu_alloc_page_node(info->iommu->node, GFP_ATOMIC);
> >               if (!entries)
> >                       return NULL;
> > @@ -156,8 +158,9 @@ static struct pasid_entry *intel_pasid_get_entry(struct device *dev, u32 pasid)
> >                * clear. However, this entry might be populated by others
> >                * while we are preparing it. Use theirs with a retry.
> >                */
> > -             if (cmpxchg64(&dir[dir_index].val, 0ULL,
> > -                           (u64)virt_to_phys(entries) | PASID_PTE_PRESENT)) {
> > +             tmp = 0ULL;
>
> nit: can this simply be
>                 tmp = 0;
> ?

I just took the same suffix as it was in the original code, but for
zero literal, it does not make any change. I can change it, it
preferred.

> > +             if (!try_cmpxchg64(&dir[dir_index].val, &tmp,
> > +                                (u64)virt_to_phys(entries) | PASID_PTE_PRESENT)) {
>
> Above change will cause a dead loop during boot. It should be

No, it is correct as written:

if (cmpxchg64(*ptr, 0, new))

can be written as:

if (cmpxchg64(*ptr, 0, new) != 0)

this is equivalent to:

tmp = 0ULL;
if (!try_cmpxchg64(*ptr, &tmp, new))

Thanks,
Uros.

Re: [PATCH 2/3] iommu/vt-d: Use try_cmpxchg64() in intel_pasid_get_entry()

[Baolu Lu]

On 2024/5/23 21:34, Uros Bizjak wrote:
>>> +             if (!try_cmpxchg64(&dir[dir_index].val, &tmp,
>>> +                                (u64)virt_to_phys(entries) | PASID_PTE_PRESENT)) {
>> Above change will cause a dead loop during boot. It should be
> No, it is correct as written:
> 
> if (cmpxchg64(*ptr, 0, new))
> 
> can be written as:
> 
> if (cmpxchg64(*ptr, 0, new) != 0)
> 
> this is equivalent to:
> 
> tmp = 0ULL;
> if (!try_cmpxchg64(*ptr, &tmp, new))

The return value of both cmpxchg64() and try_cmpxchg64() is the old
value that was loaded from the memory location, right?

If so,

	if (cmpxchg64(*ptr, 0, new) != 0)

is not equivalent to

	if (!try_cmpxchg64(*ptr, &tmp, new))

Best regards,
baolu

Re: [PATCH 2/3] iommu/vt-d: Use try_cmpxchg64() in intel_pasid_get_entry()

[Uros Bizjak]

On Thu, May 23, 2024 at 3:44 PM Baolu Lu <baolu.lu@linux.intel.com> wrote:
>
> On 2024/5/23 21:34, Uros Bizjak wrote:
> >>> +             if (!try_cmpxchg64(&dir[dir_index].val, &tmp,
> >>> +                                (u64)virt_to_phys(entries) | PASID_PTE_PRESENT)) {
> >> Above change will cause a dead loop during boot. It should be
> > No, it is correct as written:
> >
> > if (cmpxchg64(*ptr, 0, new))
> >
> > can be written as:
> >
> > if (cmpxchg64(*ptr, 0, new) != 0)
> >
> > this is equivalent to:
> >
> > tmp = 0ULL;
> > if (!try_cmpxchg64(*ptr, &tmp, new))
>
> The return value of both cmpxchg64() and try_cmpxchg64() is the old
> value that was loaded from the memory location, right?

Actually, try_cmpxchg() returns true if successful and false if it fails.

            tmp = 0ULL;
            if (!try_cmpxchg64(*ptr, &tmp, new))

The logic in the above snippet can be interpreted as:

if we fail to compare *ptr with 0, then:

            iommu_free_page(entries);
            goto retry;

as intended in the original code.

Thanks,
Uros.

Re: [PATCH 2/3] iommu/vt-d: Use try_cmpxchg64() in intel_pasid_get_entry()

[Baolu Lu]

On 5/23/24 9:57 PM, Uros Bizjak wrote:
> On Thu, May 23, 2024 at 3:44 PM Baolu Lu<baolu.lu@linux.intel.com>  wrote:
>> On 2024/5/23 21:34, Uros Bizjak wrote:
>>>>> +             if (!try_cmpxchg64(&dir[dir_index].val, &tmp,
>>>>> +                                (u64)virt_to_phys(entries) | PASID_PTE_PRESENT)) {
>>>> Above change will cause a dead loop during boot. It should be
>>> No, it is correct as written:
>>>
>>> if (cmpxchg64(*ptr, 0, new))
>>>
>>> can be written as:
>>>
>>> if (cmpxchg64(*ptr, 0, new) != 0)
>>>
>>> this is equivalent to:
>>>
>>> tmp = 0ULL;
>>> if (!try_cmpxchg64(*ptr, &tmp, new))
>> The return value of both cmpxchg64() and try_cmpxchg64() is the old
>> value that was loaded from the memory location, right?
> Actually, try_cmpxchg() returns true if successful and false if it fails.

Oh! I misunderstood this.

> 
>              tmp = 0ULL;
>              if (!try_cmpxchg64(*ptr, &tmp, new))
> 
> The logic in the above snippet can be interpreted as:
> 
> if we fail to compare *ptr with 0, then:
> 
>              iommu_free_page(entries);
>              goto retry;
> 
> as intended in the original code.

Okay, it's fine.

Best regards,
baolu

Re: [PATCH 2/3] iommu/vt-d: Use try_cmpxchg64() in intel_pasid_get_entry()

[Baolu Lu]

On 5/22/24 4:26 PM, Uros Bizjak wrote:
> Use try_cmpxchg64() instead of cmpxchg64 (*ptr, old, new) != old in
> intel_pasid_get_entry().  cmpxchg returns success in ZF flag, so
> this change saves a compare after cmpxchg (and related move
> instruction in front of cmpxchg).
> 
> Signed-off-by: Uros Bizjak<ubizjak@gmail.com>
> Cc: David Woodhouse<dwmw2@infradead.org>
> Cc: Lu Baolu<baolu.lu@linux.intel.com>
> Cc: Joerg Roedel<joro@8bytes.org>
> Cc: Will Deacon<will@kernel.org>
> Cc: Robin Murphy<robin.murphy@arm.com>
> ---
>   drivers/iommu/intel/pasid.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

[PATCH 3/3] iommufd: Use atomic_long_try_cmpxchg() in incr_user_locked_vm()

[Uros Bizjak]

Use atomic_long_try_cmpxchg() instead of
atomic_long_cmpxchg (*ptr, old, new) != old in incr_user_locked_vm().
cmpxchg returns success in ZF flag, so this change saves a compare
after cmpxchg (and related move instruction in front of cmpxchg).

Also, atomic_long_try_cmpxchg() implicitly assigns old *ptr
value to "old" when cmpxchg fails. There is no need to re-read
the value in the loop.

Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
Cc: Kevin Tian <kevin.tian@intel.com>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: Will Deacon <will@kernel.org>
Cc: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/iommufd/pages.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/iommufd/pages.c b/drivers/iommu/iommufd/pages.c
index 528f356238b3..117f644a0c5b 100644
--- a/drivers/iommu/iommufd/pages.c
+++ b/drivers/iommu/iommufd/pages.c
@@ -809,13 +809,14 @@ static int incr_user_locked_vm(struct iopt_pages *pages, unsigned long npages)
 
 	lock_limit = task_rlimit(pages->source_task, RLIMIT_MEMLOCK) >>
 		     PAGE_SHIFT;
+
+	cur_pages = atomic_long_read(&pages->source_user->locked_vm);
 	do {
-		cur_pages = atomic_long_read(&pages->source_user->locked_vm);
 		new_pages = cur_pages + npages;
 		if (new_pages > lock_limit)
 			return -ENOMEM;
-	} while (atomic_long_cmpxchg(&pages->source_user->locked_vm, cur_pages,
-				     new_pages) != cur_pages);
+	} while (!atomic_long_try_cmpxchg(&pages->source_user->locked_vm,
+					  &cur_pages, new_pages));
 	return 0;
 }
 
-- 
2.45.1

Re: [PATCH 3/3] iommufd: Use atomic_long_try_cmpxchg() in incr_user_locked_vm()

[Jason Gunthorpe]

On Wed, May 22, 2024 at 10:26:49AM +0200, Uros Bizjak wrote:
> Use atomic_long_try_cmpxchg() instead of
> atomic_long_cmpxchg (*ptr, old, new) != old in incr_user_locked_vm().
> cmpxchg returns success in ZF flag, so this change saves a compare
> after cmpxchg (and related move instruction in front of cmpxchg).
> 
> Also, atomic_long_try_cmpxchg() implicitly assigns old *ptr
> value to "old" when cmpxchg fails. There is no need to re-read
> the value in the loop.
> 
> Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> Cc: Kevin Tian <kevin.tian@intel.com>
> Cc: Joerg Roedel <joro@8bytes.org>
> Cc: Will Deacon <will@kernel.org>
> Cc: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/iommu/iommufd/pages.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

This patch may as well go through Joerg's tree with the whole series

Thanks,
Jason

Re: [PATCH 1/3] iommu/amd: Use try_cmpxchg64() in v2_alloc_pte()

[Vasant Hegde]

On 5/22/2024 1:56 PM, Uros Bizjak wrote:
> Use try_cmpxchg64() instead of cmpxchg64 (*ptr, old, new) != old in
> v2_alloc_pte().  cmpxchg returns success in ZF flag, so this change
> saves a compare after cmpxchg (and related move instruction
> in front of cmpxchg).
> 
> This is the same improvement as implemented for alloc_pte() in:
> 
>   commit 0d10fe759117 ("iommu/amd: Use try_cmpxchg64 in alloc_pte and free_clear_pte")
> 
> Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
> Cc: Joerg Roedel <joro@8bytes.org>
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Robin Murphy <robin.murphy@arm.com>

Thanks for fixing it. Looks good to me.

Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>

-Vasant

> ---
>  drivers/iommu/amd/io_pgtable_v2.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/amd/io_pgtable_v2.c b/drivers/iommu/amd/io_pgtable_v2.c
> index 78ac37c5ccc1..664e91c88748 100644
> --- a/drivers/iommu/amd/io_pgtable_v2.c
> +++ b/drivers/iommu/amd/io_pgtable_v2.c
> @@ -158,7 +158,7 @@ static u64 *v2_alloc_pte(int nid, u64 *pgd, unsigned long iova,
>  
>  			__npte = set_pgtable_attr(page);
>  			/* pte could have been changed somewhere. */
> -			if (cmpxchg64(pte, __pte, __npte) != __pte)
> +			if (!try_cmpxchg64(pte, &__pte, __npte))
>  				iommu_free_page(page);
>  			else if (IOMMU_PTE_PRESENT(__pte))
>  				*updated = true;