[PATCH 0/3] powerpc/powernv/npu: Improve ATSD invalidation overhead

[PATCH 0/3] powerpc/powernv/npu: Improve ATSD invalidation overhead

[Mark Hairgrove]

When ATS is used in a process, all CPU TLB invalidates in that process
also trigger ATSD invalidates via mmu_notifiers. This additional overhead
is noticeable in applications which do heavy memory allocation or page
migration among nodes, particularly to and from GPUs.

This patch set reduces that overhead by rearranging how the ATSDs are
issued and by using size-based ATSD invalidates.

Mark Hairgrove (3):
  powerpc/powernv/npu: Reduce eieio usage when issuing ATSD invalidates
  powerpc/powernv/npu: Use size-based ATSD invalidates
  powerpc/powernv/npu: Remove atsd_threshold debugfs setting

 arch/powerpc/platforms/powernv/npu-dma.c |  177 ++++++++++++++---------------
 1 files changed, 85 insertions(+), 92 deletions(-)

-- 
1.7.2.5

[PATCH 1/3] powerpc/powernv/npu: Reduce eieio usage when issuing ATSD invalidates

[Mark Hairgrove]

There are two types of ATSDs issued to the NPU: invalidates targeting a
specific virtual address and invalidates targeting the whole address
space. In both cases prior to this change, the sequence was:

    for each NPU
        - Write the target address to the XTS_ATSD_AVA register
        - EIEIO
        - Write the launch value to issue the ATSD

First, a target address is not required when invalidating the whole
address space, so that write and the EIEIO have been removed. The AP
(size) field in the launch is not needed either.

Second, for per-address invalidates the above sequence is inefficient in
the common case of multiple NPUs because an EIEIO is issued per NPU. This
unnecessarily forces the launches of later ATSDs to be ordered with the
launches of earlier ones. The new sequence only issues a single EIEIO:

    for each NPU
        - Write the target address to the XTS_ATSD_AVA register
    EIEIO
    for each NPU
        - Write the launch value to issue the ATSD

Performance results were gathered using a microbenchmark which creates a
1G allocation then uses mprotect with PROT_NONE to trigger invalidates in
strides across the allocation.

With only a single NPU active (one GPU) the difference is in the noise for
both types of invalidates (+/-1%).

With two NPUs active (on a 6-GPU system) the effect is more noticeable:

         mprotect rate (GB/s)
Stride   Before      After      Speedup
64K         5.9        6.5          10%
1M         31.2       33.4           7%
2M         36.3       38.7           7%
4M        322.6      356.7          11%

Signed-off-by: Mark Hairgrove <mhairgrove@nvidia.com>
---
 arch/powerpc/platforms/powernv/npu-dma.c |   99 ++++++++++++++---------------
 1 files changed, 48 insertions(+), 51 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/npu-dma.c b/arch/powerpc/platforms/powernv/npu-dma.c
index 8006c54..c8f438a 100644
--- a/arch/powerpc/platforms/powernv/npu-dma.c
+++ b/arch/powerpc/platforms/powernv/npu-dma.c
@@ -454,79 +454,76 @@ static void put_mmio_atsd_reg(struct npu *npu, int reg)
 }
 
 /* MMIO ATSD register offsets */
-#define XTS_ATSD_AVA  1
-#define XTS_ATSD_STAT 2
+#define XTS_ATSD_LAUNCH 0
+#define XTS_ATSD_AVA    1
+#define XTS_ATSD_STAT   2
 
-static void mmio_launch_invalidate(struct mmio_atsd_reg *mmio_atsd_reg,
-				unsigned long launch, unsigned long va)
+static unsigned long get_atsd_launch_val(unsigned long pid, unsigned long psize,
+					bool flush)
 {
-	struct npu *npu = mmio_atsd_reg->npu;
-	int reg = mmio_atsd_reg->reg;
+	unsigned long launch = 0;
 
-	__raw_writeq_be(va, npu->mmio_atsd_regs[reg] + XTS_ATSD_AVA);
-	eieio();
-	__raw_writeq_be(launch, npu->mmio_atsd_regs[reg]);
+	if (psize == MMU_PAGE_COUNT) {
+		/* IS set to invalidate entire matching PID */
+		launch |= PPC_BIT(12);
+	} else {
+		/* AP set to invalidate region of psize */
+		launch |= (u64)mmu_get_ap(psize) << PPC_BITLSHIFT(17);
+	}
+
+	/* PRS set to process-scoped */
+	launch |= PPC_BIT(13);
+
+	/* PID */
+	launch |= pid << PPC_BITLSHIFT(38);
+
+	/* No flush */
+	launch |= !flush << PPC_BITLSHIFT(39);
+
+	return launch;
 }
 
