| Message ID | 1460651707-23768-1-git-send-email-maxim.uvarov@linaro.org |
|---|---|
| State | New |
| Headers | show |
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 >
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 >> > >
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 >>> >> >> >
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 > > > >
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)
- 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(-)