[PATCH 0/7] iommu/iova: improve the allocation performance of dma64

[PATCH 0/7] iommu/iova: improve the allocation performance of dma64

[Zhen Lei]

64 bits devices is very common now. But currently we only defined a cached32_node
to optimize the allocation performance of dma32, and I saw some dma64 drivers chose
to allocate iova from dma32 space first, maybe becuase of current dma64 performance
problem or some other reasons.

For example:(in drivers/iommu/amd_iommu.c)
static unsigned long dma_ops_alloc_iova(......
{
	......
	if (dma_mask > DMA_BIT_MASK(32))
		pfn = alloc_iova_fast(&dma_dom->iovad, pages,
				      IOVA_PFN(DMA_BIT_MASK(32)));
	if (!pfn)
		pfn = alloc_iova_fast(&dma_dom->iovad, pages, IOVA_PFN(dma_mask));
		
For the details of why dma64 iova allocation performance is very bad, please refer the
description of patch-5.

In this patch series, I added a cached64_node to manage the dma64 iova space(iova>=4G), it
takes the same effect as cached32_node(iova<4G).

Below it's the performance data before and after my patch series:
(before)$ iperf -s
------------------------------------------------------------
Server listening on TCP port 5001
TCP window size: 85.3 KByte (default)
------------------------------------------------------------
[  4] local 192.168.1.106 port 5001 connected with 192.168.1.198 port 35898
[ ID] Interval       Transfer     Bandwidth
[  4]  0.0-10.2 sec  7.88 MBytes  6.48 Mbits/sec
[  5] local 192.168.1.106 port 5001 connected with 192.168.1.198 port 35900
[  5]  0.0-10.3 sec  7.88 MBytes  6.43 Mbits/sec
[  4] local 192.168.1.106 port 5001 connected with 192.168.1.198 port 35902
[  4]  0.0-10.3 sec  7.88 MBytes  6.43 Mbits/sec

(after)$ iperf -s
------------------------------------------------------------
Server listening on TCP port 5001
TCP window size: 85.3 KByte (default)
------------------------------------------------------------
[  4] local 192.168.1.106 port 5001 connected with 192.168.1.198 port 36330
[ ID] Interval       Transfer     Bandwidth
[  4]  0.0-10.0 sec  1.09 GBytes   933 Mbits/sec
[  5] local 192.168.1.106 port 5001 connected with 192.168.1.198 port 36332
[  5]  0.0-10.0 sec  1.10 GBytes   939 Mbits/sec
[  4] local 192.168.1.106 port 5001 connected with 192.168.1.198 port 36334
[  4]  0.0-10.0 sec  1.10 GBytes   938 Mbits/sec


Zhen Lei (7):
  iommu/iova: fix incorrect variable types
  iommu/iova: cut down judgement times
  iommu/iova: insert start_pfn boundary of dma32
  iommu/iova: adjust __cached_rbnode_insert_update
  iommu/iova: to optimize the allocation performance of dma64
  iommu/iova: move the caculation of pad mask out of loop
  iommu/iova: fix iovad->dma_32bit_pfn as the last pfn of dma32

 drivers/iommu/amd_iommu.c        |   7 +-
 drivers/iommu/dma-iommu.c        |  22 ++----
 drivers/iommu/intel-iommu.c      |  11 +--
 drivers/iommu/iova.c             | 143 +++++++++++++++++++++------------------
 drivers/misc/mic/scif/scif_rma.c |   3 +-
 include/linux/iova.h             |   7 +-
 6 files changed, 94 insertions(+), 99 deletions(-)

-- 
2.5.0

[PATCH 0/7] iommu/iova: improve the allocation performance of dma64

[Zhen Lei]

64 bits devices is very common now. But currently we only defined a cached32_node
to optimize the allocation performance of dma32, and I saw some dma64 drivers chose
to allocate iova from dma32 space first, maybe becuase of current dma64 performance
problem or some other reasons.

For example:(in drivers/iommu/amd_iommu.c)
static unsigned long dma_ops_alloc_iova(......
{
	......
	if (dma_mask > DMA_BIT_MASK(32))
		pfn = alloc_iova_fast(&dma_dom->iovad, pages,
				      IOVA_PFN(DMA_BIT_MASK(32)));
	if (!pfn)
		pfn = alloc_iova_fast(&dma_dom->iovad, pages, IOVA_PFN(dma_mask));
		
For the details of why dma64 iova allocation performance is very bad, please refer the
description of patch-5.

In this patch series, I added a cached64_node to manage the dma64 iova space(iova>=4G), it
takes the same effect as cached32_node(iova<4G).

Below it's the performance data before and after my patch series:
(before)$ iperf -s
------------------------------------------------------------
Server listening on TCP port 5001
TCP window size: 85.3 KByte (default)
------------------------------------------------------------
[  4] local 192.168.1.106 port 5001 connected with 192.168.1.198 port 35898
[ ID] Interval       Transfer     Bandwidth
[  4]  0.0-10.2 sec  7.88 MBytes  6.48 Mbits/sec
[  5] local 192.168.1.106 port 5001 connected with 192.168.1.198 port 35900
[  5]  0.0-10.3 sec  7.88 MBytes  6.43 Mbits/sec
[  4] local 192.168.1.106 port 5001 connected with 192.168.1.198 port 35902
[  4]  0.0-10.3 sec  7.88 MBytes  6.43 Mbits/sec

(after)$ iperf -s
------------------------------------------------------------
Server listening on TCP port 5001
TCP window size: 85.3 KByte (default)
------------------------------------------------------------
[  4] local 192.168.1.106 port 5001 connected with 192.168.1.198 port 36330
[ ID] Interval       Transfer     Bandwidth
[  4]  0.0-10.0 sec  1.09 GBytes   933 Mbits/sec
[  5] local 192.168.1.106 port 5001 connected with 192.168.1.198 port 36332
[  5]  0.0-10.0 sec  1.10 GBytes   939 Mbits/sec
[  4] local 192.168.1.106 port 5001 connected with 192.168.1.198 port 36334
[  4]  0.0-10.0 sec  1.10 GBytes   938 Mbits/sec


Zhen Lei (7):
  iommu/iova: fix incorrect variable types
  iommu/iova: cut down judgement times
  iommu/iova: insert start_pfn boundary of dma32
  iommu/iova: adjust __cached_rbnode_insert_update
  iommu/iova: to optimize the allocation performance of dma64
  iommu/iova: move the caculation of pad mask out of loop
  iommu/iova: fix iovad->dma_32bit_pfn as the last pfn of dma32

 drivers/iommu/amd_iommu.c        |   7 +-
 drivers/iommu/dma-iommu.c        |  22 ++----
 drivers/iommu/intel-iommu.c      |  11 +--
 drivers/iommu/iova.c             | 143 +++++++++++++++++++++------------------
 drivers/misc/mic/scif/scif_rma.c |   3 +-
 include/linux/iova.h             |   7 +-
 6 files changed, 94 insertions(+), 99 deletions(-)

-- 
2.5.0

[PATCH 1/7] iommu/iova: fix incorrect variable types

[Zhen Lei]

Keep these four variables type consistent with the paramters of function
__alloc_and_insert_iova_range and the members of struct iova:

1. static int __alloc_and_insert_iova_range(struct iova_domain *iovad,
		unsigned long size, unsigned long limit_pfn,

2. struct iova {
	unsigned long	pfn_hi;
	unsigned long	pfn_lo;

In fact, limit_pfn is most likely larger than 32 bits on DMA64.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 drivers/iommu/iova.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index b7268a1..8ba8b496 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -104,8 +104,8 @@ __cached_rbnode_delete_update(struct iova_domain *iovad, struct iova *free)
  * Computes the padding size required, to make the start address
  * naturally aligned on the power-of-two order of its size
  */
-static unsigned int
-iova_get_pad_size(unsigned int size, unsigned int limit_pfn)
+static unsigned long
+iova_get_pad_size(unsigned long size, unsigned long limit_pfn)
 {
 	return (limit_pfn + 1 - size) & (__roundup_pow_of_two(size) - 1);
 }
@@ -117,7 +117,7 @@ static int __alloc_and_insert_iova_range(struct iova_domain *iovad,
 	struct rb_node *prev, *curr = NULL;
 	unsigned long flags;
 	unsigned long saved_pfn;
-	unsigned int pad_size = 0;
+	unsigned long pad_size = 0;
 
 	/* Walk the tree backwards */
 	spin_lock_irqsave(&iovad->iova_rbtree_lock, flags);
-- 
2.5.0

[PATCH 1/7] iommu/iova: fix incorrect variable types

[Zhen Lei]

Keep these four variables type consistent with the paramters of function
__alloc_and_insert_iova_range and the members of struct iova:

1. static int __alloc_and_insert_iova_range(struct iova_domain *iovad,
		unsigned long size, unsigned long limit_pfn,

2. struct iova {
	unsigned long	pfn_hi;
	unsigned long	pfn_lo;

In fact, limit_pfn is most likely larger than 32 bits on DMA64.

Signed-off-by: Zhen Lei <thunder.leizhen-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---
 drivers/iommu/iova.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index b7268a1..8ba8b496 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -104,8 +104,8 @@ __cached_rbnode_delete_update(struct iova_domain *iovad, struct iova *free)
  * Computes the padding size required, to make the start address
  * naturally aligned on the power-of-two order of its size
  */
-static unsigned int
-iova_get_pad_size(unsigned int size, unsigned int limit_pfn)
+static unsigned long
+iova_get_pad_size(unsigned long size, unsigned long limit_pfn)
 {
 	return (limit_pfn + 1 - size) & (__roundup_pow_of_two(size) - 1);
 }
@@ -117,7 +117,7 @@ static int __alloc_and_insert_iova_range(struct iova_domain *iovad,
 	struct rb_node *prev, *curr = NULL;
 	unsigned long flags;
 	unsigned long saved_pfn;
-	unsigned int pad_size = 0;
+	unsigned long pad_size = 0;
 
 	/* Walk the tree backwards */
 	spin_lock_irqsave(&iovad->iova_rbtree_lock, flags);
-- 
2.5.0

Re: [PATCH 1/7] iommu/iova: fix incorrect variable types

[Robin Murphy]

On 22/03/17 06:27, Zhen Lei wrote:
> Keep these four variables type consistent with the paramters of function
> __alloc_and_insert_iova_range and the members of struct iova:
> 
> 1. static int __alloc_and_insert_iova_range(struct iova_domain *iovad,
> 		unsigned long size, unsigned long limit_pfn,
> 
> 2. struct iova {
> 	unsigned long	pfn_hi;
> 	unsigned long	pfn_lo;
> 
> In fact, limit_pfn is most likely larger than 32 bits on DMA64.

FWIW if pad_size manages to overflow an int something's probably gone
horribly wrong, but there's no harm in making it consistent with
everything else here. However, given that patch #6 makes this irrelevant
anyway, do we really need to bother?

Robin.

> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> ---
>  drivers/iommu/iova.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
> index b7268a1..8ba8b496 100644
> --- a/drivers/iommu/iova.c
> +++ b/drivers/iommu/iova.c
> @@ -104,8 +104,8 @@ __cached_rbnode_delete_update(struct iova_domain *iovad, struct iova *free)
>   * Computes the padding size required, to make the start address
>   * naturally aligned on the power-of-two order of its size
>   */
> -static unsigned int
> -iova_get_pad_size(unsigned int size, unsigned int limit_pfn)
> +static unsigned long
> +iova_get_pad_size(unsigned long size, unsigned long limit_pfn)
>  {
>  	return (limit_pfn + 1 - size) & (__roundup_pow_of_two(size) - 1);
>  }
> @@ -117,7 +117,7 @@ static int __alloc_and_insert_iova_range(struct iova_domain *iovad,
>  	struct rb_node *prev, *curr = NULL;
>  	unsigned long flags;
>  	unsigned long saved_pfn;
> -	unsigned int pad_size = 0;
> +	unsigned long pad_size = 0;
>  
>  	/* Walk the tree backwards */
>  	spin_lock_irqsave(&iovad->iova_rbtree_lock, flags);
> 

Re: [PATCH 1/7] iommu/iova: fix incorrect variable types

[Leizhen (ThunderTown)]

On 2017/3/23 19:42, Robin Murphy wrote:
> On 22/03/17 06:27, Zhen Lei wrote:
>> Keep these four variables type consistent with the paramters of function
>> __alloc_and_insert_iova_range and the members of struct iova:
>>
>> 1. static int __alloc_and_insert_iova_range(struct iova_domain *iovad,
>> 		unsigned long size, unsigned long limit_pfn,
>>
>> 2. struct iova {
>> 	unsigned long	pfn_hi;
>> 	unsigned long	pfn_lo;
>>
>> In fact, limit_pfn is most likely larger than 32 bits on DMA64.
> 
> FWIW if pad_size manages to overflow an int something's probably gone
> horribly wrong, but there's no harm in making it consistent with
> everything else here. However, given that patch #6 makes this irrelevant
> anyway, do we really need to bother?

Because I'm not sure whether patch #6 can be applied or not.

> 
> Robin.
> 
>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>> ---
>>  drivers/iommu/iova.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
>> index b7268a1..8ba8b496 100644
>> --- a/drivers/iommu/iova.c
>> +++ b/drivers/iommu/iova.c
>> @@ -104,8 +104,8 @@ __cached_rbnode_delete_update(struct iova_domain *iovad, struct iova *free)
>>   * Computes the padding size required, to make the start address
>>   * naturally aligned on the power-of-two order of its size
>>   */
>> -static unsigned int
>> -iova_get_pad_size(unsigned int size, unsigned int limit_pfn)
>> +static unsigned long
>> +iova_get_pad_size(unsigned long size, unsigned long limit_pfn)
>>  {
>>  	return (limit_pfn + 1 - size) & (__roundup_pow_of_two(size) - 1);
>>  }
>> @@ -117,7 +117,7 @@ static int __alloc_and_insert_iova_range(struct iova_domain *iovad,
>>  	struct rb_node *prev, *curr = NULL;
>>  	unsigned long flags;
>>  	unsigned long saved_pfn;
>> -	unsigned int pad_size = 0;
>> +	unsigned long pad_size = 0;
>>  
>>  	/* Walk the tree backwards */
>>  	spin_lock_irqsave(&iovad->iova_rbtree_lock, flags);
>>
> 
> 
> .
> 

-- 
Thanks!
BestRegards

Re: [PATCH 1/7] iommu/iova: fix incorrect variable types

[Leizhen (ThunderTown)]

On 2017/3/23 19:42, Robin Murphy wrote:
> On 22/03/17 06:27, Zhen Lei wrote:
>> Keep these four variables type consistent with the paramters of function
>> __alloc_and_insert_iova_range and the members of struct iova:
>>
>> 1. static int __alloc_and_insert_iova_range(struct iova_domain *iovad,
>> 		unsigned long size, unsigned long limit_pfn,
>>
>> 2. struct iova {
>> 	unsigned long	pfn_hi;
>> 	unsigned long	pfn_lo;
>>
>> In fact, limit_pfn is most likely larger than 32 bits on DMA64.
> 
> FWIW if pad_size manages to overflow an int something's probably gone
> horribly wrong, but there's no harm in making it consistent with
> everything else here. However, given that patch #6 makes this irrelevant
> anyway, do we really need to bother?

Because I'm not sure whether patch #6 can be applied or not.

> 
> Robin.
> 
>> Signed-off-by: Zhen Lei <thunder.leizhen-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>> ---
>>  drivers/iommu/iova.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
>> index b7268a1..8ba8b496 100644
>> --- a/drivers/iommu/iova.c
>> +++ b/drivers/iommu/iova.c
>> @@ -104,8 +104,8 @@ __cached_rbnode_delete_update(struct iova_domain *iovad, struct iova *free)
>>   * Computes the padding size required, to make the start address
>>   * naturally aligned on the power-of-two order of its size
>>   */
>> -static unsigned int
>> -iova_get_pad_size(unsigned int size, unsigned int limit_pfn)
>> +static unsigned long
>> +iova_get_pad_size(unsigned long size, unsigned long limit_pfn)
>>  {
>>  	return (limit_pfn + 1 - size) & (__roundup_pow_of_two(size) - 1);
>>  }
>> @@ -117,7 +117,7 @@ static int __alloc_and_insert_iova_range(struct iova_domain *iovad,
>>  	struct rb_node *prev, *curr = NULL;
>>  	unsigned long flags;
>>  	unsigned long saved_pfn;
>> -	unsigned int pad_size = 0;
>> +	unsigned long pad_size = 0;
>>  
>>  	/* Walk the tree backwards */
>>  	spin_lock_irqsave(&iovad->iova_rbtree_lock, flags);
>>
> 
> 
> .
> 

-- 
Thanks!
BestRegards

Re: [PATCH 1/7] iommu/iova: fix incorrect variable types

[Leizhen (ThunderTown)]

On 2017/3/24 10:27, Leizhen (ThunderTown) wrote:
> 
> 
> On 2017/3/23 19:42, Robin Murphy wrote:
>> On 22/03/17 06:27, Zhen Lei wrote:
>>> Keep these four variables type consistent with the paramters of function
>>> __alloc_and_insert_iova_range and the members of struct iova:
>>>
>>> 1. static int __alloc_and_insert_iova_range(struct iova_domain *iovad,
>>> 		unsigned long size, unsigned long limit_pfn,
>>>
>>> 2. struct iova {
>>> 	unsigned long	pfn_hi;
>>> 	unsigned long	pfn_lo;
>>>
>>> In fact, limit_pfn is most likely larger than 32 bits on DMA64.
>>
>> FWIW if pad_size manages to overflow an int something's probably gone
>> horribly wrong, but there's no harm in making it consistent with
>> everything else here. However, given that patch #6 makes this irrelevant
>> anyway, do we really need to bother?
> 
> Because I'm not sure whether patch #6 can be applied or not.
So if Patch #6 can be applied, I can merge this patch and patch #6 into one.

> 
>>
>> Robin.
>>
>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>>> ---
>>>  drivers/iommu/iova.c | 6 +++---
>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
>>> index b7268a1..8ba8b496 100644
>>> --- a/drivers/iommu/iova.c
>>> +++ b/drivers/iommu/iova.c
>>> @@ -104,8 +104,8 @@ __cached_rbnode_delete_update(struct iova_domain *iovad, struct iova *free)
>>>   * Computes the padding size required, to make the start address
>>>   * naturally aligned on the power-of-two order of its size
>>>   */
>>> -static unsigned int
>>> -iova_get_pad_size(unsigned int size, unsigned int limit_pfn)
>>> +static unsigned long
>>> +iova_get_pad_size(unsigned long size, unsigned long limit_pfn)
>>>  {
>>>  	return (limit_pfn + 1 - size) & (__roundup_pow_of_two(size) - 1);
>>>  }
>>> @@ -117,7 +117,7 @@ static int __alloc_and_insert_iova_range(struct iova_domain *iovad,
>>>  	struct rb_node *prev, *curr = NULL;
>>>  	unsigned long flags;
>>>  	unsigned long saved_pfn;
>>> -	unsigned int pad_size = 0;
>>> +	unsigned long pad_size = 0;
>>>  
>>>  	/* Walk the tree backwards */
>>>  	spin_lock_irqsave(&iovad->iova_rbtree_lock, flags);
>>>
>>
>>
>> .
>>
> 

-- 
Thanks!
BestRegards

Re: [PATCH 1/7] iommu/iova: fix incorrect variable types

[Leizhen (ThunderTown)]

On 2017/3/24 10:27, Leizhen (ThunderTown) wrote:
> 
> 
> On 2017/3/23 19:42, Robin Murphy wrote:
>> On 22/03/17 06:27, Zhen Lei wrote:
>>> Keep these four variables type consistent with the paramters of function
>>> __alloc_and_insert_iova_range and the members of struct iova:
>>>
>>> 1. static int __alloc_and_insert_iova_range(struct iova_domain *iovad,
>>> 		unsigned long size, unsigned long limit_pfn,
>>>
>>> 2. struct iova {
>>> 	unsigned long	pfn_hi;
>>> 	unsigned long	pfn_lo;
>>>
>>> In fact, limit_pfn is most likely larger than 32 bits on DMA64.
>>
>> FWIW if pad_size manages to overflow an int something's probably gone
>> horribly wrong, but there's no harm in making it consistent with
>> everything else here. However, given that patch #6 makes this irrelevant
>> anyway, do we really need to bother?
> 
> Because I'm not sure whether patch #6 can be applied or not.
So if Patch #6 can be applied, I can merge this patch and patch #6 into one.

> 
>>
>> Robin.
>>
>>> Signed-off-by: Zhen Lei <thunder.leizhen-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>>> ---
>>>  drivers/iommu/iova.c | 6 +++---
>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
>>> index b7268a1..8ba8b496 100644
>>> --- a/drivers/iommu/iova.c
>>> +++ b/drivers/iommu/iova.c
>>> @@ -104,8 +104,8 @@ __cached_rbnode_delete_update(struct iova_domain *iovad, struct iova *free)
>>>   * Computes the padding size required, to make the start address
>>>   * naturally aligned on the power-of-two order of its size
>>>   */
>>> -static unsigned int
>>> -iova_get_pad_size(unsigned int size, unsigned int limit_pfn)
>>> +static unsigned long
>>> +iova_get_pad_size(unsigned long size, unsigned long limit_pfn)
>>>  {
>>>  	return (limit_pfn + 1 - size) & (__roundup_pow_of_two(size) - 1);
>>>  }
>>> @@ -117,7 +117,7 @@ static int __alloc_and_insert_iova_range(struct iova_domain *iovad,
>>>  	struct rb_node *prev, *curr = NULL;
>>>  	unsigned long flags;
>>>  	unsigned long saved_pfn;
>>> -	unsigned int pad_size = 0;
>>> +	unsigned long pad_size = 0;
>>>  
>>>  	/* Walk the tree backwards */
>>>  	spin_lock_irqsave(&iovad->iova_rbtree_lock, flags);
>>>
>>
>>
>> .
>>
> 

-- 
Thanks!
BestRegards

[PATCH 2/7] iommu/iova: cut down judgement times

[Zhen Lei]

Below judgement can only be satisfied at the last time, which produced 2N
judgements(suppose N times failed, 0 or 1 time successed) in vain.

if ((pfn >= iova->pfn_lo) && (pfn <= iova->pfn_hi)) {
	return iova;
}

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 drivers/iommu/iova.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index 8ba8b496..1c49969 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -312,15 +312,12 @@ private_find_iova(struct iova_domain *iovad, unsigned long pfn)
 	while (node) {
 		struct iova *iova = rb_entry(node, struct iova, node);
 
-		/* If pfn falls within iova's range, return iova */
-		if ((pfn >= iova->pfn_lo) && (pfn <= iova->pfn_hi)) {
-			return iova;
-		}
-
 		if (pfn < iova->pfn_lo)
 			node = node->rb_left;
-		else if (pfn > iova->pfn_lo)
+		else if (pfn > iova->pfn_hi)
 			node = node->rb_right;
+		else
+			return iova;	/* pfn falls within iova's range */
 	}
 
 	return NULL;
-- 
2.5.0

[PATCH 2/7] iommu/iova: cut down judgement times

[Zhen Lei]

Below judgement can only be satisfied at the last time, which produced 2N
judgements(suppose N times failed, 0 or 1 time successed) in vain.

if ((pfn >= iova->pfn_lo) && (pfn <= iova->pfn_hi)) {
	return iova;
}

Signed-off-by: Zhen Lei <thunder.leizhen-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---
 drivers/iommu/iova.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index 8ba8b496..1c49969 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -312,15 +312,12 @@ private_find_iova(struct iova_domain *iovad, unsigned long pfn)
 	while (node) {
 		struct iova *iova = rb_entry(node, struct iova, node);
 
-		/* If pfn falls within iova's range, return iova */
-		if ((pfn >= iova->pfn_lo) && (pfn <= iova->pfn_hi)) {
-			return iova;
-		}
-
 		if (pfn < iova->pfn_lo)
 			node = node->rb_left;
-		else if (pfn > iova->pfn_lo)
+		else if (pfn > iova->pfn_hi)
 			node = node->rb_right;
+		else
+			return iova;	/* pfn falls within iova's range */
 	}
 
 	return NULL;
-- 
2.5.0

Re: [PATCH 2/7] iommu/iova: cut down judgement times

[Robin Murphy]

On 22/03/17 06:27, Zhen Lei wrote:
> Below judgement can only be satisfied at the last time, which produced 2N
> judgements(suppose N times failed, 0 or 1 time successed) in vain.
> 
> if ((pfn >= iova->pfn_lo) && (pfn <= iova->pfn_hi)) {
> 	return iova;
> }

For me, GCC (6.2.1 AArch64) seems to do a pretty good job of this
function already, so this change only saves two instructions in total
(pfn is compared against pfn_lo only once instead of twice), which I
wouldn't expect to see a noticeable performance effect from.

Given the improvement in readability, though, I don't even care about
any codegen differences :)

Reviewed-by: Robin Murphy <robin.murphy@arm.com>

> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> ---
>  drivers/iommu/iova.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
> index 8ba8b496..1c49969 100644
> --- a/drivers/iommu/iova.c
> +++ b/drivers/iommu/iova.c
> @@ -312,15 +312,12 @@ private_find_iova(struct iova_domain *iovad, unsigned long pfn)
>  	while (node) {
>  		struct iova *iova = rb_entry(node, struct iova, node);
>  
> -		/* If pfn falls within iova's range, return iova */
> -		if ((pfn >= iova->pfn_lo) && (pfn <= iova->pfn_hi)) {
> -			return iova;
> -		}
> -
>  		if (pfn < iova->pfn_lo)
>  			node = node->rb_left;
> -		else if (pfn > iova->pfn_lo)
> +		else if (pfn > iova->pfn_hi)
>  			node = node->rb_right;
> +		else
> +			return iova;	/* pfn falls within iova's range */
>  	}
>  
>  	return NULL;
> 

Re: [PATCH 2/7] iommu/iova: cut down judgement times

[Robin Murphy]

On 22/03/17 06:27, Zhen Lei wrote:
> Below judgement can only be satisfied at the last time, which produced 2N
> judgements(suppose N times failed, 0 or 1 time successed) in vain.
> 
> if ((pfn >= iova->pfn_lo) && (pfn <= iova->pfn_hi)) {
> 	return iova;
> }

For me, GCC (6.2.1 AArch64) seems to do a pretty good job of this
function already, so this change only saves two instructions in total
(pfn is compared against pfn_lo only once instead of twice), which I
wouldn't expect to see a noticeable performance effect from.

Given the improvement in readability, though, I don't even care about
any codegen differences :)

Reviewed-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>

> Signed-off-by: Zhen Lei <thunder.leizhen-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/iommu/iova.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
> index 8ba8b496..1c49969 100644
> --- a/drivers/iommu/iova.c
> +++ b/drivers/iommu/iova.c
> @@ -312,15 +312,12 @@ private_find_iova(struct iova_domain *iovad, unsigned long pfn)
>  	while (node) {
>  		struct iova *iova = rb_entry(node, struct iova, node);
>  
> -		/* If pfn falls within iova's range, return iova */
> -		if ((pfn >= iova->pfn_lo) && (pfn <= iova->pfn_hi)) {
> -			return iova;
> -		}
> -
>  		if (pfn < iova->pfn_lo)
>  			node = node->rb_left;
> -		else if (pfn > iova->pfn_lo)
> +		else if (pfn > iova->pfn_hi)
>  			node = node->rb_right;
> +		else
> +			return iova;	/* pfn falls within iova's range */
>  	}
>  
>  	return NULL;
> 

Re: [PATCH 2/7] iommu/iova: cut down judgement times

[Leizhen (ThunderTown)]

On 2017/3/23 20:11, Robin Murphy wrote:
> On 22/03/17 06:27, Zhen Lei wrote:
>> Below judgement can only be satisfied at the last time, which produced 2N
>> judgements(suppose N times failed, 0 or 1 time successed) in vain.
>>
>> if ((pfn >= iova->pfn_lo) && (pfn <= iova->pfn_hi)) {
>> 	return iova;
>> }
> 
> For me, GCC (6.2.1 AArch64) seems to do a pretty good job of this
> function already, so this change only saves two instructions in total
> (pfn is compared against pfn_lo only once instead of twice), which I
> wouldn't expect to see a noticeable performance effect from.
OK, thanks for your careful analysis.

Although only two instructions saved in each loop iteration, but it's also an improvment and no harm.

> 
> Given the improvement in readability, though, I don't even care about
> any codegen differences :)
> 
> Reviewed-by: Robin Murphy <robin.murphy@arm.com>
> 
>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>> ---
>>  drivers/iommu/iova.c | 9 +++------
>>  1 file changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
>> index 8ba8b496..1c49969 100644
>> --- a/drivers/iommu/iova.c
>> +++ b/drivers/iommu/iova.c
>> @@ -312,15 +312,12 @@ private_find_iova(struct iova_domain *iovad, unsigned long pfn)
>>  	while (node) {
>>  		struct iova *iova = rb_entry(node, struct iova, node);
>>  
>> -		/* If pfn falls within iova's range, return iova */
>> -		if ((pfn >= iova->pfn_lo) && (pfn <= iova->pfn_hi)) {
>> -			return iova;
>> -		}
>> -
>>  		if (pfn < iova->pfn_lo)
>>  			node = node->rb_left;
>> -		else if (pfn > iova->pfn_lo)
>> +		else if (pfn > iova->pfn_hi)
>>  			node = node->rb_right;
>> +		else
>> +			return iova;	/* pfn falls within iova's range */
>>  	}
>>  
>>  	return NULL;
>>
> 
> 
> .
> 

-- 
Thanks!
BestRegards

Re: [PATCH 2/7] iommu/iova: cut down judgement times

[Leizhen (ThunderTown)]

On 2017/3/23 20:11, Robin Murphy wrote:
> On 22/03/17 06:27, Zhen Lei wrote:
>> Below judgement can only be satisfied at the last time, which produced 2N
>> judgements(suppose N times failed, 0 or 1 time successed) in vain.
>>
>> if ((pfn >= iova->pfn_lo) && (pfn <= iova->pfn_hi)) {
>> 	return iova;
>> }
> 
> For me, GCC (6.2.1 AArch64) seems to do a pretty good job of this
> function already, so this change only saves two instructions in total
> (pfn is compared against pfn_lo only once instead of twice), which I
> wouldn't expect to see a noticeable performance effect from.
OK, thanks for your careful analysis.

Although only two instructions saved in each loop iteration, but it's also an improvment and no harm.

> 
> Given the improvement in readability, though, I don't even care about
> any codegen differences :)
> 
> Reviewed-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> 
>> Signed-off-by: Zhen Lei <thunder.leizhen-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>> ---
>>  drivers/iommu/iova.c | 9 +++------
>>  1 file changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
>> index 8ba8b496..1c49969 100644
>> --- a/drivers/iommu/iova.c
>> +++ b/drivers/iommu/iova.c
>> @@ -312,15 +312,12 @@ private_find_iova(struct iova_domain *iovad, unsigned long pfn)
>>  	while (node) {
>>  		struct iova *iova = rb_entry(node, struct iova, node);
>>  
>> -		/* If pfn falls within iova's range, return iova */
>> -		if ((pfn >= iova->pfn_lo) && (pfn <= iova->pfn_hi)) {
>> -			return iova;
>> -		}
>> -
>>  		if (pfn < iova->pfn_lo)
>>  			node = node->rb_left;
>> -		else if (pfn > iova->pfn_lo)
>> +		else if (pfn > iova->pfn_hi)
>>  			node = node->rb_right;
>> +		else
>> +			return iova;	/* pfn falls within iova's range */
>>  	}
>>  
>>  	return NULL;
>>
> 
> 
> .
> 

-- 
Thanks!
BestRegards

[PATCH 3/7] iommu/iova: insert start_pfn boundary of dma32

[Zhen Lei]

Reserve the first granule size memory(start at start_pfn) as boundary
iova, to make sure that iovad->cached32_node can not be NULL in future.
Meanwhile, changed the assignment of iovad->cached32_node from rb_next to
rb_prev of &free->node in function __cached_rbnode_delete_update.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 drivers/iommu/iova.c | 63 ++++++++++++++++++++++++++++++----------------------
 1 file changed, 37 insertions(+), 26 deletions(-)

diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index 1c49969..b5a148e 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -32,6 +32,17 @@ static unsigned long iova_rcache_get(struct iova_domain *iovad,
 static void init_iova_rcaches(struct iova_domain *iovad);
 static void free_iova_rcaches(struct iova_domain *iovad);
 
+static void
+insert_iova_boundary(struct iova_domain *iovad)
+{
+	struct iova *iova;
+	unsigned long start_pfn_32bit = iovad->start_pfn;
+
+	iova = reserve_iova(iovad, start_pfn_32bit, start_pfn_32bit);
+	BUG_ON(!iova);
+	iovad->cached32_node = &iova->node;
+}
+
 void
 init_iova_domain(struct iova_domain *iovad, unsigned long granule,
 	unsigned long start_pfn, unsigned long pfn_32bit)
@@ -45,27 +56,38 @@ init_iova_domain(struct iova_domain *iovad, unsigned long granule,
 
 	spin_lock_init(&iovad->iova_rbtree_lock);
 	iovad->rbroot = RB_ROOT;
-	iovad->cached32_node = NULL;
 	iovad->granule = granule;
 	iovad->start_pfn = start_pfn;
 	iovad->dma_32bit_pfn = pfn_32bit;
 	init_iova_rcaches(iovad);
+
+	/*
+	 * Insert boundary nodes for dma32. So cached32_node can not be NULL in
+	 * future.
+	 */
+	insert_iova_boundary(iovad);
 }
 EXPORT_SYMBOL_GPL(init_iova_domain);
 
 static struct rb_node *
 __get_cached_rbnode(struct iova_domain *iovad, unsigned long *limit_pfn)
 {
-	if ((*limit_pfn > iovad->dma_32bit_pfn) ||
-		(iovad->cached32_node == NULL))
+	struct rb_node *cached_node;
+	struct rb_node *next_node;
+
+	if (*limit_pfn > iovad->dma_32bit_pfn)
 		return rb_last(&iovad->rbroot);
-	else {
-		struct rb_node *prev_node = rb_prev(iovad->cached32_node);
-		struct iova *curr_iova =
-			rb_entry(iovad->cached32_node, struct iova, node);
-		*limit_pfn = curr_iova->pfn_lo - 1;
-		return prev_node;
+	else
+		cached_node = iovad->cached32_node;
+
+	next_node = rb_next(cached_node);
+	if (next_node) {
+		struct iova *next_iova = rb_entry(next_node, struct iova, node);
+
+		*limit_pfn = min(*limit_pfn, next_iova->pfn_lo - 1);
 	}
+
+	return cached_node;
 }
 
 static void
@@ -83,20 +105,13 @@ __cached_rbnode_delete_update(struct iova_domain *iovad, struct iova *free)
 	struct iova *cached_iova;
 	struct rb_node *curr;
 
-	if (!iovad->cached32_node)
-		return;
 	curr = iovad->cached32_node;
 	cached_iova = rb_entry(curr, struct iova, node);
 
 	if (free->pfn_lo >= cached_iova->pfn_lo) {
-		struct rb_node *node = rb_next(&free->node);
-		struct iova *iova = rb_entry(node, struct iova, node);
-
 		/* only cache if it's below 32bit pfn */
-		if (node && iova->pfn_lo < iovad->dma_32bit_pfn)
-			iovad->cached32_node = node;
-		else
-			iovad->cached32_node = NULL;
+		if (free->pfn_hi <= iovad->dma_32bit_pfn)
+			iovad->cached32_node = rb_prev(&free->node);
 	}
 }
 
@@ -114,7 +129,7 @@ static int __alloc_and_insert_iova_range(struct iova_domain *iovad,
 		unsigned long size, unsigned long limit_pfn,
 			struct iova *new, bool size_aligned)
 {
-	struct rb_node *prev, *curr = NULL;
+	struct rb_node *prev, *curr;
 	unsigned long flags;
 	unsigned long saved_pfn;
 	unsigned long pad_size = 0;
@@ -144,13 +159,9 @@ static int __alloc_and_insert_iova_range(struct iova_domain *iovad,
 		curr = rb_prev(curr);
 	}
 
-	if (!curr) {
-		if (size_aligned)
-			pad_size = iova_get_pad_size(size, limit_pfn);
-		if ((iovad->start_pfn + size + pad_size) > limit_pfn) {
-			spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
-			return -ENOMEM;
-		}
+	if (unlikely(!curr)) {
+		spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
+		return -ENOMEM;
 	}
 
 	/* pfn_lo will point to size aligned address if size_aligned is set */
-- 
2.5.0

[PATCH 3/7] iommu/iova: insert start_pfn boundary of dma32

[Zhen Lei]

Reserve the first granule size memory(start at start_pfn) as boundary
iova, to make sure that iovad->cached32_node can not be NULL in future.
Meanwhile, changed the assignment of iovad->cached32_node from rb_next to
rb_prev of &free->node in function __cached_rbnode_delete_update.

Signed-off-by: Zhen Lei <thunder.leizhen-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---
 drivers/iommu/iova.c | 63 ++++++++++++++++++++++++++++++----------------------
 1 file changed, 37 insertions(+), 26 deletions(-)

diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index 1c49969..b5a148e 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -32,6 +32,17 @@ static unsigned long iova_rcache_get(struct iova_domain *iovad,
 static void init_iova_rcaches(struct iova_domain *iovad);
 static void free_iova_rcaches(struct iova_domain *iovad);
 
+static void
+insert_iova_boundary(struct iova_domain *iovad)
+{
+	struct iova *iova;
+	unsigned long start_pfn_32bit = iovad->start_pfn;
+
+	iova = reserve_iova(iovad, start_pfn_32bit, start_pfn_32bit);
+	BUG_ON(!iova);
+	iovad->cached32_node = &iova->node;
+}
+
 void
 init_iova_domain(struct iova_domain *iovad, unsigned long granule,
 	unsigned long start_pfn, unsigned long pfn_32bit)
@@ -45,27 +56,38 @@ init_iova_domain(struct iova_domain *iovad, unsigned long granule,
 
 	spin_lock_init(&iovad->iova_rbtree_lock);
 	iovad->rbroot = RB_ROOT;
-	iovad->cached32_node = NULL;
 	iovad->granule = granule;
 	iovad->start_pfn = start_pfn;
 	iovad->dma_32bit_pfn = pfn_32bit;
 	init_iova_rcaches(iovad);
+
+	/*
+	 * Insert boundary nodes for dma32. So cached32_node can not be NULL in
+	 * future.
+	 */
+	insert_iova_boundary(iovad);
 }
 EXPORT_SYMBOL_GPL(init_iova_domain);
 
 static struct rb_node *
 __get_cached_rbnode(struct iova_domain *iovad, unsigned long *limit_pfn)
 {
-	if ((*limit_pfn > iovad->dma_32bit_pfn) ||
-		(iovad->cached32_node == NULL))
+	struct rb_node *cached_node;
+	struct rb_node *next_node;
+
+	if (*limit_pfn > iovad->dma_32bit_pfn)
 		return rb_last(&iovad->rbroot);
-	else {
-		struct rb_node *prev_node = rb_prev(iovad->cached32_node);
-		struct iova *curr_iova =
-			rb_entry(iovad->cached32_node, struct iova, node);
-		*limit_pfn = curr_iova->pfn_lo - 1;
-		return prev_node;
+	else
+		cached_node = iovad->cached32_node;
+
+	next_node = rb_next(cached_node);
+	if (next_node) {
+		struct iova *next_iova = rb_entry(next_node, struct iova, node);
+
+		*limit_pfn = min(*limit_pfn, next_iova->pfn_lo - 1);
 	}
+
+	return cached_node;
 }
 
 static void
@@ -83,20 +105,13 @@ __cached_rbnode_delete_update(struct iova_domain *iovad, struct iova *free)
 	struct iova *cached_iova;
 	struct rb_node *curr;
 
-	if (!iovad->cached32_node)
-		return;
 	curr = iovad->cached32_node;
 	cached_iova = rb_entry(curr, struct iova, node);
 
 	if (free->pfn_lo >= cached_iova->pfn_lo) {
-		struct rb_node *node = rb_next(&free->node);
-		struct iova *iova = rb_entry(node, struct iova, node);
-
 		/* only cache if it's below 32bit pfn */
-		if (node && iova->pfn_lo < iovad->dma_32bit_pfn)
-			iovad->cached32_node = node;
-		else
-			iovad->cached32_node = NULL;
+		if (free->pfn_hi <= iovad->dma_32bit_pfn)
+			iovad->cached32_node = rb_prev(&free->node);
 	}
 }
 
@@ -114,7 +129,7 @@ static int __alloc_and_insert_iova_range(struct iova_domain *iovad,
 		unsigned long size, unsigned long limit_pfn,
 			struct iova *new, bool size_aligned)
 {
-	struct rb_node *prev, *curr = NULL;
+	struct rb_node *prev, *curr;
 	unsigned long flags;
 	unsigned long saved_pfn;
 	unsigned long pad_size = 0;
@@ -144,13 +159,9 @@ static int __alloc_and_insert_iova_range(struct iova_domain *iovad,
 		curr = rb_prev(curr);
 	}
 
-	if (!curr) {
-		if (size_aligned)
-			pad_size = iova_get_pad_size(size, limit_pfn);
-		if ((iovad->start_pfn + size + pad_size) > limit_pfn) {
-			spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
-			return -ENOMEM;
-		}
+	if (unlikely(!curr)) {
+		spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
+		return -ENOMEM;
 	}
 
 	/* pfn_lo will point to size aligned address if size_aligned is set */
-- 
2.5.0

Re: [PATCH 3/7] iommu/iova: insert start_pfn boundary of dma32

[Robin Murphy]

On 22/03/17 06:27, Zhen Lei wrote:
> Reserve the first granule size memory(start at start_pfn) as boundary
> iova, to make sure that iovad->cached32_node can not be NULL in future.
> Meanwhile, changed the assignment of iovad->cached32_node from rb_next to
> rb_prev of &free->node in function __cached_rbnode_delete_update.

I'm not sure I follow this. It's a top-down allocator, so cached32_node
points to the last node allocated (or the node above the last one freed)
on the assumption that there is likely free space directly below there,
thus it's a pretty good place for the next allocation to start searching
from. On the other hand, start_pfn is a hard "do not go below this line"
limit, so it doesn't seem to make any sense to ever point the former at
the latter.

I could understand slightly more if we were reserving the PFN *above*
the cached range, but as-is I don't see what we gain from the change
here, nor what benefit the cached32_node != NULL assumption gives
(AFAICS it would be more useful to simply restrict the cases where it
may be NULL to when the address space is either completely full or
completely empty, or perhaps both).

Robin.

> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> ---
>  drivers/iommu/iova.c | 63 ++++++++++++++++++++++++++++++----------------------
>  1 file changed, 37 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
> index 1c49969..b5a148e 100644
> --- a/drivers/iommu/iova.c
> +++ b/drivers/iommu/iova.c
> @@ -32,6 +32,17 @@ static unsigned long iova_rcache_get(struct iova_domain *iovad,
>  static void init_iova_rcaches(struct iova_domain *iovad);
>  static void free_iova_rcaches(struct iova_domain *iovad);
>  
> +static void
> +insert_iova_boundary(struct iova_domain *iovad)
> +{
> +	struct iova *iova;
> +	unsigned long start_pfn_32bit = iovad->start_pfn;
> +
> +	iova = reserve_iova(iovad, start_pfn_32bit, start_pfn_32bit);
> +	BUG_ON(!iova);
> +	iovad->cached32_node = &iova->node;
> +}
> +
>  void
>  init_iova_domain(struct iova_domain *iovad, unsigned long granule,
>  	unsigned long start_pfn, unsigned long pfn_32bit)
> @@ -45,27 +56,38 @@ init_iova_domain(struct iova_domain *iovad, unsigned long granule,
>  
>  	spin_lock_init(&iovad->iova_rbtree_lock);
>  	iovad->rbroot = RB_ROOT;
> -	iovad->cached32_node = NULL;
>  	iovad->granule = granule;
>  	iovad->start_pfn = start_pfn;
>  	iovad->dma_32bit_pfn = pfn_32bit;
>  	init_iova_rcaches(iovad);
> +
> +	/*
> +	 * Insert boundary nodes for dma32. So cached32_node can not be NULL in
> +	 * future.
> +	 */
> +	insert_iova_boundary(iovad);
>  }
>  EXPORT_SYMBOL_GPL(init_iova_domain);
>  
>  static struct rb_node *
>  __get_cached_rbnode(struct iova_domain *iovad, unsigned long *limit_pfn)
>  {
> -	if ((*limit_pfn > iovad->dma_32bit_pfn) ||
> -		(iovad->cached32_node == NULL))
> +	struct rb_node *cached_node;
> +	struct rb_node *next_node;
> +
> +	if (*limit_pfn > iovad->dma_32bit_pfn)
>  		return rb_last(&iovad->rbroot);
> -	else {
> -		struct rb_node *prev_node = rb_prev(iovad->cached32_node);
> -		struct iova *curr_iova =
> -			rb_entry(iovad->cached32_node, struct iova, node);
> -		*limit_pfn = curr_iova->pfn_lo - 1;
> -		return prev_node;
> +	else
> +		cached_node = iovad->cached32_node;
> +
> +	next_node = rb_next(cached_node);
> +	if (next_node) {
> +		struct iova *next_iova = rb_entry(next_node, struct iova, node);
> +
> +		*limit_pfn = min(*limit_pfn, next_iova->pfn_lo - 1);
>  	}
> +
> +	return cached_node;
>  }
>  
>  static void
> @@ -83,20 +105,13 @@ __cached_rbnode_delete_update(struct iova_domain *iovad, struct iova *free)
>  	struct iova *cached_iova;
>  	struct rb_node *curr;
>  
> -	if (!iovad->cached32_node)
> -		return;
>  	curr = iovad->cached32_node;
>  	cached_iova = rb_entry(curr, struct iova, node);
>  
>  	if (free->pfn_lo >= cached_iova->pfn_lo) {
> -		struct rb_node *node = rb_next(&free->node);
> -		struct iova *iova = rb_entry(node, struct iova, node);
> -
>  		/* only cache if it's below 32bit pfn */
> -		if (node && iova->pfn_lo < iovad->dma_32bit_pfn)
> -			iovad->cached32_node = node;
> -		else
> -			iovad->cached32_node = NULL;
> +		if (free->pfn_hi <= iovad->dma_32bit_pfn)
> +			iovad->cached32_node = rb_prev(&free->node);
>  	}
>  }
>  
> @@ -114,7 +129,7 @@ static int __alloc_and_insert_iova_range(struct iova_domain *iovad,
>  		unsigned long size, unsigned long limit_pfn,
>  			struct iova *new, bool size_aligned)
>  {
> -	struct rb_node *prev, *curr = NULL;
> +	struct rb_node *prev, *curr;
>  	unsigned long flags;
>  	unsigned long saved_pfn;
>  	unsigned long pad_size = 0;
> @@ -144,13 +159,9 @@ static int __alloc_and_insert_iova_range(struct iova_domain *iovad,
>  		curr = rb_prev(curr);
>  	}
>  
> -	if (!curr) {
> -		if (size_aligned)
> -			pad_size = iova_get_pad_size(size, limit_pfn);
> -		if ((iovad->start_pfn + size + pad_size) > limit_pfn) {
> -			spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
> -			return -ENOMEM;
> -		}
> +	if (unlikely(!curr)) {
> +		spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
> +		return -ENOMEM;
>  	}
>  
>  	/* pfn_lo will point to size aligned address if size_aligned is set */
> 

Re: [PATCH 3/7] iommu/iova: insert start_pfn boundary of dma32

[Robin Murphy]

On 22/03/17 06:27, Zhen Lei wrote:
> Reserve the first granule size memory(start at start_pfn) as boundary
> iova, to make sure that iovad->cached32_node can not be NULL in future.
> Meanwhile, changed the assignment of iovad->cached32_node from rb_next to
> rb_prev of &free->node in function __cached_rbnode_delete_update.

I'm not sure I follow this. It's a top-down allocator, so cached32_node
points to the last node allocated (or the node above the last one freed)
on the assumption that there is likely free space directly below there,
thus it's a pretty good place for the next allocation to start searching
from. On the other hand, start_pfn is a hard "do not go below this line"
limit, so it doesn't seem to make any sense to ever point the former at
the latter.

I could understand slightly more if we were reserving the PFN *above*
the cached range, but as-is I don't see what we gain from the change
here, nor what benefit the cached32_node != NULL assumption gives
(AFAICS it would be more useful to simply restrict the cases where it
may be NULL to when the address space is either completely full or
completely empty, or perhaps both).

Robin.

> Signed-off-by: Zhen Lei <thunder.leizhen-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/iommu/iova.c | 63 ++++++++++++++++++++++++++++++----------------------
>  1 file changed, 37 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
> index 1c49969..b5a148e 100644
> --- a/drivers/iommu/iova.c
> +++ b/drivers/iommu/iova.c
> @@ -32,6 +32,17 @@ static unsigned long iova_rcache_get(struct iova_domain *iovad,
>  static void init_iova_rcaches(struct iova_domain *iovad);
>  static void free_iova_rcaches(struct iova_domain *iovad);
>  
> +static void
> +insert_iova_boundary(struct iova_domain *iovad)
> +{
> +	struct iova *iova;
> +	unsigned long start_pfn_32bit = iovad->start_pfn;
> +
> +	iova = reserve_iova(iovad, start_pfn_32bit, start_pfn_32bit);
> +	BUG_ON(!iova);
> +	iovad->cached32_node = &iova->node;
> +}
> +
>  void
>  init_iova_domain(struct iova_domain *iovad, unsigned long granule,
>  	unsigned long start_pfn, unsigned long pfn_32bit)
> @@ -45,27 +56,38 @@ init_iova_domain(struct iova_domain *iovad, unsigned long granule,
>  
>  	spin_lock_init(&iovad->iova_rbtree_lock);
>  	iovad->rbroot = RB_ROOT;
> -	iovad->cached32_node = NULL;
>  	iovad->granule = granule;
>  	iovad->start_pfn = start_pfn;
>  	iovad->dma_32bit_pfn = pfn_32bit;
>  	init_iova_rcaches(iovad);
> +
> +	/*
> +	 * Insert boundary nodes for dma32. So cached32_node can not be NULL in
> +	 * future.
> +	 */
> +	insert_iova_boundary(iovad);
>  }
>  EXPORT_SYMBOL_GPL(init_iova_domain);
>  
>  static struct rb_node *
>  __get_cached_rbnode(struct iova_domain *iovad, unsigned long *limit_pfn)
>  {
> -	if ((*limit_pfn > iovad->dma_32bit_pfn) ||
> -		(iovad->cached32_node == NULL))
> +	struct rb_node *cached_node;
> +	struct rb_node *next_node;
> +
> +	if (*limit_pfn > iovad->dma_32bit_pfn)
>  		return rb_last(&iovad->rbroot);
> -	else {
> -		struct rb_node *prev_node = rb_prev(iovad->cached32_node);
> -		struct iova *curr_iova =
> -			rb_entry(iovad->cached32_node, struct iova, node);
> -		*limit_pfn = curr_iova->pfn_lo - 1;
> -		return prev_node;
> +	else
> +		cached_node = iovad->cached32_node;
> +
> +	next_node = rb_next(cached_node);
> +	if (next_node) {
> +		struct iova *next_iova = rb_entry(next_node, struct iova, node);
> +
> +		*limit_pfn = min(*limit_pfn, next_iova->pfn_lo - 1);
>  	}
> +
> +	return cached_node;
>  }
>  
>  static void
> @@ -83,20 +105,13 @@ __cached_rbnode_delete_update(struct iova_domain *iovad, struct iova *free)
>  	struct iova *cached_iova;
>  	struct rb_node *curr;
>  
> -	if (!iovad->cached32_node)
> -		return;
>  	curr = iovad->cached32_node;
>  	cached_iova = rb_entry(curr, struct iova, node);
>  
>  	if (free->pfn_lo >= cached_iova->pfn_lo) {
> -		struct rb_node *node = rb_next(&free->node);
> -		struct iova *iova = rb_entry(node, struct iova, node);
> -
>  		/* only cache if it's below 32bit pfn */
> -		if (node && iova->pfn_lo < iovad->dma_32bit_pfn)
> -			iovad->cached32_node = node;
> -		else
> -			iovad->cached32_node = NULL;
> +		if (free->pfn_hi <= iovad->dma_32bit_pfn)
> +			iovad->cached32_node = rb_prev(&free->node);
>  	}
>  }
>  
> @@ -114,7 +129,7 @@ static int __alloc_and_insert_iova_range(struct iova_domain *iovad,
>  		unsigned long size, unsigned long limit_pfn,
>  			struct iova *new, bool size_aligned)
>  {
> -	struct rb_node *prev, *curr = NULL;
> +	struct rb_node *prev, *curr;
>  	unsigned long flags;
>  	unsigned long saved_pfn;
>  	unsigned long pad_size = 0;
> @@ -144,13 +159,9 @@ static int __alloc_and_insert_iova_range(struct iova_domain *iovad,
>  		curr = rb_prev(curr);
>  	}
>  
> -	if (!curr) {
> -		if (size_aligned)
> -			pad_size = iova_get_pad_size(size, limit_pfn);
> -		if ((iovad->start_pfn + size + pad_size) > limit_pfn) {
> -			spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
> -			return -ENOMEM;
> -		}
> +	if (unlikely(!curr)) {
> +		spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
> +		return -ENOMEM;
>  	}
>  
>  	/* pfn_lo will point to size aligned address if size_aligned is set */
> 

Re: [PATCH 3/7] iommu/iova: insert start_pfn boundary of dma32

[Leizhen (ThunderTown)]

On 2017/3/23 21:01, Robin Murphy wrote:
> On 22/03/17 06:27, Zhen Lei wrote:
>> Reserve the first granule size memory(start at start_pfn) as boundary
>> iova, to make sure that iovad->cached32_node can not be NULL in future.
>> Meanwhile, changed the assignment of iovad->cached32_node from rb_next to
>> rb_prev of &free->node in function __cached_rbnode_delete_update.
> 
> I'm not sure I follow this. It's a top-down allocator, so cached32_node
> points to the last node allocated (or the node above the last one freed)
> on the assumption that there is likely free space directly below there,
> thus it's a pretty good place for the next allocation to start searching
> from. On the other hand, start_pfn is a hard "do not go below this line"
> limit, so it doesn't seem to make any sense to ever point the former at
> the latter.
This patch just prepares for dma64. Because we really need to add the boundary
between dma32 and dma64, there are two main purposes:
1. to make dma32 iova allocation faster, because the last node which dma32 can be
seen is the boundary. So dma32 iova allocation will only try within dma32 iova space.
Meanwhile, we hope dma64 allocation try dma64 iova space(iova>=4G) first, because the
maxium dma32 iova space is 4GB, dma64 iova space is almost richer than dma32.

2. to prevent a allocated iova cross dma32 and dma64 space. Otherwise, this special
case should be considered when allocate and free iova.

After the above boundary added, it's better to add start_pfn of dma32 boundary also,
to make them to be considered in one model.

After the two boundaries added, adjust cached32/64_node point to the free iova node can
simplified programming.


> 
> I could understand slightly more if we were reserving the PFN *above*
> the cached range, but as-is I don't see what we gain from the change
> here, nor what benefit the cached32_node != NULL assumption gives
> (AFAICS it would be more useful to simply restrict the cases where it
> may be NULL to when the address space is either completely full or
> completely empty, or perhaps both).
> 
> Robin.
> 
>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>> ---
>>  drivers/iommu/iova.c | 63 ++++++++++++++++++++++++++++++----------------------
>>  1 file changed, 37 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
>> index 1c49969..b5a148e 100644
>> --- a/drivers/iommu/iova.c
>> +++ b/drivers/iommu/iova.c
>> @@ -32,6 +32,17 @@ static unsigned long iova_rcache_get(struct iova_domain *iovad,
>>  static void init_iova_rcaches(struct iova_domain *iovad);
>>  static void free_iova_rcaches(struct iova_domain *iovad);
>>  
>> +static void
>> +insert_iova_boundary(struct iova_domain *iovad)
>> +{
>> +	struct iova *iova;
>> +	unsigned long start_pfn_32bit = iovad->start_pfn;
>> +
>> +	iova = reserve_iova(iovad, start_pfn_32bit, start_pfn_32bit);
>> +	BUG_ON(!iova);
>> +	iovad->cached32_node = &iova->node;
>> +}
>> +
>>  void
>>  init_iova_domain(struct iova_domain *iovad, unsigned long granule,
>>  	unsigned long start_pfn, unsigned long pfn_32bit)
>> @@ -45,27 +56,38 @@ init_iova_domain(struct iova_domain *iovad, unsigned long granule,
>>  
>>  	spin_lock_init(&iovad->iova_rbtree_lock);
>>  	iovad->rbroot = RB_ROOT;
>> -	iovad->cached32_node = NULL;
>>  	iovad->granule = granule;
>>  	iovad->start_pfn = start_pfn;
>>  	iovad->dma_32bit_pfn = pfn_32bit;
>>  	init_iova_rcaches(iovad);
>> +
>> +	/*
>> +	 * Insert boundary nodes for dma32. So cached32_node can not be NULL in
>> +	 * future.
>> +	 */
>> +	insert_iova_boundary(iovad);
>>  }
>>  EXPORT_SYMBOL_GPL(init_iova_domain);
>>  
>>  static struct rb_node *
>>  __get_cached_rbnode(struct iova_domain *iovad, unsigned long *limit_pfn)
>>  {
>> -	if ((*limit_pfn > iovad->dma_32bit_pfn) ||
>> -		(iovad->cached32_node == NULL))
>> +	struct rb_node *cached_node;
>> +	struct rb_node *next_node;
>> +
>> +	if (*limit_pfn > iovad->dma_32bit_pfn)
>>  		return rb_last(&iovad->rbroot);
>> -	else {
>> -		struct rb_node *prev_node = rb_prev(iovad->cached32_node);
>> -		struct iova *curr_iova =
>> -			rb_entry(iovad->cached32_node, struct iova, node);
>> -		*limit_pfn = curr_iova->pfn_lo - 1;
>> -		return prev_node;
>> +	else
>> +		cached_node = iovad->cached32_node;
>> +
>> +	next_node = rb_next(cached_node);
>> +	if (next_node) {
>> +		struct iova *next_iova = rb_entry(next_node, struct iova, node);
>> +
>> +		*limit_pfn = min(*limit_pfn, next_iova->pfn_lo - 1);
>>  	}
>> +
>> +	return cached_node;
>>  }
>>  
>>  static void
>> @@ -83,20 +105,13 @@ __cached_rbnode_delete_update(struct iova_domain *iovad, struct iova *free)
>>  	struct iova *cached_iova;
>>  	struct rb_node *curr;
>>  
>> -	if (!iovad->cached32_node)
>> -		return;
>>  	curr = iovad->cached32_node;
>>  	cached_iova = rb_entry(curr, struct iova, node);
>>  
>>  	if (free->pfn_lo >= cached_iova->pfn_lo) {
>> -		struct rb_node *node = rb_next(&free->node);
>> -		struct iova *iova = rb_entry(node, struct iova, node);
>> -
>>  		/* only cache if it's below 32bit pfn */
>> -		if (node && iova->pfn_lo < iovad->dma_32bit_pfn)
>> -			iovad->cached32_node = node;
>> -		else
>> -			iovad->cached32_node = NULL;
>> +		if (free->pfn_hi <= iovad->dma_32bit_pfn)
>> +			iovad->cached32_node = rb_prev(&free->node);
>>  	}
>>  }
>>  
>> @@ -114,7 +129,7 @@ static int __alloc_and_insert_iova_range(struct iova_domain *iovad,
>>  		unsigned long size, unsigned long limit_pfn,
>>  			struct iova *new, bool size_aligned)
>>  {
>> -	struct rb_node *prev, *curr = NULL;
>> +	struct rb_node *prev, *curr;
>>  	unsigned long flags;
>>  	unsigned long saved_pfn;
>>  	unsigned long pad_size = 0;
>> @@ -144,13 +159,9 @@ static int __alloc_and_insert_iova_range(struct iova_domain *iovad,
>>  		curr = rb_prev(curr);
>>  	}
>>  
>> -	if (!curr) {
>> -		if (size_aligned)
>> -			pad_size = iova_get_pad_size(size, limit_pfn);
>> -		if ((iovad->start_pfn + size + pad_size) > limit_pfn) {
>> -			spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
>> -			return -ENOMEM;
>> -		}
>> +	if (unlikely(!curr)) {
>> +		spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
>> +		return -ENOMEM;
>>  	}
>>  
>>  	/* pfn_lo will point to size aligned address if size_aligned is set */
>>
> 
> 
> .
> 

-- 
Thanks!
BestRegards

Re: [PATCH 3/7] iommu/iova: insert start_pfn boundary of dma32

[Leizhen (ThunderTown)]

On 2017/3/23 21:01, Robin Murphy wrote:
> On 22/03/17 06:27, Zhen Lei wrote:
>> Reserve the first granule size memory(start at start_pfn) as boundary
>> iova, to make sure that iovad->cached32_node can not be NULL in future.
>> Meanwhile, changed the assignment of iovad->cached32_node from rb_next to
>> rb_prev of &free->node in function __cached_rbnode_delete_update.
> 
> I'm not sure I follow this. It's a top-down allocator, so cached32_node
> points to the last node allocated (or the node above the last one freed)
> on the assumption that there is likely free space directly below there,
> thus it's a pretty good place for the next allocation to start searching
> from. On the other hand, start_pfn is a hard "do not go below this line"
> limit, so it doesn't seem to make any sense to ever point the former at
> the latter.
This patch just prepares for dma64. Because we really need to add the boundary
between dma32 and dma64, there are two main purposes:
1. to make dma32 iova allocation faster, because the last node which dma32 can be
seen is the boundary. So dma32 iova allocation will only try within dma32 iova space.
Meanwhile, we hope dma64 allocation try dma64 iova space(iova>=4G) first, because the
maxium dma32 iova space is 4GB, dma64 iova space is almost richer than dma32.

2. to prevent a allocated iova cross dma32 and dma64 space. Otherwise, this special
case should be considered when allocate and free iova.

After the above boundary added, it's better to add start_pfn of dma32 boundary also,
to make them to be considered in one model.

After the two boundaries added, adjust cached32/64_node point to the free iova node can
simplified programming.


> 
> I could understand slightly more if we were reserving the PFN *above*
> the cached range, but as-is I don't see what we gain from the change
> here, nor what benefit the cached32_node != NULL assumption gives
> (AFAICS it would be more useful to simply restrict the cases where it
> may be NULL to when the address space is either completely full or
> completely empty, or perhaps both).
> 
> Robin.
> 
>> Signed-off-by: Zhen Lei <thunder.leizhen-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>> ---
>>  drivers/iommu/iova.c | 63 ++++++++++++++++++++++++++++++----------------------
>>  1 file changed, 37 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
>> index 1c49969..b5a148e 100644
>> --- a/drivers/iommu/iova.c
>> +++ b/drivers/iommu/iova.c
>> @@ -32,6 +32,17 @@ static unsigned long iova_rcache_get(struct iova_domain *iovad,
>>  static void init_iova_rcaches(struct iova_domain *iovad);
>>  static void free_iova_rcaches(struct iova_domain *iovad);
>>  
>> +static void
>> +insert_iova_boundary(struct iova_domain *iovad)
>> +{
>> +	struct iova *iova;
>> +	unsigned long start_pfn_32bit = iovad->start_pfn;
>> +
>> +	iova = reserve_iova(iovad, start_pfn_32bit, start_pfn_32bit);
>> +	BUG_ON(!iova);
>> +	iovad->cached32_node = &iova->node;
>> +}
>> +
>>  void
>>  init_iova_domain(struct iova_domain *iovad, unsigned long granule,
>>  	unsigned long start_pfn, unsigned long pfn_32bit)
>> @@ -45,27 +56,38 @@ init_iova_domain(struct iova_domain *iovad, unsigned long granule,
>>  
>>  	spin_lock_init(&iovad->iova_rbtree_lock);
>>  	iovad->rbroot = RB_ROOT;
>> -	iovad->cached32_node = NULL;
>>  	iovad->granule = granule;
>>  	iovad->start_pfn = start_pfn;
>>  	iovad->dma_32bit_pfn = pfn_32bit;
>>  	init_iova_rcaches(iovad);
>> +
>> +	/*
>> +	 * Insert boundary nodes for dma32. So cached32_node can not be NULL in
>> +	 * future.
>> +	 */
>> +	insert_iova_boundary(iovad);
>>  }
>>  EXPORT_SYMBOL_GPL(init_iova_domain);
>>  
>>  static struct rb_node *
>>  __get_cached_rbnode(struct iova_domain *iovad, unsigned long *limit_pfn)
>>  {
>> -	if ((*limit_pfn > iovad->dma_32bit_pfn) ||
>> -		(iovad->cached32_node == NULL))
>> +	struct rb_node *cached_node;
>> +	struct rb_node *next_node;
>> +
>> +	if (*limit_pfn > iovad->dma_32bit_pfn)
>>  		return rb_last(&iovad->rbroot);
>> -	else {
>> -		struct rb_node *prev_node = rb_prev(iovad->cached32_node);
>> -		struct iova *curr_iova =
>> -			rb_entry(iovad->cached32_node, struct iova, node);
>> -		*limit_pfn = curr_iova->pfn_lo - 1;
>> -		return prev_node;
>> +	else
>> +		cached_node = iovad->cached32_node;
>> +
>> +	next_node = rb_next(cached_node);
>> +	if (next_node) {
>> +		struct iova *next_iova = rb_entry(next_node, struct iova, node);
>> +
>> +		*limit_pfn = min(*limit_pfn, next_iova->pfn_lo - 1);
>>  	}
>> +
>> +	return cached_node;
>>  }
>>  
>>  static void
>> @@ -83,20 +105,13 @@ __cached_rbnode_delete_update(struct iova_domain *iovad, struct iova *free)
>>  	struct iova *cached_iova;
>>  	struct rb_node *curr;
>>  
>> -	if (!iovad->cached32_node)
>> -		return;
>>  	curr = iovad->cached32_node;
>>  	cached_iova = rb_entry(curr, struct iova, node);
>>  
>>  	if (free->pfn_lo >= cached_iova->pfn_lo) {
>> -		struct rb_node *node = rb_next(&free->node);
>> -		struct iova *iova = rb_entry(node, struct iova, node);
>> -
>>  		/* only cache if it's below 32bit pfn */
>> -		if (node && iova->pfn_lo < iovad->dma_32bit_pfn)
>> -			iovad->cached32_node = node;
>> -		else
>> -			iovad->cached32_node = NULL;
>> +		if (free->pfn_hi <= iovad->dma_32bit_pfn)
>> +			iovad->cached32_node = rb_prev(&free->node);
>>  	}
>>  }
>>  
>> @@ -114,7 +129,7 @@ static int __alloc_and_insert_iova_range(struct iova_domain *iovad,
>>  		unsigned long size, unsigned long limit_pfn,
>>  			struct iova *new, bool size_aligned)
>>  {
>> -	struct rb_node *prev, *curr = NULL;
>> +	struct rb_node *prev, *curr;
>>  	unsigned long flags;
>>  	unsigned long saved_pfn;
>>  	unsigned long pad_size = 0;
>> @@ -144,13 +159,9 @@ static int __alloc_and_insert_iova_range(struct iova_domain *iovad,
>>  		curr = rb_prev(curr);
>>  	}
>>  
>> -	if (!curr) {
>> -		if (size_aligned)
>> -			pad_size = iova_get_pad_size(size, limit_pfn);
>> -		if ((iovad->start_pfn + size + pad_size) > limit_pfn) {
>> -			spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
>> -			return -ENOMEM;
>> -		}
>> +	if (unlikely(!curr)) {
>> +		spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
>> +		return -ENOMEM;
>>  	}
>>  
>>  	/* pfn_lo will point to size aligned address if size_aligned is set */
>>
> 
> 
> .
> 

-- 
Thanks!
BestRegards

Re: [PATCH 3/7] iommu/iova: insert start_pfn boundary of dma32

[Leizhen (ThunderTown)]

Because the problem of my email-server, all patches sent to Joerg Roedel <joro@8bytes.org> failed.
So I repost this email again.


On 2017/3/24 11:43, Leizhen (ThunderTown) wrote:
> 
> 
> On 2017/3/23 21:01, Robin Murphy wrote:
>> On 22/03/17 06:27, Zhen Lei wrote:
>>> Reserve the first granule size memory(start at start_pfn) as boundary
>>> iova, to make sure that iovad->cached32_node can not be NULL in future.
>>> Meanwhile, changed the assignment of iovad->cached32_node from rb_next to
>>> rb_prev of &free->node in function __cached_rbnode_delete_update.
>>
>> I'm not sure I follow this. It's a top-down allocator, so cached32_node
>> points to the last node allocated (or the node above the last one freed)
>> on the assumption that there is likely free space directly below there,
>> thus it's a pretty good place for the next allocation to start searching
>> from. On the other hand, start_pfn is a hard "do not go below this line"
>> limit, so it doesn't seem to make any sense to ever point the former at
>> the latter.
> This patch just prepares for dma64. Because we really need to add the boundary
> between dma32 and dma64, there are two main purposes:
> 1. to make dma32 iova allocation faster, because the last node which dma32 can be
> seen is the boundary. So dma32 iova allocation will only try within dma32 iova space.
> Meanwhile, we hope dma64 allocation try dma64 iova space(iova>=4G) first, because the
> maxium dma32 iova space is 4GB, dma64 iova space is almost richer than dma32.
> 
> 2. to prevent a allocated iova cross dma32 and dma64 space. Otherwise, this special
> case should be considered when allocate and free iova.
> 
> After the above boundary added, it's better to add start_pfn of dma32 boundary also,
> to make them to be considered in one model.
> 
> After the two boundaries added, adjust cached32/64_node point to the free iova node can
> simplified programming.
> 
> 
>>
>> I could understand slightly more if we were reserving the PFN *above*
>> the cached range, but as-is I don't see what we gain from the change
>> here, nor what benefit the cached32_node != NULL assumption gives
>> (AFAICS it would be more useful to simply restrict the cases where it
>> may be NULL to when the address space is either completely full or
>> completely empty, or perhaps both).
>>
>> Robin.
>>
>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>>> ---
>>>  drivers/iommu/iova.c | 63 ++++++++++++++++++++++++++++++----------------------
>>>  1 file changed, 37 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
>>> index 1c49969..b5a148e 100644
>>> --- a/drivers/iommu/iova.c
>>> +++ b/drivers/iommu/iova.c
>>> @@ -32,6 +32,17 @@ static unsigned long iova_rcache_get(struct iova_domain *iovad,
>>>  static void init_iova_rcaches(struct iova_domain *iovad);
>>>  static void free_iova_rcaches(struct iova_domain *iovad);
>>>  
>>> +static void
>>> +insert_iova_boundary(struct iova_domain *iovad)
>>> +{
>>> +	struct iova *iova;
>>> +	unsigned long start_pfn_32bit = iovad->start_pfn;
>>> +
>>> +	iova = reserve_iova(iovad, start_pfn_32bit, start_pfn_32bit);
>>> +	BUG_ON(!iova);
>>> +	iovad->cached32_node = &iova->node;
>>> +}
>>> +
>>>  void
>>>  init_iova_domain(struct iova_domain *iovad, unsigned long granule,
>>>  	unsigned long start_pfn, unsigned long pfn_32bit)
>>> @@ -45,27 +56,38 @@ init_iova_domain(struct iova_domain *iovad, unsigned long granule,
>>>  
>>>  	spin_lock_init(&iovad->iova_rbtree_lock);
>>>  	iovad->rbroot = RB_ROOT;
>>> -	iovad->cached32_node = NULL;
>>>  	iovad->granule = granule;
>>>  	iovad->start_pfn = start_pfn;
>>>  	iovad->dma_32bit_pfn = pfn_32bit;
>>>  	init_iova_rcaches(iovad);
>>> +
>>> +	/*
>>> +	 * Insert boundary nodes for dma32. So cached32_node can not be NULL in
>>> +	 * future.
>>> +	 */
>>> +	insert_iova_boundary(iovad);
>>>  }
>>>  EXPORT_SYMBOL_GPL(init_iova_domain);
>>>  
>>>  static struct rb_node *
>>>  __get_cached_rbnode(struct iova_domain *iovad, unsigned long *limit_pfn)
>>>  {
>>> -	if ((*limit_pfn > iovad->dma_32bit_pfn) ||
>>> -		(iovad->cached32_node == NULL))
>>> +	struct rb_node *cached_node;
>>> +	struct rb_node *next_node;
>>> +
>>> +	if (*limit_pfn > iovad->dma_32bit_pfn)
>>>  		return rb_last(&iovad->rbroot);
>>> -	else {
>>> -		struct rb_node *prev_node = rb_prev(iovad->cached32_node);
>>> -		struct iova *curr_iova =
>>> -			rb_entry(iovad->cached32_node, struct iova, node);
>>> -		*limit_pfn = curr_iova->pfn_lo - 1;
>>> -		return prev_node;
>>> +	else
>>> +		cached_node = iovad->cached32_node;
>>> +
>>> +	next_node = rb_next(cached_node);
>>> +	if (next_node) {
>>> +		struct iova *next_iova = rb_entry(next_node, struct iova, node);
>>> +
>>> +		*limit_pfn = min(*limit_pfn, next_iova->pfn_lo - 1);
>>>  	}
>>> +
>>> +	return cached_node;
>>>  }
>>>  
>>>  static void
>>> @@ -83,20 +105,13 @@ __cached_rbnode_delete_update(struct iova_domain *iovad, struct iova *free)
>>>  	struct iova *cached_iova;
>>>  	struct rb_node *curr;
>>>  
>>> -	if (!iovad->cached32_node)
>>> -		return;
>>>  	curr = iovad->cached32_node;
>>>  	cached_iova = rb_entry(curr, struct iova, node);
>>>  
>>>  	if (free->pfn_lo >= cached_iova->pfn_lo) {
>>> -		struct rb_node *node = rb_next(&free->node);
>>> -		struct iova *iova = rb_entry(node, struct iova, node);
>>> -
>>>  		/* only cache if it's below 32bit pfn */
>>> -		if (node && iova->pfn_lo < iovad->dma_32bit_pfn)
>>> -			iovad->cached32_node = node;
>>> -		else
>>> -			iovad->cached32_node = NULL;
>>> +		if (free->pfn_hi <= iovad->dma_32bit_pfn)
>>> +			iovad->cached32_node = rb_prev(&free->node);
>>>  	}
>>>  }
>>>  
>>> @@ -114,7 +129,7 @@ static int __alloc_and_insert_iova_range(struct iova_domain *iovad,
>>>  		unsigned long size, unsigned long limit_pfn,
>>>  			struct iova *new, bool size_aligned)
>>>  {
>>> -	struct rb_node *prev, *curr = NULL;
>>> +	struct rb_node *prev, *curr;
>>>  	unsigned long flags;
>>>  	unsigned long saved_pfn;
>>>  	unsigned long pad_size = 0;
>>> @@ -144,13 +159,9 @@ static int __alloc_and_insert_iova_range(struct iova_domain *iovad,
>>>  		curr = rb_prev(curr);
>>>  	}
>>>  
>>> -	if (!curr) {
>>> -		if (size_aligned)
>>> -			pad_size = iova_get_pad_size(size, limit_pfn);
>>> -		if ((iovad->start_pfn + size + pad_size) > limit_pfn) {
>>> -			spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
>>> -			return -ENOMEM;
>>> -		}
>>> +	if (unlikely(!curr)) {
>>> +		spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
>>> +		return -ENOMEM;
>>>  	}
>>>  
>>>  	/* pfn_lo will point to size aligned address if size_aligned is set */
>>>
>>
>>
>> .
>>
> 

-- 
Thanks!
BestRegards

Re: [PATCH 3/7] iommu/iova: insert start_pfn boundary of dma32

[Leizhen (ThunderTown)]

Because the problem of my email-server, all patches sent to Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org> failed.
So I repost this email again.


On 2017/3/24 11:43, Leizhen (ThunderTown) wrote:
> 
> 
> On 2017/3/23 21:01, Robin Murphy wrote:
>> On 22/03/17 06:27, Zhen Lei wrote:
>>> Reserve the first granule size memory(start at start_pfn) as boundary
>>> iova, to make sure that iovad->cached32_node can not be NULL in future.
>>> Meanwhile, changed the assignment of iovad->cached32_node from rb_next to
>>> rb_prev of &free->node in function __cached_rbnode_delete_update.
>>
>> I'm not sure I follow this. It's a top-down allocator, so cached32_node
>> points to the last node allocated (or the node above the last one freed)
>> on the assumption that there is likely free space directly below there,
>> thus it's a pretty good place for the next allocation to start searching
>> from. On the other hand, start_pfn is a hard "do not go below this line"
>> limit, so it doesn't seem to make any sense to ever point the former at
>> the latter.
> This patch just prepares for dma64. Because we really need to add the boundary
> between dma32 and dma64, there are two main purposes:
> 1. to make dma32 iova allocation faster, because the last node which dma32 can be
> seen is the boundary. So dma32 iova allocation will only try within dma32 iova space.
> Meanwhile, we hope dma64 allocation try dma64 iova space(iova>=4G) first, because the
> maxium dma32 iova space is 4GB, dma64 iova space is almost richer than dma32.
> 
> 2. to prevent a allocated iova cross dma32 and dma64 space. Otherwise, this special
> case should be considered when allocate and free iova.
> 
> After the above boundary added, it's better to add start_pfn of dma32 boundary also,
> to make them to be considered in one model.
> 
> After the two boundaries added, adjust cached32/64_node point to the free iova node can
> simplified programming.
> 
> 
>>
>> I could understand slightly more if we were reserving the PFN *above*
>> the cached range, but as-is I don't see what we gain from the change
>> here, nor what benefit the cached32_node != NULL assumption gives
>> (AFAICS it would be more useful to simply restrict the cases where it
>> may be NULL to when the address space is either completely full or
>> completely empty, or perhaps both).
>>
>> Robin.
>>
>>> Signed-off-by: Zhen Lei <thunder.leizhen-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>>> ---
>>>  drivers/iommu/iova.c | 63 ++++++++++++++++++++++++++++++----------------------
>>>  1 file changed, 37 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
>>> index 1c49969..b5a148e 100644
>>> --- a/drivers/iommu/iova.c
>>> +++ b/drivers/iommu/iova.c
>>> @@ -32,6 +32,17 @@ static unsigned long iova_rcache_get(struct iova_domain *iovad,
>>>  static void init_iova_rcaches(struct iova_domain *iovad);
>>>  static void free_iova_rcaches(struct iova_domain *iovad);
>>>  
>>> +static void
>>> +insert_iova_boundary(struct iova_domain *iovad)
>>> +{
>>> +	struct iova *iova;
>>> +	unsigned long start_pfn_32bit = iovad->start_pfn;
>>> +
>>> +	iova = reserve_iova(iovad, start_pfn_32bit, start_pfn_32bit);
>>> +	BUG_ON(!iova);
>>> +	iovad->cached32_node = &iova->node;
>>> +}
>>> +
>>>  void
>>>  init_iova_domain(struct iova_domain *iovad, unsigned long granule,
>>>  	unsigned long start_pfn, unsigned long pfn_32bit)
>>> @@ -45,27 +56,38 @@ init_iova_domain(struct iova_domain *iovad, unsigned long granule,
>>>  
>>>  	spin_lock_init(&iovad->iova_rbtree_lock);
>>>  	iovad->rbroot = RB_ROOT;
>>> -	iovad->cached32_node = NULL;
>>>  	iovad->granule = granule;
>>>  	iovad->start_pfn = start_pfn;
>>>  	iovad->dma_32bit_pfn = pfn_32bit;
>>>  	init_iova_rcaches(iovad);
>>> +
>>> +	/*
>>> +	 * Insert boundary nodes for dma32. So cached32_node can not be NULL in
>>> +	 * future.
>>> +	 */
>>> +	insert_iova_boundary(iovad);
>>>  }
>>>  EXPORT_SYMBOL_GPL(init_iova_domain);
>>>  
>>>  static struct rb_node *
>>>  __get_cached_rbnode(struct iova_domain *iovad, unsigned long *limit_pfn)
>>>  {
>>> -	if ((*limit_pfn > iovad->dma_32bit_pfn) ||
>>> -		(iovad->cached32_node == NULL))
>>> +	struct rb_node *cached_node;
>>> +	struct rb_node *next_node;
>>> +
>>> +	if (*limit_pfn > iovad->dma_32bit_pfn)
>>>  		return rb_last(&iovad->rbroot);
>>> -	else {
>>> -		struct rb_node *prev_node = rb_prev(iovad->cached32_node);
>>> -		struct iova *curr_iova =
>>> -			rb_entry(iovad->cached32_node, struct iova, node);
>>> -		*limit_pfn = curr_iova->pfn_lo - 1;
>>> -		return prev_node;
>>> +	else
>>> +		cached_node = iovad->cached32_node;
>>> +
>>> +	next_node = rb_next(cached_node);
>>> +	if (next_node) {
>>> +		struct iova *next_iova = rb_entry(next_node, struct iova, node);
>>> +
>>> +		*limit_pfn = min(*limit_pfn, next_iova->pfn_lo - 1);
>>>  	}
>>> +
>>> +	return cached_node;
>>>  }
>>>  
>>>  static void
>>> @@ -83,20 +105,13 @@ __cached_rbnode_delete_update(struct iova_domain *iovad, struct iova *free)
>>>  	struct iova *cached_iova;
>>>  	struct rb_node *curr;
>>>  
>>> -	if (!iovad->cached32_node)
>>> -		return;
>>>  	curr = iovad->cached32_node;
>>>  	cached_iova = rb_entry(curr, struct iova, node);
>>>  
>>>  	if (free->pfn_lo >= cached_iova->pfn_lo) {
>>> -		struct rb_node *node = rb_next(&free->node);
>>> -		struct iova *iova = rb_entry(node, struct iova, node);
>>> -
>>>  		/* only cache if it's below 32bit pfn */
>>> -		if (node && iova->pfn_lo < iovad->dma_32bit_pfn)
>>> -			iovad->cached32_node = node;
>>> -		else
>>> -			iovad->cached32_node = NULL;
>>> +		if (free->pfn_hi <= iovad->dma_32bit_pfn)
>>> +			iovad->cached32_node = rb_prev(&free->node);
>>>  	}
>>>  }
>>>  
>>> @@ -114,7 +129,7 @@ static int __alloc_and_insert_iova_range(struct iova_domain *iovad,
>>>  		unsigned long size, unsigned long limit_pfn,
>>>  			struct iova *new, bool size_aligned)
>>>  {
>>> -	struct rb_node *prev, *curr = NULL;
>>> +	struct rb_node *prev, *curr;
>>>  	unsigned long flags;
>>>  	unsigned long saved_pfn;
>>>  	unsigned long pad_size = 0;
>>> @@ -144,13 +159,9 @@ static int __alloc_and_insert_iova_range(struct iova_domain *iovad,
>>>  		curr = rb_prev(curr);
>>>  	}
>>>  
>>> -	if (!curr) {
>>> -		if (size_aligned)
>>> -			pad_size = iova_get_pad_size(size, limit_pfn);
>>> -		if ((iovad->start_pfn + size + pad_size) > limit_pfn) {
>>> -			spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
>>> -			return -ENOMEM;
>>> -		}
>>> +	if (unlikely(!curr)) {
>>> +		spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
>>> +		return -ENOMEM;
>>>  	}
>>>  
>>>  	/* pfn_lo will point to size aligned address if size_aligned is set */
>>>
>>
>>
>> .
>>
> 

-- 
Thanks!
BestRegards

[PATCH 4/7] iommu/iova: adjust __cached_rbnode_insert_update

[Zhen Lei]

For case 2 and 3, adjust cached32_node to the new place, case 1 keep no
change.

For example:
case1: (the right part was allocated)
	|------------------------------|
	|<-----free---->|<--new_iova-->|
	|
	|
   cached32_node

case2: (all was allocated)
	|------------------------------|
	|<---------new_iova----------->|
	|
	|
   cached32_node

case3:
	|-----------------------|......|---------|
        |..free..|<--new_iova-->|<not enough iova space>
        |                              |
        |                              |
   cached32_node(new)             cached32_node(old)

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 drivers/iommu/iova.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index b5a148e..87a9332 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -91,12 +91,16 @@ __get_cached_rbnode(struct iova_domain *iovad, unsigned long *limit_pfn)
 }
 
 static void
-__cached_rbnode_insert_update(struct iova_domain *iovad,
-	unsigned long limit_pfn, struct iova *new)
+__cached_rbnode_insert_update(struct iova_domain *iovad, struct iova *new)
 {
-	if (limit_pfn != iovad->dma_32bit_pfn)
+	struct iova *cached_iova;
+
+	if (new->pfn_hi > iovad->dma_32bit_pfn)
 		return;
-	iovad->cached32_node = &new->node;
+
+	cached_iova = rb_entry(iovad->cached32_node, struct iova, node);
+	if (new->pfn_lo <= cached_iova->pfn_lo)
+		iovad->cached32_node = rb_prev(&new->node);
 }
 
 static void
@@ -131,12 +135,10 @@ static int __alloc_and_insert_iova_range(struct iova_domain *iovad,
 {
 	struct rb_node *prev, *curr;
 	unsigned long flags;
-	unsigned long saved_pfn;
 	unsigned long pad_size = 0;
 
 	/* Walk the tree backwards */
 	spin_lock_irqsave(&iovad->iova_rbtree_lock, flags);
-	saved_pfn = limit_pfn;
 	curr = __get_cached_rbnode(iovad, &limit_pfn);
 	prev = curr;
 	while (curr) {
@@ -197,11 +199,10 @@ static int __alloc_and_insert_iova_range(struct iova_domain *iovad,
 		rb_link_node(&new->node, parent, entry);
 		rb_insert_color(&new->node, &iovad->rbroot);
 	}
-	__cached_rbnode_insert_update(iovad, saved_pfn, new);
+	__cached_rbnode_insert_update(iovad, new);
 
 	spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
 
-
 	return 0;
 }
 
-- 
2.5.0

[PATCH 4/7] iommu/iova: adjust __cached_rbnode_insert_update

[Zhen Lei]

For case 2 and 3, adjust cached32_node to the new place, case 1 keep no
change.

For example:
case1: (the right part was allocated)
	|------------------------------|
	|<-----free---->|<--new_iova-->|
	|
	|
   cached32_node

case2: (all was allocated)
	|------------------------------|
	|<---------new_iova----------->|
	|
	|
   cached32_node

case3:
	|-----------------------|......|---------|
        |..free..|<--new_iova-->|<not enough iova space>
        |                              |
        |                              |
   cached32_node(new)             cached32_node(old)

Signed-off-by: Zhen Lei <thunder.leizhen-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---
 drivers/iommu/iova.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index b5a148e..87a9332 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -91,12 +91,16 @@ __get_cached_rbnode(struct iova_domain *iovad, unsigned long *limit_pfn)
 }
 
 static void
-__cached_rbnode_insert_update(struct iova_domain *iovad,
-	unsigned long limit_pfn, struct iova *new)
+__cached_rbnode_insert_update(struct iova_domain *iovad, struct iova *new)
 {
-	if (limit_pfn != iovad->dma_32bit_pfn)
+	struct iova *cached_iova;
+
+	if (new->pfn_hi > iovad->dma_32bit_pfn)
 		return;
-	iovad->cached32_node = &new->node;
+
+	cached_iova = rb_entry(iovad->cached32_node, struct iova, node);
+	if (new->pfn_lo <= cached_iova->pfn_lo)
+		iovad->cached32_node = rb_prev(&new->node);
 }
 
 static void
@@ -131,12 +135,10 @@ static int __alloc_and_insert_iova_range(struct iova_domain *iovad,
 {
 	struct rb_node *prev, *curr;
 	unsigned long flags;
-	unsigned long saved_pfn;
 	unsigned long pad_size = 0;
 
 	/* Walk the tree backwards */
 	spin_lock_irqsave(&iovad->iova_rbtree_lock, flags);
-	saved_pfn = limit_pfn;
 	curr = __get_cached_rbnode(iovad, &limit_pfn);
 	prev = curr;
 	while (curr) {
@@ -197,11 +199,10 @@ static int __alloc_and_insert_iova_range(struct iova_domain *iovad,
 		rb_link_node(&new->node, parent, entry);
 		rb_insert_color(&new->node, &iovad->rbroot);
 	}
-	__cached_rbnode_insert_update(iovad, saved_pfn, new);
+	__cached_rbnode_insert_update(iovad, new);
 
 	spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
 
-
 	return 0;
 }
 
-- 
2.5.0

[PATCH 5/7] iommu/iova: to optimize the allocation performance of dma64

[Zhen Lei]

Currently we always search free iova space for dma64 begin at the last
node of iovad rb-tree. In the worst case, there maybe too many nodes exist
at the tail, so that we should traverse many times for the first loop in
__alloc_and_insert_iova_range. As we traced, more than 10K times for the
case of iperf.

__alloc_and_insert_iova_range:
	......
	curr = __get_cached_rbnode(iovad, &limit_pfn);
		//--> return rb_last(&iovad->rbroot);
	while (curr) {
		......
		curr = rb_prev(curr);
	}

So add cached64_node to take the same effect as cached32_node, and add
the start_pfn boundary of dma64, to prevent a iova cross both dma32 and
dma64 area.
	|-------------------|------------------------------|
	|<--cached32_node-->|<--------cached64_node------->|
	|                   |
    start_pfn         dma_32bit_pfn + 1

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 drivers/iommu/iova.c | 46 +++++++++++++++++++++++++++-------------------
 include/linux/iova.h |  5 +++--
 2 files changed, 30 insertions(+), 21 deletions(-)

diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index 87a9332..23abe84 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -37,10 +37,15 @@ insert_iova_boundary(struct iova_domain *iovad)
 {
 	struct iova *iova;
 	unsigned long start_pfn_32bit = iovad->start_pfn;
+	unsigned long start_pfn_64bit = iovad->dma_32bit_pfn + 1;
 
 	iova = reserve_iova(iovad, start_pfn_32bit, start_pfn_32bit);
 	BUG_ON(!iova);
 	iovad->cached32_node = &iova->node;
+
+	iova = reserve_iova(iovad, start_pfn_64bit, start_pfn_64bit);
+	BUG_ON(!iova);
+	iovad->cached64_node = &iova->node;
 }
 
 void
@@ -62,8 +67,8 @@ init_iova_domain(struct iova_domain *iovad, unsigned long granule,
 	init_iova_rcaches(iovad);
 
 	/*
-	 * Insert boundary nodes for dma32. So cached32_node can not be NULL in
-	 * future.
+	 * Insert boundary nodes for dma32 and dma64. So cached32_node and
+	 * cached64_node can not be NULL in future.
 	 */
 	insert_iova_boundary(iovad);
 }
@@ -75,10 +80,10 @@ __get_cached_rbnode(struct iova_domain *iovad, unsigned long *limit_pfn)
 	struct rb_node *cached_node;
 	struct rb_node *next_node;
 
-	if (*limit_pfn > iovad->dma_32bit_pfn)
-		return rb_last(&iovad->rbroot);
-	else
+	if (*limit_pfn <= iovad->dma_32bit_pfn)
 		cached_node = iovad->cached32_node;
+	else
+		cached_node = iovad->cached64_node;
 
 	next_node = rb_next(cached_node);
 	if (next_node) {
@@ -94,29 +99,32 @@ static void
 __cached_rbnode_insert_update(struct iova_domain *iovad, struct iova *new)
 {
 	struct iova *cached_iova;
+	struct rb_node **cached_node;
 
-	if (new->pfn_hi > iovad->dma_32bit_pfn)
-		return;
+	if (new->pfn_hi <= iovad->dma_32bit_pfn)
+		cached_node = &iovad->cached32_node;
+	else
+		cached_node = &iovad->cached64_node;
 
-	cached_iova = rb_entry(iovad->cached32_node, struct iova, node);
+	cached_iova = rb_entry(*cached_node, struct iova, node);
 	if (new->pfn_lo <= cached_iova->pfn_lo)
-		iovad->cached32_node = rb_prev(&new->node);
+		*cached_node = rb_prev(&new->node);
 }
 
 static void
 __cached_rbnode_delete_update(struct iova_domain *iovad, struct iova *free)
 {
 	struct iova *cached_iova;
-	struct rb_node *curr;
+	struct rb_node **cached_node;
 
-	curr = iovad->cached32_node;
-	cached_iova = rb_entry(curr, struct iova, node);
+	if (free->pfn_hi <= iovad->dma_32bit_pfn)
+		cached_node = &iovad->cached32_node;
+	else
+		cached_node = &iovad->cached64_node;
 
-	if (free->pfn_lo >= cached_iova->pfn_lo) {
-		/* only cache if it's below 32bit pfn */
-		if (free->pfn_hi <= iovad->dma_32bit_pfn)
-			iovad->cached32_node = rb_prev(&free->node);
-	}
+	cached_iova = rb_entry(*cached_node, struct iova, node);
+	if (free->pfn_lo >= cached_iova->pfn_lo)
+		*cached_node = rb_prev(&free->node);
 }
 
 /*
@@ -283,7 +291,7 @@ EXPORT_SYMBOL_GPL(iova_cache_put);
  * alloc_iova - allocates an iova
  * @iovad: - iova domain in question
  * @size: - size of page frames to allocate
- * @limit_pfn: - max limit address
+ * @limit_pfn: - max limit address(included)
  * @size_aligned: - set if size_aligned address range is required
  * This function allocates an iova in the range iovad->start_pfn to limit_pfn,
  * searching top-down from limit_pfn to iovad->start_pfn. If the size_aligned
@@ -402,7 +410,7 @@ EXPORT_SYMBOL_GPL(free_iova);
  * alloc_iova_fast - allocates an iova from rcache
  * @iovad: - iova domain in question
  * @size: - size of page frames to allocate
- * @limit_pfn: - max limit address
+ * @limit_pfn: - max limit address(included)
  * This function tries to satisfy an iova allocation from the rcache,
  * and falls back to regular allocation on failure.
 */
diff --git a/include/linux/iova.h b/include/linux/iova.h
index f27bb2c..844d723 100644
--- a/include/linux/iova.h
+++ b/include/linux/iova.h
@@ -40,10 +40,11 @@ struct iova_rcache {
 struct iova_domain {
 	spinlock_t	iova_rbtree_lock; /* Lock to protect update of rbtree */
 	struct rb_root	rbroot;		/* iova domain rbtree root */
-	struct rb_node	*cached32_node; /* Save last alloced node */
+	struct rb_node	*cached32_node; /* Save last alloced node, 32bits */
+	struct rb_node	*cached64_node; /* Save last alloced node, 64bits */
 	unsigned long	granule;	/* pfn granularity for this domain */
 	unsigned long	start_pfn;	/* Lower limit for this domain */
-	unsigned long	dma_32bit_pfn;
+	unsigned long	dma_32bit_pfn;	/* max dma32 limit address(included) */
 	struct iova_rcache rcaches[IOVA_RANGE_CACHE_MAX_SIZE];	/* IOVA range caches */
 };
 
-- 
2.5.0

[PATCH 5/7] iommu/iova: to optimize the allocation performance of dma64

[Zhen Lei]

Currently we always search free iova space for dma64 begin at the last
node of iovad rb-tree. In the worst case, there maybe too many nodes exist
at the tail, so that we should traverse many times for the first loop in
__alloc_and_insert_iova_range. As we traced, more than 10K times for the
case of iperf.

__alloc_and_insert_iova_range:
	......
	curr = __get_cached_rbnode(iovad, &limit_pfn);
		//--> return rb_last(&iovad->rbroot);
	while (curr) {
		......
		curr = rb_prev(curr);
	}

So add cached64_node to take the same effect as cached32_node, and add
the start_pfn boundary of dma64, to prevent a iova cross both dma32 and
dma64 area.
	|-------------------|------------------------------|
	|<--cached32_node-->|<--------cached64_node------->|
	|                   |
    start_pfn         dma_32bit_pfn + 1

Signed-off-by: Zhen Lei <thunder.leizhen-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---
 drivers/iommu/iova.c | 46 +++++++++++++++++++++++++++-------------------
 include/linux/iova.h |  5 +++--
 2 files changed, 30 insertions(+), 21 deletions(-)

diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index 87a9332..23abe84 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -37,10 +37,15 @@ insert_iova_boundary(struct iova_domain *iovad)
 {
 	struct iova *iova;
 	unsigned long start_pfn_32bit = iovad->start_pfn;
+	unsigned long start_pfn_64bit = iovad->dma_32bit_pfn + 1;
 
 	iova = reserve_iova(iovad, start_pfn_32bit, start_pfn_32bit);
 	BUG_ON(!iova);
 	iovad->cached32_node = &iova->node;
+
+	iova = reserve_iova(iovad, start_pfn_64bit, start_pfn_64bit);
+	BUG_ON(!iova);
+	iovad->cached64_node = &iova->node;
 }
 
 void
@@ -62,8 +67,8 @@ init_iova_domain(struct iova_domain *iovad, unsigned long granule,
 	init_iova_rcaches(iovad);
 
 	/*
-	 * Insert boundary nodes for dma32. So cached32_node can not be NULL in
-	 * future.
+	 * Insert boundary nodes for dma32 and dma64. So cached32_node and
+	 * cached64_node can not be NULL in future.
 	 */
 	insert_iova_boundary(iovad);
 }
@@ -75,10 +80,10 @@ __get_cached_rbnode(struct iova_domain *iovad, unsigned long *limit_pfn)
 	struct rb_node *cached_node;
 	struct rb_node *next_node;
 
-	if (*limit_pfn > iovad->dma_32bit_pfn)
-		return rb_last(&iovad->rbroot);
-	else
+	if (*limit_pfn <= iovad->dma_32bit_pfn)
 		cached_node = iovad->cached32_node;
+	else
+		cached_node = iovad->cached64_node;
 
 	next_node = rb_next(cached_node);
 	if (next_node) {
@@ -94,29 +99,32 @@ static void
 __cached_rbnode_insert_update(struct iova_domain *iovad, struct iova *new)
 {
 	struct iova *cached_iova;
+	struct rb_node **cached_node;
 
-	if (new->pfn_hi > iovad->dma_32bit_pfn)
-		return;
+	if (new->pfn_hi <= iovad->dma_32bit_pfn)
+		cached_node = &iovad->cached32_node;
+	else
+		cached_node = &iovad->cached64_node;
 
-	cached_iova = rb_entry(iovad->cached32_node, struct iova, node);
+	cached_iova = rb_entry(*cached_node, struct iova, node);
 	if (new->pfn_lo <= cached_iova->pfn_lo)
-		iovad->cached32_node = rb_prev(&new->node);
+		*cached_node = rb_prev(&new->node);
 }
 
 static void
 __cached_rbnode_delete_update(struct iova_domain *iovad, struct iova *free)
 {
 	struct iova *cached_iova;
-	struct rb_node *curr;
+	struct rb_node **cached_node;
 
-	curr = iovad->cached32_node;
-	cached_iova = rb_entry(curr, struct iova, node);
+	if (free->pfn_hi <= iovad->dma_32bit_pfn)
+		cached_node = &iovad->cached32_node;
+	else
+		cached_node = &iovad->cached64_node;
 
-	if (free->pfn_lo >= cached_iova->pfn_lo) {
-		/* only cache if it's below 32bit pfn */
-		if (free->pfn_hi <= iovad->dma_32bit_pfn)
-			iovad->cached32_node = rb_prev(&free->node);
-	}
+	cached_iova = rb_entry(*cached_node, struct iova, node);
+	if (free->pfn_lo >= cached_iova->pfn_lo)
+		*cached_node = rb_prev(&free->node);
 }
 
 /*
@@ -283,7 +291,7 @@ EXPORT_SYMBOL_GPL(iova_cache_put);
  * alloc_iova - allocates an iova
  * @iovad: - iova domain in question
  * @size: - size of page frames to allocate
- * @limit_pfn: - max limit address
+ * @limit_pfn: - max limit address(included)
  * @size_aligned: - set if size_aligned address range is required
  * This function allocates an iova in the range iovad->start_pfn to limit_pfn,
  * searching top-down from limit_pfn to iovad->start_pfn. If the size_aligned
@@ -402,7 +410,7 @@ EXPORT_SYMBOL_GPL(free_iova);
  * alloc_iova_fast - allocates an iova from rcache
  * @iovad: - iova domain in question
  * @size: - size of page frames to allocate
- * @limit_pfn: - max limit address
+ * @limit_pfn: - max limit address(included)
  * This function tries to satisfy an iova allocation from the rcache,
  * and falls back to regular allocation on failure.
 */
diff --git a/include/linux/iova.h b/include/linux/iova.h
index f27bb2c..844d723 100644
--- a/include/linux/iova.h
+++ b/include/linux/iova.h
@@ -40,10 +40,11 @@ struct iova_rcache {
 struct iova_domain {
 	spinlock_t	iova_rbtree_lock; /* Lock to protect update of rbtree */
 	struct rb_root	rbroot;		/* iova domain rbtree root */
-	struct rb_node	*cached32_node; /* Save last alloced node */
+	struct rb_node	*cached32_node; /* Save last alloced node, 32bits */
+	struct rb_node	*cached64_node; /* Save last alloced node, 64bits */
 	unsigned long	granule;	/* pfn granularity for this domain */
 	unsigned long	start_pfn;	/* Lower limit for this domain */
-	unsigned long	dma_32bit_pfn;
+	unsigned long	dma_32bit_pfn;	/* max dma32 limit address(included) */
 	struct iova_rcache rcaches[IOVA_RANGE_CACHE_MAX_SIZE];	/* IOVA range caches */
 };
 
-- 
2.5.0

[PATCH 6/7] iommu/iova: move the caculation of pad mask out of loop

[Zhen Lei]

I'm not sure whether the compiler can optimize it, but move it out will
be better. At least, it does not require lock protection.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 drivers/iommu/iova.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index 23abe84..68754e4 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -127,23 +127,16 @@ __cached_rbnode_delete_update(struct iova_domain *iovad, struct iova *free)
 		*cached_node = rb_prev(&free->node);
 }
 
-/*
- * Computes the padding size required, to make the start address
- * naturally aligned on the power-of-two order of its size
- */
-static unsigned long
-iova_get_pad_size(unsigned long size, unsigned long limit_pfn)
-{
-	return (limit_pfn + 1 - size) & (__roundup_pow_of_two(size) - 1);
-}
-
 static int __alloc_and_insert_iova_range(struct iova_domain *iovad,
 		unsigned long size, unsigned long limit_pfn,
 			struct iova *new, bool size_aligned)
 {
 	struct rb_node *prev, *curr;
 	unsigned long flags;
-	unsigned long pad_size = 0;
+	unsigned long pad_mask, pad_size = 0;
+
+	if (size_aligned)
+		pad_mask = __roundup_pow_of_two(size) - 1;
 
 	/* Walk the tree backwards */
 	spin_lock_irqsave(&iovad->iova_rbtree_lock, flags);
@@ -157,8 +150,13 @@ static int __alloc_and_insert_iova_range(struct iova_domain *iovad,
 		else if (limit_pfn < curr_iova->pfn_hi)
 			goto adjust_limit_pfn;
 		else {
+			/*
+			 * Computes the padding size required, to make the start
+			 * address naturally aligned on the power-of-two order
+			 * of its size
+			 */
 			if (size_aligned)
-				pad_size = iova_get_pad_size(size, limit_pfn);
+				pad_size = (limit_pfn + 1 - size) & pad_mask;
 			if ((curr_iova->pfn_hi + size + pad_size) <= limit_pfn)
 				break;	/* found a free slot */
 		}
-- 
2.5.0

[PATCH 6/7] iommu/iova: move the caculation of pad mask out of loop

[Zhen Lei]

I'm not sure whether the compiler can optimize it, but move it out will
be better. At least, it does not require lock protection.

Signed-off-by: Zhen Lei <thunder.leizhen-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---
 drivers/iommu/iova.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index 23abe84..68754e4 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -127,23 +127,16 @@ __cached_rbnode_delete_update(struct iova_domain *iovad, struct iova *free)
 		*cached_node = rb_prev(&free->node);
 }
 
-/*
- * Computes the padding size required, to make the start address
- * naturally aligned on the power-of-two order of its size
- */
-static unsigned long
-iova_get_pad_size(unsigned long size, unsigned long limit_pfn)
-{
-	return (limit_pfn + 1 - size) & (__roundup_pow_of_two(size) - 1);
-}
-
 static int __alloc_and_insert_iova_range(struct iova_domain *iovad,
 		unsigned long size, unsigned long limit_pfn,
 			struct iova *new, bool size_aligned)
 {
 	struct rb_node *prev, *curr;
 	unsigned long flags;
-	unsigned long pad_size = 0;
+	unsigned long pad_mask, pad_size = 0;
+
+	if (size_aligned)
+		pad_mask = __roundup_pow_of_two(size) - 1;
 
 	/* Walk the tree backwards */
 	spin_lock_irqsave(&iovad->iova_rbtree_lock, flags);
@@ -157,8 +150,13 @@ static int __alloc_and_insert_iova_range(struct iova_domain *iovad,
 		else if (limit_pfn < curr_iova->pfn_hi)
 			goto adjust_limit_pfn;
 		else {
+			/*
+			 * Computes the padding size required, to make the start
+			 * address naturally aligned on the power-of-two order
+			 * of its size
+			 */
 			if (size_aligned)
-				pad_size = iova_get_pad_size(size, limit_pfn);
+				pad_size = (limit_pfn + 1 - size) & pad_mask;
 			if ((curr_iova->pfn_hi + size + pad_size) <= limit_pfn)
 				break;	/* found a free slot */
 		}
-- 
2.5.0

[PATCH 7/7] iommu/iova: fix iovad->dma_32bit_pfn as the last pfn of dma32

[Zhen Lei]

To make sure iovad->cached32_node and iovad->cached64_node can exactly
control dma32 and dma64 area. It also help us to remove the parameter
pfn_32bit of init_iova_domain.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 drivers/iommu/amd_iommu.c        |  7 ++-----
 drivers/iommu/dma-iommu.c        | 22 +++++-----------------
 drivers/iommu/intel-iommu.c      | 11 +++--------
 drivers/iommu/iova.c             |  4 ++--
 drivers/misc/mic/scif/scif_rma.c |  3 +--
 include/linux/iova.h             |  2 +-
 6 files changed, 14 insertions(+), 35 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 98940d1..78c8b93 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -61,7 +61,6 @@
 /* IO virtual address start page frame number */
 #define IOVA_START_PFN		(1)
 #define IOVA_PFN(addr)		((addr) >> PAGE_SHIFT)
-#define DMA_32BIT_PFN		IOVA_PFN(DMA_BIT_MASK(32))
 
 /* Reserved IOVA ranges */
 #define MSI_RANGE_START		(0xfee00000)
@@ -1776,8 +1775,7 @@ static struct dma_ops_domain *dma_ops_domain_alloc(void)
 	if (!dma_dom->domain.pt_root)
 		goto free_dma_dom;
 
-	init_iova_domain(&dma_dom->iovad, PAGE_SIZE,
-			 IOVA_START_PFN, DMA_32BIT_PFN);
+	init_iova_domain(&dma_dom->iovad, PAGE_SIZE, IOVA_START_PFN);
 
 	/* Initialize reserved ranges */
 	copy_reserved_iova(&reserved_iova_ranges, &dma_dom->iovad);
@@ -2747,8 +2745,7 @@ static int init_reserved_iova_ranges(void)
 	struct pci_dev *pdev = NULL;
 	struct iova *val;
 
-	init_iova_domain(&reserved_iova_ranges, PAGE_SIZE,
-			 IOVA_START_PFN, DMA_32BIT_PFN);
+	init_iova_domain(&reserved_iova_ranges, PAGE_SIZE, IOVA_START_PFN);
 
 	lockdep_set_class(&reserved_iova_ranges.iova_rbtree_lock,
 			  &reserved_rbtree_key);
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 48d36ce..7064d32 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -223,18 +223,7 @@ int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,
 		/* ...then finally give it a kicking to make sure it fits */
 		base_pfn = max_t(unsigned long, base_pfn,
 				domain->geometry.aperture_start >> order);
-		end_pfn = min_t(unsigned long, end_pfn,
-				domain->geometry.aperture_end >> order);
 	}
-	/*
-	 * PCI devices may have larger DMA masks, but still prefer allocating
-	 * within a 32-bit mask to avoid DAC addressing. Such limitations don't
-	 * apply to the typical platform device, so for those we may as well
-	 * leave the cache limit at the top of their range to save an rb_last()
-	 * traversal on every allocation.
-	 */
-	if (pci)
-		end_pfn &= DMA_BIT_MASK(32) >> order;
 
 	/* start_pfn is always nonzero for an already-initialised domain */
 	if (iovad->start_pfn) {
@@ -243,16 +232,15 @@ int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,
 			pr_warn("Incompatible range for DMA domain\n");
 			return -EFAULT;
 		}
-		/*
-		 * If we have devices with different DMA masks, move the free
-		 * area cache limit down for the benefit of the smaller one.
-		 */
-		iovad->dma_32bit_pfn = min(end_pfn, iovad->dma_32bit_pfn);
 	} else {
-		init_iova_domain(iovad, 1UL << order, base_pfn, end_pfn);
+		init_iova_domain(iovad, 1UL << order, base_pfn);
 		if (pci)
 			iova_reserve_pci_windows(to_pci_dev(dev), iovad);
 	}
+
+	if (end_pfn < iovad->dma_32bit_pfn)
+		dev_dbg(dev, "ancient device or dma range missed some bits?");
+
 	return 0;
 }
 EXPORT_SYMBOL(iommu_dma_init_domain);
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 238ad34..de467c1 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -82,8 +82,6 @@
 #define IOVA_START_PFN		(1)
 
 #define IOVA_PFN(addr)		((addr) >> PAGE_SHIFT)
-#define DMA_32BIT_PFN		IOVA_PFN(DMA_BIT_MASK(32))
-#define DMA_64BIT_PFN		IOVA_PFN(DMA_BIT_MASK(64))
 
 /* page table handling */
 #define LEVEL_STRIDE		(9)
@@ -1869,8 +1867,7 @@ static int dmar_init_reserved_ranges(void)
 	struct iova *iova;
 	int i;
 
-	init_iova_domain(&reserved_iova_list, VTD_PAGE_SIZE, IOVA_START_PFN,
-			DMA_32BIT_PFN);
+	init_iova_domain(&reserved_iova_list, VTD_PAGE_SIZE, IOVA_START_PFN);
 
 	lockdep_set_class(&reserved_iova_list.iova_rbtree_lock,
 		&reserved_rbtree_key);
@@ -1928,8 +1925,7 @@ static int domain_init(struct dmar_domain *domain, struct intel_iommu *iommu,
 	int adjust_width, agaw;
 	unsigned long sagaw;
 
-	init_iova_domain(&domain->iovad, VTD_PAGE_SIZE, IOVA_START_PFN,
-			DMA_32BIT_PFN);
+	init_iova_domain(&domain->iovad, VTD_PAGE_SIZE, IOVA_START_PFN);
 	domain_reserve_special_ranges(domain);
 
 	/* calculate AGAW */
@@ -4965,8 +4961,7 @@ static int md_domain_init(struct dmar_domain *domain, int guest_width)
 {
 	int adjust_width;
 
-	init_iova_domain(&domain->iovad, VTD_PAGE_SIZE, IOVA_START_PFN,
-			DMA_32BIT_PFN);
+	init_iova_domain(&domain->iovad, VTD_PAGE_SIZE, IOVA_START_PFN);
 	domain_reserve_special_ranges(domain);
 
 	/* calculate AGAW */
diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index 68754e4..43fd8e9 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -50,7 +50,7 @@ insert_iova_boundary(struct iova_domain *iovad)
 
 void
 init_iova_domain(struct iova_domain *iovad, unsigned long granule,
-	unsigned long start_pfn, unsigned long pfn_32bit)
+	unsigned long start_pfn)
 {
 	/*
 	 * IOVA granularity will normally be equal to the smallest
@@ -63,7 +63,7 @@ init_iova_domain(struct iova_domain *iovad, unsigned long granule,
 	iovad->rbroot = RB_ROOT;
 	iovad->granule = granule;
 	iovad->start_pfn = start_pfn;
-	iovad->dma_32bit_pfn = pfn_32bit;
+	iovad->dma_32bit_pfn = DMA_BIT_MASK(32) >> ilog2(granule);
 	init_iova_rcaches(iovad);
 
 	/*
diff --git a/drivers/misc/mic/scif/scif_rma.c b/drivers/misc/mic/scif/scif_rma.c
index 329727e..c824329 100644
--- a/drivers/misc/mic/scif/scif_rma.c
+++ b/drivers/misc/mic/scif/scif_rma.c
@@ -39,8 +39,7 @@ void scif_rma_ep_init(struct scif_endpt *ep)
 	struct scif_endpt_rma_info *rma = &ep->rma_info;
 
 	mutex_init(&rma->rma_lock);
-	init_iova_domain(&rma->iovad, PAGE_SIZE, SCIF_IOVA_START_PFN,
-			 SCIF_DMA_64BIT_PFN);
+	init_iova_domain(&rma->iovad, PAGE_SIZE, SCIF_IOVA_START_PFN);
 	spin_lock_init(&rma->tc_lock);
 	mutex_init(&rma->mmn_lock);
 	INIT_LIST_HEAD(&rma->reg_list);
diff --git a/include/linux/iova.h b/include/linux/iova.h
index 844d723..6f037b2 100644
--- a/include/linux/iova.h
+++ b/include/linux/iova.h
@@ -101,7 +101,7 @@ struct iova *reserve_iova(struct iova_domain *iovad, unsigned long pfn_lo,
 	unsigned long pfn_hi);
 void copy_reserved_iova(struct iova_domain *from, struct iova_domain *to);
 void init_iova_domain(struct iova_domain *iovad, unsigned long granule,
-	unsigned long start_pfn, unsigned long pfn_32bit);
+	unsigned long start_pfn);
 struct iova *find_iova(struct iova_domain *iovad, unsigned long pfn);
 void put_iova_domain(struct iova_domain *iovad);
 struct iova *split_and_remove_iova(struct iova_domain *iovad,
-- 
2.5.0

[PATCH 7/7] iommu/iova: fix iovad->dma_32bit_pfn as the last pfn of dma32

[Zhen Lei]

To make sure iovad->cached32_node and iovad->cached64_node can exactly
control dma32 and dma64 area. It also help us to remove the parameter
pfn_32bit of init_iova_domain.

Signed-off-by: Zhen Lei <thunder.leizhen-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---
 drivers/iommu/amd_iommu.c        |  7 ++-----
 drivers/iommu/dma-iommu.c        | 22 +++++-----------------
 drivers/iommu/intel-iommu.c      | 11 +++--------
 drivers/iommu/iova.c             |  4 ++--
 drivers/misc/mic/scif/scif_rma.c |  3 +--
 include/linux/iova.h             |  2 +-
 6 files changed, 14 insertions(+), 35 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 98940d1..78c8b93 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -61,7 +61,6 @@
 /* IO virtual address start page frame number */
 #define IOVA_START_PFN		(1)
 #define IOVA_PFN(addr)		((addr) >> PAGE_SHIFT)
-#define DMA_32BIT_PFN		IOVA_PFN(DMA_BIT_MASK(32))
 
 /* Reserved IOVA ranges */
 #define MSI_RANGE_START		(0xfee00000)
@@ -1776,8 +1775,7 @@ static struct dma_ops_domain *dma_ops_domain_alloc(void)
 	if (!dma_dom->domain.pt_root)
 		goto free_dma_dom;
 
-	init_iova_domain(&dma_dom->iovad, PAGE_SIZE,
-			 IOVA_START_PFN, DMA_32BIT_PFN);
+	init_iova_domain(&dma_dom->iovad, PAGE_SIZE, IOVA_START_PFN);
 
 	/* Initialize reserved ranges */
 	copy_reserved_iova(&reserved_iova_ranges, &dma_dom->iovad);
@@ -2747,8 +2745,7 @@ static int init_reserved_iova_ranges(void)
 	struct pci_dev *pdev = NULL;
 	struct iova *val;
 
-	init_iova_domain(&reserved_iova_ranges, PAGE_SIZE,
-			 IOVA_START_PFN, DMA_32BIT_PFN);
+	init_iova_domain(&reserved_iova_ranges, PAGE_SIZE, IOVA_START_PFN);
 
 	lockdep_set_class(&reserved_iova_ranges.iova_rbtree_lock,
 			  &reserved_rbtree_key);
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 48d36ce..7064d32 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -223,18 +223,7 @@ int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,
 		/* ...then finally give it a kicking to make sure it fits */
 		base_pfn = max_t(unsigned long, base_pfn,
 				domain->geometry.aperture_start >> order);
-		end_pfn = min_t(unsigned long, end_pfn,
-				domain->geometry.aperture_end >> order);
 	}
-	/*
-	 * PCI devices may have larger DMA masks, but still prefer allocating
-	 * within a 32-bit mask to avoid DAC addressing. Such limitations don't
-	 * apply to the typical platform device, so for those we may as well
-	 * leave the cache limit at the top of their range to save an rb_last()
-	 * traversal on every allocation.
-	 */
-	if (pci)
-		end_pfn &= DMA_BIT_MASK(32) >> order;
 
 	/* start_pfn is always nonzero for an already-initialised domain */
 	if (iovad->start_pfn) {
@@ -243,16 +232,15 @@ int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,
 			pr_warn("Incompatible range for DMA domain\n");
 			return -EFAULT;
 		}
-		/*
-		 * If we have devices with different DMA masks, move the free
-		 * area cache limit down for the benefit of the smaller one.
-		 */
-		iovad->dma_32bit_pfn = min(end_pfn, iovad->dma_32bit_pfn);
 	} else {
-		init_iova_domain(iovad, 1UL << order, base_pfn, end_pfn);
+		init_iova_domain(iovad, 1UL << order, base_pfn);
 		if (pci)
 			iova_reserve_pci_windows(to_pci_dev(dev), iovad);
 	}
+
+	if (end_pfn < iovad->dma_32bit_pfn)
+		dev_dbg(dev, "ancient device or dma range missed some bits?");
+
 	return 0;
 }
 EXPORT_SYMBOL(iommu_dma_init_domain);
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 238ad34..de467c1 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -82,8 +82,6 @@
 #define IOVA_START_PFN		(1)
 
 #define IOVA_PFN(addr)		((addr) >> PAGE_SHIFT)
-#define DMA_32BIT_PFN		IOVA_PFN(DMA_BIT_MASK(32))
-#define DMA_64BIT_PFN		IOVA_PFN(DMA_BIT_MASK(64))
 
 /* page table handling */
 #define LEVEL_STRIDE		(9)
@@ -1869,8 +1867,7 @@ static int dmar_init_reserved_ranges(void)
 	struct iova *iova;
 	int i;
 
-	init_iova_domain(&reserved_iova_list, VTD_PAGE_SIZE, IOVA_START_PFN,
-			DMA_32BIT_PFN);
+	init_iova_domain(&reserved_iova_list, VTD_PAGE_SIZE, IOVA_START_PFN);
 
 	lockdep_set_class(&reserved_iova_list.iova_rbtree_lock,
 		&reserved_rbtree_key);
@@ -1928,8 +1925,7 @@ static int domain_init(struct dmar_domain *domain, struct intel_iommu *iommu,
 	int adjust_width, agaw;
 	unsigned long sagaw;
 
-	init_iova_domain(&domain->iovad, VTD_PAGE_SIZE, IOVA_START_PFN,
-			DMA_32BIT_PFN);
+	init_iova_domain(&domain->iovad, VTD_PAGE_SIZE, IOVA_START_PFN);
 	domain_reserve_special_ranges(domain);
 
 	/* calculate AGAW */
@@ -4965,8 +4961,7 @@ static int md_domain_init(struct dmar_domain *domain, int guest_width)
 {
 	int adjust_width;
 
-	init_iova_domain(&domain->iovad, VTD_PAGE_SIZE, IOVA_START_PFN,
-			DMA_32BIT_PFN);
+	init_iova_domain(&domain->iovad, VTD_PAGE_SIZE, IOVA_START_PFN);
 	domain_reserve_special_ranges(domain);
 
 	/* calculate AGAW */
diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index 68754e4..43fd8e9 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -50,7 +50,7 @@ insert_iova_boundary(struct iova_domain *iovad)
 
 void
 init_iova_domain(struct iova_domain *iovad, unsigned long granule,
-	unsigned long start_pfn, unsigned long pfn_32bit)
+	unsigned long start_pfn)
 {
 	/*
 	 * IOVA granularity will normally be equal to the smallest
@@ -63,7 +63,7 @@ init_iova_domain(struct iova_domain *iovad, unsigned long granule,
 	iovad->rbroot = RB_ROOT;
 	iovad->granule = granule;
 	iovad->start_pfn = start_pfn;
-	iovad->dma_32bit_pfn = pfn_32bit;
+	iovad->dma_32bit_pfn = DMA_BIT_MASK(32) >> ilog2(granule);
 	init_iova_rcaches(iovad);
 
 	/*
diff --git a/drivers/misc/mic/scif/scif_rma.c b/drivers/misc/mic/scif/scif_rma.c
index 329727e..c824329 100644
--- a/drivers/misc/mic/scif/scif_rma.c
+++ b/drivers/misc/mic/scif/scif_rma.c
@@ -39,8 +39,7 @@ void scif_rma_ep_init(struct scif_endpt *ep)
 	struct scif_endpt_rma_info *rma = &ep->rma_info;
 
 	mutex_init(&rma->rma_lock);
-	init_iova_domain(&rma->iovad, PAGE_SIZE, SCIF_IOVA_START_PFN,
-			 SCIF_DMA_64BIT_PFN);
+	init_iova_domain(&rma->iovad, PAGE_SIZE, SCIF_IOVA_START_PFN);
 	spin_lock_init(&rma->tc_lock);
 	mutex_init(&rma->mmn_lock);
 	INIT_LIST_HEAD(&rma->reg_list);
diff --git a/include/linux/iova.h b/include/linux/iova.h
index 844d723..6f037b2 100644
--- a/include/linux/iova.h
+++ b/include/linux/iova.h
@@ -101,7 +101,7 @@ struct iova *reserve_iova(struct iova_domain *iovad, unsigned long pfn_lo,
 	unsigned long pfn_hi);
 void copy_reserved_iova(struct iova_domain *from, struct iova_domain *to);
 void init_iova_domain(struct iova_domain *iovad, unsigned long granule,
-	unsigned long start_pfn, unsigned long pfn_32bit);
+	unsigned long start_pfn);
 struct iova *find_iova(struct iova_domain *iovad, unsigned long pfn);
 void put_iova_domain(struct iova_domain *iovad);
 struct iova *split_and_remove_iova(struct iova_domain *iovad,
-- 
2.5.0