-static void mmio_invalidate_pid(struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS],
-				unsigned long pid, bool flush)
+static void mmio_atsd_regs_write(struct mmio_atsd_reg
+			mmio_atsd_reg[NV_MAX_NPUS], unsigned long offset,
+			unsigned long val)
 {
-	int i;
-	unsigned long launch;
+	struct npu *npu;
+	int i, reg;
 
 	for (i = 0; i <= max_npu2_index; i++) {
-		if (mmio_atsd_reg[i].reg < 0)
+		reg = mmio_atsd_reg[i].reg;
+		if (reg < 0)
 			continue;
 
-		/* IS set to invalidate matching PID */
-		launch = PPC_BIT(12);
-
-		/* PRS set to process-scoped */
-		launch |= PPC_BIT(13);
-
-		/* AP */
-		launch |= (u64)
-			mmu_get_ap(mmu_virtual_psize) << PPC_BITLSHIFT(17);
-
-		/* PID */
-		launch |= pid << PPC_BITLSHIFT(38);
+		npu = mmio_atsd_reg[i].npu;
+		__raw_writeq_be(val, npu->mmio_atsd_regs[reg] + offset);
+	}
+}
 
-		/* No flush */
-		launch |= !flush << PPC_BITLSHIFT(39);
+static void mmio_invalidate_pid(struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS],
+				unsigned long pid, bool flush)
+{
+	unsigned long launch = get_atsd_launch_val(pid, MMU_PAGE_COUNT, flush);
 
-		/* Invalidating the entire process doesn't use a va */
-		mmio_launch_invalidate(&mmio_atsd_reg[i], launch, 0);
-	}
+	/* Invalidating the entire process doesn't use a va */
+	mmio_atsd_regs_write(mmio_atsd_reg, XTS_ATSD_LAUNCH, launch);
 }
 
 static void mmio_invalidate_va(struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS],
 			unsigned long va, unsigned long pid, bool flush)
 {
-	int i;
 	unsigned long launch;
 
-	for (i = 0; i <= max_npu2_index; i++) {
-		if (mmio_atsd_reg[i].reg < 0)
-			continue;
-
-		/* IS set to invalidate target VA */
-		launch = 0;
+	launch = get_atsd_launch_val(pid, mmu_virtual_psize, flush);
 
-		/* PRS set to process scoped */
-		launch |= PPC_BIT(13);
+	/* Write all VAs first */
+	mmio_atsd_regs_write(mmio_atsd_reg, XTS_ATSD_AVA, va);
 
-		/* AP */
-		launch |= (u64)
-			mmu_get_ap(mmu_virtual_psize) << PPC_BITLSHIFT(17);
-
-		/* PID */
-		launch |= pid << PPC_BITLSHIFT(38);
-
-		/* No flush */
-		launch |= !flush << PPC_BITLSHIFT(39);
+	/* Issue one barrier for all address writes */
+	eieio();
 
-		mmio_launch_invalidate(&mmio_atsd_reg[i], launch, va);
-	}
+	/* Launch */
+	mmio_atsd_regs_write(mmio_atsd_reg, XTS_ATSD_LAUNCH, launch);
 }
 
 #define mn_to_npu_context(x) container_of(x, struct npu_context, mn)
-- 
1.7.2.5

[PATCH 2/3] powerpc/powernv/npu: Use size-based ATSD invalidates

[Mark Hairgrove]

Prior to this change only two types of ATSDs were issued to the NPU:
invalidates targeting a single page and invalidates targeting the whole
address space. The crossover point happened at the configurable
atsd_threshold which defaulted to 2M. Invalidates that size or smaller
would issue per-page invalidates for the whole range.

The NPU supports more invalidation sizes however: 64K, 2M, 1G, and all.
These invalidates target addresses aligned to their size. 2M is a common
invalidation size for GPU-enabled applications because that is a GPU
page size, so reducing the number of invalidates by 32x in that case is a
clear improvement.

ATSD latency is high in general so now we always issue a single invalidate
rather than multiple. This will over-invalidate in some cases, but for any
invalidation size over 2M it matches or improves the prior behavior.
There's also an improvement for single-page invalidates since the prior
version issued two invalidates for that case instead of one.

To show the benefit here are some performance numbers from a
microbenchmark which creates a 1G allocation then uses mprotect with
PROT_NONE to trigger invalidates in strides across the allocation.

One NPU (1 GPU):

         mprotect rate (GB/s)
Stride   Before      After      Speedup
64K         5.3        5.6           5%
1M         39.3       57.4          46%
2M         49.7       82.6          66%
4M        286.6      285.7           0%

Two NPUs (6 GPUs):

         mprotect rate (GB/s)
