[PATCH v3 0/5] new event wait support

[PATCH v3 0/5] new event wait support

[James Zhu]

In kernel amdgpu driver, kfd_wait_on_events is used to support user space signal event wait
function. For multiple threads waiting on same event scenery, race condition could occur
since some threads after checking signal condition, before calling kfd_wait_on_events, the
event interrupt could be fired and wake up other thread which are sleeping on this event.
Then those threads could fall into sleep without waking up again. Adding event age tracking
in both kernel and user mode, will help avoiding this race condition.

The changes for The user space ROCT-Thunk-Interface/ROCR-Runtime are listed below for
review togehter with kernel mode changes.

ROCT-Thunk-Interface:
https://github.com/RadeonOpenCompute/ROCT-Thunk-Interface/commit/efdbf6cfbc026bd68ac3c35d00dacf84370eb81e
https://github.com/RadeonOpenCompute/ROCT-Thunk-Interface/commit/910108272091d1ce61dbc48bd9519731e0e9cf52

ROCR-Runtime:
https://github.com/RadeonOpenCompute/ROCR-Runtime/compare/master...zhums:ROCR-Runtime:new_event_wait_review
https://github.com/RadeonOpenCompute/ROCR-Runtime/commit/e1f5bdb88eb882ac798aeca2c00ea3fbb2dba459
https://github.com/RadeonOpenCompute/ROCR-Runtime/commit/7d26afd14107b5c2a754c1a3f415d89f3aabb503

-v2: remove unnecessay link

-v3: 1. update kfd test cases (910108272091d1ce61dbc48bd9519731e0e9cf52)
     2. move event age match checking into init_event_waiter
     3. move last event age update into copy_signaled_event_data

James Zhu (5):
  drm/amdkfd: add event age tracking
  drm/amdkfd: add event_age tracking when receiving interrupt
  drm/amdkfd: set activated flag true when event age unmatchs
  drm/amdkfd: update user space last_event_age
  drm/amdkfd: bump kfd ioctl minor version for event age availability

 drivers/gpu/drm/amd/amdkfd/kfd_events.c | 44 ++++++++++++++++++-------
 drivers/gpu/drm/amd/amdkfd/kfd_events.h |  1 +
 include/uapi/linux/kfd_ioctl.h          | 13 ++++++--
 3 files changed, 44 insertions(+), 14 deletions(-)

-- 
2.34.1

[PATCH v3 1/5] drm/amdkfd: add event age tracking

[James Zhu]

Add event age tracking

Signed-off-by: James Zhu <James.Zhu@amd.com>
---
 include/uapi/linux/kfd_ioctl.h | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/kfd_ioctl.h b/include/uapi/linux/kfd_ioctl.h
index 1781e7669982..93f1c0bc5caf 100644
--- a/include/uapi/linux/kfd_ioctl.h
+++ b/include/uapi/linux/kfd_ioctl.h
@@ -320,12 +320,20 @@ struct kfd_hsa_hw_exception_data {
 	__u32 gpu_id;
 };
 
+/* hsa signal event data */
+struct kfd_hsa_signal_event_data {
+	__u64 last_event_age;	/* to and from KFD */
+};
+
 /* Event data */
 struct kfd_event_data {
 	union {
+		/* From KFD */
 		struct kfd_hsa_memory_exception_data memory_exception_data;
 		struct kfd_hsa_hw_exception_data hw_exception_data;
-	};				/* From KFD */
+		/* To and From KFD */
+		struct kfd_hsa_signal_event_data signal_event_data;
+	};
 	__u64 kfd_event_data_ext;	/* pointer to an extension structure
 					   for future exception types */
 	__u32 event_id;		/* to KFD */
-- 
2.34.1

[PATCH v3 2/5] drm/amdkfd: add event_age tracking when receiving interrupt

[James Zhu]

Add event_age tracking when receiving interrupt.

Signed-off-by: James Zhu <James.Zhu@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_events.c | 6 ++++++
 drivers/gpu/drm/amd/amdkfd/kfd_events.h | 1 +
 2 files changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
