Re: [PATCH v2 5/6] powerpc/pseries/iommu: Make use of DDW even if it does not map the partition

2020-07-03 Thread Leonardo Bras
On Thu, 2020-07-02 at 10:31 +1000, Alexey Kardashevskiy wrote:
> 
> On 02/07/2020 09:48, Leonardo Bras wrote:
> > On Wed, 2020-07-01 at 16:57 -0300, Leonardo Bras wrote:
> > > > It is not necessarily "direct" anymore as the name suggests, you may
> > > > want to change that. DMA64_PROPNAME, may be. Thanks,
> > > > 
> > > 
> > > Yeah, you are right.
> > > I will change this for next version, also changing the string name to
> > > reflect this.
> > > 
> > > -#define DIRECT64_PROPNAME "linux,direct64-ddr-window-info"
> > > +#define DMA64_PROPNAME "linux,dma64-ddr-window-info"
> > > 
> > > Is that ok?
> > > 
> > > Thank you for helping!
> > 
> > In fact, there is a lot of places in this file where it's called direct
> > window. Should I replace everything?
> > Should it be in a separated patch?
> 
> If it looks simple and you write a nice commit log explaining all that
> and why you are not reusing the existing ibm,dma-window property (to
> provide a clue what "reset" will reset to? is there any other reason?)
> for that - sure, do it :)
> 

v3 available here:
http://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=187348=%2A=both

Best regards,
Leonardo



Re: [PATCH v2 5/6] powerpc/pseries/iommu: Make use of DDW even if it does not map the partition

2020-07-01 Thread Leonardo Bras
On Thu, 2020-07-02 at 10:31 +1000, Alexey Kardashevskiy wrote:
> > In fact, there is a lot of places in this file where it's called direct
> > window. Should I replace everything?
> > Should it be in a separated patch?
> 
> If it looks simple and you write a nice commit log explaining all that
> and why you are not reusing the existing ibm,dma-window property 
> for that - sure, do it :)

Nice, I will do that :)

> (to provide a clue what "reset" will reset to? is there any other
> reason?)

That's the main reason here. 

The way I perceive this, ibm,dma-window should only point to the
default DMA window, which is guaranteed to always be the same, even if
it's destroyed and re-created. So there I see no point destroying /
overwriting it.

On the other hand, I also thought about using a new node name for this
window, but it would be very troublesome and I could see no real gain.

Thanks !



Re: [PATCH v2 5/6] powerpc/pseries/iommu: Make use of DDW even if it does not map the partition

2020-07-01 Thread Alexey Kardashevskiy



On 02/07/2020 09:48, Leonardo Bras wrote:
> On Wed, 2020-07-01 at 16:57 -0300, Leonardo Bras wrote:
>>> It is not necessarily "direct" anymore as the name suggests, you may
>>> want to change that. DMA64_PROPNAME, may be. Thanks,
>>>
>>
>> Yeah, you are right.
>> I will change this for next version, also changing the string name to
>> reflect this.
>>
>> -#define DIRECT64_PROPNAME "linux,direct64-ddr-window-info"
>> +#define DMA64_PROPNAME "linux,dma64-ddr-window-info"
>>
>> Is that ok?
>>
>> Thank you for helping!
> 
> In fact, there is a lot of places in this file where it's called direct
> window. Should I replace everything?
> Should it be in a separated patch?

If it looks simple and you write a nice commit log explaining all that
and why you are not reusing the existing ibm,dma-window property (to
provide a clue what "reset" will reset to? is there any other reason?)
for that - sure, do it :)



-- 
Alexey


Re: [PATCH v2 5/6] powerpc/pseries/iommu: Make use of DDW even if it does not map the partition

