On Sat, Oct 21, 2017 at 3:23 AM, Felix Kuehling <Felix.Kuehling at amd.com> wrote:
> From: Andres Rodriguez <andres.rodriguez at amd.com>
>
> Replace our implementation of a lockless ring buffer with the standard
> linux kernel kfifo.
>
> We shouldn't maintain our own version of a standard data structure.
>
> Signed-off-by: Andres Rodriguez <andres.rodriguez at amd.com>
> Signed-off-by: Felix Kuehling <Felix.Kuehling at amd.com>
> ---
> drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c | 78 ++++++++++--------------------
> drivers/gpu/drm/amd/amdkfd/kfd_priv.h | 6 +--
> 2 files changed, 27 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c b/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
> index 70b3a99c..ffbb91a 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
> @@ -42,25 +42,24 @@
>
> #include <linux/slab.h>
> #include <linux/device.h>
> +#include <linux/kfifo.h>
> #include "kfd_priv.h"
>
> -#define KFD_INTERRUPT_RING_SIZE 1024
> +#define KFD_IH_NUM_ENTRIES 1024
>
> static void interrupt_wq(struct work_struct *);
>
> int kfd_interrupt_init(struct kfd_dev *kfd)
> {
> - void *interrupt_ring = kmalloc_array(KFD_INTERRUPT_RING_SIZE,
> - kfd->device_info->ih_ring_entry_size,
> - GFP_KERNEL);
> - if (!interrupt_ring)
> - return -ENOMEM;
> -
> - kfd->interrupt_ring = interrupt_ring;
> - kfd->interrupt_ring_size =
> - KFD_INTERRUPT_RING_SIZE * kfd->device_info->ih_ring_entry_size;
> - atomic_set(&kfd->interrupt_ring_wptr, 0);
> - atomic_set(&kfd->interrupt_ring_rptr, 0);
> + int r;
> +
> + r = kfifo_alloc(&kfd->ih_fifo,
> + KFD_IH_NUM_ENTRIES * kfd->device_info->ih_ring_entry_size,
> + GFP_KERNEL);
> + if (r) {
> + dev_err(kfd_chardev(), "Failed to allocate IH fifo\n");
> + return r;
> + }
>
> spin_lock_init(&kfd->interrupt_lock);
>
> @@ -98,68 +97,41 @@ void kfd_interrupt_exit(struct kfd_dev *kfd)
> */
> flush_scheduled_work();
>
> - kfree(kfd->interrupt_ring);
> + kfifo_free(&kfd->ih_fifo);
> }
>
> /*
> - * This assumes that it can't be called concurrently with itself
> - * but only with dequeue_ih_ring_entry.
> + * Assumption: single reader/writer. This function is not re-entrant
> */
> bool enqueue_ih_ring_entry(struct kfd_dev *kfd, const void *ih_ring_entry)
> {
> - unsigned int rptr = atomic_read(&kfd->interrupt_ring_rptr);
> - unsigned int wptr = atomic_read(&kfd->interrupt_ring_wptr);
> + int count;
>
> - if ((rptr - wptr) % kfd->interrupt_ring_size ==
> - kfd->device_info->ih_ring_entry_size) {
> - /* This is very bad, the system is likely to hang. */
> + count = kfifo_in(&kfd->ih_fifo, ih_ring_entry,
> + kfd->device_info->ih_ring_entry_size);
> + if (count != kfd->device_info->ih_ring_entry_size) {
> dev_err_ratelimited(kfd_chardev(),
> - "Interrupt ring overflow, dropping interrupt.\n");
> + "Interrupt ring overflow, dropping interrupt %d\n",
> + count);
> return false;
> }
>
> - memcpy(kfd->interrupt_ring + wptr, ih_ring_entry,
> - kfd->device_info->ih_ring_entry_size);
> -
> - wptr = (wptr + kfd->device_info->ih_ring_entry_size) %
> - kfd->interrupt_ring_size;
> - smp_wmb(); /* Ensure memcpy'd data is visible before wptr update. */
> - atomic_set(&kfd->interrupt_ring_wptr, wptr);
> -
> return true;
> }
>
> /*
> - * This assumes that it can't be called concurrently with itself
> - * but only with enqueue_ih_ring_entry.
> + * Assumption: single reader/writer. This function is not re-entrant
> */
> static bool dequeue_ih_ring_entry(struct kfd_dev *kfd, void *ih_ring_entry)
> {
> - /*
> - * Assume that wait queues have an implicit barrier, i.e. anything that
> - * happened in the ISR before it queued work is visible.
> - */
> -
> - unsigned int wptr = atomic_read(&kfd->interrupt_ring_wptr);
> - unsigned int rptr = atomic_read(&kfd->interrupt_ring_rptr);
> + int count;
>
> - if (rptr == wptr)
> - return false;
> -
> - memcpy(ih_ring_entry, kfd->interrupt_ring + rptr,
> - kfd->device_info->ih_ring_entry_size);
> -
> - rptr = (rptr + kfd->device_info->ih_ring_entry_size) %
> - kfd->interrupt_ring_size;
> + count = kfifo_out(&kfd->ih_fifo, ih_ring_entry,
> + kfd->device_info->ih_ring_entry_size);
>
> - /*
> - * Ensure the rptr write update is not visible until
> - * memcpy has finished reading.
> - */
> - smp_mb();
> - atomic_set(&kfd->interrupt_ring_rptr, rptr);
> + WARN_ON(count && count != kfd->device_info->ih_ring_entry_size);
>
> - return true;
> + return count == kfd->device_info->ih_ring_entry_size;
> }
>
> static void interrupt_wq(struct work_struct *work)
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> index ebae8e1..e8d6c0e 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> @@ -32,6 +32,7 @@
> #include <linux/spinlock.h>
> #include <linux/kfd_ioctl.h>
> #include <linux/idr.h>
> +#include <linux/kfifo.h>
> #include <kgd_kfd_interface.h>
>
> #include "amd_shared.h"
> @@ -182,10 +183,7 @@ struct kfd_dev {
> unsigned int gtt_sa_num_of_chunks;
>
> /* Interrupts */
> - void *interrupt_ring;
> - size_t interrupt_ring_size;
> - atomic_t interrupt_ring_rptr;
> - atomic_t interrupt_ring_wptr;
> + struct kfifo ih_fifo;
> struct work_struct interrupt_work;
> spinlock_t interrupt_lock;
>
> --
> 2.7.4
>
This patch is:
Acked-by: Oded Gabbay <oded.gabbay at gmail.com>