Stride   Before      After      Speedup
64K         6.5        7.4          13%
1M         33.4       67.9         103%
2M         38.7       93.1         141%
4M        356.7      354.6          -1%

Anything over 2M is roughly the same as before since both cases issue a
single ATSD.

Signed-off-by: Mark Hairgrove <mhairgrove@nvidia.com>
---
 arch/powerpc/platforms/powernv/npu-dma.c |   71 +++++++++++++++++-------------
 1 files changed, 40 insertions(+), 31 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/npu-dma.c b/arch/powerpc/platforms/powernv/npu-dma.c
index c8f438a..e471a1a 100644
--- a/arch/powerpc/platforms/powernv/npu-dma.c
+++ b/arch/powerpc/platforms/powernv/npu-dma.c
@@ -509,15 +509,14 @@ static void mmio_invalidate_pid(struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS],
 	mmio_atsd_regs_write(mmio_atsd_reg, XTS_ATSD_LAUNCH, launch);
 }
 
-static void mmio_invalidate_va(struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS],
-			unsigned long va, unsigned long pid, bool flush)
+static void mmio_invalidate_range(struct mmio_atsd_reg
+			mmio_atsd_reg[NV_MAX_NPUS], unsigned long pid,
+			unsigned long start, unsigned long psize, bool flush)
 {
-	unsigned long launch;
-
-	launch = get_atsd_launch_val(pid, mmu_virtual_psize, flush);
+	unsigned long launch = get_atsd_launch_val(pid, psize, flush);
 
 	/* Write all VAs first */
-	mmio_atsd_regs_write(mmio_atsd_reg, XTS_ATSD_AVA, va);
+	mmio_atsd_regs_write(mmio_atsd_reg, XTS_ATSD_AVA, start);
 
 	/* Issue one barrier for all address writes */
 	eieio();
@@ -608,15 +607,38 @@ static void release_atsd_reg(struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS])
 	}
 }
 
+#define PAGE_64K (64UL * 1024)
+#define PAGE_2M  (2UL * 1024 * 1024)
+#define PAGE_1G  (1UL * 1024 * 1024 * 1024)
+
 /*
- * Invalidate either a single address or an entire PID depending on
- * the value of va.
+ * Invalidate a virtual address range
  */
-static void mmio_invalidate(struct npu_context *npu_context, int va,
-			unsigned long address, bool flush)
+static void mmio_invalidate(struct npu_context *npu_context,
+			unsigned long start, unsigned long size, bool flush)
 {
 	struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS];
 	unsigned long pid = npu_context->mm->context.id;
+	unsigned long atsd_start = 0;
+	unsigned long end = start + size - 1;
+	int atsd_psize = MMU_PAGE_COUNT;
+
+	/*
+	 * Convert the input range into one of the supported sizes. If the range
+	 * doesn't fit, use the next larger supported size. Invalidation latency
+	 * is high, so over-invalidation is preferred to issuing multiple
+	 * invalidates.
+	 */
+	if (size == PAGE_64K) {
+		atsd_start = start;
+		atsd_psize = MMU_PAGE_64K;
+	} else if (ALIGN_DOWN(start, PAGE_2M) == ALIGN_DOWN(end, PAGE_2M)) {
+		atsd_start = ALIGN_DOWN(start, PAGE_2M);
+		atsd_psize = MMU_PAGE_2M;
+	} else if (ALIGN_DOWN(start, PAGE_1G) == ALIGN_DOWN(end, PAGE_1G)) {
+		atsd_start = ALIGN_DOWN(start, PAGE_1G);
+		atsd_psize = MMU_PAGE_1G;
+	}
 
 	if (npu_context->nmmu_flush)
 		/*
@@ -631,10 +653,12 @@ static void mmio_invalidate(struct npu_context *npu_context, int va,
 	 * an invalidate.
 	 */
 	acquire_atsd_reg(npu_context, mmio_atsd_reg);
-	if (va)
-		mmio_invalidate_va(mmio_atsd_reg, address, pid, flush);
-	else
+
+	if (atsd_psize == MMU_PAGE_COUNT)
 		mmio_invalidate_pid(mmio_atsd_reg, pid, flush);