2020-07-01 Thread Leonardo Bras
On Wed, 2020-07-01 at 16:57 -0300, Leonardo Bras wrote:
> > It is not necessarily "direct" anymore as the name suggests, you may
> > want to change that. DMA64_PROPNAME, may be. Thanks,
> > 
> 
> Yeah, you are right.
> I will change this for next version, also changing the string name to
> reflect this.
> 
> -#define DIRECT64_PROPNAME "linux,direct64-ddr-window-info"
> +#define DMA64_PROPNAME "linux,dma64-ddr-window-info"
> 
> Is that ok?
> 
> Thank you for helping!

In fact, there is a lot of places in this file where it's called direct
window. Should I replace everything?
Should it be in a separated patch?

Best regards,
Leonardo



Re: [PATCH v2 5/6] powerpc/pseries/iommu: Make use of DDW even if it does not map the partition

2020-07-01 Thread Leonardo Bras
On Wed, 2020-07-01 at 18:16 +1000, Alexey Kardashevskiy wrote:
> 
> On 24/06/2020 16:24, Leonardo Bras wrote:
> > As of today, if a DDW is created and can't map the whole partition, it's
> > removed and the default DMA window "ibm,dma-window" is used instead.
> > 
> > Usually this DDW is bigger than the default DMA window, so it would be
> > better to make use of it instead.
> > 
> > Signed-off-by: Leonardo Bras 
> > ---
> >  arch/powerpc/platforms/pseries/iommu.c | 28 +-
> >  1 file changed, 19 insertions(+), 9 deletions(-)
> > 
> > diff --git a/arch/powerpc/platforms/pseries/iommu.c 
> > b/arch/powerpc/platforms/pseries/iommu.c
> > index 4fcf00016fb1..2d217cda4075 100644
> > --- a/arch/powerpc/platforms/pseries/iommu.c
> > +++ b/arch/powerpc/platforms/pseries/iommu.c
> > @@ -685,7 +685,7 @@ static void pci_dma_bus_setup_pSeriesLP(struct pci_bus 
> > *bus)
> > struct iommu_table *tbl;
> > struct device_node *dn, *pdn;
> > struct pci_dn *ppci;
> > -   const __be32 *dma_window = NULL;
> > +   const __be32 *dma_window = NULL, *alt_dma_window = NULL;
> >  
> > dn = pci_bus_to_OF_node(bus);
> >  
> > @@ -699,8 +699,13 @@ static void pci_dma_bus_setup_pSeriesLP(struct pci_bus 
> > *bus)
> > break;
> > }
> >  
> > +   /* If there is a DDW available, use it instead */
> > +   alt_dma_window = of_get_property(pdn, DIRECT64_PROPNAME, NULL);
> 
> It is not necessarily "direct" anymore as the name suggests, you may
> want to change that. DMA64_PROPNAME, may be. Thanks,
> 

Yeah, you are right.
I will change this for next version, also changing the string name to
reflect this.

-#define DIRECT64_PROPNAME "linux,direct64-ddr-window-info"
+#define DMA64_PROPNAME "linux,dma64-ddr-window-info"

Is that ok?

Thank you for helping!




Re: [PATCH v2 5/6] powerpc/pseries/iommu: Make use of DDW even if it does not map the partition

2020-07-01 Thread Alexey Kardashevskiy



On 24/06/2020 16:24, Leonardo Bras wrote:
> As of today, if a DDW is created and can't map the whole partition, it's
> removed and the default DMA window "ibm,dma-window" is used instead.
> 
> Usually this DDW is bigger than the default DMA window, so it would be
> better to make use of it instead.
> 
> Signed-off-by: Leonardo Bras 
> ---
>  arch/powerpc/platforms/pseries/iommu.c | 28 +-
>  1 file changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/iommu.c 
> b/arch/powerpc/platforms/pseries/iommu.c
> index 4fcf00016fb1..2d217cda4075 100644
> --- a/arch/powerpc/platforms/pseries/iommu.c
> +++ b/arch/powerpc/platforms/pseries/iommu.c
> @@ -685,7 +685,7 @@ static void pci_dma_bus_setup_pSeriesLP(struct pci_bus 
> *bus)
>   struct iommu_table *tbl;
>   struct device_node *dn, *pdn;
>   struct pci_dn *ppci;
> - const __be32 *dma_window = NULL;
> + const __be32 *dma_window = NULL, *alt_dma_window = NULL;
>  
>   dn = pci_bus_to_OF_node(bus);
>  
> @@ -699,8 +699,13 @@ static void pci_dma_bus_setup_pSeriesLP(struct pci_bus 
> *bus)
>   break;
>   }
>  
> + /* If there is a DDW available, use it instead */
> + alt_dma_window = of_get_property(pdn, DIRECT64_PROPNAME, NULL);


