Re: [lng-odp] PKTIO hash capability

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

> -Original Message-
> From: Savolainen, Petri (Nokia - FI/Espoo)
> Sent: Tuesday, October 23, 2018 10:59 AM
> To: 'Liron Himi' ; lng-odp-forward  o...@lists.linaro.org>
> Subject: RE: PKTIO hash capability
> 
> > I have a platform that doesn't support hashing on input queues, only
> > classification.
> > I think it be good to add the below to the PKTIO capabilities.
> > As currently in the capabilities I declare to have several input
> > queues but application has not idea it cannot activate hash on those
> queues.
> >
> > odp_bool_t classifier_enable;
> > odp_bool_t hash_enable;
> >
> >
> > What is your opinion on this request?
> >
> 
> I think those can be added to capability. At the same time the capability
> struct could be cleaned up a bit. I'll look into this shortly.
> 
> -Petri
> 

Actually, you can already express this capability (that hashed input is not 
supported). Just return 1 as max_input_queues in odp_pktio_capability_t. 
Num_queues in pktin_queue_param is ignored when classifier is used, in other 
cases application must not exceed max_input_queues capability. If max number of 
input queues is 1, there's no need for hashing. The default is 1 input queue 
(no hashing, no classifier).

We may still clean up the pktio capability API, but you don't need to wait for 
that.

-Petri


typedef struct odp_pktio_capability_t {
/** Maximum number of input queues */
unsigned max_input_queues;


typedef struct odp_pktin_queue_param_t {
/** Number of input queues to be created
  *
  * When classifier is enabled in odp_pktin_queue_config() this
  * value is ignored, otherwise at least one queue is required.
  * More than one input queues require flow hashing configured.
  * The maximum value is defined by pktio capability 'max_input_queues'.
  * Queue type is defined by the input mode. The default value is 1. */
unsigned num_queues;

Re: [lng-odp] PKTIO hash capability

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

> I have a platform that doesn't support hashing on input queues, only
> classification.
> I think it be good to add the below to the PKTIO capabilities.
> As currently in the capabilities I declare to have several input queues but
> application has not idea it cannot activate hash on those queues.
> 
> odp_bool_t classifier_enable;
> odp_bool_t hash_enable;
> 
> 
> What is your opinion on this request?
> 

I think those can be added to capability. At the same time the capability 
struct could be cleaned up a bit. I'll look into this shortly.

-Petri

Re: [lng-odp] Are those Asynchronous ordered locks available for use in 1.19?

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

Hi,

These calls have been implemented by the API spec. So, you can use those in 
your application. Lock_start() hints implementation that a lock_wait() will 
follow, but implementation is free to ignore the hint (if there's nothing it 
can do as preparation for lock_wait()). Lock_wait() is the one that must ensure 
synchronization.

Odp-linux or odp-dpdk implementations ignore lock_start() and do normal lock 
call in lock_wait(), as the (current) SW implementation is synchronous. May be 
we could e.g. do a memory prefetch at lock_start(), but not much more.

However, ODP implementations with a HW scheduler may very well use this hint 
for their benefit and your application could run there faster when using 
lock_start() and lock_wait(), instead of the normal _lock(). For example, 
Cavium implementations may benefit from these calls, as these were added from 
their request.

-Petri

> -Original Message-
> From: lng-odp  On Behalf Of Yan, Liming
> (NSB - CN/Hangzhou)
> Sent: Wednesday, September 5, 2018 6:15 AM
> To: Bill Fischofer 
> Cc: lng-odp-forward 
> Subject: Re: [lng-odp] Are those Asynchronous ordered locks available for
> use in 1.19?
> 
> Hi,
>Thanks. You mean aync ordering lock needs HW support?  I’m using odp-
> dpdk in virtual machine. So can I use odp_schedule_order_lock_start() and
> odp_schedule_order_lock_wait() for async ordering lock?
> 
> 
> BR
> Yan Limin
> 
> From: Bill Fischofer [mailto:bill.fischo...@linaro.org]
> Sent: Tuesday, September 04, 2018 7:45 PM
> To: Yan, Liming (NSB - CN/Hangzhou) 
> Cc: lng-odp-forward 
> Subject: Re: [lng-odp] Are those Asynchronous ordered locks available for
> use in 1.19?
> 
> Yes, these APIs are available however only platforms with HW support will
> show a practical difference in behavior. These APIs are hints to the
> implementation, which is why you cannot assume you hold the lock until
> after odp_schedule_order_lock_wait() completes.
> 
> On Tue, Sep 4, 2018 at 6:41 AM Yan, Liming (NSB - CN/Hangzhou)
> mailto:liming@nokia-sbell.com>> wrote:
> Hi,
>   I see in change log:
> 
>Asynchronous ordered locks
>Two new APIs, odp_schedule_order_lock_start() and
> odp_schedule_order_lock_wait() are added to allow for asynchronous
> ordered lock acquisition in addition to the existing synchronous
> odp_schedule_order_lock() API. In some implementations and applications,
> there may be a performance advantage to indicating the intent to acquire an
> ordered lock to allow the implementation to prepare for this while the
> application continues parallel processing and then enter the critical section
> protected by the ordered lock at a later time. In this case ordered lock
> protection is not guaranteed until the odp_schedule_order_lock_wait() call
> returns.
> 
> But when I check the APIs code, seems they're still synchronous calling,
> similar with schedule_order_lock. Maybe I don't get how to use these APIs.  I
> didn't see any examples. So my question is, Are those Asynchronous ordered
> locks available for use?  And how to use them?  Thanks very much.
> 
> static void schedule_order_lock_wait(uint32_t lock_index) {
> schedule_order_lock(lock_index); }
> 
> 
> 
> BR
> Yan Limin
> 

Re: [lng-odp] odp_hash_crc32() vs python zlib crc32

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

Hi,

The check string is 9 bytes of input data (9 chars, not an integer) and output 
is a 32 bit integer (0xcbf43926 in case of CRC32). 

char input_data[] = "123456789";
uint32_t result;
uint64_t result_u64;

result = odp_hash_crc32(input_data, 9, 0x);

if (result == 0xcbf43926) {
// Correct CRC32 ...
}

Output of CRC32 is 32 bits, it does not matter if you store it into signed or 
unsigned 32 bit (or 64 bit) variable. Our function prototype returns uint32_t, 
but you can store it into any int type as long as it has at least 32 bits.

result_u64 = odp_hash_crc32(input_data, 9, 0x);

if (result_u64 == 0xcbf43926) {
// Correct CRC32 ...
}

-Petri


From: Daniel Feferman [mailto:dlfefer...@gmail.com] 
Sent: Tuesday, August 21, 2018 4:32 AM
To: Savolainen, Petri (Nokia - FI/Espoo) 
Cc: lng-odp@lists.linaro.org
Subject: Re: [lng-odp] odp_hash_crc32() vs python zlib crc32

Hi Petri,

Thanks for the feedback. Yes, trying "123456789" I'm able to check the result 
0xcbf43926, but the result of crc32 on ODP is a decimal and seems to be a 
positive number, always, am I wrong?

Best,
Daniel

On Mon, Aug 20, 2018 at 4:42 AM Savolainen, Petri (Nokia - FI/Espoo) 
<mailto:petri.savolai...@nokia.com> wrote:
Hi,

Did you give 0x as the init value for zlib call? CRC32 algorithm is 
defined with that as the init value. Also, you can check correct operation by 
trying to hash  "123456789" (the common check string), which should result 
0xcbf43926 as output for CRC32.

See e.g. http://reveng.sourceforge.net/crc-catalogue/17plus.htm#crc.cat-bits.32

-Petri

> -Original Message-
> From: lng-odp [mailto:mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of 
> Daniel
> Feferman
> Sent: Friday, August 17, 2018 7:18 PM
> To: mailto:lng-odp@lists.linaro.org
> Subject: [lng-odp] odp_hash_crc32() vs python zlib crc32
> 
> Hi all,
> 
> I'm comparing odp crc32 function vs python zlib
> <https://docs.python.org/3/library/zlib.html#zlib.crc32> library output and
> they are not matching. Was this behavior expected? Can someone give me a
> hint about this?
> 
> Best,
> Daniel

Re: [lng-odp] odp_hash_crc32() vs python zlib crc32

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

Hi,

Did you give 0x as the init value for zlib call? CRC32 algorithm is 
defined with that as the init value. Also, you can check correct operation by 
trying to hash  "123456789" (the common check string), which should result 
0xcbf43926 as output for CRC32.

See e.g. http://reveng.sourceforge.net/crc-catalogue/17plus.htm#crc.cat-bits.32

-Petri

> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of Daniel
> Feferman
> Sent: Friday, August 17, 2018 7:18 PM
> To: lng-odp@lists.linaro.org
> Subject: [lng-odp] odp_hash_crc32() vs python zlib crc32
> 
> Hi all,
> 
> I'm comparing odp crc32 function vs python zlib
>  library output and
> they are not matching. Was this behavior expected? Can someone give me a
> hint about this?
> 
> Best,
> Daniel

Re: [lng-odp] armv8 gcc lock free instructions

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

> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of Maxim
> Uvarov
> Sent: Wednesday, June 6, 2018 4:26 PM
> To: Maxim Kuvyrkov 
> Cc: LNG ODP Mailman List 
> Subject: [lng-odp] armv8 gcc lock free instructions
> 
> Hello Maxim,
> 
> we have discussion here that gcc for armv8 does not generated lock free
> instructions for 64 bit types. But all other arched do.
> 
> https://github.com/Linaro/odp/pull/611
> 
> Do you know reasons for that?
> 
> Thank you,
> Maxim.

The question is: why 128 bit GCC built-in atomics are not lock-free on ARMv8 
(__atomic_is_lock_free(16) returns false)? ARMv8.0 ISA has dual 64bit atomic 
instructions available. 

-Petri

Re: [lng-odp] odp_hash_crc32 support

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

> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of Daniel
> Feferman
> Sent: Tuesday, June 5, 2018 7:52 PM
> To: lng-odp@lists.linaro.org
> Subject: [lng-odp] odp_hash_crc32 support
> 
> Hi All,
> 
> I saw on this link:
> 
> https://www.opendataplane.org/api-
> documentation/master/api/group__odp__hash.html#ga9ecafbeec4af9a0cd
> 93311a1ee324725
> 
> That odp has support to Calculate CRC-32 without Castagnoli. I've tried so
> many ways and I was not able to do so. Then, I saw an example using
> Castagnoli, I've tried to implement it and using Castagnoli worked. So, I'd 
> like
> to double check, does ODP (still) has support to Calculate CRC-32 without
> Castagnoli?
> 
> Best,
> Daniel

Hi,

Yes, odp_hash_crc32() should be supported as specified by the API. Thanks for 
noticing that we miss the implementation. It will be added soon.

-Petri

Re: [lng-odp] odp_packet_data() considered harmful

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

> -Original Message-
> From: Dmitry Eremin-Solenikov [mailto:dmitry.ereminsoleni...@linaro.org]
> Sent: Thursday, February 15, 2018 4:00 PM
> To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@nokia.com>;
> Bill Fischofer <bill.fischo...@linaro.org>
> Cc: lng-odp-forward <lng-odp@lists.linaro.org>
> Subject: Re: [lng-odp] odp_packet_data() considered harmful
> 
> On 15/02/18 16:32, Savolainen, Petri (Nokia - FI/Espoo) wrote:
> 
> > odp_packet_offset() is more complex than the proposed
> odp_packet_data_seg_len(). Application mostly finds its data from the
> first segment, so it's important to keep the most common use case fast (==
> simple). We cannot and should not modify/remove current data() and
> offset() functions. But we can add another which combines data() +
> seg_len(), which is mostly enough for application. When application knows
> already that all (interesting) data is in the first segment, it can use
> data() which is the fastest and simplest function (to inline) to get
> access to the data.
> 
> I'm fine with proposed odp_packet_data_seg_len() if that will allow us
> to drop odp_packet_data().

There's no need or possibility to remove data(). Documentation for data() may 
be improved to be more explicit about possibility of multiple segments. The 
function just returns pointer to current head of data, it does not promise 
anything more. When application knows already how much data follows (and 
usually all data follows), it does not need ask e.g. segment length over and 
over again.

static inline void* odp_packet_data(pkt) {
return (pkt_desc_t)pkt->data_ptr;
}

static inline void* odp_packet_data_seg_len(pkt, *len) {
*len = (pkt_desc_t)pkt->seg_len;
return (pkt_desc_t)pkt->data_ptr;
}

The first is faster when application does not need to ask the segment length.

-Petri

Re: [lng-odp] odp_packet_data() considered harmful

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

From: Bill Fischofer [mailto:bill.fischo...@linaro.org] 
Sent: Thursday, February 15, 2018 2:49 PM
To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@nokia.com>
Cc: Dmitry Eremin-Solenikov <dmitry.ereminsoleni...@linaro.org>; 
lng-odp-forward <lng-odp@lists.linaro.org>
Subject: Re: [lng-odp] odp_packet_data() considered harmful

We already have such a routine: odp_packet_offset(pkt, 0, _len, NULL);

The fourth parameter of this routine returns the odp_packet_seg_t of the 
segment containing the specified offset (2nd parameter). I'm not aware of any 
use of this capability and would be happy to see it deprecated. We have a 
separate set of segment-oriented APIs in the (unlikely) event that applications 
want to use them.


odp_packet_offset() is more complex than the proposed 
odp_packet_data_seg_len(). Application mostly finds its data from the first 
segment, so it's important to keep the most common use case fast (== simple). 
We cannot and should not modify/remove current data() and offset() functions. 
But we can add another which combines data() + seg_len(), which is mostly 
enough for application. When application knows already that all (interesting) 
data is in the first segment, it can use data() which is the fastest and 
simplest function (to inline) to get access to the data.

Segment APIs are needed when application wants to walk through all segmented 
data (not common, but need the support).

-Petri

Re: [lng-odp] odp_packet_data() considered harmful

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

> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of
> Dmitry Eremin-Solenikov
> Sent: Tuesday, February 13, 2018 4:46 PM
> To: lng-odp-forward 
> Subject: [lng-odp] odp_packet_data() considered harmful
> 
> Hello,
> 
> Thanks to PR #470 I've stumbled upon odp_packet_data() function again. I
> think we should deprecate and remove it. It makes false assumption that
> the whole packet can be addressed using single pointer + offset from
> that. We already have public functions dealing with segments, getting
> data+len from specified offset, so odp_packet_data() is obsolete,
> confusing and usually results in plain wrong code.
> 
> --
> With best wishes
> Dmitry

odp_packet_data() is the basis for e.g. offsets. Offsets refer to number of 
bytes distance from data(). When application is unsure if packet is segmented 
it can use  odp_packet_seg_len() or other calls (odp_packet_is_segmented(), 
odp_packet_num_segs(), etc) to check if all packet_len() follows data() or not.

Since it's used so often data() must be simple and fast, it typically just 
returns pkt_hdr->data.

We can add a new function that in one call returns both data() and seg_len(), 
if that helps to understand that data() points to beginning of the first 
segment data. It would be useful also ABI compat in mind.

/* Packet data pointer and segment length
 *
 * Returns pointer to the first byte of data and outputs number of byte in the 
(first) segment.
 */
void *odp_packet_data_seg_len(odp_packet_t, uint32_t *seg_len)


Packet API was developed mainly before ABI compat time, so many small functions 
are thought to be inlined and just return one piece of metadata from packet 
descriptor.

-Petri

Re: [lng-odp] RSS in ODP

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

> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of Oriol
> Arcas
> Sent: Wednesday, January 03, 2018 7:12 PM
> To: LNG ODP Mailman List 
> Subject: [lng-odp] RSS in ODP
> 
> Hello and happy new year,
> 
> In our company we are looking into scaling the odp_schedule() calls.
> Currently we are manually doing Receive Side Scaling, which requires one
> CPU to receive all the packets and pass them to other worker CPU in a
> flow-deterministic way (i.e., not spreading the packets from a TCP flow to
> different CPUs). Obviously this is a bottleneck.
> 
> It would be great if ODP had optional RSS policies, which ultimately would
> assign packets from the same flow to a single thread in the schedule group
> (usually hashing the address tuple). Would this probably mean having
> dedicated queues?
> 
> I don't know if there is something similar in ODP already which I have
> missed. I'll thank any feedback!
> 
> Best regards,
> 
> --
> Oriol Arcas
> Software Engineer
> Starflow Networks


Our l2fwd test application (odp_l2fwd.c) configures packet input hashing, which 
is in practice RSS, but could be also some other implementation defined packet 
input hash function. You can take a look from there. The same hash 
configuration is possible for both direct pktin queues and scheduled event 
queues. For scheduled queues you would enable it something like this:

/* Normal interface open and config steps */
pktio = odp_pktio_open(dev, pool, _param);
odp_pktio_config(pktio, );

/*
 * Setup packet input hashing into scheduled event queues
 */
if (num_rx_queues > capa.max_input_queues)
num_rx_queues = capa.max_input_queues;

odp_pktin_queue_param_init(_param);

pktin_param.queue_param.sched.prio  = ODP_SCHED_PRIO_DEFAULT;
pktin_param.queue_param.sched.sync  = ODP_SCHED_SYNC_ATOMIC;
pktin_param.queue_param.sched.group = ODP_SCHED_GROUP_ALL;
pktin_param.hash_enable = 1;
pktin_param.hash_proto.proto.ipv4_udp = 1;
pktin_param.num_queues  = num_rx_queues;

if (odp_pktin_queue_config(pktio, _param))
return -1;

/* Optionally, see which event queues has been created by the previous call.
 * May e.g. want to set queue contexts here.
 */
if (odp_pktin_event_queue(pktio, rx_queues, num_rx_queues) != num_rx_queues)
return -1;

/* Starts packet input */
odp_pktio_start(pktio);

/* Use scheduler to receive packets ...*/

Re: [lng-odp] odp_shm_reserve() with same block name

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

The same name for N queues/pools/shms/etc may be still valuable for debugging. 
E.g. application may label “packet” vs. “timeout” pools/queues. When the 
application fails with a queue (named “timeout”), debugging can be started from 
timer code path (vs the packet code path). The code paths likely the same for 
all N “timeout” queues, no need to create N unique names just for debugging.

Names are both for debugging and lookups. NULL may be used when neither is done.

-Petri

From: Stanisław Kardach [mailto:k...@semihalf.com]
Sent: Monday, December 11, 2017 12:51 PM
To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@nokia.com>
Cc: LNG ODP Mailman List <lng-odp@lists.linaro.org>
Subject: Re: [lng-odp] odp_shm_reserve() with same block name

Thank you for the explanation.

So if application should use unique names only for objects it wants to lookup 
AND it is possible to have NULL names (same as having same name multiple 
times), wouldn't it be a bit cleaner to state for each module API:
1. If no name is given then object cannot be retrieved via lookup and multiple 
objects can be created with NULL name - therefore non-named objects are 
anonymous and can be created multiple times
2. If name is given it should be unique and can be retrieved via lookup

This way there is a clean separation of 2 application use-cases: retrieval with 
lookups and retrieval via other means.

I'm not getting (please forgive my ignorance) the situation where giving X 
queues the same name would be better than creating X queues with no name. 
Lookup is useless in the first case anyway so why bother? Is it useful to have 
both scenarios allowed?

--
Best Regards,
Stanisław Kardach

On Mon, Dec 11, 2017 at 9:43 AM, Savolainen, Petri (Nokia - FI/Espoo) 
<petri.savolai...@nokia.com<mailto:petri.savolai...@nokia.com>> wrote:


> -Original Message-
> From: lng-odp 
> [mailto:lng-odp-boun...@lists.linaro.org<mailto:lng-odp-boun...@lists.linaro.org>]
>  On Behalf Of
> Stanislaw Kardach
> Sent: Friday, December 08, 2017 6:03 PM
> To: LNG ODP Mailman List 
> <lng-odp@lists.linaro.org<mailto:lng-odp@lists.linaro.org>>
> Subject: [lng-odp] odp_shm_reserve() with same block name
>
> Hi all,
>
> While rebasing on top of latest master I've noticed a set of commits to
> shared_memory API that I don't quite understand:
>
> 63ef9b37 doc: shm: defining behaviour when blocks have same name
> 4ee15486 test: api: shm: test using the same block name multiple times
>
> What is a use-case of having multiple SHM blocks with the same name?
> Documentation now states that if multiple blocks with the same name are
> created then odp_shm_lookup() can return "any" block. That sounds a bit
> undefined. If user would assign same name to multiple blocks he will
> effectively make odp_shm_lookup() useless because he has no idea (it is
> not
> specified) which block it will return. However odp_shm_reserve() will not
> warn him of that.
>
> On the implementation side this definition matters to me as I'm using the
> names to ensure there aren't any duplicates and as a base for shared file
> names (in case of externally visible pools).
>
> --
> Best Regards,
> Stanisław Kardach

It would be extra effort for application to guarantee unique names always. 
Application needs to use unique names only for those blocks it's going to 
lookup. Since API does not require unique names, also lookup can be called when 
there are duplicates - it's just implementation defined which one is returned 
(works as one would expect - returns the first one found, does not crash, etc).

If implementation needs unique names internally, it can do that e.g. by 
combining name with handle value, etc. We actually updated all other APIs with 
name to accept NULL and non-unique names. SHM had some conflict at that time 
and the API was planned to be updated later, but forgot to do that. So, I'll 
send a patch to update the API.

-Petri

Re: [lng-odp] odp_shm_reserve() with same block name

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

> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of
> Stanislaw Kardach
> Sent: Friday, December 08, 2017 6:03 PM
> To: LNG ODP Mailman List 
> Subject: [lng-odp] odp_shm_reserve() with same block name
> 
> Hi all,
> 
> While rebasing on top of latest master I've noticed a set of commits to
> shared_memory API that I don't quite understand:
> 
> 63ef9b37 doc: shm: defining behaviour when blocks have same name
> 4ee15486 test: api: shm: test using the same block name multiple times
> 
> What is a use-case of having multiple SHM blocks with the same name?
> Documentation now states that if multiple blocks with the same name are
> created then odp_shm_lookup() can return "any" block. That sounds a bit
> undefined. If user would assign same name to multiple blocks he will
> effectively make odp_shm_lookup() useless because he has no idea (it is
> not
> specified) which block it will return. However odp_shm_reserve() will not
> warn him of that.
> 
> On the implementation side this definition matters to me as I'm using the
> names to ensure there aren't any duplicates and as a base for shared file
> names (in case of externally visible pools).
> 
> --
> Best Regards,
> Stanisław Kardach


It would be extra effort for application to guarantee unique names always. 
Application needs to use unique names only for those blocks it's going to 
lookup. Since API does not require unique names, also lookup can be called when 
there are duplicates - it's just implementation defined which one is returned 
(works as one would expect - returns the first one found, does not crash, etc).

If implementation needs unique names internally, it can do that e.g. by 
combining name with handle value, etc. We actually updated all other APIs with 
name to accept NULL and non-unique names. SHM had some conflict at that time 
and the API was planned to be updated later, but forgot to do that. So, I'll 
send a patch to update the API.

-Petri

Re: [lng-odp] odp dpdk

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

Maxim, try https://github.com/Linaro/odp/pull/313 It has been tested to fix 
checksum insert for 10/40GE Intel NICs.

-Petri

> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of
> Bogdan Pricope
> Sent: Monday, December 04, 2017 12:21 PM
> To: Maxim Uvarov 
> Cc: lng-odp-forward 
> Subject: Re: [lng-odp] odp dpdk
> 
> I suspect this is actually caused by csum issue in TX side: on RX,
> socket pktio does not validate csum (and accept the packets) but on
> dpdk pktio the csum is validated and packets are dropped.
> 
> I am not seeing this in my setup because default txq_flags for igb
> driver (1G interface) is
> 
> .txq_flags = 0
> 
> while for ixgbe (10G interface) is:
> 
> .txq_flags = ETH_TXQ_FLAGS_NOMULTSEGS |
> ETH_TXQ_FLAGS_NOOFFLOADS,
> 
> 
> /B
> 
> 
> 
> 
> On 1 December 2017 at 23:47, Maxim Uvarov  wrote:
> >
> > Looking to dpdk pktio support and generator. It looks like receive part
> > is broken. If for receive I use sockets it works well but receive with
> > dpdk does not get any packets. For both master and api-next. Can
> > somebody confirm please that it's so. Lava is not supper friendly to
> > debug issue.
> >
> >
> > 1. Recv
> > odp_generator -I 0 -m r -c 0x4
> >
> > https://lng.validation.linaro.org/scheduler/job/23206.1
> > Network devices using DPDK-compatible driver
> > 
> > :07:00.1 '82599ES 10-Gigabit SFI/SFP+ Network Connection 10fb'
> > drv=igb_uio unused=
> >
> >
> >
> > 2. Send
> > odp_generator -I 0 --srcmac 38:ea:a7:93:98:94 --dstmac 38:ea:a7:93:83:a0
> > --srcip 192.168.100.2 --dstip 192.168.100.1 -m u -i 0 -c 0x8 -p 18 -e
> > 5000 -f 5001 -n 8
> >
> > https://lng.validation.linaro.org/scheduler/job/23206.0
> >
> > Thank you,
> > Maxim.

Re: [lng-odp] L4 offset

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

> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of Liron
> Himi
> Sent: Wednesday, November 29, 2017 3:13 PM
> To: lng-odp@lists.linaro.org
> Subject: [lng-odp] L4 offset
> 
> Hi,
> 
> I'm trying to understand when pktio implementation should set the L4
> offset.
> In point of view, if there is a known L3 header than the L4 offset should
> be set to the L3-offset + L3 header length.
> I noticed that Linux generic implementation is not as above. E.g. if there
> is only IP header and payload, than L4 offset is set to
> 'ODP_PACKET_OFFSET_INVALID'.
> Is the above is the real intention?
> The API is documented as foolowed:
> /**
> * Layer 4 start offset
> *
> * Returns offset to the start of the layer 4 header. The offset is
> calculated
> * from the current odp_packet_data() position in bytes.
> *
> * User is responsible to update the offset when modifying the packet data
> * pointer position.
> *
> * @param pkt  Packet handle
> *
> * @return  Layer 4 start offset
> * @retval ODP_PACKET_OFFSET_INVALID packet does not contain a valid L4
> header
> *
> * @see odp_packet_l4_offset_set(), odp_packet_has_l4()
> */
> uint32_t odp_packet_l4_offset(odp_packet_t pkt);
> 
> What is the meaning of a valid L4 header? A payload is a valid L4? Maybe
> you should remove the 'header' and just mention that there is a L4 layer.
> 
> In order to calculate IP checksum, the IP-Header-length is needed and in
> absence of this exact value, the L4 offset can be used to calculate it.
> 
> Regards,
> Liron

I had planned to remove the word "valid" from there. This is an old function 
and since then we have added parser configuration options into pktio API.

/**
 * Parser configuration
 */
typedef struct odp_pktio_parser_config_t {
/** Protocol parsing level in packet input
  *
  * Parse protocol layers in minimum up to this level during packet
  * input. The default value is ODP_PKTIO_PARSER_LAYER_ALL. */
odp_pktio_parser_layer_t layer;

} odp_pktio_parser_config_t;


These l3/l4 offsets refer to the metadata set at packet input parsing time. I 
think we could update it to:


... Returns offset to the start of the layer 4...

* @return  Layer 4 start offset
* @retval  ODP_PACKET_OFFSET_INVALID packet does not contain L4


So, we could remove both "valid" and "header".

-Petri

Re: [lng-odp] Github shows commits in wrong order

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

> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of
> Savolainen, Petri (Nokia - FI/Espoo)
> Sent: Friday, November 17, 2017 10:24 AM
> To: lng-odp@lists.linaro.org
> Subject: [lng-odp] Github shows commits in wrong order
> 
> Hi,
> 
> Here's evidence that Github shows sometimes commits in a wrong order. The
> first commit of this patch set is "api: std_types: add odp_percent_t data
> type", but it shows in the Github web page as the last one!
> 
> It's annoying and counterproductive that reviewers cannot trust on the
> patch order.
> 
> 
> -Petri

Github has done another spin on the commit order lottery. Now, the first patch 
("add odp_percent_t") is in the middle.

-Petri

Re: [lng-odp] ODP vs Protocol headers

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

> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of
> Dmitry Eremin-Solenikov
> Sent: Friday, November 10, 2017 1:36 PM
> To: lng-odp-forward 
> Subject: [lng-odp] ODP vs Protocol headers
> 
> Hello,
> 
> Historically ODP helper provided protocol-related headers with
> linux-generic ODP implementation using modified private copy of them.
> The main reason for that was, if I remember correctly, that ODP should
> not provide protocol-related definitions.
> 
> I'd like to return to that question:
>  - I'm now adding more definitions to private protocol headers and I
> would not like for them to be too much out of sync.
>  - We started adding more and more protocol-specific handling in form of
> odp_packet_parse, odp packet flags, etc.
> 
> I'd propose to put protocol headers (ip.h, tcp.h, udp.h, eth.h) into
> public ODP namespace (to ) with the following
> phrase specifying them:
> 
> 
> These headers are not a part of ODP API specification, however they are
> provided to enable applications to use standard definitions for the
> protocol data. While neither of ODP API/ABI headers uses these protocol
> headers, an implementation SHOULD provide them AS IS to ease porting
> applications between ODP implementations.
> 
> 
> --
> With best wishes
> Dmitry

Protocol definitions in ODP API are high level - e.g. has_ip, has_ipv6, 
ipv4_chksum_status. Full protocol header definitions are not needed for using 
those feature flags.

Protocol headers definitions in the API (when the API itself does not need 
those) is an extra maintenance burden. Headers have type, flag, option, etc 
fields which value/usage tend to extend over time (new RFCs define new values 
and usage for old fields, etc). Once you add first headers into ODP API, people 
start requesting "one more value", "one more extension header", etc. After a 
while there would be a full protocol suite worth of protocol definitions to 
maintain.

Application have better change to maintain protocol definitions, since they 
need to concentrate to only those they implement - ODP would need to define 
all, but would not implement anything.

-Petri

Re: [lng-odp] [EXT] Re: classifier with pktio in ODP_PKTIN_MODE_QUEUE

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

To mix things, api-next introduces queue hashing *within* a CoS. But also there 
you use odp_cls_cos_queues() to get the implementation created queue handles, 
not odp_pktin_event_queue().

-Petri


> -Original Message-
> From: Savolainen, Petri (Nokia - FI/Espoo)
> Sent: Friday, November 10, 2017 1:13 PM
> To: 'Liron Himi' <lir...@marvell.com>; Bala Manoharan
> <bala.manoha...@linaro.org>
> Cc: lng-odp@lists.linaro.org
> Subject: RE: [EXT] Re: [lng-odp] classifier with pktio in
> ODP_PKTIN_MODE_QUEUE
> 
> odp_pktin_event_queue() is *not* used when classifier is enabled.
> 
> odp_queue_deq(q)
>   1) if q has events, return an event
>   2) if q does not have events, poll packet input
>  * packet input poll finds a packet
>  * classify the packet
>  * enqueue the packet into an event queue
>  * some later odp_queue_deq() call will find the packet from the queue
> (not necessary q, but the destination queue of the CoS)
> 
> -Petri
> 
> 
> > -Original Message-
> > From: Liron Himi [mailto:lir...@marvell.com]
> > Sent: Friday, November 10, 2017 12:23 PM
> > To: Bala Manoharan <bala.manoha...@linaro.org>; Savolainen, Petri (Nokia
> -
> > FI/Espoo) <petri.savolai...@nokia.com>
> > Cc: lng-odp@lists.linaro.org; Liron Himi <lir...@marvell.com>
> > Subject: RE: [EXT] Re: [lng-odp] classifier with pktio in
> > ODP_PKTIN_MODE_QUEUE
> >
> > Hi,
> >
> > I understand what you are saying, but I don't understand (probably I
> miss
> > something here) how classifier is working in PKTIO in a
> > ODP_PKTIN_MODE_QUEUE mode.
> > Application create its own queues, associate them to the cos and connect
> > them to the classifier.
> > In order to receive packets, application will call ' odp_queue_deq()'.
> But
> > how the packets will be enqueued to this queue?
> > Who will call ' odp_pktin_recv' to receive the packets, as in the
> schedule
> > mode case?
> >
> > Consider the following scenario and let me know if it is acceptable:
> > Application wants to configure classifier but doesn't want to use the
> odp-
> > schedule. So,
> > - PKTIO is configure with ' ODP_PKTIN_MODE_QUEUE' mode and classifier
> > enable.
> > - application will call ' odp_pktin_event_queue' to get all pktio's
> event
> > queues.
> > - application will associate those queues in the desired cos and
> configure
> > the classifier with them.
> > - application will call ' odp_queue_deq()' and as a result the '
> > odp_pktin_recv' will be triggered (as it is a PKTIO's event queue).
> > Implementation should not set the 'dst_queue' in this mode and
> > as a result the packet will be return to originated queue.
> >
> > Liron
> >
> > -Original Message-
> > From: Bala Manoharan [mailto:bala.manoha...@linaro.org]
> > Sent: Friday, November 10, 2017 11:27
> > To: Liron Himi <lir...@marvell.com>
> > Cc: lng-odp@lists.linaro.org
> > Subject: Re: [EXT] Re: [lng-odp] classifier with pktio in
> > ODP_PKTIN_MODE_QUEUE
> >
> > Comments inline...
> >
> > On 10 November 2017 at 12:01, Liron Himi <lir...@marvell.com> wrote:
> > > Hi Bala,
> > >
> > > According to the documentation, classifier can be operate with
> > PKTIN_QUEUE_MODE.
> > > /** Enable classifier
> > >   *
> > >   * * 0: Classifier is disabled (default)
> > >   * * 1: Classifier is enabled. Use classifier to direct
> > incoming
> > >   *  packets into pktin event queues. Classifier can be
> > enabled
> > >   *  only in ODP_PKTIN_MODE_SCHED and ODP_PKTIN_MODE_QUEUE
> > modes.
> > >   *  Both classifier and hashing cannot be enabled
> > simultaneously
> > >   *  ('hash_enable' must be 0). */
> > > odp_bool_t classifier_enable;
> > >
> > > So, Is there a mistake in the documentation or this combination should
> > be supported?
> >
> > odp_pktin_event_queue() can only be used when hashing is enabled since
> > when you connect more than one queue to pktio you need some mechanism to
> > spread the traffic. This configuration is given using
> > odp_pktin_hash_proto_t which is valid only when hashing is enabled.
> >
> > Since classifier and hashing are orthogonal you cannot use
> classification
> > and get packets using event queues configured using
> > odp_pktin_event_queue().
> > Packets delivered after classification can be received by appl

