linux-generic: rework odp_queue_create

Message ID 1460651707-23768-1-git-send-email-maxim.uvarov@linaro.org
State New
Headers show

Commit Message

Maxim Uvarov April 14, 2016, 4:35 p.m.
- use odp_config_queues() to get number of queues and int cycle
  iterrator. That makes it more easy to reuse that function for
  other platfroms.;
- remove declaring variables to invalid at the bottom and check for
  them down the code;
- make code more easy to read;

Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
---
 platform/linux-generic/odp_queue.c | 36 +++++++++++++++---------------------
 1 file changed, 15 insertions(+), 21 deletions(-)

Comments

Bill Fischofer April 14, 2016, 7:16 p.m. | #1
On Thu, Apr 14, 2016 at 11:35 AM, Maxim Uvarov <maxim.uvarov@linaro.org>
wrote:

> - use odp_config_queues() to get number of queues and int cycle

>   iterrator. That makes it more easy to reuse that function for

>   other platfroms.;

> - remove declaring variables to invalid at the bottom and check for

>   them down the code;

> - make code more easy to read;

>

> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>

> ---

>  platform/linux-generic/odp_queue.c | 36

> +++++++++++++++---------------------

>  1 file changed, 15 insertions(+), 21 deletions(-)

>

> diff --git a/platform/linux-generic/odp_queue.c

> b/platform/linux-generic/odp_queue.c

> index 342ffa2..0f7fd15 100644

> --- a/platform/linux-generic/odp_queue.c

> +++ b/platform/linux-generic/odp_queue.c

> @@ -234,10 +234,8 @@ int odp_queue_lock_count(odp_queue_t handle)

>

>  odp_queue_t odp_queue_create(const char *name, const odp_queue_param_t

> *param)

