| Message ID | 20170612111133.1018-1-petri.savolainen@linaro.org |
|---|---|
| State | Accepted |
| Commit | 5a38050f431d904b7354cd50611304da0a601727 |
| Headers | show |
On 12.06.2017 14:11, Petri Savolainen wrote: > Clean up function and parameter naming after modular interface > patch. Queue_t type is referred as "queue internal": queue_int or > q_int. Term "handle" is reserved for API level handles (e.g. > odp_queue_t, odp_pktio_t, etc) through out linux-gen implementation. q_int/queue_int is a matter of preference, the rest should definitely go in. -- With best wishes Dmitry
On 12 June 2017 at 06:11, Petri Savolainen <petri.savolainen@linaro.org> wrote: > Clean up function and parameter naming after modular interface > patch. Queue_t type is referred as "queue internal": queue_int or > q_int. Term "handle" is reserved for API level handles (e.g. > odp_queue_t, odp_pktio_t, etc) through out linux-gen implementation. > "queue_t" type should be referred to as "handle_int". "handle_int" is clearly different from "handle". If we look at the definition of "queue_t": typedef struct { char dummy; } _queue_t; typedef _queue_t *queue_t; it is nothing but a definition of a handle. Why should it be called with some other name and create confusion? Just like how odp_queue_t is an abstract type, queue_t is also an abstract type. Just like how odp_queue_t is a handle, queue_t is also a handle, albeit a handle towards internal components. "queue internal" is same as internal implementation definition of the queue data structure. > Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org> > --- > platform/linux-generic/include/odp_queue_if.h | 28 +-- > .../linux-generic/include/odp_queue_internal.h | 4 +- > platform/linux-generic/include/odp_schedule_if.h | 2 +- > platform/linux-generic/odp_queue.c | 218 +++++++++++---------- > platform/linux-generic/odp_schedule.c | 4 +- > platform/linux-generic/odp_schedule_iquery.c | 4 +- > platform/linux-generic/odp_schedule_sp.c | 4 +- > 7 files changed, 133 insertions(+), 131 deletions(-) > > diff --git a/platform/linux-generic/include/odp_queue_if.h b/platform/linux-generic/include/odp_queue_if.h > index 4359435f..168d0e9e 100644 > --- a/platform/linux-generic/include/odp_queue_if.h > +++ b/platform/linux-generic/include/odp_queue_if.h > @@ -53,24 +53,24 @@ typedef int (*queue_term_global_fn_t)(void); > typedef int (*queue_init_local_fn_t)(void); > typedef int (*queue_term_local_fn_t)(void); > typedef queue_t (*queue_from_ext_fn_t)(odp_queue_t handle); > -typedef odp_queue_t (*queue_to_ext_fn_t)(queue_t handle); > -typedef int (*queue_enq_fn_t)(queue_t handle, odp_buffer_hdr_t *); > -typedef int (*queue_enq_multi_fn_t)(queue_t handle, odp_buffer_hdr_t **, int); > -typedef odp_buffer_hdr_t *(*queue_deq_fn_t)(queue_t handle); > -typedef int (*queue_deq_multi_fn_t)(queue_t handle, odp_buffer_hdr_t **, int); > -typedef odp_pktout_queue_t (*queue_get_pktout_fn_t)(queue_t handle); > -typedef void (*queue_set_pktout_fn_t)(queue_t handle, odp_pktio_t pktio, > +typedef odp_queue_t (*queue_to_ext_fn_t)(queue_t q_int); > +typedef int (*queue_enq_fn_t)(queue_t q_int, odp_buffer_hdr_t *); > +typedef int (*queue_enq_multi_fn_t)(queue_t q_int, odp_buffer_hdr_t **, int); > +typedef odp_buffer_hdr_t *(*queue_deq_fn_t)(queue_t q_int); > +typedef int (*queue_deq_multi_fn_t)(queue_t q_int, odp_buffer_hdr_t **, int); > +typedef odp_pktout_queue_t (*queue_get_pktout_fn_t)(queue_t q_int); > +typedef void (*queue_set_pktout_fn_t)(queue_t q_int, odp_pktio_t pktio, > int index); > -typedef odp_pktin_queue_t (*queue_get_pktin_fn_t)(queue_t handle); > -typedef void (*queue_set_pktin_fn_t)(queue_t handle, odp_pktio_t pktio, > +typedef odp_pktin_queue_t (*queue_get_pktin_fn_t)(queue_t q_int); > +typedef void (*queue_set_pktin_fn_t)(queue_t q_int, odp_pktio_t pktio, > int index); > -typedef void (*queue_set_enq_fn_t)(queue_t handle, queue_enq_fn_t func); > -typedef void (*queue_set_enq_multi_fn_t)(queue_t handle, > +typedef void (*queue_set_enq_fn_t)(queue_t q_int, queue_enq_fn_t func); > +typedef void (*queue_set_enq_multi_fn_t)(queue_t q_int, > queue_enq_multi_fn_t func); > -typedef void (*queue_set_deq_fn_t)(queue_t handle, queue_deq_fn_t func); > -typedef void (*queue_set_deq_multi_fn_t)(queue_t handle, > +typedef void (*queue_set_deq_fn_t)(queue_t q_int, queue_deq_fn_t func); > +typedef void (*queue_set_deq_multi_fn_t)(queue_t q_int, > queue_deq_multi_fn_t func); > -typedef void (*queue_set_type_fn_t)(queue_t handle, odp_queue_type_t type); > +typedef void (*queue_set_type_fn_t)(queue_t q_int, odp_queue_type_t type); > > /* Queue functions towards other internal components */ > typedef struct { > diff --git a/platform/linux-generic/include/odp_queue_internal.h b/platform/linux-generic/include/odp_queue_internal.h > index 0c31ce8a..d79abd23 100644 > --- a/platform/linux-generic/include/odp_queue_internal.h > +++ b/platform/linux-generic/include/odp_queue_internal.h > @@ -78,9 +78,9 @@ static inline uint32_t queue_to_id(odp_queue_t handle) > return _odp_typeval(handle) - 1; > } > > -static inline queue_entry_t *qentry_from_int(queue_t handle) > +static inline queue_entry_t *qentry_from_int(queue_t q_int) q_int should be "handle_int" > { > - return (queue_entry_t *)(void *)(handle); > + return (queue_entry_t *)(void *)(q_int); > } > > static inline queue_t qentry_to_int(queue_entry_t *qentry) > diff --git a/platform/linux-generic/include/odp_schedule_if.h b/platform/linux-generic/include/odp_schedule_if.h > index 9adacef7..5d10cd37 100644 > --- a/platform/linux-generic/include/odp_schedule_if.h > +++ b/platform/linux-generic/include/odp_schedule_if.h > @@ -26,7 +26,7 @@ typedef int (*schedule_init_queue_fn_t)(uint32_t queue_index, > typedef void (*schedule_destroy_queue_fn_t)(uint32_t queue_index); > typedef int (*schedule_sched_queue_fn_t)(uint32_t queue_index); > typedef int (*schedule_unsched_queue_fn_t)(uint32_t queue_index); > -typedef int (*schedule_ord_enq_multi_fn_t)(queue_t handle, > +typedef int (*schedule_ord_enq_multi_fn_t)(queue_t q_int, q_int should be "handle_int" > void *buf_hdr[], int num, int *ret); > typedef int (*schedule_init_global_fn_t)(void); > typedef int (*schedule_term_global_fn_t)(void); > diff --git a/platform/linux-generic/odp_queue.c b/platform/linux-generic/odp_queue.c > index 3e18f578..19945584 100644 > --- a/platform/linux-generic/odp_queue.c > +++ b/platform/linux-generic/odp_queue.c > @@ -35,20 +35,22 @@ > #include <string.h> > #include <inttypes.h> > > +static int queue_init(queue_entry_t *queue, const char *name, > + const odp_queue_param_t *param); > + This is unnecessary for this patch. Don't waste reviewer's time with unwanted changes. Unwanted changes have been discussed in the context of other patches and clearly agreed that they should not be done. In fact, such changes have been reversed. > typedef struct queue_table_t { > queue_entry_t queue[ODP_CONFIG_QUEUES]; > } queue_table_t; > > static queue_table_t *queue_tbl; > > -static queue_t queue_from_ext(odp_queue_t handle); > -static int _queue_enq(queue_t handle, odp_buffer_hdr_t *buf_hdr); > -static odp_buffer_hdr_t *_queue_deq(queue_t handle); > +static inline queue_entry_t *handle_to_qentry(odp_queue_t handle) Why is there a need to add this function? We already have 'queue_from_ext' and 'qentry_from_int' which are a must to implement. The functionality provided by 'handle_to_qentry' can be achieved from these two functions. 'handle_to_qentry' is adding another layer of wrapper. This adds to code complexity. > +{ > + uint32_t queue_id; > > -static int _queue_enq_multi(queue_t handle, odp_buffer_hdr_t *buf_hdr[], > - int num); > -static int _queue_deq_multi(queue_t handle, odp_buffer_hdr_t *buf_hdr[], > - int num); > + queue_id = queue_to_id(handle); > + return get_qentry(queue_id); > +} > > static inline odp_queue_t queue_from_id(uint32_t queue_id) > { > @@ -70,50 +72,6 @@ queue_entry_t *get_qentry(uint32_t queue_id) > return &queue_tbl->queue[queue_id]; > } > > -static int queue_init(queue_entry_t *queue, const char *name, > - const odp_queue_param_t *param) > -{ > - if (name == NULL) { > - queue->s.name[0] = 0; > - } else { > - strncpy(queue->s.name, name, ODP_QUEUE_NAME_LEN - 1); > - queue->s.name[ODP_QUEUE_NAME_LEN - 1] = 0; > - } > - memcpy(&queue->s.param, param, sizeof(odp_queue_param_t)); > - if (queue->s.param.sched.lock_count > sched_fn->max_ordered_locks()) > - return -1; > - > - if (param->type == ODP_QUEUE_TYPE_SCHED) { > - queue->s.param.deq_mode = ODP_QUEUE_OP_DISABLED; > - > - if (param->sched.sync == ODP_SCHED_SYNC_ORDERED) { > - unsigned i; > - > - odp_atomic_init_u64(&queue->s.ordered.ctx, 0); > - odp_atomic_init_u64(&queue->s.ordered.next_ctx, 0); > - > - for (i = 0; i < queue->s.param.sched.lock_count; i++) > - odp_atomic_init_u64(&queue->s.ordered.lock[i], > - 0); > - } > - } > - queue->s.type = queue->s.param.type; > - > - queue->s.enqueue = _queue_enq; > - queue->s.dequeue = _queue_deq; > - queue->s.enqueue_multi = _queue_enq_multi; > - queue->s.dequeue_multi = _queue_deq_multi; > - > - queue->s.pktin = PKTIN_INVALID; > - queue->s.pktout = PKTOUT_INVALID; > - > - queue->s.head = NULL; > - queue->s.tail = NULL; > - > - return 0; > -} > - > - Unnecessary change > static int queue_init_global(void) > { > uint32_t i; > @@ -204,27 +162,27 @@ static int queue_capability(odp_queue_capability_t *capa) > > static odp_queue_type_t queue_type(odp_queue_t handle) > { > - return qentry_from_int(queue_from_ext(handle))->s.type; > + return handle_to_qentry(handle)->s.type; No need to introduce another function. qentry_from_int(queue_from_ext(handle)) clearly shows the current status that there exists an internal handle. handle_to_qentry(handle) hides that fact and makes code less readable. This comment applies to all the instances of similar change. > } > > static odp_schedule_sync_t queue_sched_type(odp_queue_t handle) > { > - return qentry_from_int(queue_from_ext(handle))->s.param.sched.sync; > + return handle_to_qentry(handle)->s.param.sched.sync; > } > > static odp_schedule_prio_t queue_sched_prio(odp_queue_t handle) > { > - return qentry_from_int(queue_from_ext(handle))->s.param.sched.prio; > + return handle_to_qentry(handle)->s.param.sched.prio; > } > > static odp_schedule_group_t queue_sched_group(odp_queue_t handle) > { > - return qentry_from_int(queue_from_ext(handle))->s.param.sched.group; > + return handle_to_qentry(handle)->s.param.sched.group; > } > > static int queue_lock_count(odp_queue_t handle) > { > - queue_entry_t *queue = qentry_from_int(queue_from_ext(handle)); > + queue_entry_t *queue = handle_to_qentry(handle); > > return queue->s.param.sched.sync == ODP_SCHED_SYNC_ORDERED ? > (int)queue->s.param.sched.lock_count : -1; > @@ -299,7 +257,7 @@ void sched_cb_queue_destroy_finalize(uint32_t queue_index) > static int queue_destroy(odp_queue_t handle) > { > queue_entry_t *queue; > - queue = qentry_from_int(queue_from_ext(handle)); > + queue = handle_to_qentry(handle); > > if (handle == ODP_QUEUE_INVALID) > return -1; > @@ -352,14 +310,14 @@ static int queue_context_set(odp_queue_t handle, void *context, > uint32_t len ODP_UNUSED) > { > odp_mb_full(); > - qentry_from_int(queue_from_ext(handle))->s.param.context = context; > + handle_to_qentry(handle)->s.param.context = context; > odp_mb_full(); > return 0; > } > > static void *queue_context(odp_queue_t handle) > { > - return qentry_from_int(queue_from_ext(handle))->s.param.context; > + return handle_to_qentry(handle)->s.param.context; > } > > static odp_queue_t queue_lookup(const char *name) > @@ -385,7 +343,7 @@ static odp_queue_t queue_lookup(const char *name) > return ODP_QUEUE_INVALID; > } > > -static inline int enq_multi(queue_t handle, odp_buffer_hdr_t *buf_hdr[], > +static inline int enq_multi(queue_t q_int, odp_buffer_hdr_t *buf_hdr[], > int num) q_int does not indicate that it is a handle. Change it to 'handle_int'. > { > int sched = 0; > @@ -393,8 +351,8 @@ static inline int enq_multi(queue_t handle, odp_buffer_hdr_t *buf_hdr[], > queue_entry_t *queue; > odp_buffer_hdr_t *hdr, *tail, *next_hdr; > > - queue = qentry_from_int(handle); > - if (sched_fn->ord_enq_multi(handle, (void **)buf_hdr, num, &ret)) > + queue = qentry_from_int(q_int); > + if (sched_fn->ord_enq_multi(q_int, (void **)buf_hdr, num, &ret)) > return ret; > > /* Optimize the common case of single enqueue */ > @@ -462,17 +420,17 @@ static inline int enq_multi(queue_t handle, odp_buffer_hdr_t *buf_hdr[], > return num; /* All events enqueued */ > } > > -static int _queue_enq_multi(queue_t handle, odp_buffer_hdr_t *buf_hdr[], > - int num) > +static int queue_int_enq_multi(queue_t q_int, odp_buffer_hdr_t *buf_hdr[], > + int num) No need to introduce another naming convention. The rest of the code in ODP follows the convention of starting the function names with '_'. For ex: take a look at odp_packet_io.c file. > { > - return enq_multi(handle, buf_hdr, num); > + return enq_multi(q_int, buf_hdr, num); > } > > -static int _queue_enq(queue_t handle, odp_buffer_hdr_t *buf_hdr) > +static int queue_int_enq(queue_t q_int, odp_buffer_hdr_t *buf_hdr) > { > int ret; > > - ret = enq_multi(handle, &buf_hdr, 1); > + ret = enq_multi(q_int, &buf_hdr, 1); > > if (ret == 1) > return 0; > @@ -489,7 +447,7 @@ static int queue_enq_multi(odp_queue_t handle, const odp_event_t ev[], int num) > if (num > QUEUE_MULTI_MAX) > num = QUEUE_MULTI_MAX; > > - queue = qentry_from_int(queue_from_ext(handle)); > + queue = handle_to_qentry(handle); > > for (i = 0; i < num; i++) > buf_hdr[i] = buf_hdl_to_hdr(odp_buffer_from_event(ev[i])); > @@ -503,13 +461,13 @@ static int queue_enq(odp_queue_t handle, odp_event_t ev) > odp_buffer_hdr_t *buf_hdr; > queue_entry_t *queue; > > - queue = qentry_from_int(queue_from_ext(handle)); > + queue = handle_to_qentry(handle); > buf_hdr = buf_hdl_to_hdr(odp_buffer_from_event(ev)); > > return queue->s.enqueue(qentry_to_int(queue), buf_hdr); > } > > -static inline int deq_multi(queue_t handle, odp_buffer_hdr_t *buf_hdr[], > +static inline int deq_multi(queue_t q_int, odp_buffer_hdr_t *buf_hdr[], > int num) 'q_int' does not indicate that it is a handle. Change it to 'handle_int'. Applies to other instances of 'q_int'. > { > odp_buffer_hdr_t *hdr, *next; > @@ -517,7 +475,7 @@ static inline int deq_multi(queue_t handle, odp_buffer_hdr_t *buf_hdr[], > queue_entry_t *queue; > int updated = 0; > > - queue = qentry_from_int(handle); > + queue = qentry_from_int(q_int); > LOCK(&queue->s.lock); > if (odp_unlikely(queue->s.status < QUEUE_STATUS_READY)) { > /* Bad queue, or queue has been destroyed. > @@ -583,18 +541,18 @@ static inline int deq_multi(queue_t handle, odp_buffer_hdr_t *buf_hdr[], > return i; > } > > -static int _queue_deq_multi(queue_t handle, odp_buffer_hdr_t *buf_hdr[], > - int num) > +static int queue_int_deq_multi(queue_t q_int, odp_buffer_hdr_t *buf_hdr[], > + int num) > { > - return deq_multi(handle, buf_hdr, num); > + return deq_multi(q_int, buf_hdr, num); > } > > -static odp_buffer_hdr_t *_queue_deq(queue_t handle) > +static odp_buffer_hdr_t *queue_int_deq(queue_t q_int) No need to introduce a new naming convention. > { > odp_buffer_hdr_t *buf_hdr = NULL; > int ret; > > - ret = deq_multi(handle, &buf_hdr, 1); > + ret = deq_multi(q_int, &buf_hdr, 1); > > if (ret == 1) > return buf_hdr; > @@ -611,7 +569,7 @@ static int queue_deq_multi(odp_queue_t handle, odp_event_t events[], int num) > if (num > QUEUE_MULTI_MAX) > num = QUEUE_MULTI_MAX; > > - queue = qentry_from_int(queue_from_ext(handle)); > + queue = handle_to_qentry(handle); > > ret = queue->s.dequeue_multi(qentry_to_int(queue), buf_hdr, num); > > @@ -627,7 +585,7 @@ static odp_event_t queue_deq(odp_queue_t handle) > queue_entry_t *queue; > odp_buffer_hdr_t *buf_hdr; > > - queue = qentry_from_int(queue_from_ext(handle)); > + queue = handle_to_qentry(handle); > buf_hdr = queue->s.dequeue(qentry_to_int(queue)); > > if (buf_hdr) > @@ -636,6 +594,49 @@ static odp_event_t queue_deq(odp_queue_t handle) > return ODP_EVENT_INVALID; > } > > +static int queue_init(queue_entry_t *queue, const char *name, > + const odp_queue_param_t *param) > +{ > + if (name == NULL) { > + queue->s.name[0] = 0; > + } else { > + strncpy(queue->s.name, name, ODP_QUEUE_NAME_LEN - 1); > + queue->s.name[ODP_QUEUE_NAME_LEN - 1] = 0; > + } > + memcpy(&queue->s.param, param, sizeof(odp_queue_param_t)); > + if (queue->s.param.sched.lock_count > sched_fn->max_ordered_locks()) > + return -1; > + > + if (param->type == ODP_QUEUE_TYPE_SCHED) { > + queue->s.param.deq_mode = ODP_QUEUE_OP_DISABLED; > + > + if (param->sched.sync == ODP_SCHED_SYNC_ORDERED) { > + unsigned i; > + > + odp_atomic_init_u64(&queue->s.ordered.ctx, 0); > + odp_atomic_init_u64(&queue->s.ordered.next_ctx, 0); > + > + for (i = 0; i < queue->s.param.sched.lock_count; i++) > + odp_atomic_init_u64(&queue->s.ordered.lock[i], > + 0); > + } > + } > + queue->s.type = queue->s.param.type; > + > + queue->s.enqueue = queue_int_enq; > + queue->s.dequeue = queue_int_deq; > + queue->s.enqueue_multi = queue_int_enq_multi; > + queue->s.dequeue_multi = queue_int_deq_multi; > + > + queue->s.pktin = PKTIN_INVALID; > + queue->s.pktout = PKTOUT_INVALID; > + > + queue->s.head = NULL; > + queue->s.tail = NULL; > + > + return 0; > +} > + Unnecessary change. > void queue_lock(queue_entry_t *queue) > { > LOCK(&queue->s.lock); > @@ -776,64 +777,65 @@ static uint64_t queue_to_u64(odp_queue_t hdl) > return _odp_pri(hdl); > } > > -static odp_pktout_queue_t queue_get_pktout(queue_t handle) > +static odp_pktout_queue_t queue_get_pktout(queue_t q_int) > { > - return qentry_from_int(handle)->s.pktout; > + return qentry_from_int(q_int)->s.pktout; > } > > -static void queue_set_pktout(queue_t handle, odp_pktio_t pktio, int index) > +static void queue_set_pktout(queue_t q_int, odp_pktio_t pktio, int index) > { > - qentry_from_int(handle)->s.pktout.pktio = pktio; > - qentry_from_int(handle)->s.pktout.index = index; > + queue_entry_t *qentry = qentry_from_int(q_int); > + > + qentry->s.pktout.pktio = pktio; > + qentry->s.pktout.index = index; > } > > -static odp_pktin_queue_t queue_get_pktin(queue_t handle) > +static odp_pktin_queue_t queue_get_pktin(queue_t q_int) > { > - return qentry_from_int(handle)->s.pktin; > + return qentry_from_int(q_int)->s.pktin; > } > > -static void queue_set_pktin(queue_t handle, odp_pktio_t pktio, int index) > +static void queue_set_pktin(queue_t q_int, odp_pktio_t pktio, int index) > { > - qentry_from_int(handle)->s.pktin.pktio = pktio; > - qentry_from_int(handle)->s.pktin.index = index; > + queue_entry_t *qentry = qentry_from_int(q_int); > + > + qentry->s.pktin.pktio = pktio; > + qentry->s.pktin.index = index; > } > > -static void queue_set_enq_func(queue_t handle, queue_enq_fn_t func) > +static void queue_set_enq_func(queue_t q_int, queue_enq_fn_t func) > { > - qentry_from_int(handle)->s.enqueue = func; > + qentry_from_int(q_int)->s.enqueue = func; > } > > -static void queue_set_enq_multi_func(queue_t handle, queue_enq_multi_fn_t func) > +static void queue_set_enq_multi_func(queue_t q_int, queue_enq_multi_fn_t func) > { > - qentry_from_int(handle)->s.enqueue_multi = func; > + qentry_from_int(q_int)->s.enqueue_multi = func; > } > > -static void queue_set_deq_func(queue_t handle, queue_deq_fn_t func) > +static void queue_set_deq_func(queue_t q_int, queue_deq_fn_t func) > { > - qentry_from_int(handle)->s.dequeue = func; > + qentry_from_int(q_int)->s.dequeue = func; > } > > -static void queue_set_deq_multi_func(queue_t handle, queue_deq_multi_fn_t func) > +static void queue_set_deq_multi_func(queue_t q_int, queue_deq_multi_fn_t func) > { > - qentry_from_int(handle)->s.dequeue_multi = func; > + qentry_from_int(q_int)->s.dequeue_multi = func; > } > > -static void queue_set_type(queue_t handle, odp_queue_type_t type) > +static void queue_set_type(queue_t q_int, odp_queue_type_t type) > { > - qentry_from_int(handle)->s.type = type; > + qentry_from_int(q_int)->s.type = type; > } > > static queue_t queue_from_ext(odp_queue_t handle) > { > - uint32_t queue_id; > - > - queue_id = queue_to_id(handle); > - return qentry_to_int(get_qentry(queue_id)); > + return qentry_to_int(handle_to_qentry(handle)); 'handle_to_qentry' is another extra level of wrapper. I think we already have enough wrappers because of all the type casts. No need to introduce one more. > } > > -static odp_queue_t queue_to_ext(queue_t handle) > +static odp_queue_t queue_to_ext(queue_t q_int) > { > - return qentry_from_int(handle)->s.handle; > + return qentry_from_int(q_int)->s.handle; > } > > /* API functions */ > @@ -866,10 +868,10 @@ queue_fn_t queue_default_fn = { > .term_local = queue_term_local, > .from_ext = queue_from_ext, > .to_ext = queue_to_ext, > - .enq = _queue_enq, > - .enq_multi = _queue_enq_multi, > - .deq = _queue_deq, > - .deq_multi = _queue_deq_multi, > + .enq = queue_int_enq, > + .enq_multi = queue_int_enq_multi, > + .deq = queue_int_deq, > + .deq_multi = queue_int_deq_multi, > .get_pktout = queue_get_pktout, > .set_pktout = queue_set_pktout, > .get_pktin = queue_get_pktin, > diff --git a/platform/linux-generic/odp_schedule.c b/platform/linux-generic/odp_schedule.c > index 011d4dc4..92634235 100644 > --- a/platform/linux-generic/odp_schedule.c > +++ b/platform/linux-generic/odp_schedule.c > @@ -712,12 +712,12 @@ static inline int copy_events(odp_event_t out_ev[], unsigned int max) > return i; > } > > -static int schedule_ord_enq_multi(queue_t handle, void *buf_hdr[], > +static int schedule_ord_enq_multi(queue_t q_int, void *buf_hdr[], > int num, int *ret) > { > int i; > uint32_t stash_num = sched_local.ordered.stash_num; > - queue_entry_t *dst_queue = qentry_from_int(handle); > + queue_entry_t *dst_queue = qentry_from_int(q_int); > queue_entry_t *src_queue = sched_local.ordered.src_queue; > > if (!sched_local.ordered.src_queue || sched_local.ordered.in_order) > diff --git a/platform/linux-generic/odp_schedule_iquery.c b/platform/linux-generic/odp_schedule_iquery.c > index bdf1a460..79086843 100644 > --- a/platform/linux-generic/odp_schedule_iquery.c > +++ b/platform/linux-generic/odp_schedule_iquery.c > @@ -1159,12 +1159,12 @@ static inline void schedule_release_context(void) > schedule_release_atomic(); > } > > -static int schedule_ord_enq_multi(queue_t handle, void *buf_hdr[], > +static int schedule_ord_enq_multi(queue_t q_int, void *buf_hdr[], > int num, int *ret) > { > int i; > uint32_t stash_num = thread_local.ordered.stash_num; > - queue_entry_t *dst_queue = qentry_from_int(handle); > + queue_entry_t *dst_queue = qentry_from_int(q_int); > queue_entry_t *src_queue = thread_local.ordered.src_queue; > > if (!thread_local.ordered.src_queue || thread_local.ordered.in_order) > diff --git a/platform/linux-generic/odp_schedule_sp.c b/platform/linux-generic/odp_schedule_sp.c > index 91d70e3a..66715c13 100644 > --- a/platform/linux-generic/odp_schedule_sp.c > +++ b/platform/linux-generic/odp_schedule_sp.c > @@ -414,10 +414,10 @@ static int unsched_queue(uint32_t qi ODP_UNUSED) > return 0; > } > > -static int ord_enq_multi(queue_t handle, void *buf_hdr[], int num, > +static int ord_enq_multi(queue_t q_int, void *buf_hdr[], int num, > int *ret) > { > - (void)handle; > + (void)q_int; > (void)buf_hdr; > (void)num; > (void)ret; > -- > 2.13.0 > I think queue_t should be removed completely. We should just use odp_queue_t for internal interfaces as well. Looking at the code, using odp_queue_t would introduce 3 extra addition instructions and the impact to cache does not change. We can run l2fwd benchmark and compare the numbers. This would keep the code simple as well.
Honnappa Nagarahalli wrote: > On 12 June 2017 at 06:11, Petri Savolainen <petri.savolainen@linaro.org> wrote: > > Clean up function and parameter naming after modular interface > > patch. Queue_t type is referred as "queue internal": queue_int or > > q_int. Term "handle" is reserved for API level handles (e.g. > > odp_queue_t, odp_pktio_t, etc) through out linux-gen implementation. > > > > "queue_t" type should be referred to as "handle_int". "handle_int" is > clearly different from "handle". > If we look at the definition of "queue_t": > > typedef struct { char dummy; } _queue_t; > typedef _queue_t *queue_t; > > it is nothing but a definition of a handle. Why should it be called > with some other name and create confusion? Just like how odp_queue_t > is an abstract type, queue_t is also an abstract type. Just like how > odp_queue_t is a handle, queue_t is also a handle, albeit a handle > towards internal components. I do not see how calling variables of type queue_t handles instead of queue_int or q_int makes the call any clearer or less confusing. If the term handle is reserved for ODP API level handles, then I suppose this code should adhere to that. And 'handle_int' is not very descriptive as a variable name anyway. > > +static inline queue_entry_t *handle_to_qentry(odp_queue_t handle) > > Why is there a need to add this function? We already have > 'queue_from_ext' and 'qentry_from_int' which are a must to implement. > The functionality provided by 'handle_to_qentry' can be achieved from > these two functions. 'handle_to_qentry' is adding another layer of > wrapper. This adds to code complexity. There is a need to convert from handle to queue entry in quite many places in the code. Having a function for that makes perfect sense since it reduces code duplication and simplifies all the call sites that no longer need to know how the conversion is done. This is also how the code was before you changed it (unnecessarily, one might think), so this change merely brings back the old code structure (with a naming change). > > static odp_queue_type_t queue_type(odp_queue_t handle) > > { > > - return qentry_from_int(queue_from_ext(handle))->s.type; > > + return handle_to_qentry(handle)->s.type; > > No need to introduce another function. > qentry_from_int(queue_from_ext(handle)) clearly shows the current > status that there exists an internal handle. handle_to_qentry(handle) > hides that fact and makes code less readable. This comment applies to > all the instances of similar change. Hiding is good. Only handle_to_qentry() needs to know that the conversion is (currently) done through queue_t. I would argue that the code is more readable with handle_to_qentry() than with having to read the same conversion code all the time. An if the code ever changes so that the conversion is better done in another way, having handle_to_qentry() avoids the need to change all its call sites. Janne
>> void *buf_hdr[], int num, int *ret); >> typedef int (*schedule_init_global_fn_t)(void); >> typedef int (*schedule_term_global_fn_t)(void); >> diff --git a/platform/linux-generic/odp_queue.c b/platform/linux-generic/odp_queue.c >> index 3e18f578..19945584 100644 >> --- a/platform/linux-generic/odp_queue.c >> +++ b/platform/linux-generic/odp_queue.c >> @@ -35,20 +35,22 @@ >> #include <string.h> >> #include <inttypes.h> >> >> +static int queue_init(queue_entry_t *queue, const char *name, >> + const odp_queue_param_t *param); >> + > > This is unnecessary for this patch. Don't waste reviewer's time with > unwanted changes. Unwanted changes have been discussed in the context > of other patches and clearly agreed that they should not be done. In > fact, such changes have been reversed. This prototype has been added here to remove the need for the following function prototypes: _queue_enq, _queue_deq, _queue_enq_multi, _queue_deq_multi. There are already matching typedefs in odp_queue_if.h and "re-prototyping" them here is unnecessary and makes maintaining the code more complex. >> +{ >> + uint32_t queue_id; >> >> -static int _queue_enq_multi(queue_t handle, odp_buffer_hdr_t *buf_hdr[], >> - int num); >> -static int _queue_deq_multi(queue_t handle, odp_buffer_hdr_t *buf_hdr[], >> - int num); >> + queue_id = queue_to_id(handle); >> + return get_qentry(queue_id); >> +} >> >> static inline odp_queue_t queue_from_id(uint32_t queue_id) >> { >> @@ -70,50 +72,6 @@ queue_entry_t *get_qentry(uint32_t queue_id) >> return &queue_tbl->queue[queue_id]; >> } >> >> -static int queue_init(queue_entry_t *queue, const char *name, >> - const odp_queue_param_t *param) >> -{ >> - if (name == NULL) { >> - queue->s.name[0] = 0; >> - } else { >> - strncpy(queue->s.name, name, ODP_QUEUE_NAME_LEN - 1); >> - queue->s.name[ODP_QUEUE_NAME_LEN - 1] = 0; >> - } >> - memcpy(&queue->s.param, param, sizeof(odp_queue_param_t)); >> - if (queue->s.param.sched.lock_count > sched_fn->max_ordered_locks()) >> - return -1; >> - >> - if (param->type == ODP_QUEUE_TYPE_SCHED) { >> - queue->s.param.deq_mode = ODP_QUEUE_OP_DISABLED; >> - >> - if (param->sched.sync == ODP_SCHED_SYNC_ORDERED) { >> - unsigned i; >> - >> - odp_atomic_init_u64(&queue->s.ordered.ctx, 0); >> - odp_atomic_init_u64(&queue->s.ordered.next_ctx, 0); >> - >> - for (i = 0; i < queue->s.param.sched.lock_count; i++) >> - odp_atomic_init_u64(&queue->s.ordered.lock[i], >> - 0); >> - } >> - } >> - queue->s.type = queue->s.param.type; >> - >> - queue->s.enqueue = _queue_enq; >> - queue->s.dequeue = _queue_deq; >> - queue->s.enqueue_multi = _queue_enq_multi; >> - queue->s.dequeue_multi = _queue_deq_multi; >> - >> - queue->s.pktin = PKTIN_INVALID; >> - queue->s.pktout = PKTOUT_INVALID; >> - >> - queue->s.head = NULL; >> - queue->s.tail = NULL; >> - >> - return 0; >> -} >> - >> - > > Unnecessary change Comment above. >> >> -static int _queue_enq_multi(queue_t handle, odp_buffer_hdr_t *buf_hdr[], >> - int num) >> +static int queue_int_enq_multi(queue_t q_int, odp_buffer_hdr_t *buf_hdr[], >> + int num) > > No need to introduce another naming convention. The rest of the code > in ODP follows the convention of starting the function names with '_'. > For ex: take a look at odp_packet_io.c file. Naming conventions may be file specific and the '_' convention is clearly in minority. >> >> >> -static odp_buffer_hdr_t *_queue_deq(queue_t handle) >> +static odp_buffer_hdr_t *queue_int_deq(queue_t q_int) > > No need to introduce a new naming convention. Comment above. >> >> +static int queue_init(queue_entry_t *queue, const char *name, >> + const odp_queue_param_t *param) >> +{ >> + if (name == NULL) { >> + queue->s.name[0] = 0; >> + } else { >> + strncpy(queue->s.name, name, ODP_QUEUE_NAME_LEN - 1); >> + queue->s.name[ODP_QUEUE_NAME_LEN - 1] = 0; >> + } >> + memcpy(&queue->s.param, param, sizeof(odp_queue_param_t)); >> + if (queue->s.param.sched.lock_count > sched_fn->max_ordered_locks()) >> + return -1; >> + >> + if (param->type == ODP_QUEUE_TYPE_SCHED) { >> + queue->s.param.deq_mode = ODP_QUEUE_OP_DISABLED; >> + >> + if (param->sched.sync == ODP_SCHED_SYNC_ORDERED) { >> + unsigned i; >> + >> + odp_atomic_init_u64(&queue->s.ordered.ctx, 0); >> + odp_atomic_init_u64(&queue->s.ordered.next_ctx, 0); >> + >> + for (i = 0; i < queue->s.param.sched.lock_count; i++) >> + odp_atomic_init_u64(&queue->s.ordered.lock[i], >> + 0); >> + } >> + } >> + queue->s.type = queue->s.param.type; >> + >> + queue->s.enqueue = queue_int_enq; >> + queue->s.dequeue = queue_int_deq; >> + queue->s.enqueue_multi = queue_int_enq_multi; >> + queue->s.dequeue_multi = queue_int_deq_multi; >> + >> + queue->s.pktin = PKTIN_INVALID; >> + queue->s.pktout = PKTOUT_INVALID; >> + >> + queue->s.head = NULL; >> + queue->s.tail = NULL; >> + >> + return 0; >> +} >> + > > Unnecessary change. Earlier comment about prototypes. >> >> >> static queue_t queue_from_ext(odp_queue_t handle) >> { >> - uint32_t queue_id; >> - >> - queue_id = queue_to_id(handle); >> - return qentry_to_int(get_qentry(queue_id)); >> + return qentry_to_int(handle_to_qentry(handle)); > > 'handle_to_qentry' is another extra level of wrapper. I think we > already have enough wrappers because of all the type casts. No need to > introduce one more. > handle_to_qentry() is simply an inlined cast. > > I think queue_t should be removed completely. We should just use > odp_queue_t for internal interfaces as well. Looking at the code, > using odp_queue_t would introduce 3 extra addition instructions and > the impact to cache does not change. We can run l2fwd benchmark and > compare the numbers. This would keep the code simple as well. Not relevant for this patch. -Matias
> On 16 Jun 2017, at 11:03, Peltonen, Janne (Nokia - FI/Espoo) <janne.peltonen@nokia.com> wrote: > > > Honnappa Nagarahalli wrote: >> On 12 June 2017 at 06:11, Petri Savolainen <petri.savolainen@linaro.org> wrote: >>> Clean up function and parameter naming after modular interface >>> patch. Queue_t type is referred as "queue internal": queue_int or >>> q_int. Term "handle" is reserved for API level handles (e.g. >>> odp_queue_t, odp_pktio_t, etc) through out linux-gen implementation. >>> >> >> "queue_t" type should be referred to as "handle_int". "handle_int" is >> clearly different from "handle". >> If we look at the definition of "queue_t": >> >> typedef struct { char dummy; } _queue_t; >> typedef _queue_t *queue_t; >> >> it is nothing but a definition of a handle. Why should it be called >> with some other name and create confusion? Just like how odp_queue_t >> is an abstract type, queue_t is also an abstract type. Just like how >> odp_queue_t is a handle, queue_t is also a handle, albeit a handle >> towards internal components. > > I do not see how calling variables of type queue_t handles instead > of queue_int or q_int makes the call any clearer or less confusing. > If the term handle is reserved for ODP API level handles, then I > suppose this code should adhere to that. And 'handle_int' is not > very descriptive as a variable name anyway. > >>> +static inline queue_entry_t *handle_to_qentry(odp_queue_t handle) >> >> Why is there a need to add this function? We already have >> 'queue_from_ext' and 'qentry_from_int' which are a must to implement. >> The functionality provided by 'handle_to_qentry' can be achieved from >> these two functions. 'handle_to_qentry' is adding another layer of >> wrapper. This adds to code complexity. > > There is a need to convert from handle to queue entry in quite many > places in the code. Having a function for that makes perfect sense > since it reduces code duplication and simplifies all the call sites > that no longer need to know how the conversion is done. > > This is also how the code was before you changed it (unnecessarily, > one might think), so this change merely brings back the old code > structure (with a naming change). > >>> static odp_queue_type_t queue_type(odp_queue_t handle) >>> { >>> - return qentry_from_int(queue_from_ext(handle))->s.type; >>> + return handle_to_qentry(handle)->s.type; >> >> No need to introduce another function. >> qentry_from_int(queue_from_ext(handle)) clearly shows the current >> status that there exists an internal handle. handle_to_qentry(handle) >> hides that fact and makes code less readable. This comment applies to >> all the instances of similar change. > > Hiding is good. Only handle_to_qentry() needs to know that the > conversion is (currently) done through queue_t. I would argue that > the code is more readable with handle_to_qentry() than with having > to read the same conversion code all the time. An if the code ever > changes so that the conversion is better done in another way, having > handle_to_qentry() avoids the need to change all its call sites. > > Janne > > I agree on all points with Janne. -Matias
On 16 June 2017 at 03:03, Peltonen, Janne (Nokia - FI/Espoo) <janne.peltonen@nokia.com> wrote: > > Honnappa Nagarahalli wrote: >> On 12 June 2017 at 06:11, Petri Savolainen <petri.savolainen@linaro.org> wrote: >> > Clean up function and parameter naming after modular interface >> > patch. Queue_t type is referred as "queue internal": queue_int or >> > q_int. Term "handle" is reserved for API level handles (e.g. >> > odp_queue_t, odp_pktio_t, etc) through out linux-gen implementation. >> > >> >> "queue_t" type should be referred to as "handle_int". "handle_int" is >> clearly different from "handle". >> If we look at the definition of "queue_t": >> >> typedef struct { char dummy; } _queue_t; >> typedef _queue_t *queue_t; >> >> it is nothing but a definition of a handle. Why should it be called >> with some other name and create confusion? Just like how odp_queue_t >> is an abstract type, queue_t is also an abstract type. Just like how >> odp_queue_t is a handle, queue_t is also a handle, albeit a handle >> towards internal components. > > I do not see how calling variables of type queue_t handles instead > of queue_int or q_int makes the call any clearer or less confusing. Should we then call variables of type odp_queue_t as 'queue'? I am not suggesting variables of type queue_t to be called as 'handle', I am suggesting that they should be called as 'handle_int'. > If the term handle is reserved for ODP API level handles, then I > suppose this code should adhere to that. This code adheres to that. It is still calling variables of the type odp_queue_t as 'handle'. As this patch introduces another type of handle which is not exposed to the application, there is a need to introduce another convention for variables of internal handle type, hence it makes sense to call variables of internal type as 'handle_int'. And 'handle_int' is not > very descriptive as a variable name anyway. Then, how are 'q_int' and 'queue_int' descriptive? What do they indicate? > >> > +static inline queue_entry_t *handle_to_qentry(odp_queue_t handle) >> >> Why is there a need to add this function? We already have >> 'queue_from_ext' and 'qentry_from_int' which are a must to implement. >> The functionality provided by 'handle_to_qentry' can be achieved from >> these two functions. 'handle_to_qentry' is adding another layer of >> wrapper. This adds to code complexity. > > There is a need to convert from handle to queue entry in quite many > places in the code. Having a function for that makes perfect sense > since it reduces code duplication and simplifies all the call sites > that no longer need to know how the conversion is done. As I said, 'queue_from_ext' and 'qentry_from_int' are a must to implement. Adding one more wrapper on top of them is duplication of code, especially when this wrapper does not provide any extra functionality. Why to add wrapper for the sake of adding wrappers? qentry_from_int(queue_from_ext) makes it clear that there exists an internal handle. 'handle_to_qentry' hides that fact and creates more confusion, especially when rest of code base refers to 'qentry_from_int' and 'queue_from_ext'. > > This is also how the code was before you changed it (unnecessarily, > one might think), so this change merely brings back the old code > structure (with a naming change). > This was changed because the function becomes redundant and there is no need to have a wrapper function for the sake of having one. >> > static odp_queue_type_t queue_type(odp_queue_t handle) >> > { >> > - return qentry_from_int(queue_from_ext(handle))->s.type; >> > + return handle_to_qentry(handle)->s.type; >> >> No need to introduce another function. >> qentry_from_int(queue_from_ext(handle)) clearly shows the current >> status that there exists an internal handle. handle_to_qentry(handle) >> hides that fact and makes code less readable. This comment applies to >> all the instances of similar change. > > Hiding is good. Only handle_to_qentry() needs to know that the > conversion is (currently) done through queue_t. I would argue that > the code is more readable with handle_to_qentry() than with having > to read the same conversion code all the time. With the introduction of handle_to_qentry, one needs to understand another extra function. Reading the same conversion clearly indicates what is actually happening. An if the code ever > changes so that the conversion is better done in another way, having > handle_to_qentry() avoids the need to change all its call sites. The code should reflect the current situation correctly. Having the function 'handle_to_qentry' is already changing all the call sites today, to adopt to the new changes. Similarly, when things change in the future, we will change the code at that time accordingly. > > Janne > >
On 16 June 2017 at 05:25, Elo, Matias (Nokia - FI/Espoo) <matias.elo@nokia.com> wrote: > >>> void *buf_hdr[], int num, int *ret); >>> typedef int (*schedule_init_global_fn_t)(void); >>> typedef int (*schedule_term_global_fn_t)(void); >>> diff --git a/platform/linux-generic/odp_queue.c b/platform/linux-generic/odp_queue.c >>> index 3e18f578..19945584 100644 >>> --- a/platform/linux-generic/odp_queue.c >>> +++ b/platform/linux-generic/odp_queue.c >>> @@ -35,20 +35,22 @@ >>> #include <string.h> >>> #include <inttypes.h> >>> >>> +static int queue_init(queue_entry_t *queue, const char *name, >>> + const odp_queue_param_t *param); >>> + >> >> This is unnecessary for this patch. Don't waste reviewer's time with >> unwanted changes. Unwanted changes have been discussed in the context >> of other patches and clearly agreed that they should not be done. In >> fact, such changes have been reversed. > > This prototype has been added here to remove the need for the following function > prototypes: _queue_enq, _queue_deq, _queue_enq_multi, _queue_deq_multi. There > are already matching typedefs in odp_queue_if.h and "re-prototyping" them here is > unnecessary and makes maintaining the code more complex. > The prototypes in odp_queue_if.h are not related to _queue_enq, _queue_deq, _queue_enq_multi, _queue_deq_multi. The prototypes for _queue_enq, _queue_deq, _queue_enq_multi, _queue_deq_multi exist here as forward declarations. This cleanup should be done as part of another patch. > >>> +{ >>> + uint32_t queue_id; >>> >>> -static int _queue_enq_multi(queue_t handle, odp_buffer_hdr_t *buf_hdr[], >>> - int num); >>> -static int _queue_deq_multi(queue_t handle, odp_buffer_hdr_t *buf_hdr[], >>> - int num); >>> + queue_id = queue_to_id(handle); >>> + return get_qentry(queue_id); >>> +} >>> >>> static inline odp_queue_t queue_from_id(uint32_t queue_id) >>> { >>> @@ -70,50 +72,6 @@ queue_entry_t *get_qentry(uint32_t queue_id) >>> return &queue_tbl->queue[queue_id]; >>> } >>> >>> -static int queue_init(queue_entry_t *queue, const char *name, >>> - const odp_queue_param_t *param) >>> -{ >>> - if (name == NULL) { >>> - queue->s.name[0] = 0; >>> - } else { >>> - strncpy(queue->s.name, name, ODP_QUEUE_NAME_LEN - 1); >>> - queue->s.name[ODP_QUEUE_NAME_LEN - 1] = 0; >>> - } >>> - memcpy(&queue->s.param, param, sizeof(odp_queue_param_t)); >>> - if (queue->s.param.sched.lock_count > sched_fn->max_ordered_locks()) >>> - return -1; >>> - >>> - if (param->type == ODP_QUEUE_TYPE_SCHED) { >>> - queue->s.param.deq_mode = ODP_QUEUE_OP_DISABLED; >>> - >>> - if (param->sched.sync == ODP_SCHED_SYNC_ORDERED) { >>> - unsigned i; >>> - >>> - odp_atomic_init_u64(&queue->s.ordered.ctx, 0); >>> - odp_atomic_init_u64(&queue->s.ordered.next_ctx, 0); >>> - >>> - for (i = 0; i < queue->s.param.sched.lock_count; i++) >>> - odp_atomic_init_u64(&queue->s.ordered.lock[i], >>> - 0); >>> - } >>> - } >>> - queue->s.type = queue->s.param.type; >>> - >>> - queue->s.enqueue = _queue_enq; >>> - queue->s.dequeue = _queue_deq; >>> - queue->s.enqueue_multi = _queue_enq_multi; >>> - queue->s.dequeue_multi = _queue_deq_multi; >>> - >>> - queue->s.pktin = PKTIN_INVALID; >>> - queue->s.pktout = PKTOUT_INVALID; >>> - >>> - queue->s.head = NULL; >>> - queue->s.tail = NULL; >>> - >>> - return 0; >>> -} >>> - >>> - >> >> Unnecessary change > > Comment above. > > >>> >>> -static int _queue_enq_multi(queue_t handle, odp_buffer_hdr_t *buf_hdr[], >>> - int num) >>> +static int queue_int_enq_multi(queue_t q_int, odp_buffer_hdr_t *buf_hdr[], >>> + int num) >> >> No need to introduce another naming convention. The rest of the code >> in ODP follows the convention of starting the function names with '_'. >> For ex: take a look at odp_packet_io.c file. > > Naming conventions may be file specific and the '_' convention is clearly > in minority. I do not see any reference for 'xxx_int_' in the existing code. Majority seems to be '_odp_int_xxx'. > >>> >>> >>> -static odp_buffer_hdr_t *_queue_deq(queue_t handle) >>> +static odp_buffer_hdr_t *queue_int_deq(queue_t q_int) >> >> No need to introduce a new naming convention. > > Comment above. > >>> >>> +static int queue_init(queue_entry_t *queue, const char *name, >>> + const odp_queue_param_t *param) >>> +{ >>> + if (name == NULL) { >>> + queue->s.name[0] = 0; >>> + } else { >>> + strncpy(queue->s.name, name, ODP_QUEUE_NAME_LEN - 1); >>> + queue->s.name[ODP_QUEUE_NAME_LEN - 1] = 0; >>> + } >>> + memcpy(&queue->s.param, param, sizeof(odp_queue_param_t)); >>> + if (queue->s.param.sched.lock_count > sched_fn->max_ordered_locks()) >>> + return -1; >>> + >>> + if (param->type == ODP_QUEUE_TYPE_SCHED) { >>> + queue->s.param.deq_mode = ODP_QUEUE_OP_DISABLED; >>> + >>> + if (param->sched.sync == ODP_SCHED_SYNC_ORDERED) { >>> + unsigned i; >>> + >>> + odp_atomic_init_u64(&queue->s.ordered.ctx, 0); >>> + odp_atomic_init_u64(&queue->s.ordered.next_ctx, 0); >>> + >>> + for (i = 0; i < queue->s.param.sched.lock_count; i++) >>> + odp_atomic_init_u64(&queue->s.ordered.lock[i], >>> + 0); >>> + } >>> + } >>> + queue->s.type = queue->s.param.type; >>> + >>> + queue->s.enqueue = queue_int_enq; >>> + queue->s.dequeue = queue_int_deq; >>> + queue->s.enqueue_multi = queue_int_enq_multi; >>> + queue->s.dequeue_multi = queue_int_deq_multi; >>> + >>> + queue->s.pktin = PKTIN_INVALID; >>> + queue->s.pktout = PKTOUT_INVALID; >>> + >>> + queue->s.head = NULL; >>> + queue->s.tail = NULL; >>> + >>> + return 0; >>> +} >>> + >> >> Unnecessary change. > > Earlier comment about prototypes. > >>> >>> >>> static queue_t queue_from_ext(odp_queue_t handle) >>> { >>> - uint32_t queue_id; >>> - >>> - queue_id = queue_to_id(handle); >>> - return qentry_to_int(get_qentry(queue_id)); >>> + return qentry_to_int(handle_to_qentry(handle)); >> >> 'handle_to_qentry' is another extra level of wrapper. I think we >> already have enough wrappers because of all the type casts. No need to >> introduce one more. >> > > handle_to_qentry() is simply an inlined cast. I am not talking from performance point of view. Why to introduce another function when we already have functions which can achieve the functionality? > >> >> I think queue_t should be removed completely. We should just use >> odp_queue_t for internal interfaces as well. Looking at the code, >> using odp_queue_t would introduce 3 extra addition instructions and >> the impact to cache does not change. We can run l2fwd benchmark and >> compare the numbers. This would keep the code simple as well. > > Not relevant for this patch. > Well, if we look at the bigger picture, we are trying to identify a solution for modular queue interface. Based on the discussions we are having, introducing another internal type has created lot of confusion and disagreements. To me it is a sign that, it is not the correct solution. We should relook at what we have done instead. For ex: I see that you proposal of using 'handle_to_qentry' function fits well if we do not have an internal type. Correction to my statement above, we will have 2 addition instructions (1 already exists). This is the worst case and these 2 instructions are amortized over CONFIG_BURST_SIZE number of packets. > -Matias >
> I think queue_t should be removed completely. We should just use > odp_queue_t for internal interfaces as well. Looking at the code, > using odp_queue_t would introduce 3 extra addition instructions and > the impact to cache does not change. We can run l2fwd benchmark and > compare the numbers. This would keep the code simple as well. First, this comment highlights why one direct function call provides better modularity than unnecessary exposition of an internal mid type. With handle_to_qentry() we can change, or even remove, the mid type without need to touch any of the call sites. queue_entry_t *queue; odp_buffer_hdr_t *buf_hdr; - queue = qentry_from_int(queue_from_ext(handle)); + queue = handle_to_qentry(handle); Secondly, the internal queue pointer (or table index - format does not matter) is needed to avoid doing the same conversion multiple times. We are inside implementation, and it improves performance when the number of odp_queue_t to pointer (qentry) conversions per packet is minimized. After this basic clean up is merged, I'll look into the queue interface definition and optimize it. This current version is just a dummy, copy-paste replacement of qentry->xxx with fn_queue->xxx(). Which is OK for the first stage, but certainly not optimal/final solution. -Petri
Does anybody wants to add his review to this patch? According to call a lot of people needed this patch but no review provided. Maxim. On 06/19/17 15:54, Savolainen, Petri (Nokia - FI/Espoo) wrote: > >> I think queue_t should be removed completely. We should just use >> odp_queue_t for internal interfaces as well. Looking at the code, >> using odp_queue_t would introduce 3 extra addition instructions and >> the impact to cache does not change. We can run l2fwd benchmark and >> compare the numbers. This would keep the code simple as well. > > First, this comment highlights why one direct function call provides better modularity than unnecessary exposition of an internal mid type. With handle_to_qentry() we can change, or even remove, the mid type without need to touch any of the call sites. > > queue_entry_t *queue; > odp_buffer_hdr_t *buf_hdr; > > - queue = qentry_from_int(queue_from_ext(handle)); > + queue = handle_to_qentry(handle); > > > Secondly, the internal queue pointer (or table index - format does not matter) is needed to avoid doing the same conversion multiple times. We are inside implementation, and it improves performance when the number of odp_queue_t to pointer (qentry) conversions per packet is minimized. > > After this basic clean up is merged, I'll look into the queue interface definition and optimize it. This current version is just a dummy, copy-paste replacement of qentry->xxx with fn_queue->xxx(). Which is OK for the first stage, but certainly not optimal/final solution. > > -Petri > >
Reviewed-and-tested-by: Bill Fischofer <bill.fischofer@linaro.org> On Tue, Jun 20, 2017 at 2:10 PM Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > Does anybody wants to add his review to this patch? According to call a > lot of people needed this patch but no review provided. > > Maxim. > > On 06/19/17 15:54, Savolainen, Petri (Nokia - FI/Espoo) wrote: > > > >> I think queue_t should be removed completely. We should just use > >> odp_queue_t for internal interfaces as well. Looking at the code, > >> using odp_queue_t would introduce 3 extra addition instructions and > >> the impact to cache does not change. We can run l2fwd benchmark and > >> compare the numbers. This would keep the code simple as well. > > > > First, this comment highlights why one direct function call provides > better modularity than unnecessary exposition of an internal mid type. With > handle_to_qentry() we can change, or even remove, the mid type without need > to touch any of the call sites. > > > > queue_entry_t *queue; > > odp_buffer_hdr_t *buf_hdr; > > > > - queue = qentry_from_int(queue_from_ext(handle)); > > + queue = handle_to_qentry(handle); > > > > > > Secondly, the internal queue pointer (or table index - format does not > matter) is needed to avoid doing the same conversion multiple times. We are > inside implementation, and it improves performance when the number of > odp_queue_t to pointer (qentry) conversions per packet is minimized. > > > > After this basic clean up is merged, I'll look into the queue interface > definition and optimize it. This current version is just a dummy, > copy-paste replacement of qentry->xxx with fn_queue->xxx(). Which is OK for > the first stage, but certainly not optimal/final solution. > > > > -Petri > > > > > >
diff --git a/platform/linux-generic/include/odp_queue_if.h b/platform/linux-generic/include/odp_queue_if.h index 4359435f..168d0e9e 100644 --- a/platform/linux-generic/include/odp_queue_if.h +++ b/platform/linux-generic/include/odp_queue_if.h @@ -53,24 +53,24 @@ typedef int (*queue_term_global_fn_t)(void); typedef int (*queue_init_local_fn_t)(void); typedef int (*queue_term_local_fn_t)(void); typedef queue_t (*queue_from_ext_fn_t)(odp_queue_t handle); -typedef odp_queue_t (*queue_to_ext_fn_t)(queue_t handle); -typedef int (*queue_enq_fn_t)(queue_t handle, odp_buffer_hdr_t *); -typedef int (*queue_enq_multi_fn_t)(queue_t handle, odp_buffer_hdr_t **, int); -typedef odp_buffer_hdr_t *(*queue_deq_fn_t)(queue_t handle); -typedef int (*queue_deq_multi_fn_t)(queue_t handle, odp_buffer_hdr_t **, int); -typedef odp_pktout_queue_t (*queue_get_pktout_fn_t)(queue_t handle); -typedef void (*queue_set_pktout_fn_t)(queue_t handle, odp_pktio_t pktio, +typedef odp_queue_t (*queue_to_ext_fn_t)(queue_t q_int); +typedef int (*queue_enq_fn_t)(queue_t q_int, odp_buffer_hdr_t *); +typedef int (*queue_enq_multi_fn_t)(queue_t q_int, odp_buffer_hdr_t **, int); +typedef odp_buffer_hdr_t *(*queue_deq_fn_t)(queue_t q_int); +typedef int (*queue_deq_multi_fn_t)(queue_t q_int, odp_buffer_hdr_t **, int); +typedef odp_pktout_queue_t (*queue_get_pktout_fn_t)(queue_t q_int); +typedef void (*queue_set_pktout_fn_t)(queue_t q_int, odp_pktio_t pktio, int index); -typedef odp_pktin_queue_t (*queue_get_pktin_fn_t)(queue_t handle); -typedef void (*queue_set_pktin_fn_t)(queue_t handle, odp_pktio_t pktio, +typedef odp_pktin_queue_t (*queue_get_pktin_fn_t)(queue_t q_int); +typedef void (*queue_set_pktin_fn_t)(queue_t q_int, odp_pktio_t pktio, int index); -typedef void (*queue_set_enq_fn_t)(queue_t handle, queue_enq_fn_t func); -typedef void (*queue_set_enq_multi_fn_t)(queue_t handle, +typedef void (*queue_set_enq_fn_t)(queue_t q_int, queue_enq_fn_t func); +typedef void (*queue_set_enq_multi_fn_t)(queue_t q_int, queue_enq_multi_fn_t func); -typedef void (*queue_set_deq_fn_t)(queue_t handle, queue_deq_fn_t func); -typedef void (*queue_set_deq_multi_fn_t)(queue_t handle, +typedef void (*queue_set_deq_fn_t)(queue_t q_int, queue_deq_fn_t func); +typedef void (*queue_set_deq_multi_fn_t)(queue_t q_int, queue_deq_multi_fn_t func); -typedef void (*queue_set_type_fn_t)(queue_t handle, odp_queue_type_t type); +typedef void (*queue_set_type_fn_t)(queue_t q_int, odp_queue_type_t type); /* Queue functions towards other internal components */ typedef struct { diff --git a/platform/linux-generic/include/odp_queue_internal.h b/platform/linux-generic/include/odp_queue_internal.h index 0c31ce8a..d79abd23 100644 --- a/platform/linux-generic/include/odp_queue_internal.h +++ b/platform/linux-generic/include/odp_queue_internal.h @@ -78,9 +78,9 @@ static inline uint32_t queue_to_id(odp_queue_t handle) return _odp_typeval(handle) - 1; } -static inline queue_entry_t *qentry_from_int(queue_t handle) +static inline queue_entry_t *qentry_from_int(queue_t q_int) { - return (queue_entry_t *)(void *)(handle); + return (queue_entry_t *)(void *)(q_int); } static inline queue_t qentry_to_int(queue_entry_t *qentry) diff --git a/platform/linux-generic/include/odp_schedule_if.h b/platform/linux-generic/include/odp_schedule_if.h index 9adacef7..5d10cd37 100644 --- a/platform/linux-generic/include/odp_schedule_if.h +++ b/platform/linux-generic/include/odp_schedule_if.h @@ -26,7 +26,7 @@ typedef int (*schedule_init_queue_fn_t)(uint32_t queue_index, typedef void (*schedule_destroy_queue_fn_t)(uint32_t queue_index); typedef int (*schedule_sched_queue_fn_t)(uint32_t queue_index); typedef int (*schedule_unsched_queue_fn_t)(uint32_t queue_index); -typedef int (*schedule_ord_enq_multi_fn_t)(queue_t handle, +typedef int (*schedule_ord_enq_multi_fn_t)(queue_t q_int, void *buf_hdr[], int num, int *ret); typedef int (*schedule_init_global_fn_t)(void); typedef int (*schedule_term_global_fn_t)(void); diff --git a/platform/linux-generic/odp_queue.c b/platform/linux-generic/odp_queue.c index 3e18f578..19945584 100644 --- a/platform/linux-generic/odp_queue.c +++ b/platform/linux-generic/odp_queue.c @@ -35,20 +35,22 @@ #include <string.h> #include <inttypes.h> +static int queue_init(queue_entry_t *queue, const char *name, + const odp_queue_param_t *param); + typedef struct queue_table_t { queue_entry_t queue[ODP_CONFIG_QUEUES]; } queue_table_t; static queue_table_t *queue_tbl; -static queue_t queue_from_ext(odp_queue_t handle); -static int _queue_enq(queue_t handle, odp_buffer_hdr_t *buf_hdr); -static odp_buffer_hdr_t *_queue_deq(queue_t handle); +static inline queue_entry_t *handle_to_qentry(odp_queue_t handle) +{ + uint32_t queue_id; -static int _queue_enq_multi(queue_t handle, odp_buffer_hdr_t *buf_hdr[], - int num); -static int _queue_deq_multi(queue_t handle, odp_buffer_hdr_t *buf_hdr[], - int num); + queue_id = queue_to_id(handle); + return get_qentry(queue_id); +} static inline odp_queue_t queue_from_id(uint32_t queue_id) { @@ -70,50 +72,6 @@ queue_entry_t *get_qentry(uint32_t queue_id) return &queue_tbl->queue[queue_id]; } -static int queue_init(queue_entry_t *queue, const char *name, - const odp_queue_param_t *param) -{ - if (name == NULL) { - queue->s.name[0] = 0; - } else { - strncpy(queue->s.name, name, ODP_QUEUE_NAME_LEN - 1); - queue->s.name[ODP_QUEUE_NAME_LEN - 1] = 0; - } - memcpy(&queue->s.param, param, sizeof(odp_queue_param_t)); - if (queue->s.param.sched.lock_count > sched_fn->max_ordered_locks()) - return -1; - - if (param->type == ODP_QUEUE_TYPE_SCHED) { - queue->s.param.deq_mode = ODP_QUEUE_OP_DISABLED; - - if (param->sched.sync == ODP_SCHED_SYNC_ORDERED) { - unsigned i; - - odp_atomic_init_u64(&queue->s.ordered.ctx, 0); - odp_atomic_init_u64(&queue->s.ordered.next_ctx, 0); - - for (i = 0; i < queue->s.param.sched.lock_count; i++) - odp_atomic_init_u64(&queue->s.ordered.lock[i], - 0); - } - } - queue->s.type = queue->s.param.type; - - queue->s.enqueue = _queue_enq; - queue->s.dequeue = _queue_deq; - queue->s.enqueue_multi = _queue_enq_multi; - queue->s.dequeue_multi = _queue_deq_multi; - - queue->s.pktin = PKTIN_INVALID; - queue->s.pktout = PKTOUT_INVALID; - - queue->s.head = NULL; - queue->s.tail = NULL; - - return 0; -} - - static int queue_init_global(void) { uint32_t i; @@ -204,27 +162,27 @@ static int queue_capability(odp_queue_capability_t *capa) static odp_queue_type_t queue_type(odp_queue_t handle) { - return qentry_from_int(queue_from_ext(handle))->s.type; + return handle_to_qentry(handle)->s.type; } static odp_schedule_sync_t queue_sched_type(odp_queue_t handle) { - return qentry_from_int(queue_from_ext(handle))->s.param.sched.sync; + return handle_to_qentry(handle)->s.param.sched.sync; } static odp_schedule_prio_t queue_sched_prio(odp_queue_t handle) { - return qentry_from_int(queue_from_ext(handle))->s.param.sched.prio; + return handle_to_qentry(handle)->s.param.sched.prio; } static odp_schedule_group_t queue_sched_group(odp_queue_t handle) { - return qentry_from_int(queue_from_ext(handle))->s.param.sched.group; + return handle_to_qentry(handle)->s.param.sched.group; } static int queue_lock_count(odp_queue_t handle) { - queue_entry_t *queue = qentry_from_int(queue_from_ext(handle)); + queue_entry_t *queue = handle_to_qentry(handle); return queue->s.param.sched.sync == ODP_SCHED_SYNC_ORDERED ? (int)queue->s.param.sched.lock_count : -1; @@ -299,7 +257,7 @@ void sched_cb_queue_destroy_finalize(uint32_t queue_index) static int queue_destroy(odp_queue_t handle) { queue_entry_t *queue; - queue = qentry_from_int(queue_from_ext(handle)); + queue = handle_to_qentry(handle); if (handle == ODP_QUEUE_INVALID) return -1; @@ -352,14 +310,14 @@ static int queue_context_set(odp_queue_t handle, void *context, uint32_t len ODP_UNUSED) { odp_mb_full(); - qentry_from_int(queue_from_ext(handle))->s.param.context = context; + handle_to_qentry(handle)->s.param.context = context; odp_mb_full(); return 0; } static void *queue_context(odp_queue_t handle) { - return qentry_from_int(queue_from_ext(handle))->s.param.context; + return handle_to_qentry(handle)->s.param.context; } static odp_queue_t queue_lookup(const char *name) @@ -385,7 +343,7 @@ static odp_queue_t queue_lookup(const char *name) return ODP_QUEUE_INVALID; } -static inline int enq_multi(queue_t handle, odp_buffer_hdr_t *buf_hdr[], +static inline int enq_multi(queue_t q_int, odp_buffer_hdr_t *buf_hdr[], int num) { int sched = 0; @@ -393,8 +351,8 @@ static inline int enq_multi(queue_t handle, odp_buffer_hdr_t *buf_hdr[], queue_entry_t *queue; odp_buffer_hdr_t *hdr, *tail, *next_hdr; - queue = qentry_from_int(handle); - if (sched_fn->ord_enq_multi(handle, (void **)buf_hdr, num, &ret)) + queue = qentry_from_int(q_int); + if (sched_fn->ord_enq_multi(q_int, (void **)buf_hdr, num, &ret)) return ret; /* Optimize the common case of single enqueue */ @@ -462,17 +420,17 @@ static inline int enq_multi(queue_t handle, odp_buffer_hdr_t *buf_hdr[], return num; /* All events enqueued */ } -static int _queue_enq_multi(queue_t handle, odp_buffer_hdr_t *buf_hdr[], - int num) +static int queue_int_enq_multi(queue_t q_int, odp_buffer_hdr_t *buf_hdr[], + int num) { - return enq_multi(handle, buf_hdr, num); + return enq_multi(q_int, buf_hdr, num); } -static int _queue_enq(queue_t handle, odp_buffer_hdr_t *buf_hdr) +static int queue_int_enq(queue_t q_int, odp_buffer_hdr_t *buf_hdr) { int ret; - ret = enq_multi(handle, &buf_hdr, 1); + ret = enq_multi(q_int, &buf_hdr, 1); if (ret == 1) return 0; @@ -489,7 +447,7 @@ static int queue_enq_multi(odp_queue_t handle, const odp_event_t ev[], int num) if (num > QUEUE_MULTI_MAX) num = QUEUE_MULTI_MAX; - queue = qentry_from_int(queue_from_ext(handle)); + queue = handle_to_qentry(handle); for (i = 0; i < num; i++) buf_hdr[i] = buf_hdl_to_hdr(odp_buffer_from_event(ev[i])); @@ -503,13 +461,13 @@ static int queue_enq(odp_queue_t handle, odp_event_t ev) odp_buffer_hdr_t *buf_hdr; queue_entry_t *queue; - queue = qentry_from_int(queue_from_ext(handle)); + queue = handle_to_qentry(handle); buf_hdr = buf_hdl_to_hdr(odp_buffer_from_event(ev)); return queue->s.enqueue(qentry_to_int(queue), buf_hdr); } -static inline int deq_multi(queue_t handle, odp_buffer_hdr_t *buf_hdr[], +static inline int deq_multi(queue_t q_int, odp_buffer_hdr_t *buf_hdr[], int num) { odp_buffer_hdr_t *hdr, *next; @@ -517,7 +475,7 @@ static inline int deq_multi(queue_t handle, odp_buffer_hdr_t *buf_hdr[], queue_entry_t *queue; int updated = 0; - queue = qentry_from_int(handle); + queue = qentry_from_int(q_int); LOCK(&queue->s.lock); if (odp_unlikely(queue->s.status < QUEUE_STATUS_READY)) { /* Bad queue, or queue has been destroyed. @@ -583,18 +541,18 @@ static inline int deq_multi(queue_t handle, odp_buffer_hdr_t *buf_hdr[], return i; } -static int _queue_deq_multi(queue_t handle, odp_buffer_hdr_t *buf_hdr[], - int num) +static int queue_int_deq_multi(queue_t q_int, odp_buffer_hdr_t *buf_hdr[], + int num) { - return deq_multi(handle, buf_hdr, num); + return deq_multi(q_int, buf_hdr, num); } -static odp_buffer_hdr_t *_queue_deq(queue_t handle) +static odp_buffer_hdr_t *queue_int_deq(queue_t q_int) { odp_buffer_hdr_t *buf_hdr = NULL; int ret; - ret = deq_multi(handle, &buf_hdr, 1); + ret = deq_multi(q_int, &buf_hdr, 1); if (ret == 1) return buf_hdr; @@ -611,7 +569,7 @@ static int queue_deq_multi(odp_queue_t handle, odp_event_t events[], int num) if (num > QUEUE_MULTI_MAX) num = QUEUE_MULTI_MAX; - queue = qentry_from_int(queue_from_ext(handle)); + queue = handle_to_qentry(handle); ret = queue->s.dequeue_multi(qentry_to_int(queue), buf_hdr, num); @@ -627,7 +585,7 @@ static odp_event_t queue_deq(odp_queue_t handle) queue_entry_t *queue; odp_buffer_hdr_t *buf_hdr; - queue = qentry_from_int(queue_from_ext(handle)); + queue = handle_to_qentry(handle); buf_hdr = queue->s.dequeue(qentry_to_int(queue)); if (buf_hdr) @@ -636,6 +594,49 @@ static odp_event_t queue_deq(odp_queue_t handle) return ODP_EVENT_INVALID; } +static int queue_init(queue_entry_t *queue, const char *name, + const odp_queue_param_t *param) +{ + if (name == NULL) { + queue->s.name[0] = 0; + } else { + strncpy(queue->s.name, name, ODP_QUEUE_NAME_LEN - 1); + queue->s.name[ODP_QUEUE_NAME_LEN - 1] = 0; + } + memcpy(&queue->s.param, param, sizeof(odp_queue_param_t)); + if (queue->s.param.sched.lock_count > sched_fn->max_ordered_locks()) + return -1; + + if (param->type == ODP_QUEUE_TYPE_SCHED) { + queue->s.param.deq_mode = ODP_QUEUE_OP_DISABLED; + + if (param->sched.sync == ODP_SCHED_SYNC_ORDERED) { + unsigned i; + + odp_atomic_init_u64(&queue->s.ordered.ctx, 0); + odp_atomic_init_u64(&queue->s.ordered.next_ctx, 0); + + for (i = 0; i < queue->s.param.sched.lock_count; i++) + odp_atomic_init_u64(&queue->s.ordered.lock[i], + 0); + } + } + queue->s.type = queue->s.param.type; + + queue->s.enqueue = queue_int_enq; + queue->s.dequeue = queue_int_deq; + queue->s.enqueue_multi = queue_int_enq_multi; + queue->s.dequeue_multi = queue_int_deq_multi; + + queue->s.pktin = PKTIN_INVALID; + queue->s.pktout = PKTOUT_INVALID; + + queue->s.head = NULL; + queue->s.tail = NULL; + + return 0; +} + void queue_lock(queue_entry_t *queue) { LOCK(&queue->s.lock); @@ -776,64 +777,65 @@ static uint64_t queue_to_u64(odp_queue_t hdl) return _odp_pri(hdl); } -static odp_pktout_queue_t queue_get_pktout(queue_t handle) +static odp_pktout_queue_t queue_get_pktout(queue_t q_int) { - return qentry_from_int(handle)->s.pktout; + return qentry_from_int(q_int)->s.pktout; } -static void queue_set_pktout(queue_t handle, odp_pktio_t pktio, int index) +static void queue_set_pktout(queue_t q_int, odp_pktio_t pktio, int index) { - qentry_from_int(handle)->s.pktout.pktio = pktio; - qentry_from_int(handle)->s.pktout.index = index; + queue_entry_t *qentry = qentry_from_int(q_int); + + qentry->s.pktout.pktio = pktio; + qentry->s.pktout.index = index; } -static odp_pktin_queue_t queue_get_pktin(queue_t handle) +static odp_pktin_queue_t queue_get_pktin(queue_t q_int) { - return qentry_from_int(handle)->s.pktin; + return qentry_from_int(q_int)->s.pktin; } -static void queue_set_pktin(queue_t handle, odp_pktio_t pktio, int index) +static void queue_set_pktin(queue_t q_int, odp_pktio_t pktio, int index) { - qentry_from_int(handle)->s.pktin.pktio = pktio; - qentry_from_int(handle)->s.pktin.index = index; + queue_entry_t *qentry = qentry_from_int(q_int); + + qentry->s.pktin.pktio = pktio; + qentry->s.pktin.index = index; } -static void queue_set_enq_func(queue_t handle, queue_enq_fn_t func) +static void queue_set_enq_func(queue_t q_int, queue_enq_fn_t func) { - qentry_from_int(handle)->s.enqueue = func; + qentry_from_int(q_int)->s.enqueue = func; } -static void queue_set_enq_multi_func(queue_t handle, queue_enq_multi_fn_t func) +static void queue_set_enq_multi_func(queue_t q_int, queue_enq_multi_fn_t func) { - qentry_from_int(handle)->s.enqueue_multi = func; + qentry_from_int(q_int)->s.enqueue_multi = func; } -static void queue_set_deq_func(queue_t handle, queue_deq_fn_t func) +static void queue_set_deq_func(queue_t q_int, queue_deq_fn_t func) { - qentry_from_int(handle)->s.dequeue = func; + qentry_from_int(q_int)->s.dequeue = func; } -static void queue_set_deq_multi_func(queue_t handle, queue_deq_multi_fn_t func) +static void queue_set_deq_multi_func(queue_t q_int, queue_deq_multi_fn_t func) { - qentry_from_int(handle)->s.dequeue_multi = func; + qentry_from_int(q_int)->s.dequeue_multi = func; } -static void queue_set_type(queue_t handle, odp_queue_type_t type) +static void queue_set_type(queue_t q_int, odp_queue_type_t type) { - qentry_from_int(handle)->s.type = type; + qentry_from_int(q_int)->s.type = type; } static queue_t queue_from_ext(odp_queue_t handle) { - uint32_t queue_id; - - queue_id = queue_to_id(handle); - return qentry_to_int(get_qentry(queue_id)); + return qentry_to_int(handle_to_qentry(handle)); } -static odp_queue_t queue_to_ext(queue_t handle) +static odp_queue_t queue_to_ext(queue_t q_int) { - return qentry_from_int(handle)->s.handle; + return qentry_from_int(q_int)->s.handle; } /* API functions */ @@ -866,10 +868,10 @@ queue_fn_t queue_default_fn = { .term_local = queue_term_local, .from_ext = queue_from_ext, .to_ext = queue_to_ext, - .enq = _queue_enq, - .enq_multi = _queue_enq_multi, - .deq = _queue_deq, - .deq_multi = _queue_deq_multi, + .enq = queue_int_enq, + .enq_multi = queue_int_enq_multi, + .deq = queue_int_deq, + .deq_multi = queue_int_deq_multi, .get_pktout = queue_get_pktout, .set_pktout = queue_set_pktout, .get_pktin = queue_get_pktin, diff --git a/platform/linux-generic/odp_schedule.c b/platform/linux-generic/odp_schedule.c index 011d4dc4..92634235 100644 --- a/platform/linux-generic/odp_schedule.c +++ b/platform/linux-generic/odp_schedule.c @@ -712,12 +712,12 @@ static inline int copy_events(odp_event_t out_ev[], unsigned int max) return i; } -static int schedule_ord_enq_multi(queue_t handle, void *buf_hdr[], +static int schedule_ord_enq_multi(queue_t q_int, void *buf_hdr[], int num, int *ret) { int i; uint32_t stash_num = sched_local.ordered.stash_num; - queue_entry_t *dst_queue = qentry_from_int(handle); + queue_entry_t *dst_queue = qentry_from_int(q_int); queue_entry_t *src_queue = sched_local.ordered.src_queue; if (!sched_local.ordered.src_queue || sched_local.ordered.in_order) diff --git a/platform/linux-generic/odp_schedule_iquery.c b/platform/linux-generic/odp_schedule_iquery.c index bdf1a460..79086843 100644 --- a/platform/linux-generic/odp_schedule_iquery.c +++ b/platform/linux-generic/odp_schedule_iquery.c @@ -1159,12 +1159,12 @@ static inline void schedule_release_context(void) schedule_release_atomic(); } -static int schedule_ord_enq_multi(queue_t handle, void *buf_hdr[], +static int schedule_ord_enq_multi(queue_t q_int, void *buf_hdr[], int num, int *ret) { int i; uint32_t stash_num = thread_local.ordered.stash_num; - queue_entry_t *dst_queue = qentry_from_int(handle); + queue_entry_t *dst_queue = qentry_from_int(q_int); queue_entry_t *src_queue = thread_local.ordered.src_queue; if (!thread_local.ordered.src_queue || thread_local.ordered.in_order) diff --git a/platform/linux-generic/odp_schedule_sp.c b/platform/linux-generic/odp_schedule_sp.c index 91d70e3a..66715c13 100644 --- a/platform/linux-generic/odp_schedule_sp.c +++ b/platform/linux-generic/odp_schedule_sp.c @@ -414,10 +414,10 @@ static int unsched_queue(uint32_t qi ODP_UNUSED) return 0; } -static int ord_enq_multi(queue_t handle, void *buf_hdr[], int num, +static int ord_enq_multi(queue_t q_int, void *buf_hdr[], int num, int *ret) { - (void)handle; + (void)q_int; (void)buf_hdr; (void)num; (void)ret;
Clean up function and parameter naming after modular interface patch. Queue_t type is referred as "queue internal": queue_int or q_int. Term "handle" is reserved for API level handles (e.g. odp_queue_t, odp_pktio_t, etc) through out linux-gen implementation. Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org> --- platform/linux-generic/include/odp_queue_if.h | 28 +-- .../linux-generic/include/odp_queue_internal.h | 4 +- platform/linux-generic/include/odp_schedule_if.h | 2 +- platform/linux-generic/odp_queue.c | 218 +++++++++++---------- platform/linux-generic/odp_schedule.c | 4 +- platform/linux-generic/odp_schedule_iquery.c | 4 +- platform/linux-generic/odp_schedule_sp.c | 4 +- 7 files changed, 133 insertions(+), 131 deletions(-) -- 2.13.0