+	else
+		mmio_invalidate_range(mmio_atsd_reg, pid, atsd_start,
+					atsd_psize, flush);
 
 	mmio_invalidate_wait(mmio_atsd_reg);
 	if (flush) {
@@ -664,7 +688,7 @@ static void pnv_npu2_mn_release(struct mmu_notifier *mn,
 	 * There should be no more translation requests for this PID, but we
 	 * need to ensure any entries for it are removed from the TLB.
 	 */
-	mmio_invalidate(npu_context, 0, 0, true);
+	mmio_invalidate(npu_context, 0, ~0UL, true);
 }
 
 static void pnv_npu2_mn_change_pte(struct mmu_notifier *mn,
@@ -673,8 +697,7 @@ static void pnv_npu2_mn_change_pte(struct mmu_notifier *mn,
 				pte_t pte)
 {
 	struct npu_context *npu_context = mn_to_npu_context(mn);
-
-	mmio_invalidate(npu_context, 1, address, true);
+	mmio_invalidate(npu_context, address, PAGE_SIZE, true);
 }
 
 static void pnv_npu2_mn_invalidate_range(struct mmu_notifier *mn,
@@ -682,21 +705,7 @@ static void pnv_npu2_mn_invalidate_range(struct mmu_notifier *mn,
 					unsigned long start, unsigned long end)
 {
 	struct npu_context *npu_context = mn_to_npu_context(mn);
-	unsigned long address;
-
-	if (end - start > atsd_threshold) {
-		/*
-		 * Just invalidate the entire PID if the address range is too
-		 * large.
-		 */
-		mmio_invalidate(npu_context, 0, 0, true);
-	} else {
-		for (address = start; address < end; address += PAGE_SIZE)
-			mmio_invalidate(npu_context, 1, address, false);
-
-		/* Do the flush only on the final addess == end */
-		mmio_invalidate(npu_context, 1, address, true);
-	}
+	mmio_invalidate(npu_context, start, end - start, true);
 }
 
 static const struct mmu_notifier_ops nv_nmmu_notifier_ops = {
-- 
1.7.2.5

[PATCH 3/3] powerpc/powernv/npu: Remove atsd_threshold debugfs setting

[Mark Hairgrove]

This threshold is no longer used now that all invalidates issue a single
ATSD to each active NPU.

Signed-off-by: Mark Hairgrove <mhairgrove@nvidia.com>
---
 arch/powerpc/platforms/powernv/npu-dma.c |   13 -------------
 1 files changed, 0 insertions(+), 13 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/npu-dma.c b/arch/powerpc/platforms/powernv/npu-dma.c
index e471a1a..426df1b 100644
--- a/arch/powerpc/platforms/powernv/npu-dma.c
+++ b/arch/powerpc/platforms/powernv/npu-dma.c
@@ -42,14 +42,6 @@
 static DEFINE_SPINLOCK(npu_context_lock);
 
 /*
- * When an address shootdown range exceeds this threshold we invalidate the
- * entire TLB on the GPU for the given PID rather than each specific address in
- * the range.
- */
-static uint64_t atsd_threshold = 2 * 1024 * 1024;
-static struct dentry *atsd_threshold_dentry;
-
-/*
  * Other types of TCE cache invalidation are not functional in the
  * hardware.
  */
@@ -968,11 +960,6 @@ int pnv_npu2_init(struct pnv_phb *phb)
 	static int npu_index;
 	uint64_t rc = 0;
 
-	if (!atsd_threshold_dentry) {
-		atsd_threshold_dentry = debugfs_create_x64("atsd_threshold",
-				   0600, powerpc_debugfs_root, &atsd_threshold);
-	}
-
 	phb->npu.nmmu_flush =
 		of_property_read_bool(phb->hose->dn, "ibm,nmmu-flush");
 	for_each_child_of_node(phb->hose->dn, dn) {
-- 
1.7.2.5

Re: [PATCH 2/3] powerpc/powernv/npu: Use size-based ATSD invalidates

[Alistair Popple]

Thanks Mark,

Looks like some worthwhile improvments to be had. I've added a couple of
comments inline below.

> +#define PAGE_64K (64UL * 1024) +#define PAGE_2M (2UL * 1024 * 1024) +#define
> PAGE_1G (1UL * 1024 * 1024 * 1024)

include/linux/sizes.h includes definitions for SZ_64K, SZ_2M, SZ_1G, etc. so
unless they're redefined here for some reason I personally think it's cleaner to
use those.

>  /*
> - * Invalidate either a single address or an entire PID depending on
> - * the value of va.
> + * Invalidate a virtual address range
>   */
> -static void mmio_invalidate(struct npu_context *npu_context, int va,
> -			unsigned long address, bool flush)
> +static void mmio_invalidate(struct npu_context *npu_context,
> +			unsigned long start, unsigned long size, bool flush)

With this optimisation every caller of mmio_invalidate() sets flush == true so
it no longer appears to be used. We should drop it as a parameter unless you
think there might be some reason to use it in future?

Therefore we could also drop it as a parameter to get_atsd_launch_val(),
mmio_invalidate_pid() and mmio_invalidate_range() as well as I couldn't find any
callers of those that set it to anything other than true.

>  	struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS];
>  	unsigned long pid = npu_context->mm->context.id;
> +	unsigned long atsd_start = 0;
> +	unsigned long end = start + size - 1;
> +	int atsd_psize = MMU_PAGE_COUNT;
> +
> +	/*
> +	 * Convert the input range into one of the supported sizes. If the range
> +	 * doesn't fit, use the next larger supported size. Invalidation latency
> +	 * is high, so over-invalidation is preferred to issuing multiple
> +	 * invalidates.
> +	 */
> +	if (size == PAGE_64K) {

We also support 4K page sizes on PPC. If I am not mistaken this means every ATSD
would invalidate the entire GPU TLB for a the given PID on those systems. Could
we change the above check to `if (size <= PAGE_64K)` to avoid this?

> +		atsd_start = start;

Which would also require:

      	    atsd_start = ALIGN_DOWN(start, PAGE_64K);

> +		atsd_psize = MMU_PAGE_64K;
> +	} else if (ALIGN_DOWN(start, PAGE_2M) == ALIGN_DOWN(end, PAGE_2M)) {

Wouldn't this lead to under invalidation in ranges which happen to cross a 2M
boundary? For example invalidating a 128K (ie. 2x64K pages) range with start ==
0x1f0000 and end == 0x210000 would result in an invalidation of the range 0x0 -
0x200000 incorrectly leaving 0x200000 - 0x210000 in the GPU TLB.

> +		atsd_start = ALIGN_DOWN(start, PAGE_2M);
> +		atsd_psize = MMU_PAGE_2M;
> +	} else if (ALIGN_DOWN(start, PAGE_1G) == ALIGN_DOWN(end, PAGE_1G)) {

Ditto.

> +		atsd_start = ALIGN_DOWN(start, PAGE_1G);
> +		atsd_psize = MMU_PAGE_1G;
> +	}
>  

- Alistair

Re: [PATCH 3/3] powerpc/powernv/npu: Remove atsd_threshold debugfs setting

[Alistair Popple]

Looks good to me, thanks!

Reviewed-by: Alistair Popple <alistair@popple.id.au>

On Thursday, 27 September 2018 4:23:11 PM AEST Mark Hairgrove wrote:
> This threshold is no longer used now that all invalidates issue a single
> ATSD to each active NPU.
> 
> Signed-off-by: Mark Hairgrove <mhairgrove@nvidia.com>
> ---
>  arch/powerpc/platforms/powernv/npu-dma.c |   13 -------------
>  1 files changed, 0 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/powernv/npu-dma.c b/arch/powerpc/platforms/powernv/npu-dma.c
> index e471a1a..426df1b 100644
> --- a/arch/powerpc/platforms/powernv/npu-dma.c
> +++ b/arch/powerpc/platforms/powernv/npu-dma.c
> @@ -42,14 +42,6 @@
>  static DEFINE_SPINLOCK(npu_context_lock);
>  
>  /*
> - * When an address shootdown range exceeds this threshold we invalidate the
> - * entire TLB on the GPU for the given PID rather than each specific address in
> - * the range.
> - */
> -static uint64_t atsd_threshold = 2 * 1024 * 1024;
> -static struct dentry *atsd_threshold_dentry;
> -
> -/*
>   * Other types of TCE cache invalidation are not functional in the
>   * hardware.
>   */
> @@ -968,11 +960,6 @@ int pnv_npu2_init(struct pnv_phb *phb)
>  	static int npu_index;
>  	uint64_t rc = 0;
>  
> -	if (!atsd_threshold_dentry) {
> -		atsd_threshold_dentry = debugfs_create_x64("atsd_threshold",
> -				   0600, powerpc_debugfs_root, &atsd_threshold);
> -	}
> -
>  	phb->npu.nmmu_flush =
>  		of_property_read_bool(phb->hose->dn, "ibm,nmmu-flush");
>  	for_each_child_of_node(phb->hose->dn, dn) {
> 

Re: [PATCH 1/3] powerpc/powernv/npu: Reduce eieio usage when issuing ATSD invalidates

[Alistair Popple]

Thanks, this looks good to me.

Reviewed-by: Alistair Popple <alistair@popple.id.au>

On Thursday, 27 September 2018 4:23:09 PM AEST Mark Hairgrove wrote:
> There are two types of ATSDs issued to the NPU: invalidates targeting a
> specific virtual address and invalidates targeting the whole address
> space. In both cases prior to this change, the sequence was:
> 
>     for each NPU
>         - Write the target address to the XTS_ATSD_AVA register
>         - EIEIO
>         - Write the launch value to issue the ATSD
> 
> First, a target address is not required when invalidating the whole
> address space, so that write and the EIEIO have been removed. The AP
> (size) field in the launch is not needed either.
> 
> Second, for per-address invalidates the above sequence is inefficient in
> the common case of multiple NPUs because an EIEIO is issued per NPU. This
> unnecessarily forces the launches of later ATSDs to be ordered with the
> launches of earlier ones. The new sequence only issues a single EIEIO:
> 
>     for each NPU
>         - Write the target address to the XTS_ATSD_AVA register
>     EIEIO
>     for each NPU
>         - Write the launch value to issue the ATSD
> 
> Performance results were gathered using a microbenchmark which creates a
> 1G allocation then uses mprotect with PROT_NONE to trigger invalidates in
> strides across the allocation.
> 
> With only a single NPU active (one GPU) the difference is in the noise for
> both types of invalidates (+/-1%).
> 
> With two NPUs active (on a 6-GPU system) the effect is more noticeable:
> 
>          mprotect rate (GB/s)
> Stride   Before      After      Speedup
> 64K         5.9        6.5          10%
> 1M         31.2       33.4           7%
> 2M         36.3       38.7           7%
> 4M        322.6      356.7          11%
> 
> Signed-off-by: Mark Hairgrove <mhairgrove@nvidia.com>
> ---
>  arch/powerpc/platforms/powernv/npu-dma.c |   99 ++++++++++++++---------------
>  1 files changed, 48 insertions(+), 51 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/powernv/npu-dma.c b/arch/powerpc/platforms/powernv/npu-dma.c
> index 8006c54..c8f438a 100644
> --- a/arch/powerpc/platforms/powernv/npu-dma.c
> +++ b/arch/powerpc/platforms/powernv/npu-dma.c
> @@ -454,79 +454,76 @@ static void put_mmio_atsd_reg(struct npu *npu, int reg)
>  }
>  
>  /* MMIO ATSD register offsets */
> -#define XTS_ATSD_AVA  1
> -#define XTS_ATSD_STAT 2
> +#define XTS_ATSD_LAUNCH 0
> +#define XTS_ATSD_AVA    1
> +#define XTS_ATSD_STAT   2
>  
> -static void mmio_launch_invalidate(struct mmio_atsd_reg *mmio_atsd_reg,
> -				unsigned long launch, unsigned long va)
> +static unsigned long get_atsd_launch_val(unsigned long pid, unsigned long psize,
> +					bool flush)
>  {
> -	struct npu *npu = mmio_atsd_reg->npu;
> -	int reg = mmio_atsd_reg->reg;
> +	unsigned long launch = 0;
>  
> -	__raw_writeq_be(va, npu->mmio_atsd_regs[reg] + XTS_ATSD_AVA);
> -	eieio();
> -	__raw_writeq_be(launch, npu->mmio_atsd_regs[reg]);
> +	if (psize == MMU_PAGE_COUNT) {
> +		/* IS set to invalidate entire matching PID */
> +		launch |= PPC_BIT(12);
> +	} else {
> +		/* AP set to invalidate region of psize */
> +		launch |= (u64)mmu_get_ap(psize) << PPC_BITLSHIFT(17);
> +	}
> +
> +	/* PRS set to process-scoped */
> +	launch |= PPC_BIT(13);
> +
> +	/* PID */
> +	launch |= pid << PPC_BITLSHIFT(38);
> +
> +	/* No flush */
> +	launch |= !flush << PPC_BITLSHIFT(39);
> +
> +	return launch;
>  }
>  
> -static void mmio_invalidate_pid(struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS],
> -				unsigned long pid, bool flush)
> +static void mmio_atsd_regs_write(struct mmio_atsd_reg
> +			mmio_atsd_reg[NV_MAX_NPUS], unsigned long offset,
> +			unsigned long val)
>  {
> -	int i;
> -	unsigned long launch;
> +	struct npu *npu;
> +	int i, reg;
>  
>  	for (i = 0; i <= max_npu2_index; i++) {
> -		if (mmio_atsd_reg[i].reg < 0)
> +		reg = mmio_atsd_reg[i].reg;
> +		if (reg < 0)
>  			continue;
>  
> -		/* IS set to invalidate matching PID */
> -		launch = PPC_BIT(12);
> -
> -		/* PRS set to process-scoped */
> -		launch |= PPC_BIT(13);
> -
> -		/* AP */
> -		launch |= (u64)
> -			mmu_get_ap(mmu_virtual_psize) << PPC_BITLSHIFT(17);
> -
> -		/* PID */
> -		launch |= pid << PPC_BITLSHIFT(38);
> +		npu = mmio_atsd_reg[i].npu;
> +		__raw_writeq_be(val, npu->mmio_atsd_regs[reg] + offset);
> +	}
> +}
>  
> -		/* No flush */
> -		launch |= !flush << PPC_BITLSHIFT(39);
> +static void mmio_invalidate_pid(struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS],
> +				unsigned long pid, bool flush)
> +{
> +	unsigned long launch = get_atsd_launch_val(pid, MMU_PAGE_COUNT, flush);
>  
> -		/* Invalidating the entire process doesn't use a va */
> -		mmio_launch_invalidate(&mmio_atsd_reg[i], launch, 0);
> -	}
> +	/* Invalidating the entire process doesn't use a va */
> +	mmio_atsd_regs_write(mmio_atsd_reg, XTS_ATSD_LAUNCH, launch);
>  }
>  
>  static void mmio_invalidate_va(struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS],
>  			unsigned long va, unsigned long pid, bool flush)
>  {
> -	int i;
>  	unsigned long launch;
>  
> -	for (i = 0; i <= max_npu2_index; i++) {
> -		if (mmio_atsd_reg[i].reg < 0)
> -			continue;
> -
> -		/* IS set to invalidate target VA */
> -		launch = 0;
> +	launch = get_atsd_launch_val(pid, mmu_virtual_psize, flush);
>  
> -		/* PRS set to process scoped */
> -		launch |= PPC_BIT(13);
> +	/* Write all VAs first */
> +	mmio_atsd_regs_write(mmio_atsd_reg, XTS_ATSD_AVA, va);
>  
> -		/* AP */
> -		launch |= (u64)
> -			mmu_get_ap(mmu_virtual_psize) << PPC_BITLSHIFT(17);
> -
> -		/* PID */
> -		launch |= pid << PPC_BITLSHIFT(38);
> -
> -		/* No flush */
> -		launch |= !flush << PPC_BITLSHIFT(39);
> +	/* Issue one barrier for all address writes */
> +	eieio();
>  
> -		mmio_launch_invalidate(&mmio_atsd_reg[i], launch, va);
> -	}
> +	/* Launch */
> +	mmio_atsd_regs_write(mmio_atsd_reg, XTS_ATSD_LAUNCH, launch);
>  }
>  
>  #define mn_to_npu_context(x) container_of(x, struct npu_context, mn)
> 