>  {

> -       uint32_t i;

> +       int i;

>         queue_entry_t *queue;

> -       odp_queue_t handle = ODP_QUEUE_INVALID;

> -       odp_queue_type_t type;

>         odp_queue_param_t default_param;

>

>         if (param == NULL) {

> @@ -245,7 +243,7 @@ odp_queue_t odp_queue_create(const char *name, const

> odp_queue_param_t *param)

>                 param = &default_param;

>         }

>

> -       for (i = 0; i < ODP_CONFIG_QUEUES; i++) {

> +       for (i = 0; i < odp_config_queues(); i++) {

>


This is needlessly inefficient.  The #defines are perfectly OK for
implementation internal use.  This makes an extra function call for each
iteration.


>                 queue = &queue_tbl->queue[i];

>

>                 if (queue->s.status != QUEUE_STATUS_FREE)

> @@ -253,33 +251,29 @@ odp_queue_t odp_queue_create(const char *name, const

> odp_queue_param_t *param)

>

>                 LOCK(&queue->s.lock);

>                 if (queue->s.status == QUEUE_STATUS_FREE) {

> -                       if (queue_init(queue, name, param)) {

> +                       if (queue_init(queue, name, param) ||

> +                           queue->s.handle == ODP_QUEUE_INVALID) {

>


This is a redundant check as this field is initialized part part of
queue_init_global() and does not change


>                                 UNLOCK(&queue->s.lock);

> -                               return handle;

> +                               return ODP_QUEUE_INVALID;

>                         }

>

> -                       type = queue->s.type;

> -

> -                       if (type == ODP_QUEUE_TYPE_SCHED)

> +                       if (queue->s.type == ODP_QUEUE_TYPE_SCHED) {

>                                 queue->s.status = QUEUE_STATUS_NOTSCHED;

> -                       else

> +                               if (schedule_queue_init(queue)) {

>


In the original code this routine is called with the queue unlocked. While
holding the lock across this call should still work, in general we want to
release locks as soon as possible so I don't see this as an improvement.


> +                                       ODP_ERR("schedule queue init

> failed\n");

> +                                       UNLOCK(&queue->s.lock);

> +                                       return ODP_QUEUE_INVALID;

> +                               }

> +                       } else {

>                                 queue->s.status = QUEUE_STATUS_READY;

> -

> -                       handle = queue->s.handle;

> +                       }

>                         UNLOCK(&queue->s.lock);

> -                       break;

> +                       return queue->s.handle;

>                 }

>                 UNLOCK(&queue->s.lock);

>         }

>

> -       if (handle != ODP_QUEUE_INVALID && type == ODP_QUEUE_TYPE_SCHED) {

> -               if (schedule_queue_init(queue)) {

> -                       ODP_ERR("schedule queue init failed\n");

> -                       return ODP_QUEUE_INVALID;

> -               }

> -       }

> -

> -       return handle;

> +       return ODP_QUEUE_INVALID;

>  }

>

>  void queue_destroy_finalize(queue_entry_t *queue)

> --

> 2.7.1.250.gff4ea60

>

> _______________________________________________

> lng-odp mailing list

> lng-odp@lists.linaro.org

> https://lists.linaro.org/mailman/listinfo/lng-odp

>
Maxim Uvarov April 14, 2016, 7:50 p.m. | #2
On 14 April 2016 at 22:16, Bill Fischofer <bill.fischofer@linaro.org> wrote:

>

>

> On Thu, Apr 14, 2016 at 11:35 AM, Maxim Uvarov <maxim.uvarov@linaro.org>

> wrote:

>

>> - use odp_config_queues() to get number of queues and int cycle

>>   iterrator. That makes it more easy to reuse that function for

>>   other platfroms.;

>> - remove declaring variables to invalid at the bottom and check for

>>   them down the code;

>> - make code more easy to read;

>>

>> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>

>> ---

>>  platform/linux-generic/odp_queue.c | 36

>> +++++++++++++++---------------------

>>  1 file changed, 15 insertions(+), 21 deletions(-)

>>

>> diff --git a/platform/linux-generic/odp_queue.c

>> b/platform/linux-generic/odp_queue.c

>> index 342ffa2..0f7fd15 100644

>> --- a/platform/linux-generic/odp_queue.c

>> +++ b/platform/linux-generic/odp_queue.c

>> @@ -234,10 +234,8 @@ int odp_queue_lock_count(odp_queue_t handle)

>>

>>  odp_queue_t odp_queue_create(const char *name, const odp_queue_param_t

>> *param)

>>  {

>> -       uint32_t i;

>> +       int i;

>>         queue_entry_t *queue;

>> -       odp_queue_t handle = ODP_QUEUE_INVALID;

>> -       odp_queue_type_t type;

>>         odp_queue_param_t default_param;

>>

>>         if (param == NULL) {

>> @@ -245,7 +243,7 @@ odp_queue_t odp_queue_create(const char *name, const

>> odp_queue_param_t *param)

>>                 param = &default_param;

>>         }

>>

>> -       for (i = 0; i < ODP_CONFIG_QUEUES; i++) {

>> +       for (i = 0; i < odp_config_queues(); i++) {

>>

>

> This is needlessly inefficient.  The #defines are perfectly OK for

> implementation internal use.  This makes an extra function call for each

> iteration.

>

>

>>                 queue = &queue_tbl->queue[i];

>>

>>                 if (queue->s.status != QUEUE_STATUS_FREE)

>> @@ -253,33 +251,29 @@ odp_queue_t odp_queue_create(const char *name,

>> const odp_queue_param_t *param)

>>

>>                 LOCK(&queue->s.lock);

>>                 if (queue->s.status == QUEUE_STATUS_FREE) {

>> -                       if (queue_init(queue, name, param)) {

>> +                       if (queue_init(queue, name, param) ||

>> +                           queue->s.handle == ODP_QUEUE_INVALID) {

>>

>

> This is a redundant check as this field is initialized part part of

> queue_init_global() and does not change

>

>

>>                                 UNLOCK(&queue->s.lock);

>> -                               return handle;

>> +                               return ODP_QUEUE_INVALID;

>>                         }

>>

>> -                       type = queue->s.type;

>> -

>> -                       if (type == ODP_QUEUE_TYPE_SCHED)

>> +                       if (queue->s.type == ODP_QUEUE_TYPE_SCHED) {

>>                                 queue->s.status = QUEUE_STATUS_NOTSCHED;

>> -                       else

>> +                               if (schedule_queue_init(queue)) {

>>

>

> In the original code this routine is called with the queue unlocked. While

> holding the lock across this call should still work, in general we want to

> release locks as soon as possible so I don't see this as an improvement.

>

>


ah, that is main comment.  If we want schedule_queue_init() run without
lock than it's better to use current code, I think, so I will apply your
patch.


One small note about current code which I just realized (might be it's too
late for today).

schedule_queue_init() may fail. Should we in that case set queue->s.status
back to QUEUE_STATUS_FREE  before return ODP_QUEUE_INVALID ?

Maxim.




> +                                       ODP_ERR("schedule queue init

>> failed\n");

>> +                                       UNLOCK(&queue->s.lock);

>> +                                       return ODP_QUEUE_INVALID;

>> +                               }

>> +                       } else {

>>                                 queue->s.status = QUEUE_STATUS_READY;

>> -

>> -                       handle = queue->s.handle;

>> +                       }

>>                         UNLOCK(&queue->s.lock);

>> -                       break;

>> +                       return queue->s.handle;

>>                 }

>>                 UNLOCK(&queue->s.lock);

>>         }

>>

>> -       if (handle != ODP_QUEUE_INVALID && type == ODP_QUEUE_TYPE_SCHED) {

>> -               if (schedule_queue_init(queue)) {

>> -                       ODP_ERR("schedule queue init failed\n");

>> -                       return ODP_QUEUE_INVALID;

>> -               }

>> -       }

>> -

>> -       return handle;

>> +       return ODP_QUEUE_INVALID;

>>  }

>>

>>  void queue_destroy_finalize(queue_entry_t *queue)

>> --

>> 2.7.1.250.gff4ea60

>>

>> _______________________________________________

>> lng-odp mailing list

>> lng-odp@lists.linaro.org

>> https://lists.linaro.org/mailman/listinfo/lng-odp

>>

>

>
Bill Fischofer April 14, 2016, 8:29 p.m. | #3
On Thu, Apr 14, 2016 at 2:50 PM, Maxim Uvarov <maxim.uvarov@linaro.org>
wrote:

>

>

> On 14 April 2016 at 22:16, Bill Fischofer <bill.fischofer@linaro.org>

> wrote:

>

>>

>>

>> On Thu, Apr 14, 2016 at 11:35 AM, Maxim Uvarov <maxim.uvarov@linaro.org>

>> wrote:

>>

>>> - use odp_config_queues() to get number of queues and int cycle

>>>   iterrator. That makes it more easy to reuse that function for

>>>   other platfroms.;

>>> - remove declaring variables to invalid at the bottom and check for

>>>   them down the code;

>>> - make code more easy to read;

>>>

>>> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>

>>> ---

>>>  platform/linux-generic/odp_queue.c | 36

>>> +++++++++++++++---------------------

>>>  1 file changed, 15 insertions(+), 21 deletions(-)

>>>

>>> diff --git a/platform/linux-generic/odp_queue.c

>>> b/platform/linux-generic/odp_queue.c

>>> index 342ffa2..0f7fd15 100644

>>> --- a/platform/linux-generic/odp_queue.c

>>> +++ b/platform/linux-generic/odp_queue.c

>>> @@ -234,10 +234,8 @@ int odp_queue_lock_count(odp_queue_t handle)

>>>

>>>  odp_queue_t odp_queue_create(const char *name, const odp_queue_param_t

>>> *param)

>>>  {

>>> -       uint32_t i;

>>> +       int i;

>>>         queue_entry_t *queue;

>>> -       odp_queue_t handle = ODP_QUEUE_INVALID;

>>> -       odp_queue_type_t type;

>>>         odp_queue_param_t default_param;

>>>

>>>         if (param == NULL) {

>>> @@ -245,7 +243,7 @@ odp_queue_t odp_queue_create(const char *name, const

>>> odp_queue_param_t *param)

>>>                 param = &default_param;

>>>         }

>>>

>>> -       for (i = 0; i < ODP_CONFIG_QUEUES; i++) {

>>> +       for (i = 0; i < odp_config_queues(); i++) {

>>>

>>

>> This is needlessly inefficient.  The #defines are perfectly OK for

>> implementation internal use.  This makes an extra function call for each

>> iteration.

>>

>>

>>>                 queue = &queue_tbl->queue[i];

>>>

>>>                 if (queue->s.status != QUEUE_STATUS_FREE)

>>> @@ -253,33 +251,29 @@ odp_queue_t odp_queue_create(const char *name,

>>> const odp_queue_param_t *param)

>>>

>>>                 LOCK(&queue->s.lock);

>>>                 if (queue->s.status == QUEUE_STATUS_FREE) {

>>> -                       if (queue_init(queue, name, param)) {

>>> +                       if (queue_init(queue, name, param) ||

>>> +                           queue->s.handle == ODP_QUEUE_INVALID) {

>>>

>>

>> This is a redundant check as this field is initialized part part of

>> queue_init_global() and does not change

>>

>>

>>>                                 UNLOCK(&queue->s.lock);

>>> -                               return handle;

>>> +                               return ODP_QUEUE_INVALID;

>>>                         }

>>>

>>> -                       type = queue->s.type;

>>> -

>>> -                       if (type == ODP_QUEUE_TYPE_SCHED)

>>> +                       if (queue->s.type == ODP_QUEUE_TYPE_SCHED) {

>>>                                 queue->s.status = QUEUE_STATUS_NOTSCHED;

>>> -                       else

>>> +                               if (schedule_queue_init(queue)) {

>>>

>>

>> In the original code this routine is called with the queue unlocked.

>> While holding the lock across this call should still work, in general we

>> want to release locks as soon as possible so I don't see this as an

>> improvement.

>>

>>

>

> ah, that is main comment.  If we want schedule_queue_init() run without

> lock than it's better to use current code, I think, so I will apply your

> patch.

>

>

> One small note about current code which I just realized (might be it's too

> late for today).

>

> schedule_queue_init() may fail. Should we in that case set queue->s.status

> back to QUEUE_STATUS_FREE  before

>


That's a good catch.  However that should be done by odp_queue_destroy()
since at the point of the failure the queue lock is not held.  It's a one
line fix.  If you want to add it to this patch I'm fine with that or I can
submit a follow-up bug and patch.


> return ODP_QUEUE_INVALID ?

>

> Maxim.

>

>

>

>

>> +                                       ODP_ERR("schedule queue init

>>> failed\n");

>>> +                                       UNLOCK(&queue->s.lock);

>>> +                                       return ODP_QUEUE_INVALID;

>>> +                               }

>>> +                       } else {

>>>                                 queue->s.status = QUEUE_STATUS_READY;

>>> -

>>> -                       handle = queue->s.handle;

>>> +                       }

>>>                         UNLOCK(&queue->s.lock);

>>> -                       break;

>>> +                       return queue->s.handle;

>>>                 }

>>>                 UNLOCK(&queue->s.lock);

>>>         }

>>>

>>> -       if (handle != ODP_QUEUE_INVALID && type == ODP_QUEUE_TYPE_SCHED)

>>> {

>>> -               if (schedule_queue_init(queue)) {

>>> -                       ODP_ERR("schedule queue init failed\n");

>>> -                       return ODP_QUEUE_INVALID;

>>> -               }

>>> -       }

>>> -

>>> -       return handle;

>>> +       return ODP_QUEUE_INVALID;

>>>  }

>>>

>>>  void queue_destroy_finalize(queue_entry_t *queue)

>>> --

>>> 2.7.1.250.gff4ea60

>>>

>>> _______________________________________________

>>> lng-odp mailing list

>>> lng-odp@lists.linaro.org

>>> https://lists.linaro.org/mailman/listinfo/lng-odp

>>>

>>

>>

>
Maxim Uvarov April 15, 2016, 5:07 a.m. | #4
On 04/14/16 23:29, Bill Fischofer wrote:
>
>
> On Thu, Apr 14, 2016 at 2:50 PM, Maxim Uvarov <maxim.uvarov@linaro.org 
> <mailto:maxim.uvarov@linaro.org>> wrote:
>
>
>
>     On 14 April 2016 at 22:16, Bill Fischofer
>     <bill.fischofer@linaro.org <mailto:bill.fischofer@linaro.org>> wrote:
>
>
>
>         On Thu, Apr 14, 2016 at 11:35 AM, Maxim Uvarov
>         <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org>> wrote:
>
>             - use odp_config_queues() to get number of queues and int
>             cycle
>               iterrator. That makes it more easy to reuse that
>             function for
>               other platfroms.;
>             - remove declaring variables to invalid at the bottom and
>             check for
>               them down the code;
>             - make code more easy to read;
>
>             Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org
>             <mailto:maxim.uvarov@linaro.org>>
>             ---
>              platform/linux-generic/odp_queue.c | 36
>             +++++++++++++++---------------------
>              1 file changed, 15 insertions(+), 21 deletions(-)
>
>             diff --git a/platform/linux-generic/odp_queue.c
>             b/platform/linux-generic/odp_queue.c
>             index 342ffa2..0f7fd15 100644
>             --- a/platform/linux-generic/odp_queue.c
>             +++ b/platform/linux-generic/odp_queue.c
>             @@ -234,10 +234,8 @@ int odp_queue_lock_count(odp_queue_t
>             handle)
>
>              odp_queue_t odp_queue_create(const char *name, const
>             odp_queue_param_t *param)
>              {
>             -       uint32_t i;
>             +       int i;
>                     queue_entry_t *queue;
>             -       odp_queue_t handle = ODP_QUEUE_INVALID;
>             -       odp_queue_type_t type;
>                     odp_queue_param_t default_param;
>
>                     if (param == NULL) {
>             @@ -245,7 +243,7 @@ odp_queue_t odp_queue_create(const
>             char *name, const odp_queue_param_t *param)
>                             param = &default_param;
>                     }
>
>             -       for (i = 0; i < ODP_CONFIG_QUEUES; i++) {
>             +       for (i = 0; i < odp_config_queues(); i++) {
>
>
>         This is needlessly inefficient. The #defines are perfectly OK
>         for implementation internal use.  This makes an extra function
>         call for each iteration.
>
>                             queue = &queue_tbl->queue[i];
>
>                             if (queue->s.status != QUEUE_STATUS_FREE)
>             @@ -253,33 +251,29 @@ odp_queue_t odp_queue_create(const
>             char *name, const odp_queue_param_t *param)
>
>             LOCK(&queue->s.lock);
>                             if (queue->s.status == QUEUE_STATUS_FREE) {
>             -                       if (queue_init(queue, name, param)) {
>             +                       if (queue_init(queue, name, param) ||
>             +  queue->s.handle == ODP_QUEUE_INVALID) {
>
>
>         This is a redundant check as this field is initialized part
>         part of queue_init_global() and does not change
>
>             UNLOCK(&queue->s.lock);
>             -  return handle;
>             +  return ODP_QUEUE_INVALID;
>                                     }
>
>             -                       type = queue->s.type;
>             -
>             -                       if (type == ODP_QUEUE_TYPE_SCHED)
>             +                       if (queue->s.type ==
>             ODP_QUEUE_TYPE_SCHED) {
>             queue->s.status = QUEUE_STATUS_NOTSCHED;
>             -                       else
>             +                               if
>             (schedule_queue_init(queue)) {
>
>
>         In the original code this routine is called with the queue
>         unlocked. While holding the lock across this call should still
>         work, in general we want to release locks as soon as possible
>         so I don't see this as an improvement.
>
>
>     ah, that is main comment.  If we want schedule_queue_init() run
>     without lock than it's better to use current code, I think, so I
>     will apply your patch.
>
>
>     One small note about current code which I just realized (might be
>     it's too late for today).
>
>     schedule_queue_init() may fail. Should we in that case set
>     queue->s.status back to QUEUE_STATUS_FREE  before
>
>
> That's a good catch.  However that should be done by 
> odp_queue_destroy() since at the point of the failure the queue lock 
> is not held.  It's a one line fix.  If you want to add it to this 
> patch I'm fine with that or I can submit a follow-up bug and patch.

I think separate patch for that to not depend on original patch.

Maxim.

>     return ODP_QUEUE_INVALID ?
>
>     Maxim.
>
>
>             +      ODP_ERR("schedule queue init failed\n");
>             +  UNLOCK(&queue->s.lock);
>             +      return ODP_QUEUE_INVALID;
>             +                               }
>             +                       } else {
>             queue->s.status = QUEUE_STATUS_READY;
>             -
>             -                       handle = queue->s.handle;
>             +                       }
>             UNLOCK(&queue->s.lock);
>             -                       break;
>             +                       return queue->s.handle;
>                             }
>             UNLOCK(&queue->s.lock);
>                     }
>
>             -       if (handle != ODP_QUEUE_INVALID && type ==
>             ODP_QUEUE_TYPE_SCHED) {
>             -               if (schedule_queue_init(queue)) {
>             -  ODP_ERR("schedule queue init failed\n");
>             -                       return ODP_QUEUE_INVALID;
>             -               }
>             -       }
>             -
>             -       return handle;
>             +       return ODP_QUEUE_INVALID;
>              }
>
>              void queue_destroy_finalize(queue_entry_t *queue)
>             --
>             2.7.1.250.gff4ea60
>
>             _______________________________________________
>             lng-odp mailing list
>             lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>             https://lists.linaro.org/mailman/listinfo/lng-odp
>
>
>
>

Patch

diff --git a/platform/linux-generic/odp_queue.c b/platform/linux-generic/odp_queue.c
index 342ffa2..0f7fd15 100644
--- a/platform/linux-generic/odp_queue.c
+++ b/platform/linux-generic/odp_queue.c
@@ -234,10 +234,8 @@  int odp_queue_lock_count(odp_queue_t handle)
 
 odp_queue_t odp_queue_create(const char *name, const odp_queue_param_t *param)
 {
-	uint32_t i;
+	int i;
 	queue_entry_t *queue;
-	odp_queue_t handle = ODP_QUEUE_INVALID;
-	odp_queue_type_t type;
 	odp_queue_param_t default_param;
 
 	if (param == NULL) {
@@ -245,7 +243,7 @@  odp_queue_t odp_queue_create(const char *name, const odp_queue_param_t *param)
 		param = &default_param;
 	}
 
-	for (i = 0; i < ODP_CONFIG_QUEUES; i++) {
+	for (i = 0; i < odp_config_queues(); i++) {
 		queue = &queue_tbl->queue[i];
 
 		if (queue->s.status != QUEUE_STATUS_FREE)
@@ -253,33 +251,29 @@  odp_queue_t odp_queue_create(const char *name, const odp_queue_param_t *param)
 
 		LOCK(&queue->s.lock);
 		if (queue->s.status == QUEUE_STATUS_FREE) {
-			if (queue_init(queue, name, param)) {
+			if (queue_init(queue, name, param) ||
+			    queue->s.handle == ODP_QUEUE_INVALID) {
 				UNLOCK(&queue->s.lock);
-				return handle;
+				return ODP_QUEUE_INVALID;
 			}
 
-			type = queue->s.type;
-
-			if (type == ODP_QUEUE_TYPE_SCHED)
+			if (queue->s.type == ODP_QUEUE_TYPE_SCHED) {
 				queue->s.status = QUEUE_STATUS_NOTSCHED;
-			else
+				if (schedule_queue_init(queue)) {
+					ODP_ERR("schedule queue init failed\n");
+					UNLOCK(&queue->s.lock);
+					return ODP_QUEUE_INVALID;
+				}
+			} else {
 				queue->s.status = QUEUE_STATUS_READY;
-
-			handle = queue->s.handle;
+			}
 			UNLOCK(&queue->s.lock);
-			break;
+			return queue->s.handle;
 		}
 		UNLOCK(&queue->s.lock);
 	}
 
-	if (handle != ODP_QUEUE_INVALID && type == ODP_QUEUE_TYPE_SCHED) {
-		if (schedule_queue_init(queue)) {
-			ODP_ERR("schedule queue init failed\n");
-			return ODP_QUEUE_INVALID;
-		}
-	}
-
-	return handle;
+	return ODP_QUEUE_INVALID;
 }
 
 void queue_destroy_finalize(queue_entry_t *queue)