index 7ff5c4e1b7e2..c7689181cc22 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
@@ -431,6 +431,7 @@ int kfd_event_create(struct file *devkfd, struct kfd_process *p,
 	if (!ret) {
 		*event_id = ev->event_id;
 		*event_trigger_data = ev->event_id;
+		ev->event_age = 1;
 	} else {
 		kfree(ev);
 	}
@@ -629,6 +630,11 @@ static void set_event(struct kfd_event *ev)
 	 * updating the wait queues in kfd_wait_on_events.
 	 */
 	ev->signaled = !ev->auto_reset || !waitqueue_active(&ev->wq);
+	if (!(++ev->event_age)) {
+		/* Never wrap back to reserved/default event age 0/1 */
+		ev->event_age = 2;
+		WARN_ONCE(1, "event_age wrap back!");
+	}
 
 	list_for_each_entry(waiter, &ev->wq.head, wait.entry)
 		WRITE_ONCE(waiter->activated, true);
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.h b/drivers/gpu/drm/amd/amdkfd/kfd_events.h
index 1c62c8dd6460..52ccfd397c2b 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_events.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.h
@@ -53,6 +53,7 @@ struct signal_page;
 
 struct kfd_event {
 	u32 event_id;
+	u64 event_age;
 
 	bool signaled;
 	bool auto_reset;
-- 
2.34.1

[PATCH v3 3/5] drm/amdkfd: set activated flag true when event age unmatchs

[James Zhu]

Set waiter's activated flag true when event age unmatchs with last_event_age.

Signed-off-by: James Zhu <James.Zhu@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_events.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
index c7689181cc22..4c6907507190 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
@@ -41,6 +41,7 @@ struct kfd_event_waiter {
 	wait_queue_entry_t wait;
 	struct kfd_event *event; /* Event to wait for */
 	bool activated;		 /* Becomes true when event is signaled */
+	bool event_age_enabled;  /* set to true when last_event_age is non-zero */
 };
 
 /*
@@ -797,9 +798,9 @@ static struct kfd_event_waiter *alloc_event_waiters(uint32_t num_events)
 
 static int init_event_waiter(struct kfd_process *p,
 		struct kfd_event_waiter *waiter,
-		uint32_t event_id)
+		struct kfd_event_data *event_data)
 {
-	struct kfd_event *ev = lookup_event_by_id(p, event_id);
+	struct kfd_event *ev = lookup_event_by_id(p, event_data->event_id);
 
 	if (!ev)
 		return -EINVAL;
@@ -808,6 +809,13 @@ static int init_event_waiter(struct kfd_process *p,
 	waiter->event = ev;
 	waiter->activated = ev->signaled;
 	ev->signaled = ev->signaled && !ev->auto_reset;
+
+	/* last_event_age = 0 reserved for backward compatible */
+	waiter->event_age_enabled = !!event_data->signal_event_data.last_event_age;
+	if (waiter->event_age_enabled &&
+		ev->event_age != event_data->signal_event_data.last_event_age)
+		WRITE_ONCE(waiter->activated, true);
+
 	if (!waiter->activated)
 		add_wait_queue(&ev->wq, &waiter->wait);
 	spin_unlock(&ev->lock);
@@ -948,8 +956,7 @@ int kfd_wait_on_events(struct kfd_process *p,
 			goto out_unlock;
 		}
 
-		ret = init_event_waiter(p, &event_waiters[i],
-					event_data.event_id);
+		ret = init_event_waiter(p, &event_waiters[i], &event_data);
 		if (ret)
 			goto out_unlock;
 	}
-- 
2.34.1

[PATCH v3 4/5] drm/amdkfd: update user space last_event_age

[James Zhu]

Update user space last_event_age when event age is enabled.
It is only for KFD_EVENT_TYPE_SIGNAL which is checked by user space.

Signed-off-by: James Zhu <James.Zhu@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_events.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
index 4c6907507190..9fcfec57a094 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
@@ -863,22 +863,29 @@ static int copy_signaled_event_data(uint32_t num_events,
 		struct kfd_event_waiter *event_waiters,
 		struct kfd_event_data __user *data)
 {
-	struct kfd_hsa_memory_exception_data *src;
-	struct kfd_hsa_memory_exception_data __user *dst;
+	void *src;
+	void __user *dst;
 	struct kfd_event_waiter *waiter;
 	struct kfd_event *event;
-	uint32_t i;
+	uint32_t i, size = 0;
 
 	for (i = 0; i < num_events; i++) {
 		waiter = &event_waiters[i];
 		event = waiter->event;
 		if (!event)
 			return -EINVAL; /* event was destroyed */
-		if (waiter->activated && event->type == KFD_EVENT_TYPE_MEMORY) {
-			dst = &data[i].memory_exception_data;
-			src = &event->memory_exception_data;
-			if (copy_to_user(dst, src,
-				sizeof(struct kfd_hsa_memory_exception_data)))
+		if (waiter->activated) {
+			if (event->type == KFD_EVENT_TYPE_MEMORY) {
+				dst = &data[i].memory_exception_data;
+				src = &event->memory_exception_data;
+				size = sizeof(struct kfd_hsa_memory_exception_data);
+			} else if (event->type == KFD_EVENT_TYPE_SIGNAL &&
+				waiter->event_age_enabled) {
+				dst = &data[i].signal_event_data.last_event_age;
+				src = &event->event_age;
+				size = sizeof(u64);
+			}
+			if (size && copy_to_user(dst, src, size))
 				return -EFAULT;
 		}
 	}
-- 
2.34.1

[PATCH v3 5/5] drm/amdkfd: bump kfd ioctl minor version for event age availability

[James Zhu]

Bump the minor version to declare event age tracking feature is now
available.

Signed-off-by: James Zhu <James.Zhu@amd.com>
---
 include/uapi/linux/kfd_ioctl.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/kfd_ioctl.h b/include/uapi/linux/kfd_ioctl.h
index 93f1c0bc5caf..eeb2fdcbdcb7 100644
--- a/include/uapi/linux/kfd_ioctl.h
+++ b/include/uapi/linux/kfd_ioctl.h
@@ -39,9 +39,10 @@
  * - 1.11 - Add unified memory for ctx save/restore area
  * - 1.12 - Add DMA buf export ioctl
  * - 1.13 - Add debugger API
+ * - 1.14 - Update kfd_event_data
  */
 #define KFD_IOCTL_MAJOR_VERSION 1
-#define KFD_IOCTL_MINOR_VERSION 13
+#define KFD_IOCTL_MINOR_VERSION 14
 
 struct kfd_ioctl_get_version_args {
 	__u32 major_version;	/* from KFD */
-- 
2.34.1

Re: [PATCH v3 3/5] drm/amdkfd: set activated flag true when event age unmatchs

[Felix Kuehling]

On 2023-06-08 13:07, James Zhu wrote:
> Set waiter's activated flag true when event age unmatchs with last_event_age.
>
> Signed-off-by: James Zhu <James.Zhu@amd.com>
> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_events.c | 15 +++++++++++----
>   1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> index c7689181cc22..4c6907507190 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> @@ -41,6 +41,7 @@ struct kfd_event_waiter {
>   	wait_queue_entry_t wait;
>   	struct kfd_event *event; /* Event to wait for */
>   	bool activated;		 /* Becomes true when event is signaled */
> +	bool event_age_enabled;  /* set to true when last_event_age is non-zero */
>   };
>   
>   /*
> @@ -797,9 +798,9 @@ static struct kfd_event_waiter *alloc_event_waiters(uint32_t num_events)
>   
>   static int init_event_waiter(struct kfd_process *p,
>   		struct kfd_event_waiter *waiter,
> -		uint32_t event_id)
> +		struct kfd_event_data *event_data)
>   {
> -	struct kfd_event *ev = lookup_event_by_id(p, event_id);
> +	struct kfd_event *ev = lookup_event_by_id(p, event_data->event_id);
>   
>   	if (!ev)
>   		return -EINVAL;
> @@ -808,6 +809,13 @@ static int init_event_waiter(struct kfd_process *p,
>   	waiter->event = ev;
>   	waiter->activated = ev->signaled;
>   	ev->signaled = ev->signaled && !ev->auto_reset;
> +
> +	/* last_event_age = 0 reserved for backward compatible */
> +	waiter->event_age_enabled = !!event_data->signal_event_data.last_event_age;
> +	if (waiter->event_age_enabled &&
> +		ev->event_age != event_data->signal_event_data.last_event_age)
> +		WRITE_ONCE(waiter->activated, true);

This needs to check the event type. Looking at 
event_data->signal_event_data when this is not a signal event is 
illegal, because it is aliased in a union with other event type data.

Other than that, the series looks good to me now.

Regards,
   Felix


> +
>   	if (!waiter->activated)
>   		add_wait_queue(&ev->wq, &waiter->wait);
>   	spin_unlock(&ev->lock);
> @@ -948,8 +956,7 @@ int kfd_wait_on_events(struct kfd_process *p,
>   			goto out_unlock;
>   		}
>   
> -		ret = init_event_waiter(p, &event_waiters[i],
> -					event_data.event_id);
> +		ret = init_event_waiter(p, &event_waiters[i], &event_data);
>   		if (ret)
>   			goto out_unlock;
>   	}

Re: [PATCH v3 3/5] drm/amdkfd: set activated flag true when event age unmatchs

[James Zhu]

On 2023-06-09 15:43, Felix Kuehling wrote:
>
> On 2023-06-08 13:07, James Zhu wrote:
>> Set waiter's activated flag true when event age unmatchs with 
>> last_event_age.
>>
>> Signed-off-by: James Zhu <James.Zhu@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdkfd/kfd_events.c | 15 +++++++++++----
>>   1 file changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c 
>> b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>> index c7689181cc22..4c6907507190 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>> @@ -41,6 +41,7 @@ struct kfd_event_waiter {
>>       wait_queue_entry_t wait;
>>       struct kfd_event *event; /* Event to wait for */
>>       bool activated;         /* Becomes true when event is signaled */
>> +    bool event_age_enabled;  /* set to true when last_event_age is 
>> non-zero */
>>   };
>>     /*
>> @@ -797,9 +798,9 @@ static struct kfd_event_waiter 
>> *alloc_event_waiters(uint32_t num_events)
>>     static int init_event_waiter(struct kfd_process *p,
>>           struct kfd_event_waiter *waiter,
>> -        uint32_t event_id)
>> +        struct kfd_event_data *event_data)
>>   {
>> -    struct kfd_event *ev = lookup_event_by_id(p, event_id);
>> +    struct kfd_event *ev = lookup_event_by_id(p, event_data->event_id);
>>         if (!ev)
>>           return -EINVAL;
>> @@ -808,6 +809,13 @@ static int init_event_waiter(struct kfd_process *p,
>>       waiter->event = ev;
>>       waiter->activated = ev->signaled;
>>       ev->signaled = ev->signaled && !ev->auto_reset;
>> +
>> +    /* last_event_age = 0 reserved for backward compatible */
>> +    waiter->event_age_enabled = 
>> !!event_data->signal_event_data.last_event_age;
>> +    if (waiter->event_age_enabled &&
>> +        ev->event_age != event_data->signal_event_data.last_event_age)
>> +        WRITE_ONCE(waiter->activated, true);
>
> This needs to check the event type. Looking at 
> event_data->signal_event_data when this is not a signal event is 
> illegal, because it is aliased in a union with other event type data.
[JZ] Sure. I will add it. Thanks!
>
> Other than that, the series looks good to me now.
>
> Regards,
>   Felix
>
>
>> +
>>       if (!waiter->activated)
>>           add_wait_queue(&ev->wq, &waiter->wait);
>>       spin_unlock(&ev->lock);
>> @@ -948,8 +956,7 @@ int kfd_wait_on_events(struct kfd_process *p,
>>               goto out_unlock;
>>           }
>>   -        ret = init_event_waiter(p, &event_waiters[i],
>> -                    event_data.event_id);
>> +        ret = init_event_waiter(p, &event_waiters[i], &event_data);
>>           if (ret)
>>               goto out_unlock;
>>       }

[PATCH v4 3/5] drm/amdkfd: set activated flag true when event age unmatchs

[James Zhu]

Set waiter's activated flag true when event age unmatchs with last_event_age.

-v4: add event type check

Signed-off-by: James Zhu <James.Zhu@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_events.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
index c7689181cc22..2cc1a7e976f4 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
@@ -41,6 +41,7 @@ struct kfd_event_waiter {
 	wait_queue_entry_t wait;
 	struct kfd_event *event; /* Event to wait for */
 	bool activated;		 /* Becomes true when event is signaled */
+	bool event_age_enabled;  /* set to true when last_event_age is non-zero */
 };
 
 /*
@@ -797,9 +798,9 @@ static struct kfd_event_waiter *alloc_event_waiters(uint32_t num_events)
 
 static int init_event_waiter(struct kfd_process *p,
 		struct kfd_event_waiter *waiter,
-		uint32_t event_id)
+		struct kfd_event_data *event_data)
 {
-	struct kfd_event *ev = lookup_event_by_id(p, event_id);
+	struct kfd_event *ev = lookup_event_by_id(p, event_data->event_id);
 
 	if (!ev)
 		return -EINVAL;
@@ -808,6 +809,13 @@ static int init_event_waiter(struct kfd_process *p,
 	waiter->event = ev;
 	waiter->activated = ev->signaled;
 	ev->signaled = ev->signaled && !ev->auto_reset;
+
+	/* last_event_age = 0 reserved for backward compatible */
+	waiter->event_age_enabled = !!event_data->signal_event_data.last_event_age;
+	if (waiter->event->type == KFD_EVENT_TYPE_SIGNAL && waiter->event_age_enabled &&
+		ev->event_age != event_data->signal_event_data.last_event_age)
+		WRITE_ONCE(waiter->activated, true);
+
 	if (!waiter->activated)
 		add_wait_queue(&ev->wq, &waiter->wait);
 	spin_unlock(&ev->lock);
@@ -948,8 +956,7 @@ int kfd_wait_on_events(struct kfd_process *p,
 			goto out_unlock;
 		}
 
-		ret = init_event_waiter(p, &event_waiters[i],
-					event_data.event_id);
+		ret = init_event_waiter(p, &event_waiters[i], &event_data);
 		if (ret)
 			goto out_unlock;
 	}
-- 
2.34.1

Re: [PATCH v4 3/5] drm/amdkfd: set activated flag true when event age unmatchs

[Felix Kuehling]

On 2023-06-09 16:13, James Zhu wrote:
> Set waiter's activated flag true when event age unmatchs with last_event_age.
>
> -v4: add event type check
>
> Signed-off-by: James Zhu <James.Zhu@amd.com>
> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_events.c | 15 +++++++++++----
>   1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> index c7689181cc22..2cc1a7e976f4 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> @@ -41,6 +41,7 @@ struct kfd_event_waiter {
>   	wait_queue_entry_t wait;
>   	struct kfd_event *event; /* Event to wait for */
>   	bool activated;		 /* Becomes true when event is signaled */
> +	bool event_age_enabled;  /* set to true when last_event_age is non-zero */
>   };
>   
>   /*
> @@ -797,9 +798,9 @@ static struct kfd_event_waiter *alloc_event_waiters(uint32_t num_events)
>   
>   static int init_event_waiter(struct kfd_process *p,
>   		struct kfd_event_waiter *waiter,
> -		uint32_t event_id)
> +		struct kfd_event_data *event_data)
>   {
> -	struct kfd_event *ev = lookup_event_by_id(p, event_id);
> +	struct kfd_event *ev = lookup_event_by_id(p, event_data->event_id);
>   
>   	if (!ev)
>   		return -EINVAL;
> @@ -808,6 +809,13 @@ static int init_event_waiter(struct kfd_process *p,
>   	waiter->event = ev;
>   	waiter->activated = ev->signaled;
>   	ev->signaled = ev->signaled && !ev->auto_reset;
> +
> +	/* last_event_age = 0 reserved for backward compatible */
> +	waiter->event_age_enabled = !!event_data->signal_event_data.last_event_age;

This should also be inside the "if (waiter->event->type == 
KFD_EVENT_TYPE_SIGNAL)". I'd do something like this:

	if (waiter->event->type == KFD_EVENT_TYPE_SIGNAL &&
	    event_data->signal_event_data.last_event_age) {
		waiter->event_age_enabled = true;
		if (ev->event_age != event_data->signal_event_data.last_event_age)
			waiter->activated = true;
	}