It is not necessarily "direct" anymore as the name suggests, you may
want to change that. DMA64_PROPNAME, may be. Thanks,


> + if (alt_dma_window)
> + dma_window = alt_dma_window;
> +
>   if (dma_window == NULL) {
> - pr_debug("  no ibm,dma-window property !\n");
> + pr_debug("  no ibm,dma-window nor 
> linux,direct64-ddr-window-info property !\n");
>   return;
>   }
>  
> @@ -1166,16 +1171,19 @@ static u64 enable_ddw(struct pci_dev *dev, struct 
> device_node *pdn)
> query.page_size);
>   goto out_failed;
>   }
> +
>   /* verify the window * number of ptes will map the partition */
> - /* check largest block * page size > max memory hotplug addr */
>   max_addr = ddw_memory_hotplug_max();
>   if (query.largest_available_block < (max_addr >> page_shift)) {
> - dev_dbg(>dev, "can't map partition max 0x%llx with %llu "
> -   "%llu-sized pages\n", max_addr,  
> query.largest_available_block,
> -   1ULL << page_shift);
> - goto out_failed;
> + dev_dbg(>dev, "can't map partition max 0x%llx with %llu 
> %llu-sized pages\n",
> + max_addr, query.largest_available_block,
> + 1ULL << page_shift);
> +
> + len = order_base_2(query.largest_available_block << page_shift);
> + } else {
> + len = order_base_2(max_addr);
>   }
> - len = order_base_2(max_addr);
> +
>   win64 = kzalloc(sizeof(struct property), GFP_KERNEL);
>   if (!win64) {
>   dev_info(>dev,
> @@ -1229,7 +1237,9 @@ static u64 enable_ddw(struct pci_dev *dev, struct 
> device_node *pdn)
>   list_add(>list, _window_list);
>   spin_unlock(_window_list_lock);
>  
> - dma_addr = be64_to_cpu(ddwprop->dma_base);
> + /* Only returns the dma_addr if DDW maps the whole partition */
> + if (len == order_base_2(max_addr))
> + dma_addr = be64_to_cpu(ddwprop->dma_base);
>   goto out_unlock;
>  
>  out_free_window:
> 

-- 
Alexey


Re: [PATCH v2 5/6] powerpc/pseries/iommu: Make use of DDW even if it does not map the partition

2020-06-26 Thread Leonardo Bras
On Fri, 2020-06-26 at 12:23 -0300, Leonardo Bras wrote:
> On Wed, 2020-06-24 at 03:24 -0300, Leonardo Bras wrote:
> > As of today, if a DDW is created and can't map the whole partition, it's
> > removed and the default DMA window "ibm,dma-window" is used instead.
> > 
> > Usually this DDW is bigger than the default DMA window, so it would be
> > better to make use of it instead.
> > 
> > Signed-off-by: Leonardo Bras 
> > ---
> 
> I tested this change with a 256GB DDW which did not map the whole
> partition, with a MT27700 Family [ConnectX-4 Virtual Function].
> 
> I noticed the performance improvement is about the same as using DDW
> with IOMMU bypass.
> 
> 64 thread write throughput: +203.0%
> 64 thread read throughput: +17.5%
> 1 thread write throughput: +20.5%
> 1 thread read throughput: +3.43%
> Average write latency: -23.0%
> Average read latency:  -2.26%

The above improvements are based on the default DMA window, which is
currently used if DDW can't map the whole partition.

Those values are an average of 20 tests for each environment, 30
seconds each test.

I also did some intense testing, for 5 hour each:
64 thread write throughput 
64 thread read throughput

The throughput values are stable in the whole test, and I noticed no
error on dmesg / journalctl.



[PATCH v2 5/6] powerpc/pseries/iommu: Make use of DDW even if it does not map the partition

2020-06-24 Thread Leonardo Bras
As of today, if a DDW is created and can't map the whole partition, it's
removed and the default DMA window "ibm,dma-window" is used instead.

Usually this DDW is bigger than the default DMA window, so it would be
better to make use of it instead.

Signed-off-by: Leonardo Bras 
---
 arch/powerpc/platforms/pseries/iommu.c | 28 +-
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/iommu.c 
b/arch/powerpc/platforms/pseries/iommu.c
index 4fcf00016fb1..2d217cda4075 100644
--- a/arch/powerpc/platforms/pseries/iommu.c
+++ b/arch/powerpc/platforms/pseries/iommu.c
@@ -685,7 +685,7 @@ static void pci_dma_bus_setup_pSeriesLP(struct pci_bus *bus)
struct iommu_table *tbl;
struct device_node *dn, *pdn;
struct pci_dn *ppci;
-   const __be32 *dma_window = NULL;
+   const __be32 *dma_window = NULL, *alt_dma_window = NULL;
 
dn = pci_bus_to_OF_node(bus);
 
@@ -699,8 +699,13 @@ static void pci_dma_bus_setup_pSeriesLP(struct pci_bus 
*bus)
break;
}
 
+   /* If there is a DDW available, use it instead */
+   alt_dma_window = of_get_property(pdn, DIRECT64_PROPNAME, NULL);
+   if (alt_dma_window)
+   dma_window = alt_dma_window;
+
if (dma_window == NULL) {
-   pr_debug("  no ibm,dma-window property !\n");
+   pr_debug("  no ibm,dma-window nor 
linux,direct64-ddr-window-info property !\n");
return;
}
 
@@ -1166,16 +1171,19 @@ static u64 enable_ddw(struct pci_dev *dev, struct 
device_node *pdn)
  query.page_size);
goto out_failed;
}
+
/* verify the window * number of ptes will map the partition */
-   /* check largest block * page size > max memory hotplug addr */
max_addr = ddw_memory_hotplug_max();
if (query.largest_available_block < (max_addr >> page_shift)) {
-   dev_dbg(>dev, "can't map partition max 0x%llx with %llu "
- "%llu-sized pages\n", max_addr,  
query.largest_available_block,
- 1ULL << page_shift);
-   goto out_failed;
+   dev_dbg(>dev, "can't map partition max 0x%llx with %llu 
%llu-sized pages\n",
+   max_addr, query.largest_available_block,
+   1ULL << page_shift);
+
+   len = order_base_2(query.largest_available_block << page_shift);
+   } else {
+   len = order_base_2(max_addr);
}
-   len = order_base_2(max_addr);
+
win64 = kzalloc(sizeof(struct property), GFP_KERNEL);
if (!win64) {
dev_info(>dev,
@@ -1229,7 +1237,9 @@ static u64 enable_ddw(struct pci_dev *dev, struct 
device_node *pdn)
list_add(>list, _window_list);
spin_unlock(_window_list_lock);
 
-   dma_addr = be64_to_cpu(ddwprop->dma_base);
+   /* Only returns the dma_addr if DDW maps the whole partition */
+   if (len == order_base_2(max_addr))
+   dma_addr = be64_to_cpu(ddwprop->dma_base);
goto out_unlock;
 
 out_free_window:
-- 
2.25.4