Re: [PATCH 1/1] arm64/dma-mapping: remove an unnecessary conversion

[Leizhen (ThunderTown)]

On 2016/3/17 19:59, Catalin Marinas wrote:
> On Thu, Mar 17, 2016 at 07:06:27PM +0800, Leizhen (ThunderTown) wrote:
>> On 2016/3/16 9:56, Leizhen (ThunderTown) wrote:
>>> On 2016/3/15 23:37, Catalin Marinas wrote:
 On Tue, Mar 15, 2016 at 10:12:11AM +0800, Zhen Lei wrote:
> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
> index a6e757c..b2f2834 100644
> --- a/arch/arm64/mm/dma-mapping.c
> +++ b/arch/arm64/mm/dma-mapping.c
> @@ -187,8 +187,6 @@ static void __dma_free(struct device *dev, size_t 
> size,
>  void *vaddr, dma_addr_t dma_handle,
>  struct dma_attrs *attrs)
>  {
> - void *swiotlb_addr = phys_to_virt(dma_to_phys(dev, dma_handle));
> -
>   size = PAGE_ALIGN(size);
>
>   if (!is_device_dma_coherent(dev)) {
> @@ -196,7 +194,7 @@ static void __dma_free(struct device *dev, size_t 
> size,
>   return;
>   vunmap(vaddr);
>   }
> - __dma_free_coherent(dev, size, swiotlb_addr, dma_handle, attrs);
> + __dma_free_coherent(dev, size, vaddr, dma_handle, attrs);
>  }

 What happens when !is_device_dma_coherent(dev)? (hint: read two lines
 above __dma_free_coherent).
>>
>> Do you afraid "vaddr" maybe modified by these statement?
>> First, it could not be __free_from_pool. Otherwise, the function
>> vunmap(which after it) can not work well. Then, it count not be vunmap
>> too, the parameter is defined as "const void *".
>>
>> In the call chain:
>> __dma_free_coherent-->__dma_free_coherent-->swiotlb_free_coherent,
>> only swiotlb_free_coherent finally use "vaddr".
> 
> Exactly. So you give swiotlb_free_coherent a vaddr which has been
> unmapped. It doesn't even matter whether it's still mapped since this
> address is passed further to free_pages() which performs a
> virt_to_page(). The latter is *only* valid on linear map addresses (and
> you would actually hit the VM_BUG_ON in free_pages; you can try running
> this with CONFIG_DEBUG_VM enabled and non-coherent DMA).
> 
> For non-coherent DMA, the vaddr is not part of the linear mapping as it
> has been remapped by __dma_alloc() via dma_common_contiguous_remap(),
> hence for swiotlb freeing we need the actual linear map address (the
> original "ptr" in __dma_alloc()). We can generate it by a
> phys_to_virt(dma_to_phys(dma_handle)).
> 

OK, I got it.

So actually I should move the statement into branch "if 
(!is_device_dma_coherent(dev))", I will prepare v2.

Re: [PATCH 1/1] arm64/dma-mapping: remove an unnecessary conversion

[Leizhen (ThunderTown)]

On 2016/3/17 19:59, Catalin Marinas wrote:
> On Thu, Mar 17, 2016 at 07:06:27PM +0800, Leizhen (ThunderTown) wrote:
>> On 2016/3/16 9:56, Leizhen (ThunderTown) wrote:
>>> On 2016/3/15 23:37, Catalin Marinas wrote:
 On Tue, Mar 15, 2016 at 10:12:11AM +0800, Zhen Lei wrote:
> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
> index a6e757c..b2f2834 100644
> --- a/arch/arm64/mm/dma-mapping.c
> +++ b/arch/arm64/mm/dma-mapping.c
> @@ -187,8 +187,6 @@ static void __dma_free(struct device *dev, size_t 
> size,
>  void *vaddr, dma_addr_t dma_handle,
>  struct dma_attrs *attrs)
>  {
> - void *swiotlb_addr = phys_to_virt(dma_to_phys(dev, dma_handle));
> -
>   size = PAGE_ALIGN(size);
>
>   if (!is_device_dma_coherent(dev)) {
> @@ -196,7 +194,7 @@ static void __dma_free(struct device *dev, size_t 
> size,
>   return;
>   vunmap(vaddr);
>   }
> - __dma_free_coherent(dev, size, swiotlb_addr, dma_handle, attrs);
> + __dma_free_coherent(dev, size, vaddr, dma_handle, attrs);
>  }

 What happens when !is_device_dma_coherent(dev)? (hint: read two lines
 above __dma_free_coherent).
>>
>> Do you afraid "vaddr" maybe modified by these statement?
>> First, it could not be __free_from_pool. Otherwise, the function
>> vunmap(which after it) can not work well. Then, it count not be vunmap
>> too, the parameter is defined as "const void *".
>>
>> In the call chain:
>> __dma_free_coherent-->__dma_free_coherent-->swiotlb_free_coherent,
>> only swiotlb_free_coherent finally use "vaddr".
> 
> Exactly. So you give swiotlb_free_coherent a vaddr which has been
> unmapped. It doesn't even matter whether it's still mapped since this
> address is passed further to free_pages() which performs a
> virt_to_page(). The latter is *only* valid on linear map addresses (and
> you would actually hit the VM_BUG_ON in free_pages; you can try running
> this with CONFIG_DEBUG_VM enabled and non-coherent DMA).
> 
> For non-coherent DMA, the vaddr is not part of the linear mapping as it
> has been remapped by __dma_alloc() via dma_common_contiguous_remap(),
> hence for swiotlb freeing we need the actual linear map address (the
> original "ptr" in __dma_alloc()). We can generate it by a
> phys_to_virt(dma_to_phys(dma_handle)).
> 

OK, I got it.

So actually I should move the statement into branch "if 
(!is_device_dma_coherent(dev))", I will prepare v2.

Re: [PATCH 1/1] arm64/dma-mapping: remove an unnecessary conversion

[Leizhen (ThunderTown)]

On 2016/3/16 9:56, Leizhen (ThunderTown) wrote:
> 
> 
> On 2016/3/15 23:37, Catalin Marinas wrote:
>> On Tue, Mar 15, 2016 at 10:12:11AM +0800, Zhen Lei wrote:
>>> 1. In swiotlb_alloc_coherent, the branch of __get_free_pages. Directly
>>>return vaddr on success, and pass vaddr to free_pages on failure.
>>> 2. So, we can directly transparent pass vaddr from __dma_free to
>>>swiotlb_free_coherent, keep consistent with swiotlb_alloc_coherent.
>>>
>>> This patch have no functional change,
>>
>> I don't think so.
>>
>>> but can obtain a bit performance improvement.
>>
>> Have you actually measured it?
> I have not run any performance testing, but reduced a line of code. So I said 
> "a bit".
> 
>>
>>> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
>>> index a6e757c..b2f2834 100644
>>> --- a/arch/arm64/mm/dma-mapping.c
>>> +++ b/arch/arm64/mm/dma-mapping.c
>>> @@ -187,8 +187,6 @@ static void __dma_free(struct device *dev, size_t size,
>>>void *vaddr, dma_addr_t dma_handle,
>>>struct dma_attrs *attrs)
>>>  {
>>> -   void *swiotlb_addr = phys_to_virt(dma_to_phys(dev, dma_handle));
>>> -
>>> size = PAGE_ALIGN(size);
>>>
>>> if (!is_device_dma_coherent(dev)) {
>>> @@ -196,7 +194,7 @@ static void __dma_free(struct device *dev, size_t size,
>>> return;
>>> vunmap(vaddr);
>>> }
>>> -   __dma_free_coherent(dev, size, swiotlb_addr, dma_handle, attrs);
>>> +   __dma_free_coherent(dev, size, vaddr, dma_handle, attrs);
>>>  }
>>
>> What happens when !is_device_dma_coherent(dev)? (hint: read two lines
>> above __dma_free_coherent).
Do you afraid "vaddr" maybe modified by these statement?
First, it could not be __free_from_pool. Otherwise, the function vunmap(which 
after it) can not work well.
Then, it count not be vunmap too, the parameter is defined as "const void *".

In the call chain: 
__dma_free_coherent-->__dma_free_coherent-->swiotlb_free_coherent, only 
swiotlb_free_coherent finally use "vaddr".

>>
> The whole function of __dma_free as below: (nobody use swiotlb_addr except 
> __dma_free_coherent)
> static void __dma_free(struct device *dev, size_t size,
>void *vaddr, dma_addr_t dma_handle,
>struct dma_attrs *attrs)
> {
> void *swiotlb_addr = phys_to_virt(dma_to_phys(dev, dma_handle));
> 
> size = PAGE_ALIGN(size);
> 
> if (!is_device_dma_coherent(dev)) {
> if (__free_from_pool(vaddr, size))
> return;
> vunmap(vaddr);
> }
> __dma_free_coherent(dev, size, swiotlb_addr, dma_handle, attrs);
> }
> 

Re: [PATCH 1/1] arm64/dma-mapping: remove an unnecessary conversion

[Leizhen (ThunderTown)]

On 2016/3/16 9:56, Leizhen (ThunderTown) wrote:
> 
> 
> On 2016/3/15 23:37, Catalin Marinas wrote:
>> On Tue, Mar 15, 2016 at 10:12:11AM +0800, Zhen Lei wrote:
>>> 1. In swiotlb_alloc_coherent, the branch of __get_free_pages. Directly
>>>return vaddr on success, and pass vaddr to free_pages on failure.
>>> 2. So, we can directly transparent pass vaddr from __dma_free to
>>>swiotlb_free_coherent, keep consistent with swiotlb_alloc_coherent.
>>>
>>> This patch have no functional change,
>>
>> I don't think so.
>>
>>> but can obtain a bit performance improvement.
>>
>> Have you actually measured it?
> I have not run any performance testing, but reduced a line of code. So I said 
> "a bit".
> 
>>
>>> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
>>> index a6e757c..b2f2834 100644
>>> --- a/arch/arm64/mm/dma-mapping.c
>>> +++ b/arch/arm64/mm/dma-mapping.c
>>> @@ -187,8 +187,6 @@ static void __dma_free(struct device *dev, size_t size,
>>>void *vaddr, dma_addr_t dma_handle,
>>>struct dma_attrs *attrs)
>>>  {
>>> -   void *swiotlb_addr = phys_to_virt(dma_to_phys(dev, dma_handle));
>>> -
>>> size = PAGE_ALIGN(size);
>>>
>>> if (!is_device_dma_coherent(dev)) {
>>> @@ -196,7 +194,7 @@ static void __dma_free(struct device *dev, size_t size,
>>> return;
>>> vunmap(vaddr);
>>> }
>>> -   __dma_free_coherent(dev, size, swiotlb_addr, dma_handle, attrs);
>>> +   __dma_free_coherent(dev, size, vaddr, dma_handle, attrs);
>>>  }
>>
>> What happens when !is_device_dma_coherent(dev)? (hint: read two lines
>> above __dma_free_coherent).
Do you afraid "vaddr" maybe modified by these statement?
First, it could not be __free_from_pool. Otherwise, the function vunmap(which 
after it) can not work well.
Then, it count not be vunmap too, the parameter is defined as "const void *".

In the call chain: 
__dma_free_coherent-->__dma_free_coherent-->swiotlb_free_coherent, only 
swiotlb_free_coherent finally use "vaddr".

>>
> The whole function of __dma_free as below: (nobody use swiotlb_addr except 
> __dma_free_coherent)
> static void __dma_free(struct device *dev, size_t size,
>void *vaddr, dma_addr_t dma_handle,
>struct dma_attrs *attrs)
> {
> void *swiotlb_addr = phys_to_virt(dma_to_phys(dev, dma_handle));
> 
> size = PAGE_ALIGN(size);
> 
> if (!is_device_dma_coherent(dev)) {
> if (__free_from_pool(vaddr, size))
> return;
> vunmap(vaddr);
> }
> __dma_free_coherent(dev, size, swiotlb_addr, dma_handle, attrs);
> }
> 

Re: [PATCH 1/1] arm64/dma-mapping: remove an unnecessary conversion

[Catalin Marinas]

On Thu, Mar 17, 2016 at 07:06:27PM +0800, Leizhen (ThunderTown) wrote:
> On 2016/3/16 9:56, Leizhen (ThunderTown) wrote:
> > On 2016/3/15 23:37, Catalin Marinas wrote:
> >> On Tue, Mar 15, 2016 at 10:12:11AM +0800, Zhen Lei wrote:
> >>> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
> >>> index a6e757c..b2f2834 100644
> >>> --- a/arch/arm64/mm/dma-mapping.c
> >>> +++ b/arch/arm64/mm/dma-mapping.c
> >>> @@ -187,8 +187,6 @@ static void __dma_free(struct device *dev, size_t 
> >>> size,
> >>>  void *vaddr, dma_addr_t dma_handle,
> >>>  struct dma_attrs *attrs)
> >>>  {
> >>> - void *swiotlb_addr = phys_to_virt(dma_to_phys(dev, dma_handle));
> >>> -
> >>>   size = PAGE_ALIGN(size);
> >>>
> >>>   if (!is_device_dma_coherent(dev)) {
> >>> @@ -196,7 +194,7 @@ static void __dma_free(struct device *dev, size_t 
> >>> size,
> >>>   return;
> >>>   vunmap(vaddr);
> >>>   }
> >>> - __dma_free_coherent(dev, size, swiotlb_addr, dma_handle, attrs);
> >>> + __dma_free_coherent(dev, size, vaddr, dma_handle, attrs);
> >>>  }
> >>
> >> What happens when !is_device_dma_coherent(dev)? (hint: read two lines
> >> above __dma_free_coherent).
> 
> Do you afraid "vaddr" maybe modified by these statement?
> First, it could not be __free_from_pool. Otherwise, the function
> vunmap(which after it) can not work well. Then, it count not be vunmap
> too, the parameter is defined as "const void *".
> 
> In the call chain:
> __dma_free_coherent-->__dma_free_coherent-->swiotlb_free_coherent,
> only swiotlb_free_coherent finally use "vaddr".

Exactly. So you give swiotlb_free_coherent a vaddr which has been
unmapped. It doesn't even matter whether it's still mapped since this
address is passed further to free_pages() which performs a
virt_to_page(). The latter is *only* valid on linear map addresses (and
you would actually hit the VM_BUG_ON in free_pages; you can try running
this with CONFIG_DEBUG_VM enabled and non-coherent DMA).

For non-coherent DMA, the vaddr is not part of the linear mapping as it
has been remapped by __dma_alloc() via dma_common_contiguous_remap(),
hence for swiotlb freeing we need the actual linear map address (the
original "ptr" in __dma_alloc()). We can generate it by a
phys_to_virt(dma_to_phys(dma_handle)).

-- 
Catalin

Re: [PATCH 1/1] arm64/dma-mapping: remove an unnecessary conversion

[Catalin Marinas]

On Thu, Mar 17, 2016 at 07:06:27PM +0800, Leizhen (ThunderTown) wrote:
> On 2016/3/16 9:56, Leizhen (ThunderTown) wrote:
> > On 2016/3/15 23:37, Catalin Marinas wrote:
> >> On Tue, Mar 15, 2016 at 10:12:11AM +0800, Zhen Lei wrote:
> >>> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
> >>> index a6e757c..b2f2834 100644
> >>> --- a/arch/arm64/mm/dma-mapping.c
> >>> +++ b/arch/arm64/mm/dma-mapping.c
> >>> @@ -187,8 +187,6 @@ static void __dma_free(struct device *dev, size_t 
> >>> size,
> >>>  void *vaddr, dma_addr_t dma_handle,
> >>>  struct dma_attrs *attrs)
> >>>  {
> >>> - void *swiotlb_addr = phys_to_virt(dma_to_phys(dev, dma_handle));
> >>> -
> >>>   size = PAGE_ALIGN(size);
> >>>
> >>>   if (!is_device_dma_coherent(dev)) {
> >>> @@ -196,7 +194,7 @@ static void __dma_free(struct device *dev, size_t 
> >>> size,
> >>>   return;
> >>>   vunmap(vaddr);
> >>>   }
> >>> - __dma_free_coherent(dev, size, swiotlb_addr, dma_handle, attrs);
> >>> + __dma_free_coherent(dev, size, vaddr, dma_handle, attrs);
> >>>  }
> >>
> >> What happens when !is_device_dma_coherent(dev)? (hint: read two lines
> >> above __dma_free_coherent).
> 
> Do you afraid "vaddr" maybe modified by these statement?
> First, it could not be __free_from_pool. Otherwise, the function
> vunmap(which after it) can not work well. Then, it count not be vunmap
> too, the parameter is defined as "const void *".
> 
> In the call chain:
> __dma_free_coherent-->__dma_free_coherent-->swiotlb_free_coherent,
> only swiotlb_free_coherent finally use "vaddr".

Exactly. So you give swiotlb_free_coherent a vaddr which has been
unmapped. It doesn't even matter whether it's still mapped since this
address is passed further to free_pages() which performs a
virt_to_page(). The latter is *only* valid on linear map addresses (and
you would actually hit the VM_BUG_ON in free_pages; you can try running
this with CONFIG_DEBUG_VM enabled and non-coherent DMA).

For non-coherent DMA, the vaddr is not part of the linear mapping as it
has been remapped by __dma_alloc() via dma_common_contiguous_remap(),
hence for swiotlb freeing we need the actual linear map address (the
original "ptr" in __dma_alloc()). We can generate it by a
phys_to_virt(dma_to_phys(dma_handle)).

-- 
Catalin

Re: [PATCH 1/1] arm64/dma-mapping: remove an unnecessary conversion

[Leizhen (ThunderTown)]

On 2016/3/15 23:37, Catalin Marinas wrote:
> On Tue, Mar 15, 2016 at 10:12:11AM +0800, Zhen Lei wrote:
>> 1. In swiotlb_alloc_coherent, the branch of __get_free_pages. Directly
>>return vaddr on success, and pass vaddr to free_pages on failure.
>> 2. So, we can directly transparent pass vaddr from __dma_free to
>>swiotlb_free_coherent, keep consistent with swiotlb_alloc_coherent.
>>
>> This patch have no functional change,
> 
> I don't think so.
> 
>> but can obtain a bit performance improvement.
> 
> Have you actually measured it?
I have not run any performance testing, but reduced a line of code. So I said 
"a bit".

> 
>> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
>> index a6e757c..b2f2834 100644
>> --- a/arch/arm64/mm/dma-mapping.c
>> +++ b/arch/arm64/mm/dma-mapping.c
>> @@ -187,8 +187,6 @@ static void __dma_free(struct device *dev, size_t size,
>> void *vaddr, dma_addr_t dma_handle,
>> struct dma_attrs *attrs)
>>  {
>> -void *swiotlb_addr = phys_to_virt(dma_to_phys(dev, dma_handle));
>> -
>>  size = PAGE_ALIGN(size);
>>
>>  if (!is_device_dma_coherent(dev)) {
>> @@ -196,7 +194,7 @@ static void __dma_free(struct device *dev, size_t size,
>>  return;
>>  vunmap(vaddr);
>>  }
>> -__dma_free_coherent(dev, size, swiotlb_addr, dma_handle, attrs);
>> +__dma_free_coherent(dev, size, vaddr, dma_handle, attrs);
>>  }
> 
> What happens when !is_device_dma_coherent(dev)? (hint: read two lines
> above __dma_free_coherent).
> 
The whole function of __dma_free as below: (nobody use swiotlb_addr except 
__dma_free_coherent)
static void __dma_free(struct device *dev, size_t size,
   void *vaddr, dma_addr_t dma_handle,
   struct dma_attrs *attrs)
{
void *swiotlb_addr = phys_to_virt(dma_to_phys(dev, dma_handle));

size = PAGE_ALIGN(size);

if (!is_device_dma_coherent(dev)) {
if (__free_from_pool(vaddr, size))
return;
vunmap(vaddr);
}
__dma_free_coherent(dev, size, swiotlb_addr, dma_handle, attrs);
}

Re: [PATCH 1/1] arm64/dma-mapping: remove an unnecessary conversion

[Leizhen (ThunderTown)]

On 2016/3/15 23:37, Catalin Marinas wrote:
> On Tue, Mar 15, 2016 at 10:12:11AM +0800, Zhen Lei wrote:
>> 1. In swiotlb_alloc_coherent, the branch of __get_free_pages. Directly
>>return vaddr on success, and pass vaddr to free_pages on failure.
>> 2. So, we can directly transparent pass vaddr from __dma_free to
>>swiotlb_free_coherent, keep consistent with swiotlb_alloc_coherent.
>>
>> This patch have no functional change,
> 
> I don't think so.
> 
>> but can obtain a bit performance improvement.
> 
> Have you actually measured it?
I have not run any performance testing, but reduced a line of code. So I said 
"a bit".

> 
>> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
>> index a6e757c..b2f2834 100644
>> --- a/arch/arm64/mm/dma-mapping.c
>> +++ b/arch/arm64/mm/dma-mapping.c
>> @@ -187,8 +187,6 @@ static void __dma_free(struct device *dev, size_t size,
>> void *vaddr, dma_addr_t dma_handle,
>> struct dma_attrs *attrs)
>>  {
>> -void *swiotlb_addr = phys_to_virt(dma_to_phys(dev, dma_handle));
>> -
>>  size = PAGE_ALIGN(size);
>>
>>  if (!is_device_dma_coherent(dev)) {
>> @@ -196,7 +194,7 @@ static void __dma_free(struct device *dev, size_t size,
>>  return;
>>  vunmap(vaddr);
>>  }
>> -__dma_free_coherent(dev, size, swiotlb_addr, dma_handle, attrs);
>> +__dma_free_coherent(dev, size, vaddr, dma_handle, attrs);
>>  }
> 
> What happens when !is_device_dma_coherent(dev)? (hint: read two lines
> above __dma_free_coherent).
> 
The whole function of __dma_free as below: (nobody use swiotlb_addr except 
__dma_free_coherent)
static void __dma_free(struct device *dev, size_t size,
   void *vaddr, dma_addr_t dma_handle,
   struct dma_attrs *attrs)
{
void *swiotlb_addr = phys_to_virt(dma_to_phys(dev, dma_handle));

size = PAGE_ALIGN(size);

if (!is_device_dma_coherent(dev)) {
if (__free_from_pool(vaddr, size))
return;
vunmap(vaddr);
}
__dma_free_coherent(dev, size, swiotlb_addr, dma_handle, attrs);
}

Re: [PATCH 1/1] arm64/dma-mapping: remove an unnecessary conversion

[Catalin Marinas]

On Tue, Mar 15, 2016 at 10:12:11AM +0800, Zhen Lei wrote:
> 1. In swiotlb_alloc_coherent, the branch of __get_free_pages. Directly
>return vaddr on success, and pass vaddr to free_pages on failure.
> 2. So, we can directly transparent pass vaddr from __dma_free to
>swiotlb_free_coherent, keep consistent with swiotlb_alloc_coherent.
> 
> This patch have no functional change,

I don't think so.

> but can obtain a bit performance improvement.

Have you actually measured it?

> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
> index a6e757c..b2f2834 100644
> --- a/arch/arm64/mm/dma-mapping.c
> +++ b/arch/arm64/mm/dma-mapping.c
> @@ -187,8 +187,6 @@ static void __dma_free(struct device *dev, size_t size,
>  void *vaddr, dma_addr_t dma_handle,
>  struct dma_attrs *attrs)
>  {
> - void *swiotlb_addr = phys_to_virt(dma_to_phys(dev, dma_handle));
> -
>   size = PAGE_ALIGN(size);
> 
>   if (!is_device_dma_coherent(dev)) {
> @@ -196,7 +194,7 @@ static void __dma_free(struct device *dev, size_t size,
>   return;
>   vunmap(vaddr);
>   }
> - __dma_free_coherent(dev, size, swiotlb_addr, dma_handle, attrs);
> + __dma_free_coherent(dev, size, vaddr, dma_handle, attrs);
>  }

What happens when !is_device_dma_coherent(dev)? (hint: read two lines
above __dma_free_coherent).

-- 
Catalin

Re: [PATCH 1/1] arm64/dma-mapping: remove an unnecessary conversion

[Catalin Marinas]

On Tue, Mar 15, 2016 at 10:12:11AM +0800, Zhen Lei wrote:
> 1. In swiotlb_alloc_coherent, the branch of __get_free_pages. Directly
>return vaddr on success, and pass vaddr to free_pages on failure.
> 2. So, we can directly transparent pass vaddr from __dma_free to
>swiotlb_free_coherent, keep consistent with swiotlb_alloc_coherent.
> 
> This patch have no functional change,

I don't think so.

> but can obtain a bit performance improvement.

Have you actually measured it?

> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
> index a6e757c..b2f2834 100644
> --- a/arch/arm64/mm/dma-mapping.c
> +++ b/arch/arm64/mm/dma-mapping.c
> @@ -187,8 +187,6 @@ static void __dma_free(struct device *dev, size_t size,
>  void *vaddr, dma_addr_t dma_handle,
>  struct dma_attrs *attrs)
>  {
> - void *swiotlb_addr = phys_to_virt(dma_to_phys(dev, dma_handle));
> -
>   size = PAGE_ALIGN(size);
> 
>   if (!is_device_dma_coherent(dev)) {
> @@ -196,7 +194,7 @@ static void __dma_free(struct device *dev, size_t size,
>   return;
>   vunmap(vaddr);
>   }
> - __dma_free_coherent(dev, size, swiotlb_addr, dma_handle, attrs);
> + __dma_free_coherent(dev, size, vaddr, dma_handle, attrs);
>  }

What happens when !is_device_dma_coherent(dev)? (hint: read two lines
above __dma_free_coherent).

-- 
Catalin

[PATCH 1/1] arm64/dma-mapping: remove an unnecessary conversion

[Zhen Lei]

1. In swiotlb_alloc_coherent, the branch of __get_free_pages. Directly
   return vaddr on success, and pass vaddr to free_pages on failure.
2. So, we can directly transparent pass vaddr from __dma_free to
   swiotlb_free_coherent, keep consistent with swiotlb_alloc_coherent.

This patch have no functional change, but can obtain a bit performance
improvement.

Signed-off-by: Zhen Lei 
---
 arch/arm64/mm/dma-mapping.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index a6e757c..b2f2834 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -187,8 +187,6 @@ static void __dma_free(struct device *dev, size_t size,
   void *vaddr, dma_addr_t dma_handle,
   struct dma_attrs *attrs)
 {
-   void *swiotlb_addr = phys_to_virt(dma_to_phys(dev, dma_handle));
-
size = PAGE_ALIGN(size);

if (!is_device_dma_coherent(dev)) {
@@ -196,7 +194,7 @@ static void __dma_free(struct device *dev, size_t size,
return;
vunmap(vaddr);
}
-   __dma_free_coherent(dev, size, swiotlb_addr, dma_handle, attrs);
+   __dma_free_coherent(dev, size, vaddr, dma_handle, attrs);
 }

 static dma_addr_t __swiotlb_map_page(struct device *dev, struct page *page,
--
2.5.0

[PATCH 1/1] arm64/dma-mapping: remove an unnecessary conversion

[Zhen Lei]

1. In swiotlb_alloc_coherent, the branch of __get_free_pages. Directly
   return vaddr on success, and pass vaddr to free_pages on failure.
2. So, we can directly transparent pass vaddr from __dma_free to
   swiotlb_free_coherent, keep consistent with swiotlb_alloc_coherent.

This patch have no functional change, but can obtain a bit performance
improvement.

Signed-off-by: Zhen Lei 
---
 arch/arm64/mm/dma-mapping.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index a6e757c..b2f2834 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -187,8 +187,6 @@ static void __dma_free(struct device *dev, size_t size,
   void *vaddr, dma_addr_t dma_handle,
   struct dma_attrs *attrs)
 {
-   void *swiotlb_addr = phys_to_virt(dma_to_phys(dev, dma_handle));
-
size = PAGE_ALIGN(size);

if (!is_device_dma_coherent(dev)) {
@@ -196,7 +194,7 @@ static void __dma_free(struct device *dev, size_t size,
return;
vunmap(vaddr);
}
-   __dma_free_coherent(dev, size, swiotlb_addr, dma_handle, attrs);
+   __dma_free_coherent(dev, size, vaddr, dma_handle, attrs);
 }

 static dma_addr_t __swiotlb_map_page(struct device *dev, struct page *page,
--
2.5.0