You don't need WRITE_ONCE here because there can be no concurrent access 
before you add the waiter to the wait queue.

Regards,
   Felix


> +	if (waiter->event->type == KFD_EVENT_TYPE_SIGNAL && waiter->event_age_enabled &&
> +		ev->event_age != event_data->signal_event_data.last_event_age)
> +		WRITE_ONCE(waiter->activated, true);
> +
>   	if (!waiter->activated)
>   		add_wait_queue(&ev->wq, &waiter->wait);
>   	spin_unlock(&ev->lock);
> @@ -948,8 +956,7 @@ int kfd_wait_on_events(struct kfd_process *p,
>   			goto out_unlock;
>   		}
>   
> -		ret = init_event_waiter(p, &event_waiters[i],
> -					event_data.event_id);
> +		ret = init_event_waiter(p, &event_waiters[i], &event_data);
>   		if (ret)
>   			goto out_unlock;
>   	}

Re: [PATCH v4 3/5] drm/amdkfd: set activated flag true when event age unmatchs

[James Zhu]

On 2023-06-09 16:22, Felix Kuehling wrote:
>
> On 2023-06-09 16:13, James Zhu wrote:
>> Set waiter's activated flag true when event age unmatchs with 
>> last_event_age.
>>
>> -v4: add event type check
>>
>> Signed-off-by: James Zhu <James.Zhu@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdkfd/kfd_events.c | 15 +++++++++++----
>>   1 file changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c 
>> b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>> index c7689181cc22..2cc1a7e976f4 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>> @@ -41,6 +41,7 @@ struct kfd_event_waiter {
>>       wait_queue_entry_t wait;
>>       struct kfd_event *event; /* Event to wait for */
>>       bool activated;         /* Becomes true when event is signaled */
>> +    bool event_age_enabled;  /* set to true when last_event_age is 
>> non-zero */
>>   };
>>     /*
>> @@ -797,9 +798,9 @@ static struct kfd_event_waiter 
>> *alloc_event_waiters(uint32_t num_events)
>>     static int init_event_waiter(struct kfd_process *p,
>>           struct kfd_event_waiter *waiter,
>> -        uint32_t event_id)
>> +        struct kfd_event_data *event_data)
>>   {
>> -    struct kfd_event *ev = lookup_event_by_id(p, event_id);
>> +    struct kfd_event *ev = lookup_event_by_id(p, event_data->event_id);
>>         if (!ev)
>>           return -EINVAL;
>> @@ -808,6 +809,13 @@ static int init_event_waiter(struct kfd_process *p,
>>       waiter->event = ev;
>>       waiter->activated = ev->signaled;
>>       ev->signaled = ev->signaled && !ev->auto_reset;
>> +
>> +    /* last_event_age = 0 reserved for backward compatible */
>> +    waiter->event_age_enabled = 
>> !!event_data->signal_event_data.last_event_age;
>
> This should also be inside the "if (waiter->event->type == 
> KFD_EVENT_TYPE_SIGNAL)". I'd do something like this:
>
>     if (waiter->event->type == KFD_EVENT_TYPE_SIGNAL &&
>         event_data->signal_event_data.last_event_age) {
>         waiter->event_age_enabled = true;
>         if (ev->event_age != 
> event_data->signal_event_data.last_event_age)
>             waiter->activated = true;
>     }
>
> You don't need WRITE_ONCE here because there can be no concurrent 
> access before you add the waiter to the wait queue.
[JZ] Yes, this looks better.
>
> Regards,
>   Felix
>
>
>> +    if (waiter->event->type == KFD_EVENT_TYPE_SIGNAL && 
>> waiter->event_age_enabled &&
>> +        ev->event_age != event_data->signal_event_data.last_event_age)
>> +        WRITE_ONCE(waiter->activated, true);
>> +
>>       if (!waiter->activated)
>>           add_wait_queue(&ev->wq, &waiter->wait);
>>       spin_unlock(&ev->lock);
>> @@ -948,8 +956,7 @@ int kfd_wait_on_events(struct kfd_process *p,
>>               goto out_unlock;
>>           }
>>   -        ret = init_event_waiter(p, &event_waiters[i],
>> -                    event_data.event_id);
>> +        ret = init_event_waiter(p, &event_waiters[i], &event_data);
>>           if (ret)
>>               goto out_unlock;
>>       }

[PATCH v5 3/5] drm/amdkfd: set activated flag true when event age unmatchs

[James Zhu]

Set waiter's activated flag true when event age unmatchs with last_event_age.

-v4: add event type check
-v5: improve on event age enable and activated flags

Signed-off-by: James Zhu <James.Zhu@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_events.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
index c7689181cc22..b2586a1dd35d 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
@@ -41,6 +41,7 @@ struct kfd_event_waiter {
 	wait_queue_entry_t wait;
 	struct kfd_event *event; /* Event to wait for */
 	bool activated;		 /* Becomes true when event is signaled */
+	bool event_age_enabled;  /* set to true when last_event_age is non-zero */
 };
 
 /*
@@ -797,9 +798,9 @@ static struct kfd_event_waiter *alloc_event_waiters(uint32_t num_events)
 
 static int init_event_waiter(struct kfd_process *p,
 		struct kfd_event_waiter *waiter,
-		uint32_t event_id)
+		struct kfd_event_data *event_data)
 {
-	struct kfd_event *ev = lookup_event_by_id(p, event_id);
+	struct kfd_event *ev = lookup_event_by_id(p, event_data->event_id);
 
 	if (!ev)
 		return -EINVAL;
@@ -808,6 +809,15 @@ static int init_event_waiter(struct kfd_process *p,
 	waiter->event = ev;
 	waiter->activated = ev->signaled;
 	ev->signaled = ev->signaled && !ev->auto_reset;
+
+	/* last_event_age = 0 reserved for backward compatible */
+	if (waiter->event->type == KFD_EVENT_TYPE_SIGNAL &&
+		event_data->signal_event_data.last_event_age) {
+		waiter->event_age_enabled = true;
+		if (ev->event_age != event_data->signal_event_data.last_event_age)
+			waiter->activated = true;
+	}
+
 	if (!waiter->activated)
 		add_wait_queue(&ev->wq, &waiter->wait);
 	spin_unlock(&ev->lock);
@@ -948,8 +958,7 @@ int kfd_wait_on_events(struct kfd_process *p,
 			goto out_unlock;
 		}
 
-		ret = init_event_waiter(p, &event_waiters[i],
-					event_data.event_id);
+		ret = init_event_waiter(p, &event_waiters[i], &event_data);
 		if (ret)
 			goto out_unlock;
 	}
-- 
2.34.1

Re: [PATCH v5 3/5] drm/amdkfd: set activated flag true when event age unmatchs

[Felix Kuehling]

 From the KFD perspective, the series is

Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>

David, I looked at the ROCr and Thunk changes as well, and they look 
reasonable to me. Do you have any feedback on these patches from a ROCr 
point of view? Is there a reasonable stress test that could be used 
check that this handles the race conditions as expected and allows all 
waiters to sleep?

Regards,
   Felix


On 2023-06-09 16:43, James Zhu wrote:
> Set waiter's activated flag true when event age unmatchs with last_event_age.
>
> -v4: add event type check
> -v5: improve on event age enable and activated flags
>
> Signed-off-by: James Zhu <James.Zhu@amd.com>
> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_events.c | 17 +++++++++++++----
>   1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> index c7689181cc22..b2586a1dd35d 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> @@ -41,6 +41,7 @@ struct kfd_event_waiter {
>   	wait_queue_entry_t wait;
>   	struct kfd_event *event; /* Event to wait for */
>   	bool activated;		 /* Becomes true when event is signaled */
> +	bool event_age_enabled;  /* set to true when last_event_age is non-zero */
>   };
>   
>   /*
> @@ -797,9 +798,9 @@ static struct kfd_event_waiter *alloc_event_waiters(uint32_t num_events)
>   
>   static int init_event_waiter(struct kfd_process *p,
>   		struct kfd_event_waiter *waiter,
> -		uint32_t event_id)
> +		struct kfd_event_data *event_data)
>   {
> -	struct kfd_event *ev = lookup_event_by_id(p, event_id);
> +	struct kfd_event *ev = lookup_event_by_id(p, event_data->event_id);
>   
>   	if (!ev)
>   		return -EINVAL;
> @@ -808,6 +809,15 @@ static int init_event_waiter(struct kfd_process *p,
>   	waiter->event = ev;
>   	waiter->activated = ev->signaled;
>   	ev->signaled = ev->signaled && !ev->auto_reset;
> +
> +	/* last_event_age = 0 reserved for backward compatible */
> +	if (waiter->event->type == KFD_EVENT_TYPE_SIGNAL &&
> +		event_data->signal_event_data.last_event_age) {
> +		waiter->event_age_enabled = true;
> +		if (ev->event_age != event_data->signal_event_data.last_event_age)
> +			waiter->activated = true;
> +	}
> +
>   	if (!waiter->activated)
>   		add_wait_queue(&ev->wq, &waiter->wait);
>   	spin_unlock(&ev->lock);
> @@ -948,8 +958,7 @@ int kfd_wait_on_events(struct kfd_process *p,
>   			goto out_unlock;
>   		}
>   
> -		ret = init_event_waiter(p, &event_waiters[i],
> -					event_data.event_id);
> +		ret = init_event_waiter(p, &event_waiters[i], &event_data);
>   		if (ret)
>   			goto out_unlock;
>   	}

RE: [PATCH v5 3/5] drm/amdkfd: set activated flag true when event age unmatchs

[Yat Sin, David]

[AMD Official Use Only - General]

The current ROCr patches already address my previous feedback. I am ok with the current ROCr patches.

Currently, there is no ROCrtst that would stress this multiple-waiters issue. I was thinking something like the KFDTest, but with by calling the waiters from different threads. @Zhu, James Would you have time to look into this?

~David

> -----Original Message-----
> From: Kuehling, Felix <Felix.Kuehling@amd.com>
> Sent: Friday, June 9, 2023 6:44 PM
> To: Zhu, James <James.Zhu@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Yat Sin, David <David.YatSin@amd.com>; Zhu, James
> <James.Zhu@amd.com>
> Subject: Re: [PATCH v5 3/5] drm/amdkfd: set activated flag true when event
> age unmatchs
>
>  From the KFD perspective, the series is
>
> Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
>
> David, I looked at the ROCr and Thunk changes as well, and they look
> reasonable to me. Do you have any feedback on these patches from a ROCr
> point of view? Is there a reasonable stress test that could be used check that
> this handles the race conditions as expected and allows all waiters to sleep?
>
> Regards,
>    Felix
>
>
> On 2023-06-09 16:43, James Zhu wrote:
> > Set waiter's activated flag true when event age unmatchs with
> last_event_age.
> >
> > -v4: add event type check
> > -v5: improve on event age enable and activated flags
> >
> > Signed-off-by: James Zhu <James.Zhu@amd.com>
> > ---
> >   drivers/gpu/drm/amd/amdkfd/kfd_events.c | 17 +++++++++++++----
> >   1 file changed, 13 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> > b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> > index c7689181cc22..b2586a1dd35d 100644
> > --- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> > @@ -41,6 +41,7 @@ struct kfd_event_waiter {
> >     wait_queue_entry_t wait;
> >     struct kfd_event *event; /* Event to wait for */
> >     bool activated;          /* Becomes true when event is signaled */
> > +   bool event_age_enabled;  /* set to true when last_event_age is
> > +non-zero */
> >   };
> >
> >   /*
> > @@ -797,9 +798,9 @@ static struct kfd_event_waiter
> > *alloc_event_waiters(uint32_t num_events)
> >
> >   static int init_event_waiter(struct kfd_process *p,
> >             struct kfd_event_waiter *waiter,
> > -           uint32_t event_id)
> > +           struct kfd_event_data *event_data)
> >   {
> > -   struct kfd_event *ev = lookup_event_by_id(p, event_id);
> > +   struct kfd_event *ev = lookup_event_by_id(p, event_data->event_id);
> >
> >     if (!ev)
> >             return -EINVAL;
> > @@ -808,6 +809,15 @@ static int init_event_waiter(struct kfd_process *p,
> >     waiter->event = ev;
> >     waiter->activated = ev->signaled;
> >     ev->signaled = ev->signaled && !ev->auto_reset;
> > +
> > +   /* last_event_age = 0 reserved for backward compatible */
> > +   if (waiter->event->type == KFD_EVENT_TYPE_SIGNAL &&
> > +           event_data->signal_event_data.last_event_age) {
> > +           waiter->event_age_enabled = true;
> > +           if (ev->event_age != event_data-
> >signal_event_data.last_event_age)
> > +                   waiter->activated = true;
> > +   }
> > +
> >     if (!waiter->activated)
> >             add_wait_queue(&ev->wq, &waiter->wait);
> >     spin_unlock(&ev->lock);
> > @@ -948,8 +958,7 @@ int kfd_wait_on_events(struct kfd_process *p,
> >                     goto out_unlock;
> >             }
> >
> > -           ret = init_event_waiter(p, &event_waiters[i],
> > -                                   event_data.event_id);
> > +           ret = init_event_waiter(p, &event_waiters[i], &event_data);
> >             if (ret)
> >                     goto out_unlock;
> >     }

