On Thu, Apr 14, 2016 at 2:50 PM, Maxim Uvarov <[email protected]> wrote:
> > > 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 > 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 >>> [email protected] >>> https://lists.linaro.org/mailman/listinfo/lng-odp >>> >> >> >
_______________________________________________ lng-odp mailing list [email protected] https://lists.linaro.org/mailman/listinfo/lng-odp