Re: [PATCH 2/3] powerpc/powernv/npu: Use size-based ATSD invalidates

[Mark Hairgrove]

Thanks for the review. Comments below.

On Tue, 2 Oct 2018, Alistair Popple wrote:

> Thanks Mark,
> 
> Looks like some worthwhile improvments to be had. I've added a couple of
> comments inline below.
> 
> > +#define PAGE_64K (64UL * 1024) +#define PAGE_2M (2UL * 1024 * 1024) +#define
> > PAGE_1G (1UL * 1024 * 1024 * 1024)
> 
> include/linux/sizes.h includes definitions for SZ_64K, SZ_2M, SZ_1G, etc. so
> unless they're redefined here for some reason I personally think it's cleaner to
> use those.

Agreed, will fix. Thanks for the pointer.


> 
> >  /*
> > - * Invalidate either a single address or an entire PID depending on
> > - * the value of va.
> > + * Invalidate a virtual address range
> >   */
> > -static void mmio_invalidate(struct npu_context *npu_context, int va,
> > -			unsigned long address, bool flush)
> > +static void mmio_invalidate(struct npu_context *npu_context,
> > +			unsigned long start, unsigned long size, bool flush)
> 
> With this optimisation every caller of mmio_invalidate() sets flush == true so
> it no longer appears to be used. We should drop it as a parameter unless you
> think there might be some reason to use it in future?
> 
> Therefore we could also drop it as a parameter to get_atsd_launch_val(),
> mmio_invalidate_pid() and mmio_invalidate_range() as well as I couldn't find any
> callers of those that set it to anything other than true.

