Re: [PATCH 13/16] drm/amdkfd: use standard kernel kfifo for IH

[Oded Gabbay <oded.gabbay-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On Sat, Oct 21, 2017 at 3:23 AM, Felix Kuehling <Felix.Kuehling@amd.com> wrote:
> From: Andres Rodriguez <andres.rodriguez@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@amd.com>
> Signed-off-by: Felix Kuehling <Felix.Kuehling@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@gmail.com>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx