DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] eal/ipc: stop async IPC loop on callback request
@ 2018-04-10 10:03 Anatoly Burakov
  2018-04-10 13:53 ` Tan, Jianfeng
  2018-04-10 15:28 ` [dpdk-dev] [PATCH v2] " Anatoly Burakov
  0 siblings, 2 replies; 7+ messages in thread
From: Anatoly Burakov @ 2018-04-10 10:03 UTC (permalink / raw)
  To: dev; +Cc: jianfeng.tan, anatoly.burakov

EAL did not stop processing further asynchronous requests on
encountering a request that should trigger the callback. This
resulted in erasing valid requests but not triggering them.

Fix this by stopping the loop once we have a request that we
can trigger. Also, remove unnecessary check for trigger
request being NULL.

Fixes: f05e26051c15 ("eal: add IPC asynchronous request")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/common/eal_common_proc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_proc.c b/lib/librte_eal/common/eal_common_proc.c
index f98622f..1ea3b58 100644
--- a/lib/librte_eal/common/eal_common_proc.c
+++ b/lib/librte_eal/common/eal_common_proc.c
@@ -510,11 +510,11 @@ async_reply_handle(void *arg __rte_unused)
 					TAILQ_REMOVE(&pending_requests.requests,
 							sr, next);
 					free(sr);
-				} else if (action == ACTION_TRIGGER &&
-						trigger == NULL) {
+				} else if (action == ACTION_TRIGGER) {
 					TAILQ_REMOVE(&pending_requests.requests,
 							sr, next);
 					trigger = sr;
+					break;
 				}
 			}
 		}
-- 
2.7.4

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH] eal/ipc: stop async IPC loop on callback request
  2018-04-10 10:03 [dpdk-dev] [PATCH] eal/ipc: stop async IPC loop on callback request Anatoly Burakov
@ 2018-04-10 13:53 ` Tan, Jianfeng
  2018-04-10 14:17   ` Burakov, Anatoly
  2018-04-10 15:28 ` [dpdk-dev] [PATCH v2] " Anatoly Burakov
  1 sibling, 1 reply; 7+ messages in thread
From: Tan, Jianfeng @ 2018-04-10 13:53 UTC (permalink / raw)
  To: Anatoly Burakov, dev



On 4/10/2018 6:03 PM, Anatoly Burakov wrote:
> EAL did not stop processing further asynchronous requests on
> encountering a request that should trigger the callback. This
> resulted in erasing valid requests but not triggering them.

That means one wakeup could process multiple replies, and following 
process_async_request() will erase some valid requests?

>
> Fix this by stopping the loop once we have a request that we
> can trigger. Also, remove unnecessary check for trigger
> request being NULL.
>
> Fixes: f05e26051c15 ("eal: add IPC asynchronous request")
> Cc: anatoly.burakov@intel.com
>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>


> ---
>   lib/librte_eal/common/eal_common_proc.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/lib/librte_eal/common/eal_common_proc.c b/lib/librte_eal/common/eal_common_proc.c
> index f98622f..1ea3b58 100644
> --- a/lib/librte_eal/common/eal_common_proc.c
> +++ b/lib/librte_eal/common/eal_common_proc.c
> @@ -510,11 +510,11 @@ async_reply_handle(void *arg __rte_unused)
>   					TAILQ_REMOVE(&pending_requests.requests,
>   							sr, next);
>   					free(sr);
> -				} else if (action == ACTION_TRIGGER &&
> -						trigger == NULL) {
> +				} else if (action == ACTION_TRIGGER) {
>   					TAILQ_REMOVE(&pending_requests.requests,
>   							sr, next);
>   					trigger = sr;
> +					break;

If I understand it correctly above, break here, we will trigger an async 
action, and then go back to sleep with some ready requests not handled? 
Seems that we shall unlock, process, and lock here. Right?

Thanks,
Jianfeng

>   				}
>   			}
>   		}

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH] eal/ipc: stop async IPC loop on callback request
  2018-04-10 13:53 ` Tan, Jianfeng
@ 2018-04-10 14:17   ` Burakov, Anatoly
  2018-04-10 15:16     ` Tan, Jianfeng
  0 siblings, 1 reply; 7+ messages in thread
From: Burakov, Anatoly @ 2018-04-10 14:17 UTC (permalink / raw)
  To: Tan, Jianfeng, dev

On 10-Apr-18 2:53 PM, Tan, Jianfeng wrote:
> 
> 
> On 4/10/2018 6:03 PM, Anatoly Burakov wrote:
>> EAL did not stop processing further asynchronous requests on
>> encountering a request that should trigger the callback. This
>> resulted in erasing valid requests but not triggering them.
> 
> That means one wakeup could process multiple replies, and following 
> process_async_request() will erase some valid requests?

Yes.

> 
>>
>> Fix this by stopping the loop once we have a request that we
>> can trigger. Also, remove unnecessary check for trigger
>> request being NULL.
>>
>> Fixes: f05e26051c15 ("eal: add IPC asynchronous request")
>> Cc: anatoly.burakov@intel.com
>>
>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> 
> 
>> ---
>>   lib/librte_eal/common/eal_common_proc.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/librte_eal/common/eal_common_proc.c 
>> b/lib/librte_eal/common/eal_common_proc.c
>> index f98622f..1ea3b58 100644
>> --- a/lib/librte_eal/common/eal_common_proc.c
>> +++ b/lib/librte_eal/common/eal_common_proc.c
>> @@ -510,11 +510,11 @@ async_reply_handle(void *arg __rte_unused)
>>                       TAILQ_REMOVE(&pending_requests.requests,
>>                               sr, next);
>>                       free(sr);
>> -                } else if (action == ACTION_TRIGGER &&
>> -                        trigger == NULL) {
>> +                } else if (action == ACTION_TRIGGER) {
>>                       TAILQ_REMOVE(&pending_requests.requests,
>>                               sr, next);
>>                       trigger = sr;
>> +                    break;
> 
> If I understand it correctly above, break here, we will trigger an async 
> action, and then go back to sleep with some ready requests not handled? 
> Seems that we shall unlock, process, and lock here. Right?

Well, we won't go back to sleep - we'll just loop around and come back.

See eal_common_proc.c:472:

	/* sometimes, we don't even wait */
	if (sr->reply_received) {
		nowait = true;
		break;
	}

Followed by line 478:

	if (nowait)
		ret = 0;

Followed by line 495:

	if (ret == 0 || ret == ETIMEDOUT) {

So, having messages with replies already in the queue will cause wait to 
be cancelled. It's not much slower than unlocking, triggering, and 
locking again.

However, if you're OK with lock-loop-unlock-trigger-lock-loop-unlock-... 
sequence until we run out of triggers, then sure, i can add that.

> 
> Thanks,
> Jianfeng
> 
>>                   }
>>               }
>>           }
> 
> 


-- 
Thanks,
Anatoly

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH] eal/ipc: stop async IPC loop on callback request
  2018-04-10 14:17   ` Burakov, Anatoly
@ 2018-04-10 15:16     ` Tan, Jianfeng
  0 siblings, 0 replies; 7+ messages in thread
From: Tan, Jianfeng @ 2018-04-10 15:16 UTC (permalink / raw)
  To: Burakov, Anatoly, dev



On 4/10/2018 10:17 PM, Burakov, Anatoly wrote:
> On 10-Apr-18 2:53 PM, Tan, Jianfeng wrote:
>>
>>
>> On 4/10/2018 6:03 PM, Anatoly Burakov wrote:
>>> EAL did not stop processing further asynchronous requests on
>>> encountering a request that should trigger the callback. This
>>> resulted in erasing valid requests but not triggering them.
>>
>> That means one wakeup could process multiple replies, and following 
>> process_async_request() will erase some valid requests?
>
> Yes.
>
>>
>>>
>>> Fix this by stopping the loop once we have a request that we
>>> can trigger. Also, remove unnecessary check for trigger
>>> request being NULL.
>>>
>>> Fixes: f05e26051c15 ("eal: add IPC asynchronous request")
>>> Cc: anatoly.burakov@intel.com
>>>
>>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
>>

Acked-by: Jianfeng Tan <jianfeng.tan@intel.com>

>>
>>> ---
>>>   lib/librte_eal/common/eal_common_proc.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/lib/librte_eal/common/eal_common_proc.c 
>>> b/lib/librte_eal/common/eal_common_proc.c
>>> index f98622f..1ea3b58 100644
>>> --- a/lib/librte_eal/common/eal_common_proc.c
>>> +++ b/lib/librte_eal/common/eal_common_proc.c
>>> @@ -510,11 +510,11 @@ async_reply_handle(void *arg __rte_unused)
>>> TAILQ_REMOVE(&pending_requests.requests,
>>>                               sr, next);
>>>                       free(sr);
>>> -                } else if (action == ACTION_TRIGGER &&
>>> -                        trigger == NULL) {
>>> +                } else if (action == ACTION_TRIGGER) {
>>> TAILQ_REMOVE(&pending_requests.requests,
>>>                               sr, next);
>>>                       trigger = sr;
>>> +                    break;
>>
>> If I understand it correctly above, break here, we will trigger an 
>> async action, and then go back to sleep with some ready requests not 
>> handled? Seems that we shall unlock, process, and lock here. Right?
>
> Well, we won't go back to sleep - we'll just loop around and come back.
>
> See eal_common_proc.c:472:
>
>     /* sometimes, we don't even wait */
>     if (sr->reply_received) {
>         nowait = true;
>         break;
>     }
>
> Followed by line 478:
>
>     if (nowait)
>         ret = 0;
>
> Followed by line 495:
>
>     if (ret == 0 || ret == ETIMEDOUT) {
>
> So, having messages with replies already in the queue will cause wait 
> to be cancelled. It's not much slower than unlocking, triggering, and 
> locking again.

Ah, sorry, I overlooked that fact that every iteration we re-scan the 
request list.

>
> However, if you're OK with 
> lock-loop-unlock-trigger-lock-loop-unlock-... sequence until we run 
> out of triggers, then sure, i can add that.

Don't see the reason for that.

Thanks,
Jianfeng

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [dpdk-dev] [PATCH v2] eal/ipc: stop async IPC loop on callback request
  2018-04-10 10:03 [dpdk-dev] [PATCH] eal/ipc: stop async IPC loop on callback request Anatoly Burakov
  2018-04-10 13:53 ` Tan, Jianfeng
@ 2018-04-10 15:28 ` Anatoly Burakov
  2018-04-13 15:24   ` Tan, Jianfeng
  1 sibling, 1 reply; 7+ messages in thread
From: Anatoly Burakov @ 2018-04-10 15:28 UTC (permalink / raw)
  To: dev; +Cc: jianfeng.tan, anatoly.burakov

EAL did not stop processing further asynchronous requests on
encountering a request that should trigger the callback. This
resulted in erasing valid requests but not triggering them.

Fix this by stopping the loop once we have a request that
can trigger the callback. Once triggered, we go back to scanning
the request queue until there are no more callbacks to trigger.

Fixes: f05e26051c15 ("eal: add IPC asynchronous request")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---

Notes:
    Took the opportunity to simplify some code as well.
    
    The change is a bit more substantial, hence dropping ack.

 lib/librte_eal/common/eal_common_proc.c | 150 ++++++++++++++++++--------------
 1 file changed, 86 insertions(+), 64 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_proc.c b/lib/librte_eal/common/eal_common_proc.c
index f98622f..c888c84 100644
--- a/lib/librte_eal/common/eal_common_proc.c
+++ b/lib/librte_eal/common/eal_common_proc.c
@@ -441,49 +441,87 @@ trigger_async_action(struct pending_request *sr)
 	free(sr->request);
 }
 
+static struct pending_request *
+check_trigger(struct timespec *ts)
+{
+	struct pending_request *next, *cur, *trigger = NULL;
+
+	TAILQ_FOREACH_SAFE(cur, &pending_requests.requests, next, next) {
+		enum async_action action;
+		if (cur->type != REQUEST_TYPE_ASYNC)
+			continue;
+
+		action = process_async_request(cur, ts);
+		if (action == ACTION_FREE) {
+			TAILQ_REMOVE(&pending_requests.requests, cur, next);
+			free(cur);
+		} else if (action == ACTION_TRIGGER) {
+			TAILQ_REMOVE(&pending_requests.requests, cur, next);
+			trigger = cur;
+			break;
+		}
+	}
+	return trigger;
+}
+
+static void
+wait_for_async_messages(void)
+{
+	struct pending_request *sr;
+	struct timespec timeout;
+	bool timedwait = false;
+	bool nowait = false;
+	int ret;
+
+	/* scan through the list and see if there are any timeouts that
+	 * are earlier than our current timeout.
+	 */
+	TAILQ_FOREACH(sr, &pending_requests.requests, next) {
+		if (sr->type != REQUEST_TYPE_ASYNC)
+			continue;
+		if (!timedwait || timespec_cmp(&sr->async.param->end,
+				&timeout) < 0) {
+			memcpy(&timeout, &sr->async.param->end,
+				sizeof(timeout));
+			timedwait = true;
+		}
+
+		/* sometimes, we don't even wait */
+		if (sr->reply_received) {
+			nowait = true;
+			break;
+		}
+	}
+
+	if (nowait)
+		return;
+
+	do {
+		ret = timedwait ?
+			pthread_cond_timedwait(
+				&pending_requests.async_cond,
+				&pending_requests.lock,
+				&timeout) :
+			pthread_cond_wait(
+				&pending_requests.async_cond,
+				&pending_requests.lock);
+	} while (ret != 0 && ret != ETIMEDOUT);
+
+	/* we've been woken up or timed out */
+}
+
 static void *
 async_reply_handle(void *arg __rte_unused)
 {
-	struct pending_request *sr;
 	struct timeval now;
-	struct timespec timeout, ts_now;
+	struct timespec ts_now;
 	while (1) {
 		struct pending_request *trigger = NULL;
-		int ret;
-		bool nowait = false;
-		bool timedwait = false;
 
 		pthread_mutex_lock(&pending_requests.lock);
 
-		/* scan through the list and see if there are any timeouts that
-		 * are earlier than our current timeout.
-		 */
-		TAILQ_FOREACH(sr, &pending_requests.requests, next) {
-			if (sr->type != REQUEST_TYPE_ASYNC)
-				continue;
-			if (!timedwait || timespec_cmp(&sr->async.param->end,
-					&timeout) < 0) {
-				memcpy(&timeout, &sr->async.param->end,
-					sizeof(timeout));
-				timedwait = true;
-			}
-
-			/* sometimes, we don't even wait */
-			if (sr->reply_received) {
-				nowait = true;
-				break;
-			}
-		}
-
-		if (nowait)
-			ret = 0;
-		else if (timedwait)
-			ret = pthread_cond_timedwait(
-					&pending_requests.async_cond,
-					&pending_requests.lock, &timeout);
-		else
-			ret = pthread_cond_wait(&pending_requests.async_cond,
-					&pending_requests.lock);
+		/* we exit this function holding the lock */
+		wait_for_async_messages();
 
 		if (gettimeofday(&now, NULL) < 0) {
 			RTE_LOG(ERR, EAL, "Cannot get current time\n");
@@ -492,38 +530,22 @@ async_reply_handle(void *arg __rte_unused)
 		ts_now.tv_nsec = now.tv_usec * 1000;
 		ts_now.tv_sec = now.tv_sec;
 
-		if (ret == 0 || ret == ETIMEDOUT) {
-			struct pending_request *next;
-			/* we've either been woken up, or we timed out */
+		do {
+			trigger = check_trigger(&ts_now);
+			/* unlock request list */
+			pthread_mutex_unlock(&pending_requests.lock);
 
-			/* we have still the lock, check if anything needs
-			 * processing.
-			 */
-			TAILQ_FOREACH_SAFE(sr, &pending_requests.requests, next,
-					next) {
-				enum async_action action;
-				if (sr->type != REQUEST_TYPE_ASYNC)
-					continue;
-
-				action = process_async_request(sr, &ts_now);
-				if (action == ACTION_FREE) {
-					TAILQ_REMOVE(&pending_requests.requests,
-							sr, next);
-					free(sr);
-				} else if (action == ACTION_TRIGGER &&
-						trigger == NULL) {
-					TAILQ_REMOVE(&pending_requests.requests,
-							sr, next);
-					trigger = sr;
-				}
+			if (trigger) {
+				trigger_async_action(trigger);
+				free(trigger);
+
+				/* we've triggered a callback, but there may be
+				 * more, so lock the list and check again.
+				 */
+				pthread_mutex_lock(&pending_requests.lock);
 			}
-		}
-		pthread_mutex_unlock(&pending_requests.lock);
-		if (trigger) {
-			trigger_async_action(trigger);
-			free(trigger);
-		}
-	};
+		} while (trigger);
+	}
 
 	RTE_LOG(ERR, EAL, "ERROR: asynchronous requests disabled\n");
 
-- 
2.7.4

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH v2] eal/ipc: stop async IPC loop on callback request
  2018-04-10 15:28 ` [dpdk-dev] [PATCH v2] " Anatoly Burakov
@ 2018-04-13 15:24   ` Tan, Jianfeng
  2018-04-16 23:08     ` Thomas Monjalon
  0 siblings, 1 reply; 7+ messages in thread
From: Tan, Jianfeng @ 2018-04-13 15:24 UTC (permalink / raw)
  To: Anatoly Burakov, dev



On 4/10/2018 11:28 PM, Anatoly Burakov wrote:
> EAL did not stop processing further asynchronous requests on
> encountering a request that should trigger the callback. This
> resulted in erasing valid requests but not triggering them.
>
> Fix this by stopping the loop once we have a request that
> can trigger the callback. Once triggered, we go back to scanning
> the request queue until there are no more callbacks to trigger.
>
> Fixes: f05e26051c15 ("eal: add IPC asynchronous request")
> Cc: anatoly.burakov@intel.com
>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>

Acked-by: Jianfeng Tan <jianfeng.tan@intel.com>

Thanks!

> ---
>
> Notes:
>      Took the opportunity to simplify some code as well.
>      
>      The change is a bit more substantial, hence dropping ack.
>
>   lib/librte_eal/common/eal_common_proc.c | 150 ++++++++++++++++++--------------
>   1 file changed, 86 insertions(+), 64 deletions(-)
>
> diff --git a/lib/librte_eal/common/eal_common_proc.c b/lib/librte_eal/common/eal_common_proc.c
> index f98622f..c888c84 100644
> --- a/lib/librte_eal/common/eal_common_proc.c
> +++ b/lib/librte_eal/common/eal_common_proc.c
> @@ -441,49 +441,87 @@ trigger_async_action(struct pending_request *sr)
>   	free(sr->request);
>   }
>   
> +static struct pending_request *
> +check_trigger(struct timespec *ts)
> +{
> +	struct pending_request *next, *cur, *trigger = NULL;
> +
> +	TAILQ_FOREACH_SAFE(cur, &pending_requests.requests, next, next) {
> +		enum async_action action;
> +		if (cur->type != REQUEST_TYPE_ASYNC)
> +			continue;
> +
> +		action = process_async_request(cur, ts);
> +		if (action == ACTION_FREE) {
> +			TAILQ_REMOVE(&pending_requests.requests, cur, next);
> +			free(cur);
> +		} else if (action == ACTION_TRIGGER) {
> +			TAILQ_REMOVE(&pending_requests.requests, cur, next);
> +			trigger = cur;
> +			break;
> +		}
> +	}
> +	return trigger;
> +}
> +
> +static void
> +wait_for_async_messages(void)
> +{
> +	struct pending_request *sr;
> +	struct timespec timeout;
> +	bool timedwait = false;
> +	bool nowait = false;
> +	int ret;
> +
> +	/* scan through the list and see if there are any timeouts that
> +	 * are earlier than our current timeout.
> +	 */
> +	TAILQ_FOREACH(sr, &pending_requests.requests, next) {
> +		if (sr->type != REQUEST_TYPE_ASYNC)
> +			continue;
> +		if (!timedwait || timespec_cmp(&sr->async.param->end,
> +				&timeout) < 0) {
> +			memcpy(&timeout, &sr->async.param->end,
> +				sizeof(timeout));
> +			timedwait = true;
> +		}
> +
> +		/* sometimes, we don't even wait */
> +		if (sr->reply_received) {
> +			nowait = true;
> +			break;
> +		}
> +	}
> +
> +	if (nowait)
> +		return;
> +
> +	do {
> +		ret = timedwait ?
> +			pthread_cond_timedwait(
> +				&pending_requests.async_cond,
> +				&pending_requests.lock,
> +				&timeout) :
> +			pthread_cond_wait(
> +				&pending_requests.async_cond,
> +				&pending_requests.lock);
> +	} while (ret != 0 && ret != ETIMEDOUT);
> +
> +	/* we've been woken up or timed out */
> +}
> +
>   static void *
>   async_reply_handle(void *arg __rte_unused)
>   {
> -	struct pending_request *sr;
>   	struct timeval now;
> -	struct timespec timeout, ts_now;
> +	struct timespec ts_now;
>   	while (1) {
>   		struct pending_request *trigger = NULL;
> -		int ret;
> -		bool nowait = false;
> -		bool timedwait = false;
>   
>   		pthread_mutex_lock(&pending_requests.lock);
>   
> -		/* scan through the list and see if there are any timeouts that
> -		 * are earlier than our current timeout.
> -		 */
> -		TAILQ_FOREACH(sr, &pending_requests.requests, next) {
> -			if (sr->type != REQUEST_TYPE_ASYNC)
> -				continue;
> -			if (!timedwait || timespec_cmp(&sr->async.param->end,
> -					&timeout) < 0) {
> -				memcpy(&timeout, &sr->async.param->end,
> -					sizeof(timeout));
> -				timedwait = true;
> -			}
> -
> -			/* sometimes, we don't even wait */
> -			if (sr->reply_received) {
> -				nowait = true;
> -				break;
> -			}
> -		}
> -
> -		if (nowait)
> -			ret = 0;
> -		else if (timedwait)
> -			ret = pthread_cond_timedwait(
> -					&pending_requests.async_cond,
> -					&pending_requests.lock, &timeout);
> -		else
> -			ret = pthread_cond_wait(&pending_requests.async_cond,
> -					&pending_requests.lock);
> +		/* we exit this function holding the lock */
> +		wait_for_async_messages();
>   
>   		if (gettimeofday(&now, NULL) < 0) {
>   			RTE_LOG(ERR, EAL, "Cannot get current time\n");
> @@ -492,38 +530,22 @@ async_reply_handle(void *arg __rte_unused)
>   		ts_now.tv_nsec = now.tv_usec * 1000;
>   		ts_now.tv_sec = now.tv_sec;
>   
> -		if (ret == 0 || ret == ETIMEDOUT) {
> -			struct pending_request *next;
> -			/* we've either been woken up, or we timed out */
> +		do {
> +			trigger = check_trigger(&ts_now);
> +			/* unlock request list */
> +			pthread_mutex_unlock(&pending_requests.lock);
>   
> -			/* we have still the lock, check if anything needs
> -			 * processing.
> -			 */
> -			TAILQ_FOREACH_SAFE(sr, &pending_requests.requests, next,
> -					next) {
> -				enum async_action action;
> -				if (sr->type != REQUEST_TYPE_ASYNC)
> -					continue;
> -
> -				action = process_async_request(sr, &ts_now);
> -				if (action == ACTION_FREE) {
> -					TAILQ_REMOVE(&pending_requests.requests,
> -							sr, next);
> -					free(sr);
> -				} else if (action == ACTION_TRIGGER &&
> -						trigger == NULL) {
> -					TAILQ_REMOVE(&pending_requests.requests,
> -							sr, next);
> -					trigger = sr;
> -				}
> +			if (trigger) {
> +				trigger_async_action(trigger);
> +				free(trigger);
> +
> +				/* we've triggered a callback, but there may be
> +				 * more, so lock the list and check again.
> +				 */
> +				pthread_mutex_lock(&pending_requests.lock);
>   			}
> -		}
> -		pthread_mutex_unlock(&pending_requests.lock);
> -		if (trigger) {
> -			trigger_async_action(trigger);
> -			free(trigger);
> -		}
> -	};
> +		} while (trigger);
> +	}
>   
>   	RTE_LOG(ERR, EAL, "ERROR: asynchronous requests disabled\n");
>   

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH v2] eal/ipc: stop async IPC loop on callback request
  2018-04-13 15:24   ` Tan, Jianfeng
@ 2018-04-16 23:08     ` Thomas Monjalon
  0 siblings, 0 replies; 7+ messages in thread
From: Thomas Monjalon @ 2018-04-16 23:08 UTC (permalink / raw)
  To: Anatoly Burakov; +Cc: dev, Tan, Jianfeng

13/04/2018 17:24, Tan, Jianfeng:
> 
> On 4/10/2018 11:28 PM, Anatoly Burakov wrote:
> > EAL did not stop processing further asynchronous requests on
> > encountering a request that should trigger the callback. This
> > resulted in erasing valid requests but not triggering them.
> >
> > Fix this by stopping the loop once we have a request that
> > can trigger the callback. Once triggered, we go back to scanning
> > the request queue until there are no more callbacks to trigger.
> >
> > Fixes: f05e26051c15 ("eal: add IPC asynchronous request")
> > Cc: anatoly.burakov@intel.com
> >
> > Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> 
> Acked-by: Jianfeng Tan <jianfeng.tan@intel.com>

Applied, thanks

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2018-04-16 23:08 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-10 10:03 [dpdk-dev] [PATCH] eal/ipc: stop async IPC loop on callback request Anatoly Burakov
2018-04-10 13:53 ` Tan, Jianfeng
2018-04-10 14:17   ` Burakov, Anatoly
2018-04-10 15:16     ` Tan, Jianfeng
2018-04-10 15:28 ` [dpdk-dev] [PATCH v2] " Anatoly Burakov
2018-04-13 15:24   ` Tan, Jianfeng
2018-04-16 23:08     ` Thomas Monjalon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).