Yeah, good catch. I'll simplify all of those.


> 
> >  	struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS];
> >  	unsigned long pid = npu_context->mm->context.id;
> > +	unsigned long atsd_start = 0;
> > +	unsigned long end = start + size - 1;
> > +	int atsd_psize = MMU_PAGE_COUNT;
> > +
> > +	/*
> > +	 * Convert the input range into one of the supported sizes. If the range
> > +	 * doesn't fit, use the next larger supported size. Invalidation latency
> > +	 * is high, so over-invalidation is preferred to issuing multiple
> > +	 * invalidates.
> > +	 */
> > +	if (size == PAGE_64K) {
> 
> We also support 4K page sizes on PPC. If I am not mistaken this means every ATSD
> would invalidate the entire GPU TLB for a the given PID on those systems. Could
> we change the above check to `if (size <= PAGE_64K)` to avoid this?

PPC supports 4K pages but the GPU ATS implementation does not. For that
reason I didn't bother handling invalidates smaller than 64K. I'll add a
comment on that.

I don't know that this requirement is enforced anywhere though. I could
add a PAGE_SIZE == 64K check to pnv_npu2_init_context if you think it
would be useful.


> 
> > +		atsd_start = start;
> 
> Which would also require:
> 
>       	    atsd_start = ALIGN_DOWN(start, PAGE_64K);
> 
> > +		atsd_psize = MMU_PAGE_64K;
> > +	} else if (ALIGN_DOWN(start, PAGE_2M) == ALIGN_DOWN(end, PAGE_2M)) {
> 
> Wouldn't this lead to under invalidation in ranges which happen to cross a 2M
> boundary? For example invalidating a 128K (ie. 2x64K pages) range with start ==
> 0x1f0000 and end == 0x210000 would result in an invalidation of the range 0x0 -
> 0x200000 incorrectly leaving 0x200000 - 0x210000 in the GPU TLB.

In this example:
start                         0x1f0000
size                          0x020000
end (start + size - 1)        0x20ffff
ALIGN_DOWN(start, PAGE_2M)    0x000000
ALIGN_DOWN(end, PAGE_2M)      0x200000

Since ALIGN_DOWN(start, PAGE_2M) != ALIGN_DOWN(end, PAGE_2M), the
condition fails and we move to the 1G clause. Then
ALIGN_DOWN(start, PAGE_1G) == ALIGN_DOWN(end, PAGE_1G) == 0, so we
invalidate the range [0, 1G).

Re: [PATCH 2/3] powerpc/powernv/npu: Use size-based ATSD invalidates

[Alistair Popple]

> > 
> > We also support 4K page sizes on PPC. If I am not mistaken this means every ATSD
> > would invalidate the entire GPU TLB for a the given PID on those systems. Could
> > we change the above check to `if (size <= PAGE_64K)` to avoid this?
> 
> PPC supports 4K pages but the GPU ATS implementation does not. For that
> reason I didn't bother handling invalidates smaller than 64K. I'll add a
> comment on that.

Interesting, I was not aware of that limitation. Do you know if it is a
SW/driver limitation or a HW limitation?

> I don't know that this requirement is enforced anywhere though. I could
> add a PAGE_SIZE == 64K check to pnv_npu2_init_context if you think it
> would be useful.

Given it's a static kernel build parameter perhaps it makes more sense to do the
check as part of the driver build in a conftest rather than a runtime failure?

> >
> > > +		atsd_start = start;
> > 
> > Which would also require:
> > 
> >       	    atsd_start = ALIGN_DOWN(start, PAGE_64K);
> > 
> > > +		atsd_psize = MMU_PAGE_64K;
> > > +	} else if (ALIGN_DOWN(start, PAGE_2M) == ALIGN_DOWN(end, PAGE_2M)) {
> > 
> > Wouldn't this lead to under invalidation in ranges which happen to cross a 2M
> > boundary? For example invalidating a 128K (ie. 2x64K pages) range with start ==
> > 0x1f0000 and end == 0x210000 would result in an invalidation of the range 0x0 -
> > 0x200000 incorrectly leaving 0x200000 - 0x210000 in the GPU TLB.
> 
> In this example:
> start                         0x1f0000
> size                          0x020000
> end (start + size - 1)        0x20ffff
> ALIGN_DOWN(start, PAGE_2M)    0x000000
> ALIGN_DOWN(end, PAGE_2M)      0x200000
> 
> Since ALIGN_DOWN(start, PAGE_2M) != ALIGN_DOWN(end, PAGE_2M), the
> condition fails and we move to the 1G clause. Then
> ALIGN_DOWN(start, PAGE_1G) == ALIGN_DOWN(end, PAGE_1G) == 0, so we
> invalidate the range [0, 1G).

Oh yeah, sorry that makes sense and looks good to me.

- Alistair

Re: [PATCH 2/3] powerpc/powernv/npu: Use size-based ATSD invalidates

[Mark Hairgrove]

On Wed, 3 Oct 2018, Alistair Popple wrote:

> > > 
> > > We also support 4K page sizes on PPC. If I am not mistaken this means every ATSD
> > > would invalidate the entire GPU TLB for a the given PID on those systems. Could
> > > we change the above check to `if (size <= PAGE_64K)` to avoid this?
> > 
> > PPC supports 4K pages but the GPU ATS implementation does not. For that
> > reason I didn't bother handling invalidates smaller than 64K. I'll add a
> > comment on that.
> 
> Interesting, I was not aware of that limitation. Do you know if it is a
> SW/driver limitation or a HW limitation?

HW.

> 
> > I don't know that this requirement is enforced anywhere though. I could
> > add a PAGE_SIZE == 64K check to pnv_npu2_init_context if you think it
> > would be useful.
> 
> Given it's a static kernel build parameter perhaps it makes more sense to do the
> check as part of the driver build in a conftest rather than a runtime failure?

Sure, we can do it in the driver. I'll just stick with a comment for the
kernel.