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

2017-10-17 Thread 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)

2017-10-17 Thread Bill Fischofer 
On Tue, Oct 17, 2017 at 2:14 AM, Savolainen, Petri (Nokia - FI/Espoo)
 wrote:
>
>
> From: Bill Fischofer [mailto:notificati...@github.com]
> Sent: Tuesday, October 17, 2017 2:20 AM
> To: Linaro/odp 
> Cc: Savolainen, Petri (Nokia - FI/Espoo) ; Author 
> 
> 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, ...

I did a reply-all to the email sent to the ODP mailing list as the
result of the PR. I suspect the issue is that the bot address is
included in the reply-all. Expecting people to edit a reply-all
address list isn't very user friendly.

>
> -Petri
>

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

2017-10-17 Thread Bill Fischofer 
On Tue, Oct 17, 2017 at 3:04 AM, Savolainen, Petri (Nokia - FI/Espoo)
 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.

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

2017-10-17 Thread 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)

2017-10-17 Thread Savolainen, Petri (Nokia - FI/Espoo) 


From: muvarov [mailto:notificati...@github.com] 
Sent: Tuesday, October 17, 2017 3:01 AM
To: Linaro/odp 
Cc: Savolainen, Petri (Nokia - FI/Espoo) ; Author 

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)

2017-10-17 Thread 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 
Cc: Savolainen, Petri (Nokia - FI/Espoo) ; Author 

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 thought these were supposed to be optimization hints, not
requirements. It's OK for the application to advise the implementation
about how it expects to make use of pools. It seems contrary to the
spirit of ODP to make demands on how ODP implementations must organize
pools internally. So, for example, the only thing we require is that
the pool

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

2017-10-17 Thread Savolainen, Petri (Nokia - FI/Espoo) 


From: Bill Fischofer [mailto:notificati...@github.com] 
Sent: Tuesday, October 17, 2017 2:20 AM
To: Linaro/odp 
Cc: Savolainen, Petri (Nokia - FI/Espoo) ; Author 

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