Re: [lng-odp] [EXT] Re: classifier with pktio in ODP_PKTIN_MODE_QUEUE

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

odp_pktin_event_queue() is *not* used when classifier is enabled.

odp_queue_deq(q)
  1) if q has events, return an event
  2) if q does not have events, poll packet input
 * packet input poll finds a packet
 * classify the packet
 * enqueue the packet into an event queue
 * some later odp_queue_deq() call will find the packet from the queue (not 
necessary q, but the destination queue of the CoS)

-Petri


> -Original Message-
> From: Liron Himi [mailto:lir...@marvell.com]
> Sent: Friday, November 10, 2017 12:23 PM
> To: Bala Manoharan <bala.manoha...@linaro.org>; Savolainen, Petri (Nokia -
> FI/Espoo) <petri.savolai...@nokia.com>
> Cc: lng-odp@lists.linaro.org; Liron Himi <lir...@marvell.com>
> Subject: RE: [EXT] Re: [lng-odp] classifier with pktio in
> ODP_PKTIN_MODE_QUEUE
> 
> Hi,
> 
> I understand what you are saying, but I don't understand (probably I miss
> something here) how classifier is working in PKTIO in a
> ODP_PKTIN_MODE_QUEUE mode.
> Application create its own queues, associate them to the cos and connect
> them to the classifier.
> In order to receive packets, application will call ' odp_queue_deq()'. But
> how the packets will be enqueued to this queue?
> Who will call ' odp_pktin_recv' to receive the packets, as in the schedule
> mode case?
> 
> Consider the following scenario and let me know if it is acceptable:
> Application wants to configure classifier but doesn't want to use the odp-
> schedule. So,
> - PKTIO is configure with ' ODP_PKTIN_MODE_QUEUE' mode and classifier
> enable.
> - application will call ' odp_pktin_event_queue' to get all pktio's event
> queues.
> - application will associate those queues in the desired cos and configure
> the classifier with them.
> - application will call ' odp_queue_deq()' and as a result the '
> odp_pktin_recv' will be triggered (as it is a PKTIO's event queue).
>   Implementation should not set the 'dst_queue' in this mode and
> as a result the packet will be return to originated queue.
> 
> Liron
> 
> -Original Message-
> From: Bala Manoharan [mailto:bala.manoha...@linaro.org]
> Sent: Friday, November 10, 2017 11:27
> To: Liron Himi <lir...@marvell.com>
> Cc: lng-odp@lists.linaro.org
> Subject: Re: [EXT] Re: [lng-odp] classifier with pktio in
> ODP_PKTIN_MODE_QUEUE
> 
> Comments inline...
> 
> On 10 November 2017 at 12:01, Liron Himi <lir...@marvell.com> wrote:
> > Hi Bala,
> >
> > According to the documentation, classifier can be operate with
> PKTIN_QUEUE_MODE.
> > /** Enable classifier
> >   *
> >   * * 0: Classifier is disabled (default)
> >   * * 1: Classifier is enabled. Use classifier to direct
> incoming
> >   *  packets into pktin event queues. Classifier can be
> enabled
> >   *  only in ODP_PKTIN_MODE_SCHED and ODP_PKTIN_MODE_QUEUE
> modes.
> >   *  Both classifier and hashing cannot be enabled
> simultaneously
> >   *  ('hash_enable' must be 0). */
> > odp_bool_t classifier_enable;
> >
> > So, Is there a mistake in the documentation or this combination should
> be supported?
> 
> odp_pktin_event_queue() can only be used when hashing is enabled since
> when you connect more than one queue to pktio you need some mechanism to
> spread the traffic. This configuration is given using
> odp_pktin_hash_proto_t which is valid only when hashing is enabled.
> 
> Since classifier and hashing are orthogonal you cannot use classification
> and get packets using event queues configured using
> odp_pktin_event_queue().
> Packets delivered after classification can be received by application
> either using odp_queue_deq() or odp_schedule() function depending upon the
> dst_queue configured with CoS.
> 
> Regards,
> Bala
> 
> >
> > Liron
> >
> > -Original Message-
> > From: Bala Manoharan [mailto:bala.manoha...@linaro.org]
> > Sent: Friday, November 10, 2017 07:18
> > To: Liron Himi <lir...@marvell.com>
> > Cc: lng-odp@lists.linaro.org
> > Subject: [EXT] Re: [lng-odp] classifier with pktio in
> > ODP_PKTIN_MODE_QUEUE
> >
> > External Email
> >
> > --
> > Hi,
> >
> > Both these modes cannot co-exist.
> >
> > There is a parameter 'classifier_enable' and 'hash_enable' as part of
> odp_pktin_queue_param_t which is used to identify whether classifier or
> hashing is enabled for a particular pktio interface.
> > Both classifier and hash configuration are orthogonal and only one of
> the two can be enabled a

Re: [lng-odp] [EXT] Re: classifier with pktio in ODP_PKTIN_MODE_QUEUE

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

odp_pktin_event_queue() is used when ODP creates the event queues == when 
hashing is enabled. When  classifier is enabled, application itself creates 
event queues and links those to CoS. So, application knows which event queues 
to poll.

Classifier can be used with both types of event queues, but 
odp_pktin_event_queue() is not used in either case.


-Petri


> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of Liron
> Himi
> Sent: Friday, November 10, 2017 8:32 AM
> To: Bala Manoharan 
> Cc: lng-odp@lists.linaro.org
> Subject: Re: [lng-odp] [EXT] Re: classifier with pktio in
> ODP_PKTIN_MODE_QUEUE
> 
> Hi Bala,
> 
> According to the documentation, classifier can be operate with
> PKTIN_QUEUE_MODE.
>   /** Enable classifier
> *
> * * 0: Classifier is disabled (default)
> * * 1: Classifier is enabled. Use classifier to direct
> incoming
> *  packets into pktin event queues. Classifier can be
> enabled
> *  only in ODP_PKTIN_MODE_SCHED and ODP_PKTIN_MODE_QUEUE
> modes.
> *  Both classifier and hashing cannot be enabled
> simultaneously
> *  ('hash_enable' must be 0). */
>   odp_bool_t classifier_enable;
> 
> So, Is there a mistake in the documentation or this combination should be
> supported?
> 
> Liron
> 
> -Original Message-
> From: Bala Manoharan [mailto:bala.manoha...@linaro.org]
> Sent: Friday, November 10, 2017 07:18
> To: Liron Himi 
> Cc: lng-odp@lists.linaro.org
> Subject: [EXT] Re: [lng-odp] classifier with pktio in ODP_PKTIN_MODE_QUEUE
> 
> External Email
> 
> --
> Hi,
> 
> Both these modes cannot co-exist.
> 
> There is a parameter 'classifier_enable' and 'hash_enable' as part of
> odp_pktin_queue_param_t which is used to identify whether classifier or
> hashing is enabled for a particular pktio interface.
> Both classifier and hash configuration are orthogonal and only one of the
> two can be enabled at any time. When you configure classification on the
> pktio interface then the packets will be routed only through classifier
> 'dst_queue' and not through event queues configured with the pktio
> interface and vice versa for 'hash_enable'.
> 
> odp_pktin_event_queue() API is useful only for platforms which do not
> support classifier system. This is mainly targetted for HWs which only
> have RSS configured on the pktio and does not support flow-based
> classification. The same is documented in odp_pktin_queue_param_t
> structure pls check the documentation and let us know if you find any
> discrepancy.
> 
> Hope this clarifies,
> Bala
> 
> 
> On 9 November 2017 at 23:04, Liron Himi  wrote:
> > Hi,
> >
> > I'm trying to understand how odp-classifier can work with PKTIO in
> ODP_PKTIN_MODE_QUEUE mode, as this combination is allow in the API.
> > When configuring PKTIO in ODP_PKTIN_MODE_QUEUE then application should
> call 'odp_pktin_event_queue' to retrieve the odp-queues.
> > When application create a cos, to be used for classification, does it
> needs to create new odp-queue or used the pktio's ones?
> > When application wants to receive packets from this PKTIO, it needs to
> call 'odp_queue_deq_multi' on one of the PKTIO's queues,right?
> > As a result of a received packet, the matched cos is being selected and
> it's queue is being set as the dst_queue of the packet.
> > Then the 'pktin_recv_buf' function will enqueuer this packet to the
> 'dst_queue' if it was set.
> > So finally the packet will be located at the cos's queue and not at
> > the PKTIO's queue and the application will get zero packets from
> 'odp_queue_deq_multi'.
> > So, either this mode isn't supported with classifier or if this mode is
> set then the 'dst_queue' shouldn't be set and the packets will be located
> at the PKTIO's queue.
> >
> > Regards,
> > Liron
> >

Re: [lng-odp] [PATCH API-NEXT v3 0/3] api: ones complement metadata

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

Weekly ping.

> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of
> Savolainen, Petri (Nokia - FI/Espoo)
> Sent: Wednesday, November 01, 2017 9:51 AM
> To: lng-odp@lists.linaro.org
> Subject: Suspected SPAM - Re: [lng-odp] [PATCH API-NEXT v3 0/3] api: ones
> complement metadata
> 
> Ping.
> 
> > -Original Message-
> > From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of
> > Github ODP bot
> > Sent: Thursday, October 26, 2017 3:00 PM
> > To: lng-odp@lists.linaro.org
> > Subject: [lng-odp] [PATCH API-NEXT v3 0/3] api: ones complement metadata
> >
> > Added packet metadata for ones complement sum over IP
> > payload in a packet. Some NICs calculate the sum
> > during packet input (at least for IP fragments) and
> > store the value into the packet descriptor. This offloads
> > L4 checksum calculation for IP fragments as SW does not
> > need sum all payload data, but just combine pre-calculated
> > sums from packet descriptors and remove extra header fields
> > from the sum.
> >
> > github
> > /** Email created from pull request 242 (psavol:next-checksum-metadata)
> >  ** https://github.com/Linaro/odp/pull/242
> >  ** Patch: https://github.com/Linaro/odp/pull/242.patch
> >  ** Base sha: 63d92eb289261d1534b5b9e1e04291faa5e45d30
> >  ** Merge commit sha: 5c16247e4ce2735df80c66f11dd9c9708e8c905f
> >  **/
> > /github
> >
> > checkpatch.pl
> > total: 0 errors, 0 warnings, 0 checks, 26 lines checked
> >
> >
> > to_send-p-000.patch has no obvious style problems and is ready for
> > submission.
> > total: 0 errors, 0 warnings, 0 checks, 14 lines checked
> >
> >
> > to_send-p-001.patch has no obvious style problems and is ready for
> > submission.
> > total: 0 errors, 0 warnings, 0 checks, 55 lines checked
> >
> >
> > to_send-p-002.patch has no obvious style problems and is ready for
> > submission.
> > /checkpatch.pl

Re: [lng-odp] [PATCH API-NEXT v3 0/3] api: ones complement metadata

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

Ping.

> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of
> Github ODP bot
> Sent: Thursday, October 26, 2017 3:00 PM
> To: lng-odp@lists.linaro.org
> Subject: [lng-odp] [PATCH API-NEXT v3 0/3] api: ones complement metadata
> 
> Added packet metadata for ones complement sum over IP
> payload in a packet. Some NICs calculate the sum
> during packet input (at least for IP fragments) and
> store the value into the packet descriptor. This offloads
> L4 checksum calculation for IP fragments as SW does not
> need sum all payload data, but just combine pre-calculated
> sums from packet descriptors and remove extra header fields
> from the sum.
> 
> github
> /** Email created from pull request 242 (psavol:next-checksum-metadata)
>  ** https://github.com/Linaro/odp/pull/242
>  ** Patch: https://github.com/Linaro/odp/pull/242.patch
>  ** Base sha: 63d92eb289261d1534b5b9e1e04291faa5e45d30
>  ** Merge commit sha: 5c16247e4ce2735df80c66f11dd9c9708e8c905f
>  **/
> /github
> 
> checkpatch.pl
> total: 0 errors, 0 warnings, 0 checks, 26 lines checked
> 
> 
> to_send-p-000.patch has no obvious style problems and is ready for
> submission.
> total: 0 errors, 0 warnings, 0 checks, 14 lines checked
> 
> 
> to_send-p-001.patch has no obvious style problems and is ready for
> submission.
> total: 0 errors, 0 warnings, 0 checks, 55 lines checked
> 
> 
> to_send-p-002.patch has no obvious style problems and is ready for
> submission.
> /checkpatch.pl

Re: [lng-odp] [PATCH API-NEXT v4 0/8] api: pool subparameters

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

Ping.

> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of
> Github ODP bot
> Sent: Friday, October 27, 2017 11:00 AM
> To: lng-odp@lists.linaro.org
> Subject: [lng-odp] [PATCH API-NEXT v4 0/8] api: pool subparameters
> 
> Added packet pool parameters for more fine grained pool
> configuration. The basic usage of the parameters is not changed,
> except that implementation may now round up 'num' by default.
> Application can limit the round up with new 'max_num' parameter.
> If application does not limit max_num (but is still interested on
> max_num value), it can check the new info.pkt.max_num.
> The new specification allows implementation to use sub-pools,
> also application is able to request e.g. this kind of pool:
>^
> |
> 
> max_num | - - - - - - - -
> |
> |   *
> NUM|   * *
> |   * * *
> | * * * * *
> | * * * * * *
> +--->
> LEN
> 
> github
> /** Email created from pull request 234 (psavol:next-pool-param)
>  ** https://github.com/Linaro/odp/pull/234
>  ** Patch: https://github.com/Linaro/odp/pull/234.patch
>  ** Base sha: d61d32590d1772b70b8dcd0d0ec44d29029d7443
>  ** Merge commit sha: 436021e0a0bff8bda38fa420ebbc4545c97a7fc8
>  **/
> /github
> 
> checkpatch.pl
> total: 0 errors, 0 warnings, 0 checks, 58 lines checked
> 
> 
> to_send-p-000.patch has no obvious style problems and is ready for
> submission.
> total: 0 errors, 0 warnings, 0 checks, 72 lines checked
> 
> 
> to_send-p-001.patch has no obvious style problems and is ready for
> submission.
> total: 0 errors, 0 warnings, 0 checks, 124 lines checked
> 
> 
> to_send-p-002.patch has no obvious style problems and is ready for
> submission.
> total: 0 errors, 0 warnings, 0 checks, 122 lines checked
> 
> 
> to_send-p-003.patch has no obvious style problems and is ready for
> submission.
> total: 0 errors, 0 warnings, 0 checks, 59 lines checked
> 
> 
> to_send-p-004.patch has no obvious style problems and is ready for
> submission.
> total: 0 errors, 0 warnings, 0 checks, 9 lines checked
> 
> 
> to_send-p-005.patch has no obvious style problems and is ready for
> submission.
> total: 0 errors, 0 warnings, 0 checks, 47 lines checked
> 
> 
> to_send-p-006.patch has no obvious style problems and is ready for
> submission.
> total: 0 errors, 0 warnings, 0 checks, 217 lines checked
> 
> 
> to_send-p-007.patch has no obvious style problems and is ready for
> submission.
> /checkpatch.pl

Re: [lng-odp] [PATCH API-NEXT v4 1/3] api: ipsec: rework ODP_IPSEC_SA_DISABLE into packet error

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

> -Original Message-
> From: Dmitry Eremin-Solenikov [mailto:dmitry.ereminsoleni...@linaro.org]
> Sent: Thursday, October 26, 2017 3:45 PM
> To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@nokia.com>;
> Github ODP bot <odp...@yandex.ru>; lng-odp@lists.linaro.org
> Subject: Re: [lng-odp] [PATCH API-NEXT v4 1/3] api: ipsec: rework
> ODP_IPSEC_SA_DISABLE into packet error
> 
> On 26/10/17 14:55, Savolainen, Petri (Nokia - FI/Espoo) wrote:
> >
> >
> >> -Original Message-
> >> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of
> >> Github ODP bot
> >> Sent: Wednesday, October 25, 2017 12:00 PM
> >> To: lng-odp@lists.linaro.org
> >> Subject: [lng-odp] [PATCH API-NEXT v4 1/3] api: ipsec: rework
> >> ODP_IPSEC_SA_DISABLE into packet error
> >>
> >> From: Dmitry Eremin-Solenikov <dmitry.ereminsoleni...@linaro.org>
> >>
> >> According to the discussion on mailing list, most of implementations
> >> will not be able to support odp_ipsec_sa_disable() status event
> >> directly.  Instead they will submit a dummy packet to that SA. Then
> >> after receiving this packet after odp_ipsec_result() will detect this
> >> packet and will report it as a packet with
> >> odp_ipsec_error_t->sa_disabled bit set.
> >>
> >> Signed-off-by: Dmitry Eremin-Solenikov
> <dmitry.ereminsoleni...@linaro.org>
> >> Cc: Nikhil Agarwal <nikhil.agar...@linaro.org>
> >> Cc: Balasubramanian Manoharan <bala.manoha...@linaro.org>
> >> ---
> >> /** Email created from pull request 256 (lumag:ipsec_sa_disable_v2)
> >>  ** https://github.com/Linaro/odp/pull/256
> >>  ** Patch: https://github.com/Linaro/odp/pull/256.patch
> >>  ** Base sha: 825f75ed8644ef57c5648961e7982daf13cd9375
> >>  ** Merge commit sha: ba520d0a3f4c46777c7aedca029e9979a89c69e7
> >>  **/
> >>  include/odp/api/spec/ipsec.h | 44 
> ---
> >> -
> >>  1 file changed, 20 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/include/odp/api/spec/ipsec.h
> b/include/odp/api/spec/ipsec.h
> >> index 26e852fca..b9ad282ce 100644
> >> --- a/include/odp/api/spec/ipsec.h
> >> +++ b/include/odp/api/spec/ipsec.h
> >> @@ -843,10 +843,12 @@ odp_ipsec_sa_t odp_ipsec_sa_create(const
> >> odp_ipsec_sa_param_t *param);
> >>   *
> >>   * When in synchronous operation mode, the call will return when it's
> >> possible
> >>   * to destroy the SA. In asynchronous mode, the same is indicated by
> an
> >> - * ODP_EVENT_IPSEC_STATUS event sent to the queue specified for the
> SA.
> >> The
> >> - * status event is guaranteed to be the last event for the SA, i.e.
> all
> >> - * in-progress operations have completed and resulting events
> (including
> >> status
> >> - * events) have been enqueued before it.
> >> + * artificial packet event sent to the queue specified for the SA
> having
> >> + * sa_disabled error bit set in the odp_ipsec_packet_result_t returned
> by
> >> + * odp_ipsec_result(). The packet is guaranteed to be the last event
> for
> >> + * the SA, i.e. all in-progress operations have completed and
> resulting
> >> events
> >> + * (including status events) have been enqueued before it. No packets
> >> will come
> >> + * from SA after this one.
> >
> > This still lacks the definition of what is the difference between an
> artificial packet vs a normal packet. We could define it e.g. by saying
> that length is always zero and application must not do anything else with
> it than free it. But then, why it's a packet anyway? Why it would not then
> be e.g. a ipsec status event (which is not a packet, but could be
> implemented as an artificial packet).
> 
> No difference from my POV. Processing path for all packets coming from
> IPsec:
> - Error packet. Process error flags, drop the packet.
> - Not an error. Process warning flags, forward the packet.

Maybe then it should not be called artificial packet but an error packet. A 
packet that has odp_packet_has_error() set (on main type level), but on closer 
look it would be an ipsec packet with ipsec error/warning set.

Still application would need to distinguish if this packet was actually sent by 
the remote end (a valid packet received from the tunnel, but marked with SA 
disable warning) or not (was not received from the tunnel).

> 
> >
> > The idea of event type vs sub-event type is that:
> >  1) all events

Re: [lng-odp] [PATCH API-NEXT v4 1/3] api: ipsec: rework ODP_IPSEC_SA_DISABLE into packet error

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

> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of
> Github ODP bot
> Sent: Wednesday, October 25, 2017 12:00 PM
> To: lng-odp@lists.linaro.org
> Subject: [lng-odp] [PATCH API-NEXT v4 1/3] api: ipsec: rework
> ODP_IPSEC_SA_DISABLE into packet error
> 
> From: Dmitry Eremin-Solenikov 
> 
> According to the discussion on mailing list, most of implementations
> will not be able to support odp_ipsec_sa_disable() status event
> directly.  Instead they will submit a dummy packet to that SA. Then
> after receiving this packet after odp_ipsec_result() will detect this
> packet and will report it as a packet with
> odp_ipsec_error_t->sa_disabled bit set.
> 
> Signed-off-by: Dmitry Eremin-Solenikov 
> Cc: Nikhil Agarwal 
> Cc: Balasubramanian Manoharan 
> ---
> /** Email created from pull request 256 (lumag:ipsec_sa_disable_v2)
>  ** https://github.com/Linaro/odp/pull/256
>  ** Patch: https://github.com/Linaro/odp/pull/256.patch
>  ** Base sha: 825f75ed8644ef57c5648961e7982daf13cd9375
>  ** Merge commit sha: ba520d0a3f4c46777c7aedca029e9979a89c69e7
>  **/
>  include/odp/api/spec/ipsec.h | 44 ---
> -
>  1 file changed, 20 insertions(+), 24 deletions(-)
> 
> diff --git a/include/odp/api/spec/ipsec.h b/include/odp/api/spec/ipsec.h
> index 26e852fca..b9ad282ce 100644
> --- a/include/odp/api/spec/ipsec.h
> +++ b/include/odp/api/spec/ipsec.h
> @@ -843,10 +843,12 @@ odp_ipsec_sa_t odp_ipsec_sa_create(const
> odp_ipsec_sa_param_t *param);
>   *
>   * When in synchronous operation mode, the call will return when it's
> possible
>   * to destroy the SA. In asynchronous mode, the same is indicated by an
> - * ODP_EVENT_IPSEC_STATUS event sent to the queue specified for the SA.
> The
> - * status event is guaranteed to be the last event for the SA, i.e. all
> - * in-progress operations have completed and resulting events (including
> status
> - * events) have been enqueued before it.
> + * artificial packet event sent to the queue specified for the SA having
> + * sa_disabled error bit set in the odp_ipsec_packet_result_t returned by
> + * odp_ipsec_result(). The packet is guaranteed to be the last event for
> + * the SA, i.e. all in-progress operations have completed and resulting
> events
> + * (including status events) have been enqueued before it. No packets
> will come
> + * from SA after this one.

This still lacks the definition of what is the difference between an artificial 
packet vs a normal packet. We could define it e.g. by saying that length is 
always zero and application must not do anything else with it than free it. But 
then, why it's a packet anyway? Why it would not then be e.g. a ipsec status 
event (which is not a packet, but could be implemented as an artificial packet).

The idea of event type vs sub-event type is that:
 1) all events of the same base type (e.g. packet) contain the same metadata 
and can be processed the same way. For example, someone may write a generic 
packet statistics (application) module, and plug that before and after an ipsec 
termination (application) module, without need to know that packets before 
ipsec module carry extra ipsec metadata (from inline inbound ipsec).
 2) sub-type adds metadata and other properties to the base type

An artificial packet would not fit in either category, it would be a non-packet.

Usually, application receives multiple event types anyway: e.g. packets and 
timeouts. So, for application it would not be an issue to have one additional 
switch-case for IPSEC status events. API integrity is more important than 
(potential) save of couple if-else branches.

