On 16 June 2017 at 05:25, Elo, Matias (Nokia - FI/Espoo)
<[email protected]> 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
>
