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 <[email protected]>
Cc: Joerg Roedel <[email protected]>
Cc: Suravee Suthikulpanit <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Robin Murphy <[email protected]>
---
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
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 <[email protected]>
Cc: David Woodhouse <[email protected]>
Cc: Lu Baolu <[email protected]>
Cc: Joerg Roedel <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Robin Murphy <[email protected]>
---
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
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 <[email protected]>
Cc: Jason Gunthorpe <[email protected]>
Cc: Kevin Tian <[email protected]>
Cc: Joerg Roedel <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Robin Murphy <[email protected]>
---
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
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 <[email protected]>
> Cc: Jason Gunthorpe <[email protected]>
> Cc: Kevin Tian <[email protected]>
> Cc: Joerg Roedel <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Robin Murphy <[email protected]>
> ---
> drivers/iommu/iommufd/pages.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
Reviewed-by: Jason Gunthorpe <[email protected]>
This patch may as well go through Joerg's tree with the whole series
Thanks,
Jason
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 <[email protected]>
> Cc: Joerg Roedel <[email protected]>
> Cc: Suravee Suthikulpanit <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Robin Murphy <[email protected]>
Thanks for fixing it. Looks good to me.
Reviewed-by: Vasant Hegde <[email protected]>
-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;
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 <[email protected]>
> Cc: David Woodhouse <[email protected]>
> Cc: Lu Baolu <[email protected]>
> Cc: Joerg Roedel <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Robin Murphy <[email protected]>
> ---
> 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
On Thu, May 23, 2024 at 3:24 PM Baolu Lu <[email protected]> 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 <[email protected]>
> > Cc: David Woodhouse <[email protected]>
> > Cc: Lu Baolu <[email protected]>
> > Cc: Joerg Roedel <[email protected]>
> > Cc: Will Deacon <[email protected]>
> > Cc: Robin Murphy <[email protected]>
> > ---
> > 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.
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
On Thu, May 23, 2024 at 3:44 PM Baolu Lu <[email protected]> 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.
On 5/23/24 9:57 PM, Uros Bizjak wrote:
> On Thu, May 23, 2024 at 3:44 PM Baolu Lu<[email protected]> 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
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<[email protected]>
> Cc: David Woodhouse<[email protected]>
> Cc: Lu Baolu<[email protected]>
> Cc: Joerg Roedel<[email protected]>
> Cc: Will Deacon<[email protected]>
> Cc: Robin Murphy<[email protected]>
> ---
> drivers/iommu/intel/pasid.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
Reviewed-by: Lu Baolu <[email protected]>
Best regards,
baolu