On 14 April 2016 at 22:16, Bill Fischofer <[email protected]> wrote:
> > > On Thu, Apr 14, 2016 at 11:35 AM, Maxim Uvarov <[email protected]> > 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 <[email protected]> >> --- >> 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 >> [email protected] >> https://lists.linaro.org/mailman/listinfo/lng-odp >> > >
_______________________________________________ lng-odp mailing list [email protected] https://lists.linaro.org/mailman/listinfo/lng-odp
