[PATCH 00/16] KFD interrupt and signal event handling improvements

[PATCH 00/16] KFD interrupt and signal event handling improvements

[Felix Kuehling]

This patch series improves interrupt handling latency, signal event
processing overhead and replaces some custom data structures with
standard kernel data structures (idr, kfifo, waitqueue).

It also increases the capacity of the number of signals that can be
processed from 256 to 4096. This breaks ancient versions of the Thunk
that support only 256 signal events. The current WIP-version on github
supports both sizes. If support for ancient Thunks is considered
important, this could be fixed by allowing mappings that are smaller
than 4096 signals, and limiting the number of signals per process
depending on the size of the mapped events page.

Andres Rodriguez (4):
  drm/amdkfd: use standard kernel kfifo for IH
  drm/amdkfd: increase IH num entries to 8192
  drm/amdkfd: wait only for IH work on IH exit
  drm/amdkfd: use a high priority workqueue for IH work

Besar Wicaksono (1):
  drm/amdkfd: Add SDMA trap src id to the KFD isr wanted list

Felix Kuehling (8):
  drm/amdkfd: Don't dereference kfd_process.mm
  drm/amdkfd: Clean up kfd_wait_on_events
  drm/amdkfd: Fix event destruction with pending waiters
  drm/amdkfd: remove redundant kfd_event_waiter.input_index
  drm/amdkfd: Use wait_queue_t to implement event waiting
  drm/amdkfd: Simplify events page allocator
  drm/amdkfd: Simplify event ID and signal slot management
  drm/amdkfd: Use IH context ID for signal lookup

Oded Gabbay (1):
  drm/amdkfd: increase limit of signal events to 4096 per process

Sean Keely (2):
  drm/amdkfd: Short cut for kfd_wait_on_events without waiting
  drm/amdkfd: Fix scheduler race in kfd_wait_on_events sleep loop

 drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c |   8 +-
 drivers/gpu/drm/amd/amdkfd/cik_int.h             |   3 +-
 drivers/gpu/drm/amd/amdkfd/kfd_chardev.c         |   5 +-
 drivers/gpu/drm/amd/amdkfd/kfd_device.c          |   2 +-
 drivers/gpu/drm/amd/amdkfd/kfd_events.c          | 588 ++++++++++-------------
 drivers/gpu/drm/amd/amdkfd/kfd_events.h          |  18 +-
 drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c       |  83 ++--
 drivers/gpu/drm/amd/amdkfd/kfd_priv.h            |  32 +-
 drivers/gpu/drm/amd/amdkfd/kfd_process.c         |   6 +-
 include/uapi/linux/kfd_ioctl.h                   |   2 +-
 10 files changed, 332 insertions(+), 415 deletions(-)

-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 01/16] drm/amdkfd: Add SDMA trap src id to the KFD isr wanted list

[Felix Kuehling]

From: Besar Wicaksono <besar.wicaksono@amd.com>

This enables SDMA signalling with event interrupt.

Signed-off-by: Besar Wicaksono <Besar.Wicaksono@amd.com>
Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c | 3 +++
 drivers/gpu/drm/amd/amdkfd/cik_int.h             | 3 ++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c b/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c
index 211fc48..66164aa 100644
--- a/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c
+++ b/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c
@@ -36,6 +36,7 @@ static bool cik_event_interrupt_isr(struct kfd_dev *dev,
 	/* Do not process in ISR, just request it to be forwarded to WQ. */
 	return (pasid != 0) &&
 		(ihre->source_id == CIK_INTSRC_CP_END_OF_PIPE ||
+		ihre->source_id == CIK_INTSRC_SDMA_TRAP ||
 		ihre->source_id == CIK_INTSRC_SQ_INTERRUPT_MSG ||
 		ihre->source_id == CIK_INTSRC_CP_BAD_OPCODE);
 }
@@ -54,6 +55,8 @@ static void cik_event_interrupt_wq(struct kfd_dev *dev,
 
 	if (ihre->source_id == CIK_INTSRC_CP_END_OF_PIPE)
 		kfd_signal_event_interrupt(pasid, 0, 0);
+	else if (ihre->source_id == CIK_INTSRC_SDMA_TRAP)
+		kfd_signal_event_interrupt(pasid, 0, 0);
 	else if (ihre->source_id == CIK_INTSRC_SQ_INTERRUPT_MSG)
 		kfd_signal_event_interrupt(pasid, ihre->data & 0xFF, 8);
 	else if (ihre->source_id == CIK_INTSRC_CP_BAD_OPCODE)
diff --git a/drivers/gpu/drm/amd/amdkfd/cik_int.h b/drivers/gpu/drm/amd/amdkfd/cik_int.h
index 79a16d2..109298b 100644
--- a/drivers/gpu/drm/amd/amdkfd/cik_int.h
+++ b/drivers/gpu/drm/amd/amdkfd/cik_int.h
@@ -32,9 +32,10 @@ struct cik_ih_ring_entry {
 	uint32_t reserved;
 };
 
-#define CIK_INTSRC_DEQUEUE_COMPLETE	0xC6
 #define CIK_INTSRC_CP_END_OF_PIPE	0xB5
 #define CIK_INTSRC_CP_BAD_OPCODE	0xB7
+#define CIK_INTSRC_DEQUEUE_COMPLETE	0xC6
+#define CIK_INTSRC_SDMA_TRAP		0xE0
 #define CIK_INTSRC_SQ_INTERRUPT_MSG	0xEF
 
 #endif
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 02/16] drm/amdkfd: Don't dereference kfd_process.mm

[Felix Kuehling]

The kfd_process doesn't own a reference to the mm_struct, so it can
disappear without warning even while the kfd_process still exists.
In fact, the delayed kfd_process teardown is triggered by an MMU
notifier when the mm_struct is destroyed. Permanently holding a
reference to the mm_struct would prevent this from happening.

Therefore, avoid dereferencing the kfd_process.mm pointer and make
it opaque. Use get_task_mm to get a temporary reference to the mm
when it's needed.

Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_events.c  | 19 +++++++++++++++----
 drivers/gpu/drm/amd/amdkfd/kfd_priv.h    |  7 ++++++-
 drivers/gpu/drm/amd/amdkfd/kfd_process.c |  6 +++++-
 3 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
index 944abfa..61ce547 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
@@ -24,8 +24,8 @@
 #include <linux/slab.h>
 #include <linux/types.h>
 #include <linux/sched/signal.h>
+#include <linux/sched/mm.h>
 #include <linux/uaccess.h>
-#include <linux/mm.h>
 #include <linux/mman.h>
 #include <linux/memory.h>
 #include "kfd_priv.h"
@@ -904,14 +904,24 @@ void kfd_signal_iommu_event(struct kfd_dev *dev, unsigned int pasid,
 	 * running so the lookup function returns a locked process.
 	 */
 	struct kfd_process *p = kfd_lookup_process_by_pasid(pasid);
+	struct mm_struct *mm;
 
 	if (!p)
 		return; /* Presumably process exited. */
 
+	/* Take a safe reference to the mm_struct, which may otherwise
+	 * disappear even while the kfd_process is still referenced.
+	 */
+	mm = get_task_mm(p->lead_thread);
+	if (!mm) {
+		mutex_unlock(&p->mutex);
+		return; /* Process is exiting */
+	}
+
 	memset(&memory_exception_data, 0, sizeof(memory_exception_data));
 
-	down_read(&p->mm->mmap_sem);
-	vma = find_vma(p->mm, address);
+	down_read(&mm->mmap_sem);
+	vma = find_vma(mm, address);
 
 	memory_exception_data.gpu_id = dev->id;
 	memory_exception_data.va = address;
@@ -937,7 +947,8 @@ void kfd_signal_iommu_event(struct kfd_dev *dev, unsigned int pasid,
 		}
 	}
 
-	up_read(&p->mm->mmap_sem);
+	up_read(&mm->mmap_sem);
+	mmput(mm);
 
 	mutex_lock(&p->event_mutex);
 
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
index 7d86ec9..1a483a7 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
@@ -494,7 +494,12 @@ struct kfd_process {
 	 */
 	struct hlist_node kfd_processes;
 
-	struct mm_struct *mm;
+	/*
+	 * Opaque pointer to mm_struct. We don't hold a reference to
+	 * it so it should never be dereferenced from here. This is
+	 * only used for looking up processes by their mm.
+	 */
+	void *mm;
 
 	struct mutex mutex;
 
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
index 3ccb3b5..21d27e5 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
@@ -200,7 +200,11 @@ static void kfd_process_destroy_delayed(struct rcu_head *rcu)
 	struct kfd_process *p;
 
 	p = container_of(rcu, struct kfd_process, rcu);
-	WARN_ON(atomic_read(&p->mm->mm_count) <= 0);
+	/*
+	 * This cast should be safe here because we grabbed a
+	 * reference to the mm in kfd_process_notifier_release
+	 */
+	WARN_ON(atomic_read(&((struct mm_struct *)p->mm)->mm_count) <= 0);
 
 	mmdrop(p->mm);
 
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 03/16] drm/amdkfd: increase limit of signal events to 4096 per process

[Felix Kuehling]

From: Oded Gabbay <oded.gabbay@amd.com>

Signed-off-by: Oded Gabbay <oded.gabbay@amd.com>
Reviewed-by: Ben Goz <ben.goz@amd.com>
Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_events.c | 2 +-
 include/uapi/linux/kfd_ioctl.h          | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
