Subject: Re: [PATCH v3] powerpc/npu-dma.c: Fix deadlock in mmio_invalidate

From: Alistair Popple <[email-masked]>
Date: Thu, 08 Mar 2018 14:41:53 +1100

Michael,

This won't apply cleanly on top of Balbir's MMIO ATSD Flushing patch
(https://patchwork.ozlabs.org/patch/848343/). I will resend a v4 which applies
cleanly on top of that as the rebase/merge is non-trivial.

- Alistair

On Friday, 2 March 2018 4:18:45 PM AEDT Alistair Popple wrote:
> When sending TLB invalidates to the NPU we need to send extra flushes due
> to a hardware issue. The original implementation would lock the all the
> ATSD MMIO registers sequentially before unlocking and relocking each of
> them sequentially to do the extra flush.
> 
> This introduced a deadlock as it is possible for one thread to hold one
> ATSD register whilst waiting for another register to be freed while the
> other thread is holding that register waiting for the one in the first
> thread to be freed.
> 
> For example if there are two threads and two ATSD registers:
> 
> Thread A	Thread B
> Acquire 1
> Acquire 2
> Release 1	Acquire 1
> Wait 1		Wait 2
> 
> Both threads will be stuck waiting to acquire a register resulting in an
> RCU stall warning or soft lockup.
> 
> This patch solves the deadlock by refactoring the code to ensure registers
> are not released between flushes and to ensure all registers are either
> acquired or released together and in order.
> 
> Fixes: bbd5ff50afff ("powerpc/powernv/npu-dma: Add explicit flush when sending an ATSD")
> Signed-off-by: Alistair Popple <[email-masked]>
> 
> ---
> 
> v2:
>  - Added memory barriers around ATSD register aquisition/release
>  - Added compiler barriers around npdev[][] assignment
> 
> v3:
>  - Added comments to describe required locking
> 
> ---
>  arch/powerpc/platforms/powernv/npu-dma.c | 228 +++++++++++++++++++------------
>  1 file changed, 139 insertions(+), 89 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/powernv/npu-dma.c b/arch/powerpc/platforms/powernv/npu-dma.c
> index 0a253b64ac5f..0dec96eb3358 100644
> --- a/arch/powerpc/platforms/powernv/npu-dma.c
> +++ b/arch/powerpc/platforms/powernv/npu-dma.c
> @@ -410,6 +410,11 @@ struct npu_context {
>  	void *priv;
>  };
>  
> +struct mmio_atsd_reg {
> +	struct npu *npu;
> +	int reg;
> +};
> +
>  /*
>   * Find a free MMIO ATSD register and mark it in use. Return -ENOSPC
>   * if none are available.
> @@ -419,7 +424,7 @@ static int get_mmio_atsd_reg(struct npu *npu)
>  	int i;
>  
>  	for (i = 0; i < npu->mmio_atsd_count; i++) {
> -		if (!test_and_set_bit(i, &npu->mmio_atsd_usage))
> +		if (!test_and_set_bit_lock(i, &npu->mmio_atsd_usage))
>  			return i;
>  	}
>  
> @@ -428,86 +433,90 @@ static int get_mmio_atsd_reg(struct npu *npu)
>  
>  static void put_mmio_atsd_reg(struct npu *npu, int reg)
>  {
> -	clear_bit(reg, &npu->mmio_atsd_usage);
> +	clear_bit_unlock(reg, &npu->mmio_atsd_usage);
>  }
>  
>  /* MMIO ATSD register offsets */
>  #define XTS_ATSD_AVA  1
>  #define XTS_ATSD_STAT 2
>  
> -static int mmio_launch_invalidate(struct npu *npu, unsigned long launch,
> -				unsigned long va)
> +static void mmio_launch_invalidate(struct mmio_atsd_reg *mmio_atsd_reg,
> +				unsigned long launch, unsigned long va)
>  {
> -	int mmio_atsd_reg;
> -
> -	do {
> -		mmio_atsd_reg = get_mmio_atsd_reg(npu);
> -		cpu_relax();
> -	} while (mmio_atsd_reg < 0);
> +	struct npu *npu = mmio_atsd_reg->npu;
> +	int reg = mmio_atsd_reg->reg;
>  
>  	__raw_writeq(cpu_to_be64(va),
> -		npu->mmio_atsd_regs[mmio_atsd_reg] + XTS_ATSD_AVA);
> +		npu->mmio_atsd_regs[reg] + XTS_ATSD_AVA);
>  	eieio();
> -	__raw_writeq(cpu_to_be64(launch), npu->mmio_atsd_regs[mmio_atsd_reg]);
> -
> -	return mmio_atsd_reg;
> +	__raw_writeq(cpu_to_be64(launch), npu->mmio_atsd_regs[reg]);
>  }
>  
> -static int mmio_invalidate_pid(struct npu *npu, unsigned long pid, bool flush)
> +static void mmio_invalidate_pid(struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS],
> +				unsigned long pid, bool flush)
>  {
> +	int i;
>  	unsigned long launch;
>  
> -	/* IS set to invalidate matching PID */
> -	launch = PPC_BIT(12);
> +	for (i = 0; i <= max_npu2_index; i++) {
> +		if (mmio_atsd_reg[i].reg < 0)
> +			continue;
> +
> +		/* IS set to invalidate matching PID */
> +		launch = PPC_BIT(12);
>  
> -	/* PRS set to process-scoped */
> -	launch |= PPC_BIT(13);
> +		/* PRS set to process-scoped */
> +		launch |= PPC_BIT(13);
>  
> -	/* AP */
> -	launch |= (u64) mmu_get_ap(mmu_virtual_psize) << PPC_BITLSHIFT(17);
> +		/* AP */
> +		launch |= (u64)
> +			mmu_get_ap(mmu_virtual_psize) << PPC_BITLSHIFT(17);
>  
> -	/* PID */
> -	launch |= pid << PPC_BITLSHIFT(38);
> +		/* PID */
> +		launch |= pid << PPC_BITLSHIFT(38);
>  
> -	/* No flush */
> -	launch |= !flush << PPC_BITLSHIFT(39);
> +		/* No flush */
> +		launch |= !flush << PPC_BITLSHIFT(39);
>  
> -	/* Invalidating the entire process doesn't use a va */
> -	return mmio_launch_invalidate(npu, launch, 0);
> +		/* Invalidating the entire process doesn't use a va */
> +		mmio_launch_invalidate(&mmio_atsd_reg[i], launch, 0);
> +	}
>  }
>  
> -static int mmio_invalidate_va(struct npu *npu, unsigned long va,
> -			unsigned long pid, bool flush)
> +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;
>  
> -	/* IS set to invalidate target VA */
> -	launch = 0;
> +	for (i = 0; i <= max_npu2_index; i++) {
> +		if (mmio_atsd_reg[i].reg < 0)
> +			continue;
>  
> -	/* PRS set to process scoped */
> -	launch |= PPC_BIT(13);
> +		/* IS set to invalidate target VA */
> +		launch = 0;
>  
> -	/* AP */
> -	launch |= (u64) mmu_get_ap(mmu_virtual_psize) << PPC_BITLSHIFT(17);
> +		/* PRS set to process scoped */
> +		launch |= PPC_BIT(13);
>  
> -	/* PID */
> -	launch |= pid << PPC_BITLSHIFT(38);
> +		/* AP */
> +		launch |= (u64)
> +			mmu_get_ap(mmu_virtual_psize) << PPC_BITLSHIFT(17);
>  
> -	/* No flush */
> -	launch |= !flush << PPC_BITLSHIFT(39);
> +		/* PID */
> +		launch |= pid << PPC_BITLSHIFT(38);
>  
> -	return mmio_launch_invalidate(npu, launch, va);
> +		/* No flush */
> +		launch |= !flush << PPC_BITLSHIFT(39);
> +
> +		mmio_launch_invalidate(&mmio_atsd_reg[i], launch, va);
> +	}
>  }
>  
>  #define mn_to_npu_context(x) container_of(x, struct npu_context, mn)
>  
> -struct mmio_atsd_reg {
> -	struct npu *npu;
> -	int reg;
> -};
> -
>  static void mmio_invalidate_wait(
> -	struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS], bool flush)
> +	struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS])
>  {
>  	struct npu *npu;
>  	int i, reg;
> @@ -522,16 +531,65 @@ static void mmio_invalidate_wait(
>  		reg = mmio_atsd_reg[i].reg;
>  		while (__raw_readq(npu->mmio_atsd_regs[reg] + XTS_ATSD_STAT))
>  			cpu_relax();
> +	}
> +}
>  
> -		put_mmio_atsd_reg(npu, reg);
> +/*
> + * Acquires all the address translation shootdown (ATSD) registers required to
> + * launch an ATSD on all links this npu_context is active on.
> + */
> +static void acquire_atsd_reg(struct npu_context *npu_context,
> +			struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS])
> +{
> +	int i, j;
> +	struct npu *npu;
> +	struct pci_dev *npdev;
> +	struct pnv_phb *nphb;
>  
> -		/*
> -		 * The GPU requires two flush ATSDs to ensure all entries have
> -		 * been flushed. We use PID 0 as it will never be used for a
> -		 * process on the GPU.
> +	for (i = 0; i <= max_npu2_index; i++) {
> +		mmio_atsd_reg[i].reg = -1;
> +		for (j = 0; j < NV_MAX_LINKS; j++) {
> +			/* There are no ordering requirements with respect to
> +			 * the setup of struct npu_context, but to ensure
> +			 * consistent behaviour we need to ensure npdev[][] is
> +			 * only read once.
> +			 */
> +			npdev = READ_ONCE(npu_context->npdev[i][j]);
> +			if (!npdev)
> +				continue;
> +
> +			nphb = pci_bus_to_host(npdev->bus)->private_data;
> +			npu = &nphb->npu;
> +			mmio_atsd_reg[i].npu = npu;
> +			mmio_atsd_reg[i].reg = get_mmio_atsd_reg(npu);
> +			while (mmio_atsd_reg[i].reg < 0) {
> +				mmio_atsd_reg[i].reg = get_mmio_atsd_reg(npu);
> +				cpu_relax();
> +			}
> +			break;
> +		}
> +	}
> +}
> +
> +/*
> + * Release previously acquired ATSD registers. To avoid deadlocks the registers
> + * must be released in the same order they were acquired above in
> + * acquire_atsd_reg.
> + */
> +static void release_atsd_reg(struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS])
> +{
> +	int i;
> +
> +	for (i = 0; i <= max_npu2_index; i++) {
> +		/* We can't rely on npu_context->npdev[][] being the same here
> +		 * as when acquire_atsd_reg() was called, hence we use the
> +		 * values stored in mmio_atsd_reg during the acquire phase
> +		 * rather than re-reading npdev[][]
>  		 */
> -		if (flush)
> -			mmio_invalidate_pid(npu, 0, true);
> +		if (mmio_atsd_reg[i].reg < 0)
> +			continue;
> +
> +		put_mmio_atsd_reg(mmio_atsd_reg[i].npu, mmio_atsd_reg[i].reg);
>  	}
>  }
>  
> @@ -542,10 +600,6 @@ static void mmio_invalidate_wait(
>  static void mmio_invalidate(struct npu_context *npu_context, int va,
>  			unsigned long address, bool flush)
>  {
> -	int i, j;
> -	struct npu *npu;
> -	struct pnv_phb *nphb;
> -	struct pci_dev *npdev;
>  	struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS];
>  	unsigned long pid = npu_context->mm->context.id;
>  
> @@ -561,37 +615,25 @@ static void mmio_invalidate(struct npu_context *npu_context, int va,
>  	 * Loop over all the NPUs this process is active on and launch
>  	 * an invalidate.
>  	 */
> -	for (i = 0; i <= max_npu2_index; i++) {
> -		mmio_atsd_reg[i].reg = -1;
> -		for (j = 0; j < NV_MAX_LINKS; j++) {
> -			npdev = npu_context->npdev[i][j];
> -			if (!npdev)
> -				continue;
> -
> -			nphb = pci_bus_to_host(npdev->bus)->private_data;
> -			npu = &nphb->npu;
> -			mmio_atsd_reg[i].npu = npu;
> -
> -			if (va)
> -				mmio_atsd_reg[i].reg =
> -					mmio_invalidate_va(npu, address, pid,
> -							flush);
> -			else
> -				mmio_atsd_reg[i].reg =
> -					mmio_invalidate_pid(npu, pid, flush);
> -
> -			/*
> -			 * The NPU hardware forwards the shootdown to all GPUs
> -			 * so we only have to launch one shootdown per NPU.
> -			 */
> -			break;
> -		}
> +	acquire_atsd_reg(npu_context, mmio_atsd_reg);
> +	if (va)
> +		mmio_invalidate_va(mmio_atsd_reg, address, pid, flush);
> +	else
> +		mmio_invalidate_pid(mmio_atsd_reg, pid, flush);
> +
> +	mmio_invalidate_wait(mmio_atsd_reg);
> +	if (flush) {
> +		/*
> +		 * The GPU requires two flush ATSDs to ensure all entries have
> +		 * been flushed. We use PID 0 as it will never be used for a
> +		 * process on the GPU.
> +		 */
> +		mmio_invalidate_pid(mmio_atsd_reg, 0, true);
> +		mmio_invalidate_wait(mmio_atsd_reg);
> +		mmio_invalidate_pid(mmio_atsd_reg, 0, true);
> +		mmio_invalidate_wait(mmio_atsd_reg);
>  	}
> -
> -	mmio_invalidate_wait(mmio_atsd_reg, flush);
> -	if (flush)
> -		/* Wait for the flush to complete */
> -		mmio_invalidate_wait(mmio_atsd_reg, false);
> +	release_atsd_reg(mmio_atsd_reg);
>  }
>  
>  static void pnv_npu2_mn_release(struct mmu_notifier *mn,
> @@ -726,7 +768,15 @@ struct npu_context *pnv_npu2_init_context(struct pci_dev *gpdev,
>  	if (WARN_ON(of_property_read_u32(nvlink_dn, "ibm,npu-link-index",
>  							&nvlink_index)))
>  		return ERR_PTR(-ENODEV);
> -	npu_context->npdev[npu->index][nvlink_index] = npdev;
> +
> +	/* npdev is a pci_dev pointer setup by the PCI code. We assign it to
> +	 * npdev[][] to indicate to the mmu notifiers that an invalidation
> +	 * should also be sent over this nvlink. The notifiers don't use any
> +	 * other fields in npu_context, so we just need to ensure that when they
> +	 * deference npu_context->npdev[][] it is either a valid pointer or
> +	 * NULL.
> +	 */
> +	WRITE_ONCE(npu_context->npdev[npu->index][nvlink_index], npdev);
>  
>  	if (!nphb->npu.nmmu_flush) {
>  		/*
> @@ -778,7 +828,7 @@ void pnv_npu2_destroy_context(struct npu_context *npu_context,
>  	if (WARN_ON(of_property_read_u32(nvlink_dn, "ibm,npu-link-index",
>  							&nvlink_index)))
>  		return;
> -	npu_context->npdev[npu->index][nvlink_index] = NULL;
> +	WRITE_ONCE(npu_context->npdev[npu->index][nvlink_index], NULL);
>  	opal_npu_destroy_context(nphb->opal_id, npu_context->mm->context.id,
>  				PCI_DEVID(gpdev->bus->number, gpdev->devfn));
>  	kref_put(&npu_context->kref, pnv_npu2_release_context);
>