On 6 June 2017 at 05:53, Savolainen, Petri (Nokia - FI/Espoo) <[email protected]> wrote: > 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. >
The same should not apply because this patch has introduced another level of abstract data type and the code should reflect that. I do agree that consistent naming needs to be used. Following is my proposal: Looking at "outside of queue.c" (including odp_schedule*.c) 1) structure members of type 'odp_queue_t' and 'queue_t'. Naming is not consistent. The naming is according to the context/purpose of those members. I suggest that we do not rename these. Except members of type 'queue_t' can be suffixed with '_int'. 2) local variables and function arguments of type 'odp_queue_t' and 'queue_t'. These can be named 'queue' and 'queue_int' respectively There is no point in referring to these as 'handle' or 'handle_int', these do not convey anything outside queue.* files. Looking at "inside of queue.c" 1) anything that refers to *queue_entry_t can be called as 'qentry' or 'qe' 2) variables and function arguments of type 'odp_queue_t' and 'queue_t'. These can be named 'handle' and 'handle_int' respectively. > > >> -----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. > Agree on not using the functions via the queue interface within queue. 'queue_from_ext' and 'qentry_from_int' are needed to be implemented. 'queue_to_qentry' becomes redundant. This code reflects how the handles gets converted to internal handle nicely. > > >> >> -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. After implementing queue_from_ext and qentry_from_int, queue_to_qentry() becomes redundant. > > >> + >> +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 > Agree > > > -Petri > >
