Re: [lng-odp] [API-NEXT PATCH] linux-gen: queue: clean up after modular interface

[Bill Fischofer]

Reviewed-and-tested-by: Bill Fischofer 

On Tue, Jun 20, 2017 at 2:10 PM Maxim Uvarov 
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
> >
> >
>
>

Re: [lng-odp] [API-NEXT PATCH] linux-gen: queue: clean up after modular interface

[Maxim Uvarov]

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

Re: [lng-odp] [API-NEXT PATCH] linux-gen: queue: clean up after modular interface

[Savolainen, Petri (Nokia - FI/Espoo)]

> 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

Re: [lng-odp] [API-NEXT PATCH] linux-gen: queue: clean up after modular interface

[Honnappa Nagarahalli]

On 16 June 2017 at 05:25, Elo, Matias (Nokia - FI/Espoo)
 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 
>>> #include 
>>>
>>> +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 _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(>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(>s.ordered.ctx, 0);
>>> -   odp_atomic_init_u64(>s.ordered.next_ctx, 0);
>>> -
>>> -   for (i = 0; i < queue->s.param.sched.lock_count; 
>>> i++)
>>> -   
>>> odp_atomic_init_u64(>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);
>>> 

Re: [lng-odp] [API-NEXT PATCH] linux-gen: queue: clean up after modular interface

[Honnappa Nagarahalli]

On 16 June 2017 at 03:03, Peltonen, Janne (Nokia - FI/Espoo)
 wrote:
>
> Honnappa Nagarahalli wrote:
>> On 12 June 2017 at 06: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.
>> >
>>
>> "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
>
>

Re: [lng-odp] [API-NEXT PATCH] linux-gen: queue: clean up after modular interface

[Elo, Matias (Nokia - FI/Espoo)]

>>   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 
>> #include 
>> 
>> +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 _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(>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(>s.ordered.ctx, 0);
>> -   odp_atomic_init_u64(>s.ordered.next_ctx, 0);
>> -
>> -   for (i = 0; i < queue->s.param.sched.lock_count; i++)
>> -   
>> odp_atomic_init_u64(>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(>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(>s.ordered.ctx, 0);
>> +   

Re: [lng-odp] [API-NEXT PATCH] linux-gen: queue: clean up after modular interface

[Peltonen, Janne (Nokia - FI/Espoo)]

Honnappa Nagarahalli wrote:
> On 12 June 2017 at 06: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.
> >
> 
> "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

Re: [lng-odp] [API-NEXT PATCH] linux-gen: queue: clean up after modular interface

[Honnappa Nagarahalli]

On 12 June 2017 at 06: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.
>

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

Re: [lng-odp] [API-NEXT PATCH] linux-gen: queue: clean up after modular interface

[Dmitry Eremin-Solenikov]

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

[lng-odp] [API-NEXT PATCH] linux-gen: queue: clean up after modular interface

[Petri Savolainen]

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 
---
 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)
 {
-   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);