>> 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
