Couple of general comments first: 1) Discussion about various versions of this patch has not reached the mailing list. That needs to be fixed in Github configuration...
2) Queue handle/qentry naming should be consistent over the entire patch. Today: * "handle" refers to odp_queue_t * "queue" or "qe" refers to *queue_entry_t when inside of queue.c * "qentry" or "qe" refers to *queue_entry_t when outside of queue.c After this patch, the same should apply - especially for "handle". Try to minimize changes inside queue.c. When you add queue_t it should *not* be referred as "handle" (which is odp_queue_t), but something else - maybe consistently "queue_int". Otherwise, it's hard to follow which variable refers to odp_queue_t, queue_t or queue_entry_t. > -----Original Message----- > From: lng-odp [mailto:[email protected]] On Behalf Of > Github ODP bot > Sent: Saturday, June 03, 2017 7:00 AM > To: [email protected] > Subject: [lng-odp] [PATCH API-NEXT v5 1/1] linux-generic: queue: modular > queue interface > > From: Honnappa Nagarahalli <[email protected]> > > Created abstract queue type. Queue APIs and functions towards the > internal components are converted into function pointers. > > Signed-off-by: Honnappa Nagarahalli <[email protected]> > diff --git a/platform/linux-generic/odp_queue.c b/platform/linux- > generic/odp_queue.c > index dd430cd1..bc73728d 100644 > --- a/platform/linux-generic/odp_queue.c > +++ b/platform/linux-generic/odp_queue.c > @@ -6,6 +6,7 @@ > > #include <odp/api/queue.h> > #include <odp_queue_internal.h> > +#include <odp_queue_if.h> > #include <odp/api/std_types.h> > #include <odp/api/align.h> > #include <odp/api/buffer.h> > @@ -40,6 +41,14 @@ typedef struct queue_table_t { > > static queue_table_t *queue_tbl; > > +static int _queue_enq(queue_t handle, odp_buffer_hdr_t *buf_hdr); > +static odp_buffer_hdr_t *_queue_deq(queue_t handle); > + > +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); > + > static inline odp_queue_t queue_from_id(uint32_t queue_id) > { > return _odp_cast_scalar(odp_queue_t, queue_id + 1); > @@ -89,10 +98,10 @@ static int queue_init(queue_entry_t *queue, const char > *name, > } > 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.enqueue = _queue_enq; > + queue->s.dequeue = _queue_deq; > + queue->s.enqueue_multi = _queue_enq_multi; > + queue->s.dequeue_multi = _queue_deq_multi; No need to rename these. Static functions do not need prefix (not even '_'). > > -odp_queue_type_t odp_queue_type(odp_queue_t handle) > +static odp_queue_type_t queue_type(odp_queue_t handle) > { > - queue_entry_t *queue; > - > - queue = queue_to_qentry(handle); > - > - return queue->s.type; > + return qentry_from_int(queue_fn->from_ext(handle))->s.type; > } No need to use queue interface inside queue.c. It causes unnecessary indirection, which is harmful for both performance and code readability. Use a direct, inlined call instead. I'd prefer to keep it queue_to_qentry(). There should not be real reason to change, since this is internal queue.c. Keeping it the same would limit the change to minimum set that it needed for the new interface. > > -odp_schedule_sync_t odp_queue_sched_type(odp_queue_t handle) > +static odp_schedule_sync_t queue_sched_type(odp_queue_t handle) > { > - queue_entry_t *queue; > - > - queue = queue_to_qentry(handle); > - > - return queue->s.param.sched.sync; > + return qentry_from_int(queue_fn->from_ext(handle))- > >s.param.sched.sync; > } No need to use queue interface. Do not change implementation if not really needed. > > -odp_schedule_prio_t odp_queue_sched_prio(odp_queue_t handle) > +static odp_schedule_prio_t queue_sched_prio(odp_queue_t handle) > { > - queue_entry_t *queue; > - > - queue = queue_to_qentry(handle); > - > - return queue->s.param.sched.prio; > + return qentry_from_int(queue_fn->from_ext(handle))- > >s.param.sched.prio; > } No need to use queue interface. > > -odp_schedule_group_t odp_queue_sched_group(odp_queue_t handle) > +static odp_schedule_group_t queue_sched_group(odp_queue_t handle) > { > - queue_entry_t *queue; > - > - queue = queue_to_qentry(handle); > - > - return queue->s.param.sched.group; > + return qentry_from_int(queue_fn->from_ext(handle)) > + ->s.param.sched.group; > } No need to use queue interface. > > -int odp_queue_lock_count(odp_queue_t handle) > +static int queue_lock_count(odp_queue_t handle) > { > - queue_entry_t *queue = queue_to_qentry(handle); > + queue_entry_t *queue = qentry_from_int(queue_fn- > >from_ext(handle)); > > return queue->s.param.sched.sync == ODP_SCHED_SYNC_ORDERED ? > (int)queue->s.param.sched.lock_count : -1; > } No need to use queue interface. > > -odp_queue_t odp_queue_create(const char *name, const odp_queue_param_t > *param) > +static odp_queue_t queue_create(const char *name, > + const odp_queue_param_t *param) > { > uint32_t i; > queue_entry_t *queue; > @@ -291,10 +296,10 @@ void sched_cb_queue_destroy_finalize(uint32_t > queue_index) > UNLOCK(&queue->s.lock); > } > > -int odp_queue_destroy(odp_queue_t handle) > +static int queue_destroy(odp_queue_t handle) > { > queue_entry_t *queue; > - queue = queue_to_qentry(handle); > + queue = qentry_from_int(queue_fn->from_ext(handle)); > > if (handle == ODP_QUEUE_INVALID) > return -1; > @@ -343,25 +348,21 @@ int odp_queue_destroy(odp_queue_t handle) > return 0; > } No need to use queue interface. > > -int odp_queue_context_set(odp_queue_t handle, void *context, > - uint32_t len ODP_UNUSED) > +static int queue_context_set(odp_queue_t handle, void *context, > + uint32_t len ODP_UNUSED) > { > - queue_entry_t *queue; > - queue = queue_to_qentry(handle); > odp_mb_full(); > - queue->s.param.context = context; > + qentry_from_int(queue_fn->from_ext(handle))->s.param.context = > context; > odp_mb_full(); > return 0; > } No need to use queue interface. > > -void *odp_queue_context(odp_queue_t handle) > +static void *queue_context(odp_queue_t handle) > { > - queue_entry_t *queue; > - queue = queue_to_qentry(handle); > - return queue->s.param.context; > + return qentry_from_int(queue_fn->from_ext(handle))- > >s.param.context; > } No need to use queue interface. > > -odp_queue_t odp_queue_lookup(const char *name) > +static odp_queue_t queue_lookup(const char *name) > { > uint32_t i; > > @@ -384,15 +385,16 @@ odp_queue_t odp_queue_lookup(const char *name) > return ODP_QUEUE_INVALID; > } > > -static inline int enq_multi(queue_entry_t *queue, odp_buffer_hdr_t > *buf_hdr[], > +static inline int enq_multi(queue_t handle, odp_buffer_hdr_t *buf_hdr[], > int num) > { > int sched = 0; > int i, ret; > + queue_entry_t *queue; > odp_buffer_hdr_t *hdr, *tail, *next_hdr; > > - if (sched_fn->ord_enq_multi(queue->s.index, (void **)buf_hdr, > num, > - &ret)) > + queue = qentry_from_int(handle); > + if (sched_fn->ord_enq_multi(handle, (void **)buf_hdr, num, > &ret)) > return ret; > > /* Optimize the common case of single enqueue */ > @@ -460,16 +462,17 @@ static inline int enq_multi(queue_entry_t *queue, > odp_buffer_hdr_t *buf_hdr[], > return num; /* All events enqueued */ > } > > -int queue_enq_multi(queue_entry_t *queue, odp_buffer_hdr_t *buf_hdr[], > int num) > +static int _queue_enq_multi(queue_t handle, odp_buffer_hdr_t *buf_hdr[], > + int num) > { > - return enq_multi(queue, buf_hdr, num); > + return enq_multi(handle, buf_hdr, num); > } > > -int queue_enq(queue_entry_t *queue, odp_buffer_hdr_t *buf_hdr) > +static int _queue_enq(queue_t handle, odp_buffer_hdr_t *buf_hdr) > { > int ret; > > - ret = enq_multi(queue, &buf_hdr, 1); > + ret = enq_multi(handle, &buf_hdr, 1); > > if (ret == 1) > return 0; > @@ -477,7 +480,7 @@ int queue_enq(queue_entry_t *queue, odp_buffer_hdr_t > *buf_hdr) > return -1; > } > > -int odp_queue_enq_multi(odp_queue_t handle, const odp_event_t ev[], int > num) > +static int queue_enq_multi(odp_queue_t handle, const odp_event_t ev[], > int num) > { > odp_buffer_hdr_t *buf_hdr[QUEUE_MULTI_MAX]; > queue_entry_t *queue; > @@ -486,33 +489,35 @@ int odp_queue_enq_multi(odp_queue_t handle, const > odp_event_t ev[], int num) > if (num > QUEUE_MULTI_MAX) > num = QUEUE_MULTI_MAX; > > - queue = queue_to_qentry(handle); > + queue = qentry_from_int(queue_fn->from_ext(handle)); No need to use queue interface. > > for (i = 0; i < num; i++) > buf_hdr[i] = > buf_hdl_to_hdr(odp_buffer_from_event(ev[i])); > > - return num == 0 ? 0 : queue->s.enqueue_multi(queue, buf_hdr, > - > num); > + return num == 0 ? 0 : queue- > >s.enqueue_multi(qentry_to_int(queue), > + > buf_hdr, num); > } > > -int odp_queue_enq(odp_queue_t handle, odp_event_t ev) > +static int queue_enq(odp_queue_t handle, odp_event_t ev) > { > odp_buffer_hdr_t *buf_hdr; > queue_entry_t *queue; > > - queue = queue_to_qentry(handle); > + queue = qentry_from_int(queue_fn->from_ext(handle)); No need to use queue interface. > buf_hdr = buf_hdl_to_hdr(odp_buffer_from_event(ev)); > > - return queue->s.enqueue(queue, buf_hdr); > + return queue->s.enqueue(qentry_to_int(queue), buf_hdr); > } > > -static inline int deq_multi(queue_entry_t *queue, odp_buffer_hdr_t > *buf_hdr[], > +static inline int deq_multi(queue_t handle, odp_buffer_hdr_t *buf_hdr[], > int num) > { > odp_buffer_hdr_t *hdr, *next; > int i, j; > + queue_entry_t *queue; > int updated = 0; > > + queue = qentry_from_int(handle); > LOCK(&queue->s.lock); > if (odp_unlikely(queue->s.status < QUEUE_STATUS_READY)) { > /* Bad queue, or queue has been destroyed. > @@ -578,17 +583,18 @@ static inline int deq_multi(queue_entry_t *queue, > odp_buffer_hdr_t *buf_hdr[], > return i; > } > > -int queue_deq_multi(queue_entry_t *queue, odp_buffer_hdr_t *buf_hdr[], > int num) > +static int _queue_deq_multi(queue_t handle, odp_buffer_hdr_t *buf_hdr[], > + int num) > { > - return deq_multi(queue, buf_hdr, num); > + return deq_multi(handle, buf_hdr, num); > } > > -odp_buffer_hdr_t *queue_deq(queue_entry_t *queue) > +static odp_buffer_hdr_t *_queue_deq(queue_t handle) > { > odp_buffer_hdr_t *buf_hdr = NULL; > int ret; > > - ret = deq_multi(queue, &buf_hdr, 1); > + ret = deq_multi(handle, &buf_hdr, 1); > > if (ret == 1) > return buf_hdr; > @@ -596,7 +602,7 @@ odp_buffer_hdr_t *queue_deq(queue_entry_t *queue) > return NULL; > } > > -int odp_queue_deq_multi(odp_queue_t handle, odp_event_t events[], int > num) > +static int queue_deq_multi(odp_queue_t handle, odp_event_t events[], int > num) > { > queue_entry_t *queue; > odp_buffer_hdr_t *buf_hdr[QUEUE_MULTI_MAX]; > @@ -605,9 +611,9 @@ int odp_queue_deq_multi(odp_queue_t handle, > odp_event_t events[], int num) > if (num > QUEUE_MULTI_MAX) > num = QUEUE_MULTI_MAX; > > - queue = queue_to_qentry(handle); > + queue = qentry_from_int(queue_fn->from_ext(handle)); No need to use queue interface. > > - ret = queue->s.dequeue_multi(queue, buf_hdr, num); > + ret = queue->s.dequeue_multi(qentry_to_int(queue), buf_hdr, > num); > > for (i = 0; i < ret; i++) > events[i] = odp_buffer_to_event(buf_hdr[i]- > >handle.handle); > @@ -616,13 +622,13 @@ int odp_queue_deq_multi(odp_queue_t handle, > odp_event_t events[], int num) > } > > > -odp_event_t odp_queue_deq(odp_queue_t handle) > +static odp_event_t queue_deq(odp_queue_t handle) > { > queue_entry_t *queue; > odp_buffer_hdr_t *buf_hdr; > > - queue = queue_to_qentry(handle); > - buf_hdr = queue->s.dequeue(queue); > + queue = qentry_from_int(queue_fn->from_ext(handle)); No need to use queue interface. > + buf_hdr = queue->s.dequeue(qentry_to_int(queue)); > > if (buf_hdr) > return odp_buffer_to_event(buf_hdr->handle.handle); > @@ -640,7 +646,7 @@ void queue_unlock(queue_entry_t *queue) > UNLOCK(&queue->s.lock); > } > > -void odp_queue_param_init(odp_queue_param_t *params) > +static void queue_param_init(odp_queue_param_t *params) > { > memset(params, 0, sizeof(odp_queue_param_t)); > params->type = ODP_QUEUE_TYPE_PLAIN; > @@ -651,7 +657,7 @@ void odp_queue_param_init(odp_queue_param_t *params) > params->sched.group = ODP_SCHED_GROUP_ALL; > } > > -int odp_queue_info(odp_queue_t handle, odp_queue_info_t *info) > +static int queue_info(odp_queue_t handle, odp_queue_info_t *info) > { > uint32_t queue_id; > queue_entry_t *queue; > @@ -730,7 +736,7 @@ int sched_cb_queue_deq_multi(uint32_t queue_index, > odp_event_t ev[], int num) > queue_entry_t *qe = get_qentry(queue_index); > odp_buffer_hdr_t *buf_hdr[num]; > > - ret = deq_multi(qe, buf_hdr, num); > + ret = deq_multi(qentry_to_int(qe), buf_hdr, num); > > if (ret > 0) > for (i = 0; i < ret; i++) > @@ -765,7 +771,112 @@ int sched_cb_queue_empty(uint32_t queue_index) > return ret; > } > > -uint64_t odp_queue_to_u64(odp_queue_t hdl) > +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) > +{ > + return qentry_from_int(handle)->s.pktout; > +} > + > +static void queue_set_pktout(queue_t handle, odp_pktio_t pktio, int > index) > +{ > + qentry_from_int(handle)->s.pktout.pktio = pktio; > + qentry_from_int(handle)->s.pktout.index = index; > +} > + > +static odp_pktin_queue_t queue_get_pktin(queue_t handle) > +{ > + return qentry_from_int(handle)->s.pktin; > +} > + > +static void queue_set_pktin(queue_t handle, odp_pktio_t pktio, int index) > +{ > + qentry_from_int(handle)->s.pktin.pktio = pktio; > + qentry_from_int(handle)->s.pktin.index = index; > +} > + > +static void queue_set_enq_func(queue_t handle, queue_enq_fn_t func) > +{ > + qentry_from_int(handle)->s.enqueue = func; > +} > + > +static void queue_set_enq_multi_func(queue_t handle, queue_enq_multi_fn_t > func) > +{ > + qentry_from_int(handle)->s.enqueue_multi = func; > +} > + > +static void queue_set_deq_func(queue_t handle, queue_deq_fn_t func) > +{ > + qentry_from_int(handle)->s.dequeue = func; > +} > + > +static void queue_set_deq_multi_func(queue_t handle, queue_deq_multi_fn_t > func) > +{ > + qentry_from_int(handle)->s.dequeue_multi = func; > +} > + > +static void queue_set_type(queue_t handle, odp_queue_type_t type) > +{ > + qentry_from_int(handle)->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)); > +} I'd keep the old queue_to_qentry() as is, and implement this one by using it. > + > +static odp_queue_t queue_to_ext(queue_t handle) > +{ > + return qentry_from_int(handle)->s.handle; > +} > + > +/* API functions */ > +queue_api_t queue_default_api = { > + .queue_create = queue_create, > + .queue_destroy = queue_destroy, > + .queue_lookup = queue_lookup, > + .queue_capability = queue_capability, > + .queue_context_set = queue_context_set, > + .queue_context = queue_context, > + .queue_enq = queue_enq, > + .queue_enq_multi = queue_enq_multi, > + .queue_deq = queue_deq, > + .queue_deq_multi = queue_deq_multi, > + .queue_type = queue_type, > + .queue_sched_type = queue_sched_type, > + .queue_sched_prio = queue_sched_prio, > + .queue_sched_group = queue_sched_group, > + .queue_lock_count = queue_lock_count, > + .queue_to_u64 = queue_to_u64, > + .queue_param_init = queue_param_init, > + .queue_info = queue_info > +}; > + > +/* Functions towards internal components */ > +queue_fn_t queue_default_fn = { > + .init_global = queue_init_global, > + .term_global = queue_term_global, > + .init_local = queue_init_local, > + .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, > + .get_pktout = queue_get_pktout, > + .set_pktout = queue_set_pktout, > + .get_pktin = queue_get_pktin, > + .set_pktin = queue_set_pktin, > + .set_enq_fn = queue_set_enq_func, > + .set_enq_multi_fn = queue_set_enq_multi_func, > + .set_deq_fn = queue_set_deq_func, > + .set_deq_multi_fn = queue_set_deq_multi_func, > + .set_type = queue_set_type > +}; > diff --git a/platform/linux-generic/odp_queue_if.c b/platform/linux- > generic/odp_queue_if.c > new file mode 100644 > index 00000000..5a4f7194 > --- /dev/null > +++ b/platform/linux-generic/odp_queue_if.c > @@ -0,0 +1,103 @@ > +/* Copyright (c) 2016, ARM Limited Year 2017 -Petri