So, I think this solution is not complete yet and I don't see how it would in 
the end make a real difference to the current status event API.

-Petri

Re: [lng-odp] [PATCH API-NEXT v2 0/3] api: ones complement metadata

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

I sent v3 which adds data range as output. There are small differences between 
NICs on what is included to the sum, e.g. some include IP options, others 
don't. So, it's better not to force all implementations to the same data range, 
since (as Janne said) it would cause implementations to always adjust the sum - 
also when an application is not interested about it. Sum cannot be "lazy" 
adjusted as application might have modified the data already.

-Petri


> -Original Message-
> From: Peltonen, Janne (Nokia - FI/Espoo)
> Sent: Thursday, October 26, 2017 11:45 AM
> To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@nokia.com>;
> Github ODP bot <odp...@yandex.ru>; lng-odp@lists.linaro.org
> Subject: Re: [lng-odp] [PATCH API-NEXT v2 0/3] api: ones complement
> metadata
> 
> As discussed offline with Petri, I feel that it would be useful
> to let the ODP implementation tell from which packet offset the
> sum calculation starts (or even let the implementation specify
> the range, i.e. also the end).
> 
> The rationale is that even though starting from L4 offset is
> reasonable from API point-of-view, different HW may start the
> sum calculation from different offsets and fixing the sum in
> SW is somewhat costly in those cases where the application is
> not actually interested in the sum, for example:
> 
> - An application both forwards packets receives packets locally
>   and needs checksum checking only for the locally received packets.
> - An application receives all incoming traffic locally, but drops
>   some of it before it event wants to check the checksum (e.g.
>   QoS filtering, one fragment lost, packet to wrong TCP port).
> 
>   Janne
> 
> > -----Original Message-----
> > From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of
> Savolainen, Petri
> > (Nokia - FI/Espoo)
> > Sent: Thursday, October 26, 2017 10:00 AM
> > To: Github ODP bot <odp...@yandex.ru>; lng-odp@lists.linaro.org
> > Subject: Suspected SPAM - Re: [lng-odp] [PATCH API-NEXT v2 0/3] api:
> ones complement
> > metadata
> >
> > [This sender failed our fraud detection checks and may not be who they
> appear to be. Learn
> > about spoofing at http://aka.ms/LearnAboutSpoofing]
> >
> > Ping.
> >
> > > -Original Message-
> > > From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of
> > > Github ODP bot
> > > Sent: Monday, October 23, 2017 1:00 PM
> > > To: lng-odp@lists.linaro.org
> > > Subject: [lng-odp] [PATCH API-NEXT v2 0/3] api: ones complement
> metadata
> > >
> > > Added packet metadata for ones complement sum over IP
> > > payload in a packet. Some NICs calculate the sum
> > > during packet input (at least for IP fragments) and
> > > store the value into the packet descriptor. This offloads
> > > L4 checksum calculation for IP fragments as SW does not
> > > need sum all payload data, but just combine pre-calculated
> > > sums from packet descriptors and remove extra header fields
> > > from the sum.

Re: [lng-odp] API-next branch

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

Hi,

It's not so trivial to draw a line what is a small change. E.g. one word change 
in documentation may be a big change for an application: "ODP maintains packet 
order during operation X" vs "... does not maintain order ..."

So, it's much simpler if all API changes go through the same branch, and that 
branch is released often.

One of the largest motivation to change to github was to enable easy (and fast) 
release cycle, since all commits are always check automatically in Travis. So, 
there should be now less work to make a release. 

Tomorrow is the last Friday of the month and a perfect day for making a release 
(1.16) of everything that is ready in api-next... Maybe it's only couple of new 
APIs and some typo corrections, but it still makes the delta that much smaller.

-Petri


From: Maxim Uvarov [mailto:maxim.uva...@linaro.org] 
Sent: Thursday, October 26, 2017 10:30 AM
To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@nokia.com>
Cc: Bill Fischofer <bill.fischo...@linaro.org>; Dmitry Eremin-Solenikov 
<dmitry.ereminsoleni...@linaro.org>; lng-odp-forward <lng-odp@lists.linaro.org>
Subject: Re: [lng-odp] API-next branch

1. we have api-next branch to collect all api changes with implementation in 
tests in one places. A lot of people said that it's useful.
2. Yes it's hard to find patches which are in api-next and not in master.
3.  New API acceptance period is not very clean.  If we can improve that than 
probably we will resolve merge issue.

I.e. my proposal is  - if feature is complete (api, impl, tests) and  accepted 
(members can implement it)  all patches for this feature should be moved to 
master branch. No need to wait for release date.
From other point there is no need for small api changes (like debug prints, 
pktio capabilities, small improvements) stage in api-next. They can go directly 
to master.
The other deal is complex things which need several months for review.  This 
should go to staging branch api-next .

To help with back port we can use github issues. Where one entry will be one 
feature. And place in the comments all commit id's related to this feature. 
It's also possible to do some automation for that.

How about that?

Maxim.


On 26 October 2017 at 09:57, Savolainen, Petri (Nokia - FI/Espoo) 
<mailto:petri.savolai...@nokia.com> wrote:
We need one branch that holds all new API changes before those are released (> 
4 months currently): either it's master or api-next. If we change API 
development to master, then applications would need follow the latest release 
branch instead of master. At least previous there was resistance to change 
master into API development branch.

Separate topic branches for API changes won't scale ... we would not have a 
common view (e.g. for 4 months) what the API is before all those are merged 
together. Implementation changes are easier in topic branches as there's less 
dependency to other files and applications (other files and apps already agree 
what the API is).

Api-next does not cause the problem (big delta), it's caused by the lack of 
steady release cycle. The big  delta won't go away before we have a short 
enough release cycle (merge often => small delta).

-Petri



> -Original Message-
> From: lng-odp [mailto:mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of 
> Bill
> Fischofer
> Sent: Wednesday, October 25, 2017 6:53 PM
> To: Dmitry Eremin-Solenikov <mailto:dmitry.ereminsoleni...@linaro.org>
> Cc: lng-odp-forward <mailto:lng-odp@lists.linaro.org>
> Subject: Re: [lng-odp] API-next branch
>
> I'm all for using topic branches, especially since we've switched to
> GitHub
> and most contributors are now familiar with it and using pull requests
> rather than raw patches sent to the mailing list. The whole reason for
> api-next was to separate in-progress API changes from regular maintenance
> patches since they were all mixed together on the mailing list. The PR
> structure is much cleaner in that regard.
>
> IPsec in particular clearly could have been a separate branch. We've
> talked
> about doing this in the context of 2.0 as well since that's also going to
> involve some major subsystems that could benefit from collaborative
> development before being merged back into the 2.0 main development branch.
>
>
> On Wed, Oct 25, 2017 at 10:39 AM, Dmitry Eremin-Solenikov <
> mailto:dmitry.ereminsoleni...@linaro.org> wrote:
>
> > Hello,
> >
> > I tried to actually check, which patches are sitting in the api-next.
> > And actually I failed
> > to do that in a timely manner. git cherry produces a list of patches,
> > that contains a lot of patches, which already landed to the master.
> >
> > Quick proposal would be to stop using api-next as a long-lived branch
> > which received updates from master and rathe

Re: [lng-odp] [PATCH API-NEXT v3 0/8] api: pool subparameters

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

Ping.

> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of
> Github ODP bot
> Sent: Monday, October 23, 2017 3:00 PM
> To: lng-odp@lists.linaro.org
> Subject: [lng-odp] [PATCH API-NEXT v3 0/8] api: pool subparameters
> 
> Added packet pool parameters for more fine grained pool
> configuration. The basic usage of the parameters is not changed,
> except that implementation may now round up 'num' by default.
> Application can limit the round up with new 'max_num' parameter.
> If application does not limit max_num (but is still interested on
> max_num value), it can check the new info.pkt.max_num.
> The new specification allows implementation to use sub-pools,
> also application is able to request e.g. this kind of pool:

Re: [lng-odp] API-next branch

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

We need one branch that holds all new API changes before those are released (> 
4 months currently): either it's master or api-next. If we change API 
development to master, then applications would need follow the latest release 
branch instead of master. At least previous there was resistance to change 
master into API development branch.

Separate topic branches for API changes won't scale ... we would not have a 
common view (e.g. for 4 months) what the API is before all those are merged 
together. Implementation changes are easier in topic branches as there's less 
dependency to other files and applications (other files and apps already agree 
what the API is).

Api-next does not cause the problem (big delta), it's caused by the lack of 
steady release cycle. The big  delta won't go away before we have a short 
enough release cycle (merge often => small delta).

-Petri



> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of Bill
> Fischofer
> Sent: Wednesday, October 25, 2017 6:53 PM
> To: Dmitry Eremin-Solenikov 
> Cc: lng-odp-forward 
> Subject: Re: [lng-odp] API-next branch
> 
> I'm all for using topic branches, especially since we've switched to
> GitHub
> and most contributors are now familiar with it and using pull requests
> rather than raw patches sent to the mailing list. The whole reason for
> api-next was to separate in-progress API changes from regular maintenance
> patches since they were all mixed together on the mailing list. The PR
> structure is much cleaner in that regard.
> 
> IPsec in particular clearly could have been a separate branch. We've
> talked
> about doing this in the context of 2.0 as well since that's also going to
> involve some major subsystems that could benefit from collaborative
> development before being merged back into the 2.0 main development branch.
> 
> 
> On Wed, Oct 25, 2017 at 10:39 AM, Dmitry Eremin-Solenikov <
> dmitry.ereminsoleni...@linaro.org> wrote:
> 
> > Hello,
> >
> > I tried to actually check, which patches are sitting in the api-next.
> > And actually I failed
> > to do that in a timely manner. git cherry produces a list of patches,
> > that contains a lot of patches, which already landed to the master.
> >
> > Quick proposal would be to stop using api-next as a long-lived branch
> > which received updates from master and rather use it as a branch being
> > regularly rebased on top of current master.
> >
> > Another possiblity would be to abandon api-next completely, develop
> > features on topic branches, which get merged to master, rather than to
> > api-next. At least this would save us from situations, when there is
> > API definition (or change), but no actual implementation behind.
> >
> > --
> > With best wishes
> > Dmitry
> >

Re: [lng-odp] [Linaro/odp] [PATCH API-NEXT v1] api: packet: print data (#258)

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

that has to be odp helper, not need for that in odp api
—

Helper is not API, in general all applications cannot rely that. Out 
validation/example/etc apps can rely on helpers, but not all. This solves a 
very common debug need for data plane application - print a received packet on 
the screen.

-Petri

Re: [lng-odp] [Linaro/odp] [PATCH API-NEXT v1] api: packet: print data (#258)

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

Hi Petri there's no check on snprintf regarding the returned len. Is there any 
possibility in this code that the final buffer will be bigger than max_len?
—


I calculate max string len from number of bytes to print:

int num_rows = (byte_len + bytes_per_row - 1) / bytes_per_row;
int max_len = 256 + (3 * byte_len) + (3 * num_rows);


As long as the math is right the buffer size is larger than the string. The 
same snprintf code is used in the current odp_packet_print().

-Petri

Re: [lng-odp] [Linaro/odp] [PATCH API-NEXT v1] api: pktio: clarify odp_pktio_config() restrictions (#247)

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

> -Original Message-
> From: Bogdan Pricope [mailto:bogdan.pric...@linaro.org]
> Sent: Tuesday, October 24, 2017 11:26 AM
> To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@nokia.com>
> Cc: lng-odp@lists.linaro.org
> Subject: Re: [lng-odp] [Linaro/odp] [PATCH API-NEXT v1] api: pktio:
> clarify odp_pktio_config() restrictions (#247)
> 
> My opinion is that "undefined behavior" makes sense on the fast
> processing path (to limit checks, etc.) and makes no sense on slow
> path, especially on configuration API.
> 

It adds specification/implementation/validation work when some params are OK to 
be bad. E.g. if spec says that it's OK to pass random chksum flags to pktio 
config, we'd need to add a validation test that first checks capabilities and 
then on purpose breaks those and checks that a proper error value is returned. 
That's the only way to make sure that such "bad params passing application" 
would be portable also.

It's easier to list good params and demand that all applications use those. 
Undefined behavior includes bad params checking and error return, it just does 
not guarantee that params check is *always* done.

-Petri



> On 24 October 2017 at 11:17, Savolainen, Petri (Nokia - FI/Espoo)
> <petri.savolai...@nokia.com> wrote:
> >
> >
> > From: Bill Fischofer [mailto:notificati...@github.com]
> > Sent: Monday, October 23, 2017 5:55 PM
> > To: Linaro/odp <o...@noreply.github.com>
> > Cc: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@nokia.com>;
> Mention <ment...@noreply.github.com>
> > Subject: Re: [Linaro/odp] [PATCH API-NEXT v1] api: pktio: clarify
> odp_pktio_config() restrictions (#247)
> >
> > On Mon, Oct 23, 2017 at 8:00 AM, muvarov <notificati...@github.com>
> wrote:
> >
> >> From: "Savolainen, Petri (Nokia - FI/Espoo)"
> >>
> >>
> >> > -Original Message-
> >> > From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of
> >> > Github ODP bot
> >> > Sent: Friday, October 20, 2017 5:00 PM
> >> > To: lng-odp@lists.linaro.org
> >> > Subject: [lng-odp] [PATCH API-NEXT v1 1/1] api: pktio: clarify
> >> > odp_pktio_config() restrictions
> >> >
> >> > From: Bill Fischofer
> >> >
> >> > Add clarification that odp_pktio_config() cannot be used to
> >> > configure options that are not supported, as indicated by
> >> > odp_pktio_capability().
> >> >
> >> > Signed-off-by: Bill Fischofer
> >> > ---
> >> > /** Email created from pull request 247 (Bill-Fischofer-Linaro:pktio-
> >> > config-doc)
> >> > ** https://github.com/Linaro/odp/pull/247
> >> > ** Patch: https://github.com/Linaro/odp/pull/247.patch
> >> > ** Base sha: e3108af2f0b58c2ceca422b418439bba5de04b11
> >> > ** Merge commit sha: fc118350c5aa950e860cd53a6104b0571aa2e59b
> >> > **/
> >> > include/odp/api/spec/packet_io.h | 3 ++-
> >> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/include/odp/api/spec/packet_io.h
> >> > b/include/odp/api/spec/packet_io.h
> >> > index 52af646a6..48dd76f28 100644
> >> > --- a/include/odp/api/spec/packet_io.h
> >> > +++ b/include/odp/api/spec/packet_io.h
> >> > @@ -605,7 +605,8 @@ unsigned odp_pktio_max_index(void);
> >> > * interface setup sequence. Use odp_pktio_capability() to query
> >> > configuration
> >> > * capabilities. Use odp_pktio_config_init() to initialize
> >> > * configuration options into their default values. Default values are
> >> > used
> >> > - * when 'config' pointer is NULL.
> >> > + * when 'config' pointer is NULL. Attempting to configure options
> that
> >> > are
> >> > + * not supported, as indicated by odp_pktio_capability(), will be
> >> > rejected.
> >>
> >> Why not leave this as undefined behavior (as is usual for spec
> violations)? This addition would say that it's OK to violate the
> capability spec, since params are checked anyway (e.g. not use capability
> but iterate through param values until success).
> >>
> >> So are you saying the text should be left as is or that you'd prefer
> the
> > clarification say explicitly that behavior is undefined in this case?
> >
> > It can be left as is, since we should not repeat on every call that
> "breaking this spec results undefined behavior". That statement (or entire
> chapter) c

Re: [lng-odp] [Linaro/odp] [PATCH API-NEXT v1] api: pktio: clarify odp_pktio_config() restrictions (#247)

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

From: Bill Fischofer [mailto:notificati...@github.com] 
Sent: Monday, October 23, 2017 5:55 PM
To: Linaro/odp <o...@noreply.github.com>
Cc: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@nokia.com>; Mention 
<ment...@noreply.github.com>
Subject: Re: [Linaro/odp] [PATCH API-NEXT v1] api: pktio: clarify 
odp_pktio_config() restrictions (#247)

On Mon, Oct 23, 2017 at 8:00 AM, muvarov <notificati...@github.com> wrote:

> From: "Savolainen, Petri (Nokia - FI/Espoo)"
>
>
> > -Original Message-
> > From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of
> > Github ODP bot
> > Sent: Friday, October 20, 2017 5:00 PM
> > To: lng-odp@lists.linaro.org
> > Subject: [lng-odp] [PATCH API-NEXT v1 1/1] api: pktio: clarify
> > odp_pktio_config() restrictions
> >
> > From: Bill Fischofer
> >
> > Add clarification that odp_pktio_config() cannot be used to
> > configure options that are not supported, as indicated by
> > odp_pktio_capability().
> >
> > Signed-off-by: Bill Fischofer
> > ---
> > /** Email created from pull request 247 (Bill-Fischofer-Linaro:pktio-
> > config-doc)
> > ** https://github.com/Linaro/odp/pull/247
> > ** Patch: https://github.com/Linaro/odp/pull/247.patch
> > ** Base sha: e3108af2f0b58c2ceca422b418439bba5de04b11
> > ** Merge commit sha: fc118350c5aa950e860cd53a6104b0571aa2e59b
> > **/
> > include/odp/api/spec/packet_io.h | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/odp/api/spec/packet_io.h
> > b/include/odp/api/spec/packet_io.h
> > index 52af646a6..48dd76f28 100644
> > --- a/include/odp/api/spec/packet_io.h
> > +++ b/include/odp/api/spec/packet_io.h
> > @@ -605,7 +605,8 @@ unsigned odp_pktio_max_index(void);
> > * interface setup sequence. Use odp_pktio_capability() to query
> > configuration
> > * capabilities. Use odp_pktio_config_init() to initialize
> > * configuration options into their default values. Default values are
> > used
> > - * when 'config' pointer is NULL.
> > + * when 'config' pointer is NULL. Attempting to configure options that
> > are
> > + * not supported, as indicated by odp_pktio_capability(), will be
> > rejected.
>
> Why not leave this as undefined behavior (as is usual for spec violations)? 
> This addition would say that it's OK to violate the capability spec, since 
> params are checked anyway (e.g. not use capability but iterate through param 
> values until success).
>
> So are you saying the text should be left as is or that you'd prefer the
clarification say explicitly that behavior is undefined in this case?

It can be left as is, since we should not repeat on every call that "breaking 
this spec results undefined behavior". That statement (or entire chapter) could 
be added to beginning of user guide/ API main page 
(odp/doc/application-api-guide/output/html/index.html).


Something like:
API specification principles

Both application and implementation must comply with the API specification. If 
not otherwise documented, results are undefined if application acts against the 
specification. For example, if an application passes bad parameters to a 
function one implementation may report an error, while another would not check 
those (to maximize performance) but would just crash while using the bad 
values...


So, we'd document those rare cases when API spec guarantees that it's OK to 
pass bad parameters. By default, it would not be OK and thus application should 
check capabilities and limit it's requests based on those.


-Petri

Re: [lng-odp] [PATCH API-NEXT v1 1/1] api: pktio: clarify odp_pktio_config() restrictions

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

> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of
> Github ODP bot
> Sent: Friday, October 20, 2017 5:00 PM
> To: lng-odp@lists.linaro.org
> Subject: [lng-odp] [PATCH API-NEXT v1 1/1] api: pktio: clarify
> odp_pktio_config() restrictions
> 
> From: Bill Fischofer 
> 
> Add clarification that odp_pktio_config() cannot be used to
> configure options that are not supported, as indicated by
> odp_pktio_capability().
> 
> Signed-off-by: Bill Fischofer 
> ---
> /** Email created from pull request 247 (Bill-Fischofer-Linaro:pktio-
> config-doc)
>  ** https://github.com/Linaro/odp/pull/247
>  ** Patch: https://github.com/Linaro/odp/pull/247.patch
>  ** Base sha: e3108af2f0b58c2ceca422b418439bba5de04b11
>  ** Merge commit sha: fc118350c5aa950e860cd53a6104b0571aa2e59b
>  **/
>  include/odp/api/spec/packet_io.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/odp/api/spec/packet_io.h
> b/include/odp/api/spec/packet_io.h
> index 52af646a6..48dd76f28 100644
> --- a/include/odp/api/spec/packet_io.h
> +++ b/include/odp/api/spec/packet_io.h
> @@ -605,7 +605,8 @@ unsigned odp_pktio_max_index(void);
>   * interface setup sequence. Use odp_pktio_capability() to query
> configuration
>   * capabilities. Use odp_pktio_config_init() to initialize
>   * configuration options into their default values. Default values are
> used
> - * when 'config' pointer is NULL.
> + * when 'config' pointer is NULL. Attempting to configure options that
> are
> + * not supported, as indicated by odp_pktio_capability(), will be
> rejected.

Why not leave this as undefined behavior (as is usual for spec violations)? 
This addition would say that it's OK to violate the capability spec, since 
params are checked anyway (e.g. not use capability but iterate through param 
values until success).

-Petri

Re: [lng-odp] [Linaro/odp] [PATCH API-NEXT v1] api: pool subparameters (#234)

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

> -Original Message-
> From: Bill Fischofer [mailto:bill.fischo...@linaro.org]
> Sent: Tuesday, October 17, 2017 2:59 PM
> To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@nokia.com>
> Cc: lng-odp@lists.linaro.org
> Subject: Re: [lng-odp] [Linaro/odp] [PATCH API-NEXT v1] api: pool
> subparameters (#234)
> 
> On Tue, Oct 17, 2017 at 3:04 AM, Savolainen, Petri (Nokia - FI/Espoo)
> <petri.savolai...@nokia.com> wrote:
> >>  typedef struct odp_pool_param_t {
> >> /** Pool type */
> >> @@ -192,17 +193,34 @@ typedef struct odp_pool_param_t {
> >>
> >> /** Parameters for packet pools */
> >> struct {
> >> -   /** The number of packets that the pool must
> provide
> >> -   that are packet length 'len' bytes or
> smaller.
> >> -   The maximum value is defined by pool
> capability
> >> -   pkt.max_num. */
> >> +   /** Minimum number of 'len' byte packets.
> >> +*
> >> +*  The pool must contain at least this many
> packets
> >> +*  that are 'len' bytes or smaller. An
> implementation
> >> +*  may round up the value, as long as the
> 'max_num'
> >> +*  parameter below is not violated. The
> maximum value
> >> +*  for this field is defined by pool
> capability
> >> +*  pkt.max_num.
> >> +*/
> >> uint32_t num;
> >>
> >> -   /** Minimum packet length that the pool must
> provide
> >> -   'num' packets. The number of packets may be
> less
> >> -   than 'num' when packets are larger than
> 'len'.
> >> -   The maximum value is defined by pool
> capability
> >> -   pkt.max_len. Use 0 for default. */
> >> +   /** Maximum number of packets.
> >> +*
> >> +*  This is the maximum number of packets of
> any length
> >> +*  that can be allocated from the pool. The
> maximum
> >> +*  value is defined by pool capability
> pkt.max_num.
> >> +*  Use 0 when there's no requirement for the
> maximum
> >> +*  number of packets. The default value is 0.
> >> +*/
> >> +   uint32_t max_num;
> >
> > I'd put max_num first so that num and len are adjacent parameters for
> > consistency with how the odp_pool_pkt_subparam_t is organized.
> >
> >
> > The logic is that num and max_num are close together, so are len and
> max_len (which does not show here, but follows len below).
> 
> Then putting max_num and max_len together first would make sense since
> they are the primary controls on the configuration. The individual num
> / len pairs that follow are the optimization advisory information.


Num and len are the primary controls. Max_num and max_len are optional (default 
== 0).

Now struct order is this ...

init();
p.pkt.num = 100;
p.pkt.max_num = 1000;
p.pkt.len = 1500;
p.pkt.max_len = 9000;

... which is logical to me. If application does not define maximums, it's just:

init();
p.pkt.num = 100;
p.pkt.len = 1500;


-Petri

Re: [lng-odp] [Linaro/odp] [PATCH API-NEXT v1] api: pool subparameters (#234)

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

>  typedef struct odp_pool_param_t {
> /** Pool type */
> @@ -192,17 +193,34 @@ typedef struct odp_pool_param_t {
>
> /** Parameters for packet pools */
> struct {
> -   /** The number of packets that the pool must provide
> -   that are packet length 'len' bytes or smaller.
> -   The maximum value is defined by pool capability
> -   pkt.max_num. */
> +   /** Minimum number of 'len' byte packets.
> +*
> +*  The pool must contain at least this many packets
> +*  that are 'len' bytes or smaller. An implementation
> +*  may round up the value, as long as the 'max_num'
> +*  parameter below is not violated. The maximum value
> +*  for this field is defined by pool capability
> +*  pkt.max_num.
> +*/
> uint32_t num;
>
> -   /** Minimum packet length that the pool must provide
> -   'num' packets. The number of packets may be less
> -   than 'num' when packets are larger than 'len'.
> -   The maximum value is defined by pool capability
> -   pkt.max_len. Use 0 for default. */
> +   /** Maximum number of packets.
> +*
> +*  This is the maximum number of packets of any 
> length
> +*  that can be allocated from the pool. The maximum
> +*  value is defined by pool capability pkt.max_num.
> +*  Use 0 when there's no requirement for the maximum
> +*  number of packets. The default value is 0.
> +*/
> +   uint32_t max_num;

I'd put max_num first so that num and len are adjacent parameters for
consistency with how the odp_pool_pkt_subparam_t is organized.


The logic is that num and max_num are close together, so are len and max_len 
(which does not show here, but follows len below).

-Petri


> +
> +   /** Minimum length of 'num' packets.
> +*
> +*  The pool must contain at least 'num' packets up to
> +*  this packet length (1 ... 'len' bytes). The 
> maximum
> +*  value for this field is defined by pool capability
> +*  pkt.max_len. Use 0 for default.
> +*/
> uint32_t len;
>
> /** Maximum packet length that will be allocated from
>

Re: [lng-odp] [Linaro/odp] [PATCH API-NEXT v1] api: pool subparameters (#234)

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

From: muvarov [mailto:notificati...@github.com] 
Sent: Tuesday, October 17, 2017 3:01 AM
To: Linaro/odp <o...@noreply.github.com>
Cc: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@nokia.com>; Author 
<aut...@noreply.github.com>
Subject: Re: [Linaro/odp] [PATCH API-NEXT v1] api: pool subparameters (#234)

From: Bill Fischofer 
 On Mon, Oct 16, 2017 at 8:00 AM, Github ODP bot  wrote:
> From: Petri Savolainen 
>
> Remove anonymous union from pool parameter structure.
> Union makes it impossible to initialize parameters per
> pool type (use other values than all zeros). This change
> is not visible to applications (union was anonymous).

The reason for the union was to reduce the size of the
odp_pool_param_t. I'm not sure I understand the remark about making
initializations impossible. Pool parameters are initialized to their
default values via the odp_pool_param_init() API. Failure to use this
API exposes the application to portability issues as different
implementations may have different default values. If the use of
anonymous unions discourages attempts to have static copies of
odp_pool_param_t variables, so much the better.


odp_pool_param_init() does not include type. Union cannot have multiple 
(overlapping) default values:

typedef union {
  // default value is 3 in the API spec
  int a;

  // default value is 7 in the API spec
  int b;

  // default value is 0 in the API spec
  int c;
} foo_t;

foo_t foo;

init_foot(foo_t *foo) {
memset(, 0, sizeof(foo_t));

// Which default we now choose, a or b ?
// Whichever we choose conflicts the spec of the other one.
foo->a = 3;
//foo->b = 7;
}


Struct can be init always correctly, without knowing the type beforehand. This 
is backward compatible as init_foo() prototype  and applications do not change.

typedef struct {
  // default value is 3 in the API spec
  int a;

  // default value is 7 in the API spec
  int b;

  // default value is 0 in the API spec
  int c;
} foo_t;

foo_t foo;

init_foot(foo_t *foo) {
memset(, 0, sizeof
foo->a = 3;
foo->b = 7;
}


-Petri

Re: [lng-odp] [Linaro/odp] [PATCH API-NEXT v1] api: pool subparameters (#234)

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

Received mail through Github ... and in HTML format of course.

From: muvarov [mailto:notificati...@github.com] 
Sent: Tuesday, October 17, 2017 3:01 AM
To: Linaro/odp <o...@noreply.github.com>
Cc: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@nokia.com>; Author 
<aut...@noreply.github.com>
Subject: Re: [Linaro/odp] [PATCH API-NEXT v1] api: pool subparameters (#234)

From: Bill Fischofer 
 On Mon, Oct 16, 2017 at 7:59 AM, Github ODP bot  wrote:
> From: Petri Savolainen 
>
> Additional packet length and number specification gives
> more information to implementation about intended packet
> length distribution in the pool. This enables e.g. correct
> initialization when pool implementation is based on multiple
> fixed packet / segment sizes (subpools). The specification
> does require subpool implementation but allows it.
>
> Signed-off-by: Petri Savolainen 
> ---
> /** Email created from pull request 234 (psavol:next-pool-param)
>  ** https://github.com/Linaro/odp/pull/234
>  ** Patch: https://github.com/Linaro/odp/pull/234.patch
>  ** Base sha: afeda4d14bb6f449cb269680cdbd56b26726eedf
>  ** Merge commit sha: 54f5fc670a7c125b6b0098e34e68fe3b45875069
>  **/
>  include/odp/api/spec/pool.h | 47 
> +
>  1 file changed, 47 insertions(+)
>
> diff --git a/include/odp/api/spec/pool.h b/include/odp/api/spec/pool.h
> index f1c8b1158..7c9bee8ee 100644
> --- a/include/odp/api/spec/pool.h
> +++ b/include/odp/api/spec/pool.h
> @@ -41,6 +41,9 @@ extern "C" {
>   * Maximum pool name length in chars including null char
>   */
>
> +/** Maximum number of packet pool subparameters */
> +#define ODP_POOL_MAX_SUBPARAMS  7
> +
>  /**
>   * Pool capabilities
>   */
> @@ -134,6 +137,12 @@ typedef struct odp_pool_capability_t {
>  * The value of zero means that limited only by the available
>  * memory size for the pool. */
> uint32_t max_uarea_size;
> +
> +   /** Maximum number of subparameters
> +*
> +*  Maximum number of packet pool subparameters. Valid range 
> is
> +*  0 ... ODP_POOL_MAX_SUBPARAMS. */
> +   uint8_t max_num_sub;

max_num_subparam or max_subparam would be clearer. "sub" by itself
seems a bit too short to be intuitive.

max_num_subparam is OK.


> } pkt;
>
> /** Timeout pool capabilities  */
> @@ -163,6 +172,18 @@ typedef struct odp_pool_capability_t {
>  int odp_pool_capability(odp_pool_capability_t *capa);
>
>  /**
> + * Packet pool subparameters
> + */
> +typedef struct odp_pool_pkt_subparam_t {
> +   /** Number of 'len' byte packets. */
> +   uint32_t num;
> +
> +   /** Packet length in bytes */
> +   uint32_t len;
> +
> +} odp_pool_pkt_subparam_t;
> +
> +/**
>   * Pool parameters
>   *
>   * A note for all pool types: a single thread may not be able to allocate all
> @@ -246,6 +267,32 @@ typedef struct odp_pool_param_t {
> capability pkt.max_headroom.
> Use zero if headroom is not needed. */
> uint32_t headroom;
> +
> +   /** Number of subparameters
> +*
> +*  The number of subparameter table entries used.
> +*  The maximum value is defined by pool
> +*  capability pkt.max_num_sub. The default value is 
> 0.
> +*/
> +   uint8_t num_sub;

uint8_t num_subparam would be consistent with the above suggestion.

num_subparam is OK.

> +
> +   /** Subparameter table
> +*
> +*  Subparameters continue pool configuration with
> +*  additional packet length requirements. The first
> +*  table entry follows the num/len specification 
> above.
> +*  So that, sub[0].len > 'len', and sub[0].num refers
> +*  to packet lengths between 'len' + 1 and 
> sub[0].len.
> +*  Similarly, sub[1] follows sub[0] specification, 
> and
> +*  so on.
> +*
> +*  Each requirement is supported separately and may 
> be
> +*  rounded up, as long as the 'max_num' parameter is
> +*  not violated. It's implementation specific if some
> +*  requirements are supported simultaneously (e.g.
> +*  due to subpool design).

I t

Re: [lng-odp] [Linaro/odp] [PATCH API-NEXT v1] api: pool subparameters (#234)

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

From: Bill Fischofer [mailto:notificati...@github.com] 
Sent: Tuesday, October 17, 2017 2:20 AM
To: Linaro/odp <o...@noreply.github.com>
Cc: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@nokia.com>; Author 
<aut...@noreply.github.com>
Subject: Re: [Linaro/odp] [PATCH API-NEXT v1] api: pool subparameters (#234)

@Bill-Fischofer-Linaro requested changes on this pull request.
I've sent review comments by replying to the ODP mailing list, as you've 
requested for your PRs.
—
You are receiving this because you authored the thread.
Reply to this email directly, 
https://github.com/Linaro/odp/pull/234#pullrequestreview-69728725, or 
https://github.com/notifications/unsubscribe-auth/AJJSfnNTyPl_UoaM40WLIomO5fwWPqf4ks5ss-SagaJpZM4P6lMO.



Did you reply to the ODP mailing address directly and/or to the Github bot 
address? I can see your mails (in my inbox) only in Github generated form: all 
with the same subject, coming from muvarov, ...

-Petri

Re: [lng-odp] [PATCH API-NEXT v1 1/1] api: ipsec: return maximum antireplay window size via capability

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

Git log entry is missing. Update with some rationale why this change is needed.

-Petri

> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of
> Github ODP bot
> Sent: Sunday, October 15, 2017 9:00 AM
> To: lng-odp@lists.linaro.org
> Subject: [lng-odp] [PATCH API-NEXT v1 1/1] api: ipsec: return maximum
> antireplay window size via capability
> 
> From: Dmitry Eremin-Solenikov 
> 
> Signed-off-by: Dmitry Eremin-Solenikov 
> ---
> /** Email created from pull request 230 (lumag:ipsec-rws-cap)
>  ** https://github.com/Linaro/odp/pull/230
>  ** Patch: https://github.com/Linaro/odp/pull/230.patch
>  ** Base sha: afeda4d14bb6f449cb269680cdbd56b26726eedf
>  ** Merge commit sha: bb52bccf08a68ec2e1c3988539a0295d79ebd5bd
>  **/
>  include/odp/api/spec/ipsec.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/odp/api/spec/ipsec.h b/include/odp/api/spec/ipsec.h
> index 3bd80b266..ddcdf75bb 100644
> --- a/include/odp/api/spec/ipsec.h
> +++ b/include/odp/api/spec/ipsec.h
> @@ -282,6 +282,9 @@ typedef struct odp_ipsec_capability_t {
>*  be used for many SAs. */
>   uint32_t max_queues;
> 
> + /** Maximum anti-replay window size. */
> + uint32_t max_antireplay_ws;
> +
>   /** Supported cipher algorithms */
>   odp_crypto_cipher_algos_t ciphers;
> 

Re: [lng-odp] Code review and discussion on mailing list only

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

> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of
> Savolainen, Petri (Nokia - FI/Espoo)
> Sent: Monday, October 02, 2017 5:53 PM
> To: lng-odp-forward <lng-odp@lists.linaro.org>
> Subject: Suspected SPAM - [lng-odp] Code review and discussion on mailing
> list only
> 
> Hi,
> 
> Let's stop using Github for code review and design discussions. Mailing
> list is far more flexible and robust for human-to-human interaction than a
> web-based tool. We can continue to use Github for sending/bookkeeping pull
> requests (and run Travis on those), but all discussions should happen on
> the mailing list only.
> 
> 1) Plain text messages on the mailing list enable easy/flexible access
> (various clients, bad network connection, work offline, ...)
> 
> 2) A mail defines context (patch v3 2/7 ...) for a discussion and may
> include all comments / replies in one shot, and thus is fast to read /
> browse through. Whereas, comments on a Github web page are inherently
> scattered / mixed up, and need much more clicking / scrolling through to
> find the information. My experience is that code review on Github is 2-4x
> slower than on mails.
> 
> 3) Code review on Github is slow and has lots of issues
>* Mixes patch order in a patch series (don't see which patch is e.g.
> 1/7, 2/7, ...)
>* Mixes comments from between patches (based on file + line, so a
> comment is shown also with other patches which have the same file + line
> in context)
>* Comments are mixed between different versions of a patch set
>* No way to comment git log entry text
>* Mail <-> web page comment conversions does not produce readable mails
> or web pages. Based on scripts, but never optimal. How much context is the
> right amount? Who maintains the scripts? What if the Github API stop
> supporting things that those scripts use?
>* A long diffs in a patch does not open automatically, but reader need
> to click through those to find out if there are comments inside
>* A reviewer cannot filter out other reviewers comments, just add on
> top. Multiple comment/answer threads get mixed up on the same comment box,
> or new threads needs to be placed on artificially different but close
> together lines.
>* ... etc. End result => lots of clicking on a messy page, instead of
> getting the work done.
> 
> 
> Kernel developers use mailing lists for the same / similar reasons:
> https://lwn.net/Articles/702177/
> 
> 
> -Petri
> 

Ping.

I still think we should move discussion from Github back to the mailing list:
 * Github cannot be used for sane discussion (see above)
 * Mailing list is filled with this kind of nonsense mails:

In include/odp/api/spec/pool.h:
> @@ -290,10 +290,16 @@ odp_pool_t odp_pool_lookup(const char *name);
 /**
  * Pool information struct
  * Used to get information about a pool.
+ * @note The difference between end_addr & start_addr
+ * will result in buffers address range belong to this pool.
OK. agreed


-Petri

Re: [lng-odp] classifier_enable flag

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

odp_pktin_queue_config() creates all queues for the interface, except when 
classifier is used. That's why the classifier config is there. 
odp_pktin_queue_config() is an interface level call (there are no queue level 
config calls).

Parser is not classifier. Parser config tells which metadata application 
expects from packets, classifier (or hash) config tells how packets (flows) are 
directed into queues. odp_pktio_config() maybe always called.  When it's not 
called, default values apply (e.g. parser.layer = ODP_PKTIO_PARSER_LAYER_ALL).

-Petri


> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of
> Honnappa Nagarahalli
> Sent: Monday, October 09, 2017 3:18 AM
> To: lng-odp-forward 
> Subject: Re: [lng-odp] classifier_enable flag
> 
> Also, are classifier and parser mutually exclusive?
> 
> Thank you,
> Honnappa
> 
> On 8 October 2017 at 19:14, Honnappa Nagarahalli
>  wrote:
> > Hi,
> >The 'classifier_enable' flag is a pktin queue parameter. Shouldn't
> > this be a parameter of the pkt I/O device (port)?
> >
> > Does this mean, some queues of the pkt I/O device (port) can have
> > classifier enabled and some hash enabled?
> >
> > Thank you,
> > Honnappa

Re: [lng-odp] generic core + HW specific drivers

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

> > No, I'm pointing that the more there's common core SW, the more there
> > are trade-offs and the less direct HW access == less  performance. For
> > optimal performance, the amount of common core SW is zero.
>
> Yes this is sort of the ideal but I doubt this type of installation
> will be accepted by e.g. Red Hat for inclusion in server-oriented
> Linux distributions. Jon Masters seems to be strongly against this
> (although I have only heard this second hand). So that's why I
> proposed the common (generic) core + platform specific drivers model
> that is used by e.g. Xorg and DPDK. Since DPDK is actually a user
> space framework (unlike Xorg), this should be a good model for ODP and
> something that Red Hat cannot object against.
>

If every line of code is maintained properly, why a distro would care about the 
ratio between common core SW and HW specific driver SW?

If they care, what is an acceptable ratio? Is it 90% common SW : 10% HW 
specific SW, 80:20, 50:50, 10:90 and why not 0:100? How this ratio should be 
calculated?

DPDK is in Ubuntu already. Have anyone calculated what this ratio is for it?

I'd be interested to see ODP as part of any distro first, and only after that 
speculate what other distros may or may not say. E.g. Ubuntu seem to accept  
packages that are only for single arch, e.g.:
librte-pmd-fm10k17.05 (= 17.05.2-0ubuntu1) [amd64, i386]  <<< Intel Red Rock 
Canyon net driver, provided only for x86

-Petri

[lng-odp] Let's remove odp_packet_unshared_len

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

Hi,

I propose that we'd remove the odp_packet_unshared_len() call.

The rationale is:
* Application rarely needs this to be maintained by ODP (never ?)
  * Application may maintain offsets/lengths e.g. in packet user area
  * Application may utilize knowledge about the context:
e.g. in this pipeline stage all UDP payload is multi-referenced, etc
* It's not practical for ODP to maintain accurate byte counts when multiple 
references point into to the same packet (or reference of it) in different 
locations. Accurate book keeping would require that packet_free(ref) would 
update unshared_len on other references as well.

For example, three references to a packet.

 ref1  ref2  ref3
  | | |
  v v v 
  ++
  | |  B  | A  |
  ++

For ref1, unshared_len is "packet_len - (A + B)". When ref2 is freed, 
unshared_len of ref1 should change to "packet_len - A". Additional book keeping 
and synchronization would be needed just for that to happen. A HW based pool 
would not do such update either. Currently, I leave it unchanged until all 
references are freed.

It's easy to imagine more complex examples using references of references, or 
race conditions. E.g. during free of ref2, another thread creates new reference 
between ref1 and ref2, so unshared_len of ref1 should be updated to "packet_len 
- (A + C)", instead of "packet_len - A".


-Petri

Re: [lng-odp] generic core + HW specific drivers

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

No HTML mails, please.


From: Bill Fischofer [mailto:bill.fischo...@linaro.org] 
Sent: Wednesday, October 04, 2017 3:55 PM
To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@nokia.com>
Cc: Andriy Berestovskyy <andriy.berestovs...@caviumnetworks.com>; Ola Liljedahl 
<ola.liljed...@linaro.org>; lng-odp@lists.linaro.org
Subject: Re: [lng-odp] generic core + HW specific drivers



On Wed, Oct 4, 2017 at 7:47 AM, Savolainen, Petri (Nokia - FI/Espoo) 
<mailto:petri.savolai...@nokia.com> wrote:


> -Original Message-
> From: Andriy Berestovskyy 
> [mailto:mailto:andriy.berestovs...@caviumnetworks.com]
> Sent: Tuesday, October 03, 2017 8:22 PM
> To: Savolainen, Petri (Nokia - FI/Espoo) <mailto:petri.savolai...@nokia.com>; 
> Ola
> Liljedahl <mailto:ola.liljed...@linaro.org>; mailto:lng-odp@lists.linaro.org
> Subject: Re: [lng-odp] generic core + HW specific drivers
>
> Hey,
> Please see below.
>
> On 03.10.2017 10:12, Savolainen, Petri (Nokia - FI/Espoo) wrote:
> > So, we should be able to deliver ODP as a set of HW independent and
> > HW specific packages (libraries). For example, minimal install would
> >  include only odp, odp-linux and odp-test-suite, but when on arm64
> > (and especially when on ThunderX) odp-thunderx would be installed
>
> There are architecture dependencies (i.e. i386, amd64, arm64 etc), but
> there are no specific platform dependencies (i.e. Cavium ThunderX,
> Cavium Octeon, NXP etc).
>
> In other words, there is no such mechanism in packaging to create a
> package odp, which will automatically install package odp-thunderx only
> on Cavium ThunderX platforms.

I'd expect that ODP main package (e.g. for arm64) would run a script (written 
by us) during install which digs out information about the system and sets up 
ODP paths accordingly. E.g. libs/headers from odp-thunderx package would added 
to search paths when installing into a ThunderX system. If system is not 
recognized,  ODP libs/header paths would point into odp-linux.

That's still trying to make this a static configuration that can be done at 
install time. What about VMs/containers that are instantiated on different 
hosts as they are deployed? This really needs to be determined at run time, not 
install time. 



Also with a VM, all arm64 ODP packages would be present, and the problem to 
solve would be which implementation to use (to link against). If a run time 
code can probe the host system (e.g. are we on ThunderX), so does a script. An 
ignorant user might not run additional scripts and thus be left with the 
default setup (odp-linux). A more aware user would run an additional script 
before launching/building any ODP apps. This script would notice that we have 
e.g. ThunderX HW and would change ODP paths to point into odp-thunderx 
libs/headers. The HW discovery could be as simple as cloud administrator 
updating VM bootparams with SoC model information.


>
> All other projects you are mentioning (kernel, DPDK, Xorg) use
> architecture dependency (different packages for different architectures)
> combined with run time configuration/probing. A kernel driver might be
> installed, but it will be unused until configured/probed.

Those projects aim to maximize code re-use of the core part and minimize size 
of the driver part. Optimally, we'd do the opposite - minimize the core part to 
zero and dynamically link application directly to the right "driver" (== HW 
specific odp implementation).

If there's no core part, run time probing is not needed - install time probing 
and lib/header path setup is enough.

You're describing the embedded build case, which is similar to what we have 
today with --enable-abi-compat=no. That's not changing. We're only talking 
about what happens for --enable-abi-compat=yes builds.
 


No, I'm pointing that the more there's common core SW, the more there are 
trade-offs and the less direct HW access == less  performance. For optimal 
performance, the amount of common core SW is zero.

You may very well want to build and run non-ABI compat applications also in a 
VM. Non-ABI compat just means that you are not going to run *the same 
application image* with another implementation/system. You may still build 
locally on the VM, or have bunch of (non-ABI compat) application images 
pre-built - one per implementation/system.


>
> To support multiple platforms, runtime configuration/probing is
> inevitable. The real question is who does this probing: ODP itself or
> each application independently. To avoid code duplication, ODP looks
> like a better choice...

Install time probe/conf would be the best choice. The second best would be a 
dummy "core ODP" implementation which would be just a giant function pointer 
array (redirect every ODP API call to its implementation in a HW specific lib).

That's effect

Re: [lng-odp] generic core + HW specific drivers

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

> -Original Message-
> From: Andriy Berestovskyy [mailto:andriy.berestovs...@caviumnetworks.com]
> Sent: Tuesday, October 03, 2017 8:22 PM
> To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@nokia.com>; Ola
> Liljedahl <ola.liljed...@linaro.org>; lng-odp@lists.linaro.org
> Subject: Re: [lng-odp] generic core + HW specific drivers
> 
> Hey,
> Please see below.
> 
> On 03.10.2017 10:12, Savolainen, Petri (Nokia - FI/Espoo) wrote:
> > So, we should be able to deliver ODP as a set of HW independent and
> > HW specific packages (libraries). For example, minimal install would
> >  include only odp, odp-linux and odp-test-suite, but when on arm64
> > (and especially when on ThunderX) odp-thunderx would be installed
> 
> There are architecture dependencies (i.e. i386, amd64, arm64 etc), but
> there are no specific platform dependencies (i.e. Cavium ThunderX,
> Cavium Octeon, NXP etc).
> 
> In other words, there is no such mechanism in packaging to create a
> package odp, which will automatically install package odp-thunderx only
> on Cavium ThunderX platforms.

I'd expect that ODP main package (e.g. for arm64) would run a script (written 
by us) during install which digs out information about the system and sets up 
ODP paths accordingly. E.g. libs/headers from odp-thunderx package would added 
to search paths when installing into a ThunderX system. If system is not 
recognized,  ODP libs/header paths would point into odp-linux.


> 
> 
> > Package:
> > * odp * depends on: odp-linux
> > * odp-linux * depends on: odp
>  > * odp-thunderx [arm64] * depends on: odp
> 
> So installing odp-thunderx we will end up with odp, odp-linux and
> odp-thunderx, so still we have switch runtime between odp-linux and
> odp-thunderx...

I hope it's a matter of probing and installing paths on install time, instead 
of runtime. It's hard to believe that ODP would be the first package ever to 
choose and install a library from a set of alternative libraries.

> 
> 
> All other projects you are mentioning (kernel, DPDK, Xorg) use
> architecture dependency (different packages for different architectures)
> combined with run time configuration/probing. A kernel driver might be
> installed, but it will be unused until configured/probed.

Those projects aim to maximize code re-use of the core part and minimize size 
of the driver part. Optimally, we'd do the opposite - minimize the core part to 
zero and dynamically link application directly to the right "driver" (== HW 
specific odp implementation).

If there's no core part, run time probing is not needed - install time probing 
and lib/header path setup is enough.


> 
> To support multiple platforms, runtime configuration/probing is
> inevitable. The real question is who does this probing: ODP itself or
> each application independently. To avoid code duplication, ODP looks
> like a better choice...

Install time probe/conf would be the best choice. The second best would be a 
dummy "core ODP" implementation which would be just a giant function pointer 
array (redirect every ODP API call to its implementation in a HW specific lib). 

-Petri

Re: [lng-odp] Code review and discussion on mailing list only

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

From: Maxim Uvarov [mailto:maxim.uva...@linaro.org] 
Sent: Tuesday, October 03, 2017 12:50 PM
To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@nokia.com>
Cc: lng-odp-forward <lng-odp@lists.linaro.org>
Subject: Re: [lng-odp] Code review and discussion on mailing list only



On 2 October 2017 at 17:53, Savolainen, Petri (Nokia - FI/Espoo) 
<mailto:petri.savolai...@nokia.com> wrote:
Hi,

Let's stop using Github for code review and design discussions. Mailing list is 
far more flexible and robust for human-to-human interaction than a web-based 
tool. We can continue to use Github for sending/bookkeeping pull requests (and 
run Travis on those), but all discussions should happen on the mailing list 
only.

1) Plain text messages on the mailing list enable easy/flexible access (various 
clients, bad network connection, work offline, ...)

2) A mail defines context (patch v3 2/7 ...) for a discussion and may include 
all comments / replies in one shot, and thus is fast to read / browse through. 
Whereas, comments on a Github web page are inherently scattered / mixed up, and 
need much more clicking / scrolling through to find the information. My 
experience is that code review on Github is 2-4x slower than on mails.

3) Code review on Github is slow and has lots of issues
   * Mixes patch order in a patch series (don't see which patch is e.g. 1/7, 
2/7, ...)

trying to find reodering of patches and looks like order is correct:

For example link:
https://github.com/Linaro/odp/pull/196/commits
Patchwork:
http://patches.opendataplane.org/patch/10834/
http://patches.opendataplane.org/project/lng-odp/list/?series=613

0 0 0
2017-09-26
http://patches.opendataplane.org/project/lng-odp/list/?submitter=147

New
http://patches.opendataplane.org/patch/10833/
http://patches.opendataplane.org/project/lng-odp/list/?series=613

0 0 0
2017-09-26
http://patches.opendataplane.org/project/lng-odp/list/?submitter=147

New
http://patches.opendataplane.org/patch/10832/
http://patches.opendataplane.org/project/lng-odp/list/?series=613

0 0 0
2017-09-26
http://patches.opendataplane.org/project/lng-odp/list/?submitter=147

New
http://patches.opendataplane.org/patch/10831/
http://patches.opendataplane.org/project/lng-odp/list/?series=613

0 0 0
2017-09-26
http://patches.opendataplane.org/project/lng-odp/list/?submitter=147

New
http://patches.opendataplane.org/patch/10830/
http://patches.opendataplane.org/project/lng-odp/list/?series=613

0 0 0
2017-09-26
http://patches.opendataplane.org/project/lng-odp/list/?submitter=147

New
http://patches.opendataplane.org/patch/10829/
http://patches.opendataplane.org/project/lng-odp/list/?series=613

0 0 0
2017-09-26
http://patches.opendataplane.org/project/lng-odp/list/?submitter=147

New
http://patches.opendataplane.org/patch/10828/
http://patches.opendataplane.org/project/lng-odp/list/?series=613

0 0 0
2017-09-26
http://patches.opendataplane.org/project/lng-odp/list/?submitter=147

New
http://patches.opendataplane.org/patch/10827/
http://patches.opendataplane.org/project/lng-odp/list/?series=613

0 0 0
2017-09-26
http://patches.opendataplane.org/project/lng-odp/list/?submitter=147

New
http://patches.opendataplane.org/patch/10826/
http://patches.opendataplane.org/project/lng-odp/list/?series=613

0 0 0
2017-09-26
http://patches.opendataplane.org/project/lng-odp/list/?submitter=147

New

 
Petri, can you point where order is not consistent?

My packet references PR (https://github.com/Linaro/odp/pull/170/commits) used 
to show commits in wrong order. There were 3-4 commits in wrong places. I 
didn't figure out which order Github used for those, the order was not based on 
commits, dates or alphabets ...

Now, those are listed in order. Maybe the paged updated during PR was merged 
and closed. I think labels like "Commits on Sep 12, 2017" were not there on the 
PR page before.

Some version of one of Bala's patch sets had the same issue. It was frustrating 
to review the set when commits were not in order.

These pages are created on the fly, so it might have been a bug fixed or still 
hiding there for the next occurrence...

-Petri

Re: [lng-odp] [PATCH v1 1/1] linux-gen: User /proc/cpuinfo, if sysfs is not available. Fixes https://bugs.linaro.org/show_bug.cgi?id=3249

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

> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of
> Github ODP bot
> Sent: Tuesday, October 03, 2017 11:00 AM
> To: lng-odp@lists.linaro.org
> Subject: [lng-odp] [PATCH v1 1/1] linux-gen: User /proc/cpuinfo, if sysfs
> is not available. Fixes https://bugs.linaro.org/show_bug.cgi?id=3249
> 
> From: Ilias Apalodimas 


Git log entry is missing. Re-format commit subject line and body of the text.

For example:
"
linux-gen: system_info: use proc/cpuinfo as backup

When sysfs is not available, use /proc/cpuinfo file as
a backup for current cpu MHz.

This fixes  bug: https://bugs.linaro.org/show_bug.cgi?id=3249

Signed-off-by...
"

-Petri

Re: [lng-odp] generic core + HW specific drivers

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

> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of Ola
> Liljedahl
> Sent: Friday, September 29, 2017 8:47 PM
> To: lng-odp@lists.linaro.org
> Subject: [lng-odp] generic core + HW specific drivers
> 
> olli@vubuntu:~$ dpkg --get-selections | grep xorg
> xorg install
> xorg-docs-core install
> xserver-xorg install
> xserver-xorg-core install
> xserver-xorg-input-all install
> xserver-xorg-input-evdev install
> xserver-xorg-input-libinput install
> xserver-xorg-input-synaptics install
> xserver-xorg-input-wacom install
> xserver-xorg-video-all install
> xserver-xorg-video-amdgpu install
> xserver-xorg-video-ati install
> xserver-xorg-video-fbdev install
> xserver-xorg-video-intel install
> xserver-xorg-video-mach64 install
> xserver-xorg-video-neomagic install
> xserver-xorg-video-nouveau install<< xserver-xorg-video-openchrome install
> xserver-xorg-video-qxl install
> xserver-xorg-video-r128 install
> xserver-xorg-video-radeon install .   << xserver-xorg-video-savage install
> xserver-xorg-video-siliconmotion install
> xserver-xorg-video-sisusb install
> xserver-xorg-video-tdfx install
> xserver-xorg-video-trident install
> xserver-xorg-video-vesa install
> xserver-xorg-video-vmware install .   << 
> So let's rename ODP Cloud to ODP Core.
> 
> -- Ola


DPDK packages in Ubuntu 17.05 (https://packages.ubuntu.com/artful/dpdk) include 
many HW dependent packages 

...
librte-pmd-fm10k17.05 (= 17.05.2-0ubuntu1) [amd64, i386]  <<< Intel Red Rock 
Canyon net driver, provided only for x86
librte-pmd-i40e17.05 (= 17.05.2-0ubuntu1)
librte-pmd-ixgbe17.05 (= 17.05.2-0ubuntu1) [not ppc64el]
librte-pmd-kni17.05 (= 17.05.2-0ubuntu1) [not i386]
librte-pmd-lio17.05 (= 17.05.2-0ubuntu1)
librte-pmd-nfp17.05 (= 17.05.2-0ubuntu1)
librte-pmd-null-crypto17.05 (= 17.05.2-0ubuntu1)
librte-pmd-null17.05 (= 17.05.2-0ubuntu1)
librte-pmd-octeontx-ssovf17.05 (= 17.05.2-0ubuntu1)   <<< OcteonTX SSO eventdev 
driver files as a package
librte-pmd-pcap17.05 (= 17.05.2-0ubuntu1)
librte-pmd-qede17.05 (= 17.05.2-0ubuntu1)
librte-pmd-ring17.05 (= 17.05.2-0ubuntu1)
librte-pmd-sfc-efx17.05 (= 17.05.2-0ubuntu1) [amd64]
librte-pmd-skeleton-event17.05 (= 17.05.2-0ubuntu1)
librte-pmd-sw-event17.05 (= 17.05.2-0ubuntu1)
librte-pmd-tap17.05 (= 17.05.2-0ubuntu1)
librte-pmd-thunderx-nicvf17.05 (= 17.05.2-0ubuntu1)  <<< ThunderX net driver 
files as a package
...


So, we should be able to deliver ODP as a set of HW independent and HW specific 
packages (libraries). For example, minimal install would include only odp, 
odp-linux and odp-test-suite, but when on arm64 (and especially when on 
ThunderX) odp-thunderx would be installed also. The trick would be how to 
select odp-thunderx installed files (also headers) as default when application 
is built or run on ThunderX (and not on another arm64).

Package:
* odp (only generic folders and documentation, no implementation)
  * depends on: odp-linux, odp-test-suite
  * recommends: odp-linux, odp-dpdk, odp-thunderx, odp-dpaa2, ...
* odp-linux
  * depends on: odp, openssl
  * recommends: dpdk, netmap, ...
* odp-dpdk
  * depends on: odp, dpdk
* odp-thunderx [arm64]
  * depends on: odp, ...
* odp-test-suite
  * depends on: odp


-Petri

[lng-odp] Code review and discussion on mailing list only

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

Hi,

Let's stop using Github for code review and design discussions. Mailing list is 
far more flexible and robust for human-to-human interaction than a web-based 
tool. We can continue to use Github for sending/bookkeeping pull requests (and 
run Travis on those), but all discussions should happen on the mailing list 
only.

1) Plain text messages on the mailing list enable easy/flexible access (various 
clients, bad network connection, work offline, ...)

2) A mail defines context (patch v3 2/7 ...) for a discussion and may include 
all comments / replies in one shot, and thus is fast to read / browse through. 
Whereas, comments on a Github web page are inherently scattered / mixed up, and 
need much more clicking / scrolling through to find the information. My 
experience is that code review on Github is 2-4x slower than on mails.

3) Code review on Github is slow and has lots of issues
   * Mixes patch order in a patch series (don't see which patch is e.g. 1/7, 
2/7, ...)
   * Mixes comments from between patches (based on file + line, so a comment is 
shown also with other patches which have the same file + line in context)
   * Comments are mixed between different versions of a patch set
   * No way to comment git log entry text
   * Mail <-> web page comment conversions does not produce readable mails or 
web pages. Based on scripts, but never optimal. How much context is the right 
amount? Who maintains the scripts? What if the Github API stop supporting 
things that those scripts use?
   * A long diffs in a patch does not open automatically, but reader need to 
click through those to find out if there are comments inside
   * A reviewer cannot filter out other reviewers comments, just add on top. 
Multiple comment/answer threads get mixed up on the same comment box, or new 
threads needs to be placed on artificially different but close together lines. 
   * ... etc. End result => lots of clicking on a messy page, instead of 
getting the work done.


Kernel developers use mailing lists for the same / similar reasons: 
https://lwn.net/Articles/702177/


-Petri

Re: [lng-odp] Compiler Barrier API

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

> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of
> Andriy Berestovskyy
> Sent: Tuesday, September 05, 2017 2:08 PM
> To: lng-odp@lists.linaro.org
> Subject: Re: [lng-odp] Compiler Barrier API
> 
> Hey Maxim,
> There is a use case in the ODP code itself just search for the
> volatile("" ::: "memory"). It is rare, just like all the barriers.
> 
> We also have compiler barrier in our code, so I just thought it would be
> nice to add this to the api/sync.h, i.e.:
> 
> /**
>   * Compiler barrier
>   *
>   * This is a compiler barrier. It prevents compiler to rearrange
>   * access operations before the barrier with operations after it.
>   *
>   * It also forces compiler to load variables used in loop condition
>   * each loop cycle.
>   *
>   * This call is not needed when using ODP defined synchronization
>   * mechanisms.
>   */
> void odp_compiler_barrier(void);
> 
> 
> With the following definition:
> 
> static inline void odp_compiler_barrier(void)
> {
>   __asm__ volatile("": : :"memory");
> }
> 
> It it is OK with you, I can prepare a proper patch.
> 
> Andriy
> 

I think compiler barrier is too weak for writing portable code, since it does 
not guarantee that the CPU would not re-order the instructions. Application 
should use ODP memory barriers (odp_mb_full(), odp_mb_acquire(), etc) or 
atomics with memory ordering (odp_atomic_load_acq_u64(), 
odp_atomic_store_rel_u64()).

ODP implementation has ":::memory" in context of inline assembly instructions, 
but that's when we are writing code against specific ISA and thus know which 
amount of synchronization is needed. A portable application cannot assume any 
particular ISA.

-Petri

Re: [lng-odp] [PATCH API-NEXT v2 1/1] api: event: add documentation for ODP_EVENT_PACKET_CRYPTO

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

Reviewed-by: Petri Savolainen 


> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of
> Github ODP bot
> Sent: Tuesday, August 29, 2017 5:00 PM
> To: lng-odp@lists.linaro.org
> Subject: [lng-odp] [PATCH API-NEXT v2 1/1] api: event: add documentation
> for ODP_EVENT_PACKET_CRYPTO
> 
> From: Dmitry Eremin-Solenikov 
> 
> Signed-off-by: Dmitry Eremin-Solenikov 
> ---
> /** Email created from pull request 151 (lumag:add-event-type)
>  ** https://github.com/Linaro/odp/pull/151
>  ** Patch: https://github.com/Linaro/odp/pull/151.patch
>  ** Base sha: 91c0b58fc87ba0431241818758cea94438cd5498
>  ** Merge commit sha: 35c2d85c103d67ae4ae16b1e67ff5f66c002c6ce
>  **/
>  include/odp/api/spec/event.h | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/odp/api/spec/event.h b/include/odp/api/spec/event.h
> index 2ad3ce84..f924973f 100644
> --- a/include/odp/api/spec/event.h
> +++ b/include/odp/api/spec/event.h
> @@ -77,6 +77,10 @@ extern "C" {
>   *  List of event subtypes:
>   * - ODP_EVENT_PACKET_BASIC
>   * - Packet event (odp_packet_t) with basic packet metadata
> + * - ODP_EVENT_PACKET_CRYPTO
> + * - Packet event (odp_packet_t) generated as a result of a Crypto
> + *   operation. It contains crypto specific metadata in addition to
> the
> + *   basic packet metadata.
>   * - ODP_EVENT_PACKET_IPSEC
>   * - Packet event (odp_packet_t) generated as a result of an IPsec
>   *   operation. It contains IPSEC specific metadata in addition to
> the basic

Re: [lng-odp] Regarding crypto event subtypes

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

Looks like it's missing from the event.h documentation. It's defined by 
implementation (and crypto.h spec file), so code compiles OK, but doxygen may 
not show it

https://github.com/Linaro/odp/blob/api-next/platform/linux-generic/include/odp/api/plat/event_types.h



> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of
> shally verma
> Sent: Tuesday, August 29, 2017 12:16 PM
> To: lng-odp-forward 
> Cc: Mahipal Challa ; Narayana, Prasad Athreya
> ; Verma, Shally
> ; venkatesh.bo...@cavium.com
> Subject: [lng-odp] Regarding crypto event subtypes
> 
> Hi
> 
> In order to align to crypto, I was looking to latest spec regarding
> Sub-event types here :
> 
> https://github.com/Linaro/odp/blob/api-next/include/odp/api/spec/event.h ,
> 
> where I cannot see reference to any sub-event type for crypto.
> 
> So could anyone confirm if crypto will be supporting only one event -
> ODP_EVENT_CRYPTO_COMPL and no subtype?
> 
> Thanks
> Shally

Re: [lng-odp] [PATCH API-NEXT v8 1/1] comp: compression spec

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

> -Original Message-
> From: shally verma [mailto:shallyvermacav...@gmail.com]
> Sent: Tuesday, August 29, 2017 10:26 AM
> To: Narayana Prasad Athreya <pathr...@caviumnetworks.com>
> Cc: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@nokia.com>;
> Github ODP bot <odp...@yandex.ru>; lng-odp@lists.linaro.org; Narayana,
> Prasad Athreya <prasadathreya.naray...@cavium.com>; Mahipal Challa
> <mcha...@cavium.com>; Verma, Shally <shally.ve...@cavium.com>
> Subject: Re: [lng-odp] [PATCH API-NEXT v8 1/1] comp: compression spec
> 
> Based on last discussion, I was reworking to add odp_comp_op_pkt ()
> API based on Crypto design. Help me to answer with these questions:
> 
> 1. Current crypto packet base API is not giving Error code as an
> output to its sync version i.e. in int odp_crypto_op(const
> odp_packet_t pkt_in[],..), I do not see where it is returning
> odp_crypto_packet_result_t. Can anyone help?

Error codes are part of operation results:

/**
 * Get crypto operation results from an crypto processed packet
 *
 * Successful crypto operations of all types (SYNC and ASYNC) produce packets
 * which contain crypto result metadata. This function copies the operation
 * results from an crypto processed packet. Event subtype of this kind of
 * packet is ODP_EVENT_PACKET_crypto. Results are undefined if a non-crypto
 * processed packet is passed as input.
 *
 * @param packet  An crypto processed packet (ODP_EVENT_PACKET_CRYPTO)
 * @param[out]result  Pointer to operation result for output
 *
 * @retval  0 On success
 * @retval <0 On failure
 */
int odp_crypto_result(odp_crypto_packet_result_t *result,
  odp_packet_t packet);

/**
 * Crypto packet API operation result
 */
typedef struct odp_crypto_packet_result_t {
/** Request completed successfully */
odp_bool_t  ok;

/** Cipher status */
odp_crypto_op_status_t cipher_status;

/** Authentication status */
odp_crypto_op_status_t auth_status;

} odp_crypto_packet_result_t;

/**
 * Cryto API per packet operation completion status
 */
typedef struct odp_crypto_op_status {
/** Algorithm specific return code */
odp_crypto_alg_err_t alg_err;

/** Hardware specific return code */
odp_crypto_hw_err_t  hw_err;

} odp_crypto_op_status_t;

/**
 * Crypto API algorithm return code
 */
typedef enum {
/** Algorithm successful */
ODP_CRYPTO_ALG_ERR_NONE,
/** Invalid data block size */
ODP_CRYPTO_ALG_ERR_DATA_SIZE,
/** Key size invalid for algorithm */
ODP_CRYPTO_ALG_ERR_KEY_SIZE,
/** Computed ICV value mismatch */
ODP_CRYPTO_ALG_ERR_ICV_CHECK,
/** IV value not specified */
ODP_CRYPTO_ALG_ERR_IV_INVALID,
} odp_crypto_alg_err_t;


> 
> 2. Current crypto version of odp_crypto_op(odp_pkt_t pkt_in[] ...)
> does not have two separate version for encryption and decryption where
> as in Compression, we added two. One for compress and another for
> decompress.
> So do we want to retain two separate flavor or unify like crypto
> packet based api? Ex.
> odp_comp_op_pkt ( ... ) OR
> odp_comp_compress_pkt( ...),
> odp_comp_decompress_pkt(),
> odp_comp_compress_pkt_enq() and so on...?

Crypto has single operation, IPSEC has two operations (inbound vs outbound). 
So, both styles are used today. Benefits of an operation per direction are:
* more readable code: odp_comp_compress() vs odp_comp_op()
* possibility to have different set of arguments (parameters) for each 
direction. E.g. IPSEC does IP fragmentation on output direction and thus needs 
extra parameters for that, those params are not defined on inbound direction.
* cleaner specification of different operations e.g. "... output of 
odp_comp_compress()..." vs "... output of odp_comp_op() in compress mode "
* easier to extend since a new feature can be added to one side without 
changing the spec for the other side


BTW, since most of our operations process packets, we don't need to highlight 
it with "pkt". I'd name odp_comp_compress() for packets, and then later on add 
odp_comp_compress_mem(), odp_comp_compress_from_mem(), etc for mem -> mem, pkt 
-> mem operations.

-Petri

Re: [lng-odp] Suspected SPAM - Re: [PATCH API-NEXT v5 1/1] API:IPSEC: IPSEC events may be reported synchronously.

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

> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of
> Peltonen, Janne (Nokia - FI/Espoo)
> Sent: Wednesday, August 16, 2017 9:16 AM
> To: Bill Fischofer 
> Cc: lng-odp@lists.linaro.org
> Subject: Suspected SPAM - Re: [lng-odp] [PATCH API-NEXT v5 1/1] API:IPSEC:
> IPSEC events may be reported synchronously.
> 
> I think we are pretty much on the same page about the sync issues. It is
> then just an API
> design choice whether ODP always sends a disable completion event in async
> and inline
> mode (current API) or whether it is left to the application if it needs it
> (the patch).
> I was preferring the former, but maybe it is just me.
>

I also prefer the current API, which is simpler for application (completion 
signal comes always the same way).


 
> > So it's a simple matter to drain those events if needed before calling
> destroy.
> 
> I think an event (enqueued either by ODP or by the application after
> disabling an SA)
> is needed to do the draining safely. Maybe a timing based approach would
> do too with
> reasonable assumptions about the ODP implementation.

An async application likely needs a marker event anyway, since the same (SA) 
queue may receive events from different sources (control plane, or multiple 
SAs) and app could not rely on the queue going empty (in a limited amount of 
time).

-Petri

> 
> > If the application took events off the queue and was still doing
> something else with them
> > then it's an application responsibility to know when it's safe to issue
> the destroy call for the SA.
> 
> Yes. One way to do it is to configure the SA queue as atomic, another is
> to use an ordered
> queue and order the disable completion event handling with respect to
> other SA events
> using an ordered lock.
> 
>  Janne
> 
> 
> From: Bill Fischofer [mailto:bill.fischo...@linaro.org]
> Sent: Tuesday, August 15, 2017 3:34 PM
> To: Peltonen, Janne (Nokia - FI/Espoo) 
> Cc: Github ODP bot ; lng-odp@lists.linaro.org
> Subject: Re: [lng-odp] [PATCH API-NEXT v5 1/1] API:IPSEC: IPSEC events may
> be reported synchronously.
> 
> 
> 
> On Mon, Aug 14, 2017 at 11:42 PM, Peltonen, Janne (Nokia - FI/Espoo)
> > wrote:
> > The definition of odp_ipsec_sa_disable() is that RC = 0 means the SA is
> now guaranteed idle
> 
> It means that no further events for that SA will be posted to the SA
> queue.
> 
> > and may be safely destroyed,
> 
> Only from ODP point-of-view. The application may still have IPsec subtype
> packets for the SA
> in flight in other threads.
> 
> True. Applications must always know themselves. If an application dequeues
> an event from an SA queue and re-enqueues it elsewhere presumably it's the
> application's responsibility to track that event as well.
> 
> 
> > so there would be no other events to dequeue.
> 
> There can still be unhandled events in the event queues and outside event
> queues in other threads.
> 
> My point in my comments is that the application needs to synchronize
> between regular IPsec
> completion event handling and destroying an SA and for that an “end
> marker” event in the SA
> queue would be quite convenient, or even necessary to avoid more costly
> synchronization.
> 
> Completion of the disable operation means that the SA is "idle" from an
> ODP standpoint. As you noted, it's always up to the application to say
> when it is idle from the application's standpoint. That's why disable is
> different from destroy. A disabled SA is still valid, it will just not be
> used by ODP. Applications can still reference (but not initiate work on)
> disabled SAs.
> 
> 
> Let’s consider IPsec packet reception in inline mode as an example:
> 
> As long as an SA is active (not disabled) incoming packets can match it
> and end up as
> IPsec packet events in the SA queue. Thus when an application disables the
> SA, there can
> be unhandled events for that SA in the event queues and/or under
> processing in other threads.
> If the thread that disabled the SA immediately destroyed the SA, then
> event handling
> would attempt to use a destroyed SA (e.g. it would call
> odp_ipsec_sa_context()) for an
> SA that was already destroyed, resulting in undefined behavior.
> 
> This is a good example. Upon return from odp_ipsec_sa_disable() RC 0
> guarantees that ODP will make no further reference to this SA. So no
> additional packets will match it. But it's still up to the application to
> decide when to destroy the SA. As far as events go, if an SA event queue
> is used then RC = 0 says that queue is "idle", not empty, which means that
> anything ODP wanted to add to that queue has been added and the
> application is guaranteed that no further events will be added by ODP. So
> it's a simple matter to drain those events if needed before calling
> destroy.
> 
> Note that the async 

Re: [lng-odp] [PATCH API-NEXT v8 1/1] comp: compression spec

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

> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of
> Github ODP bot
> Sent: Friday, August 04, 2017 4:00 PM
> To: lng-odp@lists.linaro.org
> Subject: [lng-odp] [PATCH API-NEXT v8 1/1] comp: compression spec
> 
> From: Shally Verma 
> 
> Signed-off-by: Shally Verma 
> Signed-off-by: Mahipal Challa  Cc
> prasadathreya.naray...@cavium.com
> ---
> /** Email created from pull request 102 (1234sv:api-next)
>  ** https://github.com/Linaro/odp/pull/102
>  ** Patch: https://github.com/Linaro/odp/pull/102.patch
>  ** Base sha: 8390f890d4bd2babb63a24f7b15d2f4763e44050
>  ** Merge commit sha: fbdff8c82a19f5b640ae299204b3bb1bbbefdccb
>  **/
>  include/odp/api/spec/comp.h | 815
> 
>  1 file changed, 815 insertions(+)
>  create mode 100644 include/odp/api/spec/comp.h
> 
> diff --git a/include/odp/api/spec/comp.h b/include/odp/api/spec/comp.h
> new file mode 100644
> index ..2956094c
> --- /dev/null
> +++ b/include/odp/api/spec/comp.h
> @@ -0,0 +1,815 @@
> +/* Copyright (c) 2013, Linaro Limited

Year 2017

> +
> +/**
> + * Comp API hash algorithm
> + *
> + */
> +typedef enum {
> + /** ODP_COMP_HASH_ALG_NONE*/

This kind of comment is not very helpful. Each enumeration needs explanation - 
like odp_comp_alg_t under.

> + ODP_COMP_HASH_ALG_NONE,
> + /** ODP_COMP_HASH_ALG_SHA1*/
> + ODP_COMP_HASH_ALG_SHA1,
> + /**  ODP_COMP_HASH_ALG_SHA256*/
> + ODP_COMP_HASH_ALG_SHA256
> +} odp_comp_hash_alg_t;
> +
> +/**
> + * Comp API compression algorithm
> + *
> + */
> +typedef enum {
> + /** No algorithm specified.
> +  * Means no compression, output == input.
> +  * if provided, no operation (compression/decompression or
> hash)
> +  * applied on input. Added for testing purpose.
> +  */
> + ODP_COMP_ALG_NULL,
> + /** DEFLATE - RFC1951 */
> + ODP_COMP_ALG_DEFLATE,
> + /** ZLIB - RFC1950 */
> + ODP_COMP_ALG_ZLIB,
> + /** LZS */
> + ODP_COMP_ALG_LZS
> +} odp_comp_alg_t;
> +
> +
> +/**
> + * Hash algorithms in a bit field structure
> + *
> + */
> +typedef union odp_comp_hash_algos_t {
> + /** hash algorithms */
> + struct {
> + /** SHA-1 */
> + uint32_t sha1  : 1;
> +
> + /** SHA with 256 bits of Message Digest */
> + uint32_t sha256 : 1;


Need to be more explicit in algorithm definition: SHA-1, SHA-256, ... algorithm 
(SHA-2 would also do, but we use SHA-256 in crypto API since it seems to be 
used by standards).

Actually, these explanations should go under enum definitions and then just 
refer to those enums here - like odp_comp_algos_t under.


> +
> + } bit;
> +
> + /** All bits of the bit field structure
> +  *
> +  * This field can be used to set/clear all flags, or bitwise
> +  * operations over the entire structure.
> +  */
> + uint32_t all_bits;
> +} odp_comp_hash_algos_t;
> +
> +/**
> + * Comp algorithms in a bit field structure
> + *
> + */
> +typedef union odp_comp_algos_t {
> + /** Compression algorithms */
> + struct {
> + /** ODP_COMP_ALG_NULL */
> + uint32_t null   : 1;
> +
> + /** ODP_COMP_ALG_DEFLATE */
> + uint32_t deflate: 1;
> +
> + /** ODP_COMP_ALG_ZLIB */
> + uint32_t zlib   : 1;
> +
> + /** ODP_COMP_ALG_LZS */
> + uint32_t lzs:1;
> + } bit;
> +
> + /** All bits of the bit field structure
> +  * This field can be used to set/clear all flags, or bitwise
> +  * operations over the entire structure.
> +  */
> + uint32_t all_bits;
> +} odp_comp_algos_t;
> +
> +/**
> + * Compression Interface Capabilities
> + *
> + */
> +typedef struct odp_comp_capability_t {
> + /** Maximum number of  sessions */
> + uint32_t max_sessions;
> +
> + /** Supported compression algorithms */
> + odp_comp_algos_t comp_algs;

No need to save one character => comp_algos

> +
> + /** Supported hash algorithms. */
> + odp_comp_hash_algos_t hash_algs;

hash_algos

> +
> + /* sync/async mode of operation support.
> +  * Implementation should support atleast one of the mode.
> +  */


"mode" field definition missing on this line ?


> +
> + /** Support type for synchronous operation mode
> (ODP_COMP_SYNC).
> +  *  User should set odp_comp_session_param_t:mode based on
> +  *  support level as indicated by this param.
> +  */
> + odp_support_t sync;
> +
> + /** Support type for asynchronous operation mode
> (ODP_COMP_ASYNC).
> +  *  User should set odp_comp_session_param_t:mode param based
> on
> +  *  support level as indicated by this param.
> +  */
> + odp_support_t async;
> +} odp_comp_capability_t;
> +
> +/**
> + * Hash algorithm capabilities
> + *
> + */
> +typedef struct 

Re: [lng-odp] [Linaro/odp] 09ce94: api: schedule: add thread removal correction

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

This is an API change, that I did oppose on June 16th. Why it's now merged?

What is the requirement ("All threads that have joined the group must leave 
before destroying the group") ? A thread may be in a group due to automatic 
group (no create/join called), being part of thread mask in a create call or 
being part of thread mask in a join call. Also, thread itself may not call 
create/join/leave, but another thread may do that for it.

In which cases a thread needs to be removed by explicit leave() call?  Always, 
when create() was called, only when join() was called ?

-Petri
 

> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of
> GitHub
> Sent: Monday, August 07, 2017 1:59 PM
> To: lng-odp@lists.linaro.org
> Subject: [lng-odp] [Linaro/odp] 09ce94: api: schedule: add thread removal
> correction
> 
>   Branch: refs/heads/api-next
>   Home:   https://github.com/Linaro/odp
>   Commit: 09ce9406e09eacadf8f739ae8499323ad1401d36
> 
> https://github.com/Linaro/odp/commit/09ce9406e09eacadf8f739ae8499323ad1401
> d36
>   Author: Honnappa Nagarahalli 
>   Date:   2017-08-07 (Mon, 07 Aug 2017)
> 
>   Changed paths:
> M include/odp/api/spec/schedule.h
> 
>   Log Message:
>   ---
>   api: schedule: add thread removal correction
> 
> Clarify thread removal status from scheduler group
> before calling odp_schedule_group_destroy.
> 
> Signed-off-by: Honnappa Nagarahalli 
> Reviewed-by: Brian Brooks 
> Reviewed-by: Ola Liljedahl 
> Reviewed-by: Bill Fischofer 
> Signed-off-by: Maxim Uvarov 
> 

Re: [lng-odp] [API-NEXT PATCH] api: ipsec: add warning status event

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

From: Bill Fischofer [mailto:bill.fischo...@linaro.org] 
Sent: Friday, July 14, 2017 4:40 PM
To: Dmitry Eremin-Solenikov 
Cc: Petri Savolainen ; lng-odp-forward 

Subject: Re: [lng-odp] [API-NEXT PATCH] api: ipsec: add warning status event



On Fri, Jul 14, 2017 at 8:30 AM, Dmitry Eremin-Solenikov 
 wrote:
On 14.07.2017 15:43, Petri Savolainen wrote:
> Add status ID for warning messages. Outbound inline soft lifetime
> expiration is currently the only source of these events. API spec
> keeps simple when the same warning structure is shared with IPSEC
> packet and status events.
>
> Signed-off-by: Petri Savolainen 

Reviewed-by: Dmitry Eremin-Solenikov 

Ideally we can have a config option to enable/disable sending such
events in ASYNC or even SYNC mode.

If you don't want the notification wouldn't you just not set an expiration 
limit in the SA? That would seem the simplest solution.


This spec limits event generation just for outbound inline: "This status event 
is generated only for outbound SAs in ODP_IPSEC_OP_MODE_INLINE mode."

-Petri

Re: [lng-odp] [RFC/API-NEXTv3 1/3] api: classification: add support for packet hashing in classification

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

> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of
> Balasubramanian Manoharan
> Sent: Friday, July 14, 2017 12:12 PM
> To: lng-odp@lists.linaro.org
> Subject: [lng-odp] [RFC/API-NEXTv3 1/3] api: classification: add support
> for packet hashing in classification
> 
> Enable packet hashing per CoS to be able to distribute incoming packets to
> multiple queues linked with a CoS.
> 
> Signed-off-by: Balasubramanian Manoharan 
> ---
>  include/odp/api/spec/classification.h |   96
> +++--
>  include/odp/api/spec/packet_io.h  |   21 
>  2 files changed, 112 insertions(+), 5 deletions(-)
> 
> diff --git a/include/odp/api/spec/classification.h
> b/include/odp/api/spec/classification.h
> index 39831b2..d31965e 100644
> --- a/include/odp/api/spec/classification.h
> +++ b/include/odp/api/spec/classification.h
> @@ -4,7 +4,6 @@
>   * SPDX-License-Identifier: BSD-3-Clause
>   */
> 
> -
>  /**
>   * @file
>   *
> @@ -19,12 +18,13 @@
>  extern "C" {
>  #endif
> 
> +#include 
> +#include 
>  /** @defgroup odp_classification ODP CLASSIFICATION
>   *  Classification operations.
>   *  @{
>   */
> 
> -
>  /**
>   * @typedef odp_cos_t
>   * ODP Class of service handle
> @@ -126,6 +126,19 @@ typedef struct odp_cls_capability_t {
>   /** Maximum number of CoS supported */
>   unsigned max_cos;
> 
> + /* support mode for plain queue */
> + odp_support_t plain_queue;
> +
> + /* support mode for sched queue */
> + odp_support_t sched_queue;


This would be the first time we'd allow that a target queue type may be 
limited. I'd rather not do this. If e.g. sched queue are not good for cls, why 
not then do the same for all other APIs with target queues, and result a valid 
ODP implementation where application cannot use scheduled queues in  
communication with ODP (results from pktin, cls, crypto, ipsec, timer, ... 
should be polled).


> +
> + /** Maximun number of queue supported per CoS
> +  * if the value is 1, then hashing is not supported*/
> + unsigned max_queue_supported;

This is apparently max queues for hashing, so the variable should be 
"max_hash_queues" or similar.

> +
> + /** Protocol header combination supported for Hashing */
> + odp_pktin_hash_proto_t hash_supported;

hash_protocols or hash_proto, "supported" is not needed in variable name.


> +
>   /** A Boolean to denote support of PMR range */
>   odp_bool_t pmr_range_supported;
>  } odp_cls_capability_t;
> @@ -164,9 +177,40 @@ typedef enum {
>   * Used to communicate class of service creation options
>   */
>  typedef struct odp_cls_cos_param {
> - odp_queue_t queue;  /**< Queue associated with CoS */
> - odp_pool_t pool;/**< Pool associated with CoS */
> - odp_cls_drop_t drop_policy; /**< Drop policy associated
> with CoS */
> + /* Number of queues to be linked to this CoS.
> +  * If the number is greater than 1 then hashing has to be
> +  * configured. If number is equal to 1 then hashing is disabled
> +  * and queue has to be configured by the application.
> +  * When hashing is enabled the queues are created by the
> implementation
> +  * then application need not configure any queue to this CoS.
> +  * Depening on the implementation this number might be rounded-
> off to
> +  * nearest supported value (e.g power of 2)
> +  * */
> + uint32_t num_queue;
> +
> + /** Queue type */
> + odp_queue_type_t queue_type;
> +
> + /** ODP Queue dequeue mode
> +  *  This value is valid only for PLAIN queue types */
> + odp_queue_op_mode_t deq_mode;
> +
> + /** Schedule parameters for the queue created */
> + odp_schedule_param_t sched_param;


Better use odp_queue_param_t instead of individual types.


> +
> + /* Protocol header fields which are included in packet input
> +  * hash calculation */
> + odp_hash_proto_t hash_proto;


This cannot be enum, but odp_pktin_hash_proto_t since application may need to 
enable hashing e.g. for both IPv4 and IPv6 simultaneously.


> +
> + /* If hashing is disabled, then application has to configure
> +  * this queue and packets are delivered to this queue */
> + odp_queue_t queue;
> +
> + /* Pool associated with CoS */
> + odp_pool_t pool;
> +
> + /* Drop policy associated with CoS */
> + odp_cls_drop_t drop_policy;
>  } odp_cls_cos_param_t;
> 
>  /**
> @@ -209,6 +253,23 @@ int odp_cls_capability(odp_cls_capability_t
> *capability);
>  odp_cos_t odp_cls_cos_create(const char *name, odp_cls_cos_param_t
> *param);
> 
>  /**
> + * Queue hash result
> + * Returns the queue within a CoS in which a particular packet will be
> enqueued
> + * based on the packet parameters and hash protocol field configured with
> the
> + * class of service.
> + *
> + * @paramcos class of service
> + * @parampacket  Packet handle
> + *
> + 

Re: [lng-odp] [API-NEXT PATCH v1 1/1] pktio APIs to set the MAC address and MTU size.

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

> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of Vamsi
> Attunuru
> Sent: Friday, July 14, 2017 1:05 PM
> To: lng-odp@lists.linaro.org
> Cc: mcha...@cavium.com; pathr...@cavium.com; vattun...@cavium.com;
> sve...@cavium.com
> Subject: [lng-odp] [API-NEXT PATCH v1 1/1] pktio APIs to set the MAC
> address and MTU size.


When API is modified the subject line format must begin with "api: pktio: "

So, change the subject to: "api: pktio: add mac_addr and mtu set functions"


> 
> Adds new pktio APIs to set MTU and MAC address on pktio interface.
> 
> Signed-off-by: Vamsi Attunuru 
> Signed-off-by: Mahipal Challa 
> Signed-off-by: Shally Verma   
> 
> ---
>  include/odp/api/spec/packet_io.h | 45
> 
>  1 file changed, 45 insertions(+)
> 
> diff --git a/include/odp/api/spec/packet_io.h
> b/include/odp/api/spec/packet_io.h
> index 8802089..1269f44 100644
> --- a/include/odp/api/spec/packet_io.h
> +++ b/include/odp/api/spec/packet_io.h
> @@ -451,6 +451,10 @@ typedef union odp_pktio_set_op_t {
>   struct {
>   /** Promiscuous mode */
>   uint32_t promisc_mode : 1;
> + /** MAC address */
> + uint32_t mac : 1;


This should be mac_addr, since the function is odp_pktio_mac_addr_set()


> + /** MTU size */
> + uint32_t mtu : 1;
>   } op;
>   /** All bits of the bit field structure.
> * This field can be used to set/clear all flags, or bitwise
> @@ -482,6 +486,12 @@ typedef struct odp_pktio_capability_t {
>* A boolean to denote whether loop back mode is supported on
> this
>* specific interface. */
>   odp_bool_t loop_supported;
> +
> + /** Maximum MTU size supported */
> + uint32_t max_mtu_size;
> +
> + /** Length of MAC address supported on this specific interface
> */
> + uint32_t mac_addr_len;
>  } odp_pktio_capability_t;
> 
>  /**
> @@ -912,6 +922,21 @@ int odp_pktout_send(odp_pktout_queue_t queue, const
> odp_packet_t packets[],
>  uint32_t odp_pktio_mtu(odp_pktio_t pktio);
> 
>  /**
> + * Set MTU value of a packet IO interface.
> + *
> + * Application should pass value upto max_mtu_size as indicated by
> + * odp_pktio_capability_t:max_mtu_size. Any value beyond max_mtu_size
> + * limit will result in failure.
> + *
> + * @param pktio  Packet IO handle.
> + * @param mtuMTU value to be set.
> + *
> + * @return  0 on success
> + * @retval <0 on failure
> + */
> +int odp_pktio_mtu_set(odp_pktio_t pktio, uint32_t mtu);
> +
> +/**
>   * Enable/Disable promiscuous mode on a packet IO interface.
>   *
>   * @param[in] pktio  Packet IO handle.
> @@ -946,6 +971,26 @@ int odp_pktio_promisc_mode(odp_pktio_t pktio);
>  int odp_pktio_mac_addr(odp_pktio_t pktio, void *mac_addr, int size);
> 
>  /**
> + * Set the MAC address of a packet IO interface.
> + *
> + * Application should pass mac_addr buffer with size >=
> + * odp_pktio_capablity_t:mac_addr_len, size value less than
> + * implementation supported will result in API failure.
> + *
> + * On success, Implementation would read mac_addr buffer bytes
> + * upto mac_addr_len value indicated in capability information.

An interface should have only single valid mac address length value, right ? 
So, this spec should just say that:

"Changes interface MAC address to the address pointed by 'mac_addr'. Value of 
'size' must be equal to the interface mac address size, which is specified by 
'mac_addr_len' capability and return value of odp_pktio_mac_addr(). Operation 
returns failure on other values of 'size'. MAC address is not changed on 
failure."

-Petri


> + *
> + * @param pktioPacket IO handle
> + * @param mac_addr Pointer to MAC address to be set
> + * @param size Size of MAC address buffer
> + *
> + * @return  0 on success
> + * @retval <0 on failure
> + */
> +int odp_pktio_mac_addr_set(odp_pktio_t pktio, const uint8_t *mac_addr,
> +   int size);
> +
> +/**
>   * Setup per-port default class-of-service.
>   *
>   * @param[in]pktio   Ingress port pktio handle.
> --
> 1.9.3

Re: [lng-odp] [PATCH API-NEXT v3 0/4] Crypto API updates

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

Reviewed-by: Petri Savolainen 

Maxim, could you merge this ASAP since patch 1/4 fixes broken api-next build 
(when using gcc 4.8.4)


> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of
> Github ODP bot
> Sent: Friday, July 14, 2017 1:00 AM
> To: lng-odp@lists.linaro.org
> Subject: [lng-odp] [PATCH API-NEXT v3 0/4] Crypto API updates
> 
> Updates for crypto API as suggested by @psavol
> 
> github
> /** Email created from pull request 74 (lumag:crypto-packet)
>  ** https://github.com/Linaro/odp/pull/74
>  ** Patch: https://github.com/Linaro/odp/pull/74.patch
>  ** Base sha: f49ce736d461c3e8c2534ed216e8a70e8bee954e
>  ** Merge commit sha: 668e2e51ec1fae14a324a67371babeeb8ecdaeda
>  **/
> /github
> 
> checkpatch.pl
> total: 0 errors, 0 warnings, 0 checks, 8 lines checked
> 
> 
> to_send-p-000.patch has no obvious style problems and is ready for
> submission.
> total: 0 errors, 0 warnings, 0 checks, 40 lines checked
> 
> 
> to_send-p-001.patch has no obvious style problems and is ready for
> submission.
> total: 0 errors, 0 warnings, 0 checks, 336 lines checked
> 
> 
> to_send-p-002.patch has no obvious style problems and is ready for
> submission.
> total: 0 errors, 0 warnings, 0 checks, 284 lines checked
> 
> 
> to_send-p-003.patch has no obvious style problems and is ready for
> submission.
> /checkpatch.pl

Re: [lng-odp] [API-NEXT PATCHv6 2/4] linux-generic: timer: implement odp_timer_capability()

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

OK. Yes I think 10x should be conservative enough. Make it a #define so that 
it's easy to update if needed.

-Petri


From: Kevin Wang [mailto:kevin.w...@linaro.org] 
Sent: Thursday, July 13, 2017 6:31 PM
To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@nokia.com>
Cc: Kevin Wang <kevin.w...@arm.com>; lng-odp@lists.linaro.org
Subject: Re: [lng-odp] [API-NEXT PATCHv6 2/4] linux-generic: timer: implement 
odp_timer_capability()

I don't think to add/multiply the result from clock_getres() is a better 
solution here because clock_getres() always return 1ns on most platform. 

This solution did the basic measurement on the target platform.It should be 
more reliable than a constant value. 
But I agree that the value should be rounded up conservatively as the 
measurement is in the init time which doesn't has such heavy workloads.  
So do you think round up to 10x is ok?

Thanks,
Kevin

2017-07-13 18:46 GMT+08:00 Savolainen, Petri (Nokia - FI/Espoo) 
<mailto:petri.savolai...@nokia.com>:


> -Original Message-
> From: lng-odp [mailto:mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of 
> Kevin
> Wang
> Sent: Thursday, July 13, 2017 8:47 AM
> To: mailto:lng-odp@lists.linaro.org
> Cc: Kevin Wang <mailto:kevin.w...@arm.com>
> Subject: [lng-odp] [API-NEXT PATCHv6 2/4] linux-generic: timer: implement
> odp_timer_capability()
>
> Implement a new internal function timer_res_init() to detect the max
> timer resolution without overrun at the ODP init stage. It will check
> timer resolution from 1ms to 100us, 10us...1ns until the timer is
> overrun.
>

It would be simpler to use clock_getres() and add/multiply it with a safety 
margin ... or even fix it to e.g. 1ms. This way max res would be constant.

Measurement could result e.g. a 10x better value during init time vs when the 
system is actually running, thus giving too optimistic resolution sometimes. At 
least, measurement should be capped and/or rounded up to be conservative.


-Petri




-- 
Thanks,
Kevin

Re: [lng-odp] [PATCH 0/3] Change buffer handle to pointer

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

> -Original Message-
> From: Bill Fischofer [mailto:bill.fischo...@linaro.org]
> Sent: Thursday, July 13, 2017 3:18 PM
> To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@nokia.com>
> Cc: Maxim Uvarov <maxim.uva...@linaro.org>; lng-odp@lists.linaro.org
> Subject: Re: [lng-odp] [PATCH 0/3] Change buffer handle to pointer
> 
> Sorry, reposting my previous review:
> 
> For this series:
> 
> Reviewed-and-tested-by: Bill Fischofer <bill.fischo...@linaro.org>


I did send v2, which is just a rebase on top of the current master. Patch 2/3 
had a conflict.

-Petri

Re: [lng-odp] [PATCH v1 1/1] pktio APIs to modify the MAC address or the MTU size of the given packet IO handler.

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

> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of Vamsi
> Attunuru
> Sent: Thursday, July 13, 2017 11:54 AM
> To: lng-odp@lists.linaro.org
> Cc: macha...@cavium.com; pathr...@cavium.com; vattun...@cavium.com;
> sve...@cavium.com
> Subject: [lng-odp] [PATCH v1 1/1] pktio APIs to modify the MAC address or
> the MTU size of the given packet IO handler.
> 
> ---
>  include/odp/api/spec/packet_io.h | 29 +
>  1 file changed, 29 insertions(+)
> 
> diff --git a/include/odp/api/spec/packet_io.h
> b/include/odp/api/spec/packet_io.h
> index c7373fd..a950267 100644
> --- a/include/odp/api/spec/packet_io.h
> +++ b/include/odp/api/spec/packet_io.h
> @@ -382,6 +382,10 @@ typedef union odp_pktio_set_op_t {
>   struct {
>   /** Promiscuous mode */
>   uint32_t promisc_mode : 1;
> + /** mac addr */
> + uint32_t mac : 1;
> + /** mtu addr */
> + uint32_t mtu : 1;
>   } op;
>   /** All bits of the bit field structure.
> * This field can be used to set/clear all flags, or bitwise
> @@ -413,6 +417,9 @@ typedef struct odp_pktio_capability_t {
>* A boolean to denote whether loop back mode is supported on
> this
>* specific interface. */
>   odp_bool_t loop_supported;
> +
> + /* max mtu size supported */
> + uint32_t max_mtu_size;
>  } odp_pktio_capability_t;
> 
>  /**
> @@ -843,6 +850,16 @@ int odp_pktout_send(odp_pktout_queue_t queue, const
> odp_packet_t packets[],
>  uint32_t odp_pktio_mtu(odp_pktio_t pktio);
> 
>  /**
> + * Set MTU value of a packet IO interface.
> + *
> + * @param[in] pktio  Packet IO handle.

[in] is not needed, by default all params are "in" and we mark only those that 
are "out" or "in and out"

@param mtu documentation is missing. Run 'make doxygen-doc' and correct all 
warnings before posting.


> + *
> + * @return 0 on success
> + * @retval -1 on failure
> + */
> +int odp_pktio_mtu_set(odp_pktio_t pktio, int mtu);

mtu should be uint32_t


> +
> +/**
>   * Enable/Disable promiscuous mode on a packet IO interface.
>   *
>   * @param[in] pktio  Packet IO handle.
> @@ -877,6 +894,18 @@ int odp_pktio_promisc_mode(odp_pktio_t pktio);
>  int odp_pktio_mac_addr(odp_pktio_t pktio, void *mac_addr, int size);
> 
>  /**
> + * Set the MAC address of a packet IO interface.
> + *
> + * @parampktio Packet IO handle
> + * @param[in]mac_addr  Input buffer

Remove [in]

"Input buffer" is too generic. "Pointer to MAC address to be set on the 
interface" or similar


> + * @param   size  Size of output buffer
> + *
> + * @return Number of bytes written (actual size of MAC address)

Return success or fail, since application would not want to write partial MAC 
address. Read side API offer a way to get address size of application is 
uncertain. On write side application should know the size.

> + * @retval <0 on failure
> + */
> +int odp_pktio_mac_addr_set(odp_pktio_t pktio, void *mac_addr, int size);

const void *mac_addr, since it's read only data for the implementation.



-Petri

Re: [lng-odp] [PATCH API-NEXT v2 0/4] Crypto API updates

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

Dmitry, Maxim,

I think it's OK to merge v1 of this ASAP since it fixes e.g. api-next build 
break. I did send my review yesterday. V2 just adds 4/4 patch on top.

-Petri

Re: [lng-odp] [PATCH API-NEXT v2 4/4] api: crypto: revert deprecation of crypto completion API

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

> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of
> Github ODP bot
> Sent: Wednesday, July 12, 2017 5:00 PM
> To: lng-odp@lists.linaro.org
> Subject: [lng-odp] [PATCH API-NEXT v2 4/4] api: crypto: revert deprecation
> of crypto completion API
> 
> From: Dmitry Eremin-Solenikov 
> 
> It was decided that it would be benefitable to live with both API types
> at this point, as odp_crypto_compl_t was available for some time. So
> undeprecate odp_crypto_compl_t and related functionality. Validation
> tests also provide necessary tests for pref_mode and for completion
> event.
> 
> Signed-off-by: Dmitry Eremin-Solenikov 
> ---
> /** Email created from pull request 74 (lumag:crypto-packet)
>  ** https://github.com/Linaro/odp/pull/74
>  ** Patch: https://github.com/Linaro/odp/pull/74.patch
>  ** Base sha: ee5be324411a7520528a367967c28fc529d3bc2e
>  ** Merge commit sha: 5411462e6545fa2d6a286a40c2057db97714ee74
>  **/
>  include/odp/api/spec/crypto.h  | 38 +++--
> ---
>  include/odp/arch/default/api/abi/crypto.h  |  4 +--
>  include/odp/arch/default/api/abi/event.h   |  4 +--
>  .../include/odp/api/plat/crypto_types.h|  3 +-
>  .../include/odp/api/plat/event_types.h |  3 +-
>  platform/linux-generic/odp_crypto.c|  4 ---
>  platform/linux-generic/odp_event.c |  2 --
>  test/common_plat/performance/odp_crypto.c  |  1 +
>  test/common_plat/validation/api/crypto/crypto.c|  2 ++
>  .../validation/api/crypto/odp_crypto_test_inp.c| 41
> +-
>  .../validation/api/crypto/odp_crypto_test_inp.h|  2 ++
>  11 files changed, 61 insertions(+), 43 deletions(-)
> 
> diff --git a/include/odp/api/spec/crypto.h b/include/odp/api/spec/crypto.h
> index 3e47f3ef..6736214b 100644
> --- a/include/odp/api/spec/crypto.h
> +++ b/include/odp/api/spec/crypto.h
> @@ -271,11 +271,8 @@ typedef struct odp_crypto_session_param_t {
>*/
>   odp_bool_t auth_cipher_text;
> 
> - /** Preferred sync vs. async
> -  *
> -  * @deprecated no-op now, odp_crypto_operation() will always
> process
> -  * data in non-posted mode */
> - odp_crypto_op_mode_t ODP_DEPRECATE(pref_mode);
> + /** Preferred sync vs. async for odp_crypto_operation() */
> + odp_crypto_op_mode_t pref_mode;

Maybe it makes still sense to leave these @deprecated doxygen tags into 
documentation. So, that we (and user) have some means to follow where goes to 
line between new and old API. Old API documentation should not change, but just 
tag those that are part of the old API and will be removed in future.

As agreed, we'll leave out ODP_DEPRECATE() macros in this first phase. Add 
those in next phase, and remove in the last phase.

-Petri

Re: [lng-odp] [API-NEXT PATCHv6 2/4] linux-generic: timer: implement odp_timer_capability()

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

> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of Kevin
> Wang
> Sent: Thursday, July 13, 2017 8:47 AM
> To: lng-odp@lists.linaro.org
> Cc: Kevin Wang 
> Subject: [lng-odp] [API-NEXT PATCHv6 2/4] linux-generic: timer: implement
> odp_timer_capability()
> 
> Implement a new internal function timer_res_init() to detect the max
> timer resolution without overrun at the ODP init stage. It will check
> timer resolution from 1ms to 100us, 10us...1ns until the timer is
> overrun.
> 

It would be simpler to use clock_getres() and add/multiply it with a safety 
margin ... or even fix it to e.g. 1ms. This way max res would be constant.

Measurement could result e.g. a 10x better value during init time vs when the 
system is actually running, thus giving too optimistic resolution sometimes. At 
least, measurement should be capped and/or rounded up to be conservative.


-Petri

Re: [lng-odp] [API-NEXT PATCHv6 1/4] api: timer: add odp_timer_capability() api

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

> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of Kevin
> Wang
> Sent: Thursday, July 13, 2017 8:47 AM
> To: lng-odp@lists.linaro.org
> Cc: Kevin Wang 
> Subject: [lng-odp] [API-NEXT PATCHv6 1/4] api: timer: add
> odp_timer_capability() api
> 
> Currently, user needs to decide the timer resolution before creating
> a timer pool. But sometimes it will cause timer overrun as the system
> can't support such high resolution.
> So a new API is required to expose the timer capability to the user.
> 
> Signed-off-by: Kevin Wang 
> ---
>  include/odp/api/spec/timer.h | 21 +
>  1 file changed, 21 insertions(+)
> 
> diff --git a/include/odp/api/spec/timer.h b/include/odp/api/spec/timer.h
> index 75f9db9..e722cb9 100644
> --- a/include/odp/api/spec/timer.h
> +++ b/include/odp/api/spec/timer.h
> @@ -108,6 +108,27 @@ typedef struct {
>  } odp_timer_pool_param_t;
> 
>  /**
> + * Timer capability
> + */
> +typedef struct {
> + uint64_t max_res; /**< Max timer resolution in nanoseconds */


It's better to include nanosec into the variable name, so that we can later on 
support also sub-nanosec values if needed (define hertz or fractions of nsec 
param). Also, if we name it highest instead of max, it's easier to explain that 
max res == min value.


/** Highest timer resolution in nanoseconds.
 *
 *  This defines the highest resolution supported by a timer.
 *  It's the minimum valid value for 'res_ns' timer pool
 *  parameter.
 */
uint64_t highest_res_ns;


> +} odp_timer_capability_t;
> +
> +/**
> + * Query Timer interface capabilities

"Query timer capabilities"

Term "interface" is not needed

> + *
> + * Outputs Timer interface capabilities on success.

"Outputs timer capabilities on success."

Term "interface" is not needed

> + *
> + * @param clk_src Clock source for timers
> + * @param[out] capa   Pointer to capability structure for output


Align text in columns for better readability:

@param  clk_src  Clock source for timers
@param[out] capa Pointer to capability structure for output

-Petri

Re: [lng-odp] [PATCH API-NEXT v4 1/1] api: ipsec: add retain header capability

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

Reviewed-by: Petri Savolainen 

> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of
> Github ODP bot
> Sent: Thursday, July 13, 2017 12:00 PM
> To: lng-odp@lists.linaro.org
> Subject: [lng-odp] [PATCH API-NEXT v4 1/1] api: ipsec: add retain header
> capability
> 
> From: Nikhil Agarwal 
> 
> Signed-off-by: Nikhil Agarwal 
> ---
> /** Email created from pull request 75 (NikhilA-Linaro:ipsec_api)
>  ** https://github.com/Linaro/odp/pull/75
>  ** Patch: https://github.com/Linaro/odp/pull/75.patch
>  ** Base sha: ee5be324411a7520528a367967c28fc529d3bc2e
>  ** Merge commit sha: 0aeadccbe282b287d9cad51386cd0993293dce6c
>  **/
>  include/odp/api/spec/ipsec.h | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/include/odp/api/spec/ipsec.h b/include/odp/api/spec/ipsec.h
> index e602e4b8..54491b12 100644
> --- a/include/odp/api/spec/ipsec.h
> +++ b/include/odp/api/spec/ipsec.h
> @@ -262,6 +262,12 @@ typedef struct odp_ipsec_capability_t {
>*/
>   odp_support_t pipeline_cls;
> 
> + /**
> +  * Support of retaining outer headers (retain_outer) in inbound
> inline
> +  * processed packets
> +  */
> + odp_support_t retain_header;
> +
>   /** Maximum number of different destination CoSes in
> classification
>*  pipelining. The same CoS may be used for many SAs. This is
> equal or
>*  less than 'max_cos' capability in classifier API.

Re: [lng-odp] Regarding odp_pktin_rcv() packets

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

> -Original Message-
> From: shally verma [mailto:shallyvermacav...@gmail.com]
> Sent: Thursday, July 13, 2017 11:43 AM
> To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@nokia.com>
> Cc: lng-odp-forward <lng-odp@lists.linaro.org>; Shally Verma
> <sve...@cavium.com>
> Subject: Re: [lng-odp] Regarding odp_pktin_rcv() packets
> 
> On Thu, Jul 13, 2017 at 2:05 PM, Savolainen, Petri (Nokia - FI/Espoo)
> <petri.savolai...@nokia.com> wrote:
> >
> >
> >> -Original Message-
> >> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of
> >> shally verma
> >> Sent: Wednesday, July 12, 2017 3:36 PM
> >> To: lng-odp-forward <lng-odp@lists.linaro.org>; Shally Verma
> >> <sve...@cavium.com>
> >> Subject: [lng-odp] Regarding odp_pktin_rcv() packets
> >>
> >> I have a question on odp_pktin_recv(odp_pktin_queue_t, odp_packet_t [],
> >> int).
> >>
> >> Once app receives packets associated to the queue, what happens to
> >> packets buffers passed to application? Does implementation always
> >> assume that packets will be freed by caller and re-allocates fresh
> >> packets to queue?
> >>
> >> I referred to packet_io.h spec to understand more on this after
> >> pktin_rcv() but didn't get clear information as there seem no
> >> handshake mechanism between caller and implementation to indicate that
> >> packet buffers are free and can be re-used. So looks like there is
> >> implicit assumption that packets are not re-usable after
> >> odp_pktin_rcv() call.
> >>
> >> Could anyone please confirm.
> >>
> >
> > Application owns packets returned by this call (the same way as packets
> returned by queue_deq(), schedule(), alloc() etc calls). Application may
> store those, free those (odp_packet_free()), enqueue those
> (odp_queue_enq()), send those out (odp_pktout_send()), etc.  Application
> owns a packet
> 
> Agree.
> 
> >until it calls an ODP call that takes the ownership back to the
> implementation.
> 
> So, which is this API if user want to give ownership back to pktin
> interface? It is just the odp_packet_free() in case of pktio?


All above take ownership: odp_packet_free(), odp_queue_enq(), odp_pktout_send() 
... and many more. All ODP calls that consume a packet.

It's implementation defined how packet buffers circulate back into input after 
free or pktout_send.

-Petri

Re: [lng-odp] [API-NEXT PATCH] test: l2fwd: list not used features

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

Ping.

I think it's better to modify only this application now, and update other apps 
once the new API is released into master. Otherwise, we would increase diff 
between api-next vs master. Most apps do odp_init_global(, NULL, NULL) 
today, so there's no performance difference. This change enables testing the 
difference with l2fwd.



> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of Petri
> Savolainen
> Sent: Monday, June 26, 2017 1:24 PM
> To: lng-odp@lists.linaro.org
> Subject: [lng-odp] [API-NEXT PATCH] test: l2fwd: list not used features
> 
> List not used features so that performance may be optimized
> on some platforms. E.g. on odp-linux implementation, timer
> polling causes high overhead, which is disabled when timers
> are not used.
> 
> Signed-off-by: Petri Savolainen 
> ---
>  test/common_plat/performance/odp_l2fwd.c | 12 +++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/test/common_plat/performance/odp_l2fwd.c
> b/test/common_plat/performance/odp_l2fwd.c
> index 78b3633b..851614d6 100644
> --- a/test/common_plat/performance/odp_l2fwd.c
> +++ b/test/common_plat/performance/odp_l2fwd.c
> @@ -1387,9 +1387,19 @@ int main(int argc, char *argv[])
>   odp_instance_t instance;
>   int num_groups;
>   odp_schedule_group_t group[MAX_PKTIOS];
> + odp_init_t init;
> +
> + odp_init_param_init();
> +
> + /* List features not to be used (may optimize performance) */
> + init.not_used.feat.cls= 1;
> + init.not_used.feat.crypto = 1;
> + init.not_used.feat.ipsec  = 1;
> + init.not_used.feat.timer  = 1;
> + init.not_used.feat.tm = 1;
> 
>   /* Init ODP before calling anything else */
> - if (odp_init_global(, NULL, NULL)) {
> + if (odp_init_global(, , NULL)) {
>   LOG_ERR("Error: ODP global init failed.\n");
>   exit(EXIT_FAILURE);
>   }
> --
> 2.13.0

Re: [lng-odp] [PATCH 0/3] Change buffer handle to pointer

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

Ping. It seems that Bill review didn't make it to the list.

IPC test problems I did found out during the development are now confirmed on 
current master, and thus are not related to this set.



> -Original Message-
> From: Bill Fischofer [mailto:bill.fischo...@linaro.org]
> Sent: Friday, July 07, 2017 7:18 PM
> To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@nokia.com>
> Cc: Maxim Uvarov <maxim.uva...@linaro.org>
> Subject: Re: [lng-odp] [PATCH 0/3] Change buffer handle to pointer
> 
> For this series:
> 
> Reviewed-and-tested-by: Bill Fischofer <bill.fischo...@linaro.org>
> 

Re: [lng-odp] Regarding odp_pktin_rcv() packets

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

> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of
> shally verma
> Sent: Wednesday, July 12, 2017 3:36 PM
> To: lng-odp-forward ; Shally Verma
> 
> Subject: [lng-odp] Regarding odp_pktin_rcv() packets
> 
> I have a question on odp_pktin_recv(odp_pktin_queue_t, odp_packet_t [],
> int).
> 
> Once app receives packets associated to the queue, what happens to
> packets buffers passed to application? Does implementation always
> assume that packets will be freed by caller and re-allocates fresh
> packets to queue?
> 
> I referred to packet_io.h spec to understand more on this after
> pktin_rcv() but didn't get clear information as there seem no
> handshake mechanism between caller and implementation to indicate that
> packet buffers are free and can be re-used. So looks like there is
> implicit assumption that packets are not re-usable after
> odp_pktin_rcv() call.
> 
> Could anyone please confirm.
> 

Application owns packets returned by this call (the same way as packets 
returned by queue_deq(), schedule(), alloc() etc calls). Application may store 
those, free those (odp_packet_free()), enqueue those (odp_queue_enq()), send 
those out (odp_pktout_send()), etc. Application owns a packet until it calls an 
ODP call that takes the ownership back to the implementation.

-Petri

Re: [lng-odp] [PATCH API-NEXT v1 1/1] Adding additonal capability options to IPSEC APIs.

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

> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of
> Github ODP bot
> Sent: Thursday, July 13, 2017 10:00 AM
> To: lng-odp@lists.linaro.org
> Subject: [lng-odp] [PATCH API-NEXT v1 1/1] Adding additonal capability
> options to IPSEC APIs.

Typo: additional

It would be better to describe the change in subject exactly:

"api: ipsec: add retain header capability"



> 
> From: Nikhil Agarwal 
> 
> Signed-off-by: Nikhil Agarwal 
> ---
> /** Email created from pull request 12 (NikhilA-Linaro:api-next)
>  ** https://github.com/Linaro/odp/pull/12
>  ** Patch: https://github.com/Linaro/odp/pull/12.patch
>  ** Base sha: ee5be324411a7520528a367967c28fc529d3bc2e
>  ** Merge commit sha: 9707a5a9286600a44e06e0a4cb809b6f1fda0a3e
>  **/
>  include/odp/api/spec/ipsec.h | 12 
>  1 file changed, 12 insertions(+)
> 
> diff --git a/include/odp/api/spec/ipsec.h b/include/odp/api/spec/ipsec.h
> index e602e4b8..59272226 100644
> --- a/include/odp/api/spec/ipsec.h
> +++ b/include/odp/api/spec/ipsec.h
> @@ -262,12 +262,24 @@ typedef struct odp_ipsec_capability_t {
>*/
>   odp_support_t pipeline_cls;
> 
> + /**
> +  * Support of retaining outer headers (retain_outer) of
> resulting
> +  * inbound packets
> +  */
> + odp_support_t retain_header;
> +

"Support of retaining outer headers (retain_outer) in inbound inline processed 
packets"

This is more accurate reference to the feature than "...resulting inbound 
packets"


>   /** Maximum number of different destination CoSes in
> classification
>*  pipelining. The same CoS may be used for many SAs. This is
> equal or
>*  less than 'max_cos' capability in classifier API.
>*/
>   uint32_t max_cls_cos;
> 
> + /** Maximum number of different destination queues that can be
> used as
> +  *  IPsec destination queues. The same queue may be used for
> many SAs.
> +  *  This is equal or less than 'max_queue' capability in
> classifier API.
> +  */
> + uint32_t max_ipsec_queues;
> +


This is already defined in my patch set. There's no need to refer to cls API. 
Implementation may set those to be equal, but it's not required by the API.

-Petri

Re: [lng-odp] [PATCH API-NEXT v1 0/3] Crypto API updates

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

Reviewed-by: Petri Savolainen 


> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of
> Github ODP bot
> Sent: Wednesday, July 12, 2017 3:00 PM
> To: lng-odp@lists.linaro.org
> Subject: [lng-odp] [PATCH API-NEXT v1 0/3] Crypto API updates
> 
> Updates for crypto API as suggested by @psavol
> 
> github
> /** Email created from pull request 74 (lumag:crypto-packet)
>  ** https://github.com/Linaro/odp/pull/74
>  ** Patch: https://github.com/Linaro/odp/pull/74.patch
>  ** Base sha: ee5be324411a7520528a367967c28fc529d3bc2e
>  ** Merge commit sha: b7e7b563a7503a65cb3deee63590db065979a939
>  **/
> /github
> 
> checkpatch.pl
> total: 0 errors, 0 warnings, 0 checks, 8 lines checked
> 
> 
> to_send-p-000.patch has no obvious style problems and is ready for
> submission.
> total: 0 errors, 0 warnings, 0 checks, 40 lines checked
> 
> 
> to_send-p-001.patch has no obvious style problems and is ready for
> submission.
> CHECK: Alignment should match open parenthesis
> #160: FILE: include/odp/api/spec/crypto.h:845:
> +int odp_crypto_op(const odp_packet_t pkt_in[],
>odp_packet_t pkt_out[],
> 
> CHECK: Alignment should match open parenthesis
> #169: FILE: include/odp/api/spec/crypto.h:867:
> +int odp_crypto_op_enq(const odp_packet_t pkt_in[],
>const odp_packet_t pkt_out[],
> 
> total: 0 errors, 0 warnings, 2 checks, 324 lines checked
> 
> 
> to_send-p-002.patch has style problems, please review.
> 
> If any of these errors are false positives, please report
> them to the maintainer, see CHECKPATCH in MAINTAINERS.
> /checkpatch.pl

Re: [lng-odp] [API-NEXT PATCH 1/9] api: ipsec: add salt parameter

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

> -Original Message-
> From: Dmitry Eremin-Solenikov [mailto:dmitry.ereminsoleni...@linaro.org]
> Sent: Wednesday, July 12, 2017 11:53 AM
> To: Petri Savolainen ; lng-
> o...@lists.linaro.org
> Subject: Re: [lng-odp] [API-NEXT PATCH 1/9] api: ipsec: add salt parameter
> 
> On 11.07.2017 15:31, Petri Savolainen wrote:
> > Added a parameter for passing salt for AES GCM. Currently,
> > only option for length is 4 bytes, but later on other algorithms
> > may need more/less salt data.
> >
> > Signed-off-by: Petri Savolainen 
> 
> Except the suggestion for PATCH 1/9, the rest of the patches is
> Reviewed-by: Dmitry Eremin-Solenikov 

Sent v2 with 1/9 modified, other patches was not changed. There's more generic 
salt/nonce params, for both cipher and auth.

-Petri

Re: [lng-odp] [PATCH API-NEXT v3 1/3] api: ipsec: pass OUT_INLINE outer headers as const

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

This change is included in my latest IPsec API patch set.

-Petri

Re: [lng-odp] [PATCH API-NEXT v8 6/10] api: crypto: add crypto packet operation interface

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

> diff --git a/include/odp/api/spec/crypto.h b/include/odp/api/spec/crypto.h
> index 454855ea..27b12c89 100644
> --- a/include/odp/api/spec/crypto.h
> +++ b/include/odp/api/spec/crypto.h
> @@ -16,6 +16,7 @@
>  #include 
> 
>  #include 
> +#include 
> 
>  #ifdef __cplusplus
>  extern "C" {
> @@ -276,6 +277,9 @@ typedef struct odp_crypto_session_param_t {
>* data in non-posted mode */
>   odp_crypto_op_mode_t ODP_DEPRECATE(pref_mode);
> 
> + /** Operation mode when using packet interface: sync or async
> */
> + odp_crypto_op_mode_t packet_op_mode;
> +

op_mode is enough. By default deprecated field are not visible, and when user 
enables those he needs to be aware of using only old definition. So, there's no 
need for e.g. prefixes in the new version.


>   /** Cipher algorithm
>*
>*  Use odp_crypto_capability() for supported algorithms.
> @@ -311,16 +315,15 @@ typedef struct odp_crypto_session_param_t {
> 
>   /** Async mode completion event queue
>*
> -  *  When odp_crypto_operation() is asynchronous, the completion
> queue is
> -  *  used to return the completion status of the operation to
> the
> -  *  application.
> +  *  The completion queue is used to return
> odp_crypto_packet_op_enq()
> +  *  results to the application.
>*/
>   odp_queue_t compl_queue;
> 
>   /** Output pool
>*
>*  When the output packet is not specified during the call to
> -  *  odp_crypto_operation(), the output packet will be allocated
> +  *  crypto operation, the output packet will be allocated
>*  from this pool.
>*/
>   odp_pool_t output_pool;
> @@ -400,6 +403,44 @@ typedef struct odp_crypto_op_param_t {
>  typedef odp_crypto_op_param_t ODP_DEPRECATE(odp_crypto_op_params_t);
> 
>  /**
> + * Crypto packet API per packet operation parameters
> + */
> +typedef struct odp_crypto_packet_op_param_t {
> + /** Session handle from creation */
> + odp_crypto_session_t session;
> +
> + /** Override session IV pointer */
> + uint8_t *override_iv_ptr;
> +
> + /** Offset from start of packet for hash result
> +  *
> +  *  Specifies the offset where the hash result is to be stored.
> In case
> +  *  of decode sessions, input hash values will be read from
> this offset,
> +  *  and overwritten with hash results. If this offset lies
> within
> +  *  specified 'auth_range', implementation will mute this field
> before
> +  *  calculating the hash result.
> +  */
> + uint32_t hash_result_offset;
> +
> + /** Additional Authenticated Data (AAD) */
> + struct {
> + /** Pointer to ADD */
> + uint8_t *ptr;
> +
> + /** AAD length in bytes. Use
> odp_crypto_auth_capability() for
> +  *  supported AAD lengths. */
> + uint32_t length;
> + } aad;
> +
> + /** Data range to apply cipher */
> + odp_packet_data_range_t cipher_range;
> +
> + /** Data range to authenticate */
> + odp_packet_data_range_t auth_range;
> +
> +} odp_crypto_packet_op_param_t;


If there are difference in enc/decrypto direction parameters. This type could 
be split in two and named odp_crypto_enc_param_t and _dec_param_t. This would 
avoid packet_op_param_t vs op_param_t, and would be closer to ipsec operations.



> +
> +/**
>   * Crypto API session creation return code
>   */
>  typedef enum {
> @@ -444,14 +485,17 @@ typedef enum {
>  /**
>   * Cryto API per packet operation completion status
>   */
> -typedef struct odp_crypto_compl_status {
> +typedef struct odp_crypto_op_status {
>   /** Algorithm specific return code */
>   odp_crypto_alg_err_t alg_err;
> 
>   /** Hardware specific return code */
>   odp_crypto_hw_err_t  hw_err;
> 
> -} odp_crypto_compl_status_t;
> +} odp_crypto_op_status_t;
> +
> +/** @deprecated  Use ODP_DEPRECATE(odp_crypto_op_status_t) instead */
> +typedef odp_crypto_op_status_t ODP_DEPRECATE(odp_crypto_compl_status_t);
> 
>  /**
>   * Crypto API operation result
> @@ -460,27 +504,51 @@ typedef struct odp_crypto_op_result {
>   /** Request completed successfully */
>   odp_bool_t  ok;
> 
> - /** User context from request */
> - void *ctx;
> + /** User context from request
> +  *
> +  * @deprecated No need to pass context around sync calls
> +  * */
> + void *ODP_DEPRECATE(ctx);
> 
>   /** Output packet */
>   odp_packet_t pkt;
> 
>   /** Cipher status */
> - odp_crypto_compl_status_t cipher_status;
> + odp_crypto_op_status_t cipher_status;
> 
>   /** Authentication status */
> - odp_crypto_compl_status_t auth_status;
> + odp_crypto_op_status_t auth_status;
> 
>  } odp_crypto_op_result_t;

If this type is not needed in new API, deprecate the entire type instead of a 
field.


> 
>  /**
> + * Crypto packet API operation result
> + */
> +typedef struct odp_crypto_packet_result_t {
> + /** Request 

Re: [lng-odp] [PATCH API-NEXT v8 3/10] api: crypto: drop async mode support

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

> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of
> Github ODP bot
> Sent: Wednesday, July 05, 2017 5:00 PM
> To: lng-odp@lists.linaro.org
> Subject: [lng-odp] [PATCH API-NEXT v8 3/10] api: crypto: drop async mode
> support
> 
> From: Dmitry Eremin-Solenikov 
> 
> Current Crypto API provides a way to set the 'preferred' mode of
> operation, without actually telling if the operation will proceed in
> sync or async mode. In preparation of reworking crypto API declare that
> all operations always end up processed in sync mode (posted == FALSE).
> 
> Signed-off-by: Dmitry Eremin-Solenikov 
> ---
> /** Email created from pull request 64 (lumag:crypto-packet)
>  ** https://github.com/Linaro/odp/pull/64
>  ** Patch: https://github.com/Linaro/odp/pull/64.patch
>  ** Base sha: 00a21c6dce65a30c8250db59a42a43c658e8ca1b
>  ** Merge commit sha: ac9b299a7f3bee72dd9343bbfaa826217d243ea6
>  **/
>  include/odp/api/spec/crypto.h  | 10 ++--
>  .../linux-generic/include/odp_crypto_internal.h|  8 ---
>  .../linux-generic/include/odp_packet_internal.h|  4 --
>  platform/linux-generic/odp_crypto.c| 61 +
> -
>  platform/linux-generic/odp_packet.c|  1 -
>  5 files changed, 16 insertions(+), 68 deletions(-)
> 
> diff --git a/include/odp/api/spec/crypto.h b/include/odp/api/spec/crypto.h
> index 470cba05..01b15d6b 100644
> --- a/include/odp/api/spec/crypto.h
> +++ b/include/odp/api/spec/crypto.h
> @@ -640,14 +640,12 @@ void odp_crypto_compl_free(odp_crypto_compl_t
> completion_event);
>   * Crypto per packet operation
>   *
>   * Performs the cryptographic operations specified during session
> creation
> - * on the packet.  If the operation is performed synchronously, "posted"
> - * will return FALSE and the result of the operation is immediately
> available.
> - * If "posted" returns TRUE the result will be delivered via the
> completion
> - * queue specified when the session was created.
> + * on the packet. The operation is performed synchronously, the result of
> the
> + * operation is immediately available.
>   *
>   * @param param Operation parameters
> - * @param postedPointer to return posted, TRUE for async
> operation
> - * @param resultResults of operation (when posted returns
> FALSE)
> + * @param postedAlways returns FALSE
> + * @param resultResults of operation
>   *

If I remember correctly, Bala requested to maintain async operation in the old 
API.

It's OK to remove async implementation from odp-linux, but old API should 
remain as is (behind deprecated macros).

- Petri

Re: [lng-odp] [API-NEXT PATCH 1/4] linux-gen: sched: remove schedule interface depedency to qentry

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

> -Original Message-
> From: Honnappa Nagarahalli [mailto:honnappa.nagaraha...@linaro.org]
> Sent: Friday, July 07, 2017 9:28 PM
> To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@nokia.com>
> Cc: lng-odp-forward <lng-odp@lists.linaro.org>
> Subject: Re: [lng-odp] [API-NEXT PATCH 1/4] linux-gen: sched: remove
> schedule interface depedency to qentry
> 
> On 7 July 2017 at 01:46, Savolainen, Petri (Nokia - FI/Espoo)
> <petri.savolai...@nokia.com> wrote:
> >> >>  typedef struct schedule_fn_t {
> >> >> +   int status_sync;
> >> >
> >> > this structure should contain functions that are provided by
> scheduler
> >> > to other components of ODP. 'status_sync' seems to be an internal
> >> > mechanism between the default scheduler and default queue. Hence it
> >> > should not be here.
> >> >
> >> Any update on this comment?
> >
> > I did answer it already.
> 
> Ok, found your answer. Should this variable be moved to queue internal
> structure which is set only for iQuery scheduler?
> 
> This structure should contain only the functions exposed by the
> scheduler to other components of ODP. It should not contain anything
> related to the interface between queue and scheduler (they are being
> considered as a single module).

These functions are called from queue, so it's queue -> iquery interface 
(scheduler interface, not queue interface). These functions can be removed (if 
iquery is not anymore maintained) or moved as a next step, but that's out of 
scope of this patch set. This set simply removes unused functions and minimizes 
number of functions calls, but does not move functions from one (interface) 
file to another. This set removes dependency to those iquery specific functions 
already from all other code except queue and iquery, so removing / moving those 
is easier after this is merged.

-Petri

Re: [lng-odp] [PATCH v4 3/9] linux-gen: stop poisoning CPPFLAGS/LDFLAGS with DPDK flags

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

From: Maxim Uvarov [mailto:maxim.uva...@linaro.org] 
Sent: Thursday, July 06, 2017 8:28 PM
To: Dmitry Eremin-Solenikov <dmitry.ereminsoleni...@linaro.org>
Cc: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@nokia.com>; Github 
ODP bot <odp...@yandex.ru>; lng-odp@lists.linaro.org
Subject: Re: [lng-odp] [PATCH v4 3/9] linux-gen: stop poisoning 
CPPFLAGS/LDFLAGS with DPDK flags

Petri, are you ok with that serries? Patch 9/9 is the major goal for odp 
packaging.
p.s. as for me I don't have objections to merge it.
Maxim.


I'm OK with it.

-Petri

Re: [lng-odp] [API-NEXT PATCH 0/4] Clean up scheduler interface

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

This is the first step to clean up queue / scheduler dependencies. Could we 
merge this now and continue towards the next steps. Master and api-next should 
be synced soon otherwise the code base delta just increases for no real reason 
(API is the same).

-Petri

> -Original Message-
> From: Bill Fischofer [mailto:bill.fischo...@linaro.org]
> Sent: Friday, June 30, 2017 6:51 PM
> To: Petri Savolainen 
> Cc: lng-odp-forward 
> Subject: Re: [lng-odp] [API-NEXT PATCH 0/4] Clean up scheduler interface
> 
> For this series:
> 
> Reviewed-and-tested-by: Bill Fischofer 
> 
> On Fri, Jun 30, 2017 at 9:10 AM, Petri Savolainen
>  wrote:
> > First removed dependency to queue internals from scheduler interface.
> Then
> > removed almost all references to queue internals from default and iquery
> > schedulers. This required move of ordered queue data structure from
> queue to
> > scheduler files. The last references require queue / pktio interface
> change and
> > thus is left as the next step. Scheduler performance increased couple of
> > percents.
> >
> >
> > Petri Savolainen (4):
> >   linux-gen: sched: remove schedule interface depedency to qentry
> >   linux-gen: sched: use config max ordered locks
> >   linux-gen: sched: remove most dependecies to qentry
> >   linux-gen: sched: remove unused sched interface functions
> >
> >  .../linux-generic/include/odp_config_internal.h|   2 +-
> >  .../linux-generic/include/odp_queue_internal.h |   7 -
> >  platform/linux-generic/include/odp_schedule_if.h   |  13 +-
> >  platform/linux-generic/odp_queue.c |  68 +
> >  platform/linux-generic/odp_schedule.c  | 161 +-
> ---
> >  platform/linux-generic/odp_schedule_iquery.c   | 158 --
> --
> >  platform/linux-generic/odp_schedule_sp.c   |  18 +--
> >  7 files changed, 211 insertions(+), 216 deletions(-)
> >
> > --
> > 2.13.0
> >

Re: [lng-odp] [API-NEXT PATCH 1/4] linux-gen: sched: remove schedule interface depedency to qentry

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

> >>  typedef struct schedule_fn_t {
> >> +   int status_sync;
> >
> > this structure should contain functions that are provided by scheduler
> > to other components of ODP. 'status_sync' seems to be an internal
> > mechanism between the default scheduler and default queue. Hence it
> > should not be here.
> >
> Any update on this comment?

I did answer it already.

Re: [lng-odp] [API-NEXT PATCH 1/4] linux-gen: sched: remove schedule interface depedency to qentry

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

> -Original Message-
> From: Honnappa Nagarahalli [mailto:honnappa.nagaraha...@linaro.org]
> Sent: Thursday, July 06, 2017 11:07 PM
> To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@nokia.com>
> Cc: lng-odp-forward <lng-odp@lists.linaro.org>
> Subject: Re: [lng-odp] [API-NEXT PATCH 1/4] linux-gen: sched: remove
> schedule interface depedency to qentry
> 
> On 5 July 2017 at 01:31, Savolainen, Petri (Nokia - FI/Espoo)
> <petri.savolai...@nokia.com> wrote:
> >
> >
> >> -Original Message-
> >> From: Honnappa Nagarahalli [mailto:honnappa.nagaraha...@linaro.org]
> >> Sent: Wednesday, July 05, 2017 7:04 AM
> >> To: Petri Savolainen <petri.savolai...@linaro.org>
> >> Cc: lng-odp-forward <lng-odp@lists.linaro.org>
> >> Subject: Re: [lng-odp] [API-NEXT PATCH 1/4] linux-gen: sched: remove
> >> schedule interface depedency to qentry
> >>
> >> On 30 June 2017 at 09:10, Petri Savolainen
> <petri.savolai...@linaro.org>
> >> wrote:
> >> > Do not use queue internal type in schedule interface.
> >> >
> >> > Signed-off-by: Petri Savolainen <petri.savolai...@linaro.org>
> >> > ---
> >> >  platform/linux-generic/include/odp_schedule_if.h |  8 +++--
> >> >  platform/linux-generic/odp_queue.c   |  9 --
> >> >  platform/linux-generic/odp_schedule.c| 18 ---
> >> >  platform/linux-generic/odp_schedule_iquery.c | 41 +-
> ---
> >> ---
> >> >  platform/linux-generic/odp_schedule_sp.c | 18 +++
> >> >  5 files changed, 45 insertions(+), 49 deletions(-)
> >> >
> >> > diff --git a/platform/linux-generic/include/odp_schedule_if.h
> >> b/platform/linux-generic/include/odp_schedule_if.h
> >> > index 5877a1cd..5abbb732 100644
> >> > --- a/platform/linux-generic/include/odp_schedule_if.h
> >> > +++ b/platform/linux-generic/include/odp_schedule_if.h
> >> > @@ -35,9 +35,10 @@ typedef int (*schedule_term_local_fn_t)(void);
> >> >  typedef void (*schedule_order_lock_fn_t)(void);
> >> >  typedef void (*schedule_order_unlock_fn_t)(void);
> >> >  typedef unsigned (*schedule_max_ordered_locks_fn_t)(void);
> >> > -typedef void (*schedule_save_context_fn_t)(queue_entry_t *queue);
> >> > +typedef void (*schedule_save_context_fn_t)(uint32_t queue_index,
> void
> >> *ptr);
> >> >
> >> >  typedef struct schedule_fn_t {
> >> > +   int status_sync;
> >>
> >> this structure should contain functions that are provided by scheduler
> >> to other components of ODP. 'status_sync' seems to be an internal
> >> mechanism between the default scheduler and default queue. Hence it
> >> should not be here.
> >
> > This flags if unsched_queue() and save_context() needs to be called.
> Those calls are only needed by iquery scheduler. With this flag, queue
> needs to check only single variable if those calls are needed or not.
> Today, these calls are made always, which hurt performance.
> >
> >>
> >> > schedule_pktio_start_fn_t   pktio_start;
> >> > schedule_thr_add_fn_t   thr_add;
> >> > schedule_thr_rem_fn_t   thr_rem;
> >> > @@ -45,7 +46,6 @@ typedef struct schedule_fn_t {
> >> > schedule_init_queue_fn_tinit_queue;
> >> > schedule_destroy_queue_fn_t destroy_queue;
> >> > schedule_sched_queue_fn_t   sched_queue;
> >> > -   schedule_unsched_queue_fn_t unsched_queue;
> >> these queue related functions are not used by other components within
> >> ODP. These are specific to default queue and default schedulers. These
> >> should not be part of this file.
> >
> > Didn't add or remove those functions in this patch set. This discussion
> can be done in context of another patch set.
> >
> Are you just cleaning up the scheduler and queue interface in this
> patch? If not this change can be taken up in this patch.

This patch set cleans odp_schedule_if.h interface, which currently defines 
input and output interfaces for the scheduler. This set does not redesign the 
interface but cleans and optimizes the usage. The main point is to remove type 
dependencies, functions and function calls that are not needed.

These functions above are needed only by iquery. This change removes empty 
function defines and extra calls when iquery is not used. One option to get 
rid-off these functions is to remove iquery if it's not developed anymore.

-Petri

Re: [lng-odp] [API-NEXT PATCH 2/4] linux-gen: sched: use config max ordered locks

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

> -Original Message-
> From: Honnappa Nagarahalli [mailto:honnappa.nagaraha...@linaro.org]
> Sent: Thursday, July 06, 2017 11:00 PM
> To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@nokia.com>
> Cc: lng-odp-forward <lng-odp@lists.linaro.org>
> Subject: Re: [lng-odp] [API-NEXT PATCH 2/4] linux-gen: sched: use config
> max ordered locks
> 
> On 5 July 2017 at 01:35, Savolainen, Petri (Nokia - FI/Espoo)
> <petri.savolai...@nokia.com> wrote:
> >
> >> > diff --git a/platform/linux-generic/include/odp_config_internal.h
> >> b/platform/linux-generic/include/odp_config_internal.h
> >> > index 3cff0045..469396df 100644
> >> > --- a/platform/linux-generic/include/odp_config_internal.h
> >> > +++ b/platform/linux-generic/include/odp_config_internal.h
> >> > @@ -27,7 +27,7 @@
> >> >  /*
> >> >   * Maximum number of ordered locks per queue
> >> >   */
> >> > -#define CONFIG_QUEUE_MAX_ORD_LOCKS 4
> >> > +#define CONFIG_QUEUE_MAX_ORD_LOCKS 2
> >>
> >> This is unnecessary change for this patch. This patch does not need
> this
> >> change.
> >
> > With this value (2), the current situation does not change. Internal
> defines limited the number into 2, so this keeps it 2.
> 
> This change affects other implementations of the scheduler. If the
> default scheduler is implemented independent of what the value of
> CONFIG_QUEUE_MAX_ORD_LOCKS is, this change is not required.
> This change should not be necessary for this patch.

I try to keep properties as they were before the patch. I have not heard 
anybody complaining that 2 locks is not sufficient. Have you? We can later 
expand the value if needed. Your scheduler is not widely used yet (it's been in 
api-next a week or so), so people using this feature have not been exposed to 
number of locks being 4.

It was 2, I keep it 2 and we can increase it later if someone needs a larger 
value.

-Petri

Re: [lng-odp] Suspected SPAM - [API-NEXT PATCH v2 1/3] api: system_info: add function for fetching all supported huge page sizes

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

Reviewed-by: Petri Savolainen 

> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of
> Matias Elo
> Sent: Wednesday, July 05, 2017 5:23 PM
> To: lng-odp@lists.linaro.org
> Subject: Suspected SPAM - [lng-odp] [API-NEXT PATCH v2 1/3] api:
> system_info: add function for fetching all supported huge page sizes
> 
> A system may simultaneously support multiple huge page sizes. Add a new
> API
> function odp_sys_huge_page_size_all() which returns all supported page
> sizes. odp_sys_huge_page_size() stays unmodified to maintain backward
> compatibility.
> 
> Signed-off-by: Matias Elo 
> ---
> V2:
> - Changed return value and 'num' param from unsigned to int (Maxim)
> 
> 
>  include/odp/api/spec/system_info.h | 19 +++
>  1 file changed, 19 insertions(+)

Re: [lng-odp] [PATCHv2] linux-gen: scheduler: modular scheduler interface

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

> -Original Message-
> From: Honnappa Nagarahalli [mailto:honnappa.nagaraha...@linaro.org]
> Sent: Wednesday, July 05, 2017 6:31 AM
> To: Brian Brooks <brian.bro...@arm.com>
> Cc: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@nokia.com>;
> Joyce Kong <joyce.k...@arm.com>; lng-odp@lists.linaro.org
> Subject: Re: [lng-odp] [PATCHv2] linux-gen: scheduler: modular scheduler
> interface
> 
> The intention of this patch is to clean up odp_schedule_if.h file
> alone, there is no other cleanup being done. This file contains
> function declarations that should not be in this file. For ex: pkt io
> functions and queue functions that are for default scheduler and
> default queue interaction.
> 
> If this goal is not clear the commit message can be changed.
> 
> As agreed in the last call, instead of moving the pkt I/O functions to
> pkt I/O internal header file, we will explore creating a new file
> odp_packet_io_if.h and place the function declarations in that file.
> 
> The queue internal functions will be moved to odp_queue_internal.h as
> has been done in this patch.
> 

Not into odp_queue_internal.h but into odp_queue_sched_if.h, which keeps it 
clear that queue internals (qentry->s.xxx) must not be used inside scheduler.

-Petri

Re: [lng-odp] [API-NEXT PATCH 4/4] linux-gen: sched: remove unused sched interface functions

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

> >  platform/linux-generic/include/odp_schedule_if.h |  7 +--
> >  platform/linux-generic/odp_queue.c   | 21 +
> 
> >  platform/linux-generic/odp_schedule_iquery.c |  4 +---
> >  3 files changed, 3 insertions(+), 29 deletions(-)
> >
> > diff --git a/platform/linux-generic/include/odp_schedule_if.h
> b/platform/linux-generic/include/odp_schedule_if.h
> > index 5abbb732..b514c88a 100644
> > --- a/platform/linux-generic/include/odp_schedule_if.h
> > +++ b/platform/linux-generic/include/odp_schedule_if.h
> > @@ -35,7 +35,7 @@ typedef int (*schedule_term_local_fn_t)(void);
> >  typedef void (*schedule_order_lock_fn_t)(void);
> >  typedef void (*schedule_order_unlock_fn_t)(void);
> >  typedef unsigned (*schedule_max_ordered_locks_fn_t)(void);
> > -typedef void (*schedule_save_context_fn_t)(uint32_t queue_index, void
> *ptr);
> > +typedef void (*schedule_save_context_fn_t)(uint32_t queue_index);
> >
> >  typedef struct schedule_fn_t {
> > int status_sync;
> > @@ -68,11 +68,6 @@ extern const schedule_fn_t *sched_fn;
> >  int sched_cb_pktin_poll(int pktio_index, int num_queue, int index[]);
> >  void sched_cb_pktio_stop_finalize(int pktio_index);
> >  int sched_cb_num_pktio(void);
> > -int sched_cb_num_queues(void);
> > -int sched_cb_queue_prio(uint32_t queue_index);
> > -int sched_cb_queue_grp(uint32_t queue_index);
> > -int sched_cb_queue_is_ordered(uint32_t queue_index);
> > -int sched_cb_queue_is_atomic(uint32_t queue_index);
> These changes are captured by Joyce's patch already.

No, Joyce did not remove these, but just moved around.

This patch can remove these as unused, due to all other changes I made.


-Petri

Re: [lng-odp] [API-NEXT PATCH 2/4] linux-gen: sched: use config max ordered locks

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

> > diff --git a/platform/linux-generic/include/odp_config_internal.h
> b/platform/linux-generic/include/odp_config_internal.h
> > index 3cff0045..469396df 100644
> > --- a/platform/linux-generic/include/odp_config_internal.h
> > +++ b/platform/linux-generic/include/odp_config_internal.h
> > @@ -27,7 +27,7 @@
> >  /*
> >   * Maximum number of ordered locks per queue
> >   */
> > -#define CONFIG_QUEUE_MAX_ORD_LOCKS 4
> > +#define CONFIG_QUEUE_MAX_ORD_LOCKS 2
> 
> This is unnecessary change for this patch. This patch does not need this
> change.

With this value (2), the current situation does not change. Internal defines 
limited the number into 2, so this keeps it 2.

-Petri

Re: [lng-odp] [API-NEXT PATCH 1/4] linux-gen: sched: remove schedule interface depedency to qentry

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

> -Original Message-
> From: Honnappa Nagarahalli [mailto:honnappa.nagaraha...@linaro.org]
> Sent: Wednesday, July 05, 2017 7:04 AM
> To: Petri Savolainen 
> Cc: lng-odp-forward 
> Subject: Re: [lng-odp] [API-NEXT PATCH 1/4] linux-gen: sched: remove
> schedule interface depedency to qentry
> 
> On 30 June 2017 at 09:10, Petri Savolainen 
> wrote:
> > Do not use queue internal type in schedule interface.
> >
> > Signed-off-by: Petri Savolainen 
> > ---
> >  platform/linux-generic/include/odp_schedule_if.h |  8 +++--
> >  platform/linux-generic/odp_queue.c   |  9 --
> >  platform/linux-generic/odp_schedule.c| 18 ---
> >  platform/linux-generic/odp_schedule_iquery.c | 41 +
> ---
> >  platform/linux-generic/odp_schedule_sp.c | 18 +++
> >  5 files changed, 45 insertions(+), 49 deletions(-)
> >
> > diff --git a/platform/linux-generic/include/odp_schedule_if.h
> b/platform/linux-generic/include/odp_schedule_if.h
> > index 5877a1cd..5abbb732 100644
> > --- a/platform/linux-generic/include/odp_schedule_if.h
> > +++ b/platform/linux-generic/include/odp_schedule_if.h
> > @@ -35,9 +35,10 @@ typedef int (*schedule_term_local_fn_t)(void);
> >  typedef void (*schedule_order_lock_fn_t)(void);
> >  typedef void (*schedule_order_unlock_fn_t)(void);
> >  typedef unsigned (*schedule_max_ordered_locks_fn_t)(void);
> > -typedef void (*schedule_save_context_fn_t)(queue_entry_t *queue);
> > +typedef void (*schedule_save_context_fn_t)(uint32_t queue_index, void
> *ptr);
> >
> >  typedef struct schedule_fn_t {
> > +   int status_sync;
> 
> this structure should contain functions that are provided by scheduler
> to other components of ODP. 'status_sync' seems to be an internal
> mechanism between the default scheduler and default queue. Hence it
> should not be here.

This flags if unsched_queue() and save_context() needs to be called. Those 
calls are only needed by iquery scheduler. With this flag, queue needs to check 
only single variable if those calls are needed or not. Today, these calls are 
made always, which hurt performance.

> 
> > schedule_pktio_start_fn_t   pktio_start;
> > schedule_thr_add_fn_t   thr_add;
> > schedule_thr_rem_fn_t   thr_rem;
> > @@ -45,7 +46,6 @@ typedef struct schedule_fn_t {
> > schedule_init_queue_fn_tinit_queue;
> > schedule_destroy_queue_fn_t destroy_queue;
> > schedule_sched_queue_fn_t   sched_queue;
> > -   schedule_unsched_queue_fn_t unsched_queue;
> these queue related functions are not used by other components within
> ODP. These are specific to default queue and default schedulers. These
> should not be part of this file.

Didn't add or remove those functions in this patch set. This discussion can be 
done in context of another patch set.

-Petri

> 
> > schedule_ord_enq_multi_fn_t ord_enq_multi;
> > schedule_init_global_fn_t   init_global;
> > schedule_term_global_fn_t   term_global;
> > @@ -54,7 +54,11 @@ typedef struct schedule_fn_t {
> > schedule_order_lock_fn_torder_lock;
> > schedule_order_unlock_fn_t  order_unlock;
> > schedule_max_ordered_locks_fn_t max_ordered_locks;
> > +
> > +   /* Called only when status_sync is set */
> > +   schedule_unsched_queue_fn_t unsched_queue;
> > schedule_save_context_fn_t  save_context;
> > +
> >  } schedule_fn_t;

Re: [lng-odp] [PATCH v4 3/9] linux-gen: stop poisoning CPPFLAGS/LDFLAGS with DPDK flags

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

> -Original Message-
> From: Dmitry Eremin-Solenikov [mailto:dmitry.ereminsoleni...@linaro.org]
> Sent: Tuesday, July 04, 2017 3:16 PM
> To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@nokia.com>;
> Github ODP bot <odp...@yandex.ru>; lng-odp@lists.linaro.org
> Subject: Re: [lng-odp] [PATCH v4 3/9] linux-gen: stop poisoning
> CPPFLAGS/LDFLAGS with DPDK flags
> 
> On 03.07.2017 13:34, Savolainen, Petri (Nokia - FI/Espoo) wrote:
> >> diff --git a/test/Makefile.inc b/test/Makefile.inc
> >> index 1ef2a92c..bf31b374 100644
> >> --- a/test/Makefile.inc
> >> +++ b/test/Makefile.inc
> >> @@ -4,7 +4,7 @@ LIB   = $(top_builddir)/lib
> >>  #in the following line, the libs using the symbols should come before
> >>  #the libs containing them! The includer is given a chance to add
> things
> >>  #before libodp by setting PRE_LDADD before the inclusion.
> >> -LDADD = $(PRE_LDADD) $(LIB)/libodphelper.la $(LIB)/libodp-linux.la
> >> +LDADD = $(PRE_LDADD) $(LIB)/libodphelper.la $(LIB)/libodp-linux.la
> >> $(DPDK_PMDS)
> >
> > Application using ODP should only need to add dependency to ODP and
> helper libs. It's not scalable if (all) applications need to know which
> (all) libs an ODP implementation may use internally.
> >
> > I guess this solves some DPDK linking issues, but is there a way to
> avoid explicit dependency to ODP lib internals ?
> 
> Just to rephrase/emphasize my answer: user applications will use
> pkg-config file, which contains all necessary data to link with DPDK w/o
> mentioning it specifically. The only issue is internal linking inside
> ODP or linking using libtool.
> 

In principle, our example/validation apps should see ODP the same way as any 
other app. That's why it seems odd that DPDK libs need to be exposed when 
linking those (or are those needed because of the test/linux-generic folder).

My main concern is that external applications (like OFP) would need to add the 
same modification to their linking rules. That seems not to be the case, so I 
guess it's OK then.

-Petri

Re: [lng-odp] [PATCH API-NEXT v5 6/10] api: crypto: add crypto packet operation interface

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

> diff --git a/include/odp/api/spec/crypto.h b/include/odp/api/spec/crypto.h
> index 454855ea..9dd60749 100644
> --- a/include/odp/api/spec/crypto.h
> +++ b/include/odp/api/spec/crypto.h
> @@ -16,6 +16,7 @@
>  #include 
> 
>  #include 
> +#include 
> 
>  #ifdef __cplusplus
>  extern "C" {
> @@ -276,6 +277,9 @@ typedef struct odp_crypto_session_param_t {
>* data in non-posted mode */
>   odp_crypto_op_mode_t ODP_DEPRECATE(pref_mode);
> 
> + /** Operation mode when using packet interface: sync or async
> */
> + odp_crypto_op_mode_t packet_op_mode;
> +

I think "packet" is not needed here, just "op_mode".


>   /** Cipher algorithm
>*
>*  Use odp_crypto_capability() for supported algorithms.
> @@ -311,16 +315,15 @@ typedef struct odp_crypto_session_param_t {
> 
>   /** Async mode completion event queue
>*
> -  *  When odp_crypto_operation() is asynchronous, the completion
> queue is
> -  *  used to return the completion status of the operation to
> the
> -  *  application.
> +  *  The completion queue is used to return
> odp_crypto_packet_op_enq()
> +  *  results to the application.

"... return results from asynchronous operation ..." is sufficient. We don't 
need to update if there will be other functions sending events into this queue 
later on.


>*/
>   odp_queue_t compl_queue;
> 
>   /** Output pool
>*
>*  When the output packet is not specified during the call to
> -  *  odp_crypto_operation(), the output packet will be allocated
> +  *  crypto operation, the output packet will be allocated
>*  from this pool.
>*/
>   odp_pool_t output_pool;
> @@ -400,6 +403,44 @@ typedef struct odp_crypto_op_param_t {
>  typedef odp_crypto_op_param_t ODP_DEPRECATE(odp_crypto_op_params_t);
> 
>  /**
> + * Crypto packet API per packet operation parameters
> + */
> +typedef struct odp_crypto_packet_op_param_t {
> + /** Session handle from creation */
> + odp_crypto_session_t session;
> +
> + /** Override session IV pointer */
> + uint8_t *override_iv_ptr;
> +
> + /** Offset from start of packet for hash result
> +  *
> +  *  Specifies the offset where the hash result is to be stored.
> In case
> +  *  of decode sessions, input hash values will be read from
> this offset,
> +  *  and overwritten with hash results. If this offset lies
> within
> +  *  specified 'auth_range', implementation will mute this field
> before
> +  *  calculating the hash result.
> +  */
> + uint32_t hash_result_offset;
> +
> + /** Additional Authenticated Data (AAD) */
> + struct {
> + /** Pointer to ADD */
> + uint8_t *ptr;
> +
> + /** AAD length in bytes. Use
> odp_crypto_auth_capability() for
> +  *  supported AAD lengths. */
> + uint32_t length;
> + } aad;
> +
> + /** Data range to apply cipher */
> + odp_packet_data_range_t cipher_range;
> +
> + /** Data range to authenticate */
> + odp_packet_data_range_t auth_range;
> +
> +} odp_crypto_packet_op_param_t;

Again, I'd have it simply "odp_crypto_op_param_t", but that's reserved by old 
API. This would need some creativity to remove "packet" in this case.


> +
> +/**
>   * Crypto API session creation return code
>   */
>  typedef enum {
> @@ -444,14 +485,17 @@ typedef enum {
>  /**
>   * Cryto API per packet operation completion status
>   */
> -typedef struct odp_crypto_compl_status {
> +typedef struct odp_crypto_op_status {
>   /** Algorithm specific return code */
>   odp_crypto_alg_err_t alg_err;
> 
>   /** Hardware specific return code */
>   odp_crypto_hw_err_t  hw_err;
> 
> -} odp_crypto_compl_status_t;
> +} odp_crypto_op_status_t;
> +
> +/** @deprecated  Use ODP_DEPRECATE(odp_crypto_op_status_t) instead */
> +typedef odp_crypto_op_status_t ODP_DEPRECATE(odp_crypto_compl_status_t);
> 
>  /**
>   * Crypto API operation result
> @@ -460,27 +504,51 @@ typedef struct odp_crypto_op_result {
>   /** Request completed successfully */
>   odp_bool_t  ok;
> 
> - /** User context from request */
> - void *ctx;
> + /** User context from request
> +  *
> +  * @deprecated No need to pass context around sync calls
> +  * */
> + void *ODP_DEPRECATE(ctx);
> 
>   /** Output packet */
>   odp_packet_t pkt;
> 
>   /** Cipher status */
> - odp_crypto_compl_status_t cipher_status;
> + odp_crypto_op_status_t cipher_status;
> 
>   /** Authentication status */
> - odp_crypto_compl_status_t auth_status;
> + odp_crypto_op_status_t auth_status;
> 
>  } odp_crypto_op_result_t;
> 
>  /**
> + * Crypto packet API operation result
> + */
> +typedef struct odp_crypto_packet_op_result_t {
> + /** Request completed successfully */
> + odp_bool_t  ok;
> +
> + /** Cipher status */
> + odp_crypto_op_status_t cipher_status;
> +
> + /** Authentication 

Re: [lng-odp] [PATCH v4 3/9] linux-gen: stop poisoning CPPFLAGS/LDFLAGS with DPDK flags

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

> diff --git a/test/Makefile.inc b/test/Makefile.inc
> index 1ef2a92c..bf31b374 100644
> --- a/test/Makefile.inc
> +++ b/test/Makefile.inc
> @@ -4,7 +4,7 @@ LIB   = $(top_builddir)/lib
>  #in the following line, the libs using the symbols should come before
>  #the libs containing them! The includer is given a chance to add things
>  #before libodp by setting PRE_LDADD before the inclusion.
> -LDADD = $(PRE_LDADD) $(LIB)/libodphelper.la $(LIB)/libodp-linux.la
> +LDADD = $(PRE_LDADD) $(LIB)/libodphelper.la $(LIB)/libodp-linux.la
> $(DPDK_PMDS)

Application using ODP should only need to add dependency to ODP and helper 
libs. It's not scalable if (all) applications need to know which (all) libs an 
ODP implementation may use internally.

I guess this solves some DPDK linking issues, but is there a way to avoid 
explicit dependency to ODP lib internals ?

-Petri

Re: [lng-odp] [API-NEXTv2] api: ipsec: reorganize odp_ipsec_sa_param_t structure based on SA direction

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

> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of
> Bogdan Pricope
> Sent: Thursday, June 22, 2017 9:56 AM
> To: lng-odp@lists.linaro.org
> Subject: [lng-odp] [API-NEXTv2] api: ipsec: reorganize
> odp_ipsec_sa_param_t structure based on SA direction
> 
> Signed-off-by: Bogdan Pricope 
> ---
>  include/odp/api/spec/ipsec.h | 114 --
> -
>  1 file changed, 63 insertions(+), 51 deletions(-)
> 
> diff --git a/include/odp/api/spec/ipsec.h b/include/odp/api/spec/ipsec.h
> index e602e4b..5373ede 100644
> --- a/include/odp/api/spec/ipsec.h
> +++ b/include/odp/api/spec/ipsec.h
> @@ -604,8 +604,8 @@ typedef enum odp_ipsec_ip_version_t {
>   * IPSEC Security Association (SA) parameters
>   */
>  typedef struct odp_ipsec_sa_param_t {
> - /** IPSEC SA direction: inbound or outbound */
> - odp_ipsec_dir_t dir;

Direction is important for selecting parameters. It should remain the first 
field of the struct.


> + /** SPI value */
> + uint32_t spi;

SPI is simple value and should remain after more complex configuration options, 
which define how the SA works.


> 
>   /** IPSEC protocol: ESP or AH */
>   odp_ipsec_protocol_t proto;
> @@ -616,51 +616,12 @@ typedef struct odp_ipsec_sa_param_t {
>   /** Parameters for crypto and authentication algorithms */
>   odp_ipsec_crypto_param_t crypto;
> 
> - /** Parameters for tunnel mode */
> - odp_ipsec_tunnel_param_t tunnel;
> -
> - /** Fragmentation mode */
> - odp_ipsec_frag_mode_t frag_mode;
> -
> - /** Various SA option flags */
> - odp_ipsec_sa_opt_t opt;
> -

Keep opt here before the union. The inbound/outbound union should be the last 
thing in the struct. Everything before it is common for both directions.


>   /** SA lifetime parameters */
>   odp_ipsec_lifetime_t lifetime;
> 
> - /** SA lookup mode */
> - odp_ipsec_lookup_mode_t lookup_mode;
> -
> - /** Minimum anti-replay window size. Use 0 to disable anti-
> replay
> -   * service. */
> - uint32_t antireplay_ws;
> -
>   /** Initial sequence number */
>   uint64_t seq;

This may be moved to outbound struct.


> 
> - /** SPI value */
> - uint32_t spi;
> -
> - /** Additional inbound SA lookup parameters. Values are
> considered
> -  *  only in ODP_IPSEC_LOOKUP_DSTADDR_SPI lookup mode. */
> - struct {
> - /** Select IP version
> -  */
> - odp_ipsec_ip_version_t ip_version;
> -
> - /** IP destination address (NETWORK ENDIAN) */
> - void*dst_addr;
> -
> - } lookup_param;
> -
> - /** MTU for outbound IP fragmentation offload
> -  *
> -  *  This is the maximum length of IP packets that outbound
> IPSEC
> -  *  operations may produce. The value may be updated later with
> -  *  odp_ipsec_mtu_update().
> -  */
> - uint32_t mtu;
> -
>   /** Select pipelined destination for resulting events
>*
>*  Asynchronous and inline modes generate events. Select where
> @@ -677,16 +638,67 @@ typedef struct odp_ipsec_sa_param_t {
>*/
>   odp_queue_t dest_queue;
> 
> - /** Classifier destination CoS for resulting packets
> -  *
> -  *  Successfully decapsulated packets are sent to
> classification
> -  *  through this CoS. Other resulting events are sent to
> 'dest_queue'.
> -  *  This field is considered only when 'pipeline' is
> -  *  ODP_IPSEC_PIPELINE_CLS. The CoS must not be shared between
> any pktio
> -  *  interface default CoS. The maximum number of different CoS
> supported
> -  *  is defined by IPSEC capability max_cls_cos.
> -  */
> - odp_cos_t dest_cos;
> + /** IPSEC SA direction: inbound or outbound */
> + odp_ipsec_dir_t dir;
> +
> + /** IPSEC SA direction dependent parameters */
> + union {
> + /** Inbound specific parameters */
> + struct {
> + /** SA lookup mode */
> + odp_ipsec_lookup_mode_t lookup_mode;
> +
> + /** Additional inbound SA lookup
> parameters. Values are
> + *  considered only in
> ODP_IPSEC_LOOKUP_DSTADDR_SPI
> + *  lookup mode. */
> + struct {
> + /** Select IP version
> + */
> + odp_ipsec_ip_version_t
> ip_version;
> +
> + /** IP destination address
> (NETWORK ENDIAN) */
> + void*dst_addr;
> +
> + } lookup_param;
> +
> + /** Minimum anti-replay window size. Use 0
> to disable
> + *  anti-replay service. */
> + uint32_t antireplay_ws;
> +
> + /** Classifier destination CoS for
> resulting packets
> + 

Re: [lng-odp] [PATCHv2] linux-gen: scheduler: modular scheduler interface

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

> -Original Message-
> From: Brian Brooks [mailto:brian.bro...@arm.com]
> Sent: Wednesday, June 28, 2017 5:17 PM
> To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@nokia.com>
> Cc: Joyce Kong <joyce.k...@arm.com>; lng-odp@lists.linaro.org
> Subject: Re: [lng-odp] [PATCHv2] linux-gen: scheduler: modular scheduler
> interface
> 
> On 06/28 07:24:08, Savolainen, Petri (Nokia - FI/Espoo) wrote:
> >
> >
> > > -Original Message-
> > > From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of
> Joyce
> > > Kong
> > > Sent: Wednesday, June 28, 2017 5:14 AM
> > > To: lng-odp@lists.linaro.org
> > > Cc: Joyce Kong <joyce.k...@arm.com>
> > > Subject: [lng-odp] [PATCHv2] linux-gen: scheduler: modular scheduler
> > > interface
> > >
> > > The modular scheduler interface in odp_schedule_if.h includes
> functions
> > > from pktio and queue. It needs to be cleaned out. The pktio/queue
> related
> > > functions should be moved to pktio/queue internal header file.
> >
> > Sched_cb_xxx() functions are the interface from a scheduler towards
> other parts of the system. So, those calls are not in the scheduler
> interface file by mistake.
> 
> Generally speaking, function declarations should exist in the .h
> file of the .c file that contains the definitions. These functions
> are defined in queue.c and called from schedule.c. The declarations
> should be in queue.h. It can be as simple as that.

We try to enforce a tight scheduler interface (== currently odp_schedule_if.h). 
I decided to create only one file for simplicity. There could be additional 
files to define each output direction *interface* from a scheduler 
(queue_if_for_scheduler.h, pktio_if_for_scheduler.h, timer_if_for_scheduler.h) 
which includes only those functions that a scheduler may use. Moving things 
back to odp_xxx_internal.h would bring back the problem of clearly defining 
what functions (or types) a scheduler may use. Without clear interface file, 
each developer gets creative and access different parts of the system from 
different schedulers with different (e.g. locking) expectations ... and we are 
back in the original mess.


> 
> And, usually 'cb' or 'callback' is used in naming a function that is
> called through a function pointer. So, the naming is off here as well
> since these functions are always called via a direct function call.

We need common prefixes for input and output direction scheduler interface 
calls. I did pick up sched_cb_ to make distinction from sched_fn, which is the 
input direction. Those are just names. 

> 
> If you have to caution the user as to why something is written the
> way it is, and that it is not a mistake, it better be a juicy topic.
> Following best practices such as placing function declarations in
> the .h file of the .c file that they are defined in is not a juicy
> topic.


The patch claims to make the interface cleaner, but it does the opposite by 
moving interface functions back into internal header files. How do you define a 
clean interface, if the included file exposes e.g. pktio internals ? Juicy 
enough?


> 
> > If we now agree that queues and scheduler come as a package, tighter
> integration between queue.c and 3 original schedulers could be done. But
> preferably through another header file than odp_queue_internal.h, since
> that exposes internals of the queue block.
> >
> > This should *not* be done for pktio. It's the sched -> pktio interface
> as of today.
> >
> > I can look into this, since I'd inline as much as possible of those
> sched_cb_queue_xxx() functions anyway. Also this work should not affect
> the ARM scheduler, right?


I'm currently re-working ordered queue implementation so that last references 
to queue_entry_t disappear from the scheduler code. After that the interface 
gets cleaner also. Cleaning the interface needs more work than just moving 
function prototypes around.

-Petri

Re: [lng-odp] [PATCH API-NEXT v3 0/2] event subtype implementation

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

Reviewed-by: Petri Savolainen 


> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of
> Dmitry Eremin-Solenikov
> Sent: Friday, June 16, 2017 2:48 PM
> To: lng-odp@lists.linaro.org
> Subject: [lng-odp] [PATCH API-NEXT v3 0/2] event subtype implementation
> 
> Applies on top of [API-NEXT PATCH v2 0/3] IPSEC packet event
> 
> These patches provide event subtype implementation and tests. Ideally they
> should be merged together with event subtypes API definitions.
> 
> Changes since v2:
>  - Validate odp_event_types() implementation
>  - Validate CRYPTO_COMPL event type and subtype
> 
> Changes since v1:
>  - Implement odp_event_types()
>  - Update event subtype properly in odp_crypto.c
> 
> 
> Dmitry Eremin-Solenikov (2):
>   linux-generic: events subtype implementation
>   validation: test correctness of events subtype implementation
> 
>  .../linux-generic/include/odp_buffer_inlines.h |  2 +
>  .../linux-generic/include/odp_buffer_internal.h|  3 ++
>  platform/linux-generic/odp_crypto.c| 15 ---
>  platform/linux-generic/odp_event.c | 15 +++
>  platform/linux-generic/odp_packet.c|  1 +
>  platform/linux-generic/odp_pool.c  | 11 +
>  test/common_plat/validation/api/buffer/buffer.c| 50 -
> -
>  .../validation/api/crypto/odp_crypto_test_inp.c| 14 ++
>  test/common_plat/validation/api/packet/packet.c| 21 -
>  test/common_plat/validation/api/timer/timer.c  | 32 --
>  10 files changed, 140 insertions(+), 24 deletions(-)
> 
> --
> 2.11.0

Re: [lng-odp] [PATCH] linux-gen: time: use true hz

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

Reviewed-by: Petri Savolainen 

> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of Brian
> Brooks
> Sent: Monday, June 26, 2017 9:21 PM
> To: lng-odp@lists.linaro.org
> Subject: [lng-odp] [PATCH] linux-gen: time: use true hz
> 
> Use true hz value instead of dividing by 10.
> 
> The architected timer in ARM SoCs generally have a lower
> frequency than IA TSC. It is detrimental to divide the
> hertz by ten in this case. This is causing time validation
> failures on ARM systems where the architected timer runs
> at 50MHz.
> 
> Signed-off-by: Brian Brooks 
> ---
>  platform/linux-generic/odp_time.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/platform/linux-generic/odp_time.c b/platform/linux-
> generic/odp_time.c
> index 2bbe5666..a831cc51 100644
> --- a/platform/linux-generic/odp_time.c
> +++ b/platform/linux-generic/odp_time.c
> @@ -102,9 +102,7 @@ static inline odp_time_t time_hw_cur(void)
> 
>  static inline uint64_t time_hw_res(void)
>  {
> - /* Promise a bit lower resolution than average cycle counter
> -  * frequency */
> - return global.hw_freq_hz / 10;
> + return global.hw_freq_hz;
>  }
> 
>  static inline uint64_t time_hw_to_ns(odp_time_t time)
> --
> 2.13.1

Re: [lng-odp] regarding ODP_PKTIO_MACADDR_MAXSIZE in pktio spec

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

> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of
> shally verma
> Sent: Wednesday, June 28, 2017 1:23 PM
> To: lng-odp-forward 
> Subject: [lng-odp] regarding ODP_PKTIO_MACADDR_MAXSIZE in pktio spec
> 
> I have question on definition of this macro  ODP_PKTIO_MACADDR_MAXSIZE
> . In pktio,h it is mentioned as " Minimum size of output buffer for
> odp_pktio_mac_addr()" where as Macro name is MAXSIZE.
> 
> In linux-generic implementation this value is set to 16.
> 
> So how to interpret it? Does it mean user need to pass buffer minimum
> of this much length ?  i.e. size parameter value in
> odp_pktio_mac_addr() >= ODP_PKTIO_MACADDR_MAXSIZE? else API should
> fail?
> 
> int odp_pktio_mac_addr(odp_pktio_t pktio, void *mac_addr, int size);
> 
> Thanks
> Shally

/**
 * Get the default MAC address of a packet IO interface.
 *
 * @param   pktio Packet IO handle
 * @param[out]  mac_addr  Output buffer (use ODP_PKTIO_MACADDR_MAXSIZE)
 * @param   size  Size of output buffer
 *
 * @return Number of bytes written (actual size of MAC address)
 * @retval <0 on failure
 */
int odp_pktio_mac_addr(odp_pktio_t pktio, void *mac_addr, int size);

Use ODP_PKTIO_MACADDR_MAXSIZE size buffer for mac_addr, if unsure how much the 
function will output. Give buffer size as 'size'. Function returns how many 
bytes were written. I think it should fail and return <0, if 'size' is smaller 
MAC address size.

-Petri

Re: [lng-odp] [PATCHv2] linux-gen: scheduler: modular scheduler interface

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

> -Original Message-
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of Joyce
> Kong
> Sent: Wednesday, June 28, 2017 5:14 AM
> To: lng-odp@lists.linaro.org
> Cc: Joyce Kong 
> Subject: [lng-odp] [PATCHv2] linux-gen: scheduler: modular scheduler
> interface
> 
> The modular scheduler interface in odp_schedule_if.h includes functions
> from pktio and queue. It needs to be cleaned out. The pktio/queue related
> functions should be moved to pktio/queue internal header file.

Sched_cb_xxx() functions are the interface from a scheduler towards other parts 
of the system. So, those calls are not in the scheduler  interface file by 
mistake. If we now agree that queues and scheduler come as a package, tighter 
integration between queue.c and 3 original schedulers could be done. But 
preferably through another header file than odp_queue_internal.h, since that 
exposes internals of the queue block.

This should *not* be done for pktio. It's the sched -> pktio interface as of 
today.

I can look into this, since I'd inline as much as possible of those 
sched_cb_queue_xxx() functions anyway. Also this work should not affect the ARM 
scheduler, right?

-Petri

 

Re: [lng-odp] [API-NEXT, PATCH v4 1/1] comp: compression API

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

> -Original Message-
> From: shally verma [mailto:shallyvermacav...@gmail.com]
> Sent: Tuesday, June 27, 2017 12:31 PM
> To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@nokia.com>
> Cc: lng-odp-forward <lng-odp@lists.linaro.org>; Mahipal Challa
> <mcha...@cavium.com>; pathr...@cavium.com; Shally Verma
> <sve...@cavium.com>
> Subject: Re: [lng-odp] [API-NEXT, PATCH v4 1/1] comp: compression API
> 
> On Tue, Jun 27, 2017 at 2:51 PM, Savolainen, Petri (Nokia - FI/Espoo)
> <petri.savolai...@nokia.com> wrote:
> >> > diff --git a/platform/linux-
> generic/include/odp/api/plat/event_types.h
> >> b/platform/linux-generic/include/odp/api/plat/event_types.h
> >> > index 0f51783..adf2c31 100644
> >> > --- a/platform/linux-generic/include/odp/api/plat/event_types.h
> >> > +++ b/platform/linux-generic/include/odp/api/plat/event_types.h
> >> > @@ -4,7 +4,6 @@
> >> >   * SPDX-License-Identifier: BSD-3-Clause
> >> >   */
> >> >
> >> > -
> >> >  /**
> >> >   * @file
> >> >   *
> >> > @@ -39,7 +38,9 @@ typedef enum odp_event_type_t {
> >> > ODP_EVENT_PACKET   = 2,
> >> > ODP_EVENT_TIMEOUT  = 3,
> >> > ODP_EVENT_CRYPTO_COMPL = 4,
> >> > -   ODP_EVENT_IPSEC_RESULT = 5
> >> > +   ODP_EVENT_IPSEC_RESULT = 5,
> >> > +   ODP_EVENT_COMP_COMPL   = 6,
> >> > +   ODP_EVENT_DECOMP_COMPL = 7
> >> >  } odp_event_type_t;
> >
> > Event subtypes are now in api-next. This needs to be rebased on top of
> that. Part of the rebase is definition of results as ODP_EVENT_PACKET type
> + ODP_EVENT_PACKET_COMP subtype (not as COMP_COMPL events). Only one
> subtype for comp, the same way as IPsec has ODP_EVENT_PACKET_IPSEC subtype
> which is for both in- and outbound IPsec. It would be preferable to align
> comp API to IPsec API also in other parts.
> 
> So you mean if application has both compression and decompression
> ongoing simultaneously, onus is on application to track whether event
> is from compress_enq() or decomp_enq() call ?
>

Application may use e.g. different destination queues for comp and decomp 
events. Also events may have metadata (session handle, direction enum, user 
written context, etc) which tells which direction results has been received. 
There are many options, but only single event subtype should be reserved, 
otherwise application's switch-case gets large (if ipsec/crypto/comp/... define 
multiple types).

-Petri