Re: [PATCH v5 3/5] drm/amdkfd: set activated flag true when event age unmatchs

[Felix Kuehling]

Testing for intermittent failures or race conditions is not easy. If we 
create such a test, we need to make sure it can catch the problem when 
not using the event ages, just to know that the test is good enough.

I guess it could be a parametrized test that can run with or without 
event age. Without event age, we'd expect to catch a timeout. Not 
catching a timeout would be a test failure (indicating that the test is 
not good enough). With event age it should not time out, i.e. a timeout 
would be considered a failure in this case (indicating a problem with 
the event age mechanism).

That said, I'd feel better about a ROCr test that doesn't just cover the 
KFD event age mechanism, but also its use in the ROCr implementation of 
HSA signal waiting.

Regards,
   Felix


Am 2023-06-12 um 12:19 schrieb Yat Sin, David:
> [AMD Official Use Only - General]
>
> The current ROCr patches already address my previous feedback. I am ok with the current ROCr patches.
>
> Currently, there is no ROCrtst that would stress this multiple-waiters issue. I was thinking something like the KFDTest, but with by calling the waiters from different threads. @Zhu, James Would you have time to look into this?
>
> ~David
>
>> -----Original Message-----
>> From: Kuehling, Felix <Felix.Kuehling@amd.com>
>> Sent: Friday, June 9, 2023 6:44 PM
>> To: Zhu, James <James.Zhu@amd.com>; amd-gfx@lists.freedesktop.org
>> Cc: Yat Sin, David <David.YatSin@amd.com>; Zhu, James
>> <James.Zhu@amd.com>
>> Subject: Re: [PATCH v5 3/5] drm/amdkfd: set activated flag true when event
>> age unmatchs
>>
>>   From the KFD perspective, the series is
>>
>> Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
>>
>> David, I looked at the ROCr and Thunk changes as well, and they look
>> reasonable to me. Do you have any feedback on these patches from a ROCr
>> point of view? Is there a reasonable stress test that could be used check that
>> this handles the race conditions as expected and allows all waiters to sleep?
>>
>> Regards,
>>     Felix
>>
>>
>> On 2023-06-09 16:43, James Zhu wrote:
>>> Set waiter's activated flag true when event age unmatchs with
>> last_event_age.
>>> -v4: add event type check
>>> -v5: improve on event age enable and activated flags
>>>
>>> Signed-off-by: James Zhu <James.Zhu@amd.com>
>>> ---
>>>    drivers/gpu/drm/amd/amdkfd/kfd_events.c | 17 +++++++++++++----
>>>    1 file changed, 13 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>>> b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>>> index c7689181cc22..b2586a1dd35d 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
>>> @@ -41,6 +41,7 @@ struct kfd_event_waiter {
>>>      wait_queue_entry_t wait;
>>>      struct kfd_event *event; /* Event to wait for */
>>>      bool activated;          /* Becomes true when event is signaled */
>>> +   bool event_age_enabled;  /* set to true when last_event_age is
>>> +non-zero */
>>>    };
>>>
>>>    /*
>>> @@ -797,9 +798,9 @@ static struct kfd_event_waiter
>>> *alloc_event_waiters(uint32_t num_events)
>>>
>>>    static int init_event_waiter(struct kfd_process *p,
>>>              struct kfd_event_waiter *waiter,
>>> -           uint32_t event_id)
>>> +           struct kfd_event_data *event_data)
>>>    {
>>> -   struct kfd_event *ev = lookup_event_by_id(p, event_id);
>>> +   struct kfd_event *ev = lookup_event_by_id(p, event_data->event_id);
>>>
>>>      if (!ev)
>>>              return -EINVAL;
>>> @@ -808,6 +809,15 @@ static int init_event_waiter(struct kfd_process *p,
>>>      waiter->event = ev;
>>>      waiter->activated = ev->signaled;
>>>      ev->signaled = ev->signaled && !ev->auto_reset;
>>> +
>>> +   /* last_event_age = 0 reserved for backward compatible */
>>> +   if (waiter->event->type == KFD_EVENT_TYPE_SIGNAL &&
>>> +           event_data->signal_event_data.last_event_age) {
>>> +           waiter->event_age_enabled = true;
>>> +           if (ev->event_age != event_data-
>>> signal_event_data.last_event_age)
>>> +                   waiter->activated = true;
>>> +   }
>>> +
>>>      if (!waiter->activated)
>>>              add_wait_queue(&ev->wq, &waiter->wait);
>>>      spin_unlock(&ev->lock);
>>> @@ -948,8 +958,7 @@ int kfd_wait_on_events(struct kfd_process *p,
>>>                      goto out_unlock;
>>>              }
>>>
>>> -           ret = init_event_waiter(p, &event_waiters[i],
>>> -                                   event_data.event_id);
>>> +           ret = init_event_waiter(p, &event_waiters[i], &event_data);
>>>              if (ret)
>>>                      goto out_unlock;
>>>      }