index 61ce547..ade71b7 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
@@ -82,7 +82,7 @@ struct signal_page {
  * For SQ s_sendmsg interrupts, this is limited to 8 bits.
  */
 
-#define INTERRUPT_DATA_BITS 8
+#define INTERRUPT_DATA_BITS 12
 #define SIGNAL_EVENT_ID_SLOT_SHIFT 0
 
 static uint64_t *page_slots(struct signal_page *page)
diff --git a/include/uapi/linux/kfd_ioctl.h b/include/uapi/linux/kfd_ioctl.h
index 26283fe..731d0df 100644
--- a/include/uapi/linux/kfd_ioctl.h
+++ b/include/uapi/linux/kfd_ioctl.h
@@ -169,7 +169,7 @@ struct kfd_ioctl_dbg_wave_control_args {
 #define KFD_IOC_WAIT_RESULT_TIMEOUT		1
 #define KFD_IOC_WAIT_RESULT_FAIL		2
 
-#define KFD_SIGNAL_EVENT_LIMIT			256
+#define KFD_SIGNAL_EVENT_LIMIT			4096
 
 struct kfd_ioctl_create_event_args {
 	__u64 event_page_offset;	/* from KFD */
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 04/16] drm/amdkfd: Short cut for kfd_wait_on_events without waiting

[Felix Kuehling]

From: Sean Keely <sean.keely@amd.com>

If kfd_wait_on_events can return immediately, we don't need to populate
the wait list and don't need to enter the sleep-loop.

Signed-off-by: Sean Keely <sean.keely@amd.com>
Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_events.c | 43 ++++++++++++++++++++++++++++++---
 1 file changed, 39 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
index ade71b7..64cc42c 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
@@ -617,7 +617,7 @@ static struct kfd_event_waiter *alloc_event_waiters(uint32_t num_events)
 	return event_waiters;
 }
 
-static int init_event_waiter(struct kfd_process *p,
+static int init_event_waiter_get_status(struct kfd_process *p,
 		struct kfd_event_waiter *waiter,
 		uint32_t event_id,
 		uint32_t input_index)
@@ -632,11 +632,20 @@ static int init_event_waiter(struct kfd_process *p,
 	waiter->activated = ev->signaled;
 	ev->signaled = ev->signaled && !ev->auto_reset;
 
-	list_add(&waiter->waiters, &ev->waiters);
-
 	return 0;
 }
 
+static void init_event_waiter_add_to_waitlist(struct kfd_event_waiter *waiter)
+{
+	struct kfd_event *ev = waiter->event;
+
+	/* Only add to the wait list if we actually need to
+	 * wait on this event.
+	 */
+	if (!waiter->activated)
+		list_add(&waiter->waiters, &ev->waiters);
+}
+
 static bool test_event_condition(bool all, uint32_t num_events,
 				struct kfd_event_waiter *event_waiters)
 {
@@ -724,11 +733,17 @@ int kfd_wait_on_events(struct kfd_process *p,
 			(struct kfd_event_data __user *) data;
 	uint32_t i;
 	int ret = 0;
+
 	struct kfd_event_waiter *event_waiters = NULL;
 	long timeout = user_timeout_to_jiffies(user_timeout_ms);
 
 	mutex_lock(&p->event_mutex);
 
+	/* Set to something unreasonable - this is really
+	 * just a bool for now.
+	 */
+	*wait_result = KFD_WAIT_TIMEOUT;
+
 	event_waiters = alloc_event_waiters(num_events);
 	if (!event_waiters) {
 		ret = -ENOMEM;
@@ -744,14 +759,34 @@ int kfd_wait_on_events(struct kfd_process *p,
 			goto fail;
 		}
 
-		ret = init_event_waiter(p, &event_waiters[i],
+		ret = init_event_waiter_get_status(p, &event_waiters[i],
 				event_data.event_id, i);
 		if (ret)
 			goto fail;
 	}
 
+	/* Check condition once. */
+	if (test_event_condition(all, num_events, event_waiters)) {
+		if (copy_signaled_event_data(num_events,
+				event_waiters, events))
+			*wait_result = KFD_WAIT_COMPLETE;
+		else
+			*wait_result = KFD_WAIT_ERROR;
+		free_waiters(num_events, event_waiters);
+	} else {
+		/* Add to wait lists if we need to wait. */
+		for (i = 0; i < num_events; i++)
+			init_event_waiter_add_to_waitlist(&event_waiters[i]);
+	}
+
 	mutex_unlock(&p->event_mutex);
 
+	/* Return if all waits were already satisfied. */
+	if (*wait_result != KFD_WAIT_TIMEOUT) {
+		__set_current_state(TASK_RUNNING);
+		return ret;
+	}
+
 	while (true) {
 		if (fatal_signal_pending(current)) {
 			ret = -EINTR;
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 05/16] drm/amdkfd: Fix scheduler race in kfd_wait_on_events sleep loop

[Felix Kuehling]

From: Sean Keely <sean.keely@amd.com>

Signed-off-by: Sean Keely <sean.keely@amd.com>
Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_events.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
index 64cc42c..c6d9572 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
@@ -806,6 +806,17 @@ int kfd_wait_on_events(struct kfd_process *p,
 			break;
 		}
 
+		/* Set task state to interruptible sleep before
+		 * checking wake-up conditions. A concurrent wake-up
+		 * will put the task back into runnable state. In that
+		 * case schedule_timeout will not put the task to
+		 * sleep and we'll get a chance to re-check the
+		 * updated conditions almost immediately. Otherwise,
+		 * this race condition would lead to a soft hang or a
+		 * very long sleep.
+		 */
+		set_current_state(TASK_INTERRUPTIBLE);
+
 		if (test_event_condition(all, num_events, event_waiters)) {
 			if (copy_signaled_event_data(num_events,
 					event_waiters, events))
@@ -820,7 +831,7 @@ int kfd_wait_on_events(struct kfd_process *p,
 			break;
 		}
 
-		timeout = schedule_timeout_interruptible(timeout);
+		timeout = schedule_timeout(timeout);
 	}
 	__set_current_state(TASK_RUNNING);
 
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 06/16] drm/amdkfd: Clean up kfd_wait_on_events

[Felix Kuehling]

Cleaned up the code while resolving some potential bugs and
inconsistencies in the process.

Clean-ups:
* Remove enum kfd_event_wait_result, which duplicates
  KFD_IOC_EVENT_RESULT definitions
* alloc_event_waiters can be called without holding p->event_mutex
* Return an error code from copy_signaled_event_data instead of bool
* Clean up error handling code paths to minimize duplication in
  kfd_wait_on_events

Fixes:
* Consistently return an error code from kfd_wait_on_events and set
  wait_result to KFD_IOC_WAIT_RESULT_FAIL in all failure cases.
* Always call free_waiters while holding p->event_mutex
* copy_signaled_event_data might sleep. Don't call it while the task state
  is TASK_INTERRUPTIBLE.

Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_chardev.c |  5 +--
 drivers/gpu/drm/amd/amdkfd/kfd_events.c  | 71 ++++++++++++++------------------
 drivers/gpu/drm/amd/amdkfd/kfd_priv.h    |  8 +---
 3 files changed, 32 insertions(+), 52 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
index 0ef82b2..a25321f 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
@@ -835,15 +835,12 @@ static int kfd_ioctl_wait_events(struct file *filp, struct kfd_process *p,
 				void *data)
 {
 	struct kfd_ioctl_wait_events_args *args = data;
-	enum kfd_event_wait_result wait_result;
 	int err;
 
 	err = kfd_wait_on_events(p, args->num_events,
 			(void __user *)args->events_ptr,
 			(args->wait_for_all != 0),
-			args->timeout, &wait_result);
-
-	args->wait_result = wait_result;
+			args->timeout, &args->wait_result);
 
 	return err;
 }
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
index c6d9572..5bb88b74 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
@@ -668,7 +668,7 @@ static bool test_event_condition(bool all, uint32_t num_events,
  * Copy event specific data, if defined.
  * Currently only memory exception events have additional data to copy to user
  */
-static bool copy_signaled_event_data(uint32_t num_events,
+static int copy_signaled_event_data(uint32_t num_events,
 		struct kfd_event_waiter *event_waiters,
 		struct kfd_event_data __user *data)
 {
@@ -686,11 +686,11 @@ static bool copy_signaled_event_data(uint32_t num_events,
 			src = &event->memory_exception_data;
 			if (copy_to_user(dst, src,
 				sizeof(struct kfd_hsa_memory_exception_data)))
-				return false;
+				return -EFAULT;
 		}
 	}
 
-	return true;
+	return 0;
 
 }
 
@@ -727,7 +727,7 @@ static void free_waiters(uint32_t num_events, struct kfd_event_waiter *waiters)
 int kfd_wait_on_events(struct kfd_process *p,
 		       uint32_t num_events, void __user *data,
 		       bool all, uint32_t user_timeout_ms,
-		       enum kfd_event_wait_result *wait_result)
+		       uint32_t *wait_result)
 {
 	struct kfd_event_data __user *events =
 			(struct kfd_event_data __user *) data;
@@ -737,18 +737,18 @@ int kfd_wait_on_events(struct kfd_process *p,
 	struct kfd_event_waiter *event_waiters = NULL;
 	long timeout = user_timeout_to_jiffies(user_timeout_ms);
 
+	event_waiters = alloc_event_waiters(num_events);
+	if (!event_waiters) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
 	mutex_lock(&p->event_mutex);
 
 	/* Set to something unreasonable - this is really
 	 * just a bool for now.
 	 */
-	*wait_result = KFD_WAIT_TIMEOUT;
-
-	event_waiters = alloc_event_waiters(num_events);
-	if (!event_waiters) {
-		ret = -ENOMEM;
-		goto fail;
-	}
+	*wait_result = KFD_IOC_WAIT_RESULT_TIMEOUT;
 
 	for (i = 0; i < num_events; i++) {
 		struct kfd_event_data event_data;
@@ -756,23 +756,21 @@ int kfd_wait_on_events(struct kfd_process *p,
 		if (copy_from_user(&event_data, &events[i],
 				sizeof(struct kfd_event_data))) {
 			ret = -EFAULT;
-			goto fail;
+			goto out_unlock;
 		}
 
 		ret = init_event_waiter_get_status(p, &event_waiters[i],
 				event_data.event_id, i);
 		if (ret)
-			goto fail;
+			goto out_unlock;
 	}
 
 	/* Check condition once. */
 	if (test_event_condition(all, num_events, event_waiters)) {
-		if (copy_signaled_event_data(num_events,
-				event_waiters, events))
-			*wait_result = KFD_WAIT_COMPLETE;
-		else
-			*wait_result = KFD_WAIT_ERROR;
-		free_waiters(num_events, event_waiters);
+		*wait_result = KFD_IOC_WAIT_RESULT_COMPLETE;
+		ret = copy_signaled_event_data(num_events,
+					       event_waiters, events);
+		goto out_unlock;
 	} else {
 		/* Add to wait lists if we need to wait. */
 		for (i = 0; i < num_events; i++)
@@ -781,12 +779,6 @@ int kfd_wait_on_events(struct kfd_process *p,
 
 	mutex_unlock(&p->event_mutex);
 
-	/* Return if all waits were already satisfied. */
-	if (*wait_result != KFD_WAIT_TIMEOUT) {
-		__set_current_state(TASK_RUNNING);
-		return ret;
-	}
-
 	while (true) {
 		if (fatal_signal_pending(current)) {
 			ret = -EINTR;
@@ -818,16 +810,12 @@ int kfd_wait_on_events(struct kfd_process *p,
 		set_current_state(TASK_INTERRUPTIBLE);
 
 		if (test_event_condition(all, num_events, event_waiters)) {
-			if (copy_signaled_event_data(num_events,
-					event_waiters, events))
-				*wait_result = KFD_WAIT_COMPLETE;
-			else
-				*wait_result = KFD_WAIT_ERROR;
+			*wait_result = KFD_IOC_WAIT_RESULT_COMPLETE;
 			break;
 		}
 
 		if (timeout <= 0) {
-			*wait_result = KFD_WAIT_TIMEOUT;
+			*wait_result = KFD_IOC_WAIT_RESULT_TIMEOUT;
 			break;
 		}
 
@@ -835,19 +823,20 @@ int kfd_wait_on_events(struct kfd_process *p,
 	}
 	__set_current_state(TASK_RUNNING);
 
+	/* copy_signaled_event_data may sleep. So this has to happen
+	 * after the task state is set back to RUNNING.
+	 */
+	if (!ret && *wait_result == KFD_IOC_WAIT_RESULT_COMPLETE)
+		ret = copy_signaled_event_data(num_events,
+					       event_waiters, events);
+
 	mutex_lock(&p->event_mutex);
+out_unlock:
 	free_waiters(num_events, event_waiters);
 	mutex_unlock(&p->event_mutex);
-
-	return ret;
-
-fail:
-	if (event_waiters)
-		free_waiters(num_events, event_waiters);
-
-	mutex_unlock(&p->event_mutex);
-
-	*wait_result = KFD_WAIT_ERROR;
+out:
+	if (ret)
+		*wait_result = KFD_IOC_WAIT_RESULT_FAIL;
 
 	return ret;
 }
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
index 1a483a7..d3cf53a 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
@@ -726,19 +726,13 @@ uint64_t kfd_get_number_elems(struct kfd_dev *kfd);
 extern const struct kfd_event_interrupt_class event_interrupt_class_cik;
 extern const struct kfd_device_global_init_class device_global_init_class_cik;
 
-enum kfd_event_wait_result {
-	KFD_WAIT_COMPLETE,
-	KFD_WAIT_TIMEOUT,
-	KFD_WAIT_ERROR
-};
-
 void kfd_event_init_process(struct kfd_process *p);
 void kfd_event_free_process(struct kfd_process *p);
 int kfd_event_mmap(struct kfd_process *process, struct vm_area_struct *vma);
 int kfd_wait_on_events(struct kfd_process *p,
 		       uint32_t num_events, void __user *data,
 		       bool all, uint32_t user_timeout_ms,
-		       enum kfd_event_wait_result *wait_result);
+		       uint32_t *wait_result);
 void kfd_signal_event_interrupt(unsigned int pasid, uint32_t partial_id,
 				uint32_t valid_id_bits);
 void kfd_signal_iommu_event(struct kfd_dev *dev,
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 07/16] drm/amdkfd: Fix event destruction with pending waiters

[Felix Kuehling]

When an event with pending waiters is destroyed, those waiters may
end up sleeping forever unless they are notified and woken up.
Implement the notification by clearing the waiter->event pointer,
which becomes invalid anyway, when the event is freed, and waking
up the waiting tasks.

Waiters on an event that's destroyed return failure.

Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_events.c | 72 +++++++++++++++++++++------------
 1 file changed, 46 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
index 5bb88b74..6050e88 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
@@ -345,18 +345,24 @@ void kfd_event_init_process(struct kfd_process *p)
 
 static void destroy_event(struct kfd_process *p, struct kfd_event *ev)
 {
+	/* Wake up pending waiters. They will return failure */
+	while (!list_empty(&ev->waiters)) {
+		struct kfd_event_waiter *waiter =
+			list_first_entry(&ev->waiters, struct kfd_event_waiter,
+					 waiters);
+
+		waiter->event = NULL;
+		/* _init because free_waiters will call list_del */
+		list_del_init(&waiter->waiters);
+		wake_up_process(waiter->sleeping_task);
+	}
+
 	if (ev->signal_page) {
 		release_event_notification_slot(ev->signal_page,
 						ev->signal_slot_index);
 		p->signal_event_count--;
 	}
 
-	/*
-	 * Abandon the list of waiters. Individual waiting threads will
-	 * clean up their own data.
-	 */
-	list_del(&ev->waiters);
-
 	hash_del(&ev->events);
 	kfree(ev);
 }
@@ -646,22 +652,36 @@ static void init_event_waiter_add_to_waitlist(struct kfd_event_waiter *waiter)
 		list_add(&waiter->waiters, &ev->waiters);
 }
 
-static bool test_event_condition(bool all, uint32_t num_events,
+/* test_event_condition - Test condition of events being waited for
+ * @all:           Return completion only if all events have signaled
+ * @num_events:    Number of events to wait for
+ * @event_waiters: Array of event waiters, one per event
+ *
+ * Returns KFD_IOC_WAIT_RESULT_COMPLETE if all (or one) event(s) have
+ * signaled. Returns KFD_IOC_WAIT_RESULT_TIMEOUT if no (or not all)
+ * events have signaled. Returns KFD_IOC_WAIT_RESULT_FAIL if any of
+ * the events have been destroyed.
+ */
+static uint32_t test_event_condition(bool all, uint32_t num_events,
 				struct kfd_event_waiter *event_waiters)
 {
 	uint32_t i;
 	uint32_t activated_count = 0;
 
 	for (i = 0; i < num_events; i++) {
+		if (!event_waiters[i].event)
+			return KFD_IOC_WAIT_RESULT_FAIL;
+
 		if (event_waiters[i].activated) {
 			if (!all)
-				return true;
+				return KFD_IOC_WAIT_RESULT_COMPLETE;
 
 			activated_count++;
 		}
 	}
 
-	return activated_count == num_events;
+	return activated_count == num_events ?
+		KFD_IOC_WAIT_RESULT_COMPLETE : KFD_IOC_WAIT_RESULT_TIMEOUT;
 }
 
 /*
@@ -745,11 +765,6 @@ int kfd_wait_on_events(struct kfd_process *p,
 
 	mutex_lock(&p->event_mutex);
 
-	/* Set to something unreasonable - this is really
-	 * just a bool for now.
-	 */
-	*wait_result = KFD_IOC_WAIT_RESULT_TIMEOUT;
-
 	for (i = 0; i < num_events; i++) {
 		struct kfd_event_data event_data;
 
@@ -766,17 +781,22 @@ int kfd_wait_on_events(struct kfd_process *p,
 	}
 
 	/* Check condition once. */
-	if (test_event_condition(all, num_events, event_waiters)) {
-		*wait_result = KFD_IOC_WAIT_RESULT_COMPLETE;
+	*wait_result = test_event_condition(all, num_events, event_waiters);
+	if (*wait_result == KFD_IOC_WAIT_RESULT_COMPLETE) {
 		ret = copy_signaled_event_data(num_events,
 					       event_waiters, events);
 		goto out_unlock;
-	} else {
-		/* Add to wait lists if we need to wait. */
-		for (i = 0; i < num_events; i++)
-			init_event_waiter_add_to_waitlist(&event_waiters[i]);
+	} else if (WARN_ON(*wait_result == KFD_IOC_WAIT_RESULT_FAIL)) {
+		/* This should not happen. Events shouldn't be
+		 * destroyed while we're holding the event_mutex
+		 */
+		goto out_unlock;
 	}
 
+	/* Add to wait lists if we need to wait. */
+	for (i = 0; i < num_events; i++)
+		init_event_waiter_add_to_waitlist(&event_waiters[i]);
+
 	mutex_unlock(&p->event_mutex);
 
 	while (true) {
@@ -809,15 +829,13 @@ int kfd_wait_on_events(struct kfd_process *p,
 		 */
 		set_current_state(TASK_INTERRUPTIBLE);
 
-		if (test_event_condition(all, num_events, event_waiters)) {
-			*wait_result = KFD_IOC_WAIT_RESULT_COMPLETE;
+		*wait_result = test_event_condition(all, num_events,
+						    event_waiters);
+		if (*wait_result != KFD_IOC_WAIT_RESULT_TIMEOUT)
 			break;
-		}
 
-		if (timeout <= 0) {
-			*wait_result = KFD_IOC_WAIT_RESULT_TIMEOUT;
+		if (timeout <= 0)
 			break;
-		}
 
 		timeout = schedule_timeout(timeout);
 	}
@@ -837,6 +855,8 @@ int kfd_wait_on_events(struct kfd_process *p,
 out:
 	if (ret)
 		*wait_result = KFD_IOC_WAIT_RESULT_FAIL;
+	else if (*wait_result == KFD_IOC_WAIT_RESULT_FAIL)
+		ret = -EIO;
 
 	return ret;
 }
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 08/16] drm/amdkfd: remove redundant kfd_event_waiter.input_index

[Felix Kuehling]

This always identical with the index of the event_waiter in the array.
No need to store it in the waiter record.

Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_events.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
index 6050e88..949b80a 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
@@ -49,7 +49,6 @@ struct kfd_event_waiter {
 
 	/* Event */
 	struct kfd_event *event;
-	uint32_t input_index;
 };
 
 /*
@@ -625,8 +624,7 @@ static struct kfd_event_waiter *alloc_event_waiters(uint32_t num_events)
 
 static int init_event_waiter_get_status(struct kfd_process *p,
 		struct kfd_event_waiter *waiter,
-		uint32_t event_id,
-		uint32_t input_index)
+		uint32_t event_id)
 {
 	struct kfd_event *ev = lookup_event_by_id(p, event_id);
 
@@ -634,7 +632,6 @@ static int init_event_waiter_get_status(struct kfd_process *p,
 		return -EINVAL;
 
 	waiter->event = ev;
-	waiter->input_index = input_index;
 	waiter->activated = ev->signaled;
 	ev->signaled = ev->signaled && !ev->auto_reset;
 
@@ -702,7 +699,7 @@ static int copy_signaled_event_data(uint32_t num_events,
 		waiter = &event_waiters[i];
 		event = waiter->event;
 		if (waiter->activated && event->type == KFD_EVENT_TYPE_MEMORY) {
-			dst = &data[waiter->input_index].memory_exception_data;
+			dst = &data[i].memory_exception_data;
 			src = &event->memory_exception_data;
 			if (copy_to_user(dst, src,
 				sizeof(struct kfd_hsa_memory_exception_data)))
@@ -775,7 +772,7 @@ int kfd_wait_on_events(struct kfd_process *p,
 		}
 
 		ret = init_event_waiter_get_status(p, &event_waiters[i],
-				event_data.event_id, i);
+				event_data.event_id);
 		if (ret)
 			goto out_unlock;
 	}
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 09/16] drm/amdkfd: Use wait_queue_t to implement event waiting

[Felix Kuehling]

Signed-off-by: Kent Russell <kent.russell@amd.com>
Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_events.c | 53 +++++++++++----------------------
 drivers/gpu/drm/amd/amdkfd/kfd_events.h |  3 +-
 2 files changed, 19 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
index 949b80a..f178248 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
@@ -33,22 +33,12 @@
 #include <linux/device.h>
 
 /*
- * A task can only be on a single wait_queue at a time, but we need to support
- * waiting on multiple events (any/all).
- * Instead of each event simply having a wait_queue with sleeping tasks, it
- * has a singly-linked list of tasks.
- * A thread that wants to sleep creates an array of these, one for each event
- * and adds one to each event's waiter chain.
+ * Wrapper around wait_queue_entry_t
  */
 struct kfd_event_waiter {
-	struct list_head waiters;
-	struct task_struct *sleeping_task;
-
-	/* Transitions to true when the event this belongs to is signaled. */
-	bool activated;
-
-	/* Event */
-	struct kfd_event *event;
+	wait_queue_entry_t wait;
+	struct kfd_event *event; /* Event to wait for */
+	bool activated;		 /* Becomes true when event is signaled */
 };
 
 /*
@@ -344,17 +334,12 @@ void kfd_event_init_process(struct kfd_process *p)
 
 static void destroy_event(struct kfd_process *p, struct kfd_event *ev)
 {
-	/* Wake up pending waiters. They will return failure */
-	while (!list_empty(&ev->waiters)) {
-		struct kfd_event_waiter *waiter =
-			list_first_entry(&ev->waiters, struct kfd_event_waiter,
-					 waiters);
+	struct kfd_event_waiter *waiter;
 
+	/* Wake up pending waiters. They will return failure */
+	list_for_each_entry(waiter, &ev->wq.head, wait.entry)
 		waiter->event = NULL;
-		/* _init because free_waiters will call list_del */
-		list_del_init(&waiter->waiters);
-		wake_up_process(waiter->sleeping_task);
-	}
+	wake_up_all(&ev->wq);
 
 	if (ev->signal_page) {
 		release_event_notification_slot(ev->signal_page,
@@ -424,7 +409,7 @@ int kfd_event_create(struct file *devkfd, struct kfd_process *p,
 	ev->auto_reset = auto_reset;
 	ev->signaled = false;
 
-	INIT_LIST_HEAD(&ev->waiters);
+	init_waitqueue_head(&ev->wq);
 
 	*event_page_offset = 0;
 
@@ -482,19 +467,14 @@ int kfd_event_destroy(struct kfd_process *p, uint32_t event_id)
 static void set_event(struct kfd_event *ev)
 {
 	struct kfd_event_waiter *waiter;
-	struct kfd_event_waiter *next;
 
 	/* Auto reset if the list is non-empty and we're waking someone. */
-	ev->signaled = !ev->auto_reset || list_empty(&ev->waiters);
+	ev->signaled = !ev->auto_reset || !waitqueue_active(&ev->wq);
 
-	list_for_each_entry_safe(waiter, next, &ev->waiters, waiters) {
+	list_for_each_entry(waiter, &ev->wq.head, wait.entry)
 		waiter->activated = true;
 
-		/* _init because free_waiters will call list_del */
-		list_del_init(&waiter->waiters);
-
-		wake_up_process(waiter->sleeping_task);
-	}
+	wake_up_all(&ev->wq);
 }
 
 /* Assumes that p is current. */
@@ -614,8 +594,7 @@ static struct kfd_event_waiter *alloc_event_waiters(uint32_t num_events)
 					GFP_KERNEL);
 
 	for (i = 0; (event_waiters) && (i < num_events) ; i++) {
-		INIT_LIST_HEAD(&event_waiters[i].waiters);
-		event_waiters[i].sleeping_task = current;
+		init_wait(&event_waiters[i].wait);
 		event_waiters[i].activated = false;
 	}
 
@@ -646,7 +625,7 @@ static void init_event_waiter_add_to_waitlist(struct kfd_event_waiter *waiter)
 	 * wait on this event.
 	 */
 	if (!waiter->activated)
-		list_add(&waiter->waiters, &ev->waiters);
+		add_wait_queue(&ev->wq, &waiter->wait);
 }
 
 /* test_event_condition - Test condition of events being waited for
@@ -736,7 +715,9 @@ static void free_waiters(uint32_t num_events, struct kfd_event_waiter *waiters)
 	uint32_t i;
 
 	for (i = 0; i < num_events; i++)
-		list_del(&waiters[i].waiters);
+		if (waiters[i].event)
+			remove_wait_queue(&waiters[i].event->wq,
+					  &waiters[i].wait);
 
 	kfree(waiters);
 }
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.h b/drivers/gpu/drm/amd/amdkfd/kfd_events.h
index 28f6838..96f9122 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_events.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.h
@@ -27,6 +27,7 @@
 #include <linux/hashtable.h>
 #include <linux/types.h>
 #include <linux/list.h>
+#include <linux/wait.h>
 #include "kfd_priv.h"
 #include <uapi/linux/kfd_ioctl.h>
 
@@ -56,7 +57,7 @@ struct kfd_event {
 
 	int type;
 
-	struct list_head waiters; /* List of kfd_event_waiter by waiters. */
+	wait_queue_head_t wq; /* List of event waiters. */
 
 	/* Only for signal events. */
 	struct signal_page *signal_page;
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 10/16] drm/amdkfd: Simplify events page allocator

[Felix Kuehling]

The first event page is always big enough to handle all events.
Handling of multiple events pages is not supported by user mode, and
not necessary.

Signed-off-by: Yong Zhao <yong.zhao@amd.com>
Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_events.c | 197 +++++++++++---------------------
 drivers/gpu/drm/amd/amdkfd/kfd_events.h |   1 -
 drivers/gpu/drm/amd/amdkfd/kfd_priv.h   |   4 +-
 3 files changed, 70 insertions(+), 132 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
index f178248..f800e48 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
@@ -41,6 +41,9 @@ struct kfd_event_waiter {
 	bool activated;		 /* Becomes true when event is signaled */
 };
 
+#define SLOTS_PER_PAGE KFD_SIGNAL_EVENT_LIMIT
+#define SLOT_BITMAP_LONGS BITS_TO_LONGS(SLOTS_PER_PAGE)
+
 /*
  * Over-complicated pooled allocator for event notification slots.
  *
@@ -51,132 +54,98 @@ struct kfd_event_waiter {
  * Individual signal events are then allocated a slot in a page.
  */
 
-struct signal_page {
-	struct list_head event_pages;	/* kfd_process.signal_event_pages */
+struct kfd_signal_page {
 	uint64_t *kernel_address;
 	uint64_t __user *user_address;
-	uint32_t page_index;		/* Index into the mmap aperture. */
 	unsigned int free_slots;
-	unsigned long used_slot_bitmap[0];
+	unsigned long used_slot_bitmap[SLOT_BITMAP_LONGS];
 };
 
-#define SLOTS_PER_PAGE KFD_SIGNAL_EVENT_LIMIT
-#define SLOT_BITMAP_SIZE BITS_TO_LONGS(SLOTS_PER_PAGE)
-#define BITS_PER_PAGE (ilog2(SLOTS_PER_PAGE)+1)
-#define SIGNAL_PAGE_SIZE (sizeof(struct signal_page) + \
-				SLOT_BITMAP_SIZE * sizeof(long))
-
 /*
  * For signal events, the event ID is used as the interrupt user data.
  * For SQ s_sendmsg interrupts, this is limited to 8 bits.
  */
 
 #define INTERRUPT_DATA_BITS 12
-#define SIGNAL_EVENT_ID_SLOT_SHIFT 0
 
-static uint64_t *page_slots(struct signal_page *page)
+static uint64_t *page_slots(struct kfd_signal_page *page)
 {
 	return page->kernel_address;
 }
 
 static bool allocate_free_slot(struct kfd_process *process,
-				struct signal_page **out_page,
-				unsigned int *out_slot_index)
+			       unsigned int *out_slot_index)
 {
-	struct signal_page *page;
+	struct kfd_signal_page *page = process->signal_page;
+	unsigned int slot;
 
-	list_for_each_entry(page, &process->signal_event_pages, event_pages) {
-		if (page->free_slots > 0) {
-			unsigned int slot =
-				find_first_zero_bit(page->used_slot_bitmap,
-							SLOTS_PER_PAGE);
+	if (!page || page->free_slots == 0) {
+		pr_debug("No free event signal slots were found for process %p\n",
+			 process);
 
-			__set_bit(slot, page->used_slot_bitmap);
-			page->free_slots--;
+		return false;
+	}
 
-			page_slots(page)[slot] = UNSIGNALED_EVENT_SLOT;
+	slot = find_first_zero_bit(page->used_slot_bitmap, SLOTS_PER_PAGE);
 
-			*out_page = page;
-			*out_slot_index = slot;
+	__set_bit(slot, page->used_slot_bitmap);
+	page->free_slots--;
 
-			pr_debug("Allocated event signal slot in page %p, slot %d\n",
-					page, slot);
+	page_slots(page)[slot] = UNSIGNALED_EVENT_SLOT;
 
-			return true;
-		}
-	}
+	*out_slot_index = slot;
 
-	pr_debug("No free event signal slots were found for process %p\n",
-			process);
+	pr_debug("Allocated event signal slot in page %p, slot %d\n",
+		 page, slot);
 
-	return false;
+	return true;
 }
 
-#define list_tail_entry(head, type, member) \
-	list_entry((head)->prev, type, member)
-
-static bool allocate_signal_page(struct file *devkfd, struct kfd_process *p)
+static struct kfd_signal_page *allocate_signal_page(struct kfd_process *p)
 {
 	void *backing_store;
-	struct signal_page *page;
+	struct kfd_signal_page *page;
 
-	page = kzalloc(SIGNAL_PAGE_SIZE, GFP_KERNEL);
+	page = kzalloc(sizeof(*page), GFP_KERNEL);
 	if (!page)
-		goto fail_alloc_signal_page;
+		return NULL;
 
 	page->free_slots = SLOTS_PER_PAGE;
 
-	backing_store = (void *) __get_free_pages(GFP_KERNEL | __GFP_ZERO,
+	backing_store = (void *) __get_free_pages(GFP_KERNEL,
 					get_order(KFD_SIGNAL_EVENT_LIMIT * 8));
 	if (!backing_store)
 		goto fail_alloc_signal_store;
 
-	/* prevent user-mode info leaks */
+	/* Initialize all events to unsignaled */
 	memset(backing_store, (uint8_t) UNSIGNALED_EVENT_SLOT,
-		KFD_SIGNAL_EVENT_LIMIT * 8);
+	       KFD_SIGNAL_EVENT_LIMIT * 8);
 
 	page->kernel_address = backing_store;
-
-	if (list_empty(&p->signal_event_pages))
-		page->page_index = 0;
-	else
-		page->page_index = list_tail_entry(&p->signal_event_pages,
-						   struct signal_page,
-						   event_pages)->page_index + 1;
-
 	pr_debug("Allocated new event signal page at %p, for process %p\n",
 			page, p);
-	pr_debug("Page index is %d\n", page->page_index);
-
-	list_add(&page->event_pages, &p->signal_event_pages);
 
-	return true;
+	return page;
 
 fail_alloc_signal_store:
 	kfree(page);
-fail_alloc_signal_page:
-	return false;
+	return NULL;
 }
 
-static bool allocate_event_notification_slot(struct file *devkfd,
-					struct kfd_process *p,
-					struct signal_page **page,
-					unsigned int *signal_slot_index)
+static bool allocate_event_notification_slot(struct kfd_process *p,
+					     unsigned int *signal_slot_index)
 {
-	bool ret;
-
-	ret = allocate_free_slot(p, page, signal_slot_index);
-	if (!ret) {
-		ret = allocate_signal_page(devkfd, p);
-		if (ret)
-			ret = allocate_free_slot(p, page, signal_slot_index);
+	if (!p->signal_page) {
+		p->signal_page = allocate_signal_page(p);
+		if (!p->signal_page)
+			return false;
 	}
 
-	return ret;
+	return allocate_free_slot(p, signal_slot_index);
 }
 
 /* Assumes that the process's event_mutex is locked. */
-static void release_event_notification_slot(struct signal_page *page,
+static void release_event_notification_slot(struct kfd_signal_page *page,
 						size_t slot_index)
 {
 	__clear_bit(slot_index, page->used_slot_bitmap);
@@ -187,22 +156,6 @@ static void release_event_notification_slot(struct signal_page *page,
 	 */
 }
 
-static struct signal_page *lookup_signal_page_by_index(struct kfd_process *p,
-						unsigned int page_index)
-{
-	struct signal_page *page;
-
-	/*
-	 * This is safe because we don't delete signal pages until the
-	 * process exits.
-	 */
-	list_for_each_entry(page, &p->signal_event_pages, event_pages)
-		if (page->page_index == page_index)
-			return page;
-
-	return NULL;
-}
-
 /*
  * Assumes that p->event_mutex is held and of course that p is not going
  * away (current or locked).
@@ -218,13 +171,6 @@ static struct kfd_event *lookup_event_by_id(struct kfd_process *p, uint32_t id)
 	return NULL;
 }
 
-static u32 make_signal_event_id(struct signal_page *page,
-					 unsigned int signal_slot_index)
-{
-	return page->page_index |
-			(signal_slot_index << SIGNAL_EVENT_ID_SLOT_SHIFT);
-}
-
 /*
  * Produce a kfd event id for a nonsignal event.
  * These are arbitrary numbers, so we do a sequential search through
@@ -270,10 +216,9 @@ static u32 make_nonsignal_event_id(struct kfd_process *p)
 }
 
 static struct kfd_event *lookup_event_by_page_slot(struct kfd_process *p,
-						struct signal_page *page,
 						unsigned int signal_slot)
 {
-	return lookup_event_by_id(p, make_signal_event_id(page, signal_slot));
+	return lookup_event_by_id(p, signal_slot);
 }
 
 static int create_signal_event(struct file *devkfd,
@@ -288,8 +233,7 @@ static int create_signal_event(struct file *devkfd,
 		return -ENOMEM;
 	}
 
-	if (!allocate_event_notification_slot(devkfd, p, &ev->signal_page,
-						&ev->signal_slot_index)) {
+	if (!allocate_event_notification_slot(p, &ev->signal_slot_index)) {
 		pr_warn("Signal event wasn't created because out of kernel memory\n");
 		return -ENOMEM;
 	}
@@ -297,10 +241,9 @@ static int create_signal_event(struct file *devkfd,
 	p->signal_event_count++;
 
 	ev->user_signal_address =
-			&ev->signal_page->user_address[ev->signal_slot_index];
+			&p->signal_page->user_address[ev->signal_slot_index];
 
-	ev->event_id = make_signal_event_id(ev->signal_page,
-						ev->signal_slot_index);
+	ev->event_id = ev->signal_slot_index;
 
 	pr_debug("Signal event number %zu created with id %d, address %p\n",
 			p->signal_event_count, ev->event_id,
@@ -327,7 +270,7 @@ void kfd_event_init_process(struct kfd_process *p)
 {
 	mutex_init(&p->event_mutex);
 	hash_init(p->events);
-	INIT_LIST_HEAD(&p->signal_event_pages);
+	p->signal_page = NULL;
 	p->next_nonsignal_event_id = KFD_FIRST_NONSIGNAL_EVENT_ID;
 	p->signal_event_count = 0;
 }
@@ -341,8 +284,9 @@ static void destroy_event(struct kfd_process *p, struct kfd_event *ev)
 		waiter->event = NULL;
 	wake_up_all(&ev->wq);
 
-	if (ev->signal_page) {
-		release_event_notification_slot(ev->signal_page,
+	if ((ev->type == KFD_EVENT_TYPE_SIGNAL ||
+	     ev->type == KFD_EVENT_TYPE_DEBUG) && p->signal_page) {
+		release_event_notification_slot(p->signal_page,
 						ev->signal_slot_index);
 		p->signal_event_count--;
 	}
@@ -365,12 +309,11 @@ static void destroy_events(struct kfd_process *p)
  * We assume that the process is being destroyed and there is no need to
  * unmap the pages or keep bookkeeping data in order.
  */
-static void shutdown_signal_pages(struct kfd_process *p)
+static void shutdown_signal_page(struct kfd_process *p)
 {
-	struct signal_page *page, *tmp;
+	struct kfd_signal_page *page = p->signal_page;
 
-	list_for_each_entry_safe(page, tmp, &p->signal_event_pages,
-					event_pages) {
+	if (page) {
 		free_pages((unsigned long)page->kernel_address,
 				get_order(KFD_SIGNAL_EVENT_LIMIT * 8));
 		kfree(page);
@@ -380,7 +323,7 @@ static void shutdown_signal_pages(struct kfd_process *p)
 void kfd_event_free_process(struct kfd_process *p)
 {
 	destroy_events(p);
-	shutdown_signal_pages(p);
+	shutdown_signal_page(p);
 }
 
 static bool event_can_be_gpu_signaled(const struct kfd_event *ev)
@@ -420,8 +363,7 @@ int kfd_event_create(struct file *devkfd, struct kfd_process *p,
 	case KFD_EVENT_TYPE_DEBUG:
 		ret = create_signal_event(devkfd, p, ev);
 		if (!ret) {
-			*event_page_offset = (ev->signal_page->page_index |
-					KFD_MMAP_EVENTS_MASK);
+			*event_page_offset = KFD_MMAP_EVENTS_MASK;
 			*event_page_offset <<= PAGE_SHIFT;
 			*event_slot_index = ev->signal_slot_index;
 		}
@@ -523,13 +465,17 @@ int kfd_reset_event(struct kfd_process *p, uint32_t event_id)
 
 static void acknowledge_signal(struct kfd_process *p, struct kfd_event *ev)
 {
-	page_slots(ev->signal_page)[ev->signal_slot_index] =
+	page_slots(p->signal_page)[ev->signal_slot_index] =
 						UNSIGNALED_EVENT_SLOT;
 }
 
-static bool is_slot_signaled(struct signal_page *page, unsigned int index)
+static bool is_slot_signaled(struct kfd_process *p, unsigned int index)
 {
-	return page_slots(page)[index] != UNSIGNALED_EVENT_SLOT;
+	if (!p->signal_page)
+		return false;
+	else
+		return page_slots(p->signal_page)[index] !=
+			UNSIGNALED_EVENT_SLOT;
 }
 
 static void set_event_from_interrupt(struct kfd_process *p,
@@ -562,22 +508,19 @@ void kfd_signal_event_interrupt(unsigned int pasid, uint32_t partial_id,
 		/* Partial ID is a full ID. */
 		ev = lookup_event_by_id(p, partial_id);
 		set_event_from_interrupt(p, ev);
-	} else {
+	} else if (p->signal_page) {
 		/*
 		 * Partial ID is in fact partial. For now we completely
 		 * ignore it, but we could use any bits we did receive to
 		 * search faster.
 		 */
-		struct signal_page *page;
 		unsigned int i;
 
-		list_for_each_entry(page, &p->signal_event_pages, event_pages)
-			for (i = 0; i < SLOTS_PER_PAGE; i++)
-				if (is_slot_signaled(page, i)) {
-					ev = lookup_event_by_page_slot(p,
-								page, i);
-					set_event_from_interrupt(p, ev);
-				}
+		for (i = 0; i < SLOTS_PER_PAGE; i++)
+			if (is_slot_signaled(p, i)) {
+				ev = lookup_event_by_page_slot(p, i);
+				set_event_from_interrupt(p, ev);
+			}
 	}
 
 	mutex_unlock(&p->event_mutex);
@@ -842,9 +785,8 @@ int kfd_wait_on_events(struct kfd_process *p,
 int kfd_event_mmap(struct kfd_process *p, struct vm_area_struct *vma)
 {
 
-	unsigned int page_index;
 	unsigned long pfn;
-	struct signal_page *page;
+	struct kfd_signal_page *page;
 
 	/* check required size is logical */
 	if (get_order(KFD_SIGNAL_EVENT_LIMIT * 8) !=
@@ -853,13 +795,10 @@ int kfd_event_mmap(struct kfd_process *p, struct vm_area_struct *vma)
 		return -EINVAL;
 	}
 
-	page_index = vma->vm_pgoff;
-
-	page = lookup_signal_page_by_index(p, page_index);
+	page = p->signal_page;
 	if (!page) {
 		/* Probably KFD bug, but mmap is user-accessible. */
-		pr_debug("Signal page could not be found for page_index %u\n",
-				page_index);
+		pr_debug("Signal page could not be found\n");
 		return -EINVAL;
 	}
 
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.h b/drivers/gpu/drm/amd/amdkfd/kfd_events.h
index 96f9122..f85fcee 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_events.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.h
@@ -60,7 +60,6 @@ struct kfd_event {
 	wait_queue_head_t wq; /* List of event waiters. */
 
 	/* Only for signal events. */
-	struct signal_page *signal_page;
 	unsigned int signal_slot_index;
 	uint64_t __user *user_signal_address;
 
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
index d3cf53a..c1b3ee2 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
@@ -540,8 +540,8 @@ struct kfd_process {
 	struct mutex event_mutex;
 	/* All events in process hashed by ID, linked on kfd_event.events. */
 	DECLARE_HASHTABLE(events, 4);
-	/* struct slot_page_header.event_pages */
-	struct list_head signal_event_pages;
+	/* Event page */
+	struct kfd_signal_page *signal_page;
 	u32 next_nonsignal_event_id;
 	size_t signal_event_count;
 	bool signal_event_limit_reached;
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 11/16] drm/amdkfd: Simplify event ID and signal slot management

[Felix Kuehling]

Signal slots are identical to event IDs.

Replace the used_slot_bitmap and events hash table with an IDR to
allocate and lookup event IDs and signal slots more efficiently.

Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_events.c | 230 ++++++++++----------------------
 drivers/gpu/drm/amd/amdkfd/kfd_events.h |  14 +-
 drivers/gpu/drm/amd/amdkfd/kfd_priv.h   |   6 +-
 3 files changed, 80 insertions(+), 170 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
index f800e48..cddd4b9 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
@@ -41,24 +41,16 @@ struct kfd_event_waiter {
 	bool activated;		 /* Becomes true when event is signaled */
 };
 
-#define SLOTS_PER_PAGE KFD_SIGNAL_EVENT_LIMIT
-#define SLOT_BITMAP_LONGS BITS_TO_LONGS(SLOTS_PER_PAGE)
-
 /*
- * Over-complicated pooled allocator for event notification slots.
- *
  * Each signal event needs a 64-bit signal slot where the signaler will write
- * a 1 before sending an interrupt.l (This is needed because some interrupts
+ * a 1 before sending an interrupt. (This is needed because some interrupts
  * do not contain enough spare data bits to identify an event.)
- * We get whole pages from vmalloc and map them to the process VA.
- * Individual signal events are then allocated a slot in a page.
+ * We get whole pages and map them to the process VA.
+ * Individual signal events use their event_id as slot index.
  */
-
 struct kfd_signal_page {
 	uint64_t *kernel_address;
 	uint64_t __user *user_address;
-	unsigned int free_slots;
-	unsigned long used_slot_bitmap[SLOT_BITMAP_LONGS];
 };
 
 /*
@@ -73,34 +65,6 @@ static uint64_t *page_slots(struct kfd_signal_page *page)
 	return page->kernel_address;
 }
 
-static bool allocate_free_slot(struct kfd_process *process,
-			       unsigned int *out_slot_index)
-{
-	struct kfd_signal_page *page = process->signal_page;
-	unsigned int slot;
-
-	if (!page || page->free_slots == 0) {
-		pr_debug("No free event signal slots were found for process %p\n",
-			 process);
-
-		return false;
-	}
-
-	slot = find_first_zero_bit(page->used_slot_bitmap, SLOTS_PER_PAGE);
-
-	__set_bit(slot, page->used_slot_bitmap);
-	page->free_slots--;
-
-	page_slots(page)[slot] = UNSIGNALED_EVENT_SLOT;
-
-	*out_slot_index = slot;
-
-	pr_debug("Allocated event signal slot in page %p, slot %d\n",
-		 page, slot);
-
-	return true;
-}
-
 static struct kfd_signal_page *allocate_signal_page(struct kfd_process *p)
 {
 	void *backing_store;
@@ -110,8 +74,6 @@ static struct kfd_signal_page *allocate_signal_page(struct kfd_process *p)
 	if (!page)
 		return NULL;
 
-	page->free_slots = SLOTS_PER_PAGE;
-
 	backing_store = (void *) __get_free_pages(GFP_KERNEL,
 					get_order(KFD_SIGNAL_EVENT_LIMIT * 8));
 	if (!backing_store)
@@ -132,28 +94,26 @@ static struct kfd_signal_page *allocate_signal_page(struct kfd_process *p)
 	return NULL;
 }
 
-static bool allocate_event_notification_slot(struct kfd_process *p,
-					     unsigned int *signal_slot_index)
+static int allocate_event_notification_slot(struct kfd_process *p,
+					    struct kfd_event *ev)
 {
+	int id;
+
 	if (!p->signal_page) {
 		p->signal_page = allocate_signal_page(p);
 		if (!p->signal_page)
-			return false;
+			return -ENOMEM;
 	}
 
-	return allocate_free_slot(p, signal_slot_index);
-}
+	id = idr_alloc(&p->event_idr, ev, 0, KFD_SIGNAL_EVENT_LIMIT,
+		       GFP_KERNEL);
+	if (id < 0)
+		return id;
 
-/* Assumes that the process's event_mutex is locked. */
-static void release_event_notification_slot(struct kfd_signal_page *page,
-						size_t slot_index)
-{
-	__clear_bit(slot_index, page->used_slot_bitmap);
-	page->free_slots++;
+	ev->event_id = id;
+	page_slots(p->signal_page)[id] = UNSIGNALED_EVENT_SLOT;
 
-	/* We don't free signal pages, they are retained by the process
-	 * and reused until it exits.
-	 */
+	return 0;
 }
 
 /*
@@ -162,89 +122,32 @@ static void release_event_notification_slot(struct kfd_signal_page *page,
  */
 static struct kfd_event *lookup_event_by_id(struct kfd_process *p, uint32_t id)
 {
-	struct kfd_event *ev;
-
-	hash_for_each_possible(p->events, ev, events, id)
-		if (ev->event_id == id)
-			return ev;
-
-	return NULL;
-}
-
-/*
- * Produce a kfd event id for a nonsignal event.
- * These are arbitrary numbers, so we do a sequential search through
- * the hash table for an unused number.
- */
-static u32 make_nonsignal_event_id(struct kfd_process *p)
-{
-	u32 id;
-
-	for (id = p->next_nonsignal_event_id;
-		id < KFD_LAST_NONSIGNAL_EVENT_ID &&
-		lookup_event_by_id(p, id);
-		id++)
-		;
-
-	if (id < KFD_LAST_NONSIGNAL_EVENT_ID) {
-
-		/*
-		 * What if id == LAST_NONSIGNAL_EVENT_ID - 1?
-		 * Then next_nonsignal_event_id = LAST_NONSIGNAL_EVENT_ID so
-		 * the first loop fails immediately and we proceed with the
-		 * wraparound loop below.
-		 */
-		p->next_nonsignal_event_id = id + 1;
-
-		return id;
-	}
-
-	for (id = KFD_FIRST_NONSIGNAL_EVENT_ID;
-		id < KFD_LAST_NONSIGNAL_EVENT_ID &&
-		lookup_event_by_id(p, id);
-		id++)
-		;
-
-
-	if (id < KFD_LAST_NONSIGNAL_EVENT_ID) {
-		p->next_nonsignal_event_id = id + 1;
-		return id;
-	}
-
-	p->next_nonsignal_event_id = KFD_FIRST_NONSIGNAL_EVENT_ID;
-	return 0;
-}
-
-static struct kfd_event *lookup_event_by_page_slot(struct kfd_process *p,
-						unsigned int signal_slot)
-{
-	return lookup_event_by_id(p, signal_slot);
+	return idr_find(&p->event_idr, id);
 }
 
 static int create_signal_event(struct file *devkfd,
 				struct kfd_process *p,
 				struct kfd_event *ev)
 {
+	int ret;
+
 	if (p->signal_event_count == KFD_SIGNAL_EVENT_LIMIT) {
 		if (!p->signal_event_limit_reached) {
 			pr_warn("Signal event wasn't created because limit was reached\n");
 			p->signal_event_limit_reached = true;
 		}
-		return -ENOMEM;
+		return -ENOSPC;
 	}
 
-	if (!allocate_event_notification_slot(p, &ev->signal_slot_index)) {
+	ret = allocate_event_notification_slot(p, ev);
+	if (ret) {
 		pr_warn("Signal event wasn't created because out of kernel memory\n");
-		return -ENOMEM;
+		return ret;
 	}
 
 	p->signal_event_count++;
 
-	ev->user_signal_address =
-			&p->signal_page->user_address[ev->signal_slot_index];
-
-	ev->event_id = ev->signal_slot_index;
-
+	ev->user_signal_address = &p->signal_page->user_address[ev->event_id];
 	pr_debug("Signal event number %zu created with id %d, address %p\n",
 			p->signal_event_count, ev->event_id,
 			ev->user_signal_address);
@@ -252,16 +155,20 @@ static int create_signal_event(struct file *devkfd,
 	return 0;
 }
 
-/*
- * No non-signal events are supported yet.
- * We create them as events that never signal.
- * Set event calls from user-mode are failed.
- */
 static int create_other_event(struct kfd_process *p, struct kfd_event *ev)
 {
-	ev->event_id = make_nonsignal_event_id(p);
-	if (ev->event_id == 0)
-		return -ENOMEM;
+	/* Cast KFD_LAST_NONSIGNAL_EVENT to uint32_t. This allows an
+	 * intentional integer overflow to -1 without a compiler
+	 * warning. idr_alloc treats a negative value as "maximum
+	 * signed integer".
+	 */
+	int id = idr_alloc(&p->event_idr, ev, KFD_FIRST_NONSIGNAL_EVENT_ID,
+			   (uint32_t)KFD_LAST_NONSIGNAL_EVENT_ID + 1,
+			   GFP_KERNEL);
+
+	if (id < 0)
+		return id;
+	ev->event_id = id;
 
 	return 0;
 }
@@ -269,9 +176,8 @@ static int create_other_event(struct kfd_process *p, struct kfd_event *ev)
 void kfd_event_init_process(struct kfd_process *p)
 {
 	mutex_init(&p->event_mutex);
-	hash_init(p->events);
+	idr_init(&p->event_idr);
 	p->signal_page = NULL;
-	p->next_nonsignal_event_id = KFD_FIRST_NONSIGNAL_EVENT_ID;
 	p->signal_event_count = 0;
 }
 
@@ -284,25 +190,22 @@ static void destroy_event(struct kfd_process *p, struct kfd_event *ev)
 		waiter->event = NULL;
 	wake_up_all(&ev->wq);
 
-	if ((ev->type == KFD_EVENT_TYPE_SIGNAL ||
-	     ev->type == KFD_EVENT_TYPE_DEBUG) && p->signal_page) {
-		release_event_notification_slot(p->signal_page,
-						ev->signal_slot_index);
+	if (ev->type == KFD_EVENT_TYPE_SIGNAL ||
+	    ev->type == KFD_EVENT_TYPE_DEBUG)
 		p->signal_event_count--;
-	}
 
-	hash_del(&ev->events);
+	idr_remove(&p->event_idr, ev->event_id);
 	kfree(ev);
 }
 
 static void destroy_events(struct kfd_process *p)
 {
 	struct kfd_event *ev;
-	struct hlist_node *tmp;
-	unsigned int hash_bkt;
+	uint32_t id;
 
-	hash_for_each_safe(p->events, hash_bkt, tmp, ev, events)
+	idr_for_each_entry(&p->event_idr, ev, id)
 		destroy_event(p, ev);
+	idr_destroy(&p->event_idr);
 }
 
 /*
@@ -365,7 +268,7 @@ int kfd_event_create(struct file *devkfd, struct kfd_process *p,
 		if (!ret) {
 			*event_page_offset = KFD_MMAP_EVENTS_MASK;
 			*event_page_offset <<= PAGE_SHIFT;
-			*event_slot_index = ev->signal_slot_index;
+			*event_slot_index = ev->event_id;
 		}
 		break;
 	default:
@@ -374,8 +277,6 @@ int kfd_event_create(struct file *devkfd, struct kfd_process *p,
 	}
 
 	if (!ret) {
-		hash_add(p->events, &ev->events, ev->event_id);
-
 		*event_id = ev->event_id;
 		*event_trigger_data = ev->event_id;
 	} else {
@@ -465,17 +366,7 @@ int kfd_reset_event(struct kfd_process *p, uint32_t event_id)
 
 static void acknowledge_signal(struct kfd_process *p, struct kfd_event *ev)
 {
-	page_slots(p->signal_page)[ev->signal_slot_index] =
-						UNSIGNALED_EVENT_SLOT;
-}
-
-static bool is_slot_signaled(struct kfd_process *p, unsigned int index)
-{
-	if (!p->signal_page)
-		return false;
-	else
-		return page_slots(p->signal_page)[index] !=
-			UNSIGNALED_EVENT_SLOT;
+	page_slots(p->signal_page)[ev->event_id] = UNSIGNALED_EVENT_SLOT;
 }
 
 static void set_event_from_interrupt(struct kfd_process *p,
@@ -514,13 +405,31 @@ void kfd_signal_event_interrupt(unsigned int pasid, uint32_t partial_id,
 		 * ignore it, but we could use any bits we did receive to
 		 * search faster.
 		 */
-		unsigned int i;
+		uint64_t *slots = page_slots(p->signal_page);
+		uint32_t id;
+
+		if (p->signal_event_count < KFD_SIGNAL_EVENT_LIMIT/2) {
+			/* With relatively few events, it's faster to
+			 * iterate over the event IDR
+			 */
+			idr_for_each_entry(&p->event_idr, ev, id) {
+				if (id >= KFD_SIGNAL_EVENT_LIMIT)
+					break;
 
-		for (i = 0; i < SLOTS_PER_PAGE; i++)
-			if (is_slot_signaled(p, i)) {
-				ev = lookup_event_by_page_slot(p, i);
-				set_event_from_interrupt(p, ev);
+				if (slots[id] != UNSIGNALED_EVENT_SLOT)
+					set_event_from_interrupt(p, ev);
 			}
+		} else {
+			/* With relatively many events, it's faster to
+			 * iterate over the signal slots and lookup
+			 * only signaled events from the IDR.
+			 */
+			for (id = 0; id < KFD_SIGNAL_EVENT_LIMIT; id++)
+				if (slots[id] != UNSIGNALED_EVENT_SLOT) {
+					ev = lookup_event_by_id(p, id);
+					set_event_from_interrupt(p, ev);
+				}
+		}
 	}
 
 	mutex_unlock(&p->event_mutex);
@@ -832,12 +741,13 @@ static void lookup_events_by_type_and_signal(struct kfd_process *p,
 {
 	struct kfd_hsa_memory_exception_data *ev_data;
 	struct kfd_event *ev;
-	int bkt;
+	uint32_t id;
 	bool send_signal = true;
 
 	ev_data = (struct kfd_hsa_memory_exception_data *) event_data;
 
-	hash_for_each(p->events, bkt, ev, events)
+	id = KFD_FIRST_NONSIGNAL_EVENT_ID;
+	idr_for_each_entry_continue(&p->event_idr, ev, id)
 		if (ev->type == type) {
 			send_signal = false;
 			dev_dbg(kfd_device,
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.h b/drivers/gpu/drm/amd/amdkfd/kfd_events.h
index f85fcee..abca5bf 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_events.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.h
@@ -31,9 +31,13 @@
 #include "kfd_priv.h"
 #include <uapi/linux/kfd_ioctl.h>
 
-#define KFD_EVENT_ID_NONSIGNAL_MASK 0x80000000U
-#define KFD_FIRST_NONSIGNAL_EVENT_ID KFD_EVENT_ID_NONSIGNAL_MASK
-#define KFD_LAST_NONSIGNAL_EVENT_ID UINT_MAX
+/*
+ * IDR supports non-negative integer IDs. Small IDs are used for
+ * signal events to match their signal slot. Use the upper half of the
+ * ID space for non-signal events.
+ */
+#define KFD_FIRST_NONSIGNAL_EVENT_ID ((INT_MAX >> 1) + 1)
+#define KFD_LAST_NONSIGNAL_EVENT_ID INT_MAX
 
 /*
  * Written into kfd_signal_slot_t to indicate that the event is not signaled.
@@ -47,9 +51,6 @@ struct kfd_event_waiter;
 struct signal_page;
 
 struct kfd_event {
-	/* All events in process, rooted at kfd_process.events. */
-	struct hlist_node events;
-
 	u32 event_id;
 
 	bool signaled;
@@ -60,7 +61,6 @@ struct kfd_event {
 	wait_queue_head_t wq; /* List of event waiters. */
 
 	/* Only for signal events. */
-	unsigned int signal_slot_index;
 	uint64_t __user *user_signal_address;
 
 	/* type specific data */
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
index c1b3ee2..ebae8e1 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
@@ -31,6 +31,7 @@
 #include <linux/workqueue.h>
 #include <linux/spinlock.h>
 #include <linux/kfd_ioctl.h>
+#include <linux/idr.h>
 #include <kgd_kfd_interface.h>
 
 #include "amd_shared.h"
@@ -538,11 +539,10 @@ struct kfd_process {
 
 	/* Event-related data */
 	struct mutex event_mutex;
-	/* All events in process hashed by ID, linked on kfd_event.events. */
-	DECLARE_HASHTABLE(events, 4);
+	/* Event ID allocator and lookup */
+	struct idr event_idr;
 	/* Event page */
 	struct kfd_signal_page *signal_page;
-	u32 next_nonsignal_event_id;
 	size_t signal_event_count;
 	bool signal_event_limit_reached;
 };
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 12/16] drm/amdkfd: Use IH context ID for signal lookup

[Felix Kuehling]

This speeds up signal lookup when the IH ring entry includes a
valid context ID or partial context ID. Only if the context ID is
found to be invalid, fall back to an exhaustive search of all
signaled events.

Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c |  7 ++-
 drivers/gpu/drm/amd/amdkfd/kfd_events.c          | 73 +++++++++++++++++++-----
 2 files changed, 64 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c b/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c
index 66164aa..3d5ccb3 100644
--- a/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c
+++ b/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c
@@ -47,6 +47,7 @@ static void cik_event_interrupt_wq(struct kfd_dev *dev,
 	unsigned int pasid;
 	const struct cik_ih_ring_entry *ihre =
 			(const struct cik_ih_ring_entry *)ih_ring_entry;
+	uint32_t context_id = ihre->data & 0xfffffff;
 
 	pasid = (ihre->ring_id & 0xffff0000) >> 16;
 
@@ -54,11 +55,11 @@ static void cik_event_interrupt_wq(struct kfd_dev *dev,
 		return;
 
 	if (ihre->source_id == CIK_INTSRC_CP_END_OF_PIPE)
-		kfd_signal_event_interrupt(pasid, 0, 0);
+		kfd_signal_event_interrupt(pasid, context_id, 28);
 	else if (ihre->source_id == CIK_INTSRC_SDMA_TRAP)
-		kfd_signal_event_interrupt(pasid, 0, 0);
+		kfd_signal_event_interrupt(pasid, context_id, 28);
 	else if (ihre->source_id == CIK_INTSRC_SQ_INTERRUPT_MSG)
-		kfd_signal_event_interrupt(pasid, ihre->data & 0xFF, 8);
+		kfd_signal_event_interrupt(pasid, context_id & 0xff, 8);
 	else if (ihre->source_id == CIK_INTSRC_CP_BAD_OPCODE)
 		kfd_signal_hw_exception_event(pasid);
 }
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
index cddd4b9..28fead5 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
@@ -53,12 +53,6 @@ struct kfd_signal_page {
 	uint64_t __user *user_address;
 };
 
-/*
- * For signal events, the event ID is used as the interrupt user data.
- * For SQ s_sendmsg interrupts, this is limited to 8 bits.
- */
-
-#define INTERRUPT_DATA_BITS 12
 
 static uint64_t *page_slots(struct kfd_signal_page *page)
 {
@@ -125,6 +119,54 @@ static struct kfd_event *lookup_event_by_id(struct kfd_process *p, uint32_t id)
 	return idr_find(&p->event_idr, id);
 }
 
+/**
+ * lookup_signaled_event_by_partial_id - Lookup signaled event from partial ID
+ * @p:     Pointer to struct kfd_process
+ * @id:    ID to look up
+ * @bits:  Number of valid bits in @id
+ *
+ * Finds the first signaled event with a matching partial ID. If no
+ * matching signaled event is found, returns NULL. In that case the
+ * caller should assume that the partial ID is invalid and do an
+ * exhaustive search of all siglaned events.
+ *
+ * If multiple events with the same partial ID signal at the same
+ * time, they will be found one interrupt at a time, not necessarily
+ * in the same order the interrupts occurred. As long as the number of
+ * interrupts is correct, all signaled events will be seen by the
+ * driver.
+ */
+static struct kfd_event *lookup_signaled_event_by_partial_id(
+	struct kfd_process *p, uint32_t id, uint32_t bits)
+{
+	struct kfd_event *ev;
+
+	if (!p->signal_page || id >= KFD_SIGNAL_EVENT_LIMIT)
+		return NULL;
+
+	/* Fast path for the common case that @id is not a partial ID
+	 * and we only need a single lookup.
+	 */
+	if (bits > 31 || (1U << bits) >= KFD_SIGNAL_EVENT_LIMIT) {
+		if (page_slots(p->signal_page)[id] == UNSIGNALED_EVENT_SLOT)
+			return NULL;
+
+		return idr_find(&p->event_idr, id);
+	}
+
+	/* General case for partial IDs: Iterate over all matching IDs
+	 * and find the first one that has signaled.
+	 */
+	for (ev = NULL; id < KFD_SIGNAL_EVENT_LIMIT && !ev; id += 1U << bits) {
+		if (page_slots(p->signal_page)[id] == UNSIGNALED_EVENT_SLOT)
+			continue;
+
+		ev = idr_find(&p->event_idr, id);
+	}
+
+	return ev;
+}
+
 static int create_signal_event(struct file *devkfd,
 				struct kfd_process *p,
 				struct kfd_event *ev)
@@ -381,7 +423,7 @@ static void set_event_from_interrupt(struct kfd_process *p,
 void kfd_signal_event_interrupt(unsigned int pasid, uint32_t partial_id,
 				uint32_t valid_id_bits)
 {
-	struct kfd_event *ev;
+	struct kfd_event *ev = NULL;
 
 	/*
 	 * Because we are called from arbitrary context (workqueue) as opposed
@@ -395,19 +437,24 @@ void kfd_signal_event_interrupt(unsigned int pasid, uint32_t partial_id,
 
 	mutex_lock(&p->event_mutex);
 
-	if (valid_id_bits >= INTERRUPT_DATA_BITS) {
-		/* Partial ID is a full ID. */
-		ev = lookup_event_by_id(p, partial_id);
+	if (valid_id_bits)
+		ev = lookup_signaled_event_by_partial_id(p, partial_id,
+							 valid_id_bits);
+	if (ev) {
 		set_event_from_interrupt(p, ev);
 	} else if (p->signal_page) {
 		/*
-		 * Partial ID is in fact partial. For now we completely
-		 * ignore it, but we could use any bits we did receive to
-		 * search faster.
+		 * Partial ID lookup failed. Assume that the event ID
+		 * in the interrupt payload was invalid and do an
+		 * exhaustive search of signaled events.
 		 */
 		uint64_t *slots = page_slots(p->signal_page);
 		uint32_t id;
 
+		if (valid_id_bits)
+			pr_debug_ratelimited("Partial ID invalid: %u (%u valid bits)\n",
+					     partial_id, valid_id_bits);
+
 		if (p->signal_event_count < KFD_SIGNAL_EVENT_LIMIT/2) {
 			/* With relatively few events, it's faster to
 			 * iterate over the event IDR
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Felix Kuehling]

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

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 14/16] drm/amdkfd: increase IH num entries to 8192

[Felix Kuehling]

From: Andres Rodriguez <andres.rodriguez@amd.com>

A larger buffer will let us accommodate applications with a large amount
of semi-simultaneous event signals.

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c b/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
index ffbb91a..a147269 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
@@ -45,7 +45,7 @@
 #include <linux/kfifo.h>
 #include "kfd_priv.h"
 
-#define KFD_IH_NUM_ENTRIES 1024
+#define KFD_IH_NUM_ENTRIES 8192
 
 static void interrupt_wq(struct work_struct *);
 
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 15/16] drm/amdkfd: wait only for IH work on IH exit

[Felix Kuehling]

From: Andres Rodriguez <andres.rodriguez@amd.com>

We don't need to wait for all work to complete in the IH exit function.
We only need to make sure the interrupt_work has finished executing to
guarantee that ih_kfifo is no longer in use.

Signed-off-by: Andres Rodriguez <andres.rodriguez@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c b/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
index a147269..9c08d46 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
@@ -91,11 +91,11 @@ void kfd_interrupt_exit(struct kfd_dev *kfd)
 	spin_unlock_irqrestore(&kfd->interrupt_lock, flags);
 
 	/*
-	 * Flush_scheduled_work ensures that there are no outstanding
+	 * flush_work ensures that there are no outstanding
 	 * work-queue items that will access interrupt_ring. New work items
 	 * can't be created because we stopped interrupt handling above.
 	 */
-	flush_scheduled_work();
+	flush_work(&kfd->interrupt_work);
 
 	kfifo_free(&kfd->ih_fifo);
 }
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 16/16] drm/amdkfd: use a high priority workqueue for IH work

[Felix Kuehling]

From: Andres Rodriguez <andres.rodriguez@amd.com>

In systems under heavy load the IH work may experience significant
scheduling delays.

Under load + system workqueue:
    Max Latency: 7.023695 ms
    Avg Latency: 0.263994 ms

Under load + high priority workqueue:
    Max Latency: 1.162568 ms
    Avg Latency: 0.163213 ms

Further work is required to measure the impact of per-cpu settings on IH
performance.

Signed-off-by: Andres Rodriguez <andres.rodriguez@amd.com>
Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_device.c    | 2 +-
 drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c | 3 ++-
 drivers/gpu/drm/amd/amdkfd/kfd_priv.h      | 1 +
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
index 46049f0..621a3b5 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
@@ -403,7 +403,7 @@ void kgd2kfd_interrupt(struct kfd_dev *kfd, const void *ih_ring_entry)
 	if (kfd->interrupts_active
 	    && interrupt_is_wanted(kfd, ih_ring_entry)
 	    && enqueue_ih_ring_entry(kfd, ih_ring_entry))
-		schedule_work(&kfd->interrupt_work);
+		queue_work(kfd->ih_wq, &kfd->interrupt_work);
 
 	spin_unlock(&kfd->interrupt_lock);
 }
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c b/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
index 9c08d46..035c351 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
@@ -61,6 +61,7 @@ int kfd_interrupt_init(struct kfd_dev *kfd)
 		return r;
 	}
 
+	kfd->ih_wq = alloc_workqueue("KFD IH", WQ_HIGHPRI, 1);
 	spin_lock_init(&kfd->interrupt_lock);
 
 	INIT_WORK(&kfd->interrupt_work, interrupt_wq);
@@ -95,7 +96,7 @@ void kfd_interrupt_exit(struct kfd_dev *kfd)
 	 * work-queue items that will access interrupt_ring. New work items
 	 * can't be created because we stopped interrupt handling above.
 	 */
-	flush_work(&kfd->interrupt_work);
+	flush_workqueue(kfd->ih_wq);
 
 	kfifo_free(&kfd->ih_fifo);
 }
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
index e8d6c0e..bf29021 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
@@ -184,6 +184,7 @@ struct kfd_dev {
 
 	/* Interrupts */
 	struct kfifo ih_fifo;
+	struct workqueue_struct *ih_wq;
 	struct work_struct interrupt_work;
 	spinlock_t interrupt_lock;
 
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 01/16] drm/amdkfd: Add SDMA trap src id to the KFD isr wanted list

[Oded Gabbay]

On Sat, Oct 21, 2017 at 3:23 AM, Felix Kuehling <Felix.Kuehling@amd.com> wrote:
> From: Besar Wicaksono <besar.wicaksono@amd.com>
>
> This enables SDMA signalling with event interrupt.
>
> Signed-off-by: Besar Wicaksono <Besar.Wicaksono@amd.com>
> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
>  drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c | 3 +++
>  drivers/gpu/drm/amd/amdkfd/cik_int.h             | 3 ++-
>  2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c b/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c
> index 211fc48..66164aa 100644
> --- a/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c
> +++ b/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c
> @@ -36,6 +36,7 @@ static bool cik_event_interrupt_isr(struct kfd_dev *dev,
>         /* Do not process in ISR, just request it to be forwarded to WQ. */
>         return (pasid != 0) &&
>                 (ihre->source_id == CIK_INTSRC_CP_END_OF_PIPE ||
> +               ihre->source_id == CIK_INTSRC_SDMA_TRAP ||
>                 ihre->source_id == CIK_INTSRC_SQ_INTERRUPT_MSG ||
>                 ihre->source_id == CIK_INTSRC_CP_BAD_OPCODE);
>  }
> @@ -54,6 +55,8 @@ static void cik_event_interrupt_wq(struct kfd_dev *dev,
>
>         if (ihre->source_id == CIK_INTSRC_CP_END_OF_PIPE)
>                 kfd_signal_event_interrupt(pasid, 0, 0);
> +       else if (ihre->source_id == CIK_INTSRC_SDMA_TRAP)
> +               kfd_signal_event_interrupt(pasid, 0, 0);
>         else if (ihre->source_id == CIK_INTSRC_SQ_INTERRUPT_MSG)
>                 kfd_signal_event_interrupt(pasid, ihre->data & 0xFF, 8);
>         else if (ihre->source_id == CIK_INTSRC_CP_BAD_OPCODE)
> diff --git a/drivers/gpu/drm/amd/amdkfd/cik_int.h b/drivers/gpu/drm/amd/amdkfd/cik_int.h
> index 79a16d2..109298b 100644
> --- a/drivers/gpu/drm/amd/amdkfd/cik_int.h
> +++ b/drivers/gpu/drm/amd/amdkfd/cik_int.h
> @@ -32,9 +32,10 @@ struct cik_ih_ring_entry {
>         uint32_t reserved;
>  };
>
> -#define CIK_INTSRC_DEQUEUE_COMPLETE    0xC6
>  #define CIK_INTSRC_CP_END_OF_PIPE      0xB5
>  #define CIK_INTSRC_CP_BAD_OPCODE       0xB7
> +#define CIK_INTSRC_DEQUEUE_COMPLETE    0xC6
> +#define CIK_INTSRC_SDMA_TRAP           0xE0
>  #define CIK_INTSRC_SQ_INTERRUPT_MSG    0xEF
>
>  #endif
> --
> 2.7.4
>
This patch is:
Reviewed-by: Oded Gabbay <oded.gabbay@gmail.com>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 02/16] drm/amdkfd: Don't dereference kfd_process.mm

[Oded Gabbay]

On Sat, Oct 21, 2017 at 3:23 AM, Felix Kuehling <Felix.Kuehling@amd.com> wrote:
> The kfd_process doesn't own a reference to the mm_struct, so it can
> disappear without warning even while the kfd_process still exists.
> In fact, the delayed kfd_process teardown is triggered by an MMU
> notifier when the mm_struct is destroyed. Permanently holding a
> reference to the mm_struct would prevent this from happening.
>
> Therefore, avoid dereferencing the kfd_process.mm pointer and make
> it opaque. Use get_task_mm to get a temporary reference to the mm
> when it's needed.
>
> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_events.c  | 19 +++++++++++++++----
>  drivers/gpu/drm/amd/amdkfd/kfd_priv.h    |  7 ++++++-
>  drivers/gpu/drm/amd/amdkfd/kfd_process.c |  6 +++++-
>  3 files changed, 26 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> index 944abfa..61ce547 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> @@ -24,8 +24,8 @@
>  #include <linux/slab.h>
>  #include <linux/types.h>
>  #include <linux/sched/signal.h>
> +#include <linux/sched/mm.h>
>  #include <linux/uaccess.h>
> -#include <linux/mm.h>
>  #include <linux/mman.h>
>  #include <linux/memory.h>
>  #include "kfd_priv.h"
> @@ -904,14 +904,24 @@ void kfd_signal_iommu_event(struct kfd_dev *dev, unsigned int pasid,
>          * running so the lookup function returns a locked process.
>          */
>         struct kfd_process *p = kfd_lookup_process_by_pasid(pasid);
> +       struct mm_struct *mm;
>
>         if (!p)
>                 return; /* Presumably process exited. */
>
> +       /* Take a safe reference to the mm_struct, which may otherwise
> +        * disappear even while the kfd_process is still referenced.
> +        */
> +       mm = get_task_mm(p->lead_thread);
> +       if (!mm) {
> +               mutex_unlock(&p->mutex);
> +               return; /* Process is exiting */
> +       }
> +
>         memset(&memory_exception_data, 0, sizeof(memory_exception_data));
>
> -       down_read(&p->mm->mmap_sem);
> -       vma = find_vma(p->mm, address);
> +       down_read(&mm->mmap_sem);
> +       vma = find_vma(mm, address);
>
>         memory_exception_data.gpu_id = dev->id;
>         memory_exception_data.va = address;
> @@ -937,7 +947,8 @@ void kfd_signal_iommu_event(struct kfd_dev *dev, unsigned int pasid,
>                 }
>         }
>
> -       up_read(&p->mm->mmap_sem);
> +       up_read(&mm->mmap_sem);
> +       mmput(mm);
>
>         mutex_lock(&p->event_mutex);
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> index 7d86ec9..1a483a7 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> @@ -494,7 +494,12 @@ struct kfd_process {
>          */
>         struct hlist_node kfd_processes;
>
> -       struct mm_struct *mm;
> +       /*
> +        * Opaque pointer to mm_struct. We don't hold a reference to
> +        * it so it should never be dereferenced from here. This is
> +        * only used for looking up processes by their mm.
> +        */
> +       void *mm;
>
>         struct mutex mutex;
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> index 3ccb3b5..21d27e5 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> @@ -200,7 +200,11 @@ static void kfd_process_destroy_delayed(struct rcu_head *rcu)
>         struct kfd_process *p;
>
>         p = container_of(rcu, struct kfd_process, rcu);
> -       WARN_ON(atomic_read(&p->mm->mm_count) <= 0);
> +       /*
> +        * This cast should be safe here because we grabbed a
> +        * reference to the mm in kfd_process_notifier_release
> +        */
> +       WARN_ON(atomic_read(&((struct mm_struct *)p->mm)->mm_count) <= 0);
>
>         mmdrop(p->mm);
>
> --
> 2.7.4
>
This patch is:
Reviewed-by: Oded Gabbay <oded.gabbay@gmail.com>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 00/16] KFD interrupt and signal event handling improvements

[Oded Gabbay]

On Sat, Oct 21, 2017 at 3:23 AM, Felix Kuehling <Felix.Kuehling@amd.com> wrote:
> This patch series improves interrupt handling latency, signal event
> processing overhead and replaces some custom data structures with
> standard kernel data structures (idr, kfifo, waitqueue).
>
> It also increases the capacity of the number of signals that can be
> processed from 256 to 4096. This breaks ancient versions of the Thunk
> that support only 256 signal events. The current WIP-version on github
> supports both sizes. If support for ancient Thunks is considered
> important, this could be fixed by allowing mappings that are smaller
> than 4096 signals, and limiting the number of signals per process
> depending on the size of the mapped events page.

Hi Felix,
I don't have enough data to say whether it is important in terms of
whether there are actual users out there.
I *can* say that the no.1 rule of the kernel is *never break userspace*

Even if I want to take the relevant patches, I can't really do it,
especially since you mentioned that there is a possible fix for it.

I know its a bit more work, but I suggest you add an additional patch
that supports the old thunk.

Thanks,
Oded

>
> Andres Rodriguez (4):
>   drm/amdkfd: use standard kernel kfifo for IH
>   drm/amdkfd: increase IH num entries to 8192
>   drm/amdkfd: wait only for IH work on IH exit
>   drm/amdkfd: use a high priority workqueue for IH work
>
> Besar Wicaksono (1):
>   drm/amdkfd: Add SDMA trap src id to the KFD isr wanted list
>
> Felix Kuehling (8):
>   drm/amdkfd: Don't dereference kfd_process.mm
>   drm/amdkfd: Clean up kfd_wait_on_events
>   drm/amdkfd: Fix event destruction with pending waiters
>   drm/amdkfd: remove redundant kfd_event_waiter.input_index
>   drm/amdkfd: Use wait_queue_t to implement event waiting
>   drm/amdkfd: Simplify events page allocator
>   drm/amdkfd: Simplify event ID and signal slot management
>   drm/amdkfd: Use IH context ID for signal lookup
>
> Oded Gabbay (1):
>   drm/amdkfd: increase limit of signal events to 4096 per process
>
> Sean Keely (2):
>   drm/amdkfd: Short cut for kfd_wait_on_events without waiting
>   drm/amdkfd: Fix scheduler race in kfd_wait_on_events sleep loop
>
>  drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c |   8 +-
>  drivers/gpu/drm/amd/amdkfd/cik_int.h             |   3 +-
>  drivers/gpu/drm/amd/amdkfd/kfd_chardev.c         |   5 +-
>  drivers/gpu/drm/amd/amdkfd/kfd_device.c          |   2 +-
>  drivers/gpu/drm/amd/amdkfd/kfd_events.c          | 588 ++++++++++-------------
>  drivers/gpu/drm/amd/amdkfd/kfd_events.h          |  18 +-
>  drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c       |  83 ++--
>  drivers/gpu/drm/amd/amdkfd/kfd_priv.h            |  32 +-
>  drivers/gpu/drm/amd/amdkfd/kfd_process.c         |   6 +-
>  include/uapi/linux/kfd_ioctl.h                   |   2 +-
>  10 files changed, 332 insertions(+), 415 deletions(-)
>
> --
> 2.7.4
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 00/16] KFD interrupt and signal event handling improvements

[Christian König]

Am 25.10.2017 um 08:57 schrieb Oded Gabbay:
> On Sat, Oct 21, 2017 at 3:23 AM, Felix Kuehling <Felix.Kuehling@amd.com> wrote:
>> This patch series improves interrupt handling latency, signal event
>> processing overhead and replaces some custom data structures with
>> standard kernel data structures (idr, kfifo, waitqueue).
>>
>> It also increases the capacity of the number of signals that can be
>> processed from 256 to 4096. This breaks ancient versions of the Thunk
>> that support only 256 signal events. The current WIP-version on github
>> supports both sizes. If support for ancient Thunks is considered
>> important, this could be fixed by allowing mappings that are smaller
>> than 4096 signals, and limiting the number of signals per process
>> depending on the size of the mapped events page.
> Hi Felix,
> I don't have enough data to say whether it is important in terms of
> whether there are actual users out there.
> I *can* say that the no.1 rule of the kernel is *never break userspace*

Well I think it should be "never break *existing* userspace", but in 
general I agree.

> Even if I want to take the relevant patches, I can't really do it,
> especially since you mentioned that there is a possible fix for it.
>
> I know its a bit more work, but I suggest you add an additional patch
> that supports the old thunk.

When the user space stack would be 10+ years old I would just say go ahead.

But I agree with Oded here that we need this clean and backward compatible.

Regards,
Christian.

>
> Thanks,
> Oded
>
>> Andres Rodriguez (4):
>>    drm/amdkfd: use standard kernel kfifo for IH
>>    drm/amdkfd: increase IH num entries to 8192
>>    drm/amdkfd: wait only for IH work on IH exit
>>    drm/amdkfd: use a high priority workqueue for IH work
>>
>> Besar Wicaksono (1):
>>    drm/amdkfd: Add SDMA trap src id to the KFD isr wanted list
>>
>> Felix Kuehling (8):
>>    drm/amdkfd: Don't dereference kfd_process.mm
>>    drm/amdkfd: Clean up kfd_wait_on_events
>>    drm/amdkfd: Fix event destruction with pending waiters
>>    drm/amdkfd: remove redundant kfd_event_waiter.input_index
>>    drm/amdkfd: Use wait_queue_t to implement event waiting
>>    drm/amdkfd: Simplify events page allocator
>>    drm/amdkfd: Simplify event ID and signal slot management
>>    drm/amdkfd: Use IH context ID for signal lookup
>>
>> Oded Gabbay (1):
>>    drm/amdkfd: increase limit of signal events to 4096 per process
>>
>> Sean Keely (2):
>>    drm/amdkfd: Short cut for kfd_wait_on_events without waiting
>>    drm/amdkfd: Fix scheduler race in kfd_wait_on_events sleep loop
>>
>>   drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c |   8 +-
>>   drivers/gpu/drm/amd/amdkfd/cik_int.h             |   3 +-
>>   drivers/gpu/drm/amd/amdkfd/kfd_chardev.c         |   5 +-
>>   drivers/gpu/drm/amd/amdkfd/kfd_device.c          |   2 +-
>>   drivers/gpu/drm/amd/amdkfd/kfd_events.c          | 588 ++++++++++-------------
>>   drivers/gpu/drm/amd/amdkfd/kfd_events.h          |  18 +-
>>   drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c       |  83 ++--
>>   drivers/gpu/drm/amd/amdkfd/kfd_priv.h            |  32 +-
>>   drivers/gpu/drm/amd/amdkfd/kfd_process.c         |   6 +-
>>   include/uapi/linux/kfd_ioctl.h                   |   2 +-
>>   10 files changed, 332 insertions(+), 415 deletions(-)
>>
>> --
>> 2.7.4
>>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 03/16] drm/amdkfd: increase limit of signal events to 4096 per process

[Oded Gabbay]

On Sat, Oct 21, 2017 at 3:23 AM, Felix Kuehling <Felix.Kuehling@amd.com> wrote:
> From: Oded Gabbay <oded.gabbay@amd.com>
>
> Signed-off-by: Oded Gabbay <oded.gabbay@amd.com>
> Reviewed-by: Ben Goz <ben.goz@amd.com>
> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_events.c | 2 +-
>  include/uapi/linux/kfd_ioctl.h          | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> index 61ce547..ade71b7 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> @@ -82,7 +82,7 @@ struct signal_page {
>   * For SQ s_sendmsg interrupts, this is limited to 8 bits.
>   */
>
> -#define INTERRUPT_DATA_BITS 8
> +#define INTERRUPT_DATA_BITS 12
>  #define SIGNAL_EVENT_ID_SLOT_SHIFT 0
>
>  static uint64_t *page_slots(struct signal_page *page)
> diff --git a/include/uapi/linux/kfd_ioctl.h b/include/uapi/linux/kfd_ioctl.h
> index 26283fe..731d0df 100644
> --- a/include/uapi/linux/kfd_ioctl.h
> +++ b/include/uapi/linux/kfd_ioctl.h
> @@ -169,7 +169,7 @@ struct kfd_ioctl_dbg_wave_control_args {
>  #define KFD_IOC_WAIT_RESULT_TIMEOUT            1
>  #define KFD_IOC_WAIT_RESULT_FAIL               2
>
> -#define KFD_SIGNAL_EVENT_LIMIT                 256
> +#define KFD_SIGNAL_EVENT_LIMIT                 4096
>
>  struct kfd_ioctl_create_event_args {
>         __u64 event_page_offset;        /* from KFD */
> --
> 2.7.4
>
NAK until we support existing thunk library.
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 04/16] drm/amdkfd: Short cut for kfd_wait_on_events without waiting

[Oded Gabbay]

On Sat, Oct 21, 2017 at 3:23 AM, Felix Kuehling <Felix.Kuehling@amd.com> wrote:
> From: Sean Keely <sean.keely@amd.com>
>
> If kfd_wait_on_events can return immediately, we don't need to populate
> the wait list and don't need to enter the sleep-loop.
>
> Signed-off-by: Sean Keely <sean.keely@amd.com>
> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_events.c | 43 ++++++++++++++++++++++++++++++---
>  1 file changed, 39 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> index ade71b7..64cc42c 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> @@ -617,7 +617,7 @@ static struct kfd_event_waiter *alloc_event_waiters(uint32_t num_events)
>         return event_waiters;
>  }
>
> -static int init_event_waiter(struct kfd_process *p,
> +static int init_event_waiter_get_status(struct kfd_process *p,
>                 struct kfd_event_waiter *waiter,
>                 uint32_t event_id,
>                 uint32_t input_index)
> @@ -632,11 +632,20 @@ static int init_event_waiter(struct kfd_process *p,
>         waiter->activated = ev->signaled;
>         ev->signaled = ev->signaled && !ev->auto_reset;
>
> -       list_add(&waiter->waiters, &ev->waiters);
> -
>         return 0;
>  }
>
> +static void init_event_waiter_add_to_waitlist(struct kfd_event_waiter *waiter)
> +{
> +       struct kfd_event *ev = waiter->event;
> +
> +       /* Only add to the wait list if we actually need to
> +        * wait on this event.
> +        */
> +       if (!waiter->activated)
> +               list_add(&waiter->waiters, &ev->waiters);
> +}
> +
>  static bool test_event_condition(bool all, uint32_t num_events,
>                                 struct kfd_event_waiter *event_waiters)
>  {
> @@ -724,11 +733,17 @@ int kfd_wait_on_events(struct kfd_process *p,
>                         (struct kfd_event_data __user *) data;
>         uint32_t i;
>         int ret = 0;
> +
>         struct kfd_event_waiter *event_waiters = NULL;
>         long timeout = user_timeout_to_jiffies(user_timeout_ms);
>
>         mutex_lock(&p->event_mutex);
>
> +       /* Set to something unreasonable - this is really
> +        * just a bool for now.
> +        */
> +       *wait_result = KFD_WAIT_TIMEOUT;
> +
>         event_waiters = alloc_event_waiters(num_events);
>         if (!event_waiters) {
>                 ret = -ENOMEM;
> @@ -744,14 +759,34 @@ int kfd_wait_on_events(struct kfd_process *p,
>                         goto fail;
>                 }
>
> -               ret = init_event_waiter(p, &event_waiters[i],
> +               ret = init_event_waiter_get_status(p, &event_waiters[i],
>                                 event_data.event_id, i);
>                 if (ret)
>                         goto fail;
>         }
>
> +       /* Check condition once. */
> +       if (test_event_condition(all, num_events, event_waiters)) {
> +               if (copy_signaled_event_data(num_events,
> +                               event_waiters, events))
> +                       *wait_result = KFD_WAIT_COMPLETE;
> +               else
> +                       *wait_result = KFD_WAIT_ERROR;
> +               free_waiters(num_events, event_waiters);
> +       } else {
> +               /* Add to wait lists if we need to wait. */
> +               for (i = 0; i < num_events; i++)
> +                       init_event_waiter_add_to_waitlist(&event_waiters[i]);
> +       }
> +
>         mutex_unlock(&p->event_mutex);
>
> +       /* Return if all waits were already satisfied. */
> +       if (*wait_result != KFD_WAIT_TIMEOUT) {
> +               __set_current_state(TASK_RUNNING);
> +               return ret;
> +       }
> +
>         while (true) {
>                 if (fatal_signal_pending(current)) {
>                         ret = -EINTR;
> --
> 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

Re: [PATCH 05/16] drm/amdkfd: Fix scheduler race in kfd_wait_on_events sleep loop

[Oded Gabbay]

On Sat, Oct 21, 2017 at 3:23 AM, Felix Kuehling <Felix.Kuehling@amd.com> wrote:
> From: Sean Keely <sean.keely@amd.com>
>
> Signed-off-by: Sean Keely <sean.keely@amd.com>
> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_events.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> index 64cc42c..c6d9572 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> @@ -806,6 +806,17 @@ int kfd_wait_on_events(struct kfd_process *p,
>                         break;
>                 }
>
> +               /* Set task state to interruptible sleep before
> +                * checking wake-up conditions. A concurrent wake-up
> +                * will put the task back into runnable state. In that
> +                * case schedule_timeout will not put the task to
> +                * sleep and we'll get a chance to re-check the
> +                * updated conditions almost immediately. Otherwise,
> +                * this race condition would lead to a soft hang or a
> +                * very long sleep.
> +                */
> +               set_current_state(TASK_INTERRUPTIBLE);
> +
>                 if (test_event_condition(all, num_events, event_waiters)) {
>                         if (copy_signaled_event_data(num_events,
>                                         event_waiters, events))
> @@ -820,7 +831,7 @@ int kfd_wait_on_events(struct kfd_process *p,
>                         break;
>                 }
>
> -               timeout = schedule_timeout_interruptible(timeout);
> +               timeout = schedule_timeout(timeout);
>         }
>         __set_current_state(TASK_RUNNING);
>
> --
> 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

Re: [PATCH 06/16] drm/amdkfd: Clean up kfd_wait_on_events

[Oded Gabbay]

On Sat, Oct 21, 2017 at 3:23 AM, Felix Kuehling <Felix.Kuehling@amd.com> wrote:
> Cleaned up the code while resolving some potential bugs and
> inconsistencies in the process.
>
> Clean-ups:
> * Remove enum kfd_event_wait_result, which duplicates
>   KFD_IOC_EVENT_RESULT definitions
> * alloc_event_waiters can be called without holding p->event_mutex
> * Return an error code from copy_signaled_event_data instead of bool
> * Clean up error handling code paths to minimize duplication in
>   kfd_wait_on_events
>
> Fixes:
> * Consistently return an error code from kfd_wait_on_events and set
>   wait_result to KFD_IOC_WAIT_RESULT_FAIL in all failure cases.
> * Always call free_waiters while holding p->event_mutex
> * copy_signaled_event_data might sleep. Don't call it while the task state
>   is TASK_INTERRUPTIBLE.
>
> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_chardev.c |  5 +--
>  drivers/gpu/drm/amd/amdkfd/kfd_events.c  | 71 ++++++++++++++------------------
>  drivers/gpu/drm/amd/amdkfd/kfd_priv.h    |  8 +---
>  3 files changed, 32 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> index 0ef82b2..a25321f 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> @@ -835,15 +835,12 @@ static int kfd_ioctl_wait_events(struct file *filp, struct kfd_process *p,
>                                 void *data)
>  {
>         struct kfd_ioctl_wait_events_args *args = data;
> -       enum kfd_event_wait_result wait_result;
>         int err;
>
>         err = kfd_wait_on_events(p, args->num_events,
>                         (void __user *)args->events_ptr,
>                         (args->wait_for_all != 0),
> -                       args->timeout, &wait_result);
> -
> -       args->wait_result = wait_result;
> +                       args->timeout, &args->wait_result);
>
>         return err;
>  }
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> index c6d9572..5bb88b74 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> @@ -668,7 +668,7 @@ static bool test_event_condition(bool all, uint32_t num_events,
>   * Copy event specific data, if defined.
>   * Currently only memory exception events have additional data to copy to user
>   */
> -static bool copy_signaled_event_data(uint32_t num_events,
> +static int copy_signaled_event_data(uint32_t num_events,
>                 struct kfd_event_waiter *event_waiters,
>                 struct kfd_event_data __user *data)
>  {
> @@ -686,11 +686,11 @@ static bool copy_signaled_event_data(uint32_t num_events,
>                         src = &event->memory_exception_data;
>                         if (copy_to_user(dst, src,
>                                 sizeof(struct kfd_hsa_memory_exception_data)))
> -                               return false;
> +                               return -EFAULT;
>                 }
>         }
>
> -       return true;
> +       return 0;
>
>  }
>
> @@ -727,7 +727,7 @@ static void free_waiters(uint32_t num_events, struct kfd_event_waiter *waiters)
>  int kfd_wait_on_events(struct kfd_process *p,
>                        uint32_t num_events, void __user *data,
>                        bool all, uint32_t user_timeout_ms,
> -                      enum kfd_event_wait_result *wait_result)
> +                      uint32_t *wait_result)
>  {
>         struct kfd_event_data __user *events =
>                         (struct kfd_event_data __user *) data;
> @@ -737,18 +737,18 @@ int kfd_wait_on_events(struct kfd_process *p,
>         struct kfd_event_waiter *event_waiters = NULL;
>         long timeout = user_timeout_to_jiffies(user_timeout_ms);
>
> +       event_waiters = alloc_event_waiters(num_events);
> +       if (!event_waiters) {
> +               ret = -ENOMEM;
> +               goto out;
> +       }
> +
>         mutex_lock(&p->event_mutex);
>
>         /* Set to something unreasonable - this is really
>          * just a bool for now.
>          */
> -       *wait_result = KFD_WAIT_TIMEOUT;
> -
> -       event_waiters = alloc_event_waiters(num_events);
> -       if (!event_waiters) {
> -               ret = -ENOMEM;
> -               goto fail;
> -       }
> +       *wait_result = KFD_IOC_WAIT_RESULT_TIMEOUT;
>
>         for (i = 0; i < num_events; i++) {
>                 struct kfd_event_data event_data;
> @@ -756,23 +756,21 @@ int kfd_wait_on_events(struct kfd_process *p,
>                 if (copy_from_user(&event_data, &events[i],
>                                 sizeof(struct kfd_event_data))) {
>                         ret = -EFAULT;
> -                       goto fail;
> +                       goto out_unlock;
>                 }
>
>                 ret = init_event_waiter_get_status(p, &event_waiters[i],
>                                 event_data.event_id, i);
>                 if (ret)
> -                       goto fail;
> +                       goto out_unlock;
>         }
>
>         /* Check condition once. */
>         if (test_event_condition(all, num_events, event_waiters)) {
> -               if (copy_signaled_event_data(num_events,
> -                               event_waiters, events))
> -                       *wait_result = KFD_WAIT_COMPLETE;
> -               else
> -                       *wait_result = KFD_WAIT_ERROR;
> -               free_waiters(num_events, event_waiters);
> +               *wait_result = KFD_IOC_WAIT_RESULT_COMPLETE;
> +               ret = copy_signaled_event_data(num_events,
> +                                              event_waiters, events);
> +               goto out_unlock;
>         } else {
>                 /* Add to wait lists if we need to wait. */
>                 for (i = 0; i < num_events; i++)
> @@ -781,12 +779,6 @@ int kfd_wait_on_events(struct kfd_process *p,
>
>         mutex_unlock(&p->event_mutex);
>
> -       /* Return if all waits were already satisfied. */
> -       if (*wait_result != KFD_WAIT_TIMEOUT) {
> -               __set_current_state(TASK_RUNNING);
> -               return ret;
> -       }
> -
>         while (true) {
>                 if (fatal_signal_pending(current)) {
>                         ret = -EINTR;
> @@ -818,16 +810,12 @@ int kfd_wait_on_events(struct kfd_process *p,
>                 set_current_state(TASK_INTERRUPTIBLE);
>
>                 if (test_event_condition(all, num_events, event_waiters)) {
> -                       if (copy_signaled_event_data(num_events,
> -                                       event_waiters, events))
> -                               *wait_result = KFD_WAIT_COMPLETE;
> -                       else
> -                               *wait_result = KFD_WAIT_ERROR;
> +                       *wait_result = KFD_IOC_WAIT_RESULT_COMPLETE;
>                         break;
>                 }
>
>                 if (timeout <= 0) {
> -                       *wait_result = KFD_WAIT_TIMEOUT;
> +                       *wait_result = KFD_IOC_WAIT_RESULT_TIMEOUT;
>                         break;
>                 }
>
> @@ -835,19 +823,20 @@ int kfd_wait_on_events(struct kfd_process *p,
>         }
>         __set_current_state(TASK_RUNNING);
>
> +       /* copy_signaled_event_data may sleep. So this has to happen
> +        * after the task state is set back to RUNNING.
> +        */
> +       if (!ret && *wait_result == KFD_IOC_WAIT_RESULT_COMPLETE)
> +               ret = copy_signaled_event_data(num_events,
> +                                              event_waiters, events);
> +
>         mutex_lock(&p->event_mutex);
> +out_unlock:
>         free_waiters(num_events, event_waiters);
>         mutex_unlock(&p->event_mutex);
> -
> -       return ret;
> -
> -fail:
> -       if (event_waiters)
> -               free_waiters(num_events, event_waiters);
> -
> -       mutex_unlock(&p->event_mutex);
> -
> -       *wait_result = KFD_WAIT_ERROR;
> +out:
> +       if (ret)
> +               *wait_result = KFD_IOC_WAIT_RESULT_FAIL;
>
>         return ret;
>  }
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> index 1a483a7..d3cf53a 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> @@ -726,19 +726,13 @@ uint64_t kfd_get_number_elems(struct kfd_dev *kfd);
>  extern const struct kfd_event_interrupt_class event_interrupt_class_cik;
>  extern const struct kfd_device_global_init_class device_global_init_class_cik;
>
> -enum kfd_event_wait_result {
> -       KFD_WAIT_COMPLETE,
> -       KFD_WAIT_TIMEOUT,
> -       KFD_WAIT_ERROR
> -};
> -
>  void kfd_event_init_process(struct kfd_process *p);
>  void kfd_event_free_process(struct kfd_process *p);
>  int kfd_event_mmap(struct kfd_process *process, struct vm_area_struct *vma);
>  int kfd_wait_on_events(struct kfd_process *p,
>                        uint32_t num_events, void __user *data,
>                        bool all, uint32_t user_timeout_ms,
> -                      enum kfd_event_wait_result *wait_result);
> +                      uint32_t *wait_result);
>  void kfd_signal_event_interrupt(unsigned int pasid, uint32_t partial_id,
>                                 uint32_t valid_id_bits);
>  void kfd_signal_iommu_event(struct kfd_dev *dev,
> --
> 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

Re: [PATCH 07/16] drm/amdkfd: Fix event destruction with pending waiters

[Oded Gabbay]

On Sat, Oct 21, 2017 at 3:23 AM, Felix Kuehling <Felix.Kuehling@amd.com> wrote:
> When an event with pending waiters is destroyed, those waiters may
> end up sleeping forever unless they are notified and woken up.
> Implement the notification by clearing the waiter->event pointer,
> which becomes invalid anyway, when the event is freed, and waking
> up the waiting tasks.
>
> Waiters on an event that's destroyed return failure.
>
> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_events.c | 72 +++++++++++++++++++++------------
>  1 file changed, 46 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> index 5bb88b74..6050e88 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> @@ -345,18 +345,24 @@ void kfd_event_init_process(struct kfd_process *p)
>
>  static void destroy_event(struct kfd_process *p, struct kfd_event *ev)
>  {
> +       /* Wake up pending waiters. They will return failure */
> +       while (!list_empty(&ev->waiters)) {
> +               struct kfd_event_waiter *waiter =
> +                       list_first_entry(&ev->waiters, struct kfd_event_waiter,
> +                                        waiters);
> +
> +               waiter->event = NULL;
> +               /* _init because free_waiters will call list_del */
> +               list_del_init(&waiter->waiters);
> +               wake_up_process(waiter->sleeping_task);
> +       }
> +
>         if (ev->signal_page) {
>                 release_event_notification_slot(ev->signal_page,
>                                                 ev->signal_slot_index);
>                 p->signal_event_count--;
>         }
>
> -       /*
> -        * Abandon the list of waiters. Individual waiting threads will
> -        * clean up their own data.
> -        */
> -       list_del(&ev->waiters);
> -
>         hash_del(&ev->events);
>         kfree(ev);
>  }
> @@ -646,22 +652,36 @@ static void init_event_waiter_add_to_waitlist(struct kfd_event_waiter *waiter)
>                 list_add(&waiter->waiters, &ev->waiters);
>  }
>
> -static bool test_event_condition(bool all, uint32_t num_events,
> +/* test_event_condition - Test condition of events being waited for
> + * @all:           Return completion only if all events have signaled
> + * @num_events:    Number of events to wait for
> + * @event_waiters: Array of event waiters, one per event
> + *
> + * Returns KFD_IOC_WAIT_RESULT_COMPLETE if all (or one) event(s) have
> + * signaled. Returns KFD_IOC_WAIT_RESULT_TIMEOUT if no (or not all)
> + * events have signaled. Returns KFD_IOC_WAIT_RESULT_FAIL if any of
> + * the events have been destroyed.
> + */
> +static uint32_t test_event_condition(bool all, uint32_t num_events,
>                                 struct kfd_event_waiter *event_waiters)
>  {
>         uint32_t i;
>         uint32_t activated_count = 0;
>
>         for (i = 0; i < num_events; i++) {
> +               if (!event_waiters[i].event)
> +                       return KFD_IOC_WAIT_RESULT_FAIL;
> +
>                 if (event_waiters[i].activated) {
>                         if (!all)
> -                               return true;
> +                               return KFD_IOC_WAIT_RESULT_COMPLETE;
>
>                         activated_count++;
>                 }
>         }
>
> -       return activated_count == num_events;
> +       return activated_count == num_events ?
> +               KFD_IOC_WAIT_RESULT_COMPLETE : KFD_IOC_WAIT_RESULT_TIMEOUT;
>  }
>
>  /*
> @@ -745,11 +765,6 @@ int kfd_wait_on_events(struct kfd_process *p,
>
>         mutex_lock(&p->event_mutex);
>
> -       /* Set to something unreasonable - this is really
> -        * just a bool for now.
> -        */
> -       *wait_result = KFD_IOC_WAIT_RESULT_TIMEOUT;
> -
>         for (i = 0; i < num_events; i++) {
>                 struct kfd_event_data event_data;
>
> @@ -766,17 +781,22 @@ int kfd_wait_on_events(struct kfd_process *p,
>         }
>
>         /* Check condition once. */
> -       if (test_event_condition(all, num_events, event_waiters)) {
> -               *wait_result = KFD_IOC_WAIT_RESULT_COMPLETE;
> +       *wait_result = test_event_condition(all, num_events, event_waiters);
> +       if (*wait_result == KFD_IOC_WAIT_RESULT_COMPLETE) {
>                 ret = copy_signaled_event_data(num_events,
>                                                event_waiters, events);
>                 goto out_unlock;
> -       } else {
> -               /* Add to wait lists if we need to wait. */
> -               for (i = 0; i < num_events; i++)
> -                       init_event_waiter_add_to_waitlist(&event_waiters[i]);
> +       } else if (WARN_ON(*wait_result == KFD_IOC_WAIT_RESULT_FAIL)) {
> +               /* This should not happen. Events shouldn't be
> +                * destroyed while we're holding the event_mutex
> +                */
> +               goto out_unlock;
>         }
>
> +       /* Add to wait lists if we need to wait. */
> +       for (i = 0; i < num_events; i++)
> +               init_event_waiter_add_to_waitlist(&event_waiters[i]);
> +
>         mutex_unlock(&p->event_mutex);
>
>         while (true) {
> @@ -809,15 +829,13 @@ int kfd_wait_on_events(struct kfd_process *p,
>                  */
>                 set_current_state(TASK_INTERRUPTIBLE);
>
> -               if (test_event_condition(all, num_events, event_waiters)) {
> -                       *wait_result = KFD_IOC_WAIT_RESULT_COMPLETE;
> +               *wait_result = test_event_condition(all, num_events,
> +                                                   event_waiters);
> +               if (*wait_result != KFD_IOC_WAIT_RESULT_TIMEOUT)
>                         break;
> -               }
>
> -               if (timeout <= 0) {
> -                       *wait_result = KFD_IOC_WAIT_RESULT_TIMEOUT;
> +               if (timeout <= 0)
>                         break;
> -               }
>
>                 timeout = schedule_timeout(timeout);
>         }
> @@ -837,6 +855,8 @@ int kfd_wait_on_events(struct kfd_process *p,
>  out:
>         if (ret)
>                 *wait_result = KFD_IOC_WAIT_RESULT_FAIL;
> +       else if (*wait_result == KFD_IOC_WAIT_RESULT_FAIL)
> +               ret = -EIO;
>
>         return ret;
>  }
> --
> 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

Re: [PATCH 08/16] drm/amdkfd: remove redundant kfd_event_waiter.input_index

[Oded Gabbay]

On Sat, Oct 21, 2017 at 3:23 AM, Felix Kuehling <Felix.Kuehling@amd.com> wrote:
> This always identical with the index of the event_waiter in the array.
> No need to store it in the waiter record.
>
> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_events.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> index 6050e88..949b80a 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> @@ -49,7 +49,6 @@ struct kfd_event_waiter {
>
>         /* Event */
>         struct kfd_event *event;
> -       uint32_t input_index;
>  };
>
>  /*
> @@ -625,8 +624,7 @@ static struct kfd_event_waiter *alloc_event_waiters(uint32_t num_events)
>
>  static int init_event_waiter_get_status(struct kfd_process *p,
>                 struct kfd_event_waiter *waiter,
> -               uint32_t event_id,
> -               uint32_t input_index)
> +               uint32_t event_id)
>  {
>         struct kfd_event *ev = lookup_event_by_id(p, event_id);
>
> @@ -634,7 +632,6 @@ static int init_event_waiter_get_status(struct kfd_process *p,
>                 return -EINVAL;
>
>         waiter->event = ev;
> -       waiter->input_index = input_index;
>         waiter->activated = ev->signaled;
>         ev->signaled = ev->signaled && !ev->auto_reset;
>
> @@ -702,7 +699,7 @@ static int copy_signaled_event_data(uint32_t num_events,
>                 waiter = &event_waiters[i];
>                 event = waiter->event;
>                 if (waiter->activated && event->type == KFD_EVENT_TYPE_MEMORY) {
> -                       dst = &data[waiter->input_index].memory_exception_data;
> +                       dst = &data[i].memory_exception_data;
>                         src = &event->memory_exception_data;
>                         if (copy_to_user(dst, src,
>                                 sizeof(struct kfd_hsa_memory_exception_data)))
> @@ -775,7 +772,7 @@ int kfd_wait_on_events(struct kfd_process *p,
>                 }
>
>                 ret = init_event_waiter_get_status(p, &event_waiters[i],
> -                               event_data.event_id, i);
> +                               event_data.event_id);
>                 if (ret)
>                         goto out_unlock;
>         }
> --
> 2.7.4
>
This patch is:
Reviewed-by: Oded Gabbay <oded.gabbay@gmail.com>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 09/16] drm/amdkfd: Use wait_queue_t to implement event waiting

[Oded Gabbay]

On Sat, Oct 21, 2017 at 3:23 AM, Felix Kuehling <Felix.Kuehling@amd.com> wrote:
> Signed-off-by: Kent Russell <kent.russell@amd.com>
> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>

I would like some more explanation in the commit message about this change.
- What is the general idea in this solution.
- Why it was done, i.e. what is the improvement
Thanks,
Oded


> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_events.c | 53 +++++++++++----------------------
>  drivers/gpu/drm/amd/amdkfd/kfd_events.h |  3 +-
>  2 files changed, 19 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> index 949b80a..f178248 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> @@ -33,22 +33,12 @@
>  #include <linux/device.h>
>
>  /*
> - * A task can only be on a single wait_queue at a time, but we need to support
> - * waiting on multiple events (any/all).
> - * Instead of each event simply having a wait_queue with sleeping tasks, it
> - * has a singly-linked list of tasks.
> - * A thread that wants to sleep creates an array of these, one for each event
> - * and adds one to each event's waiter chain.
> + * Wrapper around wait_queue_entry_t
>   */
>  struct kfd_event_waiter {
> -       struct list_head waiters;
> -       struct task_struct *sleeping_task;
> -
> -       /* Transitions to true when the event this belongs to is signaled. */
> -       bool activated;
> -
> -       /* Event */
> -       struct kfd_event *event;
> +       wait_queue_entry_t wait;
> +       struct kfd_event *event; /* Event to wait for */
> +       bool activated;          /* Becomes true when event is signaled */
>  };
>
>  /*
> @@ -344,17 +334,12 @@ void kfd_event_init_process(struct kfd_process *p)
>
>  static void destroy_event(struct kfd_process *p, struct kfd_event *ev)
>  {
> -       /* Wake up pending waiters. They will return failure */
> -       while (!list_empty(&ev->waiters)) {
> -               struct kfd_event_waiter *waiter =
> -                       list_first_entry(&ev->waiters, struct kfd_event_waiter,
> -                                        waiters);
> +       struct kfd_event_waiter *waiter;
>
> +       /* Wake up pending waiters. They will return failure */
> +       list_for_each_entry(waiter, &ev->wq.head, wait.entry)
>                 waiter->event = NULL;
> -               /* _init because free_waiters will call list_del */
> -               list_del_init(&waiter->waiters);
> -               wake_up_process(waiter->sleeping_task);
> -       }
> +       wake_up_all(&ev->wq);
>
>         if (ev->signal_page) {
>                 release_event_notification_slot(ev->signal_page,
> @@ -424,7 +409,7 @@ int kfd_event_create(struct file *devkfd, struct kfd_process *p,
>         ev->auto_reset = auto_reset;
>         ev->signaled = false;
>
> -       INIT_LIST_HEAD(&ev->waiters);
> +       init_waitqueue_head(&ev->wq);
>
>         *event_page_offset = 0;
>
> @@ -482,19 +467,14 @@ int kfd_event_destroy(struct kfd_process *p, uint32_t event_id)
>  static void set_event(struct kfd_event *ev)
>  {
>         struct kfd_event_waiter *waiter;
> -       struct kfd_event_waiter *next;
>
>         /* Auto reset if the list is non-empty and we're waking someone. */
> -       ev->signaled = !ev->auto_reset || list_empty(&ev->waiters);
> +       ev->signaled = !ev->auto_reset || !waitqueue_active(&ev->wq);

This line gives a warning in the checkpatch script. Per Linus's patch
(http://www.spinics.net/lists/mm-commits/msg111660.html), please add a
comment explaining the use of waitqueue_active and why its not racy
with the caller.

Oded

>
> -       list_for_each_entry_safe(waiter, next, &ev->waiters, waiters) {
> +       list_for_each_entry(waiter, &ev->wq.head, wait.entry)
>                 waiter->activated = true;
>
> -               /* _init because free_waiters will call list_del */
> -               list_del_init(&waiter->waiters);
> -
> -               wake_up_process(waiter->sleeping_task);
> -       }
> +       wake_up_all(&ev->wq);
>  }
>
>  /* Assumes that p is current. */
> @@ -614,8 +594,7 @@ static struct kfd_event_waiter *alloc_event_waiters(uint32_t num_events)
>                                         GFP_KERNEL);
>
>         for (i = 0; (event_waiters) && (i < num_events) ; i++) {
> -               INIT_LIST_HEAD(&event_waiters[i].waiters);
> -               event_waiters[i].sleeping_task = current;
> +               init_wait(&event_waiters[i].wait);
>                 event_waiters[i].activated = false;
>         }
>
> @@ -646,7 +625,7 @@ static void init_event_waiter_add_to_waitlist(struct kfd_event_waiter *waiter)
>          * wait on this event.
>          */
>         if (!waiter->activated)
> -               list_add(&waiter->waiters, &ev->waiters);
> +               add_wait_queue(&ev->wq, &waiter->wait);
>  }
>
>  /* test_event_condition - Test condition of events being waited for
> @@ -736,7 +715,9 @@ static void free_waiters(uint32_t num_events, struct kfd_event_waiter *waiters)
>         uint32_t i;
>
>         for (i = 0; i < num_events; i++)
> -               list_del(&waiters[i].waiters);
> +               if (waiters[i].event)
> +                       remove_wait_queue(&waiters[i].event->wq,
> +                                         &waiters[i].wait);
>
>         kfree(waiters);
>  }
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.h b/drivers/gpu/drm/amd/amdkfd/kfd_events.h
> index 28f6838..96f9122 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_events.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.h
> @@ -27,6 +27,7 @@
>  #include <linux/hashtable.h>
>  #include <linux/types.h>
>  #include <linux/list.h>
> +#include <linux/wait.h>
>  #include "kfd_priv.h"
>  #include <uapi/linux/kfd_ioctl.h>
>
> @@ -56,7 +57,7 @@ struct kfd_event {
>
>         int type;
>
> -       struct list_head waiters; /* List of kfd_event_waiter by waiters. */
> +       wait_queue_head_t wq; /* List of event waiters. */
>
>         /* Only for signal events. */
>         struct signal_page *signal_page;
> --
> 2.7.4
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 10/16] drm/amdkfd: Simplify events page allocator

[Oded Gabbay]

On Sat, Oct 21, 2017 at 3:23 AM, Felix Kuehling <Felix.Kuehling@amd.com> wrote:
> The first event page is always big enough to handle all events.
> Handling of multiple events pages is not supported by user mode, and
> not necessary.
>
> Signed-off-by: Yong Zhao <yong.zhao@amd.com>
> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_events.c | 197 +++++++++++---------------------
>  drivers/gpu/drm/amd/amdkfd/kfd_events.h |   1 -
>  drivers/gpu/drm/amd/amdkfd/kfd_priv.h   |   4 +-
>  3 files changed, 70 insertions(+), 132 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> index f178248..f800e48 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> @@ -41,6 +41,9 @@ struct kfd_event_waiter {
>         bool activated;          /* Becomes true when event is signaled */
>  };
>
> +#define SLOTS_PER_PAGE KFD_SIGNAL_EVENT_LIMIT
> +#define SLOT_BITMAP_LONGS BITS_TO_LONGS(SLOTS_PER_PAGE)
> +
>  /*
>   * Over-complicated pooled allocator for event notification slots.
>   *
> @@ -51,132 +54,98 @@ struct kfd_event_waiter {
>   * Individual signal events are then allocated a slot in a page.
>   */
>
> -struct signal_page {
> -       struct list_head event_pages;   /* kfd_process.signal_event_pages */
> +struct kfd_signal_page {
>         uint64_t *kernel_address;
>         uint64_t __user *user_address;
> -       uint32_t page_index;            /* Index into the mmap aperture. */
>         unsigned int free_slots;
> -       unsigned long used_slot_bitmap[0];
> +       unsigned long used_slot_bitmap[SLOT_BITMAP_LONGS];
>  };
>
> -#define SLOTS_PER_PAGE KFD_SIGNAL_EVENT_LIMIT
> -#define SLOT_BITMAP_SIZE BITS_TO_LONGS(SLOTS_PER_PAGE)
> -#define BITS_PER_PAGE (ilog2(SLOTS_PER_PAGE)+1)
> -#define SIGNAL_PAGE_SIZE (sizeof(struct signal_page) + \
> -                               SLOT_BITMAP_SIZE * sizeof(long))
> -
>  /*
>   * For signal events, the event ID is used as the interrupt user data.
>   * For SQ s_sendmsg interrupts, this is limited to 8 bits.
>   */
>
>  #define INTERRUPT_DATA_BITS 12
> -#define SIGNAL_EVENT_ID_SLOT_SHIFT 0
>
> -static uint64_t *page_slots(struct signal_page *page)
> +static uint64_t *page_slots(struct kfd_signal_page *page)
>  {
>         return page->kernel_address;
>  }
>
>  static bool allocate_free_slot(struct kfd_process *process,
> -                               struct signal_page **out_page,
> -                               unsigned int *out_slot_index)
> +                              unsigned int *out_slot_index)
>  {
> -       struct signal_page *page;
> +       struct kfd_signal_page *page = process->signal_page;
> +       unsigned int slot;
>
> -       list_for_each_entry(page, &process->signal_event_pages, event_pages) {
> -               if (page->free_slots > 0) {
> -                       unsigned int slot =
> -                               find_first_zero_bit(page->used_slot_bitmap,
> -                                                       SLOTS_PER_PAGE);
> +       if (!page || page->free_slots == 0) {
> +               pr_debug("No free event signal slots were found for process %p\n",
> +                        process);
>
> -                       __set_bit(slot, page->used_slot_bitmap);
> -                       page->free_slots--;
> +               return false;
> +       }
>
> -                       page_slots(page)[slot] = UNSIGNALED_EVENT_SLOT;
> +       slot = find_first_zero_bit(page->used_slot_bitmap, SLOTS_PER_PAGE);
>
> -                       *out_page = page;
> -                       *out_slot_index = slot;
> +       __set_bit(slot, page->used_slot_bitmap);
> +       page->free_slots--;
>
> -                       pr_debug("Allocated event signal slot in page %p, slot %d\n",
> -                                       page, slot);
> +       page_slots(page)[slot] = UNSIGNALED_EVENT_SLOT;
>
> -                       return true;
> -               }
> -       }
> +       *out_slot_index = slot;
>
> -       pr_debug("No free event signal slots were found for process %p\n",
> -                       process);
> +       pr_debug("Allocated event signal slot in page %p, slot %d\n",
> +                page, slot);
>
> -       return false;
> +       return true;
>  }
>
> -#define list_tail_entry(head, type, member) \
> -       list_entry((head)->prev, type, member)
> -
> -static bool allocate_signal_page(struct file *devkfd, struct kfd_process *p)
> +static struct kfd_signal_page *allocate_signal_page(struct kfd_process *p)
>  {
>         void *backing_store;
> -       struct signal_page *page;
> +       struct kfd_signal_page *page;
>
> -       page = kzalloc(SIGNAL_PAGE_SIZE, GFP_KERNEL);
> +       page = kzalloc(sizeof(*page), GFP_KERNEL);
>         if (!page)
> -               goto fail_alloc_signal_page;
> +               return NULL;
>
>         page->free_slots = SLOTS_PER_PAGE;
>
> -       backing_store = (void *) __get_free_pages(GFP_KERNEL | __GFP_ZERO,
> +       backing_store = (void *) __get_free_pages(GFP_KERNEL,
>                                         get_order(KFD_SIGNAL_EVENT_LIMIT * 8));
>         if (!backing_store)
>                 goto fail_alloc_signal_store;
>
> -       /* prevent user-mode info leaks */
> +       /* Initialize all events to unsignaled */
>         memset(backing_store, (uint8_t) UNSIGNALED_EVENT_SLOT,
> -               KFD_SIGNAL_EVENT_LIMIT * 8);
> +              KFD_SIGNAL_EVENT_LIMIT * 8);
>
>         page->kernel_address = backing_store;
> -
> -       if (list_empty(&p->signal_event_pages))
> -               page->page_index = 0;
> -       else
> -               page->page_index = list_tail_entry(&p->signal_event_pages,
> -                                                  struct signal_page,
> -                                                  event_pages)->page_index + 1;
> -
>         pr_debug("Allocated new event signal page at %p, for process %p\n",
>                         page, p);
> -       pr_debug("Page index is %d\n", page->page_index);
> -
> -       list_add(&page->event_pages, &p->signal_event_pages);
>
> -       return true;
> +       return page;
>
>  fail_alloc_signal_store:
>         kfree(page);
> -fail_alloc_signal_page:
> -       return false;
> +       return NULL;
>  }
>
> -static bool allocate_event_notification_slot(struct file *devkfd,
> -                                       struct kfd_process *p,
> -                                       struct signal_page **page,
> -                                       unsigned int *signal_slot_index)
> +static bool allocate_event_notification_slot(struct kfd_process *p,
> +                                            unsigned int *signal_slot_index)
>  {
> -       bool ret;
> -
> -       ret = allocate_free_slot(p, page, signal_slot_index);
> -       if (!ret) {
> -               ret = allocate_signal_page(devkfd, p);
> -               if (ret)
> -                       ret = allocate_free_slot(p, page, signal_slot_index);
> +       if (!p->signal_page) {
> +               p->signal_page = allocate_signal_page(p);
> +               if (!p->signal_page)
> +                       return false;
>         }
>
> -       return ret;
> +       return allocate_free_slot(p, signal_slot_index);
>  }
>
>  /* Assumes that the process's event_mutex is locked. */
> -static void release_event_notification_slot(struct signal_page *page,
> +static void release_event_notification_slot(struct kfd_signal_page *page,
>                                                 size_t slot_index)
>  {
>         __clear_bit(slot_index, page->used_slot_bitmap);
> @@ -187,22 +156,6 @@ static void release_event_notification_slot(struct signal_page *page,
>          */
>  }
>
> -static struct signal_page *lookup_signal_page_by_index(struct kfd_process *p,
> -                                               unsigned int page_index)
> -{
> -       struct signal_page *page;
> -
> -       /*
> -        * This is safe because we don't delete signal pages until the
> -        * process exits.
> -        */
> -       list_for_each_entry(page, &p->signal_event_pages, event_pages)
> -               if (page->page_index == page_index)
> -                       return page;
> -
> -       return NULL;
> -}
> -
>  /*
>   * Assumes that p->event_mutex is held and of course that p is not going
>   * away (current or locked).
> @@ -218,13 +171,6 @@ static struct kfd_event *lookup_event_by_id(struct kfd_process *p, uint32_t id)
>         return NULL;
>  }
>
> -static u32 make_signal_event_id(struct signal_page *page,
> -                                        unsigned int signal_slot_index)
> -{
> -       return page->page_index |
> -                       (signal_slot_index << SIGNAL_EVENT_ID_SLOT_SHIFT);
> -}
> -
>  /*
>   * Produce a kfd event id for a nonsignal event.
>   * These are arbitrary numbers, so we do a sequential search through
> @@ -270,10 +216,9 @@ static u32 make_nonsignal_event_id(struct kfd_process *p)
>  }
>
>  static struct kfd_event *lookup_event_by_page_slot(struct kfd_process *p,
> -                                               struct signal_page *page,
>                                                 unsigned int signal_slot)
>  {
> -       return lookup_event_by_id(p, make_signal_event_id(page, signal_slot));
> +       return lookup_event_by_id(p, signal_slot);
>  }
>
>  static int create_signal_event(struct file *devkfd,
> @@ -288,8 +233,7 @@ static int create_signal_event(struct file *devkfd,
>                 return -ENOMEM;
>         }
>
> -       if (!allocate_event_notification_slot(devkfd, p, &ev->signal_page,
> -                                               &ev->signal_slot_index)) {
> +       if (!allocate_event_notification_slot(p, &ev->signal_slot_index)) {
>                 pr_warn("Signal event wasn't created because out of kernel memory\n");
>                 return -ENOMEM;
>         }
> @@ -297,10 +241,9 @@ static int create_signal_event(struct file *devkfd,
>         p->signal_event_count++;
>
>         ev->user_signal_address =
> -                       &ev->signal_page->user_address[ev->signal_slot_index];
> +                       &p->signal_page->user_address[ev->signal_slot_index];
>
> -       ev->event_id = make_signal_event_id(ev->signal_page,
> -                                               ev->signal_slot_index);
> +       ev->event_id = ev->signal_slot_index;
>
>         pr_debug("Signal event number %zu created with id %d, address %p\n",
>                         p->signal_event_count, ev->event_id,
> @@ -327,7 +270,7 @@ void kfd_event_init_process(struct kfd_process *p)
>  {
>         mutex_init(&p->event_mutex);
>         hash_init(p->events);
> -       INIT_LIST_HEAD(&p->signal_event_pages);
> +       p->signal_page = NULL;
>         p->next_nonsignal_event_id = KFD_FIRST_NONSIGNAL_EVENT_ID;
>         p->signal_event_count = 0;
>  }
> @@ -341,8 +284,9 @@ static void destroy_event(struct kfd_process *p, struct kfd_event *ev)
>                 waiter->event = NULL;
>         wake_up_all(&ev->wq);
>
> -       if (ev->signal_page) {
> -               release_event_notification_slot(ev->signal_page,
> +       if ((ev->type == KFD_EVENT_TYPE_SIGNAL ||
> +            ev->type == KFD_EVENT_TYPE_DEBUG) && p->signal_page) {
> +               release_event_notification_slot(p->signal_page,
>                                                 ev->signal_slot_index);
>                 p->signal_event_count--;
>         }
> @@ -365,12 +309,11 @@ static void destroy_events(struct kfd_process *p)
>   * We assume that the process is being destroyed and there is no need to
>   * unmap the pages or keep bookkeeping data in order.
>   */
> -static void shutdown_signal_pages(struct kfd_process *p)
> +static void shutdown_signal_page(struct kfd_process *p)
>  {
> -       struct signal_page *page, *tmp;
> +       struct kfd_signal_page *page = p->signal_page;
>
> -       list_for_each_entry_safe(page, tmp, &p->signal_event_pages,
> -                                       event_pages) {
> +       if (page) {
>                 free_pages((unsigned long)page->kernel_address,
>                                 get_order(KFD_SIGNAL_EVENT_LIMIT * 8));
>                 kfree(page);
> @@ -380,7 +323,7 @@ static void shutdown_signal_pages(struct kfd_process *p)
>  void kfd_event_free_process(struct kfd_process *p)
>  {
>         destroy_events(p);
> -       shutdown_signal_pages(p);
> +       shutdown_signal_page(p);
>  }
>
>  static bool event_can_be_gpu_signaled(const struct kfd_event *ev)
> @@ -420,8 +363,7 @@ int kfd_event_create(struct file *devkfd, struct kfd_process *p,
>         case KFD_EVENT_TYPE_DEBUG:
>                 ret = create_signal_event(devkfd, p, ev);
>                 if (!ret) {
> -                       *event_page_offset = (ev->signal_page->page_index |
> -                                       KFD_MMAP_EVENTS_MASK);
> +                       *event_page_offset = KFD_MMAP_EVENTS_MASK;
>                         *event_page_offset <<= PAGE_SHIFT;
>                         *event_slot_index = ev->signal_slot_index;
>                 }
> @@ -523,13 +465,17 @@ int kfd_reset_event(struct kfd_process *p, uint32_t event_id)
>
>  static void acknowledge_signal(struct kfd_process *p, struct kfd_event *ev)
>  {
> -       page_slots(ev->signal_page)[ev->signal_slot_index] =
> +       page_slots(p->signal_page)[ev->signal_slot_index] =
>                                                 UNSIGNALED_EVENT_SLOT;
>  }
>
> -static bool is_slot_signaled(struct signal_page *page, unsigned int index)
> +static bool is_slot_signaled(struct kfd_process *p, unsigned int index)
>  {
> -       return page_slots(page)[index] != UNSIGNALED_EVENT_SLOT;
> +       if (!p->signal_page)
> +               return false;
> +       else
> +               return page_slots(p->signal_page)[index] !=
> +                       UNSIGNALED_EVENT_SLOT;
>  }
>
>  static void set_event_from_interrupt(struct kfd_process *p,
> @@ -562,22 +508,19 @@ void kfd_signal_event_interrupt(unsigned int pasid, uint32_t partial_id,
>                 /* Partial ID is a full ID. */
>                 ev = lookup_event_by_id(p, partial_id);
>                 set_event_from_interrupt(p, ev);
> -       } else {
> +       } else if (p->signal_page) {
>                 /*
>                  * Partial ID is in fact partial. For now we completely
>                  * ignore it, but we could use any bits we did receive to
>                  * search faster.
>                  */
> -               struct signal_page *page;
>                 unsigned int i;
>
> -               list_for_each_entry(page, &p->signal_event_pages, event_pages)
> -                       for (i = 0; i < SLOTS_PER_PAGE; i++)
> -                               if (is_slot_signaled(page, i)) {
> -                                       ev = lookup_event_by_page_slot(p,
> -                                                               page, i);
> -                                       set_event_from_interrupt(p, ev);
> -                               }
> +               for (i = 0; i < SLOTS_PER_PAGE; i++)
> +                       if (is_slot_signaled(p, i)) {
> +                               ev = lookup_event_by_page_slot(p, i);
> +                               set_event_from_interrupt(p, ev);
> +                       }
>         }
>
>         mutex_unlock(&p->event_mutex);
> @@ -842,9 +785,8 @@ int kfd_wait_on_events(struct kfd_process *p,
>  int kfd_event_mmap(struct kfd_process *p, struct vm_area_struct *vma)
>  {
>
> -       unsigned int page_index;
>         unsigned long pfn;
> -       struct signal_page *page;
> +       struct kfd_signal_page *page;
>
>         /* check required size is logical */
>         if (get_order(KFD_SIGNAL_EVENT_LIMIT * 8) !=
> @@ -853,13 +795,10 @@ int kfd_event_mmap(struct kfd_process *p, struct vm_area_struct *vma)
>                 return -EINVAL;
>         }
>
> -       page_index = vma->vm_pgoff;
> -
> -       page = lookup_signal_page_by_index(p, page_index);
> +       page = p->signal_page;
>         if (!page) {
>                 /* Probably KFD bug, but mmap is user-accessible. */
> -               pr_debug("Signal page could not be found for page_index %u\n",
> -                               page_index);
> +               pr_debug("Signal page could not be found\n");
>                 return -EINVAL;
>         }
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.h b/drivers/gpu/drm/amd/amdkfd/kfd_events.h
> index 96f9122..f85fcee 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_events.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.h
> @@ -60,7 +60,6 @@ struct kfd_event {
>         wait_queue_head_t wq; /* List of event waiters. */
>
>         /* Only for signal events. */
> -       struct signal_page *signal_page;
>         unsigned int signal_slot_index;
>         uint64_t __user *user_signal_address;
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> index d3cf53a..c1b3ee2 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> @@ -540,8 +540,8 @@ struct kfd_process {
>         struct mutex event_mutex;
>         /* All events in process hashed by ID, linked on kfd_event.events. */
>         DECLARE_HASHTABLE(events, 4);
> -       /* struct slot_page_header.event_pages */
> -       struct list_head signal_event_pages;
> +       /* Event page */
> +       struct kfd_signal_page *signal_page;
>         u32 next_nonsignal_event_id;
>         size_t signal_event_count;
>         bool signal_event_limit_reached;
> --
> 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

Re: [PATCH 11/16] drm/amdkfd: Simplify event ID and signal slot management

[Oded Gabbay]

On Sat, Oct 21, 2017 at 3:23 AM, Felix Kuehling <Felix.Kuehling@amd.com> wrote:
> Signal slots are identical to event IDs.
>
> Replace the used_slot_bitmap and events hash table with an IDR to
> allocate and lookup event IDs and signal slots more efficiently.
>
> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_events.c | 230 ++++++++++----------------------
>  drivers/gpu/drm/amd/amdkfd/kfd_events.h |  14 +-
>  drivers/gpu/drm/amd/amdkfd/kfd_priv.h   |   6 +-
>  3 files changed, 80 insertions(+), 170 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> index f800e48..cddd4b9 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> @@ -41,24 +41,16 @@ struct kfd_event_waiter {
>         bool activated;          /* Becomes true when event is signaled */
>  };
>
> -#define SLOTS_PER_PAGE KFD_SIGNAL_EVENT_LIMIT
> -#define SLOT_BITMAP_LONGS BITS_TO_LONGS(SLOTS_PER_PAGE)
> -
>  /*
> - * Over-complicated pooled allocator for event notification slots.
> - *
>   * Each signal event needs a 64-bit signal slot where the signaler will write
> - * a 1 before sending an interrupt.l (This is needed because some interrupts
> + * a 1 before sending an interrupt. (This is needed because some interrupts
>   * do not contain enough spare data bits to identify an event.)
> - * We get whole pages from vmalloc and map them to the process VA.
> - * Individual signal events are then allocated a slot in a page.
> + * We get whole pages and map them to the process VA.
> + * Individual signal events use their event_id as slot index.
>   */
> -
>  struct kfd_signal_page {
>         uint64_t *kernel_address;
>         uint64_t __user *user_address;
> -       unsigned int free_slots;
> -       unsigned long used_slot_bitmap[SLOT_BITMAP_LONGS];
>  };
>
>  /*
> @@ -73,34 +65,6 @@ static uint64_t *page_slots(struct kfd_signal_page *page)
>         return page->kernel_address;
>  }
>
> -static bool allocate_free_slot(struct kfd_process *process,
> -                              unsigned int *out_slot_index)
> -{
> -       struct kfd_signal_page *page = process->signal_page;
> -       unsigned int slot;
> -
> -       if (!page || page->free_slots == 0) {
> -               pr_debug("No free event signal slots were found for process %p\n",
> -                        process);
> -
> -               return false;
> -       }
> -
> -       slot = find_first_zero_bit(page->used_slot_bitmap, SLOTS_PER_PAGE);
> -
> -       __set_bit(slot, page->used_slot_bitmap);
> -       page->free_slots--;
> -
> -       page_slots(page)[slot] = UNSIGNALED_EVENT_SLOT;
> -
> -       *out_slot_index = slot;
> -
> -       pr_debug("Allocated event signal slot in page %p, slot %d\n",
> -                page, slot);
> -
> -       return true;
> -}
> -
>  static struct kfd_signal_page *allocate_signal_page(struct kfd_process *p)
>  {
>         void *backing_store;
> @@ -110,8 +74,6 @@ static struct kfd_signal_page *allocate_signal_page(struct kfd_process *p)
>         if (!page)
>                 return NULL;
>
> -       page->free_slots = SLOTS_PER_PAGE;
> -
>         backing_store = (void *) __get_free_pages(GFP_KERNEL,
>                                         get_order(KFD_SIGNAL_EVENT_LIMIT * 8));
>         if (!backing_store)
> @@ -132,28 +94,26 @@ static struct kfd_signal_page *allocate_signal_page(struct kfd_process *p)
>         return NULL;
>  }
>
> -static bool allocate_event_notification_slot(struct kfd_process *p,
> -                                            unsigned int *signal_slot_index)
> +static int allocate_event_notification_slot(struct kfd_process *p,
> +                                           struct kfd_event *ev)
>  {
> +       int id;
> +
>         if (!p->signal_page) {
>                 p->signal_page = allocate_signal_page(p);
>                 if (!p->signal_page)
> -                       return false;
> +                       return -ENOMEM;
>         }
>
> -       return allocate_free_slot(p, signal_slot_index);
> -}
> +       id = idr_alloc(&p->event_idr, ev, 0, KFD_SIGNAL_EVENT_LIMIT,
> +                      GFP_KERNEL);
> +       if (id < 0)
> +               return id;
>
> -/* Assumes that the process's event_mutex is locked. */
> -static void release_event_notification_slot(struct kfd_signal_page *page,
> -                                               size_t slot_index)
> -{
> -       __clear_bit(slot_index, page->used_slot_bitmap);
> -       page->free_slots++;
> +       ev->event_id = id;
> +       page_slots(p->signal_page)[id] = UNSIGNALED_EVENT_SLOT;
>
> -       /* We don't free signal pages, they are retained by the process
> -        * and reused until it exits.
> -        */
> +       return 0;
>  }
>
>  /*
> @@ -162,89 +122,32 @@ static void release_event_notification_slot(struct kfd_signal_page *page,
>   */
>  static struct kfd_event *lookup_event_by_id(struct kfd_process *p, uint32_t id)
>  {
> -       struct kfd_event *ev;
> -
> -       hash_for_each_possible(p->events, ev, events, id)
> -               if (ev->event_id == id)
> -                       return ev;
> -
> -       return NULL;
> -}
> -
> -/*
> - * Produce a kfd event id for a nonsignal event.
> - * These are arbitrary numbers, so we do a sequential search through
> - * the hash table for an unused number.
> - */
> -static u32 make_nonsignal_event_id(struct kfd_process *p)
> -{
> -       u32 id;
> -
> -       for (id = p->next_nonsignal_event_id;
> -               id < KFD_LAST_NONSIGNAL_EVENT_ID &&
> -               lookup_event_by_id(p, id);
> -               id++)
> -               ;
> -
> -       if (id < KFD_LAST_NONSIGNAL_EVENT_ID) {
> -
> -               /*
> -                * What if id == LAST_NONSIGNAL_EVENT_ID - 1?
> -                * Then next_nonsignal_event_id = LAST_NONSIGNAL_EVENT_ID so
> -                * the first loop fails immediately and we proceed with the
> -                * wraparound loop below.
> -                */
> -               p->next_nonsignal_event_id = id + 1;
> -
> -               return id;
> -       }
> -
> -       for (id = KFD_FIRST_NONSIGNAL_EVENT_ID;
> -               id < KFD_LAST_NONSIGNAL_EVENT_ID &&
> -               lookup_event_by_id(p, id);
> -               id++)
> -               ;
> -
> -
> -       if (id < KFD_LAST_NONSIGNAL_EVENT_ID) {
> -               p->next_nonsignal_event_id = id + 1;
> -               return id;
> -       }
> -
> -       p->next_nonsignal_event_id = KFD_FIRST_NONSIGNAL_EVENT_ID;
> -       return 0;
> -}
> -
> -static struct kfd_event *lookup_event_by_page_slot(struct kfd_process *p,
> -                                               unsigned int signal_slot)
> -{
> -       return lookup_event_by_id(p, signal_slot);
> +       return idr_find(&p->event_idr, id);
>  }
>
>  static int create_signal_event(struct file *devkfd,
>                                 struct kfd_process *p,
>                                 struct kfd_event *ev)
>  {
> +       int ret;
> +
>         if (p->signal_event_count == KFD_SIGNAL_EVENT_LIMIT) {
>                 if (!p->signal_event_limit_reached) {
>                         pr_warn("Signal event wasn't created because limit was reached\n");
>                         p->signal_event_limit_reached = true;
>                 }
> -               return -ENOMEM;
> +               return -ENOSPC;
>         }
>
> -       if (!allocate_event_notification_slot(p, &ev->signal_slot_index)) {
> +       ret = allocate_event_notification_slot(p, ev);
> +       if (ret) {
>                 pr_warn("Signal event wasn't created because out of kernel memory\n");
> -               return -ENOMEM;
> +               return ret;
>         }
>
>         p->signal_event_count++;
>
> -       ev->user_signal_address =
> -                       &p->signal_page->user_address[ev->signal_slot_index];
> -
> -       ev->event_id = ev->signal_slot_index;
> -
> +       ev->user_signal_address = &p->signal_page->user_address[ev->event_id];
>         pr_debug("Signal event number %zu created with id %d, address %p\n",
>                         p->signal_event_count, ev->event_id,
>                         ev->user_signal_address);
> @@ -252,16 +155,20 @@ static int create_signal_event(struct file *devkfd,
>         return 0;
>  }
>
> -/*
> - * No non-signal events are supported yet.
> - * We create them as events that never signal.
> - * Set event calls from user-mode are failed.
> - */
>  static int create_other_event(struct kfd_process *p, struct kfd_event *ev)
>  {
> -       ev->event_id = make_nonsignal_event_id(p);
> -       if (ev->event_id == 0)
> -               return -ENOMEM;
> +       /* Cast KFD_LAST_NONSIGNAL_EVENT to uint32_t. This allows an
> +        * intentional integer overflow to -1 without a compiler
> +        * warning. idr_alloc treats a negative value as "maximum
> +        * signed integer".
> +        */
> +       int id = idr_alloc(&p->event_idr, ev, KFD_FIRST_NONSIGNAL_EVENT_ID,
> +                          (uint32_t)KFD_LAST_NONSIGNAL_EVENT_ID + 1,
> +                          GFP_KERNEL);
> +
> +       if (id < 0)
> +               return id;
> +       ev->event_id = id;
>
>         return 0;
>  }
> @@ -269,9 +176,8 @@ static int create_other_event(struct kfd_process *p, struct kfd_event *ev)
>  void kfd_event_init_process(struct kfd_process *p)
>  {
>         mutex_init(&p->event_mutex);
> -       hash_init(p->events);
> +       idr_init(&p->event_idr);
>         p->signal_page = NULL;
> -       p->next_nonsignal_event_id = KFD_FIRST_NONSIGNAL_EVENT_ID;
>         p->signal_event_count = 0;
>  }
>
> @@ -284,25 +190,22 @@ static void destroy_event(struct kfd_process *p, struct kfd_event *ev)
>                 waiter->event = NULL;
>         wake_up_all(&ev->wq);
>
> -       if ((ev->type == KFD_EVENT_TYPE_SIGNAL ||
> -            ev->type == KFD_EVENT_TYPE_DEBUG) && p->signal_page) {
> -               release_event_notification_slot(p->signal_page,
> -                                               ev->signal_slot_index);
> +       if (ev->type == KFD_EVENT_TYPE_SIGNAL ||
> +           ev->type == KFD_EVENT_TYPE_DEBUG)
>                 p->signal_event_count--;
> -       }
>
> -       hash_del(&ev->events);
> +       idr_remove(&p->event_idr, ev->event_id);
>         kfree(ev);
>  }
>
>  static void destroy_events(struct kfd_process *p)
>  {
>         struct kfd_event *ev;
> -       struct hlist_node *tmp;
> -       unsigned int hash_bkt;
> +       uint32_t id;
>
> -       hash_for_each_safe(p->events, hash_bkt, tmp, ev, events)
> +       idr_for_each_entry(&p->event_idr, ev, id)
>                 destroy_event(p, ev);
> +       idr_destroy(&p->event_idr);
>  }
>
>  /*
> @@ -365,7 +268,7 @@ int kfd_event_create(struct file *devkfd, struct kfd_process *p,
>                 if (!ret) {
>                         *event_page_offset = KFD_MMAP_EVENTS_MASK;
>                         *event_page_offset <<= PAGE_SHIFT;
> -                       *event_slot_index = ev->signal_slot_index;
> +                       *event_slot_index = ev->event_id;
>                 }
>                 break;
>         default:
> @@ -374,8 +277,6 @@ int kfd_event_create(struct file *devkfd, struct kfd_process *p,
>         }
>
>         if (!ret) {
> -               hash_add(p->events, &ev->events, ev->event_id);
> -
>                 *event_id = ev->event_id;
>                 *event_trigger_data = ev->event_id;
>         } else {
> @@ -465,17 +366,7 @@ int kfd_reset_event(struct kfd_process *p, uint32_t event_id)
>
>  static void acknowledge_signal(struct kfd_process *p, struct kfd_event *ev)
>  {
> -       page_slots(p->signal_page)[ev->signal_slot_index] =
> -                                               UNSIGNALED_EVENT_SLOT;
> -}
> -
> -static bool is_slot_signaled(struct kfd_process *p, unsigned int index)
> -{
> -       if (!p->signal_page)
> -               return false;
> -       else
> -               return page_slots(p->signal_page)[index] !=
> -                       UNSIGNALED_EVENT_SLOT;
> +       page_slots(p->signal_page)[ev->event_id] = UNSIGNALED_EVENT_SLOT;
>  }
>
>  static void set_event_from_interrupt(struct kfd_process *p,
> @@ -514,13 +405,31 @@ void kfd_signal_event_interrupt(unsigned int pasid, uint32_t partial_id,
>                  * ignore it, but we could use any bits we did receive to
>                  * search faster.
>                  */
> -               unsigned int i;
> +               uint64_t *slots = page_slots(p->signal_page);
> +               uint32_t id;
> +
> +               if (p->signal_event_count < KFD_SIGNAL_EVENT_LIMIT/2) {
> +                       /* With relatively few events, it's faster to
> +                        * iterate over the event IDR
> +                        */
> +                       idr_for_each_entry(&p->event_idr, ev, id) {
> +                               if (id >= KFD_SIGNAL_EVENT_LIMIT)
> +                                       break;
>
> -               for (i = 0; i < SLOTS_PER_PAGE; i++)
> -                       if (is_slot_signaled(p, i)) {
> -                               ev = lookup_event_by_page_slot(p, i);
> -                               set_event_from_interrupt(p, ev);
> +                               if (slots[id] != UNSIGNALED_EVENT_SLOT)
> +                                       set_event_from_interrupt(p, ev);
>                         }
> +               } else {
> +                       /* With relatively many events, it's faster to
> +                        * iterate over the signal slots and lookup
> +                        * only signaled events from the IDR.
> +                        */
> +                       for (id = 0; id < KFD_SIGNAL_EVENT_LIMIT; id++)
> +                               if (slots[id] != UNSIGNALED_EVENT_SLOT) {
> +                                       ev = lookup_event_by_id(p, id);
> +                                       set_event_from_interrupt(p, ev);
> +                               }
> +               }
>         }
>
>         mutex_unlock(&p->event_mutex);
> @@ -832,12 +741,13 @@ static void lookup_events_by_type_and_signal(struct kfd_process *p,
>  {
>         struct kfd_hsa_memory_exception_data *ev_data;
>         struct kfd_event *ev;
> -       int bkt;
> +       uint32_t id;
>         bool send_signal = true;
>
>         ev_data = (struct kfd_hsa_memory_exception_data *) event_data;
>
> -       hash_for_each(p->events, bkt, ev, events)
> +       id = KFD_FIRST_NONSIGNAL_EVENT_ID;
> +       idr_for_each_entry_continue(&p->event_idr, ev, id)
>                 if (ev->type == type) {
>                         send_signal = false;
>                         dev_dbg(kfd_device,
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.h b/drivers/gpu/drm/amd/amdkfd/kfd_events.h
> index f85fcee..abca5bf 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_events.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.h
> @@ -31,9 +31,13 @@
>  #include "kfd_priv.h"
>  #include <uapi/linux/kfd_ioctl.h>
>
> -#define KFD_EVENT_ID_NONSIGNAL_MASK 0x80000000U
> -#define KFD_FIRST_NONSIGNAL_EVENT_ID KFD_EVENT_ID_NONSIGNAL_MASK
> -#define KFD_LAST_NONSIGNAL_EVENT_ID UINT_MAX
> +/*
> + * IDR supports non-negative integer IDs. Small IDs are used for
> + * signal events to match their signal slot. Use the upper half of the
> + * ID space for non-signal events.
> + */
> +#define KFD_FIRST_NONSIGNAL_EVENT_ID ((INT_MAX >> 1) + 1)
> +#define KFD_LAST_NONSIGNAL_EVENT_ID INT_MAX
>
>  /*
>   * Written into kfd_signal_slot_t to indicate that the event is not signaled.
> @@ -47,9 +51,6 @@ struct kfd_event_waiter;
>  struct signal_page;
>
>  struct kfd_event {
> -       /* All events in process, rooted at kfd_process.events. */
> -       struct hlist_node events;
> -
>         u32 event_id;
>
>         bool signaled;
> @@ -60,7 +61,6 @@ struct kfd_event {
>         wait_queue_head_t wq; /* List of event waiters. */
>
>         /* Only for signal events. */
> -       unsigned int signal_slot_index;
>         uint64_t __user *user_signal_address;
>
>         /* type specific data */
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> index c1b3ee2..ebae8e1 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> @@ -31,6 +31,7 @@
>  #include <linux/workqueue.h>
>  #include <linux/spinlock.h>
>  #include <linux/kfd_ioctl.h>
> +#include <linux/idr.h>
>  #include <kgd_kfd_interface.h>
>
>  #include "amd_shared.h"
> @@ -538,11 +539,10 @@ struct kfd_process {
>
>         /* Event-related data */
>         struct mutex event_mutex;
> -       /* All events in process hashed by ID, linked on kfd_event.events. */
> -       DECLARE_HASHTABLE(events, 4);
> +       /* Event ID allocator and lookup */
> +       struct idr event_idr;
>         /* Event page */
>         struct kfd_signal_page *signal_page;
> -       u32 next_nonsignal_event_id;
>         size_t signal_event_count;
>         bool signal_event_limit_reached;
>  };
> --
> 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

Re: [PATCH 12/16] drm/amdkfd: Use IH context ID for signal lookup

[Oded Gabbay]

On Sat, Oct 21, 2017 at 3:23 AM, Felix Kuehling <Felix.Kuehling@amd.com> wrote:
> This speeds up signal lookup when the IH ring entry includes a
> valid context ID or partial context ID. Only if the context ID is
> found to be invalid, fall back to an exhaustive search of all
> signaled events.
>
> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
>  drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c |  7 ++-
>  drivers/gpu/drm/amd/amdkfd/kfd_events.c          | 73 +++++++++++++++++++-----
>  2 files changed, 64 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c b/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c
> index 66164aa..3d5ccb3 100644
> --- a/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c
> +++ b/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c
> @@ -47,6 +47,7 @@ static void cik_event_interrupt_wq(struct kfd_dev *dev,
>         unsigned int pasid;
>         const struct cik_ih_ring_entry *ihre =
>                         (const struct cik_ih_ring_entry *)ih_ring_entry;
> +       uint32_t context_id = ihre->data & 0xfffffff;
>
>         pasid = (ihre->ring_id & 0xffff0000) >> 16;
>
> @@ -54,11 +55,11 @@ static void cik_event_interrupt_wq(struct kfd_dev *dev,
>                 return;
>
>         if (ihre->source_id == CIK_INTSRC_CP_END_OF_PIPE)
> -               kfd_signal_event_interrupt(pasid, 0, 0);
> +               kfd_signal_event_interrupt(pasid, context_id, 28);
>         else if (ihre->source_id == CIK_INTSRC_SDMA_TRAP)
> -               kfd_signal_event_interrupt(pasid, 0, 0);
> +               kfd_signal_event_interrupt(pasid, context_id, 28);
>         else if (ihre->source_id == CIK_INTSRC_SQ_INTERRUPT_MSG)
> -               kfd_signal_event_interrupt(pasid, ihre->data & 0xFF, 8);
> +               kfd_signal_event_interrupt(pasid, context_id & 0xff, 8);
>         else if (ihre->source_id == CIK_INTSRC_CP_BAD_OPCODE)
>                 kfd_signal_hw_exception_event(pasid);
>  }
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> index cddd4b9..28fead5 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> @@ -53,12 +53,6 @@ struct kfd_signal_page {
>         uint64_t __user *user_address;
>  };
>
> -/*
> - * For signal events, the event ID is used as the interrupt user data.
> - * For SQ s_sendmsg interrupts, this is limited to 8 bits.
> - */
> -
> -#define INTERRUPT_DATA_BITS 12
>
>  static uint64_t *page_slots(struct kfd_signal_page *page)
>  {
> @@ -125,6 +119,54 @@ static struct kfd_event *lookup_event_by_id(struct kfd_process *p, uint32_t id)
>         return idr_find(&p->event_idr, id);
>  }
>
> +/**
> + * lookup_signaled_event_by_partial_id - Lookup signaled event from partial ID
> + * @p:     Pointer to struct kfd_process
> + * @id:    ID to look up
> + * @bits:  Number of valid bits in @id
> + *
> + * Finds the first signaled event with a matching partial ID. If no
> + * matching signaled event is found, returns NULL. In that case the
> + * caller should assume that the partial ID is invalid and do an
> + * exhaustive search of all siglaned events.
> + *
> + * If multiple events with the same partial ID signal at the same
> + * time, they will be found one interrupt at a time, not necessarily
> + * in the same order the interrupts occurred. As long as the number of
> + * interrupts is correct, all signaled events will be seen by the
> + * driver.
> + */
> +static struct kfd_event *lookup_signaled_event_by_partial_id(
> +       struct kfd_process *p, uint32_t id, uint32_t bits)
> +{
> +       struct kfd_event *ev;
> +
> +       if (!p->signal_page || id >= KFD_SIGNAL_EVENT_LIMIT)
> +               return NULL;
> +
> +       /* Fast path for the common case that @id is not a partial ID
> +        * and we only need a single lookup.
> +        */
> +       if (bits > 31 || (1U << bits) >= KFD_SIGNAL_EVENT_LIMIT) {
> +               if (page_slots(p->signal_page)[id] == UNSIGNALED_EVENT_SLOT)
> +                       return NULL;
> +
> +               return idr_find(&p->event_idr, id);
> +       }
> +
> +       /* General case for partial IDs: Iterate over all matching IDs
> +        * and find the first one that has signaled.
> +        */
> +       for (ev = NULL; id < KFD_SIGNAL_EVENT_LIMIT && !ev; id += 1U << bits) {
> +               if (page_slots(p->signal_page)[id] == UNSIGNALED_EVENT_SLOT)
> +                       continue;
> +
> +               ev = idr_find(&p->event_idr, id);
> +       }
> +
> +       return ev;
> +}
> +
>  static int create_signal_event(struct file *devkfd,
>                                 struct kfd_process *p,
>                                 struct kfd_event *ev)
> @@ -381,7 +423,7 @@ static void set_event_from_interrupt(struct kfd_process *p,
>  void kfd_signal_event_interrupt(unsigned int pasid, uint32_t partial_id,
>                                 uint32_t valid_id_bits)
>  {
> -       struct kfd_event *ev;
> +       struct kfd_event *ev = NULL;
>
>         /*
>          * Because we are called from arbitrary context (workqueue) as opposed
> @@ -395,19 +437,24 @@ void kfd_signal_event_interrupt(unsigned int pasid, uint32_t partial_id,
>
>         mutex_lock(&p->event_mutex);
>
> -       if (valid_id_bits >= INTERRUPT_DATA_BITS) {
> -               /* Partial ID is a full ID. */
> -               ev = lookup_event_by_id(p, partial_id);
> +       if (valid_id_bits)
> +               ev = lookup_signaled_event_by_partial_id(p, partial_id,
> +                                                        valid_id_bits);
> +       if (ev) {
>                 set_event_from_interrupt(p, ev);
>         } else if (p->signal_page) {
>                 /*
> -                * Partial ID is in fact partial. For now we completely
> -                * ignore it, but we could use any bits we did receive to
> -                * search faster.
> +                * Partial ID lookup failed. Assume that the event ID
> +                * in the interrupt payload was invalid and do an
> +                * exhaustive search of signaled events.
>                  */
>                 uint64_t *slots = page_slots(p->signal_page);
>                 uint32_t id;
>
> +               if (valid_id_bits)
> +                       pr_debug_ratelimited("Partial ID invalid: %u (%u valid bits)\n",
> +                                            partial_id, valid_id_bits);
> +
>                 if (p->signal_event_count < KFD_SIGNAL_EVENT_LIMIT/2) {
>                         /* With relatively few events, it's faster to
>                          * iterate over the event IDR
> --
> 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

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

[Oded Gabbay]

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

Re: [PATCH 14/16] drm/amdkfd: increase IH num entries to 8192

[Oded Gabbay]

On Sat, Oct 21, 2017 at 3:23 AM, Felix Kuehling <Felix.Kuehling@amd.com> wrote:
> From: Andres Rodriguez <andres.rodriguez@amd.com>
>
> A larger buffer will let us accommodate applications with a large amount
> of semi-simultaneous event signals.
>
> 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 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c b/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
> index ffbb91a..a147269 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
> @@ -45,7 +45,7 @@
>  #include <linux/kfifo.h>
>  #include "kfd_priv.h"
>
> -#define KFD_IH_NUM_ENTRIES 1024
> +#define KFD_IH_NUM_ENTRIES 8192
>
>  static void interrupt_wq(struct work_struct *);
>
> --
> 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

Re: [PATCH 15/16] drm/amdkfd: wait only for IH work on IH exit

[Oded Gabbay]

On Sat, Oct 21, 2017 at 3:23 AM, Felix Kuehling <Felix.Kuehling@amd.com> wrote:
> From: Andres Rodriguez <andres.rodriguez@amd.com>
>
> We don't need to wait for all work to complete in the IH exit function.
> We only need to make sure the interrupt_work has finished executing to
> guarantee that ih_kfifo is no longer in use.
>
> Signed-off-by: Andres Rodriguez <andres.rodriguez@amd.com>
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c b/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
> index a147269..9c08d46 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
> @@ -91,11 +91,11 @@ void kfd_interrupt_exit(struct kfd_dev *kfd)
>         spin_unlock_irqrestore(&kfd->interrupt_lock, flags);
>
>         /*
> -        * Flush_scheduled_work ensures that there are no outstanding
> +        * flush_work ensures that there are no outstanding
>          * work-queue items that will access interrupt_ring. New work items
>          * can't be created because we stopped interrupt handling above.
>          */
> -       flush_scheduled_work();
> +       flush_work(&kfd->interrupt_work);
>
>         kfifo_free(&kfd->ih_fifo);
>  }
> --
> 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

Re: [PATCH 16/16] drm/amdkfd: use a high priority workqueue for IH work

[Oded Gabbay]

On Sat, Oct 21, 2017 at 3:23 AM, Felix Kuehling <Felix.Kuehling@amd.com> wrote:
> From: Andres Rodriguez <andres.rodriguez@amd.com>
>
> In systems under heavy load the IH work may experience significant
> scheduling delays.
>
> Under load + system workqueue:
>     Max Latency: 7.023695 ms
>     Avg Latency: 0.263994 ms
>
> Under load + high priority workqueue:
>     Max Latency: 1.162568 ms
>     Avg Latency: 0.163213 ms
>
> Further work is required to measure the impact of per-cpu settings on IH
> performance.
>
> Signed-off-by: Andres Rodriguez <andres.rodriguez@amd.com>
> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_device.c    | 2 +-
>  drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c | 3 ++-
>  drivers/gpu/drm/amd/amdkfd/kfd_priv.h      | 1 +
>  3 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> index 46049f0..621a3b5 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> @@ -403,7 +403,7 @@ void kgd2kfd_interrupt(struct kfd_dev *kfd, const void *ih_ring_entry)
>         if (kfd->interrupts_active
>             && interrupt_is_wanted(kfd, ih_ring_entry)
>             && enqueue_ih_ring_entry(kfd, ih_ring_entry))
> -               schedule_work(&kfd->interrupt_work);
> +               queue_work(kfd->ih_wq, &kfd->interrupt_work);
>
>         spin_unlock(&kfd->interrupt_lock);
>  }
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c b/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
> index 9c08d46..035c351 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c
> @@ -61,6 +61,7 @@ int kfd_interrupt_init(struct kfd_dev *kfd)
>                 return r;
>         }
>
> +       kfd->ih_wq = alloc_workqueue("KFD IH", WQ_HIGHPRI, 1);
>         spin_lock_init(&kfd->interrupt_lock);
>
>         INIT_WORK(&kfd->interrupt_work, interrupt_wq);
> @@ -95,7 +96,7 @@ void kfd_interrupt_exit(struct kfd_dev *kfd)
>          * work-queue items that will access interrupt_ring. New work items
>          * can't be created because we stopped interrupt handling above.
>          */
> -       flush_work(&kfd->interrupt_work);
> +       flush_workqueue(kfd->ih_wq);
>
>         kfifo_free(&kfd->ih_fifo);
>  }
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> index e8d6c0e..bf29021 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> @@ -184,6 +184,7 @@ struct kfd_dev {
>
>         /* Interrupts */
>         struct kfifo ih_fifo;
> +       struct workqueue_struct *ih_wq;
>         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

Re: [PATCH 09/16] drm/amdkfd: Use wait_queue_t to implement event waiting

[Felix Kuehling]

On 2017-10-25 06:28 AM, Oded Gabbay wrote:
> On Sat, Oct 21, 2017 at 3:23 AM, Felix Kuehling <Felix.Kuehling@amd.com> wrote:
>> Signed-off-by: Kent Russell <kent.russell@amd.com>
>> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
> I would like some more explanation in the commit message about this change.
> - What is the general idea in this solution.
> - Why it was done, i.e. what is the improvement

It's mostly about not duplicating the existing wait queue mechanism. Not
sure how much else there is to say about it.

> Thanks,
> Oded
>
>
>> ---
>>  drivers/gpu/drm/amd/amdkfd/kfd_events.c | 53 +++++++++++----------------------
>>  drivers/gpu/drm/amd/amdkfd/kfd_events.h |  3 +-
>>  2 files changed, 19 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>> index 949b80a..f178248 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>> @@ -33,22 +33,12 @@
>>  #include <linux/device.h>
>>
>>  /*
>> - * A task can only be on a single wait_queue at a time, but we need to support
>> - * waiting on multiple events (any/all).
>> - * Instead of each event simply having a wait_queue with sleeping tasks, it
>> - * has a singly-linked list of tasks.
>> - * A thread that wants to sleep creates an array of these, one for each event
>> - * and adds one to each event's waiter chain.
>> + * Wrapper around wait_queue_entry_t
>>   */
>>  struct kfd_event_waiter {
>> -       struct list_head waiters;
>> -       struct task_struct *sleeping_task;
>> -
>> -       /* Transitions to true when the event this belongs to is signaled. */
>> -       bool activated;
>> -
>> -       /* Event */
>> -       struct kfd_event *event;
>> +       wait_queue_entry_t wait;
>> +       struct kfd_event *event; /* Event to wait for */
>> +       bool activated;          /* Becomes true when event is signaled */
>>  };
>>
>>  /*
>> @@ -344,17 +334,12 @@ void kfd_event_init_process(struct kfd_process *p)
>>
>>  static void destroy_event(struct kfd_process *p, struct kfd_event *ev)
>>  {
>> -       /* Wake up pending waiters. They will return failure */
>> -       while (!list_empty(&ev->waiters)) {
>> -               struct kfd_event_waiter *waiter =
>> -                       list_first_entry(&ev->waiters, struct kfd_event_waiter,
>> -                                        waiters);
>> +       struct kfd_event_waiter *waiter;
>>
>> +       /* Wake up pending waiters. They will return failure */
>> +       list_for_each_entry(waiter, &ev->wq.head, wait.entry)
>>                 waiter->event = NULL;
>> -               /* _init because free_waiters will call list_del */
>> -               list_del_init(&waiter->waiters);
>> -               wake_up_process(waiter->sleeping_task);
>> -       }
>> +       wake_up_all(&ev->wq);
>>
>>         if (ev->signal_page) {
>>                 release_event_notification_slot(ev->signal_page,
>> @@ -424,7 +409,7 @@ int kfd_event_create(struct file *devkfd, struct kfd_process *p,
>>         ev->auto_reset = auto_reset;
>>         ev->signaled = false;
>>
>> -       INIT_LIST_HEAD(&ev->waiters);
>> +       init_waitqueue_head(&ev->wq);
>>
>>         *event_page_offset = 0;
>>
>> @@ -482,19 +467,14 @@ int kfd_event_destroy(struct kfd_process *p, uint32_t event_id)
>>  static void set_event(struct kfd_event *ev)
>>  {
>>         struct kfd_event_waiter *waiter;
>> -       struct kfd_event_waiter *next;
>>
>>         /* Auto reset if the list is non-empty and we're waking someone. */
>> -       ev->signaled = !ev->auto_reset || list_empty(&ev->waiters);
>> +       ev->signaled = !ev->auto_reset || !waitqueue_active(&ev->wq);
> This line gives a warning in the checkpatch script. Per Linus's patch
> (http://www.spinics.net/lists/mm-commits/msg111660.html), please add a
> comment explaining the use of waitqueue_active and why its not racy
> with the caller.

There is a comment, but checkpatch still complains. I can add more to
the comment, but not sure that will fix the warning.

Regards,
  Felix

>
> Oded
>
>> -       list_for_each_entry_safe(waiter, next, &ev->waiters, waiters) {
>> +       list_for_each_entry(waiter, &ev->wq.head, wait.entry)
>>                 waiter->activated = true;
>>
>> -               /* _init because free_waiters will call list_del */
>> -               list_del_init(&waiter->waiters);
>> -
>> -               wake_up_process(waiter->sleeping_task);
>> -       }
>> +       wake_up_all(&ev->wq);
>>  }
>>
>>  /* Assumes that p is current. */
>> @@ -614,8 +594,7 @@ static struct kfd_event_waiter *alloc_event_waiters(uint32_t num_events)
>>                                         GFP_KERNEL);
>>
>>         for (i = 0; (event_waiters) && (i < num_events) ; i++) {
>> -               INIT_LIST_HEAD(&event_waiters[i].waiters);
>> -               event_waiters[i].sleeping_task = current;
>> +               init_wait(&event_waiters[i].wait);
>>                 event_waiters[i].activated = false;
>>         }
>>
>> @@ -646,7 +625,7 @@ static void init_event_waiter_add_to_waitlist(struct kfd_event_waiter *waiter)
>>          * wait on this event.
>>          */
>>         if (!waiter->activated)
>> -               list_add(&waiter->waiters, &ev->waiters);
>> +               add_wait_queue(&ev->wq, &waiter->wait);
>>  }
>>
>>  /* test_event_condition - Test condition of events being waited for
>> @@ -736,7 +715,9 @@ static void free_waiters(uint32_t num_events, struct kfd_event_waiter *waiters)
>>         uint32_t i;
>>
>>         for (i = 0; i < num_events; i++)
>> -               list_del(&waiters[i].waiters);
>> +               if (waiters[i].event)
>> +                       remove_wait_queue(&waiters[i].event->wq,
>> +                                         &waiters[i].wait);
>>
>>         kfree(waiters);
>>  }
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.h b/drivers/gpu/drm/amd/amdkfd/kfd_events.h
>> index 28f6838..96f9122 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_events.h
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.h
>> @@ -27,6 +27,7 @@
>>  #include <linux/hashtable.h>
>>  #include <linux/types.h>
>>  #include <linux/list.h>
>> +#include <linux/wait.h>
>>  #include "kfd_priv.h"
>>  #include <uapi/linux/kfd_ioctl.h>
>>
>> @@ -56,7 +57,7 @@ struct kfd_event {
>>
>>         int type;
>>
>> -       struct list_head waiters; /* List of kfd_event_waiter by waiters. */
>> +       wait_queue_head_t wq; /* List of event waiters. */
>>
>>         /* Only for signal events. */
>>         struct signal_page *signal_page;
>> --
>> 2.7.4
>>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 00/16] KFD interrupt and signal event handling improvements

[Felix Kuehling]

On 2017-10-25 02:57 AM, Oded Gabbay wrote:
> On Sat, Oct 21, 2017 at 3:23 AM, Felix Kuehling <Felix.Kuehling@amd.com> wrote:
>> This patch series improves interrupt handling latency, signal event
>> processing overhead and replaces some custom data structures with
>> standard kernel data structures (idr, kfifo, waitqueue).
>>
>> It also increases the capacity of the number of signals that can be
>> processed from 256 to 4096. This breaks ancient versions of the Thunk
>> that support only 256 signal events. The current WIP-version on github
>> supports both sizes. If support for ancient Thunks is considered
>> important, this could be fixed by allowing mappings that are smaller
>> than 4096 signals, and limiting the number of signals per process
>> depending on the size of the mapped events page.
> Hi Felix,
> I don't have enough data to say whether it is important in terms of
> whether there are actual users out there.
> I *can* say that the no.1 rule of the kernel is *never break userspace*
>
> Even if I want to take the relevant patches, I can't really do it,
> especially since you mentioned that there is a possible fix for it.

Right. FWIW, the only patch that causes the problem is your "increase
limit of signal events to 4096..." patch. ;) The rest should be safe.
I'll fix up your patch to maintain backwards compatibility.

Regards,
  Felix

>
> I know its a bit more work, but I suggest you add an additional patch
> that supports the old thunk.
>
> Thanks,
> Oded
>
>> Andres Rodriguez (4):
>>   drm/amdkfd: use standard kernel kfifo for IH
>>   drm/amdkfd: increase IH num entries to 8192
>>   drm/amdkfd: wait only for IH work on IH exit
>>   drm/amdkfd: use a high priority workqueue for IH work
>>
>> Besar Wicaksono (1):
>>   drm/amdkfd: Add SDMA trap src id to the KFD isr wanted list
>>
>> Felix Kuehling (8):
>>   drm/amdkfd: Don't dereference kfd_process.mm
>>   drm/amdkfd: Clean up kfd_wait_on_events
>>   drm/amdkfd: Fix event destruction with pending waiters
>>   drm/amdkfd: remove redundant kfd_event_waiter.input_index
>>   drm/amdkfd: Use wait_queue_t to implement event waiting
>>   drm/amdkfd: Simplify events page allocator
>>   drm/amdkfd: Simplify event ID and signal slot management
>>   drm/amdkfd: Use IH context ID for signal lookup
>>
>> Oded Gabbay (1):
>>   drm/amdkfd: increase limit of signal events to 4096 per process
>>
>> Sean Keely (2):
>>   drm/amdkfd: Short cut for kfd_wait_on_events without waiting
>>   drm/amdkfd: Fix scheduler race in kfd_wait_on_events sleep loop
>>
>>  drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c |   8 +-
>>  drivers/gpu/drm/amd/amdkfd/cik_int.h             |   3 +-
>>  drivers/gpu/drm/amd/amdkfd/kfd_chardev.c         |   5 +-
>>  drivers/gpu/drm/amd/amdkfd/kfd_device.c          |   2 +-
>>  drivers/gpu/drm/amd/amdkfd/kfd_events.c          | 588 ++++++++++-------------
>>  drivers/gpu/drm/amd/amdkfd/kfd_events.h          |  18 +-
>>  drivers/gpu/drm/amd/amdkfd/kfd_interrupt.c       |  83 ++--
>>  drivers/gpu/drm/amd/amdkfd/kfd_priv.h            |  32 +-
>>  drivers/gpu/drm/amd/amdkfd/kfd_process.c         |   6 +-
>>  include/uapi/linux/kfd_ioctl.h                   |   2 +-
>>  10 files changed, 332 insertions(+), 415 deletions(-)
>>
>> --
>> 2.7.4
>>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 02/16] drm/amdkfd: Don't dereference kfd_process.mm

[Christian König]

Am 21.10.2017 um 02:23 schrieb Felix Kuehling:
> The kfd_process doesn't own a reference to the mm_struct, so it can
> disappear without warning even while the kfd_process still exists.
> In fact, the delayed kfd_process teardown is triggered by an MMU
> notifier when the mm_struct is destroyed. Permanently holding a
> reference to the mm_struct would prevent this from happening.
>
> Therefore, avoid dereferencing the kfd_process.mm pointer and make
> it opaque. Use get_task_mm to get a temporary reference to the mm
> when it's needed.

Actually that patch is unnecessary.

Process tear down (and calling the MMU release callback) is triggered 
when mm_struct->mm_users reaches zero.

The mm_struct is freed up when mm_struct->mm_count becomes zero.

So what you can do is grab a reference to the mm_struct with mmgrab() 
and still be notified by process tear down.

Regards,
Christian.

>
> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_events.c  | 19 +++++++++++++++----
>   drivers/gpu/drm/amd/amdkfd/kfd_priv.h    |  7 ++++++-
>   drivers/gpu/drm/amd/amdkfd/kfd_process.c |  6 +++++-
>   3 files changed, 26 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> index 944abfa..61ce547 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> @@ -24,8 +24,8 @@
>   #include <linux/slab.h>
>   #include <linux/types.h>
>   #include <linux/sched/signal.h>
> +#include <linux/sched/mm.h>
>   #include <linux/uaccess.h>
> -#include <linux/mm.h>
>   #include <linux/mman.h>
>   #include <linux/memory.h>
>   #include "kfd_priv.h"
> @@ -904,14 +904,24 @@ void kfd_signal_iommu_event(struct kfd_dev *dev, unsigned int pasid,
>   	 * running so the lookup function returns a locked process.
>   	 */
>   	struct kfd_process *p = kfd_lookup_process_by_pasid(pasid);
> +	struct mm_struct *mm;
>   
>   	if (!p)
>   		return; /* Presumably process exited. */
>   
> +	/* Take a safe reference to the mm_struct, which may otherwise
> +	 * disappear even while the kfd_process is still referenced.
> +	 */
> +	mm = get_task_mm(p->lead_thread);
> +	if (!mm) {
> +		mutex_unlock(&p->mutex);
> +		return; /* Process is exiting */
> +	}
> +
>   	memset(&memory_exception_data, 0, sizeof(memory_exception_data));
>   
> -	down_read(&p->mm->mmap_sem);
> -	vma = find_vma(p->mm, address);
> +	down_read(&mm->mmap_sem);
> +	vma = find_vma(mm, address);
>   
>   	memory_exception_data.gpu_id = dev->id;
>   	memory_exception_data.va = address;
> @@ -937,7 +947,8 @@ void kfd_signal_iommu_event(struct kfd_dev *dev, unsigned int pasid,
>   		}
>   	}
>   
> -	up_read(&p->mm->mmap_sem);
> +	up_read(&mm->mmap_sem);
> +	mmput(mm);
>   
>   	mutex_lock(&p->event_mutex);
>   
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> index 7d86ec9..1a483a7 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> @@ -494,7 +494,12 @@ struct kfd_process {
>   	 */
>   	struct hlist_node kfd_processes;
>   
> -	struct mm_struct *mm;
> +	/*
> +	 * Opaque pointer to mm_struct. We don't hold a reference to
> +	 * it so it should never be dereferenced from here. This is
> +	 * only used for looking up processes by their mm.
> +	 */
> +	void *mm;
>   
>   	struct mutex mutex;
>   
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> index 3ccb3b5..21d27e5 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> @@ -200,7 +200,11 @@ static void kfd_process_destroy_delayed(struct rcu_head *rcu)
>   	struct kfd_process *p;
>   
>   	p = container_of(rcu, struct kfd_process, rcu);
> -	WARN_ON(atomic_read(&p->mm->mm_count) <= 0);
> +	/*
> +	 * This cast should be safe here because we grabbed a
> +	 * reference to the mm in kfd_process_notifier_release
> +	 */
> +	WARN_ON(atomic_read(&((struct mm_struct *)p->mm)->mm_count) <= 0);
>   
>   	mmdrop(p->mm);
>   


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 02/16] drm/amdkfd: Don't dereference kfd_process.mm

[Felix Kuehling]

On 2017-10-26 03:33 AM, Christian König wrote:
> Am 21.10.2017 um 02:23 schrieb Felix Kuehling:
>> The kfd_process doesn't own a reference to the mm_struct, so it can
>> disappear without warning even while the kfd_process still exists.
>> In fact, the delayed kfd_process teardown is triggered by an MMU
>> notifier when the mm_struct is destroyed. Permanently holding a
>> reference to the mm_struct would prevent this from happening.
>>
>> Therefore, avoid dereferencing the kfd_process.mm pointer and make
>> it opaque. Use get_task_mm to get a temporary reference to the mm
>> when it's needed.
>
> Actually that patch is unnecessary.
>
> Process tear down (and calling the MMU release callback) is triggered
> when mm_struct->mm_users reaches zero.
>
> The mm_struct is freed up when mm_struct->mm_count becomes zero.
>
> So what you can do is grab a reference to the mm_struct with mmgrab()
> and still be notified by process tear down.

Hmm, the mm_struct has two different reference counters. So if I grab
the right type of reference it would work. Either way, one of two
changes is needed:

  * Take a permanent reference to the mm-struct, or
  * Don't dereference the mm pointer because I'm not holding a reference

IMO, KFD doesn't need to hold a reference to the mm_struct permanently.
So I believe my change still makes sense. I should probably just remove
the comment "Permanently holding a reference to the mm_struct would
prevent ...". Like you say, that's not accurate.

Regards,
  Felix

>
> Regards,
> Christian.
>
>>
>> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdkfd/kfd_events.c  | 19 +++++++++++++++----
>>   drivers/gpu/drm/amd/amdkfd/kfd_priv.h    |  7 ++++++-
>>   drivers/gpu/drm/amd/amdkfd/kfd_process.c |  6 +++++-
>>   3 files changed, 26 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>> b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>> index 944abfa..61ce547 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>> @@ -24,8 +24,8 @@
>>   #include <linux/slab.h>
>>   #include <linux/types.h>
>>   #include <linux/sched/signal.h>
>> +#include <linux/sched/mm.h>
>>   #include <linux/uaccess.h>
>> -#include <linux/mm.h>
>>   #include <linux/mman.h>
>>   #include <linux/memory.h>
>>   #include "kfd_priv.h"
>> @@ -904,14 +904,24 @@ void kfd_signal_iommu_event(struct kfd_dev
>> *dev, unsigned int pasid,
>>        * running so the lookup function returns a locked process.
>>        */
>>       struct kfd_process *p = kfd_lookup_process_by_pasid(pasid);
>> +    struct mm_struct *mm;
>>         if (!p)
>>           return; /* Presumably process exited. */
>>   +    /* Take a safe reference to the mm_struct, which may otherwise
>> +     * disappear even while the kfd_process is still referenced.
>> +     */
>> +    mm = get_task_mm(p->lead_thread);
>> +    if (!mm) {
>> +        mutex_unlock(&p->mutex);
>> +        return; /* Process is exiting */
>> +    }
>> +
>>       memset(&memory_exception_data, 0, sizeof(memory_exception_data));
>>   -    down_read(&p->mm->mmap_sem);
>> -    vma = find_vma(p->mm, address);
>> +    down_read(&mm->mmap_sem);
>> +    vma = find_vma(mm, address);
>>         memory_exception_data.gpu_id = dev->id;
>>       memory_exception_data.va = address;
>> @@ -937,7 +947,8 @@ void kfd_signal_iommu_event(struct kfd_dev *dev,
>> unsigned int pasid,
>>           }
>>       }
>>   -    up_read(&p->mm->mmap_sem);
>> +    up_read(&mm->mmap_sem);
>> +    mmput(mm);
>>         mutex_lock(&p->event_mutex);
>>   diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>> b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>> index 7d86ec9..1a483a7 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>> @@ -494,7 +494,12 @@ struct kfd_process {
>>        */
>>       struct hlist_node kfd_processes;
>>   -    struct mm_struct *mm;
>> +    /*
>> +     * Opaque pointer to mm_struct. We don't hold a reference to
>> +     * it so it should never be dereferenced from here. This is
>> +     * only used for looking up processes by their mm.
>> +     */
>> +    void *mm;
>>         struct mutex mutex;
>>   diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>> b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>> index 3ccb3b5..21d27e5 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>> @@ -200,7 +200,11 @@ static void kfd_process_destroy_delayed(struct
>> rcu_head *rcu)
>>       struct kfd_process *p;
>>         p = container_of(rcu, struct kfd_process, rcu);
>> -    WARN_ON(atomic_read(&p->mm->mm_count) <= 0);
>> +    /*
>> +     * This cast should be safe here because we grabbed a
>> +     * reference to the mm in kfd_process_notifier_release
>> +     */
>> +    WARN_ON(atomic_read(&((struct mm_struct *)p->mm)->mm_count) <= 0);
>>         mmdrop(p->mm);
>>   
>
>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 02/16] drm/amdkfd: Don't dereference kfd_process.mm

[Christian König]

Am 26.10.2017 um 18:47 schrieb Felix Kuehling:
> On 2017-10-26 03:33 AM, Christian König wrote:
>> Am 21.10.2017 um 02:23 schrieb Felix Kuehling:
>>> The kfd_process doesn't own a reference to the mm_struct, so it can
>>> disappear without warning even while the kfd_process still exists.
>>> In fact, the delayed kfd_process teardown is triggered by an MMU
>>> notifier when the mm_struct is destroyed. Permanently holding a
>>> reference to the mm_struct would prevent this from happening.
>>>
>>> Therefore, avoid dereferencing the kfd_process.mm pointer and make
>>> it opaque. Use get_task_mm to get a temporary reference to the mm
>>> when it's needed.
>> Actually that patch is unnecessary.
>>
>> Process tear down (and calling the MMU release callback) is triggered
>> when mm_struct->mm_users reaches zero.
>>
>> The mm_struct is freed up when mm_struct->mm_count becomes zero.
>>
>> So what you can do is grab a reference to the mm_struct with mmgrab()
>> and still be notified by process tear down.
> Hmm, the mm_struct has two different reference counters. So if I grab
> the right type of reference it would work. Either way, one of two
> changes is needed:
>
>    * Take a permanent reference to the mm-struct, or
>    * Don't dereference the mm pointer because I'm not holding a reference
>
> IMO, KFD doesn't need to hold a reference to the mm_struct permanently.

Ah! From the patch description I was assuming that you took a reference 
(but the wrong one) and tried to fix a memory leak with that approach.

> So I believe my change still makes sense. I should probably just remove
> the comment "Permanently holding a reference to the mm_struct would
> prevent ...". Like you say, that's not accurate.

Yeah, good idea.

But now reading the patch there is something else which I stumbled over:
> +    /*
> +     * This cast should be safe here because we grabbed a
> +     * reference to the mm in kfd_process_notifier_release
> +     */
> +    WARN_ON(atomic_read(&((struct mm_struct *)p->mm)->mm_count) <= 0);
>          mmdrop(p->mm);
Well that isn't good coding style. You shouldn't obfuscate what pointer 
it is by changing it to "void*", but rather set it to NULL as soon as 
you know that it is stale.

Additional to that it is certainly not job of the driver to warn on a 
run over mm_count.

Regards,
Christian.

>
> Regards,
>    Felix
>
>> Regards,
>> Christian.
>>
>>> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
>>> ---
>>>    drivers/gpu/drm/amd/amdkfd/kfd_events.c  | 19 +++++++++++++++----
>>>    drivers/gpu/drm/amd/amdkfd/kfd_priv.h    |  7 ++++++-
>>>    drivers/gpu/drm/amd/amdkfd/kfd_process.c |  6 +++++-
>>>    3 files changed, 26 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>>> b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>>> index 944abfa..61ce547 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>>> @@ -24,8 +24,8 @@
>>>    #include <linux/slab.h>
>>>    #include <linux/types.h>
>>>    #include <linux/sched/signal.h>
>>> +#include <linux/sched/mm.h>
>>>    #include <linux/uaccess.h>
>>> -#include <linux/mm.h>
>>>    #include <linux/mman.h>
>>>    #include <linux/memory.h>
>>>    #include "kfd_priv.h"
>>> @@ -904,14 +904,24 @@ void kfd_signal_iommu_event(struct kfd_dev
>>> *dev, unsigned int pasid,
>>>         * running so the lookup function returns a locked process.
>>>         */
>>>        struct kfd_process *p = kfd_lookup_process_by_pasid(pasid);
>>> +    struct mm_struct *mm;
>>>          if (!p)
>>>            return; /* Presumably process exited. */
>>>    +    /* Take a safe reference to the mm_struct, which may otherwise
>>> +     * disappear even while the kfd_process is still referenced.
>>> +     */
>>> +    mm = get_task_mm(p->lead_thread);
>>> +    if (!mm) {
>>> +        mutex_unlock(&p->mutex);
>>> +        return; /* Process is exiting */
>>> +    }
>>> +
>>>        memset(&memory_exception_data, 0, sizeof(memory_exception_data));
>>>    -    down_read(&p->mm->mmap_sem);
>>> -    vma = find_vma(p->mm, address);
>>> +    down_read(&mm->mmap_sem);
>>> +    vma = find_vma(mm, address);
>>>          memory_exception_data.gpu_id = dev->id;
>>>        memory_exception_data.va = address;
>>> @@ -937,7 +947,8 @@ void kfd_signal_iommu_event(struct kfd_dev *dev,
>>> unsigned int pasid,
>>>            }
>>>        }
>>>    -    up_read(&p->mm->mmap_sem);
>>> +    up_read(&mm->mmap_sem);
>>> +    mmput(mm);
>>>          mutex_lock(&p->event_mutex);
>>>    diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>> b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>> index 7d86ec9..1a483a7 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>> @@ -494,7 +494,12 @@ struct kfd_process {
>>>         */
>>>        struct hlist_node kfd_processes;
>>>    -    struct mm_struct *mm;
>>> +    /*
>>> +     * Opaque pointer to mm_struct. We don't hold a reference to
>>> +     * it so it should never be dereferenced from here. This is
>>> +     * only used for looking up processes by their mm.
>>> +     */
>>> +    void *mm;
>>>          struct mutex mutex;
>>>    diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>> b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>> index 3ccb3b5..21d27e5 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>> @@ -200,7 +200,11 @@ static void kfd_process_destroy_delayed(struct
>>> rcu_head *rcu)
>>>        struct kfd_process *p;
>>>          p = container_of(rcu, struct kfd_process, rcu);
>>> -    WARN_ON(atomic_read(&p->mm->mm_count) <= 0);
>>> +    /*
>>> +     * This cast should be safe here because we grabbed a
>>> +     * reference to the mm in kfd_process_notifier_release
>>> +     */
>>> +    WARN_ON(atomic_read(&((struct mm_struct *)p->mm)->mm_count) <= 0);
>>>          mmdrop(p->mm);
>>>    
>>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 02/16] drm/amdkfd: Don't dereference kfd_process.mm

[Felix Kuehling]

On 2017-10-26 02:11 PM, Christian König wrote:
> But now reading the patch there is something else which I stumbled over:
>> -    WARN_ON(atomic_read(&p->mm->mm_count) <= 0);
>> +    /*
>> +     * This cast should be safe here because we grabbed a
>> +     * reference to the mm in kfd_process_notifier_release
>> +     */
>> +    WARN_ON(atomic_read(&((struct mm_struct *)p->mm)->mm_count) <= 0);
>>          mmdrop(p->mm);
> Well that isn't good coding style. You shouldn't obfuscate what
> pointer it is by changing it to "void*", but rather set it to NULL as
> soon as you know that it is stale.
>
> Additional to that it is certainly not job of the driver to warn on a
> run over mm_count.

Yeah. We don't have this in our current staging branch. The whole
process teardown has changed quite a bit. I just fixed this up to make
it work with current upstream.

If you prefer, I could just remove the WARN_ON.

Regards,
  Felix

>
> Regards,
> Christian.
>
>>
>> Regards,
>>    Felix
>>
>>> Regards,
>>> Christian.
>>>
>>>> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
>>>> ---
>>>>    drivers/gpu/drm/amd/amdkfd/kfd_events.c  | 19 +++++++++++++++----
>>>>    drivers/gpu/drm/amd/amdkfd/kfd_priv.h    |  7 ++++++-
>>>>    drivers/gpu/drm/amd/amdkfd/kfd_process.c |  6 +++++-
>>>>    3 files changed, 26 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>>>> b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>>>> index 944abfa..61ce547 100644
>>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>>>> @@ -24,8 +24,8 @@
>>>>    #include <linux/slab.h>
>>>>    #include <linux/types.h>
>>>>    #include <linux/sched/signal.h>
>>>> +#include <linux/sched/mm.h>
>>>>    #include <linux/uaccess.h>
>>>> -#include <linux/mm.h>
>>>>    #include <linux/mman.h>
>>>>    #include <linux/memory.h>
>>>>    #include "kfd_priv.h"
>>>> @@ -904,14 +904,24 @@ void kfd_signal_iommu_event(struct kfd_dev
>>>> *dev, unsigned int pasid,
>>>>         * running so the lookup function returns a locked process.
>>>>         */
>>>>        struct kfd_process *p = kfd_lookup_process_by_pasid(pasid);
>>>> +    struct mm_struct *mm;
>>>>          if (!p)
>>>>            return; /* Presumably process exited. */
>>>>    +    /* Take a safe reference to the mm_struct, which may otherwise
>>>> +     * disappear even while the kfd_process is still referenced.
>>>> +     */
>>>> +    mm = get_task_mm(p->lead_thread);
>>>> +    if (!mm) {
>>>> +        mutex_unlock(&p->mutex);
>>>> +        return; /* Process is exiting */
>>>> +    }
>>>> +
>>>>        memset(&memory_exception_data, 0,
>>>> sizeof(memory_exception_data));
>>>>    -    down_read(&p->mm->mmap_sem);
>>>> -    vma = find_vma(p->mm, address);
>>>> +    down_read(&mm->mmap_sem);
>>>> +    vma = find_vma(mm, address);
>>>>          memory_exception_data.gpu_id = dev->id;
>>>>        memory_exception_data.va = address;
>>>> @@ -937,7 +947,8 @@ void kfd_signal_iommu_event(struct kfd_dev *dev,
>>>> unsigned int pasid,
>>>>            }
>>>>        }
>>>>    -    up_read(&p->mm->mmap_sem);
>>>> +    up_read(&mm->mmap_sem);
>>>> +    mmput(mm);
>>>>          mutex_lock(&p->event_mutex);
>>>>    diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>>> b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>>> index 7d86ec9..1a483a7 100644
>>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>>> @@ -494,7 +494,12 @@ struct kfd_process {
>>>>         */
>>>>        struct hlist_node kfd_processes;
>>>>    -    struct mm_struct *mm;
>>>> +    /*
>>>> +     * Opaque pointer to mm_struct. We don't hold a reference to
>>>> +     * it so it should never be dereferenced from here. This is
>>>> +     * only used for looking up processes by their mm.
>>>> +     */
>>>> +    void *mm;
>>>>          struct mutex mutex;
>>>>    diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>>> b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>>> index 3ccb3b5..21d27e5 100644
>>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>>> @@ -200,7 +200,11 @@ static void kfd_process_destroy_delayed(struct
>>>> rcu_head *rcu)
>>>>        struct kfd_process *p;
>>>>          p = container_of(rcu, struct kfd_process, rcu);
>>>> -    WARN_ON(atomic_read(&p->mm->mm_count) <= 0);
>>>> +    /*
>>>> +     * This cast should be safe here because we grabbed a
>>>> +     * reference to the mm in kfd_process_notifier_release
>>>> +     */
>>>> +    WARN_ON(atomic_read(&((struct mm_struct *)p->mm)->mm_count) <=
>>>> 0);
>>>>          mmdrop(p->mm);
>>>>    
>>>
>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 02/16] drm/amdkfd: Don't dereference kfd_process.mm

[Christian König]

Am 26.10.2017 um 20:54 schrieb Felix Kuehling:
> On 2017-10-26 02:11 PM, Christian König wrote:
>> But now reading the patch there is something else which I stumbled over:
>>> -    WARN_ON(atomic_read(&p->mm->mm_count) <= 0);
>>> +    /*
>>> +     * This cast should be safe here because we grabbed a
>>> +     * reference to the mm in kfd_process_notifier_release
>>> +     */
>>> +    WARN_ON(atomic_read(&((struct mm_struct *)p->mm)->mm_count) <= 0);
>>>           mmdrop(p->mm);
>> Well that isn't good coding style. You shouldn't obfuscate what
>> pointer it is by changing it to "void*", but rather set it to NULL as
>> soon as you know that it is stale.
>>
>> Additional to that it is certainly not job of the driver to warn on a
>> run over mm_count.
> Yeah. We don't have this in our current staging branch. The whole
> process teardown has changed quite a bit. I just fixed this up to make
> it work with current upstream.
>
> If you prefer, I could just remove the WARN_ON.

That sounds like a good idea to me, yes.

Regards,
Christian.

>
> Regards,
>    Felix
>
>> Regards,
>> Christian.
>>
>>> Regards,
>>>     Felix
>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
>>>>> ---
>>>>>     drivers/gpu/drm/amd/amdkfd/kfd_events.c  | 19 +++++++++++++++----
>>>>>     drivers/gpu/drm/amd/amdkfd/kfd_priv.h    |  7 ++++++-
>>>>>     drivers/gpu/drm/amd/amdkfd/kfd_process.c |  6 +++++-
>>>>>     3 files changed, 26 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>>>>> b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>>>>> index 944abfa..61ce547 100644
>>>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>>>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>>>>> @@ -24,8 +24,8 @@
>>>>>     #include <linux/slab.h>
>>>>>     #include <linux/types.h>
>>>>>     #include <linux/sched/signal.h>
>>>>> +#include <linux/sched/mm.h>
>>>>>     #include <linux/uaccess.h>
>>>>> -#include <linux/mm.h>
>>>>>     #include <linux/mman.h>
>>>>>     #include <linux/memory.h>
>>>>>     #include "kfd_priv.h"
>>>>> @@ -904,14 +904,24 @@ void kfd_signal_iommu_event(struct kfd_dev
>>>>> *dev, unsigned int pasid,
>>>>>          * running so the lookup function returns a locked process.
>>>>>          */
>>>>>         struct kfd_process *p = kfd_lookup_process_by_pasid(pasid);
>>>>> +    struct mm_struct *mm;
>>>>>           if (!p)
>>>>>             return; /* Presumably process exited. */
>>>>>     +    /* Take a safe reference to the mm_struct, which may otherwise
>>>>> +     * disappear even while the kfd_process is still referenced.
>>>>> +     */
>>>>> +    mm = get_task_mm(p->lead_thread);
>>>>> +    if (!mm) {
>>>>> +        mutex_unlock(&p->mutex);
>>>>> +        return; /* Process is exiting */
>>>>> +    }
>>>>> +
>>>>>         memset(&memory_exception_data, 0,
>>>>> sizeof(memory_exception_data));
>>>>>     -    down_read(&p->mm->mmap_sem);
>>>>> -    vma = find_vma(p->mm, address);
>>>>> +    down_read(&mm->mmap_sem);
>>>>> +    vma = find_vma(mm, address);
>>>>>           memory_exception_data.gpu_id = dev->id;
>>>>>         memory_exception_data.va = address;
>>>>> @@ -937,7 +947,8 @@ void kfd_signal_iommu_event(struct kfd_dev *dev,
>>>>> unsigned int pasid,
>>>>>             }
>>>>>         }
>>>>>     -    up_read(&p->mm->mmap_sem);
>>>>> +    up_read(&mm->mmap_sem);
>>>>> +    mmput(mm);
>>>>>           mutex_lock(&p->event_mutex);
>>>>>     diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>>>> b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>>>> index 7d86ec9..1a483a7 100644
>>>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>>>> @@ -494,7 +494,12 @@ struct kfd_process {
>>>>>          */
>>>>>         struct hlist_node kfd_processes;
>>>>>     -    struct mm_struct *mm;
>>>>> +    /*
>>>>> +     * Opaque pointer to mm_struct. We don't hold a reference to
>>>>> +     * it so it should never be dereferenced from here. This is
>>>>> +     * only used for looking up processes by their mm.
>>>>> +     */
>>>>> +    void *mm;
>>>>>           struct mutex mutex;
>>>>>     diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>>>> b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>>>> index 3ccb3b5..21d27e5 100644
>>>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>>>> @@ -200,7 +200,11 @@ static void kfd_process_destroy_delayed(struct
>>>>> rcu_head *rcu)
>>>>>         struct kfd_process *p;
>>>>>           p = container_of(rcu, struct kfd_process, rcu);
>>>>> -    WARN_ON(atomic_read(&p->mm->mm_count) <= 0);
>>>>> +    /*
>>>>> +     * This cast should be safe here because we grabbed a
>>>>> +     * reference to the mm in kfd_process_notifier_release
>>>>> +     */
>>>>> +    WARN_ON(atomic_read(&((struct mm_struct *)p->mm)->mm_count) <=
>>>>> 0);
>>>>>           mmdrop(p->mm);
>>>>>     
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 02/16] drm/amdkfd: Don't dereference kfd_process.mm

[Christian König]

Am 27.10.2017 um 09:22 schrieb Christian König:
> Am 26.10.2017 um 20:54 schrieb Felix Kuehling:
>> On 2017-10-26 02:11 PM, Christian König wrote:
>>> But now reading the patch there is something else which I stumbled 
>>> over:
>>>> - WARN_ON(atomic_read(&p->mm->mm_count) <= 0);
>>>> +    /*
>>>> +     * This cast should be safe here because we grabbed a
>>>> +     * reference to the mm in kfd_process_notifier_release
>>>> +     */
>>>> +    WARN_ON(atomic_read(&((struct mm_struct *)p->mm)->mm_count) <= 
>>>> 0);
>>>>           mmdrop(p->mm);
>>> Well that isn't good coding style. You shouldn't obfuscate what
>>> pointer it is by changing it to "void*", but rather set it to NULL as
>>> soon as you know that it is stale.
>>>
>>> Additional to that it is certainly not job of the driver to warn on a
>>> run over mm_count.
>> Yeah. We don't have this in our current staging branch. The whole
>> process teardown has changed quite a bit. I just fixed this up to make
>> it work with current upstream.
>>
>> If you prefer, I could just remove the WARN_ON.
>
> That sounds like a good idea to me, yes.

Wait a second, now that I though once more about it what do you mean 
with this comment:
> +    /*
> +     * Opaque pointer to mm_struct. We don't hold a reference to
> +     * it so it should never be dereferenced from here. This is
> +     * only used for looking up processes by their mm.
> +     */ 
E.g. what means looking up the processes by their mm?

Do you use a hashtable or something like this and then check if you got 
the correct mm structure by comparing the pointers?

If that is the case then you should certainly set p->mm to NULL after 
mmdrop(p->mm).

Otherwise it can happen that a new mm structure is allocated at the same 
location as the old one and you run into problems because the kernel 
driver accidentally uses the wrong kfd_process structure.

Regards,
Christian.

>
> Regards,
> Christian.
>
>>
>> Regards,
>>    Felix
>>
>>> Regards,
>>> Christian.
>>>
>>>> Regards,
>>>>     Felix
>>>>
>>>>> Regards,
>>>>> Christian.
>>>>>
>>>>>> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
>>>>>> ---
>>>>>>     drivers/gpu/drm/amd/amdkfd/kfd_events.c  | 19 
>>>>>> +++++++++++++++----
>>>>>>     drivers/gpu/drm/amd/amdkfd/kfd_priv.h    |  7 ++++++-
>>>>>>     drivers/gpu/drm/amd/amdkfd/kfd_process.c |  6 +++++-
>>>>>>     3 files changed, 26 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>>>>>> b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>>>>>> index 944abfa..61ce547 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>>>>>> @@ -24,8 +24,8 @@
>>>>>>     #include <linux/slab.h>
>>>>>>     #include <linux/types.h>
>>>>>>     #include <linux/sched/signal.h>
>>>>>> +#include <linux/sched/mm.h>
>>>>>>     #include <linux/uaccess.h>
>>>>>> -#include <linux/mm.h>
>>>>>>     #include <linux/mman.h>
>>>>>>     #include <linux/memory.h>
>>>>>>     #include "kfd_priv.h"
>>>>>> @@ -904,14 +904,24 @@ void kfd_signal_iommu_event(struct kfd_dev
>>>>>> *dev, unsigned int pasid,
>>>>>>          * running so the lookup function returns a locked process.
>>>>>>          */
>>>>>>         struct kfd_process *p = kfd_lookup_process_by_pasid(pasid);
>>>>>> +    struct mm_struct *mm;
>>>>>>           if (!p)
>>>>>>             return; /* Presumably process exited. */
>>>>>>     +    /* Take a safe reference to the mm_struct, which may 
>>>>>> otherwise
>>>>>> +     * disappear even while the kfd_process is still referenced.
>>>>>> +     */
>>>>>> +    mm = get_task_mm(p->lead_thread);
>>>>>> +    if (!mm) {
>>>>>> +        mutex_unlock(&p->mutex);
>>>>>> +        return; /* Process is exiting */
>>>>>> +    }
>>>>>> +
>>>>>>         memset(&memory_exception_data, 0,
>>>>>> sizeof(memory_exception_data));
>>>>>>     -    down_read(&p->mm->mmap_sem);
>>>>>> -    vma = find_vma(p->mm, address);
>>>>>> +    down_read(&mm->mmap_sem);
>>>>>> +    vma = find_vma(mm, address);
>>>>>>           memory_exception_data.gpu_id = dev->id;
>>>>>>         memory_exception_data.va = address;
>>>>>> @@ -937,7 +947,8 @@ void kfd_signal_iommu_event(struct kfd_dev *dev,
>>>>>> unsigned int pasid,
>>>>>>             }
>>>>>>         }
>>>>>>     -    up_read(&p->mm->mmap_sem);
>>>>>> +    up_read(&mm->mmap_sem);
>>>>>> +    mmput(mm);
>>>>>>           mutex_lock(&p->event_mutex);
>>>>>>     diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>>>>> b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>>>>> index 7d86ec9..1a483a7 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>>>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>>>>> @@ -494,7 +494,12 @@ struct kfd_process {
>>>>>>          */
>>>>>>         struct hlist_node kfd_processes;
>>>>>>     -    struct mm_struct *mm;
>>>>>> +    /*
>>>>>> +     * Opaque pointer to mm_struct. We don't hold a reference to
>>>>>> +     * it so it should never be dereferenced from here. This is
>>>>>> +     * only used for looking up processes by their mm.
>>>>>> +     */
>>>>>> +    void *mm;
>>>>>>           struct mutex mutex;
>>>>>>     diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>>>>> b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>>>>> index 3ccb3b5..21d27e5 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>>>>> @@ -200,7 +200,11 @@ static void kfd_process_destroy_delayed(struct
>>>>>> rcu_head *rcu)
>>>>>>         struct kfd_process *p;
>>>>>>           p = container_of(rcu, struct kfd_process, rcu);
>>>>>> -    WARN_ON(atomic_read(&p->mm->mm_count) <= 0);
>>>>>> +    /*
>>>>>> +     * This cast should be safe here because we grabbed a
>>>>>> +     * reference to the mm in kfd_process_notifier_release
>>>>>> +     */
>>>>>> +    WARN_ON(atomic_read(&((struct mm_struct *)p->mm)->mm_count) <=
>>>>>> 0);
>>>>>>           mmdrop(p->mm);
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 02/16] drm/amdkfd: Don't dereference kfd_process.mm

[Felix Kuehling]

On 2017-10-27 03:41 AM, Christian König wrote:
> Am 27.10.2017 um 09:22 schrieb Christian König:
>> Am 26.10.2017 um 20:54 schrieb Felix Kuehling:
>>> On 2017-10-26 02:11 PM, Christian König wrote:
>>>> But now reading the patch there is something else which I stumbled
>>>> over:
>>>>> - WARN_ON(atomic_read(&p->mm->mm_count) <= 0);
>>>>> +    /*
>>>>> +     * This cast should be safe here because we grabbed a
>>>>> +     * reference to the mm in kfd_process_notifier_release
>>>>> +     */
>>>>> +    WARN_ON(atomic_read(&((struct mm_struct *)p->mm)->mm_count)
>>>>> <= 0);
>>>>>           mmdrop(p->mm);
>>>> Well that isn't good coding style. You shouldn't obfuscate what
>>>> pointer it is by changing it to "void*", but rather set it to NULL as
>>>> soon as you know that it is stale.
>>>>
>>>> Additional to that it is certainly not job of the driver to warn on a
>>>> run over mm_count.
>>> Yeah. We don't have this in our current staging branch. The whole
>>> process teardown has changed quite a bit. I just fixed this up to make
>>> it work with current upstream.
>>>
>>> If you prefer, I could just remove the WARN_ON.
>>
>> That sounds like a good idea to me, yes.
>
> Wait a second, now that I though once more about it what do you mean
> with this comment:
>> +    /*
>> +     * Opaque pointer to mm_struct. We don't hold a reference to
>> +     * it so it should never be dereferenced from here. This is
>> +     * only used for looking up processes by their mm.
>> +     */ 
> E.g. what means looking up the processes by their mm?
>
> Do you use a hashtable or something like this and then check if you
> got the correct mm structure by comparing the pointers?
>
> If that is the case then you should certainly set p->mm to NULL after
> mmdrop(p->mm).
>
> Otherwise it can happen that a new mm structure is allocated at the
> same location as the old one and you run into problems because the
> kernel driver accidentally uses the wrong kfd_process structure.

The processes are in an SRCU-protected hashtable that's read by
find_process_by_mm. Before this function (kfd_process_destroy_delayed)
runs, the process is already removed from the hash table and
synchronize_srcu is used to ensure there are no more readers active.
That means the process being destroyed can't be found any more, so there
is no need to reset p->mm to NULL at that point.

Regards,
  Felix

>
> Regards,
> Christian.
>
>>
>> Regards,
>> Christian.
>>
>>>
>>> Regards,
>>>    Felix
>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>> Regards,
>>>>>     Felix
>>>>>
>>>>>> Regards,
>>>>>> Christian.
>>>>>>
>>>>>>> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
>>>>>>> ---
>>>>>>>     drivers/gpu/drm/amd/amdkfd/kfd_events.c  | 19
>>>>>>> +++++++++++++++----
>>>>>>>     drivers/gpu/drm/amd/amdkfd/kfd_priv.h    |  7 ++++++-
>>>>>>>     drivers/gpu/drm/amd/amdkfd/kfd_process.c |  6 +++++-
>>>>>>>     3 files changed, 26 insertions(+), 6 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>>>>>>> b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>>>>>>> index 944abfa..61ce547 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>>>>>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>>>>>>> @@ -24,8 +24,8 @@
>>>>>>>     #include <linux/slab.h>
>>>>>>>     #include <linux/types.h>
>>>>>>>     #include <linux/sched/signal.h>
>>>>>>> +#include <linux/sched/mm.h>
>>>>>>>     #include <linux/uaccess.h>
>>>>>>> -#include <linux/mm.h>
>>>>>>>     #include <linux/mman.h>
>>>>>>>     #include <linux/memory.h>
>>>>>>>     #include "kfd_priv.h"
>>>>>>> @@ -904,14 +904,24 @@ void kfd_signal_iommu_event(struct kfd_dev
>>>>>>> *dev, unsigned int pasid,
>>>>>>>          * running so the lookup function returns a locked process.
>>>>>>>          */
>>>>>>>         struct kfd_process *p = kfd_lookup_process_by_pasid(pasid);
>>>>>>> +    struct mm_struct *mm;
>>>>>>>           if (!p)
>>>>>>>             return; /* Presumably process exited. */
>>>>>>>     +    /* Take a safe reference to the mm_struct, which may
>>>>>>> otherwise
>>>>>>> +     * disappear even while the kfd_process is still referenced.
>>>>>>> +     */
>>>>>>> +    mm = get_task_mm(p->lead_thread);
>>>>>>> +    if (!mm) {
>>>>>>> +        mutex_unlock(&p->mutex);
>>>>>>> +        return; /* Process is exiting */
>>>>>>> +    }
>>>>>>> +
>>>>>>>         memset(&memory_exception_data, 0,
>>>>>>> sizeof(memory_exception_data));
>>>>>>>     -    down_read(&p->mm->mmap_sem);
>>>>>>> -    vma = find_vma(p->mm, address);
>>>>>>> +    down_read(&mm->mmap_sem);
>>>>>>> +    vma = find_vma(mm, address);
>>>>>>>           memory_exception_data.gpu_id = dev->id;
>>>>>>>         memory_exception_data.va = address;
>>>>>>> @@ -937,7 +947,8 @@ void kfd_signal_iommu_event(struct kfd_dev
>>>>>>> *dev,
>>>>>>> unsigned int pasid,
>>>>>>>             }
>>>>>>>         }
>>>>>>>     -    up_read(&p->mm->mmap_sem);
>>>>>>> +    up_read(&mm->mmap_sem);
>>>>>>> +    mmput(mm);
>>>>>>>           mutex_lock(&p->event_mutex);
>>>>>>>     diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>>>>>> b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>>>>>> index 7d86ec9..1a483a7 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>>>>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>>>>>> @@ -494,7 +494,12 @@ struct kfd_process {
>>>>>>>          */
>>>>>>>         struct hlist_node kfd_processes;
>>>>>>>     -    struct mm_struct *mm;
>>>>>>> +    /*
>>>>>>> +     * Opaque pointer to mm_struct. We don't hold a reference to
>>>>>>> +     * it so it should never be dereferenced from here. This is
>>>>>>> +     * only used for looking up processes by their mm.
>>>>>>> +     */
>>>>>>> +    void *mm;
>>>>>>>           struct mutex mutex;
>>>>>>>     diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>>>>>> b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>>>>>> index 3ccb3b5..21d27e5 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>>>>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>>>>>> @@ -200,7 +200,11 @@ static void kfd_process_destroy_delayed(struct
>>>>>>> rcu_head *rcu)
>>>>>>>         struct kfd_process *p;
>>>>>>>           p = container_of(rcu, struct kfd_process, rcu);
>>>>>>> -    WARN_ON(atomic_read(&p->mm->mm_count) <= 0);
>>>>>>> +    /*
>>>>>>> +     * This cast should be safe here because we grabbed a
>>>>>>> +     * reference to the mm in kfd_process_notifier_release
>>>>>>> +     */
>>>>>>> +    WARN_ON(atomic_read(&((struct mm_struct *)p->mm)->mm_count) <=
>>>>>>> 0);
>>>>>>>           mmdrop(p->mm);
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>
>>
>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx